-
Notifications
You must be signed in to change notification settings - Fork 582
Move UseClientProtocol to ClusterSettings #7347
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: jukie <[email protected]>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7347 +/- ##
==========================================
- Coverage 72.06% 72.02% -0.05%
==========================================
Files 230 230
Lines 33404 33404
==========================================
- Hits 24074 24059 -15
- Misses 7583 7596 +13
- Partials 1747 1749 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
thanks for picking this one up @jukie, doesnt this need some more plumbing in gateway-api layer and xds layer as well ? |
|
@arkodg it's an inline field so no changes necessary. We already have testdata that makes use of
|
|
/retest |
|
thanks, asked because I did a quick search and saw some special handling here
|
|
That should be covered by the existing testdata right? Let me know if there's any case that isn't and I can add more testdata. ClusterSettings is added inline so any refs to btp.Spec.UseClientProtocol continue working without any changes necessary. |
|
/retest |
1 similar comment
|
/retest |
|
checked again and this isnt going to work, the IR field needs to change as well gateway/internal/xds/translator/cluster.go Line 1227 in f133f5e
also this may need to be tested e2e to make sure that the protocol of downstream request can impact ext auth protocol type |
|
I'm not following, what needs to change in the IR field? If that was broken the existing testdata would be failing here. I think the follow-up PR would be for actually using it within ext auth, etc but moving into cluster settings is the first piece. Or are you suggesting doing both here? |
|
ah I wasn't aware this was part of the fix, if we do it in parts, it gets tricky because we cannot add a |
What type of PR is this?
What this PR does / why we need it:
Moves UseClientProtocol to ClusterSettings for use with ext Auth and ext Proc
Which issue(s) this PR fixes:
Fixes #7310
Release Notes: No