xds: don't fail channel/server startup when xds creds is specified, but bootstrap is missing certificate providers #6848

easwars · 2023-12-11T23:12:45Z

We currently have checks in the xDS resolver and xDS-enabled gRPC server that verify that if xDS credentials are specified by the user, then the associated certificate providers configuration in the bootstrap is non-empty. If the check fails, then channel/server start-up fails. This is broken for a number of reasons:

On the client, we could have multiple clusters, some of which specify valid security configuration (i.e certificate provider instance name that is present in the bootstrap) and some of which specify invalid security configuration (i.e certificate provider instance name that is not present in the bootstrap). In this scenario, the clusters with valid security configuration need to work and the ones with invalid security configuration should fail with TRANSIENT_FAILURE. The same applies on the server side where the security configuration is part of the listener resource. FYI: We currently handle this scenario correctly by NACKing the appropriate resource.
Checking for the presence of the certificate provider configuration at channel/server startup is a very weak check and does not protect us from anything that the above check does not.
C-core and Java do not have this check.

This PR fixes #6756, and brings the grpc-go implementation to be in sync with C-core and Java.

RELEASE NOTES:

xds: don't fail channel/server startup when xds creds is specified, but bootstrap is missing certificate providers

…ut bootstrap is missing certificate providers

codecov · 2023-12-11T23:21:57Z

Codecov Report

Merging #6848 (25050b6) into master (52baf16) will increase coverage by 0.09%.
Report is 12 commits behind head on master.
The diff coverage is 14.28%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6848      +/-   ##
==========================================
+ Coverage   83.43%   83.53%   +0.09%     
==========================================
  Files         286      286              
  Lines       30801    30756      -45     
==========================================
- Hits        25699    25692       -7     
+ Misses       4028     3993      -35     
+ Partials     1074     1071       -3

Files	Coverage Δ
xds/internal/resolver/xds_resolver.go	`78.93% <ø> (-0.04%)`	⬇️
xds/internal/server/listener_wrapper.go	`67.88% <ø> (-0.15%)`	⬇️
xds/server.go	`86.50% <ø> (+0.51%)`	⬆️
xds/internal/balancer/cdsbalancer/cdsbalancer.go	`80.80% <25.00%> (+0.43%)`	⬆️
xds/internal/server/conn_wrapper.go	`71.64% <0.00%> (-1.97%)`	⬇️

... and 18 files with indirect coverage changes

zasweq · 2023-12-12T01:13:52Z

Can you please add an e2e test as to what the behavior should be e2e? (i.e. not fail, resolver NACKs, we handle in a graceful case). It used to be to fail, now what should it be? I see the component test for resolver only

zasweq · 2023-12-12T01:18:32Z

Server and Client side please for e2e test. Also, we only process security config from top level cluster incorrectly now, it needs to be for each specific cluster.

easwars · 2023-12-15T00:04:33Z

Added e2e style tests for both client and server now. PTAL.

zasweq

Will need to rebase my server fix onto this PR. Hopefully I can get that PR out by Thurs so you can take at least once pass before your leave for me to get back to next year.

test/xds/xds_server_certificate_providers_test.go

test/xds/xds_client_certificate_providers_test.go

test/xds/xds_server_certificate_providers_test.go

zasweq · 2023-12-19T04:29:18Z

test/xds/xds_server_certificate_providers_test.go

+	// Initialize an xDS-enabled gRPC server and register the stubServer on it.
+	// Pass it a mode change server option that pushes on a channel the mode
+	// changes to "not serving".
+	modeCh := make(chan struct{})
+	modeChangeOpt := xds.ServingModeCallback(func(addr net.Addr, args xds.ServingModeChangeArgs) {
+		if args.Mode == connectivity.ServingModeServing {
+			close(modeCh)
+		}
+	})


Would rather call modeCh. Also, this sends only once by closing it (and will panic if you close twice). Thinking about it, I made this an event for my server side e2e tests for the server side fix serving := grpcsync.NewEvent()...if args.Mode == connectivity.ServingModeServing {
serving.Fire()
}. I think that this semantically makes more sense here (and below).

Would rather call modeCh

It is called modeCh already. Don't understand this comment.

Also, this sends only once by closing it (and will panic if you close twice).

Since, we don't expect this to even happen once, I'm not too worried about a double close.

I think that this semantically makes more sense here (and below)

The grpcsync.Event provides a way to spot check if the event has fired and a way to block on the event firing. Since we only need the latter here, I feel a channel is more appropriate and it does not make the code harder to read in anyway. So, I would like to stick to using a channel here, unless you have a very good reason for using a grpcsync.Event instead.

Sorry servingModeCh. Hmmmm I guess that a channel does make sense here since this is something we expect not to happen vs. to happen (i.e. wait till serving until RPC happens)

Renamed to servingModeCh.

test/xds/xds_server_certificate_providers_test.go

xds/internal/balancer/cdsbalancer/cdsbalancer.go

zasweq · 2023-12-19T04:42:22Z

xds/internal/resolver/xds_resolver_test.go

@@ -86,71 +83,36 @@ func (s) TestResolverBuilder_ClientCreationFails_NoBootstap(t *testing.T) {
 	}
 }

-// Tests the resolver builder's Build() method with different xDS bootstrap
-// configurations.
-func (s) TestResolverBuilder_DifferentBootstrapConfigs(t *testing.T) {


Optional: leave a t-test in case we add more failure modes for resolver construction in the future.

What is a t-test?

Ah - Menghan/Doug described the functionality of defining test cases, then looping through each of them as a unity test as a "t-test'. So keep a slice of length 1. Optional though.

zasweq · 2023-12-19T20:57:08Z

test/xds/xds_server_certificate_providers_test.go

+					}}}}},
+		},
+		HttpFilters: []*v3httppb.HttpFilter{e2e.RouterHTTPFilter},
+	}
+	ts := &v3corepb.TransportSocket{
+		Name: "envoy.transport_sockets.tls",
+		ConfigType: &v3corepb.TransportSocket_TypedConfig{
+			TypedConfig: testutils.MarshalAny(t, &v3tlspb.DownstreamTlsContext{
+				RequireClientCertificate: &wrapperspb.BoolValue{Value: true},
+				CommonTlsContext: &v3tlspb.CommonTlsContext{
+					TlsCertificateCertificateProviderInstance: &v3tlspb.CommonTlsContext_CertificateProviderInstance{
+						InstanceName: "non-existent-certificate-provider",
+					},
+					ValidationContextType: &v3tlspb.CommonTlsContext_ValidationContextCertificateProviderInstance{
+						ValidationContextCertificateProviderInstance: &v3tlspb.CommonTlsContext_CertificateProviderInstance{
+							InstanceName: "non-existent-certificate-provider",
+						},
+					},
+				},
+			}),
+		},
+	}
+	resource2 := &v3listenerpb.Listener{
+		Name: fmt.Sprintf(e2e.ServerListenerResourceNameTemplate, net.JoinHostPort(host2, strconv.Itoa(int(port2)))),
+		Address: &v3corepb.Address{
+			Address: &v3corepb.Address_SocketAddress{
+				SocketAddress: &v3corepb.SocketAddress{
+					Address: host2,
+					PortSpecifier: &v3corepb.SocketAddress_PortValue{
+						PortValue: port2,
+					},
+				},
+			},
+		},
+		FilterChains: []*v3listenerpb.FilterChain{
+			{
+				Name: "v4-wildcard",
+				FilterChainMatch: &v3listenerpb.FilterChainMatch{
+					PrefixRanges: []*v3corepb.CidrRange{
+						{
+							AddressPrefix: "0.0.0.0",
+							PrefixLen: &wrapperspb.UInt32Value{
+								Value: uint32(0),
+							},
+						},
+					},
+					SourceType: v3listenerpb.FilterChainMatch_SAME_IP_OR_LOOPBACK,
+					SourcePrefixRanges: []*v3corepb.CidrRange{
+						{
+							AddressPrefix: "0.0.0.0",
+							PrefixLen: &wrapperspb.UInt32Value{
+								Value: uint32(0),
+							},
+						},
+					},
+				},
+				Filters: []*v3listenerpb.Filter{
+					{
+						Name:       "filter-1",
+						ConfigType: &v3listenerpb.Filter_TypedConfig{TypedConfig: testutils.MarshalAny(t, hcm)},
+					},
+				},
+				TransportSocket: ts,
+			},
+			{
+				Name: "v6-wildcard",
+				FilterChainMatch: &v3listenerpb.FilterChainMatch{
+					PrefixRanges: []*v3corepb.CidrRange{
+						{
+							AddressPrefix: "::",
+							PrefixLen: &wrapperspb.UInt32Value{
+								Value: uint32(0),
+							},
+						},
+					},
+					SourceType: v3listenerpb.FilterChainMatch_SAME_IP_OR_LOOPBACK,
+					SourcePrefixRanges: []*v3corepb.CidrRange{
+						{
+							AddressPrefix: "::",
+							PrefixLen: &wrapperspb.UInt32Value{
+								Value: uint32(0),
+							},
+						},
+					},
+				},
+				Filters: []*v3listenerpb.Filter{
+					{
+						Name:       "filter-1",
+						ConfigType: &v3listenerpb.Filter_TypedConfig{TypedConfig: testutils.MarshalAny(t, hcm)},
+					},
+				},
+				TransportSocket: ts,
+			},
+		},
+	}
+	resources := e2e.UpdateOptions{
+		NodeID:         nodeID,
+		Listeners:      []*v3listenerpb.Listener{resource1, resource2},
+		SkipValidation: true,
+	}


This is a super long inline block of xDS resources. Is there not e2e helpers for these? Should we refactor some of the e2e helpers if not to pull out common functionality for these?

This is not used anywhere else so far, so we haven't needed helpers for this. But if it turns out that you will need something like this for your server side tests, maybe we can pull it out at that point. But its going to be a bit of work.

https://github.com/grpc/grpc-go/blob/master/test/xds/xds_server_rbac_test.go#L323

zasweq · 2023-12-19T20:59:28Z

test/xds/xds_server_certificate_providers_test.go

+	if _, err := client.EmptyCall(sCtx, &testpb.Empty{}); status.Code(err) != codes.DeadlineExceeded {
+		t.Fatalf("EmptyCall() failed: %v", err)
+	}


Ohhhh right, I was thinking as if my change was already in master with respect to Accept() + Close(), and not blocking as it is now. I'll probably switch this over (and I have tests for this Accept() + Close() - I thought hard about the invariant of a signal that a connection was Accepted + Closed - UNAVAILABLE with error string sgtm).

zasweq

LGTM

zasweq · 2023-12-19T21:03:55Z

test/xds/xds_server_certificate_providers_test.go

+// server. The management server responds with two listener resources:
+//  1. contains valid security configuration pointing to the certificate provider
+//     instance specified in the bootstrap
+//  3. contains invalid security configuration pointing to a non-existent


xds: don't fail channel/server startup when xds creds is specified, b…

b7dc2dc

…ut bootstrap is missing certificate providers

easwars requested a review from zasweq December 11, 2023 23:12

easwars assigned zasweq Dec 11, 2023

easwars added the Type: Bug label Dec 11, 2023

easwars added this to the 1.61 Release milestone Dec 11, 2023

make vet happy

a1143b6

e2e style tests for client side

6d26f6f

easwars force-pushed the xds_certificate_providers_fix branch from 1363ae5 to 6d26f6f Compare December 12, 2023 20:51

e2e style tests for server side

99c6171

make vet happy

55f51d4

zasweq requested changes Dec 19, 2023

View reviewed changes

zasweq assigned easwars and unassigned zasweq Dec 19, 2023

review comments pass 1

e76c29e

easwars assigned zasweq and unassigned easwars Dec 19, 2023

zasweq reviewed Dec 19, 2023

View reviewed changes

zasweq approved these changes Dec 19, 2023

View reviewed changes

zasweq assigned easwars and unassigned zasweq Dec 19, 2023

review comments pass 2

25050b6

easwars merged commit bb0d32f into grpc:master Dec 20, 2023
14 checks passed

easwars deleted the xds_certificate_providers_fix branch December 20, 2023 00:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

xds: don't fail channel/server startup when xds creds is specified, but bootstrap is missing certificate providers #6848

xds: don't fail channel/server startup when xds creds is specified, but bootstrap is missing certificate providers #6848

easwars commented Dec 11, 2023

codecov bot commented Dec 11, 2023 •

edited

zasweq commented Dec 12, 2023

zasweq commented Dec 12, 2023

easwars commented Dec 15, 2023

zasweq left a comment

zasweq Dec 19, 2023

easwars Dec 19, 2023

zasweq Dec 19, 2023

easwars Dec 20, 2023

zasweq Dec 19, 2023

easwars Dec 19, 2023

zasweq Dec 19, 2023

zasweq Dec 19, 2023

easwars Dec 20, 2023

zasweq Dec 20, 2023

zasweq Dec 19, 2023 •

edited

zasweq left a comment

zasweq Dec 19, 2023

easwars Dec 20, 2023

xds: don't fail channel/server startup when xds creds is specified, but bootstrap is missing certificate providers #6848

xds: don't fail channel/server startup when xds creds is specified, but bootstrap is missing certificate providers #6848

Conversation

easwars commented Dec 11, 2023

codecov bot commented Dec 11, 2023 • edited

Codecov Report

zasweq commented Dec 12, 2023

zasweq commented Dec 12, 2023

easwars commented Dec 15, 2023

zasweq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zasweq Dec 19, 2023 • edited

Choose a reason for hiding this comment

zasweq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 11, 2023 •

edited

zasweq Dec 19, 2023 •

edited