- Notifications
You must be signed in to change notification settings - Fork10k
Fix(discovery/aws): Fixed ECS SD region loading to check AWS config before IMDS.#17684
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
akshatsinha0 commentedDec 14, 2025
@krajorama Please have a look here. |
0a38efe toe887e6eComparebboreham commentedDec 16, 2025
Please rebase on main after#17695 to fix the Fuzzing CI failure. Also you seem to have some format issue. |
akshatsinha0 commentedDec 16, 2025
Yes i will |
…efore IMDS.Signed-off-by: Akshat Sinha <akshatsinhasramhardy@gmail.com>
e887e6e to41d4ffdCompare| ifc.Region=="" { | ||
| cfg,err:=awsConfig.LoadDefaultConfig(context.TODO()) | ||
| cfg,err:=awsConfig.LoadDefaultConfig(context.Background()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I cannot comment on whether this is correct or not, will ask@sysadmind and@matt-gp about that, but given that now we'll have this code 3 times copied, let's refactor into a function, something like:
func ensureRegion(region string) (string, error) {if region != "" {return region, nil}cfg, err := awsConfig.LoadDefaultConfig(context.Background())if err != nil {return "", err}if cfg.Region != "" {// Use the region from the AWS config. It will load environment variables and shared config files.return cfg.Region, nil}// Try to get the region from the instance metadata service (IMDS).imdsClient := imds.NewFromConfig(cfg)imdsRegion, err := imdsClient.GetRegion(context.Background(), &imds.GetRegionInput{})if err != nil {return "", err}return imdsRegion.Region, nil}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think abstracting to a function is worthwhile. Its the same code in 3 places.
This PR is okay as it sits though from a correctness perspective. LGTM
Uh oh!
There was an error while loading.Please reload this page.
Which issue(s) does the PR fix:
NONE
Description:
This PR fixes the ECS service discovery region loading logic to match the behavior of EC2 and Lightsail discovery.
Problem:
The ECS discovery was added after the region loading bugfix (PR#17376) was applied to EC2 and Lightsail. ECS has incomplete region detection logic...
It skips checking
cfg.Region(which loads fromAWS_REGIONenv var and~/.aws/config)used
context.Background()(like other AWS discoveries)Does this PR introduce a user-facing change?
Does this PR introduce a user-facing change?