Add Templates feature to foreman-proxy#519
Conversation
dcadf66 to
9a7993c
Compare
77b2cc9 to
6be7b5b
Compare
6be7b5b to
26ddc10
Compare
26ddc10 to
215ec14
Compare
215ec14 to
c5bb26e
Compare
|
Does templates need to be in the features.yaml? I do not see how the feature in this PR ensures foreman-proxy gets added. |
Yes, templates is declared in features.yaml under the foreman_proxy key (same pattern as bmc). I don't think we have a way to ensure if foreman-proxy gets added. |
713b3d9 to
ded1e9b
Compare
|
@ehelms Made some updates that ensures foreman-proxy gets added on the feature install. Not sure if that should be part of this PR. |
f0ba618 to
b20b505
Compare
b20b505 to
da1bfa2
Compare
Signed-off-by: Shubham Ganar <shubhamsg123m@gmail.com>
da1bfa2 to
5485068
Compare
| @@ -0,0 +1,3 @@ | |||
| --- | |||
| :enabled: {{ feature_enabled }} | |||
There was a problem hiding this comment.
as you're exposing "listen on", you'd need to alter the value of :enabled: accordingly. (both maps to true, the others can be used directly to only enable http/https mode.
so far we have not been opening the http port at all, and all plugins are configured for both/true.
given templates is the first one that is actually useful over plain http, we need to rethink that now
There was a problem hiding this comment.
What's the point of setting it to http or https? Why not just true / false?
Do we enforce SSL when the value is https or true?
There was a problem hiding this comment.
We don't enforce SSL in that case, no.
But if a module is enabled on plain HTTP, the authorization is reduced to "the reverse DNS matches an entry in trusted_hosts" aka "no authorization as everyone can fake this shit"¹.
So we need to only ever allow plain HTTP to modules that don't need authorization (like templates).
¹: This is not a problem today, as we don't expose the HTTP port at all
| settings = proxy_v2_features['templates'].get('settings', {}) | ||
| template_url = settings.get('template_url') | ||
| assert template_url == 'http://quadlet.example.com:8000' | ||
|
|
There was a problem hiding this comment.
can we write a test that actually uses the template feature to get data?
| | `--add-feature bmc` | Enable BMC feature | `--foreman-proxy-bmc` | | ||
| | `--bmc-ipmi-implementation` | IPMI implementation to use for BMC | `--foreman-proxy-bmc-default-provider` | | ||
| | `--bmc-redfish-verify-ssl` | Verify SSL certificates for Redfish BMC connections | `--foreman-proxy-bmc-redfish-verify-ssl` | | ||
| | `--add-feature templates` | Enable Templates feature on Smart Proxy | `--foreman-proxy-templates` | |
There was a problem hiding this comment.
Can you please update the https://github.com/theforeman/foremanctl/blob/master/src/plugins/modules/migrate_answers.py#L14 as well?
| @@ -0,0 +1,3 @@ | |||
| --- | |||
| :enabled: {{ feature_enabled }} | |||
There was a problem hiding this comment.
What's the point of setting it to http or https? Why not just true / false?
Do we enforce SSL when the value is https or true?
| @@ -0,0 +1,3 @@ | |||
| --- | |||
| :enabled: {{ feature_enabled }} | |||
| :template_url: {{ foreman_proxy_templates_url | default('http://' + ansible_facts['fqdn'] + ':8000') }} No newline at end of file | |||
There was a problem hiding this comment.
Question: What if we did not provide a default value and instead required users to set it manually?
Plus, you need two default values: one for HTTP and one for HTTPS.
There was a problem hiding this comment.
No newline at end of file
That should be fixed as well.
| settings = proxy_v2_features['templates'].get('settings', {}) | ||
| template_url = settings.get('template_url') | ||
| assert template_url == 'http://quadlet.example.com:8000' | ||
|
|
| --add-feature remote-execution \ | ||
| --add-feature bmc \ | ||
| --add-feature templates \ | ||
| --templates-listen-on http \ |
There was a problem hiding this comment.
What if we drop it? Is this parameter still relevant?
Why are you introducing these changes? (Problem description, related links)
What are the changes introduced in this pull request?
How to test this pull request
Steps to reproduce:
Checklist