endpoint resolver/provider #3597

kai-ion · 2025-10-27T18:13:49Z

Issue #, if available:

Description of changes:
Adding an endpoint resolver template to be called in client init to resolve endpoint in order chain

Check all that applies:

Did a review by yourself.
Added proper tests to cover this PR. (If tests are not applicable, explain.)
Checked if this PR is a breaking (APIs have been changed) change.
Checked if this PR will not introduce cross-platform inconsistent behavior.
Checked if this PR would require a ReadMe/Wiki update.

Check which platforms you have built SDK on to verify the correctness of this PR.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

sbiscigl · 2025-11-03T16:45:17Z

src/aws-cpp-sdk-core/source/endpoint/BuiltInParameters.cpp

@@ -78,6 +79,51 @@ namespace Endpoint
        return SetFromClientConfiguration(static_cast<const Client::ClientConfiguration&>(config));
    }

+    void BuiltInParameters::SetFromClientConfiguration(const Client::ClientConfiguration& config, const Aws::String& serviceName)
+    {
+        bool forceFIPS = false;


this logic is copy and pasted from the other function SetFromClientConfiguration with the exception that we handle the setting of override differently. lets not repeat ourselves and refactor this function so that we dont have copy and pasted code.

Refactor the logic to use a shared SetFromClientConfigurationImpl function

sbiscigl · 2025-11-03T16:50:41Z

src/aws-cpp-sdk-core/source/endpoint/BuiltInParameters.cpp

+                SetStringParameter(AWS_REGION, "region-not-set");
+            }
+        } else if (!serviceName.empty()) {
+            Aws::Config::EndpointResolver::EndpointSource(serviceName, config.profileName, config.scheme, *this);


we shouldnt be passing *this, the objective here is figure out what the endpoint override it, we should not also be mutating self in this call. something along the lines of

OverrideEndpoint( Aws::Config::EndpointResolver::EndpointSource(serviceName, config), config.scheme);

would be what we want to be doing

Refactor endpointResolver to return an endpoint instead of mutating the config directly

sbiscigl · 2025-11-03T16:55:59Z

src/aws-cpp-sdk-core/include/aws/core/endpoint/BuiltInParameters.h

@@ -26,6 +26,8 @@ namespace Aws

            virtual void SetFromClientConfiguration(const Client::ClientConfiguration& config);
            virtual void SetFromClientConfiguration(const Client::GenericClientConfiguration& config);
+            virtual void SetFromClientConfiguration(const Client::ClientConfiguration& config, const Aws::String& serviceName);


should the non - service name ones have a implementation? why would they not simply call the service name function but with a empty service name?

would this be breaking if someone subclasses BuiltInParameters and they were already overriding SetFromClientConfiguration?

Refactor to use a fallback call instead of implementing new virtual functions

sbiscigl · 2025-11-03T16:56:43Z

src/aws-cpp-sdk-core/include/aws/core/config/EndpointResolver.h

+        /**
+         * Resolver that sources endpoints and sets them on endpoint providers.
+         */
+        class AWS_CORE_API EndpointResolver


why do we have a class if all the methods are static and we have no members? currently this class is default constructible.

Change this class into a name space instead

sbiscigl · 2025-11-03T16:59:32Z

src/aws-cpp-sdk-core/include/aws/core/endpoint/EndpointProviderBase.h

+            /**
+             * Initialize client context parameters from a ClientConfiguration with service name
+             */
+            virtual void InitBuiltInParameters(const ClientConfigurationT& config, const Aws::String& serviceName) = 0;


adding a new pure virtual method means that anything else implementing this will break. why not have a default implementation that calls the previous function to preserve backwards compat?

Refactor to use a fallback call with empty service name

sbiscigl · 2025-11-05T20:17:06Z

src/aws-cpp-sdk-core/include/aws/core/endpoint/BuiltInParameters.h

@@ -26,6 +26,8 @@ namespace Aws

            virtual void SetFromClientConfiguration(const Client::ClientConfiguration& config);
            virtual void SetFromClientConfiguration(const Client::GenericClientConfiguration& config);
+            void SetFromClientConfiguration(const Client::ClientConfiguration& config, const Aws::String& serviceName);


why are we not allowing this to be virtual as it was before?

This is me being stupid. Reverted back to virtual with the new impl private function to reduce code duplication.

sbiscigl · 2025-11-05T20:19:22Z

src/aws-cpp-sdk-core/source/config/EndpointResolver.cpp

+
+                // 2) Service-specific environment variable
+                {
+                    Aws::String service = "AWS_ENDPOINT_URL_";


why do we create a new variable eachtime? shouldnt this be const and static

Added SERVICE_ENDPOINT_ENV_PREFIX as static const char*, also did the same for GLOBAL_ENDPOINT_ENV_VAR

sbiscigl · 2025-11-05T20:20:47Z

src/aws-cpp-sdk-core/source/config/EndpointResolver.cpp

+                    service += serviceKey;
+                    Aws::String fromEnv = Aws::Environment::GetEnv(service.c_str());
+                    if (!fromEnv.empty()) {
+                        AWS_LOGSTREAM_DEBUG(ENDPOINT_RESOLVER_TAG, "Resolved configured endpoint from service-specific environment variable: " << service);


why are we using DEBUG and TRACE in the same if statement, that means that no matter which log level you choose one or both will be displayed. additionally when you want to know one, but not the other?

Replaced the DEBUG + TRACE with single DEBUG messages that include both the source information and the resolved URL

sbiscigl · 2025-11-05T20:22:58Z

tests/aws-cpp-sdk-s3-integration-tests/EndpointResolverIntegrationTest.cpp

+
+const char* ALLOCATION_TAG = "EndpointResolverIntegrationTest";
+
+class S3TestClient : public S3Client


why does this class exist? it looks like it just calls the base class and nothing more

Ops, copied over from s3unit test. Yes there is nothing in the test client being used in this integration test. (Headbucket request is already public).
Removed this class.

sbiscigl · 2025-11-05T20:27:46Z

tests/aws-cpp-sdk-s3-integration-tests/EndpointResolverIntegrationTest.cpp

+    EXPECT_EQ("https://test-bucket.custom-s3.example.com", seenRequest.GetUri().GetURIString());
+}
+
+TEST_F(EndpointResolverIntegrationTest, GlobalEnvironmentVariable)


I think we would want a test for GlobalEnvironmentVariableUsedOverProfile so that we validate that global endpoints are chosen over a specific profile endpoint.

Added a GlobalEnvironmentVariableUsedOverProfile test, works as expected with global environment > profile

kai-ion force-pushed the noEnd branch 3 times, most recently from 2ace84b to 5e53be5 Compare October 28, 2025 21:09

kai-ion marked this pull request as ready for review October 31, 2025 17:07

sbiscigl reviewed Nov 3, 2025

View reviewed changes

kai-ion force-pushed the noEnd branch from 883d4b7 to cad5808 Compare November 3, 2025 19:12

sbiscigl reviewed Nov 5, 2025

View reviewed changes

kai-ion force-pushed the noEnd branch from e2a5c58 to 4d6e792 Compare November 5, 2025 21:03

init

c3c9be1

kai-ion force-pushed the noEnd branch from 4d6e792 to c3c9be1 Compare November 5, 2025 21:32


		const char* ALLOCATION_TAG = "EndpointResolverIntegrationTest";

		class S3TestClient : public S3Client

endpoint resolver/provider #3597

Are you sure you want to change the base?

endpoint resolver/provider #3597

Conversation

kai-ion commented Oct 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kai-ion Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kai-ion Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kai-ion Nov 4, 2025 •

edited

Loading

kai-ion Nov 5, 2025 •

edited

Loading