googlec2p: use the bootstrap parsing code to generate parsed bootstrap config instead of handcrafting it #7040
Conversation
There was a problem hiding this comment.
I've made a pass at the PR. I think the TestBuildXDS test also needs to be updated. But I need some clarity on what the expected behavior of the ClientListenerResourceNameTemplate is for c2p. I will get back to that in a bit.
cc: @apolcyn
There was a problem hiding this comment.
@proxfly Heads up - I'm merging a change that would cause a conflict with your branch.
There was a behavior regression in the resolver. I wanted to fix that as a different PR. So that this change looks cleaner. Please rebase your changes with master when you get a chance.
xds/googledirectpath/googlec2p.go
Outdated
| @@ -106,25 +102,21 @@ func (c2pResolverBuilder) Build(t resolver.Target, cc resolver.ClientConn, opts | |||
| if balancerName == "" { | |||
There was a problem hiding this comment.
optional: i feel balancerName is a misnomer. This is the xDSServerURI
| if balancerName == "" { | |
| if balancerName == "" { |
xds/googledirectpath/googlec2p.go
Outdated
| node := newNode(<-zoneCh, <-ipv6CapableCh) | ||
| xdsServer := newXdsServer(balancerName) | ||
| authorities := newAuthorities(xdsServer) |
There was a problem hiding this comment.
optional: how about new(Whatever)Config? newXdsServer sounds to me like we are creating something new here. But essentially its only the config
There was a problem hiding this comment.
Done. Good insight!
|
The changes in #7048 should resolve the diff for |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #7040 +/- ##
==========================================
- Coverage 82.55% 82.40% -0.15%
==========================================
Files 300 300
Lines 31351 31357 +6
==========================================
- Hits 25881 25841 -40
- Misses 4416 4454 +38
- Partials 1054 1062 +8
|
xds/googledirectpath/googlec2p.go
Outdated
| "user_agent_name": "%s", | ||
| "UserAgentVersionType": { | ||
| "userAgentVersion": "%s" | ||
| }, | ||
| "client_features": ["%s", "%s"], |
There was a problem hiding this comment.
UserAgentVersionType is in camel case while the rest seem to be in snake case.
But this got me thinking newConfigFromContents overrides these values here
grpc-go/xds/internal/xdsclient/bootstrap/bootstrap.go
Lines 568 to 570 in cce1632
I would prefer if we didnt set it here. Sorry about the back and forth on this.
| "user_agent_name": "%s", | |
| "UserAgentVersionType": { | |
| "userAgentVersion": "%s" | |
| }, | |
| "client_features": ["%s", "%s"], |
| @@ -567,7 +565,7 @@ func newConfigFromContents(data []byte) (*Config, error) { | |||
| } | |||
| node.UserAgentName = gRPCUserAgentName | |||
| node.UserAgentVersionType = &v3corepb.Node_UserAgentVersion{UserAgentVersion: grpc.Version} | |||
| node.ClientFeatures = append(node.ClientFeatures, clientFeatureNoOverprovisioning, clientFeatureResourceWrapper) | |||
| node.ClientFeatures = AppendIfNotPresent(node.ClientFeatures, clientFeatureNoOverprovisioning, clientFeatureResourceWrapper) | |||
There was a problem hiding this comment.
Thanks for making the change. I think with the last commit, this can also be reverted
Fixes: #6901
It looks like the test cases are failing because the bootstrap parsing code is populating fallback values that are not anticipated by the failed test cases. I’m unfamiliar with these fallback values and would appreciate everyone’s insight.
RELEASE NOTES: none