saltstack-formulas / bind-formula

http://docs.saltstack.com/en/latest/topics/development/conventions/formulas.html
Other
29 stars 117 forks source link

DRY: configured_zones, available_zones #114

Open aphor opened 5 years ago

aphor commented 5 years ago

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 2947dde649e480091f99552bf7479a0a15c1fb36

It's probably a good idea in general.

aanriot commented 5 years ago

Hello @aphor ,

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

aphor commented 5 years ago

Zone configuration is templated in bind/files/named.conf.local.jinja and only seems to emits zones found by iterating over configured_zones https://github.com/saltstack-formulas/bind-formula/blob/a96669c191f286214527a4991e28e1b5dc4241f0/bind/files/named.conf.local.jinja#L80

aphor commented 5 years ago

Why is there even a distinction between configured_zones and available_zones ?

javierbertoli commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 :smile:

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 commented 5 years ago

I missed this discussion, great news @javierbertoli.

aanriot commented 5 years ago

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,