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

Wed Jan 3 14:13:35 CST 2018

On Wed, Jan 3, 2018 at 1: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.
>

+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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20180103/27f31825/attachment-0001.html>