Fixes #38455 - Add container cert setup workflow to global registration #11402
Conversation
Reviewer's GuideThis PR integrates a new UI checkbox for toggling container registry certificate setup into the global registration workflow and exposes the corresponding backend flag by extending allowed registration variables and API parameters. Sequence Diagram for Global Registration with Container Cert SetupsequenceDiagram
actor User
participant RegistrationUI as "Registration UI"
participant BackendAPI as "Backend API"
participant Host
User->>RegistrationUI: Navigates to Global Registration Form
User->>RegistrationUI: Checks "Set up container registry certs" checkbox
User->>RegistrationUI: Clicks "Generate Command"
RegistrationUI->>BackendAPI: POST /api/v2/registration_commands/generate (with setup_container_registry_certs=true)
BackendAPI-->>RegistrationUI: Returns registration command
User->>Host: Executes registration command
Host->>BackendAPI: Registers with Foreman (standard process)
alt Command includes cert setup instruction
Host->>Host: Sets up /etc/containers/certs.d/ (soft links)
end
BackendAPI-->>Host: Registration confirmation (standard process)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @sjha4 - I've reviewed your changes - here's some feedback:
- Fix the typo in the SetupContainerRegistryCerts label ("Set up ctainer registry certs" → "Set up container registry certs").
- Remove the unused
pluginValuesprop passed into SetupContainerRegistryCerts since the component only uses value, onChange, and isLoading. - Make sure the registration command generator is updated to include the
--setup-container-registry-certsflag when the checkbox is selected.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
webpack/components/extensions/RegistrationCommands/fields/SetupContainerRegistryCerts.js
Outdated
Show resolved
Hide resolved
webpack/components/extensions/RegistrationCommands/fields/SetupContainerRegistryCerts.js
Outdated
Show resolved
Hide resolved
| @@ -31,6 +31,7 @@ module ApiPieExtensions | |||
| param :activation_keys, Array, desc: N_('Activation keys for subscription-manager client, required for CentOS and Red Hat Enterprise Linux. Required only if host group has no activation keys or if you do not provide a host group.') | |||
| param :force, :bool, required: false, desc: N_('Clear any previous registration and run subscription-manager with --force.') | |||
| param :ignore_subman_errors, :bool, required: false, desc: N_('Ignore subscription-manager errors for `subscription-manager register` command') | |||
| param :setup_container_registry_certs, :bool, required: false, desc: N_('Use container certificates for container registry authentication. If it is set to true, container registry certificates will be installed on the host') | |||
There was a problem hiding this comment.
Have we thought about also adding a host parameter for this too? Similar to what we do for Insights, where in the registration form for "Set up Insights" you can either inherit from the param, or override it with the registration option.
There was a problem hiding this comment.
I haven't really thought about making this a host parameter. Would it make sense here? The boolean basically controls whether we copy certs from entitlements to containers directory during this registration. It isn't reliable information after the fact given entitlement certs can change leaving container cert setup out of date.
There was a problem hiding this comment.
My main concern is how is the default set? If you have container repositories, you might want to set this to true for all new registered hosts. If you don't, you probably don't want to run this. And adding a param may be a good way for the user to express "this is my preference for all new hosts..". Or maybe we could automate that some other way. Just throwing thoughts out there.
There was a problem hiding this comment.
The default here would be false/nil. I'm not opposed to making this a host param eventually if the API param is not enough with the caveat that the host param value doesn't necessarily mean cert-auth is working on the host due to the nature of container cert auth. In the future, we can look at some way to have sub-man place the certs directly in the container certs directory so we don't have to update the links when entitlement certs change.
There was a problem hiding this comment.
Agree, can definitely add that later.
Oh I didn't think about that either.. When entitlement certs change, the filenames will also change so we would need to update the symlinks as well. Maybe a REX job will be needed for that.
There was a problem hiding this comment.
Ya..I am planning to reuse the existing flatpak rex template to do this: #11403
webpack/components/extensions/RegistrationCommands/fields/SetupContainerRegistryCerts.js
Outdated
Show resolved
Hide resolved
What are the changes introduced in this pull request?
Enable container registry certificate setup in the global registration workflow by adding a UI control and exposing the corresponding backend parameter.
New Features:
Enhancements:
What are the testing steps for this pull request? (Same steps as on the foreman PR)
Summary by Sourcery
Enable container registry certificate setup in the global registration workflow by adding a UI toggle and exposing the corresponding backend parameter.
New Features:
Enhancements: