[GCP auth filter] hold ref to service config (#37831)

This fixes the following crash:

https://btx.cloud.google.com/invocations/3a7065d4-db7f-4f01-a239-5376b7f5ee8b/targets/%2F%2Ftest%2Fcpp%2Fend2end%2Fxds:xds_gcp_authn_end2end_test@experiment%3Dwork_serializer_dispatch;config=1a7dc092b28796b045d00aec96c95b85c1d4dc656912e0021a1fc84b3ecb2ac9/log

The problem is caused by a race whereby the channel swaps out the service config due to a resolver update while the old dynamic filter stack is still processing calls in another thread.  The GCP auth filter was dereferencing the old service config but not holding a ref to it.  I've fixed this by having it hold a ref.

In the long run, I suspect that we may run into other cases like this, in which case we may want the dynamic filter stack itself to hold a ref to the service config, so that individual filters don't have to.

Closes #37831

COPYBARA_INTEGRATE_REVIEW=https://github.com/grpc/grpc/pull/37831 from markdroth:gcp_auth_race_fix f2a0d1dd79
PiperOrigin-RevId: 681221795
pull/37683/head
Mark D. Roth 4 months ago committed by Copybara-Service
parent b44de399c1
commit cd129b49c2
  1. 11
      src/core/ext/filters/gcp_authentication/gcp_authentication_filter.cc
  2. 4
      src/core/ext/filters/gcp_authentication/gcp_authentication_filter.h

@ -153,7 +153,7 @@ absl::StatusOr<std::unique_ptr<GcpAuthenticationFilter>>
GcpAuthenticationFilter::Create(const ChannelArgs& args,
ChannelFilter::Args filter_args) {
// Get filter config.
auto* service_config = args.GetObject<ServiceConfig>();
auto service_config = args.GetObjectRef<ServiceConfig>();
if (service_config == nullptr) {
return absl::InvalidArgumentError(
"gcp_auth: no service config in channel args");
@ -184,15 +184,18 @@ GcpAuthenticationFilter::Create(const ChannelArgs& args,
// cache but it has the wrong size.
cache->SetMaxSize(filter_config->cache_size);
// Instantiate filter.
return std::unique_ptr<GcpAuthenticationFilter>(new GcpAuthenticationFilter(
filter_config, std::move(xds_config), std::move(cache)));
return std::unique_ptr<GcpAuthenticationFilter>(
new GcpAuthenticationFilter(std::move(service_config), filter_config,
std::move(xds_config), std::move(cache)));
}
GcpAuthenticationFilter::GcpAuthenticationFilter(
RefCountedPtr<ServiceConfig> service_config,
const GcpAuthenticationParsedConfig::Config* filter_config,
RefCountedPtr<const XdsConfig> xds_config,
RefCountedPtr<CallCredentialsCache> cache)
: filter_config_(filter_config),
: service_config_(std::move(service_config)),
filter_config_(filter_config),
xds_config_(std::move(xds_config)),
cache_(std::move(cache)) {}

@ -80,10 +80,14 @@ class GcpAuthenticationFilter
};
GcpAuthenticationFilter(
RefCountedPtr<ServiceConfig> service_config,
const GcpAuthenticationParsedConfig::Config* filter_config,
RefCountedPtr<const XdsConfig> xds_config,
RefCountedPtr<CallCredentialsCache> cache);
// TODO(roth): Consider having the channel stack hold this ref so that
// individual filters don't need to.
const RefCountedPtr<ServiceConfig> service_config_;
const GcpAuthenticationParsedConfig::Config* filter_config_;
const RefCountedPtr<const XdsConfig> xds_config_;
const RefCountedPtr<CallCredentialsCache> cache_;

Loading…
Cancel
Save