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

Use data in modules to determine values #424

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ekohl
Copy link
Member

@ekohl ekohl commented Sep 26, 2023

Testing this out with an installer PR. Not ready at all.

@ekohl
Copy link
Member Author

ekohl commented Sep 26, 2023

@ehelms together with this I get:

$ bundle exec ./bin/foreman-certs --help 
Usage:
    foreman-certs [OPTIONS]

Options:

= Generic:
    -i, --interactive                  Run in interactive mode
    -n, --noop                         Run puppet in noop mode? (default: false)
    -s, --skip-checks-i-know-better    Skip all system checks (default: false)
    -v, --[no-]verbose                 Display log on STDOUT instead of progressbar (default: false)
    -l, --verbose-log-level LEVEL      Log level for log based terminal output.
                                       The available levels are ERROR, WARN, NOTICE, INFO, DEBUG. See --full-help for definitions. (default: "notice")
    -S, --scenario SCENARIO            Use installation scenario
    --list-scenarios                   List available installation scenarios
    -h, --help                         print help
    --full-help                        print complete help
    --[no-]enable-certs                Enable 'certs' puppet module (default: true)
    --[no-]enable-certs-apache         Enable 'certs_apache' puppet module (default: true)


= Module certs:
    --certs-cname                      The alternative names of the host the generated certificates
                                       should be for (current: [])
    --certs-node-fqdn                  The fqdn of the host the generated certificates
                                       should be for (current: "guus.kohlvanwijngaarden.nl")
    --certs-server-ca-cert             Path to the CA that issued the ssl certificates for https
                                       if not specified, the default CA will be used (current: UNDEF)
    --certs-server-cert                Path to the ssl certificate for https
                                       if not specified, the default CA will generate one (current: UNDEF)
    --certs-server-cert-req            Path to the ssl certificate request for https
                                       if not specified, the default CA will generate one (current: UNDEF)
    --certs-server-key                 Path to the ssl key for https
                                       if not specified, the default CA will generate one (current: UNDEF)
    --certs-tar-file                   Use a tarball with certificates rather than generate
                                       new ones. This can be used on another node which is
                                       not the CA. (current: UNDEF)


= Module certs_apache:
    --certs-apache-cname               The alternative names of the host the generated certificates
                                       should be for (current: [])
    --certs-apache-hostname            The fqdn of the host the generated certificates
                                       should be for (current: "guus.kohlvanwijngaarden.nl")
    --certs-apache-server-cert         Path to the ssl certificate for https
                                       if not specified, the default CA will generate one (current: "")
    --certs-apache-server-cert-req     Path to the ssl certificate request for https
                                       if not specified, the default CA will generate one (current: "")
    --certs-apache-server-key          Path to the ssl key for https
                                       if not specified, the default CA will generate one (current: "")

Only commonly used options have been displayed.
Use --full-help to view the complete list.

As you can see, it converts aliases that are undef to empty strings. I recall this being a Hiera problem with alias. However, it does correctly determine values for the other items.

String $org,
String $org_unit,
String $expiration,
Stdlib::Absolutepath $ca_key_password_file,
Copy link
Member

Choose a reason for hiding this comment

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

This is defined inside certs/init.pp and I think should be dropped here? or do the same pick strategy down below. With that change your proposal here did allow me to run my command with success.

@ehelms
Copy link
Member

ehelms commented Sep 26, 2023

Are there any reasons not to go this route with changing the module in this way? This is a bit different than our other modules, but does enable some more interesting abilities to use these classes.

@ekohl
Copy link
Member Author

ekohl commented Sep 26, 2023

Are there any reasons not to go this route with changing the module in this way? This is a bit different than our other modules, but does enable some more interesting abilities to use these classes.

Way WAY back in the day that was the intention. You can see https://github.com/theforeman/puppet-tftp was converted by Dominic: theforeman/puppet-tftp@e2003d4

There are some downsides, like puppet-strings output being less useful since it doesn't show the default value. I aimed to mitigate that with puppetlabs/puppet-strings#273 but never found the time to wrap it up.

We can note that puppet-strings issues don't affect us since we already use rdoc due to parameter grouping. You can still argue that parameters directly in the file are better, but this is no worse than looking it up in params.pp. Except if there are multiple OS-specific overrides. Then you need multiple files open to track the data.

@ehelms
Copy link
Member

ehelms commented Sep 26, 2023

How much of this is Puppet and how much of this is how Kafo is making assumptions and doing look ups? For example, would it be better to teach Kafo about classes as the base rather than modules? I'm not sure if that fixes things but if native Puppet works just fine, I'm curious what special things we do in Kafo are actually breaking this.

@ekohl
Copy link
Member Author

ekohl commented Sep 26, 2023

Kafo needs to extract data to determine the values. If puppet-strings can determine the value, it will be faster because it can use a (JSON) cache. Otherwise it needs to evaluate the class in noop mode. That will increase the runtime of the whole installer. On the other hand, this isn't slower than the code we need now for $class::params::*. Now if everything is Hiera, we can perhaps use puppet hiera lookup (or some equivalent) to bypass all of that, but we don't have the code today.

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

Successfully merging this pull request may close these issues.

2 participants