Fix rex_pub_key retrieval for containerized Satellite - #22139
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjusts rex public key retrieval to support foremanctl/foreman-proxy running in a container and adds validation that the retrieved key is non-empty. Flow diagram for updated rex_pub_key retrieval logicflowchart TD
A[Call rex_pub_key] --> B{install_method == InstallMethod.FOREMANCTL}
B -->|yes| C[execute podman exec foreman-proxy cat /var/lib/foreman-proxy/ssh/id_rsa_foreman_proxy.pub]
B -->|no| D[execute cat rex_key_path]
C --> E[Strip result.stdout to key]
D --> E
E --> F{key is empty?}
F -->|yes| G[raise ValueError with status and stderr]
F -->|no| H[return key]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The hard-coded path and container name in the FOREMANCTL branch (
foreman-proxyand/var/lib/foreman-proxy/ssh/id_rsa_foreman_proxy.pub) might benefit from being configurable or derived from existing settings to avoid breakage if these values change. - Consider moving the
InstallMethodimport to the module level to avoid repeated imports and potential circular import issues whenrex_pub_keyis accessed frequently.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The hard-coded path and container name in the FOREMANCTL branch (`foreman-proxy` and `/var/lib/foreman-proxy/ssh/id_rsa_foreman_proxy.pub`) might benefit from being configurable or derived from existing settings to avoid breakage if these values change.
- Consider moving the `InstallMethod` import to the module level to avoid repeated imports and potential circular import issues when `rex_pub_key` is accessed frequently.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
1cd1604 to
b41e5c1
Compare
b41e5c1 to
7829c3c
Compare
|
Do I need another ack on this @vijaysawant or is this ok to merge? Not sure about the whole review process here.. |
1eb79c0 to
d5905df
Compare
f5c3781 to
eab9301
Compare
eab9301 to
00406f5
Compare
| "podman secret inspect" | ||
| " foreman_proxy-remote_execution_ssh-id_rsa_foreman_proxy-pub" | ||
| " --showsecret --format '{{.SecretData}}'" | ||
| ) |
There was a problem hiding this comment.
@Gauravtalreja1 I think fetching this from podman secret makes most sense in case of foremanctl rather than relying on the cat.. We can get around reading files from mounted paths entirely with this.
There was a problem hiding this comment.
Good suggestion! I don't have a strong opinion either way, but I'd lean toward sticking with podman exec cat .. here, it mirrors the existing installer pattern and keeps the code readable and consistent across both install methods.
Additionally, I believe the podman secret name is an implementation detail that could change in future, while key path would be consistent.
CC @evgeni curious to hear your opinion as well on this.
There was a problem hiding this comment.
Is there a reason not to use the APIs we have (Foreman's /api/smart_proxies which contains the key in remote_execution_pubkey or the Smart Proxy API at /ssh/pubkey) to retrieve the key?
There was a problem hiding this comment.
@sjha4 I'm not entirely familiar with this API endpoint and don't have a strong opinion against it, but if it works, it would be a much cleaner solution than checking for the public key with two different install-methods
| def rex_pub_key(self): | ||
| return self.execute(f'cat {self.rex_key_path}').stdout.strip() | ||
| if settings.server.install_method == InstallMethod.FOREMANCTL: | ||
| if 'remote-execution' in self.list_foremanctl_features(enabled=True): |
There was a problem hiding this comment.
rex is always enabled on sat: theforeman/foremanctl#628
There was a problem hiding this comment.
Its not enabled today how we deploy without flavor, but I believe its good to have that check in place as its quick one
There was a problem hiding this comment.
Are there plans for something similar with REX enabled on capsule/smart proxies with deploy-proxy. The current default flavor https://github.com/theforeman/foremanctl/blob/master/src/vars/flavors/foreman-proxy-content.yml doesn't have it.
There was a problem hiding this comment.
yes, maybe in future we'll have dedicate capsule flavor which will have this enabled by default, but I think we do need a quick check today to see if its enabled then fetch pubkey
00406f5 to
45e9bb9
Compare
|
@Gauravtalreja1 With the last push, I have taken your suggestion and tested the existing path ~foreman-proxy/.ssh/id_rsa_foreman_proxy.pub in both installs..For @Evegeni's question I don't know why we don't use the APIs, not sure if it was written this way for readability or cause the endpoints differ for sat and capsule and the method to get key fits both? |
|
You can get the key of every capsule from the sat. |
|
@evgeni Ack..The /ssh/pubkey is promising. @Gauravtalreja1 , @vsedmik , @vijaysawant Thoughts on this? We could refactor the logic for rex_pub_key to grab the rex public key from API at |
I'm not sure about the why either, I haven't been working with REX too often, guessing it just seemed more straightforward. @pondrejk might know more. |
|
@sjha4 I'm not entirely familiar with this API endpoint and don't have a strong opinion against it, but if it works, I think it would be a much cleaner solution than checking for the public key with two different install-methods. That said, it's up to you whether you'd like to include this refactor in the same PR or handle it separately. |
45e9bb9 to
e83caa3
Compare
|
@Gauravtalreja1 I prefer merging this with the old rex_key_path approach which has acks and I will open a fresh PR for the refactoring unless there are strong objections. |
Gauravtalreja1
left a comment
There was a problem hiding this comment.
@sjha4 ACK, I'll merge even without PRT results this time since there are no REX tests to run as the feature is disabled by default in the current containerized deployments
Problem Statement
Update REX public key retrieval to handle foremanctl foreman-proxy container
Solution
Use
podman exec foreman-proxy cat /var/lib/foreman-proxy/ssh/id_rsa_foreman_proxy.pubto retrieve public key contents.Related Issues
Summary by Sourcery
Handle REX public key retrieval correctly for containerized Satellite installations and validate that the retrieved key is not empty.
Bug Fixes:
Enhancements: