Foreman Proxy #279
Foreman Proxy #279
Conversation
I know I suggested going this route to get started since the feature implementation is not flushed out, and having a dedicated command allows for easy testing and conceptualizing right now. And helps start off with a "clean" split between Foreman and smart-proxy. |
|
Ok. As long as the Ansible role prepares for extension then we can later include it with proper features support into the main deploy playbook |
stejskalleos
force-pushed
the
ls/proxy
branch
from
8a54989 to
002e9ae
Compare
| - name: Configure Foreman Proxy | ||
| theforeman.foreman.smart_proxy: |
There was a problem hiding this comment.
Shouldn't we consider configuring the foreman_proxy and its container, during foremanctl deploy time, since it doesn't create default proxy for the foreman deployment yet?
There was a problem hiding this comment.
Take a look at: #279 (comment)
And then, I'd be curious, why do you think deploy should include a foreman-proxy by default?
There was a problem hiding this comment.
I was checking the endpoint we use -> https://satellite.example.com/api/v2/smart_proxies with data {"search": "name=satellite.example.com"} and it returned empty results for the proxy present by default.
Also, I'm curious how we'd handle the external proxy ?
There was a problem hiding this comment.
Also, I'm curious how we'd handle the external proxy ?
In future PR, not this one.
There was a problem hiding this comment.
I tested the search in UI and it works fine, weird it doesn't work for API.
There was a problem hiding this comment.
Be careful, the first proxy is named satellite.example.com-pulp
stejskalleos
force-pushed
the
ls/proxy
branch
from
002e9ae to
ce9fc7c
Compare
|
ekohl
left a comment
ekohl
left a comment
There was a problem hiding this comment.
For configuration it might be time to dust off theforeman/smart-proxy#656 again. Not as something we need to get done before we release it, but keep it in mind.
| server_url: "{{ foreman_url }}" | ||
| username: "{{ foreman_initial_admin_username }}" | ||
| password: "{{ foreman_initial_admin_password }}" | ||
| validate_certs: false |
There was a problem hiding this comment.
IMHO we should always validate certificates. We should have them on the system already.
There was a problem hiding this comment.
On the system yes, but not in the system trust. you'll have to use ca_path parameter to the module (which we currently also don't do for the pulp proxy)
There was a problem hiding this comment.
Follow up issue for foremanctl 3.0?
There was a problem hiding this comment.
Clarification for the 3.0 ticket:
Do we want to support the ca_path parameter for the Ansible module, or do we want to add the CA to the trusted store?
There was a problem hiding this comment.
ca_path, we should not alter the trust store.
There was a problem hiding this comment.
| username: "{{ foreman_initial_admin_username }}" | ||
| password: "{{ foreman_initial_admin_password }}" |
There was a problem hiding this comment.
This is unreliable, since the user may change username & password after installation. The oauth credentials should be reliable.
There was a problem hiding this comment.
Our Ansible collection can't do OAutho tho.
There was a problem hiding this comment.
foremanctl/src/roles/foreman/tasks/main.yaml
Lines 225 to 226 in adfc046
There was a problem hiding this comment.
Oh, that's going to be some reliability issue down the line. We should track that somehow.
There was a problem hiding this comment.
stejskalleos
force-pushed
the
ls/proxy
branch
from
57ae275 to
c37721c
Compare
ekohl
left a comment
ekohl
left a comment
There was a problem hiding this comment.
Can I, as a user, run this on its own host and foremanctl foreman-proxy --foreman-url https://foreman.example.com or is that fully out of scope for now?
| @@ -0,0 +1,2 @@ | |||
| --- | |||
| :enabled: https | |||
There was a problem hiding this comment.
Do we need this to be done in foremanctl? I see 2 options:
- Properly templated in foremanctl where the user can enable/disable the module via a variable
- The container image defaults it to this and we don't override it at all
My preference would be to keep it simple and go for option 2.
There was a problem hiding this comment.
Right now the image does not do it by default, so should I move the logic from here to the image?
I can do it, but if we are going to manage the other features and their templates with foremanctl, IMHO, we can keep it as it is.
There was a problem hiding this comment.
I tend to agree. While logging is a tad special (as we're effectively abusing it for the proxy to work at all here), we'll be adding more modules soon, and those will be configurable, so… leave it here as is and (maybe) make it configurable later
| hostname: "{{ ansible_fqdn }}" | ||
| secrets: | ||
| - 'foreman-proxy-settings-yaml,type=mount,target=/etc/foreman-proxy/settings.yml' | ||
| - 'foreman-proxy-logs-yaml,type=mount,target=/etc/foreman-proxy/settings.d/logs.yml' |
There was a problem hiding this comment.
Not a blocking comment, but this is more a forward thinking discussion, but we know it's coming soon so it's best to start thinking about it.
Thinking more about this, how are we going to make it extensible? We know there will be many modules. Just to name a few off the top of my head:
- REX
- DHCP (with a provider, so at least 2 config files)
- DNS (also with a provider)
- TFTP
- Realm
Design wise we may want to split those out into into separate Ansible task files. How are we going to add the secrets? Will we use systemd drop in files to keep the container file here reasonably simple?
There was a problem hiding this comment.
I think /etc/containers/systemd/foreman-proxy.container.d/<feature>.conf makes a lot of sense for this.
Right now it's not exposed and I think should belong into a separate PR (especially as it implies having some way to transfer certificate bundles) |
| :bind_host: '*' | ||
|
|
||
| :log_level: INFO | ||
| :log_file: JOURNAL |
There was a problem hiding this comment.
I didn't expect this to work, but it does. Magic.
Is there any benefit over STDOUT (which will also work in the OpenShift case)?
There was a problem hiding this comment.
I don't know TBH, not really sure how this smart-proxy -> container -> journal relationship works under the hood, so I'll leave the final decision up to you
There was a problem hiding this comment.
Then I think I'd prefer the plain STDOUT
There was a problem hiding this comment.
Perhaps we can start with journal now and investigate whether Foreman Proxy can learn to autodetect it?
There was a problem hiding this comment.
There was a problem hiding this comment.
theforeman/smart-proxy#925 is what I was thinking about. Not something we need to sort out right now, but I'm starting to think about ways we can make foreman-proxy itself more container native.
stejskalleos
force-pushed
the
ls/proxy
branch
from
db1dd11 to
1cb9339
Compare
evgeni
left a comment
evgeni
left a comment
There was a problem hiding this comment.
Two last small things, then we can get this in.
Thanks!
foremanctl setup-foreman-proxy command for deploying Foreman Proxy.
stejskalleos
force-pushed
the
ls/proxy
branch
from
1cb9339 to
bc2641d
Compare
6 participants
No description provided.