-
Notifications
You must be signed in to change notification settings - Fork 134
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
Fixes #29803 - Move --certs* to hooks/ #514
Conversation
Given this and a few other PRs depend on the logic, I implemented the |
0423c7d
to
4ed80ec
Compare
4ed80ec
to
d205021
Compare
d205021
to
7f9da5b
Compare
hooks/pre/20-certs_update.rb
Outdated
if app_value('certs_update_server') | ||
mark_for_update("#{hostname}-apache", hostname) | ||
mark_for_update("#{hostname}-foreman-proxy", hostname) | ||
end | ||
|
||
if app_value('certs_update_all') || app_value('certs_update_default_ca') || app_value('certs_reset') | ||
all_cert_names = Dir.glob(File.join(SSL_BUILD_DIR, hostname, '*.noarch.rpm')).map do |rpm| | ||
File.basename(rpm).sub(/-1\.0-\d+\.noarch\.rpm/, '') | ||
end.uniq | ||
|
||
all_cert_names.each do |cert_name| | ||
mark_for_update(cert_name, hostname) | ||
end | ||
end | ||
|
||
if app_value('certs_update_server_ca') || app_value('certs_reset') | ||
mark_for_update('katello-server-ca') | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to gather an array on all of this? We now have all_cert_names
but really we can gather them all. Explicitly gathering the paths would make mark_for_update
redundant. Because I wanted to know how it looked, I took a stab at it:
require 'fileutils'
if module_enabled?('certs')
if param('foreman_proxy_certs', 'foreman_proxy_fqdn')
hostname = param('foreman_proxy_certs', 'foreman_proxy_fqdn').value
else
hostname = param('certs', 'node_fqdn').value
end
SSL_BUILD_DIR = param('certs', 'ssl_build_dir').value
HOST_BUILD_DIR = File.join(SSL_BUILD_DIR, hostname)
certs_to_update = []
if app_value('certs_update_server')
certs_to_update << File.join(HOST_BUILD_DIR, "#{hostname}-apache")
certs_to_update << File.join(HOST_BUILD_DIR, "#{hostname}-foreman-proxy")
end
if app_value('certs_update_all') || app_value('certs_update_default_ca') || app_value('certs_reset')
certs_to_update += Dir.glob(File.join(HOST_BUILD_DIR, '*.noarch.rpm')).map do |rpm|
rpm.sub(/-1\.0-\d+\.noarch\.rpm/, '')
end
end
if app_value('certs_update_server_ca') || app_value('certs_reset')
certs_to_update << File.join(SSL_BUILD_DIR, 'katello-server-ca')
end
certs_to_update.uniq.each do |path|
if app_value(:noop)
puts "Marking certificate #{path} for update (noop)"
else
puts "Marking certificate #{path} for update"
FileUtils.touch("#{path}.update")
end
end
end
I think it looks and better shows the intention of this hook. Note I left out resetting the params since I believe that should be in a different hook (https://github.com/theforeman/foreman-installer/pull/514/files#r476563297).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that katello_certs/hooks/pre/20-certs_update.rb
is a symlink to the current certs update. That will need an update too.
Given this hook has worked by all accounts, it would be nice if this could be a move and then a refactor rather than combining the two. That will make tracking down issues easier. |
7f9da5b
to
f113084
Compare
7b98a19
to
cc07b39
Compare
17ca33e
to
fdb66e9
Compare
fdb66e9
to
8245a13
Compare
../../../hooks/boot/20-certs_update.rb |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I always forget these and what they're exactly supposed to do. It would be great to have some integration tests to make sure the functionality actually does what it's supposed to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm working on theforeman/forklift#1208 as a first step towards integration testing of installer PRs.
Could we go ahead and merge this, and add tests in a future PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, tests can come after. Please file a Redmine issue to not lose track of the need.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree they can come later, but I'm wonder how much we actually verified this continues to work. With the rest of the code I'm decently familiar with how it should work but I'm not that familiar with foreman-proxy-certs-generate
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally with an install test
No description provided.