Skip to content

Pass endpoint to the CloudWatch Logs logging driver #37374

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

Merged
merged 1 commit into from
Jun 30, 2018

Conversation

haikuoliu
Copy link
Contributor

Signed-off-by: haikuoliu haikuo@amazon.com

- What I did
Pass endpoint to the CloudWatch Logs logging driver. This enables the driver to be used in AWS regions where the endpoint is not guessed correctly (e.g., regions like cn-northwest-1 where the correct endpoint is "logs.cn-northwest-1.amazonaws.com.cn" instead of "logs.cn-northwest-1.amazonaws.com") without updating the vendored revision of the AWS SDK.

- How I did it
Extract the endpoint from logger info with a predefined endpoint key, pass it to the session which is used to configure CloudWatch client.

Also add the endpoint key in ValidateLogOpt.

- How to verify it
When we call newAWSLogsClient in cloudwatchlogs.go and provide endpoint in logger info, we should be able to get the a client whose endpoint filed is set to the endpoint we passed.

- Description for the changelog

None

- A picture of a cute animal (not mandatory but encouraged)

╱▏┈┈┈┈┈┈▕╲▕╲┈┈┈
▏▏┈┈┈┈┈┈▕▏▔▔╲┈┈
▏╲┈┈┈┈┈┈╱┈▔┈▔╲┈
╲▏▔▔▔▔▔▔╯╯╰┳━━▀
┈▏╯╯╯╯╯╯╯╯╱┃┈┈┈
┈┃┏┳┳━━━┫┣┳┃┈┈┈
┈┃┃┃┃┈┈┈┃┃┃┃┈┈┈
┈┗┛┗┛┈┈┈┗┛┗┛┈┈┈

Signed-off-by: haikuoliu <haikuo@amazon.com>
Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@cpuguy83
Copy link
Member

ping @samuelkarp

Copy link
Member

@samuelkarp samuelkarp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

It looks like the failing test (docker_api_swarm_test.go:296: DockerSwarmSuite.TestAPISwarmLeaderElection) is unrelated to this PR.

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

this needs changes to the documentation as well (https://docs.docker.com/config/containers/logging/awslogs/#awslogs-region)

could you open a follow-up pull request in the https://github.com/docker/docker.github.io repository (against the vnext-engine branch)?

@thaJeztah
Copy link
Member

Ignoring the flaky test on s390x

@haikuoliu
Copy link
Contributor Author

@thaJeztah Thanks! I will open the follow-up PR in a few days.

@ferrx
Copy link

ferrx commented Jul 2, 2018

This is great to see! What is the process for getting this into docker-ce? How long does it usually take?

@thaJeztah
Copy link
Member

a first release-candidate for Docker CE 18.06 has already been cut, so likely in the version after that; I'll discuss if this is important enough to backport to that version.

@thaJeztah
Copy link
Member

without updating the vendored revision of the AWS SDK

@haikuoliu @samuelkarp I missed this earlier; does the above mean that we wouldn't need this change if we'd update the version of AWS SDK? Is there a reason to not update?

@ferrx
Copy link

ferrx commented Jul 2, 2018

@thaJeztah Unless I'm misunderstanding what he meant by that, I believe he's referring to the fact that these isolated regions have their own aws sdk with region specific endpoints (e.g. amazonaws.com.cn), and using a region specific version is a typical requirement for users of those various regions.

This update seems to allow you to specify the endpoint for your isolated region w/o an sdk change (which most importantly isn't viable for end-users since the sdk is baked into the docker binary). The alternative would be to create a docker-ce for each of these isolated regions that has their own aws sdk revision.

@thaJeztah
Copy link
Member

Hm, right; I was wondering if a newer version of the SDK had better detection (e.g., if region starts with cn-, use amazonaws.com.cn instead of amazonaws.com)

Building different versions for each region doesn't sound like a feasible option (what if, say Google or Azure uses different regions with their own exceptions)?

@haikuoliu
Copy link
Contributor Author

haikuoliu commented Jul 2, 2018

@thaJeztah Yes, we can just update AWS SDK in Docker. But the problem is ECS cannot control Docker update, so ECS will probably be blocked by Docker update when there is a new region. ECS Windows uses Docker EE, we don't know how long does it take to include the SDK update in Docker EE.

@samuelkarp
Copy link
Member

@thaJeztah AWS regions are divided into different partitions (e.g,. for standard regions, the partition is "aws" while for regions in China the partition is "aws-cn"). Partitions tend to have different endpoint suffixes (e.g., amazonaws.com for aws, amazonaws.com.cn for aws-cn). The AWS SDK contains logic that guesses the endpoint when it encounters a region it doesn't otherwise know about, and that logic defaults to guessing an endpoint in the aws partition.

When the cn-northwest-1 region was launched, older versions of Docker (I believe anything prior to 18.03 or so) incorrectly guess the CloudWatch Logs endpoint as "logs.cn-northwest-1.amazonaws.com" instead of "logs.cn-northwest-1.amazonaws.com.cn", making the awslogs driver unusable in that region. This change seeks to provide a way to override the endpoint if the SDK guesses incorrectly.

@haikuoliu
Copy link
Contributor Author

@thaJeztah

Follow up PR to docker.github.io: docker/docs#7003

@ferrx
Copy link

ferrx commented Jul 25, 2018

@thaJeztah This missed docker-ce 18.06, correct? Not seeing it in the release notes.

@ferrx
Copy link

ferrx commented Sep 19, 2018

I see the 18.09.0-ce-beta1 contains this fix. One qustion:

  • Is this something that can be defined in docker-compose?

@ferrx
Copy link

ferrx commented Sep 20, 2018

FYI this works for me using docker CLI w/ 18.09.0-ce-beta1 when specifying awslogs-endpoint log-opt. Have not tested w/ compose yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants