-
Notifications
You must be signed in to change notification settings - Fork 613
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
Resolving cloudwatch endpoint for all regions #4191
Conversation
5e34cef
to
9ea4d3c
Compare
} | ||
if endpoint == "" { | ||
endpoint = fmt.Sprintf("https://logs.%s.%s", region, dnsSuffix) |
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.
INFO: Just asking to learn, is this the default format of the endpoint if its not found in partition.EndpointFor? Does this mean it's a special type of region/not-opt in region?
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.
Does this work in Non-Commercial regions?
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.
Yep, this is typically the format for all service endpoints. We're adding this in the case that AWS SDK Go V1 can't resolve it on it's own (i.e. the information for a new region aren't present).
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.
Does this work in Non-Commercial regions?
As in isolated regions? If so then yes they do. This is more of a follow up to #4143.
"defaultDNSSuffix": ep.AwsPartition().DNSSuffix(), | ||
}) | ||
dnsSuffix = ep.AwsPartition().DNSSuffix() | ||
region := hostConfig.LogConfig.Config["awslogs-region"] |
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.
Would hostConfig.LogConfig.Config["awslogs-region"]
ever be empty or null? If yes, should fallback to use region := engine.cfg.AWSRegion
before moving to line 1863?
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.
RIght so awslogs-region
shouldn't be empty/null ever. If a customer were to omit this option from the task definition then it would be considered invalid.
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.
My original thought process was to also default to engine.cfg.AWSRegion
but after testing manually I couldn't find a scenario where this is not aparent from the task payload. Although I'm open to have this as a safety net or maybe fail the task.
}, | ||
{ | ||
name: "test container that uses awslogs log driver in BJS", | ||
region: "cn-north-1", |
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.
Note: cn-north-1 and us-gov-east-1 are both available/published in https://github.com/aws/aws-sdk-go-v2, so we should be ok to use them in our test cases.
Summary
This PR will aim to have agent resolve the correct AWS Cloudwatch endpoint for all regions in the case where tasks are using
awslogs
as the log driver type. Previously, we ran into an issue where the current docker version we're relying on is unable to resolve the correct Cloudwatch endpoints for the new regions and as a immediate remediation we're now relying on agent to resolve the endpoints. We should have this behavior be consistent across all regions.Note: We will be relying on AWS SDK Go V1 to attempt to resolve the correct endpoint (similar to moby). This will need to be updated once we upgrade to AWS SDK Go V2.
Implementation details
awslogs-region
to obtain the correct region of the Cloudwatch endpointTesting
Manual testing
Used the following task definition:
Task transitioned to running
Task container exited gracefully
Task was able to successfully write to Cloudwatch log group in
ca-central-1
fromus-west-2
New tests cover the changes: Yes
Description for the changelog
enhancement: Resolving Cloudwatch endpoint in all regions
Does this PR include breaking model changes? If so, Have you added transformation functions?
Licensing
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.