Skip to content

Conversation

@ekohl
Copy link
Member

@ekohl ekohl commented Sep 12, 2024

When passing in a tar file, it should respect all the files from that tar file and not overwrite them. I haven't done proper digging into when this exactly regressed yet.

@ekohl ekohl force-pushed the add-test-for-certs-tar-with-server-certs branch from 729502a to 57eabc3 Compare September 12, 2024 14:19
@ekohl ekohl marked this pull request as draft September 12, 2024 15:54
@ekohl ekohl force-pushed the add-test-for-certs-tar-with-server-certs branch from 57eabc3 to 6ba3aee Compare September 12, 2024 15:55
@ekohl
Copy link
Member Author

ekohl commented Sep 12, 2024

This is growing to take a stab at fixing #456 but I'm not sure it's the right track. I suspect that all the various classes (apache, etc) will still attempt to generate a certificate.

@ekohl ekohl force-pushed the add-test-for-certs-tar-with-server-certs branch from 6ba3aee to dc92400 Compare September 12, 2024 19:00
@ekohl
Copy link
Member Author

ekohl commented Sep 12, 2024

@ehelms this is now green, but I'm a bit hesitant on touching such a core class.

I wonder if passing $certs::tar_file should imply generate => false and regenerate => false.

@ekohl ekohl marked this pull request as ready for review September 12, 2024 19:09
@ekohl ekohl linked an issue Sep 12, 2024 that may be closed by this pull request
@ekohl ekohl changed the title Add a test for customer server certificates in tar file Respect all files extracted from tar_file Sep 12, 2024
@ekohl
Copy link
Member Author

ekohl commented Sep 12, 2024

This feels like a big regression and deserves a Redmine issue. It's one of our core workflows that we broke.

@ehelms
Copy link
Member

ehelms commented Sep 13, 2024

See #463 for a more targeted approach that relies on the way this worked prior to the re-design. I have also included a Redmine issue with it.

manifests/ca.pp Outdated
target => $server_ca_path,
require => File[$server_ca_path],
if $generate {
file { "${certs::ssl_build_dir}/KATELLO-TRUSTED-SSL-CERT":
Copy link
Member Author

Choose a reason for hiding this comment

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

Random note: I couldn't find a use for this anymore. I think we can drop it in a separate backwards-incompatible PR.

Copy link
Member

Choose a reason for hiding this comment

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

Yes - to my knowledge as well this can be dropped.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ekohl ekohl marked this pull request as draft September 13, 2024 10:24
This is reused a few times and makes it easier to follow what's related.
This always defined the same file, just with a different source. That
source is either the provided server_ca_cert or the default CA.
This avoids writing out a password file that isn't needed.
@ekohl ekohl force-pushed the add-test-for-certs-tar-with-server-certs branch from dc92400 to 86ee380 Compare September 13, 2024 10:24
Copy link
Member Author

Choose a reason for hiding this comment

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

Should the header be?

  Boolean $generate = $certs::generate and !$certs::tar_file,

Though I think it should be part of init.pp and reused in other places. Ideally we'd get rid of generate and regenerate altogether.

@ekohl
Copy link
Member Author

ekohl commented Oct 15, 2024

I split off the refactors without impact in #467.

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