Skip to content

internal/transport: validate balancer and address metadata - #9203

Merged
easwars merged 1 commit into
grpc:masterfrom
buger:codex/fix-lb-address-metadata-validation
Aug 4, 2026
Merged

internal/transport: validate balancer and address metadata#9203
easwars merged 1 commit into
grpc:masterfrom
buger:codex/fix-lb-address-metadata-validation

Conversation

@buger

@buger buger commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

Fixes #9199

Validate metadata supplied by balancer PickResult.Metadata before merging it into the outgoing context, and validate resolver/address metadata before converting it to HTTP/2 header fields.

The tests cover invalid address metadata at the transport boundary and invalid balancer metadata through an end-to-end RPC path.

RELEASE NOTES:

  • transport: Invalid metadata supplied by balancers or resolver addresses is rejected before request headers are created.

@codecov

codecov Bot commented Jun 27, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 83.15%. Comparing base (d74861e) to head (b67d483).
⚠️ Report is 46 commits behind head on master.

Files with missing lines Patch % Lines
internal/transport/http2_client.go 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #9203      +/-   ##
==========================================
- Coverage   83.21%   83.15%   -0.06%     
==========================================
  Files         420      422       +2     
  Lines       34008    34866     +858     
==========================================
+ Hits        28299    28994     +695     
- Misses       4275     4382     +107     
- Partials     1434     1490      +56     
Files with missing lines Coverage Δ
stream.go 82.72% <100.00%> (+0.76%) ⬆️
internal/transport/http2_client.go 92.30% <0.00%> (-0.11%) ⬇️

... and 57 files with indirect coverage changes

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

@buger
buger force-pushed the codex/fix-lb-address-metadata-validation branch from a77ed9e to 99ce15e Compare June 27, 2026 08:51
@buger
buger marked this pull request as ready for review June 27, 2026 08:52
@buger
buger force-pushed the codex/fix-lb-address-metadata-validation branch from 99ce15e to 1155fc0 Compare June 27, 2026 08:59
@easwars

easwars commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Apologies for the delay in reviewing this. I will have this reviewed shortly.

@easwars
easwars self-requested a review July 8, 2026 05:39
@easwars easwars self-assigned this Jul 8, 2026
@easwars easwars added this to the 1.83 Release milestone Jul 8, 2026
Comment thread test/balancer_test.go Outdated
Comment thread internal/transport/address_metadata_validation_test.go Outdated
@eshitachandwani

Copy link
Copy Markdown
Member

Thank you for fixing this. It mostly looks good to me, I have added a few comments about test.

@easwars easwars modified the milestones: 1.83 Release, 1.84 Release Jul 14, 2026
@buger
buger force-pushed the codex/fix-lb-address-metadata-validation branch from 1155fc0 to 2ea7b27 Compare July 19, 2026 17:38
@buger

buger commented Jul 19, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @eshitachandwani, I updated this PR to address your review feedback.

Changes applied:

  • moved address metadata validation coverage into test/balancer_test.go as an E2E test using r.InitialState
  • covered both valid address metadata and invalid key/value address metadata
  • tightened TestInvalidMetadataInPickResult to assert the validation error string in addition to codes.Internal

Local validation passed:

go test ./test -run '^Test/(InvalidMetadataInPickResult|AddressMetadataValidation)' -count=1\ngo test ./internal/transport -count=1\ngo test ./test -count=1\n```

@buger
buger force-pushed the codex/fix-lb-address-metadata-validation branch from 2ea7b27 to ed23ab8 Compare July 19, 2026 17:45
@easwars easwars assigned eshitachandwani and unassigned buger Jul 22, 2026

@eshitachandwani eshitachandwani left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM modulo one minor nit.
Adding @easwars as a second reviewer.

Comment thread test/balancer_test.go Outdated
@buger
buger force-pushed the codex/fix-lb-address-metadata-validation branch from ed23ab8 to b67d483 Compare July 28, 2026 11:11
@buger

buger commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @eshitachandwani, I updated the picker helper naming to address the readability nit.

Change applied:

  • replaced the p.p.Pick callsite with imp.picker.Pick

Local validation passed:

go test ./test -run '^Test/(InvalidMetadataInPickResult|AddressMetadataValidation)' -count=1
git diff --check

@buger

buger commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

I looked into the failed tests (latest) job: https://github.com/grpc/grpc-go/actions/runs/30353786653/job/90257151056

The failure appears unrelated to this PR. The failing package/test is google.golang.org/grpc/internal/xds/clients/xdsclient/test, specifically TestConnectedMetric_Reconnection:

metrics_test.go:774: XDSClientConnected check failed after 1st NewStream - got: timeout waiting for specific metric: context deadline exceeded, want 1
FAIL google.golang.org/grpc/internal/xds/clients/xdsclient/test

This PR only changes metadata validation paths in internal/transport/http2_client.go, stream.go, and adds coverage in test/balancer_test.go. The adjacent matrix jobs passed, including tests (-race, latest), tests (arm, latest), tests (i386, latest), and tests (latest-1).

I also ran the exact failing test locally against this PR branch multiple times and it passed:

go test ./internal/xds/clients/xdsclient/test -run '^Test/ConnectedMetric_Reconnection$' -count=10

So this looks like an xDS metrics timing flake rather than a regression from the metadata validation change.

@eshitachandwani

Copy link
Copy Markdown
Member

I looked into the failed tests (latest) job: grpc/grpc-go/actions/runs/30353786653/job/90257151056

The failure appears unrelated to this PR. The failing package/test is google.golang.org/grpc/internal/xds/clients/xdsclient/test, specifically TestConnectedMetric_Reconnection:

metrics_test.go:774: XDSClientConnected check failed after 1st NewStream - got: timeout waiting for specific metric: context deadline exceeded, want 1
FAIL google.golang.org/grpc/internal/xds/clients/xdsclient/test

This PR only changes metadata validation paths in internal/transport/http2_client.go, stream.go, and adds coverage in test/balancer_test.go. The adjacent matrix jobs passed, including tests (-race, latest), tests (arm, latest), tests (i386, latest), and tests (latest-1).

I also ran the exact failing test locally against this PR branch multiple times and it passed:

go test ./internal/xds/clients/xdsclient/test -run '^Test/ConnectedMetric_Reconnection$' -count=10

So this looks like an xDS metrics timing flake rather than a regression from the metadata validation change.

We already have an open issue for this test, I have added the run there for the record and am re-running the test here.

@easwars
easwars merged commit fa603ec into grpc:master Aug 4, 2026
26 of 27 checks passed
@easwars

easwars commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

@buger : Thanks for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

internal/transport: validate balancer and resolver-injected metadata

3 participants