Skip to content
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

Support AWS ECR credentials in China #50108

Merged
merged 4 commits into from
Nov 17, 2017

Conversation

zzq889
Copy link
Contributor

@zzq889 zzq889 commented Aug 3, 2017

What this PR does / why we need it:
Resolve AWS ECR credentials bug in China region

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

I haven't open any issue yet

Special notes for your reviewer:

Release note:

@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
  • If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: [email protected]

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 3, 2017
@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-label-needed labels Aug 3, 2017
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Aug 3, 2017
@k8s-github-robot k8s-github-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Aug 3, 2017
@therc
Copy link
Member

therc commented Aug 3, 2017

Whoa, it's kinda crazy that they went for a separate domain, but I can see why. Is there any Amazon documentation? Even amazonaws.cn does not mention it.

const registryURLTemplate = "*.dkr.ecr.%s.amazonaws.com"
const chinaRegistryURLTemplate = "*.dkr.ecr.%s.amazonaws.com.cn"
Copy link
Member

Choose a reason for hiding this comment

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

Is it amazonaws.com.cn or amazonaws.cn?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm sure is amazonaws.com.cn.

screen shot 2017-08-04 at 5 13 49 am

Copy link
Member

@therc therc Aug 3, 2017

Choose a reason for hiding this comment

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

Thanks, it just looked weird, given that the site is on amazonaws.cn...

I kinda verified that at least the domain seems to resolve and point to a proxy to somewhere...

$ host 1.dkr.ecr.cn-north-1.amazonaws.com.cn
1.dkr.ecr.cn-north-1.amazonaws.com.cn is an alias for proxy-cn-n-ProxyLB-OBNGHVYY3YZ4-1759429497.cn-north-1.elb.amazonaws.com.cn.
proxy-cn-n-ProxyLB-OBNGHVYY3YZ4-1759429497.cn-north-1.elb.amazonaws.com.cn has address 54.223.199.172
proxy-cn-n-ProxyLB-OBNGHVYY3YZ4-1759429497.cn-north-1.elb.amazonaws.com.cn has address 54.223.149.208

Copy link
Contributor Author

Choose a reason for hiding this comment

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

amazonaws.cn is kind like aws.amazon.com for web visit and amazonaws.com.cn is the mirror site for amazonaws.com. It's really painful here to deal with the GFW related problems.

@david-mcmahon david-mcmahon removed their assignment Aug 3, 2017
@zzq889
Copy link
Contributor Author

zzq889 commented Aug 4, 2017

/assign @deads2k

@zzq889
Copy link
Contributor Author

zzq889 commented Aug 4, 2017

Should I wait for the ok-to-test?

@zzq889
Copy link
Contributor Author

zzq889 commented Aug 4, 2017

/approve no-issue

@zzq889
Copy link
Contributor Author

zzq889 commented Aug 5, 2017

/assign @erictune @liggitt

@zzq889
Copy link
Contributor Author

zzq889 commented Aug 7, 2017

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 7, 2017
@k8s-ci-robot
Copy link
Contributor

@zzq889: you can't request testing unless you are a kubernetes member.

In response to this:

/ok-to-test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@zzq889
Copy link
Contributor Author

zzq889 commented Aug 7, 2017

@therc can you type ok-to-test?

@therc
Copy link
Member

therc commented Aug 7, 2017

/ok-to-test

@therc
Copy link
Member

therc commented Aug 7, 2017

@k8s-bot ok to test

@therc
Copy link
Member

therc commented Aug 7, 2017

@k8s-bot test this

@zzq889
Copy link
Contributor Author

zzq889 commented Aug 8, 2017

@therc I've update the code and testcase. Is ok-to-test still required for retest?

@zzq889
Copy link
Contributor Author

zzq889 commented Aug 9, 2017

@therc Even if I revert the code I still got test fail:

--- FAIL: TestEcrProvide (0.00s)
	aws_credentials_test.go:83: Didn't find expected URL: 123456789012.dkr.ecr.lala-land-1.amazonaws.com/foo/bar
FAIL

Any ideas?

@justinsb
Copy link
Member

/lgtm

/retest

I also can't see the reason for the test failure, but if the restest doesn't get it I can fire it up in a debugger

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 31, 2017
@justinsb
Copy link
Member

justinsb commented Sep 1, 2017

@zzq889 awsStandardDNSSuffix should be amazonaws.com:

-const awsStandardDNSSuffix = "amazon.com"
+const awsStandardDNSSuffix = "amazonaws.com"

@lareeth
Copy link

lareeth commented Sep 4, 2017

Any chance this can be added into a 1.6 release as well?

@lareeth
Copy link

lareeth commented Oct 16, 2017

@deads2k Any idea when this can be approved? This is super critical for running in China.

@deads2k
Copy link
Contributor

deads2k commented Oct 16, 2017

@deads2k Any idea when this can be approved? This is super critical for running in China.

I see the comment as outstanding: #50108 (comment)

Also, needs a squash.

@lareeth
Copy link

lareeth commented Oct 16, 2017

@zzq889 When do you plan to make this fix? #50108 (comment)

@lareeth
Copy link

lareeth commented Nov 14, 2017

@zzq889 Any update? Its been a month.

@zzq889
Copy link
Contributor Author

zzq889 commented Nov 15, 2017

@lareeth I use this container and wrote a cronjob for replacement. You can replace the image with your own build.

This pull request didn't pass the test with unknown reasons which may exist before I fork. I'll rebase and submit ag.

@zzq889
Copy link
Contributor Author

zzq889 commented Nov 15, 2017

/retest

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 15, 2017
@zzq889
Copy link
Contributor Author

zzq889 commented Nov 15, 2017

I1115 08:42:08.488] process 23 exited with code 0 after 0.1m
I1115 08:42:08.489] Call:  git checkout -B test c3e4084066105ff632668deb3732cdb276b0e622
W1115 08:42:08.713] Reset branch 'test'
I1115 08:42:08.716] process 35 exited with code 0 after 0.0m
I1115 08:42:08.717] Call:  git merge --no-ff -m 'Merge +refs/pull/50108/head:refs/pr/50108' 65bed1d982c10b386ddcc3ea899501df448f8565
W1115 08:42:08.732] merge: 65bed1d982c10b386ddcc3ea899501df448f8565 - not something we can merge
E1115 08:42:08.733] Command failed

What I need to do with this error? @justinsb
I run these commands locally without any error

@justinsb
Copy link
Member

/retest

That just looks like a test flake

@justinsb
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 17, 2017
@zzq889
Copy link
Contributor Author

zzq889 commented Nov 17, 2017

/assign @deads2k

@deads2k
Copy link
Contributor

deads2k commented Nov 17, 2017

/approve no-issue

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, justinsb, zzq889

Associated issue requirement bypassed by: deads2k

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 17, 2017
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Nov 17, 2017

@zzq889: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-bazel df344b5 link /test pull-kubernetes-bazel

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 55254, 55525, 50108, 54674, 55263). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 8058423 into kubernetes:master Nov 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.