From b137e60b2a93ce0e6740fa85d6392414cd65a1db Mon Sep 17 00:00:00 2001 From: Andrew Stucki Date: Fri, 17 Feb 2023 17:14:39 +0000 Subject: [PATCH 1/2] backport of commit 01ca6e268bd43597d3876f6736a954433e4eb644 --- agent/xds/delta.go | 7 +++++++ agent/xds/testing.go | 4 ++++ 2 files changed, 11 insertions(+) diff --git a/agent/xds/delta.go b/agent/xds/delta.go index 5a1ef17d244..de4a450de24 100644 --- a/agent/xds/delta.go +++ b/agent/xds/delta.go @@ -47,6 +47,13 @@ type ADSDeltaStream = envoy_discovery_v3.AggregatedDiscoveryService_DeltaAggrega // DeltaAggregatedResources implements envoy_discovery_v3.AggregatedDiscoveryServiceServer func (s *Server) DeltaAggregatedResources(stream ADSDeltaStream) error { + // this guard is mainly for our tests where we sometimes nil out the + // server stream, any use of the stream after we nil it causes + // pretty much all of the below code to panic. + if stream == nil { + return nil + } + defer s.activeStreams.Increment(stream.Context())() // a channel for receiving incoming requests diff --git a/agent/xds/testing.go b/agent/xds/testing.go index 967a96e6acd..1e090091972 100644 --- a/agent/xds/testing.go +++ b/agent/xds/testing.go @@ -227,6 +227,10 @@ func (e *TestEnvoy) Close() error { // unblock the recv chans to simulate recv errors when client disconnects if e.deltaStream != nil && e.deltaStream.recvCh != nil { close(e.deltaStream.recvCh) + // TODO: This is causing a bunch of panics in testing code due to inflight + // requests attempting to grab a context from a stream that's nil. Added + // some defensive code in the xDS handler, but really, this should get fixed + // so we no longer have a data-race e.deltaStream = nil } if e.cancel != nil { From bfd03ef292d374ea92cd6b721809b7a3d68dcbaf Mon Sep 17 00:00:00 2001 From: Andrew Stucki Date: Fri, 17 Feb 2023 17:42:21 +0000 Subject: [PATCH 2/2] backport of commit 485cb4d88d25f064f418b9dbbbe2dd291a9d38a3 --- agent/xds/delta.go | 7 ------- agent/xds/testing.go | 10 ++++------ 2 files changed, 4 insertions(+), 13 deletions(-) diff --git a/agent/xds/delta.go b/agent/xds/delta.go index de4a450de24..5a1ef17d244 100644 --- a/agent/xds/delta.go +++ b/agent/xds/delta.go @@ -47,13 +47,6 @@ type ADSDeltaStream = envoy_discovery_v3.AggregatedDiscoveryService_DeltaAggrega // DeltaAggregatedResources implements envoy_discovery_v3.AggregatedDiscoveryServiceServer func (s *Server) DeltaAggregatedResources(stream ADSDeltaStream) error { - // this guard is mainly for our tests where we sometimes nil out the - // server stream, any use of the stream after we nil it causes - // pretty much all of the below code to panic. - if stream == nil { - return nil - } - defer s.activeStreams.Increment(stream.Context())() // a channel for receiving incoming requests diff --git a/agent/xds/testing.go b/agent/xds/testing.go index 1e090091972..76cc53f49fd 100644 --- a/agent/xds/testing.go +++ b/agent/xds/testing.go @@ -85,6 +85,8 @@ type TestEnvoy struct { EnvoyVersion string deltaStream *TestADSDeltaStream // Incremental v3 + + closed bool } // NewTestEnvoy creates a TestEnvoy instance. @@ -225,13 +227,9 @@ func (e *TestEnvoy) Close() error { defer e.mu.Unlock() // unblock the recv chans to simulate recv errors when client disconnects - if e.deltaStream != nil && e.deltaStream.recvCh != nil { + if !e.closed && e.deltaStream.recvCh != nil { close(e.deltaStream.recvCh) - // TODO: This is causing a bunch of panics in testing code due to inflight - // requests attempting to grab a context from a stream that's nil. Added - // some defensive code in the xDS handler, but really, this should get fixed - // so we no longer have a data-race - e.deltaStream = nil + e.closed = true } if e.cancel != nil { e.cancel()