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

Proposal for adding to the blueprint customizations #634

Merged
merged 11 commits into from
May 3, 2019

Conversation

bcl
Copy link
Contributor

@bcl bcl commented Mar 7, 2019

The goal here is to do the minimum needed to get the images setup for
use so they can have more complex customizations applied later.
I think this list is a pretty good minimal set of features without going
full kickstart.

@wgwoods
Copy link
Contributor

wgwoods commented Mar 9, 2019

I like what's going on here - definitely feels like the right set of customizations - but I have a couple thoughts about config syntax/schema to make sure we retain flexibility, but still without going Full Kickstart

First: I feel like it's generally a good idea if values under [customizations.*] are container types - right now some are strings (language, timezone) and I worry about what happens if we need to extend the string-value ones, since we can't just add new keys like with tables..

Looking at Ansible's timezone role, though, the only thing it really uses is name, and timezone = "name" is just as good. So I think that's fine.

But language is trickier, because there's definitely other related things you might want to twiddle (other locale variables, keymaps, etc.) So maybe we want a block for locale, even if the only setting we handle (for now) is "lang":

[customizations.locale]
lang="en_US.utf8"

As for service, the only thing from the service role that makes sense in Composer is enabled, and enabled = ["service", ...] is equivalent, so I'm good with that.. but can we use "enabled" instead of "enable"? We're describing the intended final state of the system, and I want to avoid anything that sounds imperative or script-y.

Lastly, firewall - I'm pumped that you've basically got a nice, limited subset of linux-system-roles/firewall, except we've lost the explicit state/enable bit, which means you can't disable things. Might need to accept multiple blocks:

[[customizations.firewall]]
port = "4222/tcp"
state = "enabled"

[[customizations.firewall]]
port = "22/tcp"
state = "disabled"

Or, following the enabled = ["item", ...] pattern from service:

[customizations.firewall.ports]
enabled = ["4022/tcp"]
disabled = ["22/tcp"]

But I'm worried about firewall.services - the list of valid service names can vary depending on which packages you install. Is there a documented/supported way to list valid services without running firewall-cmd inside the system being built?

@bcl
Copy link
Contributor Author

bcl commented Mar 11, 2019

First: I feel like it's generally a good idea if values under [customizations.*] are container types - right now some are strings (language, timezone) and I worry about what happens if we need to extend the string-value ones, since we can't just add new keys like with tables..

That helps keep things limited :) ButI see your point.

But language is trickier, because there's definitely other related things you might want to twiddle (other locale variables, keymaps, etc.) So maybe we want a block for locale, even if the only setting we handle (for now) is "lang":

[customizations.locale]
lang="en_US.utf8"

Yeah, that makes sense if we want to allow setting multiple locales, etc. My plan for that was to give them just enough to get the system setup so they can use it in their native language and apply customizations later.

As for service, the only thing from the service role that makes sense in Composer is enabled, and enabled = ["service", ...] is equivalent, so I'm good with that.. but can we use "enabled" instead of "enable"? We're describing the intended final state of the system, and I want to avoid anything that sounds imperative or script-y.

Yeah, that's fine.

Lastly, firewall - I'm pumped that you've basically got a nice, limited subset of linux-system-roles/firewall, except we've lost the explicit state/enable bit, which means you can't disable things. Might need to accept multiple blocks:

[[customizations.firewall]]
port = "4222/tcp"
state = "enabled"

[[customizations.firewall]]
port = "22/tcp"
state = "disabled"

The thought process there is that everything is disabled by default, other than things like ssh that enable themselves. I suppose there may be cases where you want to disable things opened by a service (eg. 3rd party package that doesn't meet your security requirements)

Or, following the enabled = ["item", ...] pattern from service:

[customizations.firewall.ports]
enabled = ["4022/tcp"]
disabled = ["22/tcp"]

I like this form better, keeps it consistent with service.

But I'm worried about firewall.services - the list of valid service names can vary depending on which packages you install. Is there a documented/supported way to list valid services without running firewall-cmd inside the system being built?

Not that I know of. The list doesn't match /etc/services, and I tried reading the firewalld code to see where it gets the list but it was 4 layers of abstraction before I gave up. I'd lean towards just having ports available, but I think some of the services support dynamic port assignment so it's probably a good idea to keep that in there and direct them to the distribution's firewalld documentation.

@bcl bcl force-pushed the master-new-customizations branch from b3417c1 to 902eaf9 Compare April 11, 2019 18:30
@coveralls
Copy link

coveralls commented Apr 11, 2019

Pull Request Test Coverage Report for Build 1520

  • 104 of 105 (99.05%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+3.007%) to 60.43%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/pylorax/api/compose.py 102 103 99.03%
Totals Coverage Status
Change from base Build 1485: 3.007%
Covered Lines: 3259
Relevant Lines: 5269

💛 - Coveralls

@bcl bcl force-pushed the master-new-customizations branch from 902eaf9 to eafa479 Compare April 11, 2019 21:50
@bcl
Copy link
Contributor Author

bcl commented Apr 12, 2019

I need someone who understands Anaconda's use of keyboard to check the locale commit, I wrote it thinking I could use the same values for --vckeymap and --xlayouts but I'm not sure. And I was surprised to discover I cannot use us-dvorak even though it is listed by localectl list-keymaps

@atodorov
Copy link
Member

@bcl what is the status here? is this ready for merge?

OTOH should we go ahead and merge #690 with failing Travis CI if this isn't ready ?

@bcl
Copy link
Contributor Author

bcl commented Apr 22, 2019

I think it's ready, but it needs to be reviewed in detail. Lots of new code added.

@bcl bcl force-pushed the master-new-customizations branch from 0000190 to 0d69f2f Compare April 30, 2019 21:34
@bcl
Copy link
Contributor Author

bcl commented Apr 30, 2019

Updated with locale keyboard feedback from mkolman.

@bcl bcl force-pushed the master-new-customizations branch from 0d69f2f to 8331003 Compare May 1, 2019 16:38
bcl added 11 commits May 2, 2019 16:21
The goal here is to do the minimum needed to get the images setup for
use so they can have more complex customizations applied later.
I think this list is a pretty good minimal set of features without going
full kickstart.
For example:

[customizations.timezone]
timezone = "US/Samoa"
ntpservers = ["0.pool.ntp.org"]

Also includes tests.

This removes the timezone kickstart command from all of the templates
except for google.ks which needs to set it's own ntp servers and timezone.

If timezone isn't included in the blueprint, and it is not already in a
template, it will be set to 'timezone UTC' by default.

If timezone is set in a template it is left as-is, under the assumption
that the image type requires it to boot correctly.
This fixes the customizations list problem earlier than in
add_customizations.

In the recipe it should be [customizations] not [[customizations]]
which creates a list. If it was used that way grab the first element and
replace the list with it.
You can now set the keyboard layout and language. Eg.

[customizations.locale]
languages = ["en_CA.utf8", "en_HK.utf8"]
keyboard = "de (dvorak)"

Existing entries in the kickstart templates are replaced with the new
ones. If there are no entries then it will default to 'keyboard us' and
'lang en_US.UTF-8'

Includes tests, and leaves the existing keyboard and lang entries in the
templates with a note that they can be replaced by the blueprint.
You can now open ports in the firewall, using port numbers or service
names:

    [customizations.firewall]
    ports = ["22:tcp", "80:tcp", "imap:tcp", "53:tcp", "53:udp"]

Or enable/disable services registered with firewalld:

     [customizations.firewall.services]
     enabled = ["ftp", "ntp", "dhcp"]
     disabled = ["telnet"]

If the template contains firewall --disabled it cannot be overridden,
under the assumption that it is required for the image to boot in the
selected environment.
There is a problem with rpmfluff and the current version of rpm in
rawhide.

Changes are upstream, but no new build has been done yet.
Add support for enabling and disabling systemd services in the
blueprint. It works like this:

    [customizations.services]
    enabled = ["sshd", "cockpit.socket", "httpd"]
    disabled = ["postfix", "telnetd"]

They are *added* to any existing settings in the kickstart templates.
Make it clear that the services are added to services already listed in
the image templates, and that you can specify any systemd unit filename.

Older releases are more restrictive, and this documentation will need to
be updated when these changes are backported.
To maintain consistency with the other options this changes firewall to
combine the existing settings from the image template with the settings
from the blueprint.

Also updated the docs, added a new test for it, and sorted the output
for consistency.
@bcl bcl force-pushed the master-new-customizations branch from 8331003 to 72ff8e7 Compare May 2, 2019 23:22
@bcl bcl merged commit 72ff8e7 into weldr:master May 3, 2019
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.

4 participants