[asterisk-dev] Problems with the ASTERISK-27206 patch.

[Matt Fredrickson]

On Wed, Jan 3, 2018 at 2:04 PM, Joshua Colp <jcolp at digium.com> wrote:
> On Wed, Jan 3, 2018, at 3:53 PM, Richard Mudgett wrote:
>> On Wed, Jan 3, 2018 at 5:08 AM, Joshua Colp <jcolp at digium.com> wrote:
>>
>> > On Wed, Jan 3, 2018, at 12:03 AM, Richard Mudgett wrote:
>> > > On Tue, Jan 2, 2018 at 5:41 PM, Joshua Colp <jcolp at digium.com> 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.
>
>> 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.
>
>> >
>> >
>> > 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.
>
>> 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.

I'm thinking that since this doesn't seem to be a true regression but
more of a question of behavior, it shouldn't be treated as regression
worthy and hold up any releases.  We can however take some of
Richard's good suggestions and implement them further down the road as
improvements.

-- 
Matthew Fredrickson
Digium, Inc. | Engineering Manager
445 Jan Davis Drive NW - Huntsville, AL 35806 - USA