Skip to content
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

xds/server: fix RDS handling for non-inline route configs #6915

Merged
merged 6 commits into from Jan 17, 2024

Conversation

zasweq
Copy link
Contributor

@zasweq zasweq commented Jan 10, 2024

Continuation and squashed based on #6889.

Fix the xDS Server to align with spec in https://github.com/grpc/proposal/blob/master/A36-xds-for-servers.md.

xds/internal/server/listener_wrapper:

  • Wait for all resources (LDS and RDS) to transition into serving, and if not serving accept() connections then close(). If received another LDS configuration, keep serving using old LDS until the new LDS has full RDS resources (i.e. each RDS resource has received an update or an error), then gracefully drain connections that were accepted and configured with old LDS, and switch to new LDS for new accepted connections.
  • Build all routing configurations for Filter Chains when LDS goes READY.
  • Build routing configuration dynamically using LDS + RDS on any RDS update.
  • Handle mode switches inline, drain server transports on any transition into non serving, accept() + close() if not serving.

xds/internal/server/conn_wrapper: Read routing configuration dynamically through an atomic load of a pointer, also add L7 error representation.

xds/internal/server/rds_handler: Persist rds updates and error conditions for each specific route resource being watched. Add a helper which determines whether all dynamic rds resources needed have been received. RDS errors mean the RDS has received configuration, trigger failure at L7 routing layer against incoming RPCs.

xds/server: Don’t block on a “good update”, immediately start serving on lis passed into Serve once wrapped, if xDS resources aren’t ready or LDS returns resource not found Accept() and Close() in not serving mode. Log any L7 routing errors due to xDS Configuration problems.

server: Call a callback with the server transport once it’s created on the Conn. This gives access to the server transport to xDS layer, which will be gracefully closed on transitions into not serving and transitions to a new LDS configuration. It also guarantees at some point the server transport will be gracefully drained. This replaces the Drain() operation previously present.

All LDS + RDS callbacks from the client are synchronous. The events that can happen asynchronous are the Accepting of a Conn (protected by mutex) and also an RPC on the Conn (protected with an atomic pointer). Thus dropped spawned event goroutines handleServingModeChanges() in the xDS Server, and run() on the listener_wrapper. Handled all the xDS resources inline and synced data structures/behaviors using above methods. L7 error = fail RPC with status code UNAVAILABLE (don’t give back more information due to security risk).

Fixes #6788.

RELEASE NOTES:

  • xds/server: fix RDS handling for non-inline route configs

@zasweq zasweq requested a review from dfawley January 10, 2024 18:18
@zasweq zasweq added Type: Bug Type: Feature New features or improvements in behavior labels Jan 10, 2024
@zasweq zasweq added this to the 1.61 Release milestone Jan 10, 2024
Copy link

codecov bot commented Jan 10, 2024

Codecov Report

Merging #6915 (3db6c1b) into master (bb0d32f) will decrease coverage by 0.02%.
Report is 11 commits behind head on master.
The diff coverage is 82.95%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6915      +/-   ##
==========================================
- Coverage   83.65%   83.64%   -0.02%     
==========================================
  Files         286      287       +1     
  Lines       30756    30920     +164     
==========================================
+ Hits        25730    25862     +132     
- Misses       3963     3992      +29     
- Partials     1063     1066       +3     
Files Coverage Δ
server.go 81.06% <100.00%> (-0.40%) ⬇️
xds/internal/xdsclient/client_new.go 84.72% <100.00%> (+1.64%) ⬆️
xds/xds.go 34.61% <ø> (ø)
xds/internal/server/conn_wrapper.go 73.49% <83.33%> (+1.85%) ⬆️
xds/server.go 82.73% <80.00%> (-3.77%) ⬇️
xds/internal/xdsclient/xdsresource/filter_chain.go 92.33% <91.22%> (+0.36%) ⬆️
xds/internal/xdsclient/clientimpl_watchers.go 76.36% <53.84%> (-6.97%) ⬇️
...ds/internal/xdsclient/xdsresource/resource_type.go 78.57% <71.42%> (-21.43%) ⬇️
xds/internal/server/listener_wrapper.go 73.61% <91.17%> (+5.72%) ⬆️
xds/internal/xdsclient/authority.go 89.92% <57.14%> (-1.89%) ⬇️
... and 1 more

... and 29 files with indirect coverage changes

Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

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

Looks really good overall! Mostly just some style and maintainability comments.

internal/transport/http2_server.go Outdated Show resolved Hide resolved
xds/internal/server/conn_wrapper.go Outdated Show resolved Hide resolved
xds/internal/server/conn_wrapper.go Outdated Show resolved Hide resolved
xds/internal/server/listener_wrapper.go Outdated Show resolved Hide resolved
xds/internal/server/listener_wrapper.go Outdated Show resolved Hide resolved
xds/internal/xdsclient/client_new.go Outdated Show resolved Hide resolved
xds/internal/xdsclient/clientimpl_watchers.go Outdated Show resolved Hide resolved
xds/internal/xdsclient/xdsresource/filter_chain.go Outdated Show resolved Hide resolved
xds/xds.go Outdated Show resolved Hide resolved
xds/internal/xdsclient/xdsresource/filter_chain.go Outdated Show resolved Hide resolved
@zasweq zasweq force-pushed the xds-server-fix-rebased branch 2 times, most recently from 7f59c17 to fcd71cd Compare January 11, 2024 17:27
@sergiitk
Copy link
Member

sergiitk commented Jan 11, 2024

Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

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

I only glanced at the tests so far; will look a bit more closely next round.

Just a few minor things.

xds/internal/server/conn_wrapper.go Outdated Show resolved Hide resolved
xds/internal/server/listener_wrapper.go Outdated Show resolved Hide resolved
xds/internal/server/rds_handler.go Outdated Show resolved Hide resolved
@dfawley dfawley assigned zasweq and unassigned dfawley Jan 12, 2024
@zasweq zasweq assigned dfawley and unassigned zasweq Jan 12, 2024
@zasweq
Copy link
Contributor Author

zasweq commented Jan 16, 2024

All interop tests continue to pass.

test/xds/xds_client_integration_test.go Outdated Show resolved Hide resolved
test/xds/xds_server_test.go Outdated Show resolved Hide resolved
test/xds/xds_server_test.go Outdated Show resolved Hide resolved
test/xds/xds_server_test.go Outdated Show resolved Hide resolved
test/xds/xds_server_test.go Outdated Show resolved Hide resolved
test/xds/xds_server_test.go Outdated Show resolved Hide resolved
xds/internal/server/listener_wrapper_test.go Outdated Show resolved Hide resolved
@dfawley dfawley assigned zasweq and unassigned dfawley Jan 16, 2024
@dfawley dfawley removed the Type: Feature New features or improvements in behavior label Jan 16, 2024
@zasweq zasweq changed the title Fix xDS Server in the case of dynamic RDS. xds/server: fix RDS handling for non-inline route configs Jan 16, 2024
@zasweq zasweq merged commit ddd377f into grpc:master Jan 17, 2024
14 checks passed
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.

Support dynamic RDS server side
3 participants