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

No support for chroot #40

Open
inkblot opened this issue May 19, 2015 · 16 comments
Open

No support for chroot #40

inkblot opened this issue May 19, 2015 · 16 comments

Comments

@inkblot
Copy link
Owner

inkblot commented May 19, 2015

No description provided.

@cedef
Copy link
Contributor

cedef commented Apr 26, 2017

Hi @inkblot,

Do you have any idea if you plan to implement chroot suppor for your bind module ?
I'm interested in this feature and the alternative thias-bind module seems to be late with its maintenance (lots of PR/issues unaddressed...)

Regards,

@cedef
Copy link
Contributor

cedef commented Apr 26, 2017

I'm thinking about implementing this feature but I would like to discuss with you on the way to do it.
Here is a bunch of technical choices I'm thinking about:

where to put named config files ?

RedHat family uses the /usr/libexec/setup-named-chroot.sh script to prepare the chroot place. It basically consists of mount --bind a list of config files (i.e. /etc/named.conf, /etc/named.rfc1912.zones)
We could either:

  1. Ensure Puppet only drops files into /etc/named (i.e instead of using /etc/named.default-zones.conf).
  2. Drop directly files into the chroot and remove all files from /etc/named*. Although this solution might trick people looking for the config file into /etc (also backup system that take /etc snapshots, etc...

I prefer solution 1, but then how should we change the module's config file hierarchy ? Only for chroot mode ? for everybody in the next release ?

Distributions specific stuff

  • IMO RedHat's /usr/libexec/setup-named-chroot.sh script should be called directly by the systemd named-chroot service as a PreExec directive.
  • Debian provides a HowTo where it tolds to copy files

I think we could either

  • Follow distributions as close as possible and follow for each their way to chroot bind
  • Use the same way on both (i.e. use the RHEL script on debian in order to mount bind config files instead of copying them)

Sorry for such a long paragraph :) Let me know if you have any idea about the chroot feature.

Regards,

@inkblot
Copy link
Owner Author

inkblot commented Apr 26, 2017

Honestly, I haven't thought about this feature in a long time. I had looked over the Debian HowTo when I originally opened this issue, but did not pursue it further. I have not seen the RedHat setup script before.

What RedHat package provides the setup-named-chroot script? I've installed bind in a CentOS docker container but it did not create /usr/libexec/setup-named-chroot.sh. I'd like to take a look at it to see how it works.

Whatever the implementation is, I think that feature should be enabled/disabled using a simple chroot parameter to the bind class.

@cedef
Copy link
Contributor

cedef commented Apr 26, 2017

What RedHat package provides the setup-named-chroot script? I've installed bind in a CentOS docker container but it did not create /usr/libexec/setup-named-chroot.sh. I'd like to take a look at it to see how it works.

The package is called bind-chroot and it comes with a dedicated named-chroot systemd service.

Whatever the implementation is, I think that feature should be enabled/disabled using a simple chroot parameter to the bind class.

I totally agree. I'll manage to base changes only on $chroot parameter.
I'll submit a PR in few days if I do not encounter big problem 😄

@cedef
Copy link
Contributor

cedef commented Sep 4, 2017

I finally found some time to implement what we've talked about in PR #125

Here is a first draft on my forked repo. Before going further, I'd like to get your feedback on the following points:

  • code has been splitted into bind::chroot::package (for RHEL) and bind::chroot::manual (for Debian and probably any other distro).

  • I'm not sure how the Debian way should translate into Puppet idem-potency: the /etc/bind directory points to /var/bind9/chroot/etc/bind, so I've used an exec resource to move the /etc/bind dir and then I declare a symlink. This way I don't need to change all paths already declared into your code (/etc/bind/named.conf, /etc/bind/**, ..) as it will pass through the symlink transparently.

Note: this draft does not work on Debian for now, due to an ordering error between /etc/bind dir creation, symlinking and moving.

Regards,

@cedef
Copy link
Contributor

cedef commented Oct 10, 2017

Any news on your side ? Have you been able to checkout draft ?
Maybe you would prefer a PR ?

Regards,

@inkblot
Copy link
Owner Author

inkblot commented Oct 14, 2017

I'm taking a look now. Sorry for the delay.

@inkblot
Copy link
Owner Author

inkblot commented Oct 14, 2017

In spirit, this looks like a very good solution. The things that I would like to see amended are all small details.

