-
Notifications
You must be signed in to change notification settings - Fork 29
fix(client): fix SPIFFE X509-SVID retry logic for timing issues #741
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: Tibor Kircsi <[email protected]>
|
The latest Buf updates on your PR. Results from workflow Buf CI / verify-proto (pull_request).
|
Signed-off-by: Tibor Kircsi <[email protected]>
Signed-off-by: Tibor Kircsi <[email protected]>
Signed-off-by: Tibor Kircsi <[email protected]>
Signed-off-by: Tibor Kircsi <[email protected]>
Signed-off-by: Tibor Kircsi <[email protected]>
…ckage Signed-off-by: Tibor Kircsi <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes "certificate contains no URI SAN" errors in dirctl workloads by consolidating duplicate retry logic into a shared utils/spiffe package with configurable retry parameters.
Key changes:
- New shared retry implementation in utils/spiffe package with configurable backoff parameters
- Wrapper type X509SourceWithRetry that applies retry logic during TLS handshakes
- Comprehensive test coverage for retry scenarios including zero ID, nil SVID, and exponential backoff
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| utils/spiffe/retry.go | New shared retry implementation with X509SourceWithRetry wrapper and GetX509SVIDWithRetry function |
| utils/spiffe/retry_test.go | Comprehensive test coverage for all retry scenarios with fast test-specific parameters |
| client/options.go | Migrated to shared retry implementation and X509SourceWithRetry wrapper |
| server/authn/service.go | Migrated to shared retry implementation for both JWT and X509 modes |
| utils/go.mod | Added go-spiffe/v2 and testify dependencies to utils module |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Tibor Kircsi <[email protected]>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Fixes "certificate contains no URI SAN" errors in
dirctlworkloads (especially CronJobs) caused by SPIRE agent-server sync delays.Changes:
Move duplicate retry logic from client and server packages into a shared
utils/spiffe package. Make retry parameters configurable via constructor
to allow faster unit tests. Move retry tests to utils/spiffe/retry_test.go
for better code organization.
The retry logic handles cases where SPIRE entries haven't synced to the agent yet, which is common with short-lived workloads.