Bugzilla – Bug 1269
inconsistent use of built-in local zones with views
Last modified: 2017-05-31 19:16:58 CEST
I've noticed some counter-intuitive behavior in the following configuration (it's the case as of svn trunk revision 4190): server: [...] local-zone: a.onion redirect local-data: 'a.onion A 192.0.2.1' access-control-view: 127.0.0.1 view4 access-control-view: ::1 view6 view: name: view4 view-first: yes view: name: view6 view-first: yes local-zone: example.com always_nxdomain That is, there's a global local-zone to override a built-in local-zone "onion." (Note: it's the same for other built-in local-zones such as "localhost.") Also, 2 views are defined, one with a view-specific local-zone and the other without any view-specific local-zone. If 'view4' is first consulted for a.onion/A, no view-specific local-zone is applied and the overriding global local-zone is used. So the answer will be '192.0.2.1'. If 'view6' is first consulted for a.onion/A, a built-in view-specific local-zone for "onion." is applied and the answer will be NXDOMAIN. The difference between these two cases seem to come from the fact that view-specific built-in local-zones are not configured unless there is at least one user-defined local-zone or local-data for the view. This might be called a "feature", but I think this is quite counter-intuitive from the user's point of view, since neither view defines any explicit local zone for "onion.". If the difference is accidental rather than intentional, I can think of two alternatives: 1. always define built-in local-zones regardless of whether there's user-defined local-zone/data for the view 2. if view-first is yes, do not define built-in local-zones for any view. if view-first is no, always define built-in local-zones for the view like option 1. Either way the behavior will be more "consistent". I also personally think option 2 will be more intuitive for users since any overriding by a global local-zone/data will work naturally (and it's also a bit more memory efficient if a huge number of views are defined with view-first being yes). If, on the other hand, the "inconsistent" behavior is intentional for a reason, I think it should at least be explicitly documented somewhere.
Hi Jinmei, Thank you for this bug report. Although intentional, I do recognize that this behavior can be counter-intuitive. It is indeed not necessary to have the defaults in the view specific local-zone, as long as the global local-zone is used. This happens when view-first is "yes", or when the view does not have a local zone. Therefore, the default local-zones in the view are only needed when there is a local-zone and view-first is set to "no". I just committed code to change this behavior. I also made some changes so that local-zones with type nodefault will be added to the view local zone, when there are no defaults in the view-specific local-zone. This way it is still be possible to exclude a default for a specific view. Besides that, I made a change that the defaults are added to a view local-zone, when that local-zone creation is triggered by an unbound-control command, and view-first is "no". Regards, -- Ralph
Thanks for the response and for the update. As of revision 4199 the behavior for 'view-first=yes' looks reasonable to me. I've noticed it might still be confusing when view-first is no. If you define the two views in the original problem description with view-first being no: view: name: view4 view-first: no view: name: view6 view-first: no local-zone: example.com always_nxdomain The default zones are used for 'view6' (because it has a user-defined local-zone) but not for 'view4'. I probably don't need to set view-first to no, so that's not a big deal for my own purposes, but I guess this can confuse users who need to have views with view-first=no. If the behavior is intentional you'll probably want to document it somewhere. I also noticed one minor possible glitch in the code. In services/view.c:views_apply_cfg(): cfg_str2list_insert(&lz_cfg.local_zones, strdup(nd->str), strdup("nodefault")); Failure of strdup() calls and of cfg_str2list_insert() is ignored. It may be intentional, but even if so, if one call of strdup() succeeds but the other fails the copied string will leak. You'll probably at least want to prevent the leak.
Hi Jinmei, You are right, this is not correct behaviour. The issue is in the logic on when to use the global local-zones. I think these should also be used when view-first is no, but there is no local-zone in the view. This is in my opinion the correct behaviour because we like to add non local-zone related options to views in the future. The view should then only override the specified options, not all possible view option. This was also what I had in mind while making last changes, hence the unclear behaviour. Do you agree? I just committed the code for this: Index: services/localzone.c =================================================================== --- services/localzone.c (revision 4200) +++ services/localzone.c (working copy) @@ -1590,7 +1590,7 @@ lock_rw_rdlock(&z->lock); lzt = z->type; } - if(!z && !view->isfirst){ + if(view->local_zones && !z && !view->isfirst){ lock_rw_unlock(&view->lock); return 0; } You are right that whatever behaviour we decide on, it has to be documented. I added some text to the unbound.conf man page. Thanks for the remark about checking the strdup() return value. This can indeed result in a memory leak. Fixed. Regards, -- Ralph
Hi Ralph, > This was also what I had in mind while making last changes, hence the > unclear behaviour. Do you agree? Okay, I now see the intent. I suspect 'view-first' might become harder to understand and use with that semantics, but I don't have a strong opinion on the decision itself. So, yes, I do agree in that sense. I also confirmed the leak has been fixed. Please feel free to close this report.