Log in to AWS ECR automatically using the cluster-provided credentials. #147

MartinEmrich · 2021-06-01T18:43:24Z

As discussed in #139

This mostly restores the flux v1 functionality (for me)

if the environment variable USE_ECR is set, logs in to the AWS Elastic
Container Registry in the image URL by fetching a temporary token from
AWS.

As discussed in fluxcd#139 if the environment variable USE_ECR is set, logs in to the AWS Elastic Container Registry in the image URL by fetching a temporary token from AWS. Signed-off-by: Martin Emrich <martin@martinemrich.me>

…roller into main

controllers/imagerepository_controller.go

squaremo

I'd like to see this go ahead -- there's one thing I think is worth discussing (in a review comment), and one procedural thing: instead of merging main into this branch, please rebase this branch on main.

squaremo · 2021-06-24T16:12:16Z

controllers/imagerepository_controller.go

@@ -210,6 +265,28 @@ func (r *ImageRepositoryReconciler) scan(ctx context.Context, imageRepo *imagev1
 		options = append(options, remote.WithAuth(auth))
 	}

+	if accountId, awsEcrRegion, _, _, _, err := parseAwsImageURL(imageRepo.Spec.Image); err == nil {
+		if _, present := os.LookupEnv("USE_ECR"); present {


I would prefer this as a flag to the controller, rather than an environment variable. Really only because that's more in the spirit of controller-runtime with its config file support -- in most respects environment vars and flags work equally well.

I think it is appropriate for it to be set at the controller level, since it's up to the platform admin to grant global access to ambient authority, rather than for users to claim it. (If there is a reason to opt in per image repository, then fine, but I would still want the flag on the controller to explicitly grant access to everyone who wants it).

Yes, I'll try to find out how to access the command line and change the PR....

But I have no Idea what "instead of merging main into this branch, please rebase this branch on main" means...

@squaremo I guess I got the command-line option working. As for the rebase topic, I brushed up on Git theory, but I have no clue on how to execute it here on Github without damaging anything (Yes, it's my very first Github PR).

I have no clue on how to execute it here on Github without damaging anything (Yes, it's my very first Github PR).

First GitHub PR high five! ✋
I don't think GitHub has an in-app way to do this; it's something you would do on your local computer, after cloning your fork of this repo.

But I have no Idea what "instead of merging main into this branch, please rebase this branch on main" means...

Since you're working from a fork, you'll have to add this repo as a <remote>, first:

git remote add upstream https://github.com/fluxcd/image-reflector-controller

Also, it's worth working in a branch, and reserving main for keeping track of the upstream main branch:

git switch -c ecr-auto-creds git push -u origin ecr-auto-creds

(The second command pushes the new branch to your forked repo, and -u sets it as the default place to push any further commits on that branch)

Then, rebasing on the main branch here can be done with

git pull upstream main --rebase

A lot of the time, this will succeed without any more work from you. If you get into trouble, you can git rebase --abort to take you back to the state before the rebase (or pull --rebase).

To push the branch back to GitHub, you'll need to use --force-with-lease, because you've replaced the commits in the branch:

git push --force-with-lease # it knows where to push to, because of `-u` above

@squaremo Thanks! I followed your instructions, and all commands succeeded. But apparently it is not possible to change the source branch of a Github PR, so it's still from my "main" branch :(

(... and quote a dot in the ECR URL pattern regexp) Signed-off-by: Martin Emrich <martin@martinemrich.me>

Signed-off-by: Martin Emrich <martin@martinemrich.me>

Signed-off-by: Michael Bridgen <michael@weave.works>

Release v0.11.0

Signed-off-by: Michael Bridgen <michael@weave.works>

Correct apiVersion of v1beta1 samples

hiddeco · 2021-07-14T09:48:06Z

main.go

@@ -79,6 +80,8 @@ func main() {
 	flag.StringVar(&storagePath, "storage-path", "/data", "Where to store the persistent database of image metadata")
 	flag.Int64Var(&storageValueLogFileSize, "storage-value-log-file-size", 1<<28, "Set the database's memory mapped value log file size in bytes. Effective memory usage is about two times this size.")
 	flag.IntVar(&concurrent, "concurrent", 4, "The number of concurrent resource reconciles.")
+	flag.BoolVar(&useAwsEcr, "use-aws-ecr", false, "Log in to AWS Elastic Container Registry with IAM")


Wondering if a flag like --with-aws-ecr-iam or simply --enable-aws-iam would be more descriptive. In my opinion, you only actually start making use of AWS Elastic Container Registry once a scan detects it.

hiddeco · 2021-07-14T09:50:12Z

controllers/imagerepository_controller.go

+			auth := authn.FromConfig(authConfig)
+			options = append(options, remote.WithAuth(auth))
+		} else {
+			logr.FromContext(ctx).Info("AWS ECR authentication is not enabled, to enable, set USE_ECR environment variable")


Not sure if implementation details from another layer (set USE_ECR) should be mentioned here, it may be better (and less maintenance heavy) to simply refer to the feature.

kingdonb · 2021-07-22T17:45:42Z

This looks like it needs to be rebased before it can be approved. (Is there any other feedback that was left here, which still needs to be addressed?)

MartinEmrich · 2021-07-22T18:04:02Z

@kingdonb I am very sorry for the long inactivity, I am quite busy atm... I'll try to follow squaremos rebase tutorial above ASAP.

As discussed in fluxcd#139 if the environment variable USE_ECR is set, logs in to the AWS Elastic Container Registry in the image URL by fetching a temporary token from AWS. Signed-off-by: Martin Emrich <martin@martinemrich.me>

(... and quote a dot in the ECR URL pattern regexp) Signed-off-by: Martin Emrich <martin@martinemrich.me>

Signed-off-by: Martin Emrich <martin@martinemrich.me>

…ller

kingdonb · 2021-07-22T19:01:22Z

Sorry for the interruption! This PR got a mention in our bug scrub event today, I think there is no urgency, it was just the right age and staleness for this issue to come to the top of our list and get our attention, I'm glad to see it moving ahead.

Thank you for doing the rebase; it looks like the next step is for a maintainer to approve the test execution and have another look for review.

Also, 🙌
– first time Flux contributor high-five!

tibz-enex · 2021-09-06T07:35:51Z

Hello all,

Nice work being performed here, we are looking forward to using it. As of now, we use a pods that retrieves a token every 6 hours and stores it as kubesecret to be used by the image reflector controller.
Do you have any idea when this PR could be merged?

Thanks.

squaremo · 2021-09-23T12:37:59Z

I'm going to rebase this and put it in another PR -- usually I would rebase and push to the fork, but I don't want to mess with someone's main. @MartinEmrich you will still get the credit, don't worry ;-)

MartinEmrich · 2021-09-23T13:06:44Z

@squaremo sure, do whatever you have to, no worries! :)

THB, navigating all these processes with git, forks, branches, rebases, checks, reviews and stuff seems to be more hassle than implementing these few lines of code... Maybe I'm getting old, but in hindsight diff -Naur+email was easier.
So I apologize if my ignorance held this up more than necessary.

squaremo · 2021-09-23T14:55:29Z

Maybe I'm getting old, but in hindsight diff -Naur+email was easier

Ha yep, fair enough. I think it's worth brushing up on git in general, and all those things you mention get easier, but it is steep hill to climb to scratch just one itch*, that's true.

(*A mixed metaphor should be derailed, no matter how it sings!)

squaremo · 2021-10-14T11:20:45Z

Closing in favour in #174, which repackaged the change for more convenient review. Thank you for your initiative and efforts, @MartinEmrich 🍏

…d): elaborate on how to enable autologin for ECR fluxcd/flux#1619 fluxcd/flux2#107 fluxcd#11 fluxcd#139 fluxcd#147 fluxcd#174 fluxcd#179 fluxcd#180 fluxcd/website#601 fluxcd#193 fluxcd/website#611 https://fluxcd.io/docs/guides/image-update/#aws-elastic-container-registry Signed-off-by: lloydchang <lloydchang@gmail.com>

…d): elaborate on how to enable autologin for ECR fluxcd/flux#1619 fluxcd/flux2#107 fluxcd#11 fluxcd#139 fluxcd#147 fluxcd#174 fluxcd#179 fluxcd#180 fluxcd/website#601 fluxcd#193 fluxcd/website#611 https://fluxcd.io/docs/guides/image-update/#aws-elastic-container-registry Signed-off-by: lloydchang <lloydchang@gmail.com> Signed-off-by: Kingdon Barrett <kingdon@weave.works>

MartinEmrich force-pushed the main branch from 3e61740 to fd5a039 Compare June 1, 2021 18:49

Merge branch 'main' of https://github.com/fluxcd/image-reflector-cont…

b7e90af

…roller into main

makkes reviewed Jun 23, 2021

View reviewed changes

controllers/imagerepository_controller.go Outdated Show resolved Hide resolved

squaremo reviewed Jun 24, 2021

View reviewed changes

MartinEmrich added 2 commits June 24, 2021 18:29

Merge branch 'main' into main

133d1e3

Enable AWS ECR login with flag instead of env variable

a56624b

(... and quote a dot in the ECR URL pattern regexp) Signed-off-by: Martin Emrich <martin@martinemrich.me>

MartinEmrich force-pushed the main branch from b26d236 to a56624b Compare June 24, 2021 18:05

MartinEmrich and others added 7 commits June 24, 2021 20:08

removed (as of now) unused return values

dd9fbbf

Signed-off-by: Martin Emrich <martin@martinemrich.me>

Bump API version to v0.11.0

22f2317

Signed-off-by: Michael Bridgen <michael@weave.works>

Write changelog entry for v0.11.0

4b48cf1

Signed-off-by: Michael Bridgen <michael@weave.works>

Bump controller version used in config

5c94f3d

Signed-off-by: Michael Bridgen <michael@weave.works>

Merge pull request fluxcd#155 from fluxcd/release-v0.11.0

4542bd0

Release v0.11.0

Correct apiVersion of v1beta1 samples

5da1152

Signed-off-by: Michael Bridgen <michael@weave.works>

Merge pull request fluxcd#158 from fluxcd/samples-to-v1beta1

c22a8d0

Correct apiVersion of v1beta1 samples

hiddeco reviewed Jul 14, 2021

View reviewed changes

MartinEmrich added 4 commits July 22, 2021 20:11

Enable AWS ECR login with flag instead of env variable

801d106

(... and quote a dot in the ECR URL pattern regexp) Signed-off-by: Martin Emrich <martin@martinemrich.me>

removed (as of now) unused return values

782d111

Signed-off-by: Martin Emrich <martin@martinemrich.me>

Merge branch 'main' of github.com:MartinEmrich/image-reflector-contro…

4a461c1

…ller

kingdonb self-assigned this Jul 22, 2021

kingdonb removed their assignment Jul 22, 2021

squaremo mentioned this pull request Sep 30, 2021

Get credentials from AWS ECR when needed #174

Merged

4 tasks

squaremo closed this Oct 14, 2021

lloydchang mentioned this pull request Oct 31, 2021

docs(image-reflector-controller/docs/spec/v1beta1/imagerepositories.md): elaborate on how to enable autologin for ECR kingdonb/image-reflector-controller#1

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Log in to AWS ECR automatically using the cluster-provided credentials. #147

Log in to AWS ECR automatically using the cluster-provided credentials. #147

MartinEmrich commented Jun 1, 2021

squaremo left a comment

squaremo Jun 24, 2021

MartinEmrich Jun 24, 2021

MartinEmrich Jun 25, 2021

squaremo Jun 28, 2021

MartinEmrich Jul 22, 2021

hiddeco Jul 14, 2021

hiddeco Jul 14, 2021

kingdonb commented Jul 22, 2021

MartinEmrich commented Jul 22, 2021

kingdonb commented Jul 22, 2021 •

edited

Loading

tibz-enex commented Sep 6, 2021

squaremo commented Sep 23, 2021

MartinEmrich commented Sep 23, 2021

squaremo commented Sep 23, 2021

squaremo commented Oct 14, 2021

Log in to AWS ECR automatically using the cluster-provided credentials. #147

Log in to AWS ECR automatically using the cluster-provided credentials. #147

Conversation

MartinEmrich commented Jun 1, 2021

squaremo left a comment

Choose a reason for hiding this comment

squaremo Jun 24, 2021

Choose a reason for hiding this comment

MartinEmrich Jun 24, 2021

Choose a reason for hiding this comment

MartinEmrich Jun 25, 2021

Choose a reason for hiding this comment

squaremo Jun 28, 2021

Choose a reason for hiding this comment

MartinEmrich Jul 22, 2021

Choose a reason for hiding this comment

hiddeco Jul 14, 2021

Choose a reason for hiding this comment

hiddeco Jul 14, 2021

Choose a reason for hiding this comment

kingdonb commented Jul 22, 2021

MartinEmrich commented Jul 22, 2021

kingdonb commented Jul 22, 2021 • edited Loading

tibz-enex commented Sep 6, 2021

squaremo commented Sep 23, 2021

MartinEmrich commented Sep 23, 2021

squaremo commented Sep 23, 2021

squaremo commented Oct 14, 2021

kingdonb commented Jul 22, 2021 •

edited

Loading