-
Notifications
You must be signed in to change notification settings - Fork 3
fix: add a retry policy and async fetch for Connect's remote JWKS #87
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
Summary of ChangesHello @markgoddard, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the reliability and robustness of the JWT authentication process within Envoy. By introducing a retry mechanism and enabling asynchronous fetching of JSON Web Key Sets (JWKS), the changes aim to prevent authentication failures caused by intermittent network issues or Identity Provider flakiness, ensuring a more stable service operation. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request aims to improve the reliability of fetching remote JWKS in Envoy by introducing a retry policy and enabling asynchronous fetching. These are excellent changes for enhancing resilience against intermittent failures from the identity provider. However, there's a critical configuration error in the YAML structure where retry_policy is misplaced. I've provided a specific comment with a code suggestion to correct the indentation. Once this is fixed, the change will be ready.
We sometimes see API requests to the TLS API of Connect fail with: Jwks remote fetch is failed This has been traced to Envoy's JWT authenticator. Previously we configured it without retry policy, leaving it vulnerable to intermittent failures. We also used on-demand JWKS fetch, rather than a background thread (async fetch). This required each thread to individually cache the JWKS, increasing the likeliness of being affected by IdP flakiness. This change addresses both of these issues. Fixes: cofide/cofide-connect#223
30907b8 to
e60fa3e
Compare
alec-w
left a comment
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.
Good find 👍
We sometimes see API requests to the TLS API of Connect fail with:
Jwks remote fetch is failed
This has been traced to Envoy's JWT authenticator. Previously we
configured it without retry policy, leaving it vulnerable to
intermittent failures.
We also used on-demand JWKS fetch, rather than a background thread
(async fetch). This required each thread to individually cache the JWKS,
increasing the likeliness of being affected by IdP flakiness.
This change addresses both of these issues.
Fixes: cofide/cofide-connect#223
Docs: https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/filters/http/jwt_authn/v3/config.proto#extensions-filters-http-jwt-authn-v3-remotejwks