Add awslogs-endpoint for isolated regions via sdk go v2 #4570
Conversation
Yiyuanzzz
changed the title
agent/engine/docker_task_engine.go
Outdated
| logger.Warn("No partition resolved for region. Using AWS default", logger.Fields{ | ||
| dnsSuffix := "amazonaws.com" | ||
| partition := awsrulesfn.GetPartition(region) | ||
| if partition == nil { |
There was a problem hiding this comment.
Do we have (or need) tests that cover the various partitions where the partition values may differ from the default?
There was a problem hiding this comment.
https://github.com/prateekchaudhry/amazon-ecs-agent/blob/8d0adff66e63ea9b94852966c4e4bdf1af511f04/agent/engine/docker_task_engine_test.go#L2949
Yes, we do! TestCreateContainerAwslogsLogDriver explicitly covers various isolated regions across different AWS partitions. It validates the generated awslogs-endpoint against the expected endpoint for each isolated region
agent/awsrulesfn/partitions.gen.go
Outdated
|
|
||
| // aws-sdk-go-v2 does not export partition metadata, so copy the files from vendor to make it accessible. | ||
|
|
||
| //go:generate cp ../vendor/github.com/aws/aws-sdk-go-v2/internal/endpoints/awsrulesfn/partition.go ../vendor/github.com/aws/aws-sdk-go-v2/internal/endpoints/awsrulesfn/partitions.go . |
There was a problem hiding this comment.
curious if the go generate needs to be run manually to keep partitions.go updated, or if is automated in some way
There was a problem hiding this comment.
whenever SDK dependencies is updated in go.mod and static checks fail due to go generate producing a diff in the future, we'll need to run go generate manually to ensure that the generated files stay in sync with the vendored AWS SDK files.
There was a problem hiding this comment.
Will the gogenerate target keep it updated? That is each time there is an sdk update, there needs to be a gogenerate run to keep this updated? Is this correct?
There was a problem hiding this comment.
yes, it's correct
prateekchaudhry
dismissed
their stale review
Requesting comments to be addressed
Yiyuanzzz
force-pushed
the
sdkv2/endpoint
branch
from
8e7933a to
26a6003
Compare
Yiyuanzzz
force-pushed
the
sdkv2/endpoint
branch
from
26a6003 to
0727ed7
Compare
Yiyuanzzz
force-pushed
the
sdkv2/endpoint
branch
from
0727ed7 to
e37b724
Compare
Yiyuanzzz
force-pushed
the
sdkv2/endpoint
branch
from
b986fba to
186b582
Compare
| @@ -1954,27 +1954,20 @@ func (engine *DockerTaskEngine) createContainer(task *apitask.Task, container *a | |||
| } else { | |||
There was a problem hiding this comment.
minor, non blocking: This is a short term solution only for specific regions comment is specific to the else section for isolated regions as per my understanding. We can consider moving the comment to relevant block.
Yiyuanzzz
force-pushed
the
sdkv2/endpoint
branch
from
186b582 to
69433d1
Compare
Yiyuanzzz
force-pushed
the
sdkv2/endpoint
branch
from
69433d1 to
a891a27
Compare
* migrate CW fix to v2 * improve logging
* migrate CW fix to v2 * improve logging
Summary
Currently agent rely on aws-sdk-go v1 to add awslogs-endpoint for all isolated regions when awslogs driver is configured for the container, This PR migrates the logic to use aws-sdk-go-v2 instead.
Implementation details
Testing
New tests cover the changes:
Description for the changelog
Enhancement: Add awslogs-endpoint for isolated regions via aws-sdk-go v2
Licensing
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.