feat: add Secret/ConfigMap references for JWT CA certificates#772
Open
bkhizgiy wants to merge 1 commit into
Open
feat: add Secret/ConfigMap references for JWT CA certificates#772bkhizgiy wants to merge 1 commit into
bkhizgiy wants to merge 1 commit into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughJWT authenticators can now reference CA PEM stored in Kubernetes Secrets or ConfigMaps. The operator resolves referenced CA PEM during controller config generation, inlines it into the generated controller ConfigMap, computes a deterministic hash of the ConfigMap data for pod-template restart annotations, and watches referenced Secrets/ConfigMaps to automatically reconcile on rotation. ChangesJWT CA Certificate References in Jumpstarter
Sequence DiagramsequenceDiagram
participant JumpstarterCR
participant ControllerReconcile
participant buildConfig
participant resolveJWTAuthenticators
participant KubernetesAPI
participant ControllerConfigMap
JumpstarterCR->>ControllerReconcile: trigger (watch/manual)
ControllerReconcile->>buildConfig: buildConfig(ctx, jumpstarter)
buildConfig->>resolveJWTAuthenticators: resolve JWT authenticators
loop per JWT entry with CA selector
resolveJWTAuthenticators->>KubernetesAPI: GET Secret or ConfigMap
KubernetesAPI-->>resolveJWTAuthenticators: resource data
resolveJWTAuthenticators->>resolveJWTAuthenticators: extract key, inline PEM into Issuer.CertificateAuthority
end
resolveJWTAuthenticators-->>buildConfig: resolved authenticators
buildConfig-->>ControllerReconcile: config with inlined PEM
ControllerReconcile->>ControllerReconcile: compute configMapDataHash from Data
ControllerReconcile->>ControllerConfigMap: create/update ConfigMap with hash annotation
Note over KubernetesAPI: CA Secret/ConfigMap rotation
KubernetesAPI->>ControllerReconcile: watch enqueue (Secret/ConfigMap watch)
ControllerReconcile->>buildConfig: rebuild config
buildConfig->>resolveJWTAuthenticators: resolve updated CA
resolveJWTAuthenticators-->>buildConfig: resolved with new PEM
ControllerReconcile->>ControllerConfigMap: update ConfigMap with rotated PEM
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
bkhizgiy
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
controller/deploy/operator/internal/controller/jumpstarter/jumpstarter_controller_test.go (1)
237-238: ⚡ Quick winStrengthen CA assertions to verify the expected PEM payload, not only a generic marker.
ContainSubstring("BEGIN CERTIFICATE")can pass even if the wrong certificate is rendered. AsserttestPEM(and after rotation,testPEM2plus absence oftestPEM) to lock the behavior to this feature.Proposed assertion updates
-Expect(getConfigData()).To(ContainSubstring("BEGIN CERTIFICATE")) +Expect(getConfigData()).To(ContainSubstring(testPEM))-Expect(getConfigData()).To(ContainSubstring("BEGIN CERTIFICATE")) +Expect(getConfigData()).To(ContainSubstring(testPEM))Expect(secondConfig).To(ContainSubstring("BEGIN CERTIFICATE")) -Expect(secondConfig).NotTo(Equal(firstConfig)) +Expect(secondConfig).To(ContainSubstring(testPEM2)) +Expect(secondConfig).NotTo(ContainSubstring(testPEM)) +Expect(secondConfig).NotTo(Equal(firstConfig))Also applies to: 278-279, 369-370
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controller/deploy/operator/internal/controller/jumpstarter/jumpstarter_controller_test.go` around lines 237 - 238, Replace the weak substring assertions against certificate output with exact PEM-value checks: in the test case(s) that call getConfigData() (and any related checks after rotation), assert that the returned string contains the expected test PEM payload variable testPEM; for rotation scenarios assert it contains testPEM2 and does not contain testPEM. Locate assertions around getConfigData() and update them to use positive containment of testPEM / testPEM2 and negative containment (Not/ToNot) for the old PEM after rotation so the tests validate the actual certificate payload rather than just "BEGIN CERTIFICATE".controller/deploy/operator/internal/controller/jumpstarter/ca_resolution_test.go (1)
323-326: ⚡ Quick winAdd assertion for
"jwt[0]"in the error message to maintain consistency.All other error test cases verify that the error message includes the JWT entry index (e.g.,
"jwt[0]"). This test should follow the same pattern for consistency and to confirm that error messages properly identify which JWT entry caused the failure.✅ Suggested addition
_, err := reconciler.resolveJWTAuthenticators(ctx, js) Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("jwt[0]")) Expect(err.Error()).To(ContainSubstring(`key "wrong-key" not found`)) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controller/deploy/operator/internal/controller/jumpstarter/ca_resolution_test.go` around lines 323 - 326, Update the test assertion in ca_resolution_test.go that calls reconciler.resolveJWTAuthenticators(ctx, js) so the expected error message includes the JWT index; specifically, adjust the Expect(err.Error()).To(ContainSubstring(...)) to assert the error contains both `jwt[0]` and the existing `key "wrong-key" not found` fragment (i.e., verify the message references "jwt[0]" as well as the missing key) so resolveJWTAuthenticators' error identification remains consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@controller/deploy/operator/api/v1alpha1/jumpstarter_types.go`:
- Around line 402-425: The current cross-namespace references (fields Namespace
on Secret/ConfigMap selectors defined in types ConfigMapKeySelector and the
Secret selector that backs
certificateAuthoritySecret/certificateAuthorityConfigMap) allow arbitrary
Jumpstarter CRs to point at Secrets/ConfigMaps in other namespaces; update the
API/behavior to prevent silent cross-namespace data exfiltration by either (a)
restricting the Namespace fields so selectors are only valid when equal to the
Jumpstarter CR namespace (remove or validate external Namespace values in the
ConfigMapKeySelector and Secret selector parsing), or (b) add an explicit
authorization/allowlist check in the resolver (the code path in
controller/deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.go
that reads certificateAuthoritySecret/certificateAuthorityConfigMap) to verify
the caller is permitted to read the target namespace before copying bytes;
ensure the change references ConfigMapKeySelector, the Secret selector struct,
and the certificateAuthoritySecret/certificateAuthorityConfigMap handling so
reviewers can find and update both the type validation and the controller
resolver logic.
In
`@controller/deploy/operator/internal/controller/jumpstarter/jumpstarter_controller_test.go`:
- Around line 142-160: The shared test fixture makeJumpstarterSpec returns a
JumpstarterSpec but omits cert-manager configuration; update makeJumpstarterSpec
so the returned JumpstarterSpec has CertManager.Enabled = false (i.e., set
spec.CertManager.Enabled to false on the returned struct) to ensure
envtest-compatible setup for JWT tests and avoid unrelated reconciliation
failures.
In
`@controller/deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.go`:
- Around line 170-185: You built the desired ConfigMap early (createConfigMap
and configMapDataHash) which is fine for computing the pod template hash, but
you must not call reconcileConfigMaps yet—move the call to reconcileConfigMaps
so it runs after Services/networking reconciliation and before Secrets
reconciliation (i.e., keep createConfigMap and configMapDataHash where they are,
but relocate the reconcileConfigMaps(...) invocation to the documented phase
between reconcileServices/reconcileNetworking and reconcileSecrets) to restore
the required reconcile ordering for this controller.
In `@controller/deploy/operator/test/e2e/e2e_test.go`:
- Around line 2028-2058: The test currently only checks that the controller
ConfigMap changed and contains a certificate; update the eventual assertion
block (the code that fetches updatedCM) to also verify the actual rotated CA PEM
was applied by asserting that updatedCM.Data["config"] contains the rotatedPEM
value (or its string form) you set on the jwt-ca-secret; reference the
rotatedPEM variable and updatedCM.Data["config"] in the assertion so the test
fails if the ConfigMap was changed for unrelated reasons.
- Around line 1967-2026: The Jumpstarter test is missing required controller and
router fields so the operator cannot reconcile; update the Jumpstarter spec in
the test (the js variable in the It block that creates the Jumpstarter) to
include controller.image, controller.replicas, controller.grpc.endpoints and a
routers section matching other e2e tests so the controller Deployment and
ConfigMap are created, and replace the weak assertion that only checks for
"BEGIN CERTIFICATE" with an exact containment check for the fakePEM variable
(verify cm.Data["config"] contains fakePEM) to ensure the CA PEM was inlined.
---
Nitpick comments:
In
`@controller/deploy/operator/internal/controller/jumpstarter/ca_resolution_test.go`:
- Around line 323-326: Update the test assertion in ca_resolution_test.go that
calls reconciler.resolveJWTAuthenticators(ctx, js) so the expected error message
includes the JWT index; specifically, adjust the
Expect(err.Error()).To(ContainSubstring(...)) to assert the error contains both
`jwt[0]` and the existing `key "wrong-key" not found` fragment (i.e., verify the
message references "jwt[0]" as well as the missing key) so
resolveJWTAuthenticators' error identification remains consistent.
In
`@controller/deploy/operator/internal/controller/jumpstarter/jumpstarter_controller_test.go`:
- Around line 237-238: Replace the weak substring assertions against certificate
output with exact PEM-value checks: in the test case(s) that call
getConfigData() (and any related checks after rotation), assert that the
returned string contains the expected test PEM payload variable testPEM; for
rotation scenarios assert it contains testPEM2 and does not contain testPEM.
Locate assertions around getConfigData() and update them to use positive
containment of testPEM / testPEM2 and negative containment (Not/ToNot) for the
old PEM after rotation so the tests validate the actual certificate payload
rather than just "BEGIN CERTIFICATE".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b711568e-26cd-4a30-aa39-2a76ba69bba0
📒 Files selected for processing (7)
controller/deploy/operator/api/v1alpha1/jumpstarter_types.gocontroller/deploy/operator/api/v1alpha1/zz_generated.deepcopy.gocontroller/deploy/operator/config/crd/bases/operator.jumpstarter.dev_jumpstarters.yamlcontroller/deploy/operator/internal/controller/jumpstarter/ca_resolution_test.gocontroller/deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.gocontroller/deploy/operator/internal/controller/jumpstarter/jumpstarter_controller_test.gocontroller/deploy/operator/test/e2e/e2e_test.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
controller/deploy/operator/config/crd/bases/operator.jumpstarter.dev_jumpstarters.yaml (2)
104-130: ⚡ Quick winClarify which ConfigMap gets updated on line 109.
Line 109 states "updates the ConfigMap" which is ambiguous—it could refer to the referenced ConfigMap or the controller's generated ConfigMap. Clarify that the operator updates the controller ConfigMap (the generated configuration), not the referenced ConfigMap.
Suggestion: "When the ConfigMap changes, the operator reconciles and updates the controller ConfigMap."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controller/deploy/operator/config/crd/bases/operator.jumpstarter.dev_jumpstarters.yaml` around lines 104 - 130, The description for certificateAuthorityConfigMap is ambiguous about which ConfigMap is updated; update the sentence in the certificateAuthorityConfigMap description (in operator.jumpstarter.dev_jumpstarters.yaml, inside the certificateAuthorityConfigMap property) to explicitly state that the operator reconciles and updates the controller ConfigMap (the generated configuration) rather than the referenced ConfigMap—for example, replace "When the ConfigMap changes, the operator reconciles and updates the ConfigMap." with a sentence like "When the referenced ConfigMap changes, the operator reconciles and updates the controller ConfigMap (the generated configuration)."
131-157: ⚡ Quick winClarify which ConfigMap gets updated on line 136.
Line 136 has the same ambiguity as line 109: "updates the ConfigMap" should specify that the operator updates the controller ConfigMap (the generated configuration), not the referenced Secret.
Suggestion: "When the Secret changes, the operator reconciles and updates the controller ConfigMap."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controller/deploy/operator/config/crd/bases/operator.jumpstarter.dev_jumpstarters.yaml` around lines 131 - 157, Update the description for the certificateAuthoritySecret object (property certificateAuthoritySecret) to explicitly state that when the referenced Secret changes the operator reconciles and updates the controller ConfigMap (the generated controller configuration), not the referenced Secret; replace the ambiguous phrase "updates the ConfigMap" with "updates the controller ConfigMap" and ensure the wording parallels the existing CertificateAuthorityConfigMap description for consistency with Jumpstarter CR and controller behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@controller/deploy/operator/config/crd/bases/operator.jumpstarter.dev_jumpstarters.yaml`:
- Around line 104-130: The description for certificateAuthorityConfigMap is
ambiguous about which ConfigMap is updated; update the sentence in the
certificateAuthorityConfigMap description (in
operator.jumpstarter.dev_jumpstarters.yaml, inside the
certificateAuthorityConfigMap property) to explicitly state that the operator
reconciles and updates the controller ConfigMap (the generated configuration)
rather than the referenced ConfigMap—for example, replace "When the ConfigMap
changes, the operator reconciles and updates the ConfigMap." with a sentence
like "When the referenced ConfigMap changes, the operator reconciles and updates
the controller ConfigMap (the generated configuration)."
- Around line 131-157: Update the description for the certificateAuthoritySecret
object (property certificateAuthoritySecret) to explicitly state that when the
referenced Secret changes the operator reconciles and updates the controller
ConfigMap (the generated controller configuration), not the referenced Secret;
replace the ambiguous phrase "updates the ConfigMap" with "updates the
controller ConfigMap" and ensure the wording parallels the existing
CertificateAuthorityConfigMap description for consistency with Jumpstarter CR
and controller behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6b13e090-5124-4256-80d2-97b59f7781d2
📒 Files selected for processing (8)
controller/deploy/operator/api/v1alpha1/jumpstarter_types.gocontroller/deploy/operator/api/v1alpha1/zz_generated.deepcopy.gocontroller/deploy/operator/config/crd/bases/operator.jumpstarter.dev_jumpstarters.yamlcontroller/deploy/operator/internal/controller/jumpstarter/ca_resolution_test.gocontroller/deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.gocontroller/deploy/operator/internal/controller/jumpstarter/jumpstarter_controller_test.gocontroller/deploy/operator/test/e2e/e2e_test.gotypos.toml
✅ Files skipped from review due to trivial changes (2)
- typos.toml
- controller/deploy/operator/api/v1alpha1/zz_generated.deepcopy.go
🚧 Files skipped from review as they are similar to previous changes (5)
- controller/deploy/operator/test/e2e/e2e_test.go
- controller/deploy/operator/api/v1alpha1/jumpstarter_types.go
- controller/deploy/operator/internal/controller/jumpstarter/jumpstarter_controller_test.go
- controller/deploy/operator/internal/controller/jumpstarter/ca_resolution_test.go
- controller/deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@controller/deploy/operator/internal/controller/jumpstarter/jumpstarter_controller_test.go`:
- Around line 206-245: Add a string constant named testPEM containing a
PEM-formatted certificate (including the "-----BEGIN CERTIFICATE-----" header)
and place it near the top of the test suite (e.g., after the other test-level
declarations) so the reference in the test that creates the Secret (which uses
caSecretName and caKey) compiles; ensure the constant value includes the full
PEM delimiters and newlines so the
Expect(getConfigData()).To(ContainSubstring("BEGIN CERTIFICATE")) assertion
passes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 508ccc00-0fa6-4c22-99a2-ca3117a3633b
📒 Files selected for processing (8)
controller/deploy/operator/api/v1alpha1/jumpstarter_types.gocontroller/deploy/operator/api/v1alpha1/zz_generated.deepcopy.gocontroller/deploy/operator/config/crd/bases/operator.jumpstarter.dev_jumpstarters.yamlcontroller/deploy/operator/internal/controller/jumpstarter/ca_resolution_test.gocontroller/deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.gocontroller/deploy/operator/internal/controller/jumpstarter/jumpstarter_controller_test.gocontroller/deploy/operator/test/e2e/e2e_test.gotypos.toml
✅ Files skipped from review due to trivial changes (2)
- typos.toml
- controller/deploy/operator/api/v1alpha1/zz_generated.deepcopy.go
🚧 Files skipped from review as they are similar to previous changes (5)
- controller/deploy/operator/test/e2e/e2e_test.go
- controller/deploy/operator/config/crd/bases/operator.jumpstarter.dev_jumpstarters.yaml
- controller/deploy/operator/api/v1alpha1/jumpstarter_types.go
- controller/deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.go
- controller/deploy/operator/internal/controller/jumpstarter/ca_resolution_test.go
10 tasks
mangelajo
reviewed
Jun 19, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@controller/deploy/operator/internal/controller/jumpstarter/ca_resolution_test.go`:
- Around line 43-67: Replace the fake.NewClientBuilder() approach with the
envtest-backed client that should be initialized in the test setup. Move the
Secret (local-ca-secret) and ConfigMap (local-ca-cm) object creation from the
fake client builder into a BeforeEach block where they should be created and
seeded using the actual envtest API client. Remove the fake.NewClientBuilder()
call entirely and ensure test cases use the envtest client configured in the
test environment instead.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 93d188b5-55e8-4569-99e5-18cefd0be791
📒 Files selected for processing (9)
controller/deploy/operator/api/v1alpha1/jumpstarter_types.gocontroller/deploy/operator/api/v1alpha1/zz_generated.deepcopy.gocontroller/deploy/operator/config/crd/bases/operator.jumpstarter.dev_jumpstarters.yamlcontroller/deploy/operator/internal/controller/jumpstarter/ca_resolution_test.gocontroller/deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.gocontroller/deploy/operator/internal/controller/jumpstarter/jumpstarter_controller_test.gocontroller/deploy/operator/internal/controller/jumpstarter/testdata_test.gocontroller/deploy/operator/test/e2e/e2e_test.gotypos.toml
✅ Files skipped from review due to trivial changes (2)
- controller/deploy/operator/internal/controller/jumpstarter/testdata_test.go
- typos.toml
🚧 Files skipped from review as they are similar to previous changes (4)
- controller/deploy/operator/config/crd/bases/operator.jumpstarter.dev_jumpstarters.yaml
- controller/deploy/operator/internal/controller/jumpstarter/jumpstarter_controller_test.go
- controller/deploy/operator/test/e2e/e2e_test.go
- controller/deploy/operator/api/v1alpha1/zz_generated.deepcopy.go
Signed-off-by: Bella Khizgiyaev <bkhizgiy@redhat.com> Assisted-by: Claude Sonnet 4.6 <claude@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add support for referencing OIDC CA certificates from secrets and configMaps
Extend the jumpstarter CRD's JWT authentication configuration to allow CA certificates to be sourced from kubernetes secrets or configMaps, rather than requiring inline PEM content. The operator resolves these references at reconcile time and inlines the PEM into the controller configMap.
Watches are registered on the referenced secrets/configMaps so that a CA rotation automatically triggers a reconcile and updates the running configuration without any manual intervention.
related to #646