Feature new templates set #54

ulvida · 2022-07-02T00:37:09Z

Hello, hereafter we propose you a PR we already talked you about. It consists essentially in a new set of templates for stronger, strict authoritative nameservers.

We explored the possibility to include the new features in the default template set (and we indeed included some enhancements for these templates, but it is secondary), finally it was more productive to develop a new set where we try to follow another design than the default ones.

The new set of templates is quite extensively documented, and in the readme we will also see that it led us to develop a sister role that may interestingly enhance it.

In fact, considering coding only, it also deserves to merge the two roles in a single one, with a refactor of the code and the design of a new version of the API of the role. But I think it isn't good to do that without discussing it with you and preparing it for the community of users of the role.

The quite barroque solution proposed: ansible code that writes locally ansible configuration should allow to explore the possibilities without loosing the present role and its simple evolutions.

Closes #38, as this PR is a proposal of its implementation.

ulvida · 2022-07-02T00:39:01Z

Note: I'm doing this PR from a personal fork, because I had a very strange problem with our University's fork.

xshadow · 2022-07-04T14:32:04Z

Can you rebase the branch on the current main branch? The molecule/docker directory, is not necessary anymore.

xshadow · 2022-07-04T14:38:22Z

defaults/main.yml

-bind9_authoritative: no
+# variable by default templates and as conditionnal of several tasks
+# If using `strict_authoritative/` templates, this variable _must_ be true
+bind9_authoritative: "{{ true if bind9_templates == 'strict_authoritative/' else false }}"


I think yes or no would be better the better choice here since all other variables are also yes or no.

With this conditional default definition I want to achieve two goals:

preserve the default value no (or false, better to avoid warning in lint) for the default templates, avoiding breaking behavior in other deployments of the role,

set the value to yes when selecting strict_authoritative templates, as it is always needed for the role's tasks logic to configure zones.

xshadow · 2022-07-04T14:39:25Z

defaults/main.yml

@@ -32,15 +35,23 @@ bind9_hidden_master: no
 # Necessary to keep traffic between nameservers in private network.
 bind9_notify_explicit: no

+# bind9_notify: '{{ "explicit" if bind9_notify_explicit else undef }}'
+# undef doesn't work here. f**k legacy bind9_notify_explicit variable?
+


I think this comment not necessary to merge, since it's not used

I agree. And it's not very nice for the author of this variable... Sorry.I was supposed to be development temporary notes.

xshadow · 2022-07-04T14:39:35Z

defaults/main.yml

 # Default zone type
 bind9_zone_type: master

+## //!\\ Several of the following variables have different meanings or (no meaning at all) depending on the templates' set you use
+## See here after bind9_template variable.
+


I think this comment not necessary to merge, since it's not used

xshadow · 2022-07-04T14:39:43Z

defaults/main.yml

 # bind9_masters:
 #  - name: ns-primary
 #    addresses:
 #      - 1.2.3.4
+# For BIND 9.17.3 (not yet in debian): https://downloads.isc.org/isc/bind9/9.17.3/doc/arm/html/notes.html#feature-changes
+# Let's progressively rename this variable with bind's preferred terminology:
+# bind9_primaries: "{{ bind9_masters }}"


I think this comment not necessary to merge, since it's not used

xshadow · 2022-07-04T14:40:00Z

defaults/main.yml

+# `allow-transfer` may include `acl` lists but not `masters` ones. In YAML role's variables structures are identical, but
+# if they appear in BIND configuration list inclusions it will fail.
+# Practically: if you use `masters` lists (defined with `bind9_masters`or `bind9_masters_extra` variables of this role),
+# yo must re-define separately `bind9_also_allow_transfer`, probably defining an ACL with same values than master lists. 


I think this comment not necessary to merge, since it's not used

Now, it is indeed redundant with the README documentation, but this part is quite important to undertand why in the role you can mix references to masters lists and acls, while in BIND9 configuration you can't.
I think it deserves to be recalled in this file, but as you wish.

xshadow · 2022-07-04T14:41:21Z

defaults/main.yml

+#   It's files live in {{ role_path }}/templates/strict_authoritative/ directory
+#   Note that several default variables `bind9_*` have different meanings than with default templates' set.
+# bind9_templates nust be set as a relative or absolute directory, including it's trailing "/":
+# bind9_templates: strict_authoritative/


