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