fix/opv2-jwt - #318
Open
collinol wants to merge 1 commit into
Open
Conversation
collinol
marked this pull request as ready for review
August 5, 2026 21:38
Contributor
📝 WalkthroughWalkthroughThe manager discovers and stores the Kubernetes service-account issuer. RBAC permits access to the discovery endpoint. The webhook stops defaulting the issuer. Reconciliation resolves the issuer centrally and pauses when it is unavailable. ChangesService-account issuer discovery
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ManagerStartup
participant KubernetesAPIServer
participant ServiceIssuerStore
participant ControllerReconciler
ManagerStartup->>KubernetesAPIServer: GET /.well-known/openid-configuration
KubernetesAPIServer-->>ManagerStartup: issuer discovery response
ManagerStartup->>ServiceIssuerStore: SetServiceAccountIssuer(validated issuer)
ControllerReconciler->>ServiceIssuerStore: resolveInternalServiceAuthIssuer()
alt issuer resolved
ControllerReconciler->>ControllerReconciler: reconcile applications and JWT issuer mappings
else issuer unavailable
ControllerReconciler->>ControllerReconciler: update readiness and requeue after one minute
end
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 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}
Contributor
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 `@cmd/manager/main.go`:
- Around line 291-297: Update the startup issuer-discovery call around
RegisterServiceAccountIssuer to use a context.WithTimeout with an appropriate
bounded duration instead of context.Background(), and ensure the derived context
is canceled. Preserve the existing non-fatal error logging and manager startup
behavior when discovery times out or fails.
In `@internal/controller/reconciler/reconcile_v2.go`:
- Around line 527-530: Restore shared declarations for
internalServiceAuthEnabled, resolveInternalServiceAuthIssuer,
serviceAccountIssuerUnknownReason, and serviceAccountIssuerUnknownMessage, then
reference those declarations in internal/controller/reconciler/reconcile_v2.go
lines 527-530 and internal/controller/reconciler/pods.go lines 413-416. Ensure
both call sites use the same helpers and readiness constants, then run make lint
and make test.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ff361e20-ef45-4246-bd5d-00192898ad2e
📒 Files selected for processing (8)
cmd/manager/main.goconfig/rbac/role.yamldeploy/operator/templates/wandb-operator-wandb-role.yamlinternal/controller/reconciler/pods.gointernal/controller/reconciler/reconcile_v2.gointernal/controller/weightsandbiases_controller.gointernal/webhook/v2/weightsandbiases_webhook.gointernal/webhook/v2/weightsandbiases_webhook_test.go
Comment on lines
+291
to
+297
| // Non-fatal: a cluster that blocks the discovery endpoint should still start. | ||
| // The reconciler surfaces a not-ready condition rather than asserting an | ||
| // issuer it never verified. | ||
| if err := RegisterServiceAccountIssuer(context.Background()); err != nil { | ||
| setupLog.Error(err, "failed to discover the cluster service-account issuer; "+ | ||
| "internal service auth will not reconcile until spec.wandb.internalServiceAuth.oidcIssuer is set") | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add a deadline to issuer discovery.
context.Background() at Line 294 has no deadline. If the API server stalls, DoRaw blocks before the manager starts. The non-fatal error path does not run.
Use context.WithTimeout for this startup lookup.
Proposed fix
+ "time"
+
- if err := RegisterServiceAccountIssuer(context.Background()); err != nil {
+ issuerCtx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
+ defer cancel()
+ if err := RegisterServiceAccountIssuer(issuerCtx); err != nil {📝 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
| // Non-fatal: a cluster that blocks the discovery endpoint should still start. | |
| // The reconciler surfaces a not-ready condition rather than asserting an | |
| // issuer it never verified. | |
| if err := RegisterServiceAccountIssuer(context.Background()); err != nil { | |
| setupLog.Error(err, "failed to discover the cluster service-account issuer; "+ | |
| "internal service auth will not reconcile until spec.wandb.internalServiceAuth.oidcIssuer is set") | |
| } | |
| // Non-fatal: a cluster that blocks the discovery endpoint should still start. | |
| // The reconciler surfaces a not-ready condition rather than asserting an | |
| // issuer it never verified. | |
| issuerCtx, cancel := context.WithTimeout(context.Background(), 10*time.Second) | |
| defer cancel() | |
| if err := RegisterServiceAccountIssuer(issuerCtx); err != nil { | |
| setupLog.Error(err, "failed to discover the cluster service-account issuer; "+ | |
| "internal service auth will not reconcile until spec.wandb.internalServiceAuth.oidcIssuer is set") | |
| } |
🤖 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 `@cmd/manager/main.go` around lines 291 - 297, Update the startup
issuer-discovery call around RegisterServiceAccountIssuer to use a
context.WithTimeout with an appropriate bounded duration instead of
context.Background(), and ensure the derived context is canceled. Preserve the
existing non-fatal error logging and manager startup behavior when discovery
times out or fails.
Source: Coding guidelines
Comment on lines
+527
to
+530
| if internalServiceAuthEnabled(wandb) && resolveInternalServiceAuthIssuer(wandb) == "" { | ||
| logger.Info("Cluster service-account issuer unknown; not reconciling applications") | ||
| if err := updateReadyStatus(ctx, client, wandb, statusBefore, false, | ||
| serviceAccountIssuerUnknownReason, serviceAccountIssuerUnknownMessage); err != nil { |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Restore the shared issuer-resolution declarations.
internalServiceAuthEnabled, resolveInternalServiceAuthIssuer, serviceAccountIssuerUnknownReason, and serviceAccountIssuerUnknownMessage are undefined in package reconciler. The golangci-lint typecheck phase fails before the operator can build.
internal/controller/reconciler/reconcile_v2.go#L527-L530: add or reference the shared helper functions and readiness constants.internal/controller/reconciler/pods.go#L413-L416: use the same declared helper functions.
After the fix, run make lint and make test. As per coding guidelines, “Run both make lint and make test before considering a task complete.”
📍 Affects 2 files
internal/controller/reconciler/reconcile_v2.go#L527-L530(this comment)internal/controller/reconciler/pods.go#L413-L416
🤖 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 `@internal/controller/reconciler/reconcile_v2.go` around lines 527 - 530,
Restore shared declarations for internalServiceAuthEnabled,
resolveInternalServiceAuthIssuer, serviceAccountIssuerUnknownReason, and
serviceAccountIssuerUnknownMessage, then reference those declarations in
internal/controller/reconciler/reconcile_v2.go lines 527-530 and
internal/controller/reconciler/pods.go lines 413-416. Ensure both call sites use
the same helpers and readiness constants, then run make lint and make test.
Sources: Coding guidelines, Linters/SAST tools
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
1 participant
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.
The defaulting webhook populated
oidcIssuerwithhttps://kubernetes.default.svc.cluster.localso on EKS theissof the projected tokens did not correspond to the jwt-issuer-map, got a 401 errNow operator discovers real issuer from /.well-known/openid-configuration at startup, with precedence over the explicit value in the CR.
If the issuer remains unknown the CR goes not-ready with reason ServiceAccountIssuerUnknown instead of emitting an incorrect value.
The RBAC rule is added to both the generated role and the chart's ClusterRole, I believe this requires a re-release
Summary by CodeRabbit
New Features
Bug Fixes