Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DRY: configured_zones, available_zones #114

Open
aphor opened this issue Sep 29, 2018 · 10 comments
Open

DRY: configured_zones, available_zones #114

aphor opened this issue Sep 29, 2018 · 10 comments

Comments

@aphor
Copy link

aphor commented Sep 29, 2018

Convention over configuration:

If a zone is defined in a pillar, I want the salt states to ensure it is declared in the conf file and also the records are declared in the zone file.

@ppieprzycki thought it was a good idea for Debian in 2947dde

It's probably a good idea in general.

@aanriot
Copy link
Contributor

aanriot commented Oct 1, 2018

Hello @aphor ,

Have you tried to use available_zones ? It should declare the zone and manage its content as well.

@aphor
Copy link
Author

aphor commented Oct 3, 2018

Zone configuration is templated in bind/files/named.conf.local.jinja and only seems to emits zones found by iterating over configured_zones

{% for key, args in salt['pillar.get']('bind:configured_zones', {})|dictsort -%}

@aphor
Copy link
Author

aphor commented Oct 3, 2018

Why is there even a distinction between configured_zones and available_zones ?

@javierbertoli
Copy link
Member

Hi @aphor, AFAICS, it dates way back to ~2015. But there's some documentation on the usage of these 2 variables here

TL; RL: you use configured_zones to declare which zones are being served by this particular nameserver, being them master, slave, zone files managed or not by this formula or some other way, etc., etc. And then, you use available_zones to populate the zone files.

Hope this helps.

@aphor
Copy link
Author

aphor commented Oct 3, 2018

That helps, but seems over complicated.

I think the behavior implemented for Debian in 2947dde was a move in the right direction.

Would anyone object to making the formula behave as @aanriot suggested?

@javierbertoli
Copy link
Member

This issue, #120 and PR #121 are all suggestions that this formula needs some refactoring to DRY/simplify the logic on zone/s - view/s management.

I think that something like (pseudo here, not necessarily correct):

views:
  - view1
  - view2
  - viewN

zones:
  zone1:
    enabled: true
    external_file: /path/or/ref/to/source/of/file
    records:
        all the records code
    view:         # If present, zone will be exposed in the corresponding view
       - view1
  zone2:
    enabled: false     # to keep the file, but disable the zone to be served
    external_file: /path/or/ref/to/source/of/file  # if present, file with zone records will be managed externally
  zoneN:

Maybe something like this covers all cases we need to consider?

@aanriot
Copy link
Contributor

aanriot commented Dec 31, 2018

This issue, #120 and PR #121 are all suggestions that this formula needs some refactoring to DRY/simplify the logic on zone/s - view/s management.

Indeed @javierbertoli , the formula has become too much complicated. Though, I think that we should opt for bind-ng states and pillars, like https://github.com/saltstack-formulas/nginx-formula did, to avoid breaking existing setups.

I think that something like (pseudo here, not necessarily correct):

views:
  - view1
  - view2
  - viewN

zones:
  zone1:
    enabled: true
    external_file: /path/or/ref/to/source/of/file

As the file path is always needed, we may want to use instead:

zones:
  zone1:
    enabled: true
    zonefile: /path/or/ref/to/source/of/file
    managed (or external): true
records:
    all the records code
view:         # If present, zone will be exposed in the corresponding view
   - view1

zone2:
enabled: false # to keep the file, but disable the zone to be served
external_file: /path/or/ref/to/source/of/file # if present, file with zone records will be managed externally
zoneN:


Maybe something like this covers all cases we need to consider?

It seems to, if we keep a way to detect the file path when it's not defined.

@javierbertoli
Copy link
Member

Indeed @javierbertoli , the formula has become too much complicated. Though, I think that we should opt for bind-ng states and pillars, like https://github.com/saltstack-formulas/nginx-formula did, to avoid breaking existing setups.

Please, if possible, don't start a new -ng branching situation 😄

We had a lengthy discussion about that issue a few days back and the consensus was to use semantic versioning and adding proper documentation in the formulas, so we are trying to start moving formulas that way.

@aanriot
Copy link
Contributor

aanriot commented Dec 31, 2018

I missed this discussion, great news @javierbertoli.

@aanriot
Copy link
Contributor

aanriot commented Apr 26, 2019

Hello,

We were talking about this issue with a colleague and a new pillar key could be a a way to avoid breaking existing setups:

zones:
  zone1:
    managed: true

@javierbertoli , I know that it doesn't solve the structural problem of the formula but it would at least allow to use managed: false if we don't want bind to manage the zone file.

Any feelings?

Best regards,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants