Migrate to go-oidc v3#281
Conversation
|
Built in conflict solver failed :/ will need manual rebase |
WalkthroughUpdated OIDC to use go-oidc/v3, added context.Context to NewOIDCServiceProvider and propagated it from server initialization. Provider creation now uses the passed context. go.mod updated to require go-oidc v3 and adjust JOSE dependencies. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Server init
participant HTTP as backend/http
participant OIDC as go-oidc v3
App->>HTTP: NewOIDCServiceProvider(ctx, clientID, secret, redirectURI, issuerURL)
HTTP->>OIDC: oidc.NewProvider(ctx, issuerURL)
OIDC-->>HTTP: Provider
HTTP-->>App: *OIDCServiceProvider, error
note over HTTP,OIDC: Context now controls provider discovery and cancellation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/http/oidc.go (1)
45-58: Bound discovery with a timeout to avoid startup hangs.
oidc.NewProviderperforms network discovery; without a bounded context it can stall startup. Wrap with a short timeout.Apply this diff inside the constructor:
- provider, err := oidc.NewProvider(ctx, issuerURL) + ctx, cancel := context.WithTimeout(ctx, 15*time.Second) + defer cancel() + provider, err := oidc.NewProvider(ctx, issuerURL)Also add the import:
// add import "time"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
backend/http/oidc.go(2 hunks)backend/server.go(1 hunks)go.mod(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Generate and push docs
- GitHub Check: lint
- GitHub Check: verify
- GitHub Check: go-test
- GitHub Check: go-test-e2e
🔇 Additional comments (4)
backend/http/oidc.go (1)
22-22: Import path migrated to go-oidc/v3 — LGTM.backend/server.go (1)
74-74: Context plumbed into OIDC constructor — LGTM.
Call site and constructor signatures match: backend/server.go:73-76, backend/http/oidc.go:45-48.go.mod (2)
32-32: Approved — go-oidc imports upgraded to v3; no stale imports found.
go.mod (line 32) and go.sum entries reference github.com/coreos/go-oidc/v3; backend/http/oidc.go imports github.com/coreos/go-oidc/v3/oidc.
92-92: Verify no legacy JOSE v2/gopkg.in imports (prevent mixed majors)go.mod now pins github.com/go-jose/go-jose/v4 v4.0.5 (go.mod: line 92). Search the repo for imports referencing gopkg.in/go-jose.v2 or github.com/go-jose/go-jose/v2 and update/remove any hits to avoid mixed-major build/runtime issues.
Suggested command: rg -n 'gopkg.in/(square/)?go-jose|github.com/go-jose/go-jose/v2' . || grep -RInE 'gopkg.in/(square/)?go-jose|github.com/go-jose/go-jose/v2' .
Summary
This upgrade is mostly because go-oidc switched to a Go-compatible versioning/branching system.
I also eliminated one TODO comment by tieing the oidc verifier's lifecycle to the server.
What Type of PR Is This?
/kind cleanup
Release Notes
Summary by CodeRabbit