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

Conditionally generate the CA cert. #783

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

coolguydork
Copy link

No description provided.

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Some minor suggestions, overall it makes sense to have a parameter for this.

@@ -536,6 +536,8 @@
# invokes when on static_file_content requests.
# Defaults to undef
#
# $generate_ca_cert:: Defaults to true. When true, the a ca cert is generated.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# $generate_ca_cert:: Defaults to true. When true, the a ca cert is generated.
# $generate_ca_cert:: Whether or not a CA certificate is generated.

Copy link
Author

Choose a reason for hiding this comment

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

Will do.

Concat["${puppet::server::dir}/puppet.conf"],
Exec['puppet_server_config-create_ssl_dir'],
],
if $puppet::generate_ca_cert {
Copy link
Member

Choose a reason for hiding this comment

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

I think you can move this up a bit so it also captures the if/else block to determine $creates and $command. They're only used in this exec.

Copy link
Author

Choose a reason for hiding this comment

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

ok, will do.

@@ -157,22 +157,24 @@

# Generate a new CA and host cert if our host cert doesn't exist
if $puppet::server::ca {
Copy link
Author

Choose a reason for hiding this comment

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

@ekohl I followed your suggestion. However, if this is the case, shouldn't there just be a $ca in the manifests/init.pp? There is no $ca option in manifests/init.pp now.

I just need this turned off so this module can complete doing its thing without failing about preexisting certs.

Copy link
Member

@ekohl ekohl Apr 13, 2021

Choose a reason for hiding this comment

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

@ekohl I followed your suggestion. However, if this is the case, shouldn't there just be a $ca in the manifests/init.pp? There is no $ca option in manifests/init.pp now.

We do have $server_ca in init.pp

I just need this turned off so this module can complete doing its thing without failing about preexisting certs.

We have a creates. Why isn't it picking that up?

Copy link
Author

Choose a reason for hiding this comment

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

Hi @ekohl ,

We do have $server_ca in init.pp

You must be referring to this. I did see this, but this parameter has no effect on whether to generate a CA file.

We have a creates. Why isn't it picking that up?

Where in the code is this? I did not find a creates parameter in the init.pp.

@@ -734,6 +736,8 @@
Optional[Integer[1]] $server_max_open_files = $puppet::params::server_max_open_files,
Optional[Stdlib::Absolutepath] $server_versioned_code_id = undef,
Optional[Stdlib::Absolutepath] $server_versioned_code_content = undef,
Boolean $generate_ca_cert = $puppet::params::generate_ca_cert,

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

@coolguydork
Copy link
Author

Hi any news on this? Should I continue to pursue this PR or should I just leave it?

@zjhuntin zjhuntin requested a review from ekohl May 17, 2021 14:53
@ekohl
Copy link
Member

ekohl commented Feb 4, 2022

My apologies for not getting to this. I've been really neglecting this module. Sadly there's a merge conflict now. If you're still interested, please rebase and resolve the conflict.

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.

3 participants