-
Notifications
You must be signed in to change notification settings - Fork 4.6k
xds/resolver: pass route's auto_host_rewrite to LB picker (gRFC A81) #8740
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: master
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #8740 +/- ##
==========================================
+ Coverage 83.28% 83.30% +0.01%
==========================================
Files 418 418
Lines 32367 32394 +27
==========================================
+ Hits 26958 26985 +27
+ Misses 4034 4030 -4
- Partials 1375 1379 +4
🚀 New features to boost your workflow:
|
2d08842 to
a6618d6
Compare
|
Sorry for suggesting that we could possibly send the whole route instead of sending only the So, let's keep it simple for now and just add an attribute that carries the |
easwars
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.
Also please remove any mentions to MatchedRoute from the PR description and from comments in the PR.
easwars
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.
LGTM, modulo one minor comment
| autohostRewrite, _ := ctx.Value(autoHostRewriteKey{}).(bool) | ||
| return autohostRewrite |
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.
Nit: maybe rename the variable with a one letter name. Something like v?
See: go/go-style/decisions#variable-names
TL;Dr
The general rule of thumb is that the length of a name should be proportional to the size of its scope and inversely proportional to the number of times that it is used within that scope
| autoHostRewrite: false, | ||
| envconfig: false, | ||
| wantAutoHostRewrite: false, |
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.
FYI: Please read this section: go/go-style/decisions#literal-zero-value-fields
IMO, it makes sense to have the values be explicitly specified in the table for this test case (as you have done currently). So, I'm not requesting changes, but just bringing this to your attention.
This PR implements the ConfigSelector changes required for gRFC A81.
It ensures that the
auto_host_rewritefield from the xDS Route Configuration is correctly propagated through the resolver and made available to the Load Balancer picker via the RPC context.Key Changes:
AutoHostRewritefield value fromRoutestruct via RPC context.AutoHostRewriteininternal/xds/balancer/cluserimpl/picker.go.ConfigSelector.SelectConfigto pass theAutoHostRewriteboolean in RPC context.RELEASE NOTES: None