-
Notifications
You must be signed in to change notification settings - Fork 438
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
feat: Allow users to disable extauth sidecar on a per proxy basis #8898
Conversation
c388adb
to
6543b6d
Compare
Issues linked to changelog: |
Visit the preview URL for this PR (updated for commit e190aea): https://gloo-edge--pr8898-disable-extauth-side-8ks7trcg.web.app (expires Wed, 22 Nov 2023 04:23:26 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 77c2b86e287749579b7ff9cadb81e099042ef677 |
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.
One nit, but LGTM
Description
When multiple gateway proxies are defined as well as extauth enabled, a user might want to enable the sidecar for a subset of gateway proxies (since some of them do not require extauth) to reduce resource usage and unnecessary network calls to an unused sidecar.
This feature adds the new helm value
gatewayproxy.proxyName.disableExtauthSidecar
to disable the extauth sidecar on itIt also adds additional helper methods to merge resources while omitting keys in those resources which should not be overwritten if empty
Helm changes
gatewayproxy.proxyName.disableExtauthSidecar
valueCode changes
N/A
CI changes
N/A
Docs changes
N/A
Context
#8430
Interesting decisions
Had initially thought of the idea to omit fields in an overwrite based on keys as in ddc6774 but decided against it as future changes would require modifying those keys if the same field is modified in different locations. Eg: if one part of the code did not want the field modified it would add the key to the list of omitKeys but another part of the code when changing the field would need to remove it from the list
Testing steps
Unit tests in enterprise to validate the behaviour of the new helm fields and helpers
Notes for reviewers
N/A
Please proofread comments on ...
N/A
Checklist: