Skip to content

xdsclient: preserve original bytes for decoding when the resource is wrapped #8411

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 25, 2025
Merged

xdsclient: preserve original bytes for decoding when the resource is wrapped #8411

merged 1 commit into from
Jun 25, 2025

Conversation

@easwars
Copy link
Contributor

@easwars easwars commented Jun 25, 2025

Some resource types do not have a name field in them. They rely on being wrapped in a discovery.Resource proto to give them a name. So when the xdsClient unwraps such a resource for validation purposes, it still needs to send the wrapped message bytes to the decoder.

RELEASE NOTES: none
@easwars easwars requested a review from arjan-bal June 25, 2025 10:11
@easwars easwars added the Type: Internal Cleanup Refactors, etc label Jun 25, 2025
@easwars easwars added this to the 1.74 Release milestone Jun 25, 2025
@codecov
Copy link

codecov bot commented Jun 25, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.30%. Comparing base (a2d6045) to head (4b67976).
Report is 1 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #8411      +/-   ##
==========================================
- Coverage   82.51%   82.30%   -0.22%     
==========================================
  Files         414      414              
  Lines       40420    40424       +4     
==========================================
- Hits        33354    33271      -83     
- Misses       5719     5787      +68     
- Partials     1347     1366      +19
Files with missing lines Coverage Δ
xds/internal/clients/xdsclient/channel.go 78.04% <100.00%> (+0.54%) ⬆️

... and 26 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
arjan-bal
arjan-bal approved these changes Jun 25, 2025
View reviewed changes
Copy link
Contributor

@arjan-bal arjan-bal left a comment

Choose a reason for hiding this comment

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

LGTM. From my reading of the code, all the existing decoders are calling xdsresource.UnwrapResource on the passed anypb.Any. All of them should continue to work the same even if the wrapped resource is passed after this change.
@arjan-bal arjan-bal added the Area: xDS Includes everything xDS related, including LB policies used with xDS. label Jun 25, 2025
@easwars
Copy link
Contributor Author

easwars commented Jun 25, 2025

same even if the wrapped resource is passed

Yes. Earlier, we used to pass the resource as-is to the decoders and let them call xdsresource.UnwrapResource. And for resource types like LDS, RDS, CDS, EDS, it doesn't matter if they get the wrapped resource or the unwrapped one, since they have a resource name in the resource itself. But for certain other resource types, the name is only available in the wrapped resource.
@easwars easwars merged commit 6207142 into grpc:master Jun 25, 2025
17 of 19 checks passed
@easwars easwars deleted the resource_unwrap branch June 25, 2025 10:50
purnesh42H
purnesh42H reviewed Jun 25, 2025
View reviewed changes
xds/internal/clients/xdsclient/channel.go
topLevelErrors = append(topLevelErrors, xdsresource.NewErrorf(xdsresource.ErrorTypeResourceTypeUnsupported, "unexpected resource type: %q ", inner.GetTypeUrl()))
continue
}
result, err := rType.Decoder.Decode(r.GetValue(), *opts)
Copy link
Contributor

@purnesh42H purnesh42H Jun 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

if r is wrapped and xdsresource.UnwrapResource() returns inner resource, will r.GetValue() return the same resource as inner.Resource?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. In that case r.GetValue will return the wrapped resource and the decoder will have to unwrap it.
Copy link
Contributor

@arjan-bal arjan-bal Jun 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

No, they will not be the same. r.GetValue() will return a serialized anypb.Any that contains a v3discoverypb.Resource{} while inner.Resource.GetValue() will return a serialized anypb.Any that contains the unwrapped xDS resouce proto.

This doesn't effect the decoders because they call UnwrapResource on the anypb.Any that they are passed to undo any wrapping.
Copy link
Contributor

Choose a reason for hiding this comment

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

yeah as long as decoders are doing that. Our 4 types do it.
Pranjali-2501 pushed a commit that referenced this pull request Jul 16, 2025
@easwars @Pranjali-2501
xdsclient: preserve original bytes for decoding when the resource is …
d307d75 
…wrapped (#8411)
Pranjali-2501 pushed a commit to Pranjali-2501/grpc-go that referenced this pull request Jul 17, 2025
@easwars @Pranjali-2501
xdsclient: preserve original bytes for decoding when the resource is …
94c8dc2 
…wrapped (grpc#8411)
@arjan-bal arjan-bal mentioned this pull request Jul 17, 2025
Merged
arjan-bal added a commit that referenced this pull request Jul 17, 2025
@Pranjali-2501 @easwars
@arjan-bal @purnesh42H @dfawley
Cherry-pick #8411, #8419, #8422, #8445 and #8451 to v1.74.x (#8454)
077e2c3 
* xdsclient: preserve original bytes for decoding when the resource is wrapped (#8411)

* xds: Avoid error logs when setting fallback bootstrap config (#8419)

* xdsclient: relay marshalled bytes of complete resource proto to decoders (#8422)

* xds: give up pool lock before closing xdsclient channel (#8445)

* transport: release mutex before returning on expired deadlines in server streams (#8451)

---------

Co-authored-by: Easwar Swaminathan <easwars@google.com>
Co-authored-by: Arjan Singh Bal <46515553+arjan-bal@users.noreply.github.com>
Co-authored-by: Purnesh Dixit <purneshdixit@google.com>
Co-authored-by: Doug Fawley <dfawley@google.com>
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: Internal Cleanup Refactors, etc

3 participants

@easwars @purnesh42H @arjan-bal