aws: Removed runtime flag use_http_client_to_fetch_aws_credentials (envoyproxy#37513)

ggreenway · web-flow · commit 885693b552e7 · 2024-12-05T08:34:41.000-08:00
Risk Level: Low Testing: Adjusted tests Docs Changes: Release Notes: added Progress towards envoyproxy#11816 Signed-off-by: Greg Greenway <ggreenway@apple.com>
diff --git a/changelogs/current.yaml b/changelogs/current.yaml
@@ -171,6 +171,9 @@ removed_config_or_runtime:
 - area: upstream
   change: |
     Removed runtime flag ``envoy.restart_features.allow_client_socket_creation_failure`` and legacy code paths.
+- area: aws
+  change: |
+    Removed runtime flag ``envoy.reloadable_features.use_http_client_to_fetch_aws_credentials``.
 - area: upstream
   change: |
     Removed runtime flag ``envoy.reloadable_features.exclude_host_in_eds_status_draining``.
diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc
@@ -98,7 +98,6 @@ RUNTIME_GUARD(envoy_reloadable_features_uhv_allow_malformed_url_encoding);
 RUNTIME_GUARD(envoy_reloadable_features_upstream_remote_address_use_connection);
 RUNTIME_GUARD(envoy_reloadable_features_use_config_in_happy_eyeballs);
 RUNTIME_GUARD(envoy_reloadable_features_use_filter_manager_state_for_downstream_end_stream);
-RUNTIME_GUARD(envoy_reloadable_features_use_http_client_to_fetch_aws_credentials);
 RUNTIME_GUARD(envoy_reloadable_features_use_route_host_mutation_for_auto_sni_san);
 RUNTIME_GUARD(envoy_reloadable_features_use_typed_metadata_in_proxy_protocol_listener);
 RUNTIME_GUARD(envoy_reloadable_features_validate_connect);
diff --git a/source/extensions/common/aws/credentials_provider_impl.cc b/source/extensions/common/aws/credentials_provider_impl.cc
@@ -132,8 +132,15 @@ MetadataCredentialsProviderBase::MetadataCredentialsProviderBase(
       cluster_name_(std::string(cluster_name)), cluster_type_(cluster_type), uri_(std::string(uri)),
       cache_duration_(getCacheDuration()), refresh_state_(refresh_state),
       initialization_timer_(initialization_timer), debug_name_(cluster_name) {
+
+  // Most code sets the context and uses the async http client, except for one extension
+  // which is scheduled to be deprecated and deleted. Modes can no longer be switched via runtime,
+  // so each caller should only pass parameters to support a single mode.
+  // https://github.com/envoyproxy/envoy/issues/36910
+  ASSERT((context.has_value() ^ (fetch_metadata_using_curl != nullptr)));
+
   // Async provider cluster setup
-  if (useHttpAsyncClient() && context_) {
+  if (context_) {
     // Set up metadata credentials statistics
     scope_ = api.rootScope().createScope(
         fmt::format("aws.metadata_credentials_provider.{}.", cluster_name_));
@@ -249,7 +256,7 @@ void MetadataCredentialsProviderBase::ThreadLocalCredentialsCache::onClusterRemo
 
 // Async provider uses its own refresh mechanism. Calling refreshIfNeeded() here is not thread safe.
 Credentials MetadataCredentialsProviderBase::getCredentials() {
-  if (useHttpAsyncClient()) {
+  if (context_) {
     if (tls_slot_) {
       return *(*tls_slot_)->credentials_.get();
     } else {
@@ -269,7 +276,7 @@ std::chrono::seconds MetadataCredentialsProviderBase::getCacheDuration() {
 }
 
 void MetadataCredentialsProviderBase::handleFetchDone() {
-  if (useHttpAsyncClient() && context_) {
+  if (context_) {
     if (cache_duration_timer_ && !cache_duration_timer_->enabled()) {
       // Receiver state handles the initial credential refresh scenario. If for some reason we are
       // unable to perform credential refresh after cluster initialization has completed, we use a
@@ -317,11 +324,6 @@ void MetadataCredentialsProviderBase::setCredentialsToAllThreads(
   }
 }
 
-bool MetadataCredentialsProviderBase::useHttpAsyncClient() {
-  return Runtime::runtimeFeatureEnabled(
-      "envoy.reloadable_features.use_http_client_to_fetch_aws_credentials");
-}
-
 bool CredentialsFileCredentialsProvider::needsRefresh() {
   return api_.timeSource().systemTime() - last_updated_ > REFRESH_INTERVAL;
 }
@@ -403,7 +405,7 @@ void InstanceProfileCredentialsProvider::refresh() {
   token_req_message.headers().setCopy(Http::LowerCaseString(EC2_IMDS_TOKEN_TTL_HEADER),
                                       EC2_IMDS_TOKEN_TTL_DEFAULT_VALUE);
 
-  if (!useHttpAsyncClient() || !context_) {
+  if (!context_) {
     // Using curl to fetch the AWS credentials where we first get the token.
     const auto token_string = fetch_metadata_using_curl_(token_req_message);
     if (token_string) {
@@ -555,7 +557,7 @@ void InstanceProfileCredentialsProvider::extractCredentials(
             session_token.empty() ? "" : "*****");
 
   last_updated_ = api_.timeSource().systemTime();
-  if (useHttpAsyncClient() && context_) {
+  if (context_) {
     setCredentialsToAllThreads(
         std::make_unique<Credentials>(access_key_id, secret_access_key, session_token));
     stats_->credential_refreshes_succeeded_.inc();
@@ -642,7 +644,7 @@ void ContainerCredentialsProvider::refresh() {
   message.headers().setHost(host);
   message.headers().setPath(path);
   message.headers().setCopy(Http::CustomHeaders::get().Authorization, authorization_header);
-  if (!useHttpAsyncClient() || !context_) {
+  if (!context_) {
     // Using curl to fetch the AWS credentials.
     const auto credential_document = fetch_metadata_using_curl_(message);
     if (!credential_document) {
@@ -708,7 +710,7 @@ void ContainerCredentialsProvider::extractCredentials(
   }
 
   last_updated_ = api_.timeSource().systemTime();
-  if (useHttpAsyncClient() && context_) {
+  if (context_) {
     setCredentialsToAllThreads(
         std::make_unique<Credentials>(access_key_id, secret_access_key, session_token));
     ENVOY_LOG(debug, "Metadata receiver {} moving to Ready state", cluster_name_);
@@ -760,12 +762,6 @@ bool WebIdentityCredentialsProvider::needsRefresh() {
 }
 
 void WebIdentityCredentialsProvider::refresh() {
-  // If http async client is not enabled then just set empty credentials and return.
-  if (!useHttpAsyncClient()) {
-    cached_credentials_ = Credentials();
-    return;
-  }
-
   ENVOY_LOG(debug, "Getting AWS web identity credentials from STS: {}", sts_endpoint_);
 
   std::string identity_token = token_;
@@ -1085,9 +1081,8 @@ absl::StatusOr<CredentialsProviderSharedPtr> createCredentialsProviderFromConfig
     // This "two seconds" is a bit arbitrary, but matches the other places in the codebase.
     const auto initialization_timer = std::chrono::seconds(2);
     return std::make_shared<WebIdentityCredentialsProvider>(
-        context.api(), context, Extensions::Common::Aws::Utility::fetchMetadata,
-        MetadataFetcher::create, "", token, sts_endpoint, role_arn, role_session_name,
-        refresh_state, initialization_timer, cluster_name);
+        context.api(), context, nullptr, MetadataFetcher::create, "", token, sts_endpoint, role_arn,
+        role_session_name, refresh_state, initialization_timer, cluster_name);
   } else {
     return absl::InvalidArgumentError("No AWS credential provider specified");
   }
diff --git a/source/extensions/common/aws/credentials_provider_impl.h b/source/extensions/common/aws/credentials_provider_impl.h
@@ -190,10 +190,6 @@ class MetadataCredentialsProviderBase : public CachedCredentialsProviderBase {
   // Set Credentials shared_ptr on all threads.
   void setCredentialsToAllThreads(CredentialsConstUniquePtr&& creds);
 
-  // Returns true if http async client can be used instead of libcurl to fetch the aws credentials,
-  // else false.
-  bool useHttpAsyncClient();
-
   Api::Api& api_;
   // The optional server factory context.
   ServerFactoryContextOptRef context_;
diff --git a/source/extensions/common/aws/utility.cc b/source/extensions/common/aws/utility.cc
@@ -331,7 +331,7 @@ static size_t curlCallback(char* ptr, size_t, size_t nmemb, void* data) {
   return nmemb;
 }
 
-absl::optional<std::string> Utility::fetchMetadata(Http::RequestMessage& message) {
+absl::optional<std::string> Utility::fetchMetadataWithCurl(Http::RequestMessage& message) {
   static const size_t MAX_RETRIES = 4;
   static const std::chrono::milliseconds RETRY_DELAY{1000};
   static const std::chrono::seconds TIMEOUT{5};
diff --git a/source/extensions/common/aws/utility.h b/source/extensions/common/aws/utility.h
@@ -92,7 +92,7 @@ class Utility {
   static std::string getSTSEndpoint(absl::string_view region);
 
   /**
-   * Fetch AWS instance or task metadata.
+   * Fetch AWS instance or task metadata with curl.
    *
    * @param message An HTTP request.
    * @return Metadata document or nullopt in case if unable to fetch it.
@@ -102,7 +102,7 @@ class Utility {
    * @note This is not main loop safe method as it is blocking. It is intended to be used from the
    * gRPC auth plugins that are able to schedule blocking plugins on a different thread.
    */
-  static absl::optional<std::string> fetchMetadata(Http::RequestMessage& message);
+  static absl::optional<std::string> fetchMetadataWithCurl(Http::RequestMessage& message);
 
   /**
    * @brief Creates the prototype for a static cluster towards a credentials provider
diff --git a/source/extensions/filters/http/aws_lambda/config.cc b/source/extensions/filters/http/aws_lambda/config.cc
@@ -60,7 +60,7 @@ AwsLambdaFilterFactory::getCredentialsProvider(
   }
   return std::make_shared<Extensions::Common::Aws::DefaultCredentialsProviderChain>(
       server_context.api(), makeOptRef(server_context), server_context.singletonManager(), region,
-      Extensions::Common::Aws::Utility::fetchMetadata);
+      nullptr);
 }
 
 absl::StatusOr<Http::FilterFactoryCb> AwsLambdaFilterFactory::createFilterFactoryFromProtoTyped(
diff --git a/source/extensions/filters/http/aws_request_signing/config.cc b/source/extensions/filters/http/aws_request_signing/config.cc
@@ -67,8 +67,7 @@ AwsRequestSigningFilterFactory::createFilterFactoryFromProtoTyped(
                     server_context, region, config.credential_provider())
               : std::make_shared<Extensions::Common::Aws::DefaultCredentialsProviderChain>(
                     server_context.api(), makeOptRef(server_context),
-                    server_context.singletonManager(), region,
-                    Extensions::Common::Aws::Utility::fetchMetadata);
+                    server_context.singletonManager(), region, nullptr);
   if (!credentials_provider.ok()) {
     return credentials_provider.status();
   }
@@ -137,7 +136,7 @@ AwsRequestSigningFilterFactory::createRouteSpecificFilterConfigTyped(
                     context, region, per_route_config.aws_request_signing().credential_provider())
               : std::make_shared<Extensions::Common::Aws::DefaultCredentialsProviderChain>(
                     context.api(), makeOptRef(context), context.singletonManager(), region,
-                    Extensions::Common::Aws::Utility::fetchMetadata);
+                    nullptr);
   if (!credentials_provider.ok()) {
     throw EnvoyException(std::string(credentials_provider.status().message()));
   }
diff --git a/source/extensions/grpc_credentials/aws_iam/config.cc b/source/extensions/grpc_credentials/aws_iam/config.cc
@@ -69,7 +69,7 @@ std::shared_ptr<grpc::ChannelCredentials> AwsIamGrpcCredentialsFactory::getChann
 
         auto credentials_provider = std::make_shared<Common::Aws::DefaultCredentialsProviderChain>(
             context.api(), absl::nullopt /*Empty factory context*/, context.singletonManager(),
-            region, Common::Aws::Utility::fetchMetadata);
+            region, Common::Aws::Utility::fetchMetadataWithCurl);
         auto signer = std::make_unique<Common::Aws::SigV4SignerImpl>(
             config.service_name(), region, credentials_provider, context,
             // TODO: extend API to allow specifying header exclusion. ref:
diff --git a/test/extensions/common/aws/aws_metadata_fetcher_integration_test.cc b/test/extensions/common/aws/aws_metadata_fetcher_integration_test.cc
@@ -102,7 +102,7 @@ TEST_F(AwsMetadataIntegrationTestSuccess, Success) {
   auto headers = Http::RequestHeaderMapPtr{new Http::TestRequestHeaderMapImpl{
       {":path", "/"}, {":authority", authority}, {":scheme", "http"}, {":method", "GET"}}};
   Http::RequestMessageImpl message(std::move(headers));
-  const auto response = Utility::fetchMetadata(message);
+  const auto response = Utility::fetchMetadataWithCurl(message);
 
   ASSERT_TRUE(response.has_value());
   EXPECT_EQ("METADATA_VALUE", *response);
@@ -121,7 +121,7 @@ TEST_F(AwsMetadataIntegrationTestSuccess, AuthToken) {
                                          {":method", "GET"},
                                          {"authorization", "AUTH_TOKEN"}}};
   Http::RequestMessageImpl message(std::move(headers));
-  const auto response = Utility::fetchMetadata(message);
+  const auto response = Utility::fetchMetadataWithCurl(message);
 
   ASSERT_TRUE(response.has_value());
   EXPECT_EQ("METADATA_VALUE_WITH_AUTH", *response);
@@ -140,7 +140,7 @@ TEST_F(AwsMetadataIntegrationTestSuccess, FetchTokenHttpPut) {
                                          {":method", "PUT"},
                                          {"X-aws-ec2-metadata-token-ttl-seconds", "21600"}}};
   Http::RequestMessageImpl message(std::move(headers));
-  const auto response = Utility::fetchMetadata(message);
+  const auto response = Utility::fetchMetadataWithCurl(message);
 
   ASSERT_TRUE(response.has_value());
   EXPECT_EQ("TOKEN_VALUE", *response);
@@ -161,7 +161,7 @@ TEST_F(AwsMetadataIntegrationTestSuccess, Redirect) {
                                          {":method", "GET"},
                                          {"authorization", "AUTH_TOKEN"}}};
   Http::RequestMessageImpl message(std::move(headers));
-  const auto response = Utility::fetchMetadata(message);
+  const auto response = Utility::fetchMetadataWithCurl(message);
 
   ASSERT_TRUE(response.has_value());
   EXPECT_EQ("METADATA_VALUE_WITH_AUTH", *response);
@@ -191,7 +191,7 @@ TEST_F(AwsMetadataIntegrationTestFailure, Failure) {
 
   Http::RequestMessageImpl message(std::move(headers));
   const auto start_time = timeSystem().monotonicTime();
-  const auto response = Utility::fetchMetadata(message);
+  const auto response = Utility::fetchMetadataWithCurl(message);
   const auto end_time = timeSystem().monotonicTime();
 
   EXPECT_FALSE(response.has_value());
@@ -218,7 +218,7 @@ TEST_F(AwsMetadataIntegrationTestTimeout, Timeout) {
   Http::RequestMessageImpl message(std::move(headers));
 
   const auto start_time = timeSystem().monotonicTime();
-  const auto response = Utility::fetchMetadata(message);
+  const auto response = Utility::fetchMetadataWithCurl(message);
   const auto end_time = timeSystem().monotonicTime();
 
   EXPECT_FALSE(response.has_value());
diff --git a/test/extensions/common/aws/credentials_provider_impl_test.cc b/test/extensions/common/aws/credentials_provider_impl_test.cc
@@ -382,10 +382,7 @@ class InstanceProfileCredentialsProviderTest : public testing::Test {
     ON_CALL(context_, clusterManager()).WillByDefault(ReturnRef(cluster_manager_));
 
     provider_ = std::make_shared<InstanceProfileCredentialsProvider>(
-        *api_, context_,
-        [this](Http::RequestMessage& message) -> absl::optional<std::string> {
-          return this->fetch_metadata_.fetch(message);
-        },
+        *api_, context_, nullptr,
         [this](Upstream::ClusterManager&, absl::string_view) {
           metadata_fetcher_.reset(raw_metadata_fetcher_);
           return std::move(metadata_fetcher_);
@@ -1081,9 +1078,6 @@ class InstanceProfileCredentialsProviderUsingLibcurlTest : public testing::Test
 
   void setupProvider() {
 
-    scoped_runtime_.mergeValues(
-        {{"envoy.reloadable_features.use_http_client_to_fetch_aws_credentials", "false"}});
-
     provider_ = std::make_shared<InstanceProfileCredentialsProvider>(
         *api_, absl::nullopt,
         [this](Http::RequestMessage& message) -> absl::optional<std::string> {
@@ -1407,10 +1401,7 @@ class ContainerCredentialsProviderTest : public testing::Test {
                      std::chrono::seconds initialization_timer = std::chrono::seconds(2)) {
     ON_CALL(context_, clusterManager()).WillByDefault(ReturnRef(cluster_manager_));
     provider_ = std::make_shared<ContainerCredentialsProvider>(
-        *api_, context_,
-        [this](Http::RequestMessage& message) -> absl::optional<std::string> {
-          return this->fetch_metadata_.fetch(message);
-        },
+        *api_, context_, nullptr,
         [this](Upstream::ClusterManager&, absl::string_view) {
           metadata_fetcher_.reset(raw_metadata_fetcher_);
           return std::move(metadata_fetcher_);
@@ -1835,10 +1826,7 @@ class ContainerEKSPodIdentityCredentialsProviderTest : public testing::Test {
                      std::chrono::seconds initialization_timer = std::chrono::seconds(2)) {
     ON_CALL(context_, clusterManager()).WillByDefault(ReturnRef(cluster_manager_));
     provider_ = std::make_shared<ContainerCredentialsProvider>(
-        *api_, context_,
-        [this](Http::RequestMessage& message) -> absl::optional<std::string> {
-          return this->fetch_metadata_.fetch(message);
-        },
+        *api_, context_, nullptr,
         [this](Upstream::ClusterManager&, absl::string_view) {
           metadata_fetcher_.reset(raw_metadata_fetcher_);
           return std::move(metadata_fetcher_);
@@ -1961,10 +1949,7 @@ class WebIdentityCredentialsProviderTest : public testing::Test {
       token_file_path = TestEnvironment::writeStringToFileForTest("web_token_file", "web_token");
     }
     provider_ = std::make_shared<WebIdentityCredentialsProvider>(
-        *api_, context_,
-        [this](Http::RequestMessage& message) -> absl::optional<std::string> {
-          return this->fetch_metadata_.fetch(message);
-        },
+        *api_, context_, nullptr,
         [this](Upstream::ClusterManager&, absl::string_view) {
           metadata_fetcher_.reset(raw_metadata_fetcher_);
           return std::move(metadata_fetcher_);
@@ -1995,10 +1980,7 @@ class WebIdentityCredentialsProviderTest : public testing::Test {
     }
     ON_CALL(context_, clusterManager()).WillByDefault(ReturnRef(cluster_manager_));
     provider_ = std::make_shared<WebIdentityCredentialsProvider>(
-        *api_, context_,
-        [this](Http::RequestMessage& message) -> absl::optional<std::string> {
-          return this->fetch_metadata_.fetch(message);
-        },
+        *api_, context_, nullptr,
         [this](Upstream::ClusterManager&, absl::string_view) {
           metadata_fetcher_.reset(raw_metadata_fetcher_);
           return std::move(metadata_fetcher_);

Original file line number	Diff line number	Diff line change
`@@ -331,7 +331,7 @@ static size_t curlCallback(char* ptr, size_t, size_t nmemb, void* data) {`
`331`	`331`	`return nmemb;`
`332`	`332`	`}`
`333`	`333`
`334`		`-absl::optional<std::string> Utility::fetchMetadata(Http::RequestMessage& message) {`
	`334`	`+absl::optional<std::string> Utility::fetchMetadataWithCurl(Http::RequestMessage& message) {`
`335`	`335`	`static const size_t MAX_RETRIES = 4;`
`336`	`336`	`static const std::chrono::milliseconds RETRY_DELAY{1000};`
`337`	`337`	`static const std::chrono::seconds TIMEOUT{5};`
Original file line number	Diff line number	Diff line change
`@@ -60,7 +60,7 @@ AwsLambdaFilterFactory::getCredentialsProvider(`
`60`	`60`	`}`
`61`	`61`	`return std::make_shared<Extensions::Common::Aws::DefaultCredentialsProviderChain>(`
`62`	`62`	`server_context.api(), makeOptRef(server_context), server_context.singletonManager(), region,`
`63`		`- Extensions::Common::Aws::Utility::fetchMetadata);`
	`63`	`+ nullptr);`
`64`	`64`	`}`
`65`	`65`
`66`	`66`	`absl::StatusOr<Http::FilterFactoryCb> AwsLambdaFilterFactory::createFilterFactoryFromProtoTyped(`