Fixes #31759 - Make the CA configurable and add default certs #10
Conversation
|
Issues: #31759 |
ianballou
force-pushed
the
31759-default-certs
branch
from
d4302b1
to
f769f2d
Compare
| :katello_registry_path => '/v2/', | ||
| :sqlite_db_path => '/var/lib/foreman-proxy/smart_proxy_container_gateway.db' | ||
| rescue => Errno::ENOENT | ||
| logger.warn("Default settings could not be loaded. Default certs will not be set.") |
There was a problem hiding this comment.
out of curiosity what would cause the default settings to not be loaded?
There was a problem hiding this comment.
The tests fail to load the default settings because they don't exist in .vendor.
There was a problem hiding this comment.
I don't think this is the correct implementation. Calling Proxy::Settings.initialize_global_settings while loading a plugin sounds like a recipe for breakage.
Why do you need explicit pulp settings here? The actual code could use SETTINGS.foreman_ssl_ca directly.
A related example is the templates module which relies on Proxy::SETTINGS.foreman_url. It's stubbed in tests:
https://github.com/theforeman/smart-proxy/blob/fdeef1dc6febcfae22c8d3273cb18d6bdeb31a23/test/templates/template_proxy_request_test.rb#L11-L12
Please do not use Proxy::Settings.initialize_global_settings this way. It is a recipe for disaster and has the potential to break the entire smart proxy.
There was a problem hiding this comment.
I don't see how Proxy::Settings.initialize_global_settings could break the entire smart proxy. From the scope of the container gateway, SETTINGS is nil. So is Proxy::SETTINGS. All initialize_global_settings does is read the proxy settings file and returns it. It doesn't modify any special background runtime variables or anything like that: https://github.com/theforeman/smart-proxy/blob/develop/lib/proxy/settings.rb#L10. I feel like the method should've been called read_global_settings instead.
There was a problem hiding this comment.
For the record, SETTINGS seems to be nil here because this code is running before SETTINGS can be set in smart-proxy.
There was a problem hiding this comment.
Still, you could be messing with the order of initialization. I don't think it was intended as a public API for plugins and strongly insist you don't use it. Remember that when you call this function, you could still be running bundler's intialization code. That's because the gem is loaded and this plugin file is included. It's then executed in the static context as a DSL.
There was a problem hiding this comment.
Hm okay, so my alternative then will be to remove the default cert
settings. During runtime, the container gateway will check if the cert
settings are set. If they're not, it'll default to what's in SETTINGS. It
feels a little strange to do it like that, but it looks like it's the
workaround that'll have to happen since SETTINGS is nil in the default init
code.
ianballou
force-pushed
the
31759-default-certs
branch
from
f769f2d
to
c165f46
Compare
|
Everything should be all set now. |
|
There is also |
|
Other things you may want to consider is validations. From Puppet's config: validate :puppet_url, :url => true
validate_readable :puppet_ssl_ca, :puppet_ssl_cert, :puppet_ssl_keyThis ensures the URL is really a URL and the various SSL files are readable files for the Proxy user. |
|
I've created some issues from the suggestions: |
The CA cert is now configurable and the container gateway defaults to the main proxy certs for talking with Pulp.