I think comment should go above bind9_templates value :)

xshadow · 2022-07-04T14:42:34Z

meta/main.yml

@@ -5,7 +5,7 @@ galaxy_info:
  description: Role to install and maintain the Bind9 nameserver on Debian
  company: systemli.org
  license: GPLv3
-  min_ansible_version: "2.4"
+  min_ansible_version: '2.10'


Why is this version bump necessary ?

xshadow · 2022-07-04T14:42:52Z

meta/main.yml

@@ -16,3 +16,5 @@ galaxy_info:
      versions:
        - bullseye
        - buster
+  notifications:


The notifications are also not necessary anymore

xshadow · 2022-07-04T14:51:35Z

templates/bind/named.conf.local.j2

 {%   for master in bind9_masters %}
-masters {{ master.name }} {
+masters "{{ master.name }}" {


Is this change necessary? In strict_authoritative templates it's without ""

xshadow · 2022-07-04T14:59:22Z

Thanks for the PR.

In general I was wondering how you would like to use the variables , if there are commented.

Wouldn't it make sense to leave them in and rename the variables depending on which templates they are used. So all variables which go into the strict_authoritative templates are preseed with bind9_strict_authoritative_ ...

ulvida · 2022-07-05T22:08:59Z

Thanks for the PR.
thanks for your role!

In general I was wondering how you would like to use the variables , if there are commented.
I wondered too. I tried to keep it simple and maintain the logic. But in this path led me to the idea that it may be worthy to re-design a bit this variables' set, for the whole role.

Wouldn't it make sense to leave them in and rename the variables depending on which templates they are used. So all variables which go into the strict_authoritative templates are preseed with bind9_strict_authoritative_ ...

It's an interesting proposal, and could be for the variables which are specific to this set of templates. But for some variables it won't work, for example for bind9_authoritative, which is needed true for strict_authoritative templates, but not for the templates themselves, but for the logic of role.

However, for me, a better goal would be to consider this proposal:

... it led us to develop a sister role that may interestingly enhance it.

In fact, considering coding only, it also deserves to merge the two roles in a single one, with a refactor of the code and the design of a new version of the API of the role. But I think it isn't good to do that without discussing it with you and preparing it for the community of users of the role.

If you consider this can be a common path, I would be glad to propose the merge of the two role. If not, I'm always concerned about micro-communities an their yet-another-abandonware, but maybe I work it out even.

ulvida · 2022-07-06T12:26:33Z

Sorry I started a new review, inestead of answering yours.

ulvida added 2 commits July 1, 2022 20:31

copied from coorupted udelarinterior.bind9 repo, branch feature-issue5

08479e2

Documentation and link to sister role udelarinterior.bind9_cluster

f1c0aff

ulvida mentioned this pull request Jul 2, 2022

Initial commit hash? #53

Closed

fixing typos and anslible-lint errors

1e1d0c5

ulvida force-pushed the feature-new-templates-set branch from c1426d3 to 1e1d0c5 Compare July 3, 2022 13:30

xshadow reviewed Jul 4, 2022

View reviewed changes

following @xshadow review

a34d679

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature new templates set #54

Feature new templates set #54

ulvida commented Jul 2, 2022 •

edited

Loading

ulvida commented Jul 2, 2022

xshadow commented Jul 4, 2022

xshadow Jul 4, 2022

ulvida Jul 4, 2022

xshadow Jul 4, 2022

ulvida Jul 4, 2022

xshadow Jul 4, 2022

xshadow Jul 4, 2022

xshadow Jul 4, 2022

ulvida Jul 4, 2022

xshadow Jul 4, 2022

xshadow Jul 4, 2022

xshadow Jul 4, 2022

xshadow Jul 4, 2022

xshadow commented Jul 4, 2022

ulvida commented Jul 5, 2022 •

edited

Loading

ulvida commented Jul 6, 2022

Feature new templates set #54

Are you sure you want to change the base?

Feature new templates set #54

Conversation

ulvida commented Jul 2, 2022 • edited Loading

ulvida commented Jul 2, 2022

xshadow commented Jul 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xshadow commented Jul 4, 2022

ulvida commented Jul 5, 2022 • edited Loading

ulvida commented Jul 6, 2022

ulvida commented Jul 2, 2022 •

edited

Loading

ulvida commented Jul 5, 2022 •

edited

Loading