Update FSM config parser #3586

kai-ion · 2025-10-15T21:07:37Z

Add support for parsing [services] sections with service-specific endpoint_url configuration following AWS SDKs & Tools specification.

Issue #, if available:

Description of changes:

Extend Profile class with endpoint_url and servicesEndpointUrl map
Add FSM states for [services] and [services serviceId] sections
Add GetServiceEndpointUrl() and GetGlobalEndpointUrl() API methods
Support case-insensitive service ID lookup
Add comprehensive test coverage

Supports config format:
[profile default]
endpoint_url = https://global.example.com

[services s3]
endpoint_url = http://localhost:9000

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-10-17T15:25:13Z

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

+            /**
+             * Get service-specific endpoint URL for a given profile and service.
+             */
+            const Aws::String* GetServiceEndpointUrl(const Aws::String& profileName, const Aws::String& serviceId) const;


why are we returning a pointer to a string? we should likely be returning by value and rely on RVO or NVRO, or returning bt ref if we actually want to edit the owned string, but we dont want to be returning by pointer.

Updated by using Aws::Crt::Optional<Aws::String> instead of pointer returns.

sbiscigl · 2025-10-17T15:30:00Z

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

                return iter->second;
            }

+            inline const Aws::String& GetServicesName() const {


why return are we returing by ref? we definitely should not be editing the underlying value.

Same as above. Updated to use Aws::Crt::Optional<Aws::String>

sbiscigl · 2025-10-17T15:33:21Z

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

+            inline const Aws::String& GetServicesName() const {
+                auto iter = m_allKeyValPairs.find("services");
+                static const Aws::String empty;
+                return (iter != m_allKeyValPairs.end()) ? iter->second : empty;


This likely not how we should be returning the idea of "lack of service endpoint". likely Crt::Optional is a good candidate for representing "lacking of value" instead of empty string.

Same as above. Updated to use Aws::Crt::Optional<Aws::String>

sbiscigl · 2025-10-17T15:55:32Z

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

+
+                        // New service block: "s3 =" (right hand side empty)
+                        if (!left.empty() && right.empty()) {
+                            activeServiceId = StringUtils::ToLower(left.c_str());


from the spec

This transformation MUST replace any spaces in the service ID with underscores and uppercase all letters

so ToLower here I think should be ToUpper no?

Fixed normalization to use ToUpper

sbiscigl · 2025-10-17T16:03:38Z

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

+            auto profileIter = m_profiles.find(profileName);
+            if (profileIter == m_profiles.end()) 
+            {
+                return nullptr;


yeah dont use "nullptr" to denote absence of a value, or a failed outcome, use a optional, or result type for that

Same as above. Updated to use Aws::Crt::Optional<Aws::String>

sbiscigl · 2025-10-17T17:09:32Z

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

+            /**
+             * Get global endpoint URL for a given profile.
+             */
+            const Aws::String* GetGlobalEndpointUrl(const Aws::String& profileName) const;


Alright i think we have a slightly larger design problem here that we should deal with lets take a look at the two new added APIS

Aws::String GetServiceEndpointUrl(const Aws::String& profileName, const Aws::String& serviceId) const; Aws::String* GetGlobalEndpointUrl(const Aws::String& profileName) const;

both of these functions have something in common, they take a profileName, and even in the comments you mention there is a mapping between profile and endpoint urls. This says to me instead of maintaining a foreign key mapping via this API, this is something that should exist in the profile object, i.e. Profile . so instead of adding this api, lets add this data to the Profile object that we store.

Moved GetEndpointUrl() and GetServicesName() as computed getters in the profile object, also added a static helper that does the lookup for the service.

NVM on the static helper, removed it!
I was only using it as a temporary 'resolver' for the new unit tests anyways

sbiscigl · 2025-10-20T15:44:34Z

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

        private:
            Aws::String m_fileName;
            bool m_useProfilePrefix;
+            Aws::Map<Aws::String, Aws::Map<Aws::String, Aws::String>> m_services;


i dont think this is being used anymore

removed this map in new commit

sbiscigl · 2025-10-20T18:19:14Z

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

                return iter->second;
            }

+            inline Aws::Crt::Optional<Aws::String> GetServicesName() const {


so i still think this is the wrong API, the profile object will have a services section, once loaded, we wont be doing any further look ups or joining of the services section and the profile sections, from the outside a API caller will see a profile object, just not it will have a services section/global endpoint member

Updated to have the profile to include a service object that will contain the map of endpoints and service block name

sbaluja · 2025-10-22T15:44:01Z

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

                            auto value = StringUtils::Trim(line.substr(equalsPos + 1).c_str());
                            currentKeyValues[key] = value;
                            continue;
                        }


Are we not handling global endpoint URL's for all services?
Ex.

[profile dev-global] endpoint_url = https://play.min.io:9000

global endpoint URL is handled by the FlushSection, and added directly to the profile

sbaluja · 2025-10-22T16:04:29Z

tests/aws-cpp-sdk-core-tests/aws/config/ServiceEndpointsConfigFileLoaderTest.cpp

+    const auto& endpoints = services.GetEndpoints();
+    ASSERT_EQ(1u, endpoints.size());
+    ASSERT_EQ("http://foo.com", endpoints.at("S3"));
+}


Curious what the behavior would be if we have multiple endpoint_url key/values in a profile or service configuration. I think we should add tests for scenarios like

[profile dev-global] endpoint_url = https://play.min.io:9000 endpoint_url = https://play2.min.io:9000

[services s3test] s3 endpoint_url = https://play.min.io:9000 s3 endpoint_url = https://play2.min.io:9000

From my understanding of the implementation, I think we'd just override the the value, so we'd end up with endpoint_url = https://play2.min.io:9000, but it's probably worth discussing if we want to error out here.

Either way this behavior is likely specific to OUR implementation since the SEP doesn't explicitly define this behavior, and this will be important to catch any regressions if we move to the CRT parser later on.

In the current parser it will just overwrite the url with the latest endpoint_url, I will add two tests for this behavior

sbiscigl

Mentioned some minor fixes before your ship, but API wise looks good

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

sbiscigl · 2025-10-23T16:59:05Z

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

+            inline void SetServices(Services&& services) { m_services = std::move(services); }
+
+            inline Aws::Crt::Optional<Aws::String> GetGlobalEndpointUrl() const {
+                const Aws::String& endpoint = GetValue("endpoint_url");


this is the only Get function that uses GetValue, why not create a memeber of profile endpointUrl to be like all the other data members of Profile?

Add support for parsing [services] sections with service-specific endpoint_url configuration following AWS SDKs & Tools specification. Changes: - Extend Profile class with endpoint_url and servicesEndpointUrl map - Add FSM states for [services] and [services serviceId] sections - Add GetServiceEndpointUrl() and GetGlobalEndpointUrl() API methods - Support case-insensitive service ID lookup - Add comprehensive test coverage Supports config format: [profile default] endpoint_url = https://global.example.com [services s3] endpoint_url = http://localhost:9000 Add Profile computed getters using Aws::Crt::Optional for proper null handling: - GetServicesName() returns services definition name or empty Optional - GetEndpointUrl() returns global endpoint URL or empty Optional - Add static Profile::GetServiceEndpointUrl() helper for endpoint resolution - Takes profile, services map, and serviceId as parameters - Maintains Profile as stateless data container - Add AWSConfigFileProfileConfigLoader::GetServices() const accessor Uses Aws::Crt::Optional instead of empty strings to properly represent "no value" state. Static helper pattern keeps Profile ABI-stable while enabling service endpoint resolution without coupling to loader internals. added a test for multiple services definition updated test to use .find before creatng the profile update to use a service object instead of using getvalue added 3 more tests verifying duplication urls, modify the services object with more encapsulation

kai-ion force-pushed the TheEnd branch from 721d7b2 to 9d3406c Compare October 16, 2025 19:49

sbiscigl reviewed Oct 17, 2025

View reviewed changes

kai-ion force-pushed the TheEnd branch from acfc595 to a1d3e94 Compare October 17, 2025 18:59

sbiscigl reviewed Oct 20, 2025

View reviewed changes

kai-ion force-pushed the TheEnd branch from 7b6a4b1 to bac51d2 Compare October 20, 2025 17:56

sbiscigl reviewed Oct 20, 2025

View reviewed changes

kai-ion force-pushed the TheEnd branch from 30ae557 to de8e6cb Compare October 21, 2025 18:22

sbaluja reviewed Oct 22, 2025

View reviewed changes

kai-ion force-pushed the TheEnd branch from 84ead78 to 2627fe9 Compare October 22, 2025 19:07

kai-ion marked this pull request as ready for review October 22, 2025 20:09

kai-ion force-pushed the TheEnd branch from 2627fe9 to 59f5ae9 Compare October 23, 2025 15:01

sbiscigl approved these changes Oct 23, 2025

View reviewed changes

kai-ion force-pushed the TheEnd branch from a9a3bf0 to 7b91863 Compare October 23, 2025 19:10

kai-ion marked this pull request as draft October 24, 2025 14:39

kai-ion force-pushed the TheEnd branch from a691c82 to f34ce97 Compare October 24, 2025 18:59

kai-ion marked this pull request as ready for review October 24, 2025 19:00

kai-ion marked this pull request as draft October 24, 2025 20:31

kai-ion force-pushed the TheEnd branch from e3a7204 to dea80d7 Compare October 24, 2025 20:34

kai-ion marked this pull request as ready for review October 24, 2025 20:35

Update FSM config parser #3586

Are you sure you want to change the base?

Update FSM config parser #3586

Conversation

kai-ion commented Oct 15, 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!

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!

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!

sbiscigl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants