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

Respect all files extracted from tar_file #461

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

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