The hiera hierarchy is not hierarchical. I have some local changes that fix that, but it is mostly a stylistic issue. Any sub categories of osfamily should descend from the existing osfamily/${facts.os.family} category and act as refinements. I would also prefer that the semantic hierarchy of the hiera configuration match the filesystem hierarchy, to wit that the elements of the paths in hiera.yaml should be / delimited so as to express a directory hierarchy rather an a file naming convention.

The exec resources in bind::chroot::manual could be simplified. mknod is always installed at /bin/mknod (per POSIX specification) and so it is harmless to fully-qualify the invocation in the command property, which would obviate the need for the path property. Also, using creates instead of unless is a very natural fit for these execs and would simultaneously make the code more concise and avoid forking a test process during catalog application.

I'm concerned about tight coupling in the tests. For example these lines from spec/classes/bind_spec.rb:

        case facts[:os]['family']
        when 'RedHat'
          it { is_expected.to contain_file(expected_default_zones_include) }
          it { is_expected.not_to contain_service('bind-no-chroot') }
        when 'Debian'
          it { is_expected.not_to contain_file(expected_default_zones_include) }
        end

There are two distinct things being asserted here in unison. First that certain conditions are met when bind::chroot::class is set to bind::chroot::package and other conditions are met when bind::chroot::class is set to bind::chroot::manual. Second is that facts[:os]['family'] is a proxy for the bind::chroot::class setting. The former concern is worth testing, as it asserts that the functionality of the module is correct. The latter should not be tested at all. Configuration should always be free to vary.

I would like to take some more time to look at this work again later. Thank you very much for your effort.

@cedef
Copy link
Contributor

cedef commented Oct 15, 2017

Thank you for your answer.

I would like to take some more time to look at this work again later.

I'll try to submit a new draft including your feedback so that we could go further in feedback and code review.

Thank you very much for your effort.

Thank you :)

Have a nice day,

@cedef
Copy link
Contributor

cedef commented Oct 18, 2017

Hi @inkblot,

I've updated my forked repo based on your suggestions. Here are additional notes:

  • On CentOS7, which -a mknod returns /usr/bin/mknod
  • I've created osfamily/%{facts.os.name}/%{facts.os.release.major} so that Ubuntu-16.04 data would be in osfamily/Ubuntu/16.04.yaml and Debian-9 data in osfamily/Debian/9.yaml
  • Regarding spec tests, I'm not sure I've catched what you are talking about exactly (maybe because english is not my primary language). Maybe you could give me an example ?
  • The latest commit (9282354) compiles and runs perfectly on vanilla debian9, installing and starting chrooted bind service.

For further contributions and adjustments should I open a pull request ?
Please let me know if you have other suggestions,

Regards,

@cedef
Copy link
Contributor

cedef commented Nov 20, 2017

Little update on that one, posted a month ago :)

@inkblot
Copy link
Owner Author

inkblot commented Mar 30, 2018

Sorry for the long lapse in communication. I am trying out the feat/chroot branch now. The code changes look good. I just have a minor change to the hiera data that I would like to make. If everything works out, I'd like to merge the work.

@inkblot
Copy link
Owner Author

inkblot commented Mar 30, 2018

I've noticed a couple of things while attempting to apply the changes with a chroot on Debian.

  1. There is some awkward resource naming. I'll make some small changes to fix this.
  2. The existing /var/cache/bind directory is not moved into the chroot, which causes all of the accrued dynamic changes to be lost. The is of critical importance when the bind server is e.g. receiving regular frequent updates from a DHCP server.

@inkblot
Copy link
Owner Author

inkblot commented Mar 30, 2018

I'm trying to decide whether that second point is worthwhile or not. There are some complications related to provisioning vs. maintenance. Existing dynamic zones on a maintained server would have to be frozen before the files are moved, but that would fail entirely during provisioning of a new server. It would have to happen before any Bind::Zone[*] application, too. That might be handled by the implied dependencies between file resources.

It's a corner case. I think I've talked myself into ignoring it for now.

@inkblot
Copy link
Owner Author

inkblot commented Mar 30, 2018

I have included the chroot work in version 7.4.0 published on the Puppet Forge. I really appreciate all the hard work that you put into this and your patience in dealing with me.

@cedef
Copy link
Contributor

cedef commented Apr 2, 2018

Wow !! Sorry for the delay, I've been quite busy.
I'm very glad you accepted my PR :)

Thank you for your review and your fixes.

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

2 participants