feat: add requireExistingSecret for OAuth2Client - #305
Open
matzegebbe wants to merge 2 commits into
Open
Conversation
Signed-off-by: Mathias Gebbe <mgebbe@hellmann.com>
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change adds controller-wide and per-resource settings for externally managed OAuth2Client secrets. Missing secrets produce pending status and exponential requeues. The API, CRD, CLI, environment configuration, sample manifest, documentation, and integration tests cover the new behavior. ChangesExternally managed secret support
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant OAuth2ClientReconciler
participant KubernetesAPI
participant Hydra
OAuth2ClientReconciler->>KubernetesAPI: Read referenced Secret
alt Secret is missing and existing secret is required
OAuth2ClientReconciler->>KubernetesAPI: Set SECRET_NOT_FOUND pending status
OAuth2ClientReconciler->>OAuth2ClientReconciler: Schedule exponential requeue
else Secret is available
OAuth2ClientReconciler->>Hydra: Register OAuth2Client with supplied credentials
OAuth2ClientReconciler->>KubernetesAPI: Update OAuth2Client status
end
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 2
🤖 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 `@controllers/oauth2client_controller.go`:
- Around line 499-516: Update updateReconciliationStatusPending to mark
ObservedGeneration stale whenever a resource enters pending status, or adjust
the reconciliation early-return guard to proceed when ReconciliationError is
non-empty. Add a regression test covering a ready resource losing its Secret and
recovering after the Secret returns, verifying the error clears and Ready
becomes true.
In `@README.md`:
- Line 73: Update the hydra-url example in the README table to remove the
leading space from the sample value, ensuring copied values begin directly with
the Hydra hostname.
🪄 Autofix
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 Plus
Run ID: f9d50670-34ab-419c-b4dc-7af730d72314
📒 Files selected for processing (8)
README.mdapi/v1alpha1/oauth2client_types.goapi/v1alpha1/zz_generated.deepcopy.goconfig/crd/bases/hydra.ory.sh_oauth2clients.yamlconfig/samples/hydra_v1alpha1_oauth2client_external_secret.yamlcontrollers/oauth2client_controller.gocontrollers/oauth2client_controller_integration_test.gomain.go
Comment on lines
+499
to
+516
| // updateReconciliationStatusPending reports that reconciliation has not finished | ||
| // yet and will be retried. Unlike updateReconciliationStatusError it leaves | ||
| // ObservedGeneration untouched, so the current generation is still reconciled | ||
| // from scratch once the blocker is gone. | ||
| func (r *OAuth2ClientReconciler) updateReconciliationStatusPending(ctx context.Context, c *hydrav1alpha1.OAuth2Client, code hydrav1alpha1.StatusCode, err error) error { | ||
| _, updateErr := controllerutil.CreateOrPatch(ctx, r.Client, c, func() error { | ||
| c.Status.ReconciliationError = hydrav1alpha1.ReconciliationError{ | ||
| Code: code, | ||
| Description: err.Error(), | ||
| } | ||
| c.Status.Conditions = []hydrav1alpha1.OAuth2ClientCondition{ | ||
| { | ||
| Type: hydrav1alpha1.OAuth2ClientConditionReady, | ||
| Status: hydrav1alpha1.ConditionFalse, | ||
| }, | ||
| } | ||
|
|
||
| return nil |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep ObservedGeneration stale while the resource is pending.
If a ready resource later loses its Secret, ObservedGeneration already equals Generation. This method preserves that equality. When the Secret returns, the early return at lines 258-262 skips updateRegisteredOAuth2Client, so SECRET_NOT_FOUND and Ready=False remain until the spec changes.
Mark the observed generation as stale in the pending status, or make the early return require an empty reconciliation error. Add a regression test for this recovery path.
Proposed fix
func (r *OAuth2ClientReconciler) updateReconciliationStatusPending(ctx context.Context, c *hydrav1alpha1.OAuth2Client, code hydrav1alpha1.StatusCode, err error) error {
_, updateErr := controllerutil.CreateOrPatch(ctx, r.Client, c, func() error {
+ if c.Status.ObservedGeneration >= c.Generation {
+ c.Status.ObservedGeneration = c.Generation - 1
+ }
c.Status.ReconciliationError = hydrav1alpha1.ReconciliationError{📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // updateReconciliationStatusPending reports that reconciliation has not finished | |
| // yet and will be retried. Unlike updateReconciliationStatusError it leaves | |
| // ObservedGeneration untouched, so the current generation is still reconciled | |
| // from scratch once the blocker is gone. | |
| func (r *OAuth2ClientReconciler) updateReconciliationStatusPending(ctx context.Context, c *hydrav1alpha1.OAuth2Client, code hydrav1alpha1.StatusCode, err error) error { | |
| _, updateErr := controllerutil.CreateOrPatch(ctx, r.Client, c, func() error { | |
| c.Status.ReconciliationError = hydrav1alpha1.ReconciliationError{ | |
| Code: code, | |
| Description: err.Error(), | |
| } | |
| c.Status.Conditions = []hydrav1alpha1.OAuth2ClientCondition{ | |
| { | |
| Type: hydrav1alpha1.OAuth2ClientConditionReady, | |
| Status: hydrav1alpha1.ConditionFalse, | |
| }, | |
| } | |
| return nil | |
| // updateReconciliationStatusPending reports that reconciliation has not finished | |
| // yet and will be retried. Unlike updateReconciliationStatusError it leaves | |
| // ObservedGeneration untouched, so the current generation is still reconciled | |
| // from scratch once the blocker is gone. | |
| func (r *OAuth2ClientReconciler) updateReconciliationStatusPending(ctx context.Context, c *hydrav1alpha1.OAuth2Client, code hydrav1alpha1.StatusCode, err error) error { | |
| _, updateErr := controllerutil.CreateOrPatch(ctx, r.Client, c, func() error { | |
| if c.Status.ObservedGeneration >= c.Generation { | |
| c.Status.ObservedGeneration = c.Generation - 1 | |
| } | |
| c.Status.ReconciliationError = hydrav1alpha1.ReconciliationError{ | |
| Code: code, | |
| Description: err.Error(), | |
| } | |
| c.Status.Conditions = []hydrav1alpha1.OAuth2ClientCondition{ | |
| { | |
| Type: hydrav1alpha1.OAuth2ClientConditionReady, | |
| Status: hydrav1alpha1.ConditionFalse, | |
| }, | |
| } | |
| return nil |
🤖 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 `@controllers/oauth2client_controller.go` around lines 499 - 516, Update
updateReconciliationStatusPending to mark ObservedGeneration stale whenever a
resource enters pending status, or adjust the reconciliation early-return guard
to proceed when ReconciliationError is non-empty. Add a regression test covering
a ready resource losing its Secret and recovering after the Secret returns,
verifying the error clears and Ready becomes true.
| | **leader-elector-namespace** | no | Leader elector namespace where controller should be set. | `""` | `"my-namespace"` | | ||
| | Name | Required | Description | Default value | Example values | | ||
| | ---------------------------- | -------- | ------------------------------------------------------------------------------------------------------------------------------------------- | ------------- | ---------------------------------------- | | ||
| | **hydra-url** | yes | ORY Hydra's service address | - | ` ory-hydra-admin.ory.svc.cluster.local` | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the leading space from the hydra-url example.
The current example renders a value that starts with a space. Copying it can produce an invalid Hydra URL.
Proposed fix
-| **hydra-url** | yes | ORY Hydra's service address | - | ` ory-hydra-admin.ory.svc.cluster.local` |
+| **hydra-url** | yes | ORY Hydra's service address | - | `ory-hydra-admin.ory.svc.cluster.local` |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| | **hydra-url** | yes | ORY Hydra's service address | - | ` ory-hydra-admin.ory.svc.cluster.local` | | |
| | **hydra-url** | yes | ORY Hydra's service address | - | `ory-hydra-admin.ory.svc.cluster.local` | |
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 73-73: Spaces inside code span elements
(MD038, no-space-in-code)
🤖 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 `@README.md` at line 73, Update the hydra-url example in the README table to
remove the leading space from the sample value, ensuring copied values begin
directly with the Hydra hostname.
Source: Linters/SAST tools
controller-gen rewrites config/crd/bases with its own indentation, which fails the format CI job. Re-apply prettier to the generated manifest. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Mathias Gebbe <mgebbe@hellmann.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.
Adds an opt-in option to wait for the Secret referenced by
spec.secretNameinstead of registering the OAuth2 client with a Hydra-generated secret and
creating that Secret itself.
Why. When
spec.secretNamepoints at an externally managed Secret — oneproduced by the External Secrets Operator, sealed-secrets, or any other
controller syncing it from a vault — the current "secret not found" branch in
controllers/oauth2client_controller.goloses a race. If the
OAuth2Clientreconciles before the Secret has beenmaterialized, hydra-maester registers the client in Hydra with a random
secret and creates its own Secret with an
OwnerReference. The externalcontroller then either refuses to adopt that pre-existing, unmanaged Secret
(External Secrets Operator with
creationPolicy: Owner) or ends up disagreeingwith the value Hydra holds. The end state depends purely on which controller
wins the race.
What. The behavior is available controller-wide and per resource.
Controller-wide, via a new flag that also reads from an environment variable:
Per resource, via
spec.requireExistingSecret, which overrides thecontroller-wide default in both directions — a resource can opt in while the
controller default is off, and opt out while it is on:
While the referenced Secret is missing, the controller does not register the
client in Hydra, does not create a Secret, reports a pending condition, and
requeues with an exponential backoff (15s, doubling, capped at 5m):
Once the Secret appears, reconciliation proceeds normally: the client is
registered with the externally provided
CLIENT_ID/CLIENT_SECRET, and theSecret is left untouched — in particular no
OwnerReferenceis added to it.No breaking changes. The flag defaults to
falseand the spec field isoptional, so without any configuration the controller behaves exactly as before.
Documented in the
Externally managed secrets section of
the README.
Related Issue or Design Document
Fixes #294 Fixes #304
Checklist
and signed the CLA.
introduces a new feature.
vulnerability. If this pull request addresses a security vulnerability, I
confirm that I got approval (please contact
security@ory.com) from the maintainers to push
the changes.
works.
appropriate).
Further comments
Design decisions
controller-level switch fits operators who run hydra-maester exclusively in a
bring-your-own-secret setup, while the per-resource field is needed for mixed
clusters. Making
spec.requireExistingSecreta*boolkeeps "unset"distinguishable from "explicitly false", so a single resource can opt out of a
globally enabled default.
ObservedGenerationis deliberately left untouched while pending. UnlikeupdateReconciliationStatusError, the new pending-status writer does notadvance it. Otherwise a client that goes pending and then receives its Secret
could hit the
Generation == ObservedGenerationshort-circuit inReconcileand stay
Ready=Falseforever.RequeueAfterinstead of the workqueue rate limiter.Result.Requeueis deprecated in controller-runtime v0.24, and returning anerror to get the built-in backoff would log an expected pending state at error
level on every attempt. The attempt counter is tracked per object and cleared
as soon as the Secret is found or the object is deleted.
Alternatives considered
OAuth2Client. This wouldmake pickup immediate rather than bounded by the backoff (up to 5 minutes in
the worst case), and costs little, since the manager cache already runs a
Secret informer for the existing
r.Geton secrets. It was left out becauseit changes reconcile triggers for every user, not just those who opt in. Happy
to add it if maintainers prefer that.
Brittle, tool-specific, and no protection against re-creation or drift after
the initial apply.
Changes
api/v1alpha1/oauth2client_types.goStatusSecretNotFoundstatus code andspec.requireExistingSecretfieldapi/v1alpha1/zz_generated.deepcopy.goconfig/crd/bases/hydra.ory.sh_oauth2clients.yamlcontrollers/oauth2client_controller.goWithRequireExistingSecretoption, wait-and-requeue branch, backoff tracking, pending-status writemain.go--require-existing-secretflag, defaulting fromREQUIRE_EXISTING_SECRETREADME.mdconfig/samples/hydra_v1alpha1_oauth2client_external_secret.yamlTests
Four new integration specs in
controllers/oauth2client_controller_integration_test.go:reports
SECRET_NOT_FOUNDwithReady=False,ObservedGenerationstaysbehind
credentials, Secret left without an
OwnerReferencecontroller-wide default enabled
make testpasses: 13 specs, all green.Summary by CodeRabbit
REQUIRE_EXISTING_SECRET, with per-resource overrides.