On Wed, Jan 3, 2018 at 4: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. > > 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. > > 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. > In the end, this is what it comes down to for me. The end-user perspective is the most important perspective. > > > > > > > > > > > > > > Relevant endpoint 101 configuration values: > > > > > > > > [global] > > > > ; The order by which endpoint identifier methods are given priority. > > > > ; The endpoint_identifier_order default is > > > > ;endpoint_identifier_order=ip,username,anonymous > > > > > > > > [101] > > > > type=endpoint > > > > ; The identify_by default is > > > > ;identify_by=username > > > > > > > > [identify_me] > > > > type=identify > > > > endpoint=101 > > > > ; Match by some IP address > > > > match=127.0.0.1/24 > > > > > > > > Before the patch, the 101 endpoint above would identify by either the > > > > username endpoint identifier method or the > > > > res_pjsip_endpoint_identifier_ip endpoint identifier method's > > > > [identify_me] section. > > > > > > > > After the patch, the 101 endpoint is still matched by either method. > > > > However, the identify_by default had to change to "username,ip" and > the > > > > option meaning had to change slightly do it. I think this is > wrong. If > > > > you went to the trouble to create an [identify_me] section which must > > > > explicitly specify the endpoint it matches then the endpoint should > not > > > > need to also specify in the identify_by value that it is identified > by > > > the > > > > res_pjsip_endpoint_identifier_ip method. Doing so is redundant and > will > > > > cause unnecessary configuration errors. > > > > > > I think removing the redundant requirement is a nice improvement for > the > > > future, but that comes at a cost to do it in a way that is user > friendly > > > and automatic. > > > > > > > The goal of ASTERISK-27206 is to prevent the endpoint from being > > > > identified by the username identification method so it could only be > > > > identified by the res_pjsip_endpoint_identifier_ip method. The key > > > > difference between the two methods is the username identification > method > > > > has no other configuration than the endpoint's identify_by value > > > available > > > > to specify to which endpoint the method applies. > > > > > > > > I think the previous and current identify_by documentation is a bit > > > > misleading in any case. The identify_by option historically > specified > > > > which identification methods that have no other configuration > > > requirements > > > > can match the endpoint. i.e., The username and auth_username > > > > identification methods. To satisfy ASTERISK-27206, what is needed > for > > > the > > > > identify_by option is a value to prevent methods that have no other > > > > configuration requirements from matching the endpoint. > > > > > > The identify_by option was originally an option which listed the > endpoint > > > identifiers that an endpoint could be identified by. That was its > original > > > goal. It later evolved slightly with the addition of auth_username to > also > > > influence how. The addition of "ip" goes back to its original goal. > Over > > > all the option has muddied meaning. > > > > > > > Listing how an endpoint is to be identified may have been the identify_by > > option's original goal. That is not how it was coded to behave by the > > original v12.0.0 release. See > > https://issues.asterisk.org/jira/browse/ASTERISK-22135 > > (895c8e0d2c97cd04299f3f179e99d8a3873c06c6) which removed the identify_by > > "location" value in 2013 apparently because it wasn't used anywhere. The > > ASTERISK-22135 patch "redefined" identify_by to how I've described it > > historically behaves above. > > > > Restoring the original definition and actually implementing that behavior > > correctly is the huge scope creep we wish to avoid at this time. > Partially > > implementing it as the ASTERISK-27206 patch attempts will require changes > > to res_pjsip to add a new widgit identification method to identify_by. > In > > fact https://issues.asterisk.org/jira/browse/ASTERISK-27491 would need > to > > do just that if the original definition is "restored" by the > ASTERISK-27206 > > patch. That was something I know we tried to avoid in the beginning. We > > were attempting to add or extend functionality by just loading modules. > > It is only through actual usage that we find where simply loading a module > to extend functionality works and where things need to further adapt. > > > > > > > > > > > I disagree that whether an endpoint identifier is configurable or not > has > > > a bearing on its naming or meaning in identify_by. The configuration > may > > > imply that it should only be matched using that identifier, but that > is not > > > currently something that is done and is of a greater scope. I think > having > > > a global identify_by option which could actually mean multiple endpoint > > > identifiers by itself is also confusing and hard to define the > behavior of. > > > > > > > Agree or not, that is how it was coded when v12 was originally > released. I > > am not expanding the scope. I am preserving how the option actually > > behaves; not redefining it. > > > > What global identify_by option? I did not mention such an option. The > > identify_by option is per endpoint as it is part of the endpoint > definition > > config section. > > I meant an identify_by option which covers multiple endpoint identifiers, > like your "none" or "other". > > > > > > > > > > > Really what this just illustrates is that the option itself has evolved > > > and hasn't been clearly defined with a strict purpose. I don't think > now is > > > the right time to solve that particular problem. If there is a defined > use > > > case which is currently broken by the change though, that is something > we > > > need to fix. > > > > > > > The ASTERISK-27206 patch incompletely attempts to restore the originally > > intended option behavior and actually is not needed to fix the issue. > > -- > 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
