Feature issue5 - new set of templates, strict authoritative NS #6
Conversation
README.md
Outdated
| ```yaml | ||
| bind9_templates: strict_authoritative/ | ||
| ``` | ||
| Note that the same variable of the role may have different meanings, or no meaning at all, depending on the choosen set of templates. |
There was a problem hiding this comment.
In this section it is necessary to specifically clarify that if you choose your own set of templates or strict_authoritative, bind9_authoritative has no meaning at all.
En esta sección, merece especificamente aclarar que si elegimos nuestro propio conjunto de templates o las strict_authortitave, la variable bind9_authoritative queda sin efecto.
There was a problem hiding this comment.
Absolutely right. The new set of templates is not yet documented. I only documented the minimum to understand how could work a version of the role with several sets of templates.
| {% elif zone_type == 'slave' %} | ||
| file "/var/lib/bind/db.{{ zone.name }}"; | ||
| {% if zone.masters | default() or bind9_masters | default() %} | ||
| notify {{ zone.notify | default ('no') | ternary ('yes','no') }}; |
There was a problem hiding this comment.
Why we have an notify no; in the default template file of this role?
There was a problem hiding this comment.
(BUG) Correct this line to notify {{ zone.notify | default (true) | ternary ('yes','no') }};
There was a problem hiding this comment.
notify yes; means the primary notifies all secondaries explicitly defined as NS servers. It's an exceptional behavior that a secondary sever has to notify another. Here, we are talking about zones the server secondary or slave for. I think default behavior for notifications should be no.
There was a problem hiding this comment.
Ok, I agree. Just set notify no; by default. However, this line doesn't work correctly. If you look at our test nameserver you will see a notifiy yes; for every zone the server is slave for (and we didn't set no zone.notify at all). That's because this line have a little problem, so now I propose you to change it with this:
notify {{ zone.notify | default (false) | ternary ('yes','no') }};
There was a problem hiding this comment.
I also think this line should replace the notify no; of this other line.
README.md
Outdated
|
|
||
| ## Role varibles | ||
| Basically the role builds bind9 configuration, i.e. `/etc/bind/named.conf,*` files, as well aa zone definition files, whicha are placed in `/etc/bind/zones/` directory. |
There was a problem hiding this comment.
Ok. README still needs work.
There was a problem hiding this comment.
I mark this unresolved 'cause these typos still here.
| {% elif zone.notify|default(true) %} | ||
| notify yes; | ||
| {% elif zone.notify | default(true) %} | ||
| notify {{ zone.notify | default(true) | ternary ('yes','no') }}; |
There was a problem hiding this comment.
I'm not sure about the order filters are applied: What about if zone.notify is set to explicit? I guess the value will fall to yes. the ternary should apply only if something clearly true or false.
There was a problem hiding this comment.
In any case, before proposing the PR I will bring back the default templates to their previous content.
| {% endfor %} | ||
| }; | ||
| {% endif %} | ||
| {% if bind9_notify_explicit %} |
There was a problem hiding this comment.
bind9_notify_explicit is a boolean that sets a forced value, not a default value as many other global variables. It must be documented for the default template. I don't understand the reason of this behavior, so in other templates we may change it, to manage a default value and not a forced value, with a bind9_notify, eventually distinguishing masters and slaves.
ulvida
force-pushed
the
feature-issue5
branch
3 times, most recently
from
9c3e24a to
d84a739
Compare
| {% set zone_allow_transfer = zone.allow_transfer | default ( bind9_allow_transfer | default( ( zone_calc_allow_transfer ) | unique ) ) %} | ||
| {% if zone_allow_transfer | length > 0 %} | ||
|
|
||
| // allow transfer from secundaries, also-notify hosts and othe allow-transfer specified |
| }; | ||
| {% endif %} | ||
| {% if zone.notify is defined %} | ||
| notify {{ zone.notify }}; |
There was a problem hiding this comment.
A filter | ternary ('yes','no') is missing.
There was a problem hiding this comment.
I decided to eliminate this filter because notify can have other values: explicit and master-only. It has the downgrade that it avoids true and false values for the role and that you have to be precise in the value, but it's simpler and complete. Documentation is updated accordingly.
There was a problem hiding this comment.
Default templates use this filter and manage separately (and strangely) explicit value with code, but not master-only that may be interesting to avoid useless notification noise. For new templates I preferred simpler code, with exact values for the clause.
This PR codes #5 proposed features with a new set of templates.
We are instructing this PR in our fork before deploying the role on our legacy hand crafted NS servers, and then put the PR to the upstream role.