xds: parse allowed_grpc_services from the bootstrap config (A102)#9194
xds: parse allowed_grpc_services from the bootstrap config (A102)#9194mbissa wants to merge 3 commits into
Conversation
Adds parsing and credential resolution for the allowed_grpc_services bootstrap field (gRFC A102), used to validate side-channel targets received from untrusted xDS servers. The channel-credential selection stops at the first supported type; call credentials are built only when the call-creds env var is enabled, and are otherwise retained as config without being built. RELEASE NOTES: n/a
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the AllowedGrpcService struct and integrates an allowlist of gRPC services (allowed_grpc_services) into the xDS bootstrap configuration, along with corresponding JSON marshaling, unmarshaling, equality checks, and comprehensive unit tests. The review feedback highlights two important improvements: first, implementing a cleanup helper in Config.UnmarshalJSON to prevent resource leaks of active file watchers and credentials if subsequent parsing steps fail; second, refactoring AllowedGrpcService.UnmarshalJSON to be transactionally safe by using local variables and only mutating the receiver upon successful validation.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #9194 +/- ##
==========================================
- Coverage 83.27% 83.15% -0.12%
==========================================
Files 419 419
Lines 33863 33977 +114
==========================================
+ Hits 28198 28253 +55
- Misses 4252 4290 +38
- Partials 1413 1434 +21
🚀 New features to boost your workflow:
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the AllowedGrpcService struct to the XDS bootstrap configuration, enabling the definition of gRPC service-specific credentials. The changes include updates to the Config struct, implementation of JSON marshaling and unmarshaling, and comprehensive testing for credential iteration and cleanup logic. The reviewer identified potential resource leaks in the UnmarshalJSON method, specifically noting that Authorities are not currently cleaned up and that resources might leak if json.Unmarshal fails mid-execution, and provided a suggested fix to improve the cleanup defer block.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for AllowedGrpcServices in the xDS bootstrap configuration, allowing credentials configuration for allowed gRPC services. It also improves resource management by ensuring that any eagerly built credentials are properly cleaned up and released if bootstrap parsing or validation fails. The reviewer feedback suggests idiomatic Go improvements, such as using method expressions instead of anonymous function wrappers in slice/map equality checks, and using var declarations for empty slices to avoid unnecessary allocations.
mbissa
changed the title
| callCredsConfigs []CallCredsConfig | ||
| selectedChannelCreds ChannelCreds | ||
| selectedCallCreds []credentials.PerRPCCredentials | ||
| dialOptions []grpc.DialOption |
There was a problem hiding this comment.
Can we add a comment for what the fields represent , mainly for the cleanup and dialOption
| return a.selectedChannelCreds | ||
| } | ||
|
|
||
| // DialOptions returns dial options built from these credentials. |
There was a problem hiding this comment.
Can we clarify what these credentials mean ?
| } | ||
|
|
||
| // ChannelCreds returns the list of parsed channel credentials. | ||
| func (a *AllowedGrpcService) ChannelCreds() []ChannelCreds { |
There was a problem hiding this comment.
Just curious, where are we going to use these functions ?
| // Equal reports whether a and other are considered equal. | ||
| func (a *AllowedGrpcService) Equal(other *AllowedGrpcService) bool { | ||
| switch { | ||
| case a == nil && other == nil: |
There was a problem hiding this comment.
I think using if statements with early return is more suitable here. WDYT ?
| // A map from certprovider instance names to parsed buildable configs. | ||
| certProviderConfigs map[string]*certprovider.BuildableConfig | ||
|
|
||
| allowedGrpcServices map[string]*AllowedGrpcService |
There was a problem hiding this comment.
Can we add a comment here as to what is the key and what does the map represent ?
| break | ||
| } | ||
|
|
||
| if credsDialOption == nil { |
There was a problem hiding this comment.
Shouldn't we do this check inside the for loop just after assigning credsDialOption ? Is there a reason it is done later ?
| }, | ||
| allowedGrpcServices: func() map[string]*AllowedGrpcService { | ||
| var svc AllowedGrpcService | ||
| if err := json.Unmarshal([]byte(`{"channel_creds": [{"type": "insecure"}]}`), &svc); err != nil { |
There was a problem hiding this comment.
Instead of doing Unmarshal and then panic , can't we do this :
svc.channelCreds = []ChannelCreds{{Type: "insecure"}}
| // channel-credential selection skips unsupported types and stops at the first | ||
| // supported one, and that call credentials are only built when the call-creds | ||
| // env var is enabled. | ||
| func (s) TestBootstrap_AllowedGrpcServices_CredentialIteration(t *testing.T) { |
There was a problem hiding this comment.
Can we separate this test into different test since it anyway calls different t.Run for each different test
3 participants
Adds parsing and credential resolution for the allowed_grpc_services bootstrap field (gRFC A102), used to validate side-channel targets received from untrusted xDS servers. Channel credentials stop at the first supported type; call credentials are built only when the call-creds env var is enabled.
This is second of 4 PRs and is independent of other PRs. Can be reviewed in isolation.
RELEASE NOTES: n/a