On Wed, Jan 3, 2018 at 1:04 PM, Joshua Colp <[email protected]> wrote:
> On Wed, Jan 3, 2018, at 3:53 PM, Richard Mudgett wrote: > > On Wed, Jan 3, 2018 at 5:08 AM, Joshua Colp <[email protected]> wrote: > > > > > On Wed, Jan 3, 2018, at 12:03 AM, Richard Mudgett wrote: > > > > On Tue, Jan 2, 2018 at 5:41 PM, Joshua Colp <[email protected]> > wrote: > > > > > > > > > On Tue, Jan 2, 2018, at 7:14 PM, Richard Mudgett wrote: > > > > > > The patch for https://issues.asterisk.org/ > jira/browse/ASTERISK-27206 > > > > > which > > > > > > is committed in 7385d1e017e562afe64431606e857e704f86a16d causes > a > > > > > > configuration regression by requiring the endpoint and identifier > > > method > > > > > > to agree to match the endpoint. Doing so is redundant for > methods > > > that > > > > > > explicitly specify which endpoints they match. The redundancy > allows > > > > > > configuration errors that cannot be caught when the > configuration is > > > > > > loaded. > > > > > > > > > > Can you clarify what the precise regression you are referring to > is? > > > Even > > > > > after reading this email I'm still unclear. > > > > > > > > > > > > > The regression is the new requirement for the endpoint identify_by > option > > > > to list ip in order for > > > > the type=identify method to be accepted by the endpoint. This new > > > > requirement is unnecessary > > > > as the type=identify section must specify an endpoint name to know > which > > > > endpoint it recognizes. > > > > > > > > More specifically, the change to > > > > res/res_pjsip_endpoint_identifier_ip.c:ip_identify() enforces this > > > > new requirement. > > > > > > > > I should have used the term complexity instead of redundancy. > > > > > > > > The new requirement adds configuration complexity because the > endpoint > > > > identify_by option and > > > > the type=identify method must agree to match the endpoint. The added > > > > complexity doesn't add any > > > > value, can needlessly break existing installations regardless of the > note > > > > in CHANGES saying it will, > > > > makes match by ip configuration more error prone, and the error is > not > > > > recognizable on configuration > > > > load. > > > > > > > > In addition, as I pointed out, I think the entire ASTERISK-27206 > patch > > > was > > > > unnecessary. Before > > > > the patch, setting the identify_by option to an empty string would > > > disable > > > > the username > > > > identification method from matching the endpoint. > > > > > > I disagree that those are a regression. Those are certainly valid > findings > > > and comments, but I don't see a specific regression based on what > you've > > > said. If you are trying to state that the regression is "we now > require ip > > > to be listed in identify_by in order to match using the IP endpoint > > > identifier" then that was the purpose of the patch itself and is not a > > > regression. I also don't think that the option to set it to an empty > string > > > would have been user friendly, that's more of a side effect of how it > was > > > coded. Having a specific option for IP only based matching is explicit. > > > > > > > I accept that defining the identify_by option to list the methods the > > endpoint should be identified by is easy to document and conceptually > > simple. To comply with that definition, for now, we can just add the > "ip" > > value, the "header" value, and any other widget name down the road as > > needed. Later we need to re-implement it to support dynamically added > > widget names. I do not accept the unnecessary requirement that the > > type=identify method must be listed in the identify_by value when the > user > > must explicitly specify the endpoint name for the type=identify method to > > even work. The "ip" value in identify_by should just be documentation in > > this case. Adding a needless source of configuration error by requiring > > the "ip" value to be listed does not help the user experience at all. > That > > requirement is the regression. > > Yes, I think having it be dynamic based on the registration of the > endpoint identifier would be a nice thing to have long term. I continue to > disagree that the requirement is a regression. It's a behavior change and > generally the PJSIP configuration is driven around being explicit in things. > +1 > > > The ASTERISK-27206 reporter's problem was to somehow stop the username > > identification method from recognizing the endpoint. Setting the > > identify_by value to something that does not include "username" is all > that > > is necessary. Adding the requirement that the type=identify method be > > listed in the identify_by value to accept a match from the type=identify > > method goes too far. It goes into the negative user experience realm. > > The fact that setting it to "something" that does not include username is > an implementation detail. I disagree that it's a negative user experience, > it's an explicit decision which is easily documented and seen by anyone > looking at the configuration. > +2 > > > > > > > > > > Having a non-explicit option like you mentioned previously for > identify_by > > > becomes a free for all on the other endpoint identifiers. If we > encounter a > > > scenario where two "other" endpoint identifiers could match but only > want > > > to match one then boom - we're back to the same scenario. You've just > > > enforced that two endpoint identifiers that are configurable in some > way > > > can never match the same request even if for some reason they did if > they > > > fall under the "other" option. If the answer is "don't configure things > > > them that way" then perhaps it isn't possible, we don't know. As well > even > > > with your "none" or "other" you'd still need to list it in the > identify_by > > > list, so I don't see how that's different than requiring "ip" to be > there > > > except being less specific. > > > > > > > The proposed "other" values are not a free for all and we are not back to > > the same scenario. It is already known which identifiers do and do not > > need listing in the identify_by value. > > > > * Adding a new identifier like "my_username" that does not have > > configuration to explicitly identify a specific endpoint has to be listed > > in the identify_by value. Otherwise, there is no way to restrict which > > endpoints it identifies. i.e., Which endpoint the "my_username" > identifier > > matches is vague. > > > > * Adding a new identifier like "my_ip" that does have configuration to > > explicitly identify a specific endpoint does not have to be listed in the > > identify_by value. Listing "my_ip" in the identify_by value would just > be > > for documentation purposes. i.e., Which endpoint the "my_ip" identifier > > matches is specific. It can match one and only one endpoint. > > > > The "", "none", or "other" proposed value is a place holder which means > not > > "username", "auth_username", or any other "my_username" type identifier. > > The proposed value is only needed if you explicitly did not want to > > identify the endpoint by "username", "auth_username", or any other > > "my_username" type identifier. i.e., The "", "none", or "other" value > > would be the only thing in the list. The "" value actually makes that > > point more explicit. > > This makes things even more complicated to comprehend and explain to > people. > +5 > > > We currently can have many type=identifier identifiers match endpoint > foo. > > There is no restriction on that. However, doing so does point out a > > limitation in > > res/res_pjsip_endpoint_identifier.c:format_ami_endpoint_identify() which > > only handles the first type=identifier identifying endpoint foo. > > > > > > > > > > Approaching this from an end-user perspective who knows NOTHING of this > > > stuff having "ip" in the identify_by also makes logical sense. It's > > > explicit, you can understand what it means and you can understand what > it > > > does. The use of "other" is vague and requires more thought and > > > investigation. The use of "none" is just downright confusing at first > > > glance unless you dive deeper into things. > > > > > > > If you have preconceived notions of what the option means and the > > documentation does not disagree then yes it is easier to understand. > > However, until it supports dynamic widget names, that identify_by > > definition requires a third party creating their own identifier module to > > also modify Asterisk modules. > > I think long term removing that requirement is a nice thing, but since we > have created the PJSIP support no other individual has contributed an > endpoint identifier module so I don't see it as a pressing thing or > something that is a hindrance. > +8 > > -- > Joshua Colp > Digium, Inc. | Senior Software Developer > 445 Jan Davis Drive NW - Huntsville, AL 35806 - US > Check us out at: www.digium.com & www.asterisk.org > > -- > _____________________________________________________________________ > -- Bandwidth and Colocation Provided by http://www.api-digital.com -- > > asterisk-dev mailing list > To UNSUBSCRIBE or update options visit: > http://lists.digium.com/mailman/listinfo/asterisk-dev > -- George Joseph Digium, Inc. | Software Developer 445 Jan Davis Drive NW - Huntsville, AL 35806 - US Check us out at: www.digium.com & www.asterisk.org
-- _____________________________________________________________________ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
