Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Service endpoints cannot be set using shared config file #2413

Closed
gdavison opened this issue Dec 6, 2023 · 3 comments · Fixed by #2416
Closed

Service endpoints cannot be set using shared config file #2413

gdavison opened this issue Dec 6, 2023 · 3 comments · Fixed by #2416
Assignees
Labels
bug This issue is a bug. p1 This is a high priority issue

Comments

@gdavison
Copy link
Contributor

gdavison commented Dec 6, 2023

Describe the bug

When service endpoints are set in shared config files, they are not applied to the aws.Config.

[default]
services = example

[services example]
s3 =
  endpoint_url = https://s3.example.com

Expected Behavior

The service endpoints would be set

Current Behavior

The service endpoints are not set

Reproduction Steps

Add the following test case to TestNewSharedConfig in config/shared_config_test.go

"service endpoint config": {
	ConfigFilenames: []string{testConfigFilename},
	Profile:         "service-endpoints",
	Expected: SharedConfig{
		Profile: "service-endpoints",
		Services: Services{
			ServiceValues: map[string]map[string]string{
				"s3": map[string]string{
					"endpoint_url": "https://s3.example.com",
				},
			},
		},
	},
},

Add the following sections to config/testdata/shared_config

[profile service-endpoints]
services = test

[services test]
s3 =
  endpoint_url = https://s3.example.com

The specific test case will fail with the following diff

config.SharedConfig{
                ... // 32 identical fields
                IgnoreConfiguredEndpoints: nil,
                BaseEndpoint:              "",
        - 	Services: config.Services{
        - 		ServiceValues: map[string]map[string]string{"s3": {"endpoint_url": "https://s3.example.com"}},
        - 	},
        + 	Services:             config.Services{ServiceValues: map[string]map[string]string{"s3": {}}},
                S3DisableExpressAuth:      nil,
          }

In addition, most of the other test cases will now fail with the following diff

config.SharedConfig{
                ... // 32 identical fields
                IgnoreConfiguredEndpoints: nil,
                BaseEndpoint:              "",
        -       Services:                  config.Services{},
        +       Services:                  config.Services{ServiceValues: map[string]map[string]string{"s3": {}}},
                S3DisableExpressAuth: nil,
          }

It looks like all profiles are referencing the [services test] section

Possible Solution

No response

Additional Information/Context

No response

AWS Go SDK V2 Module Versions Used

Using the main branch of aws-sdk-go-v2, at commit 8c02c46d48

Compiler and Version used

go version go1.21.0 darwin/arm64

Operating System and version

macOS 13.4.1

@gdavison gdavison added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Dec 6, 2023
@lucix-aws lucix-aws assigned lucix-aws and unassigned isaiahvita Dec 7, 2023
@lucix-aws lucix-aws linked a pull request Dec 7, 2023 that will close this issue
@lucix-aws
Copy link
Contributor

Regression from the INI parse refactor -- will be resolved by #2416. You also caught a small miss in how we load the services section - it's only supposed to be loaded into shared config if referenced in the profile via the services key (that's why every other test started failing with that data) - the PR includes that fix.

@lucix-aws lucix-aws added p1 This is a high priority issue and removed needs-triage This issue or PR still needs to be triaged. labels Dec 7, 2023
Copy link

github-actions bot commented Dec 7, 2023

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

@lucix-aws
Copy link
Contributor

This has been merged. Please be sure that both the config and internal/ini module versions are up-to-date after tomorrow's release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. p1 This is a high priority issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants