-
Notifications
You must be signed in to change notification settings - Fork 602
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
base: dev
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1854,32 +1854,31 @@ func (engine *DockerTaskEngine) createContainer(task *apitask.Task, container *a | |
} | ||
} | ||
|
||
// This is a short term solution only for specific regions until AWS SDK Go is upgraded to V2 | ||
// We're currently relying on AWS SDK Go V1 to obtain/format the correct cloudwatch endpoint. | ||
// This is will need to be updated once we upgrade to AWS SDK Go V2. | ||
if hostConfig.LogConfig.Type == logDriverTypeAwslogs { | ||
region := engine.cfg.AWSRegion | ||
if region == "eu-isoe-west-1" || region == "us-isof-south-1" || region == "us-isof-east-1" { | ||
endpoint := "" | ||
dnsSuffix := "" | ||
partition, ok := ep.PartitionForRegion(ep.DefaultPartitions(), region) | ||
if !ok { | ||
logger.Warn("No partition resolved for region. Using AWS default", logger.Fields{ | ||
"region": region, | ||
"defaultDNSSuffix": ep.AwsPartition().DNSSuffix(), | ||
}) | ||
dnsSuffix = ep.AwsPartition().DNSSuffix() | ||
region := hostConfig.LogConfig.Config["awslogs-region"] | ||
endpoint := "" | ||
dnsSuffix := "" | ||
partition, ok := ep.PartitionForRegion(ep.DefaultPartitions(), region) | ||
if !ok { | ||
logger.Warn("No partition resolved for region. Using AWS default", logger.Fields{ | ||
"region": region, | ||
"defaultDNSSuffix": ep.AwsPartition().DNSSuffix(), | ||
}) | ||
dnsSuffix = ep.AwsPartition().DNSSuffix() | ||
} else { | ||
resolvedEndpoint, err := partition.EndpointFor("logs", region) | ||
if err == nil { | ||
endpoint = resolvedEndpoint.URL | ||
} else { | ||
resolvedEndpoint, err := partition.EndpointFor("logs", region) | ||
if err == nil { | ||
endpoint = resolvedEndpoint.URL | ||
} else { | ||
dnsSuffix = partition.DNSSuffix() | ||
} | ||
dnsSuffix = partition.DNSSuffix() | ||
} | ||
if endpoint == "" { | ||
endpoint = fmt.Sprintf("https://logs.%s.%s", region, dnsSuffix) | ||
} | ||
hostConfig.LogConfig.Config["awslogs-endpoint"] = endpoint | ||
} | ||
if endpoint == "" { | ||
endpoint = fmt.Sprintf("https://logs.%s.%s", region, dnsSuffix) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more.
As in isolated regions? If so then yes they do. This is more of a follow up to #4143. |
||
} | ||
hostConfig.LogConfig.Config["awslogs-endpoint"] = endpoint | ||
} | ||
|
||
//Apply the log driver secret into container's LogConfig and Env secrets to container.Environment | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2877,7 +2877,17 @@ func TestCreateContainerAwslogsLogDriver(t *testing.T) { | |
{ | ||
name: "test container that uses awslogs log driver in IAD", | ||
region: "us-east-1", | ||
expectedLogConfigEndpoint: "", | ||
expectedLogConfigEndpoint: "https://logs.us-east-1.amazonaws.com", | ||
}, | ||
{ | ||
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 commentThe 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. |
||
expectedLogConfigEndpoint: "https://logs.cn-north-1.amazonaws.com.cn", | ||
}, | ||
{ | ||
name: "test container that uses awslogs log driver in OSU", | ||
region: "us-gov-east-1", | ||
expectedLogConfigEndpoint: "https://logs.us-gov-east-1.amazonaws.com", | ||
}, | ||
{ | ||
name: "test container that uses awslogs log driver in NCL", | ||
|
@@ -2906,8 +2916,10 @@ func TestCreateContainerAwslogsLogDriver(t *testing.T) { | |
|
||
rawHostConfigInput := dockercontainer.HostConfig{ | ||
LogConfig: dockercontainer.LogConfig{ | ||
Type: "awslogs", | ||
Config: map[string]string{}, | ||
Type: "awslogs", | ||
Config: map[string]string{ | ||
"awslogs-region": tc.region, | ||
}, | ||
}, | ||
} | ||
rawHostConfig, err := json.Marshal(&rawHostConfigInput) | ||
|
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 useregion := 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.