Skip to content

Conversation

@Pranjali-2501
Copy link
Contributor

@Pranjali-2501 Pranjali-2501 commented Dec 3, 2025

This PR implements the ConfigSelector changes required for gRFC A81.

It ensures that the auto_host_rewrite field 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:

  • Pass the AutoHostRewrite field value from Route struct via RPC context.
  • Add helper functions for AutoHostRewrite in internal/xds/balancer/cluserimpl/picker.go.
  • Update ConfigSelector.SelectConfig to pass the AutoHostRewrite boolean in RPC context.

RELEASE NOTES: None

@Pranjali-2501 Pranjali-2501 added Type: Feature New features or improvements in behavior Area: xDS Includes everything xDS related, including LB policies used with xDS. labels Dec 3, 2025
@Pranjali-2501 Pranjali-2501 changed the title xds/resolver: propagate route's auto_host_rewrite field in MatchedRoute (gRFC A81) xds/resolver: pass route's auto_host_rewrite to LB picker (gRFC A81) Dec 3, 2025
@Pranjali-2501 Pranjali-2501 added this to the 1.78 Release milestone Dec 3, 2025
@codecov
Copy link

codecov bot commented Dec 3, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.30%. Comparing base (4c27cc8) to head (29dd0bb).
⚠️ Report is 3 commits behind head on master.

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     
Files with missing lines Coverage Δ
internal/xds/balancer/clusterimpl/picker.go 97.40% <100.00%> (+0.25%) ⬆️
internal/xds/resolver/serviceconfig.go 88.05% <100.00%> (+0.08%) ⬆️
internal/xds/resolver/xds_resolver.go 88.63% <100.00%> (+0.06%) ⬆️

... and 22 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@easwars
Copy link
Contributor

easwars commented Dec 4, 2025

Sorry for suggesting that we could possibly send the whole route instead of sending only the auto_host_rewrite field, as specified in A60. After reading A60, I realized that sending the whole route down is not as simple as just stashing a pointer to the route in an attribute. We need to take into account things like reference counts to the route and the selected cluster and the whole cluster map.

So, let's keep it simple for now and just add an attribute that carries the auto_host_rewrite just like how we pass the selected cluster name from the config selector to the LB policy. Thanks.

@easwars easwars assigned Pranjali-2501 and unassigned easwars Dec 4, 2025
Copy link
Contributor

@easwars easwars left a 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 easwars assigned Pranjali-2501 and unassigned easwars Dec 4, 2025
@easwars easwars assigned Pranjali-2501 and unassigned easwars Dec 8, 2025
@Pranjali-2501 Pranjali-2501 removed their assignment Dec 9, 2025
Copy link
Contributor

@easwars easwars left a 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

Comment on lines +204 to +205
autohostRewrite, _ := ctx.Value(autoHostRewriteKey{}).(bool)
return autohostRewrite
Copy link
Contributor

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

Comment on lines +1320 to +1322
autoHostRewrite: false,
envconfig: false,
wantAutoHostRewrite: false,
Copy link
Contributor

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.

@easwars easwars assigned Pranjali-2501 and unassigned easwars Dec 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: xDS Includes everything xDS related, including LB policies used with xDS. Type: Feature New features or improvements in behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants