-
Notifications
You must be signed in to change notification settings - Fork 18.7k
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
Conversation
Signed-off-by: haikuoliu <haikuo@amazon.com>
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.
LGTM
ping @samuelkarp |
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.
LGTM
It looks like the failing test (docker_api_swarm_test.go:296: DockerSwarmSuite.TestAPISwarmLeaderElection
) is unrelated to this PR.
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.
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)?
Ignoring the flaky test on s390x |
@thaJeztah Thanks! I will open the follow-up PR in a few days. |
This is great to see! What is the process for getting this into docker-ce? How long does it usually take? |
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. |
@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? |
@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. |
Hm, right; I was wondering if a newer version of the SDK had better detection (e.g., if region starts with 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)? |
@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. |
@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., When the |
Follow up PR to docker.github.io: docker/docs#7003 |
@thaJeztah This missed docker-ce 18.06, correct? Not seeing it in the release notes. |
I see the 18.09.0-ce-beta1 contains this fix. One qustion:
|
FYI this works for me using docker CLI w/ 18.09.0-ce-beta1 when specifying |
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
incloudwatchlogs.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)
╱▏┈┈┈┈┈┈▕╲▕╲┈┈┈
▏▏┈┈┈┈┈┈▕▏▔▔╲┈┈
▏╲┈┈┈┈┈┈╱┈▔┈▔╲┈
╲▏▔▔▔▔▔▔╯╯╰┳━━▀
┈▏╯╯╯╯╯╯╯╯╱┃┈┈┈
┈┃┏┳┳━━━┫┣┳┃┈┈┈
┈┃┃┃┃┈┈┈┃┃┃┃┈┈┈
┈┗┛┗┛┈┈┈┗┛┗┛┈┈┈