googlec2p: use the bootstrap parsing code to generate parsed bootstrap config instead of handcrafting it #7040

clement2026 · 2024-03-16T02:37:31Z

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

linux-foundation-easycla · 2024-03-16T02:37:35Z

The committers listed above are authorized under a signed CLA.

✅ login: proxfly / name: Clement (b5787ea, 2b3c8ef, 69b5cd7, a3b44e2, 3b52470, b09ebd0, 4898d01)

arvindbr8

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

xds/googledirectpath/googlec2p.go

arvindbr8

@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.

arvindbr8 · 2024-03-19T20:56:54Z

xds/googledirectpath/googlec2p.go

@@ -106,25 +102,21 @@ func (c2pResolverBuilder) Build(t resolver.Target, cc resolver.ClientConn, opts
 	if balancerName == "" {


optional: i feel balancerName is a misnomer. This is the xDSServerURI

Suggested change

if balancerName == "" {

if balancerName == "" {

arvindbr8 · 2024-03-19T21:06:12Z

xds/googledirectpath/googlec2p.go

+	node := newNode(<-zoneCh, <-ipv6CapableCh)
+	xdsServer := newXdsServer(balancerName)
+	authorities := newAuthorities(xdsServer)


optional: how about new(Whatever)Config? newXdsServer sounds to me like we are creating something new here. But essentially its only the config

Done. Good insight!

xds/googledirectpath/googlec2p.go

arvindbr8 · 2024-03-19T21:22:51Z

The changes in #7048 should resolve the diff for ClientListenerResourceNameTemplate in the test. For the diff in server_features -- xds.config.resource-in-sotw is expected to be part of the config - So I would augment the test golden config to have that as well.

…p config instead of handcrafting it

…tures

codecov · 2024-03-20T18:54:37Z

Codecov Report

Merging #7040 (b09ebd0) into master (cce1632) will decrease coverage by 0.15%.
The diff coverage is 100.00%.

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

Files	Coverage Δ
xds/googledirectpath/googlec2p.go	`85.71% <100.00%> (+1.00%)`	⬆️
xds/internal/xdsclient/bootstrap/bootstrap.go	`74.48% <100.00%> (ø)`
xds/internal/xdsclient/client_new.go	`84.72% <100.00%> (ø)`

... and 16 files with indirect coverage changes

arvindbr8

Thanks for the PR. Looks really good to me modulo one comment.

Per policy we need another pass from one of the maintainer. cc: @zasweq

arvindbr8 · 2024-03-20T22:44:33Z

xds/googledirectpath/googlec2p.go

+		"user_agent_name": "%s",
+		"UserAgentVersionType": {
+			"userAgentVersion": "%s"
+		},
+		"client_features": ["%s", "%s"],


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

node.UserAgentName = gRPCUserAgentName

node.UserAgentVersionType = &v3corepb.Node_UserAgentVersion{UserAgentVersion: grpc.Version}

node.ClientFeatures = append(node.ClientFeatures, clientFeatureNoOverprovisioning, clientFeatureResourceWrapper)

I would prefer if we didnt set it here. Sorry about the back and forth on this.

Suggested change

"user_agent_name": "%s",

"UserAgentVersionType": {

"userAgentVersion": "%s"

},

"client_features": ["%s", "%s"],

arvindbr8 · 2024-03-20T23:23:45Z

xds/internal/xdsclient/bootstrap/bootstrap.go

@@ -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)


Thanks for making the change. I think with the last commit, this can also be reverted

zasweq

LGTM.

clement2026 force-pushed the 6901 branch from 0268a86 to 53b7a7d Compare March 16, 2024 02:45

clement2026 marked this pull request as ready for review March 16, 2024 02:50

arvindbr8 self-requested a review March 18, 2024 22:09

arvindbr8 self-assigned this Mar 18, 2024

arvindbr8 requested changes Mar 19, 2024

View reviewed changes

xds/googledirectpath/googlec2p.go Show resolved Hide resolved

xds/googledirectpath/googlec2p.go Outdated Show resolved Hide resolved

xds/googledirectpath/googlec2p.go Show resolved Hide resolved

arvindbr8 assigned clement2026 Mar 19, 2024

arvindbr8 added the Status: Requires Reporter Clarification label Mar 19, 2024

arvindbr8 mentioned this pull request Mar 19, 2024

googlec2p: use xdstp style template for client LDS resource name #7048

Merged

arvindbr8 approved these changes Mar 19, 2024

View reviewed changes

arvindbr8 removed their assignment Mar 19, 2024

arvindbr8 requested changes Mar 19, 2024

View reviewed changes

clement2026 force-pushed the 6901 branch from 53b7a7d to 378986d Compare March 20, 2024 16:21

clement2026 added 4 commits March 21, 2024 02:51

googlec2p: use the bootstrap parsing code to generate parsed bootstra…

2b3c8ef

…p config instead of handcrafting it

appply suggestions from code review

4898d01

fix JSON syntax error

69b5cd7

optimize how xds bootstrap fills in default values for node.ClientFea…

3b52470

…tures

clement2026 force-pushed the 6901 branch from d5354a9 to 3b52470 Compare March 20, 2024 18:51

go vet

b5787ea

clement2026 requested a review from arvindbr8 March 20, 2024 19:43

arvindbr8 approved these changes Mar 20, 2024

View reviewed changes

arvindbr8 requested a review from zasweq March 20, 2024 22:48

arvindbr8 added the Type: Internal Cleanup Refactors, etc label Mar 20, 2024

arvindbr8 added this to the 1.64 Release milestone Mar 20, 2024

let XDS bootstrap set default values for node

a3b44e2

arvindbr8 reviewed Mar 20, 2024

View reviewed changes

revert redundant duplicate checks

b09ebd0

arvindbr8 assigned zasweq Mar 22, 2024

arvindbr8 unassigned clement2026 Mar 22, 2024

arvindbr8 removed the Status: Requires Reporter Clarification label Mar 22, 2024

zasweq approved these changes Mar 27, 2024

View reviewed changes

zasweq merged commit 57e4391 into grpc:master Mar 27, 2024
14 checks passed

clement2026 deleted the 6901 branch March 27, 2024 18:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

googlec2p: use the bootstrap parsing code to generate parsed bootstrap config instead of handcrafting it #7040

googlec2p: use the bootstrap parsing code to generate parsed bootstrap config instead of handcrafting it #7040

clement2026 commented Mar 16, 2024 •

edited by arvindbr8

linux-foundation-easycla bot commented Mar 16, 2024 •

edited

arvindbr8 left a comment

arvindbr8 left a comment

arvindbr8 Mar 19, 2024

arvindbr8 Mar 19, 2024

clement2026 Mar 20, 2024

arvindbr8 commented Mar 19, 2024

codecov bot commented Mar 20, 2024 •

edited

arvindbr8 left a comment

arvindbr8 Mar 20, 2024

arvindbr8 Mar 20, 2024

zasweq left a comment

		@@ -106,25 +102,21 @@ func (c2pResolverBuilder) Build(t resolver.Target, cc resolver.ClientConn, opts
		if balancerName == "" {

	node.UserAgentName = gRPCUserAgentName
	node.UserAgentVersionType = &v3corepb.Node_UserAgentVersion{UserAgentVersion: grpc.Version}
	node.ClientFeatures = append(node.ClientFeatures, clientFeatureNoOverprovisioning, clientFeatureResourceWrapper)

googlec2p: use the bootstrap parsing code to generate parsed bootstrap config instead of handcrafting it #7040

googlec2p: use the bootstrap parsing code to generate parsed bootstrap config instead of handcrafting it #7040

Conversation

clement2026 commented Mar 16, 2024 • edited by arvindbr8

linux-foundation-easycla bot commented Mar 16, 2024 • edited

arvindbr8 left a comment

Choose a reason for hiding this comment

arvindbr8 left a comment

Choose a reason for hiding this comment

arvindbr8 Mar 19, 2024

Choose a reason for hiding this comment

arvindbr8 Mar 19, 2024

Choose a reason for hiding this comment

clement2026 Mar 20, 2024

Choose a reason for hiding this comment

arvindbr8 commented Mar 19, 2024

codecov bot commented Mar 20, 2024 • edited

Codecov Report

arvindbr8 left a comment

Choose a reason for hiding this comment

arvindbr8 Mar 20, 2024

Choose a reason for hiding this comment

arvindbr8 Mar 20, 2024

Choose a reason for hiding this comment

zasweq left a comment

Choose a reason for hiding this comment

clement2026 commented Mar 16, 2024 •

edited by arvindbr8

linux-foundation-easycla bot commented Mar 16, 2024 •

edited

codecov bot commented Mar 20, 2024 •

edited