Add ECR creds provider #157

ayberk · 2020-11-20T01:05:48Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
Adds the credentials provider for ECR as part of the extraction effort.

Special notes for your reviewer:

Currently breaks the build due to 1.20 dependencies. DO NOT MERGE.
This PR is a slightly modified version of the example here. framework folder has been copied without any modifications.

Does this PR introduce a user-facing change?:

Add credentials provider for ECR

k8s-ci-robot · 2020-11-20T01:05:56Z

Hi @ayberk. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

ayberk · 2020-11-20T01:07:36Z

/assign @andrewsykim
/assign @nckturner

cmd/credential-providers/ecr/main.go

cmd/ecr-credential-provider/main.go

nckturner · 2020-11-22T06:37:47Z

cmd/ecr-credential-provider/main.go

+		// kubelet does not cache it in-memory
+		cacheDuration = &metav1.Duration{Duration: 0}
+	} else {
+		duration := expiresAt.Sub(time.Now())


Let me know if I'm missing something here, but if we are extracting the expiration from the ECR authorization token, converting that to a cache duration, then sending it back to kubelet, then I think we should shave some time off the duration here to account for transit back to kubelet, right?

Good callout. Actually since token comes from an makes an API call, we need to consider that latency as well, which would more random. Is it too late to change the CredentialProviderResponse to pass the CacheExpiresAt directly instead of CacheDuration?

For compatibility, we can add it as an optional parameter and it'd take precedence over CacheDuration.

I've substracted 1 second for now, which I think should be good for most cases. We can increase it if we want to be safer.

Even 1 second seems pretty aggressive, can we subtract maybe an hour? That gives plenty of headroom in case there are errors retrieving new tokens but not too short of a duration where we're unnecessarily fetching tokens.

Even half the expiration from ECR seems reasonable to me but I'll defer to @nckturner on what is appropriate.

ayberk · 2020-12-02T20:05:52Z

/hold until we update to 1.20 (#151)

andrewsykim · 2020-12-03T01:01:07Z

/ok-to-test

ayberk · 2020-12-11T23:03:46Z

/test pull-cloud-provider-aws-check

ayberk · 2020-12-11T23:16:39Z

/unhold

.gitignore

cmd/ecr-credential-provider/framework/plugin.go

cmd/ecr-credential-provider/main.go

andrewsykim · 2020-12-15T02:14:48Z

Makefile

+	 GO111MODULE=on CGO_ENABLED=0 GOOS=$(GOOS) GOPROXY=$(GOPROXY) go build \
+		-ldflags="-w -s -X 'main.version=$(VERSION)'" \
+		-o=ecr-credential-provider \
+		cmd/ecr-credential-provider/*.go


I don't think the *.go is necessary, can just be cmd/ecr-credential-provider or cmd/ecr-credential-provider/main.go

Unfortunately that doesn't work. I either need to build all go files together or move the framework into /pkg. Otherwise it complains about undefined NewCredentialProvider.

It's completely counter-intuitive to me, but based on my research go doesn't like having it in the main package.

Hmm, I haven't seen the need to use *.go, what do you mean by 'doesn't work'?

ip-172-31-62-224 cloud-provider-aws git:(ecr_creds_provider) ✗ 12/15 17:25 make ecr-credential-provider GO111MODULE=on CGO_ENABLED=0 GOOS=linux GOPROXY=https://proxy.golang.org,direct go build \ -ldflags="-w -s -X 'main.version=ec091a51-dirty'" \ -o=ecr-credential-provider \ cmd/ecr-credential-provider/main.go # command-line-arguments cmd/ecr-credential-provider/main.go:154:7: undefined: NewCredentialProvider Makefile:30: recipe for target 'ecr-credential-provider' failed make: *** [ecr-credential-provider] Error 2

Which is interesting because vscode can correctly locate it.

Ok I think we can just fix this in a follow up PR if necessary, not a big deal

nckturner · 2020-12-16T19:33:28Z

I'm going to merge this as I don't think there's anything that can't be addressed in a follow-up.
/lgtm
/approve

k8s-ci-robot · 2020-12-16T19:33:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ayberk, nckturner

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [nckturner]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 20, 2020

k8s-ci-robot requested review from andrewsykim and M00nF1sh November 20, 2020 01:06

k8s-ci-robot assigned andrewsykim and nckturner Nov 20, 2020

andrewsykim reviewed Nov 20, 2020

View reviewed changes

cmd/credential-providers/ecr/main.go Outdated Show resolved Hide resolved

ayberk force-pushed the ecr_creds_provider branch from 8fb78ed to e59fdb7 Compare November 20, 2020 23:27

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Nov 20, 2020

nckturner reviewed Nov 22, 2020

View reviewed changes

ayberk force-pushed the ecr_creds_provider branch 2 times, most recently from 0bbe3aa to 2987c1e Compare November 30, 2020 17:48

ayberk changed the title ~~[WIP] Add ECR creds provider~~ Add ECR creds provider Dec 2, 2020

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 2, 2020

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 2, 2020

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 3, 2020

ayberk force-pushed the ecr_creds_provider branch from 2987c1e to fbb1194 Compare December 11, 2020 22:40

ayberk force-pushed the ecr_creds_provider branch from fbb1194 to f4da31f Compare December 11, 2020 23:09

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 11, 2020

andrewsykim reviewed Dec 11, 2020

View reviewed changes

.gitignore Show resolved Hide resolved

cmd/ecr-credential-provider/framework/plugin.go Outdated Show resolved Hide resolved

cmd/ecr-credential-provider/main.go Outdated Show resolved Hide resolved

cmd/ecr-credential-provider/main.go Outdated Show resolved Hide resolved

ayberk force-pushed the ecr_creds_provider branch from f4da31f to f4d000b Compare December 14, 2020 18:03

Add creds provider for ECR

ec091a5

ayberk force-pushed the ecr_creds_provider branch from f4d000b to ec091a5 Compare December 15, 2020 01:07

andrewsykim reviewed Dec 15, 2020

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 16, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 16, 2020

k8s-ci-robot merged commit 86f15c2 into kubernetes:master Dec 16, 2020

ayberk mentioned this pull request Feb 2, 2021

REQUEST: New membership for ayberk kubernetes/org#2481

Closed

6 tasks

ayberk deleted the ecr_creds_provider branch February 2, 2021 23:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ECR creds provider #157

Add ECR creds provider #157

ayberk commented Nov 20, 2020 •

edited

Loading

k8s-ci-robot commented Nov 20, 2020

ayberk commented Nov 20, 2020

nckturner Nov 22, 2020

ayberk Nov 23, 2020

ayberk Nov 23, 2020

ayberk Nov 26, 2020

andrewsykim Nov 26, 2020

ayberk commented Dec 2, 2020

andrewsykim commented Dec 3, 2020

ayberk commented Dec 11, 2020

ayberk commented Dec 11, 2020

andrewsykim Dec 15, 2020

ayberk Dec 15, 2020

nckturner Dec 15, 2020

ayberk Dec 15, 2020

nckturner Dec 16, 2020

nckturner commented Dec 16, 2020

k8s-ci-robot commented Dec 16, 2020

Add ECR creds provider #157

Add ECR creds provider #157

Conversation

ayberk commented Nov 20, 2020 • edited Loading

k8s-ci-robot commented Nov 20, 2020

ayberk commented Nov 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ayberk commented Dec 2, 2020

andrewsykim commented Dec 3, 2020

ayberk commented Dec 11, 2020

ayberk commented Dec 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nckturner commented Dec 16, 2020

k8s-ci-robot commented Dec 16, 2020

ayberk commented Nov 20, 2020 •

edited

Loading