Store ephemeral cluster credentials in a Secret instead of the status#5124
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 3 minutes and 55 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
WalkthroughThe pull request updates the EphemeralCluster API to consolidate credential management by replacing direct Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/controller/ephemeralcluster/reconciler_test.go (1)
1352-1363: Consider also asserting the credentials Secret's OwnerReference.The test fetches the Secret and diffs only
Data. Given that the whole motivation of this PR is "the Secret is owned by the EphemeralCluster to enable automatic cleanup," it would be valuable to assert that the created Secret carries the expected controller ownerref pointing at the EC (especially if the [SetControllerReference/ownership-verification changes suggested onreconciler.go] land). This prevents silent regressions where ownership is dropped.♻️ Example addition
if tc.wantSecret != nil { gotSecret := corev1.Secret{} if err := client.Get(context.TODO(), types.NamespacedName{ Name: tc.wantSecret.Name, Namespace: tc.wantSecret.Namespace, }, &gotSecret); err != nil { t.Fatalf("get credentials secret: %s", err) } if diff := cmp.Diff(tc.wantSecret.Data, gotSecret.Data); diff != "" { t.Errorf("unexpected credentials secret data: %s", diff) } + if !metav1.IsControlledBy(&gotSecret, tc.ec) { + t.Errorf("credentials secret %s/%s is not controlled by ephemeralcluster %s", + gotSecret.Namespace, gotSecret.Name, tc.ec.Name) + } }(Note: the test ECs would need to be instantiated with a non-empty
UIDforIsControlledByto work against the fake client, since the fake client does not always populate UIDs onWithObjects.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/ephemeralcluster/reconciler_test.go` around lines 1352 - 1363, Test currently compares only Secret.Data (tc.wantSecret vs gotSecret) and misses asserting the Secret's OwnerReference; update the test in reconciler_test.go to also verify the Secret is owned by the EphemeralCluster by checking gotSecret.OwnerReferences (or using metav1.IsControlledBy against the EphemeralCluster instance used in the test), ensuring the expected controller ownerRef (controller=true, UID matching the EC UID) is present; if necessary, ensure the fake EphemeralCluster object used in the test has a non-empty UID so IsControlledBy/ownerRef comparisons succeed, and reference tc.wantSecret, gotSecret, and the EphemeralCluster test object when making the assertion.pkg/controller/ephemeralcluster/reconciler.go (1)
619-624: Usecontrollerutil.SetControllerReferenceinstead of hand-crafted OwnerReference.The current approach hard-codes the GVK (
ci.openshift.io/v1,EphemeralCluster) and omits theController: trueandBlockOwnerDeletion: truefields. WithoutController: true, another controller can claim ownership of this Secret without collision, andmetav1.IsControlledBywill return false.SetControllerReferenceresolves the GVK from the scheme and sets both fields properly, following the pattern already used elsewhere in the codebase (e.g.,multiarchbuildconfig.go).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/ephemeralcluster/reconciler.go` around lines 619 - 624, Replace the hand-crafted OwnerReferences block with a call to controllerutil.SetControllerReference to let the scheme resolve the GVK and set Controller and BlockOwnerDeletion properly; specifically, in the code that currently sets OwnerReferences for the Secret, call controllerutil.SetControllerReference(ec, secret, r.scheme) (or r.Scheme if that’s the reconciler field name) and remove the manual APIVersion/Kind/Name/UID assignment, ensuring the controller-runtime controllerutil package is imported.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/controller/ephemeralcluster/reconciler_test.go`:
- Around line 1352-1363: Test currently compares only Secret.Data (tc.wantSecret
vs gotSecret) and misses asserting the Secret's OwnerReference; update the test
in reconciler_test.go to also verify the Secret is owned by the EphemeralCluster
by checking gotSecret.OwnerReferences (or using metav1.IsControlledBy against
the EphemeralCluster instance used in the test), ensuring the expected
controller ownerRef (controller=true, UID matching the EC UID) is present; if
necessary, ensure the fake EphemeralCluster object used in the test has a
non-empty UID so IsControlledBy/ownerRef comparisons succeed, and reference
tc.wantSecret, gotSecret, and the EphemeralCluster test object when making the
assertion.
In `@pkg/controller/ephemeralcluster/reconciler.go`:
- Around line 619-624: Replace the hand-crafted OwnerReferences block with a
call to controllerutil.SetControllerReference to let the scheme resolve the GVK
and set Controller and BlockOwnerDeletion properly; specifically, in the code
that currently sets OwnerReferences for the Secret, call
controllerutil.SetControllerReference(ec, secret, r.scheme) (or r.Scheme if
that’s the reconciler field name) and remove the manual APIVersion/Kind/Name/UID
assignment, ensuring the controller-runtime controllerutil package is imported.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d2b68d1d-5bb1-4bf1-a352-658f1f62af93
📒 Files selected for processing (4)
pkg/api/ephemeralcluster/v1/ci.openshift.io_ephemeralclusters.yamlpkg/api/ephemeralcluster/v1/types.gopkg/controller/ephemeralcluster/reconciler.gopkg/controller/ephemeralcluster/reconciler_test.go
The kubeconfig and kubeAdminPassword were previously written directly to the EphemeralCluster status, exposing sensitive credentials in a resource that may be logged or cached. Instead, create a credentials Secret in the same namespace as the EphemeralCluster and reference it via a new status.secretRef field. The Secret is owned by the EphemeralCluster for automatic cleanup. Assisted-by: Claude claude-opus-4-6 Signed-off-by: amisstea <amisstea@redhat.com>
|
/assign @danilo-gemoli The corresponding change on the Konflux side is in konflux-ci/crossplane-control-plane#81. |
|
Scheduling tests matching the |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
3 similar comments
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
|
||
| existing := corev1.Secret{} | ||
| err := r.masterClient.Get(ctx, types.NamespacedName{Name: secretName, Namespace: ec.Namespace}, &existing) | ||
| if err != nil && !kerrors.IsNotFound(err) { |
There was a problem hiding this comment.
Maybe it's me, but I find this variation easier to reason about (even though it's a bit more nested):
if err != nil {
if !kerrors.IsNotFound(err) {
return fmt.Errorf("get credentials secret: %w", err)
} else {
if !v1.IsControlledBy(&existing, ec) {
return fmt.Errorf("credentials secret %s/%s exists but is not owned by ephemeralcluster %s (uid=%s)",
ec.Namespace, secretName, ec.Name, ec.UID)
}
return nil
}
}
|
/lgtm |
|
/override ci/prow/e2e |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: amisstea, danilo-gemoli The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
@danilo-gemoli: Overrode contexts on behalf of danilo-gemoli: ci/prow/e2e DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/override ci/prow/images |
|
@danilo-gemoli: Overrode contexts on behalf of danilo-gemoli: ci/prow/images DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@amisstea: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
openshift-merge-bot
Bot
merged commit 0dc0fdf
into
openshift:main
2 participants
The kubeconfig and kubeAdminPassword were previously written directly to the EphemeralCluster status, exposing sensitive credentials in a resource that may be logged or cached. Instead, create a credentials Secret in the same namespace as the EphemeralCluster and reference it via a new status.secretRef field. The Secret is owned by the EphemeralCluster for automatic cleanup.
Jira: https://redhat.atlassian.net/browse/KFLUXVNGD-899
Assisted-by: Claude claude-opus-4-6
Summary by CodeRabbit
New Features
secretRef) for storing credentials instead of direct fields in the specification and status. Credentials are consolidated into a single Secret resource in the same namespace containingkubeconfigand optionalkubeAdminPasswordkeys.Chores