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

Update zone-serial only on changing zone-records #42

Closed
wants to merge 1 commit into from

Conversation

kubashin-a
Copy link
Contributor

This is remake of previous pull request.
I ask forgiveness for some unnecessary changes in config.pp. If you don't like this, i can remake patch.

In this version i didn't use echo in Exec, instead i created small shell script zone_soa.sh.

# Splitting zone file on two parts: zone-soa (soa record) and zone-data (all other records).
# This is to update zone_serial only by need (then zone-data updated).

$soa_exec_cmdline = template("${module_name}/soa_exec_cmdline.erb")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this place i create command line for zone_soa.sh (it's necessary because we have unknown numbers of nameservers, i.e. different number of arguments for script).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know about this. I kinda liked your previous approach better.

This approach is pretty confusing:

  • Puppet generates a command + args to generate... from a erb template
  • Puppet executes those commands, which the main command is another erb template
  • That templated script is a script that echos out a the zone file components into the real zone file
  • The real zone file is used by bind.

This is pretty complicated.

What if you took a different approach:

  • Use erb + concat to build out the zone file (what we have)
  • Make that zone file zone.data, with the word SERIAL in place of the serial number
  • Make it exec a sed -i 's/SERIAL/$date_in_seconds zone.data' > zone

I think that is more straightforward, still converges, and has only 1 intermediate step?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Puppet generates a command + args to generate... from a erb template
    I did it because zone_soa.sh syntax look like:
zone_soa.sh <many params> <nameserver1> <nameserver2> ... <nameserverN>

I don't know another way to generate cmdline with a different numbers of args.

  • Puppet executes those commands, which the main command is another erb template
    In a template the path to files of zones changes only. As alternative I can pass zone_dir as a parameter to zone_soa.sh.
  • That templated script is a script that echos out a the zone file components into the real zone file
    When I used echo directly in Exec I had a row of problems: different versions of puppet do this different, and i wrote some workarounds. I am afraid that in other Puppet versions workarounds wan't work.

What if you took a different approach:

  • Make it exec a sed -i 's/SERIAL/$date_in_seconds zone.data' > zone

Hmm...
If we create real zone file via concat with SERIAL as template and notify exec with sed then next time Puppet will update zone file to set real serial to SERIAL (because the file will different from concat template).
If we create fake zone file with SERIAL as template and notify exec with sed we will have two identical files (except SERIAL) and one Exec. I don't see big difference between my variant in this case.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The big difference is we are not generating a large command line. The only thing we would be executing would be sed.

Se would have concat make zonefile.staging and sed would be creating zonefile.

It is true that both methods achieve the same effect, I'm just suggesting the sed method is much easier to understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Main problem in exec with echo was output redirection ">" and pipes "|". Variant with sed has similar problem :(
As variant:

$nameservers_str = inline_template("<% @nameservers.each do |nameserver| -%> <%= nameserver %><% end -%>"),
exec { "soa-${zone}":
  command => "$cfg_dir/puppet-scripts/zone_soa.sh $zone $soa $soa_email $zone_serial $zone_ttl $zone_refresh $zone_retry $zone_expire $zone_minimum $nameservers_str",
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need a special command based on a template with lots of arguments. We only care about the serial. Other parts can be handled in the ERB like normal.

$zone_file = '/tmp/zone.staging'
$time = inline_template('<%= Time.now.to_i %>')
exec { "Bump serials on ${zone}":
  command  => "/bin/sed 's/SERIAL/${time}/' $zone_file > /tmp/zone",
  provider => 'shell',
}

@kubashin-a
Copy link
Contributor Author

I have a question: Does we want to save possibility of setting zone serial by hand or we can drop this (and drop compatibility)?

@kubashin-a
Copy link
Contributor Author

I maked another version of this PR: https://github.com/kubashin-a/puppet-dns/compare/master...zone-serial-cat?expand=1
It is closer to option with echo: it haven't additional shell scripts or cmdline generation.
I tested it only on Ubuntu 14.04 with Puppet 3.4.3, tomorrow i will test it on Ubuntu 12.04.

If you don't like this variant, i will make version with sed.

@solarkennedy
Copy link
Collaborator

Yes please.

In the end, I think an exec that gets its command from a template is too complicated, and too error prone, and too difficult to test against.

@solarkennedy
Copy link
Collaborator

Closing in favor of #45

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

Successfully merging this pull request may close these issues.

2 participants