Fix(discovery/aws): Fixed ECS SD region loading to check AWS config before IMDS.#17684
Fix(discovery/aws): Fixed ECS SD region loading to check AWS config before IMDS.#17684akshatsinha0 wants to merge 1 commit intoprometheus:mainfrom
Conversation
|
@krajorama Please have a look here. |
0a38efe to
e887e6e
Compare
|
Please rebase on main after #17695 to fix the Fuzzing CI failure. Also you seem to have some format issue. |
|
Yes i will |
…efore IMDS. Signed-off-by: Akshat Sinha <[email protected]>
e887e6e to
41d4ffd
Compare
|
|
||
| if c.Region == "" { | ||
| cfg, err := awsConfig.LoadDefaultConfig(context.TODO()) | ||
| cfg, err := awsConfig.LoadDefaultConfig(context.Background()) |
There was a problem hiding this comment.
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.
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
There was a problem hiding this comment.
Yeh adding it to it's own function would be good, we can use it in other AWS service discovery modules that we want to add, eg MSK, Elasticache etc.
There was a problem hiding this comment.
So should I take the initiative of adding it?
There was a problem hiding this comment.
Is this going to be added? I can pick it up if needs be.
There was a problem hiding this comment.
Yes please discover.
There was a problem hiding this comment.
I notice that the region loading logic in UnmarshalYAML is actually doing two distinct things:
1). Region resolution(AWS config -> IMDS fallback)
2). Config validation(ensuring region is not empty)
The current pattern mixes these concerns. when refactoring, we might wanna consider
// region.go -can be created
// attempting to load the AWS region from multiple sources
// in order of precedence: explicit config -> AWS config/env vars ->IMDS.
// Returns empty string (not error) if region cannot be determined, allowing
// the caller to decide whether to fail or use a default.
func loadRegionWithFallback(ctx context.Context, explicitRegion string) (string, error) {
if explicitRegion != "" {
return explicitRegion, nil
}
cfg, err := awsConfig.LoadDefaultConfig(ctx)
if err != nil {
return "", fmt.Errorf("failed to load AWS config: %w", err)
}
if cfg.Region != "" {
return cfg.Region, nil
}
// Fallback (may fail in non-AWS environments)
imdsClient := imds.NewFromConfig(cfg)
region, err := imdsClient.GetRegion(ctx, &imds.GetRegionInput{})
if err != nil {
return "", fmt.Errorf("failed to get region from IMDS: %w", err)
}
return region.Region, nil
}|
Hello from the bug scrub! Under active review. |
|
#18019 has now been merged so we can probably close this |
|
Yah. Wow. Thanks. |
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?