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

Add docs for autologin(ACR/ECR/GCR) #702

Merged
merged 4 commits into from
Feb 1, 2022

Conversation

somtochiama
Copy link
Member

@somtochiama somtochiama commented Jan 11, 2022

This pull request adds documentation for GCR/ACR auto-login Integration and also moves the docs on using cron jobs to a separate page.

Part of: fluxcd/flux2#2308

Signed-off-by: Somtochi Onyekwere somtochionyekwere@gmail.com

Signed-off-by: Somtochi Onyekwere <somtochionyekwere@gmail.com>
@stefanprodan stefanprodan added the area/docs Documentation related issues and pull requests label Jan 11, 2022
@stefanprodan stefanprodan changed the title Add docs for autologin(GCR/ACR) Add docs for autologin(ACR/ECR/GCR) Jan 11, 2022
Signed-off-by: Somtochi Onyekwere <somtochionyekwere@gmail.com>
@kingdonb
Copy link
Member

I'm testing the Google Cloud Workload Identity work, I ran into this error applying one of the patches from these examples:

02-gke-cluster-1      	False	kustomize build failed: add operation does not apply: doc is missing path: "/metadata/annotations/spec/": missing value	                                                   	False

I suspect it would work fine if I had used patchesStrategicMerge instead: kubernetes-sigs/kustomize#2986 (comment)

tl;dr from the link above, json patch 6902 cannot really be used to add a path that does not exist, if you have a hash then you can add a field to it, but if there is no hash "annotations" which I think there generally isn't, it will not be created in a json patch. Maybe this works if you annotate the service account by hand with the gcloud CLI, I'm still new at this.

I tried this patch instead and it worked:

apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
resources:
- gotk-components.yaml
- gotk-sync.yaml
patches:
- target:
    version: v1
    group: apps
    kind: Deployment
    name: image-reflector-controller
    namespace: flux-system
  patch: |-
    - op: add
      path: /spec/template/spec/containers/0/args/-
      value: --gcp-autologin-for-gcr
patchesStrategicMerge:
- |-
  apiVersion: v1
  kind: ServiceAccount
  metadata:
    name: image-reflector-controller
    namespace: flux-system
    annotations:
      iam.gke.io/gcp-service-account: kingdon-test-gcr-sa@dx-kingdon.iam.gserviceaccount.com
images: # []
- name: ghcr.io/fluxcd/image-reflector-controller
  newName: docker.io/somma/image-reflector-controller
  newTag: test-autologin-18df01042

I'm sending you some corrections for the issues I ran into 👍 the good news is, I was able to confirm the GKE workload identity can access my gcr.io/dx-kingdon/podinfo private repository and sync tags without issue:

$ flux get image repository
NAME   	READY	MESSAGE                      	LAST SCAN                	SUSPENDED
podinfo	True 	successful scan, found 2 tags	2022-01-14T09:02:47-05:00	False

I know I'm annotating the SA correctly now, because when it was incorrect I got a 400 "bad request" error. There is a . instead of an @ in your example annotation. I found references to all of the correct information in the guide you linked here (the example you gave there is also looks to be more correct):

@kingdonb
Copy link
Member

This is great work

It gets a 👍 from me, please note the suggestions I've made after testing GKE Workload Identity.

I'm going to grab the actual roles I assigned through the wizard and bring them back here so we can review them, also give the "artifact registry" a try since I've been able to get through "container registry" with Workload Identity and overcome all of the IAM hurdles, I think they are actually a different permission set, and that's what I'm hyper-focusing on here now.

If there are different role-permissions required for the SA to access Artifact Registry and Container Registry, then we should mention both roles in the doc.

Copy link
Member

@kingdonb kingdonb left a comment

Choose a reason for hiding this comment

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

👍 please adopt the suggestions or LMK if there are other changes

I am really pleased that I was able to make this work, it was not easy, but the information in this comment was super concise and very helpful to my overall understanding here:

content/en/docs/guides/image-update.md Outdated Show resolved Hide resolved
content/en/docs/guides/image-update.md Outdated Show resolved Hide resolved
content/en/docs/guides/cron-job-image-auth.md Show resolved Hide resolved
content/en/docs/guides/image-update.md Show resolved Hide resolved
@kingdonb
Copy link
Member

I can adopt this branch myself, and reissue it with my suggestions incorporated as a new PR (if that helps!)

@stefanprodan
Copy link
Member

@somtochiama please add @kingdonb to your fork so he can push to to your branch.

@somtochiama
Copy link
Member Author

@kingdonb I have sent out an invite to my fork of this repo

kingdonb pushed a commit to kingdonb/image-reflector-controller that referenced this pull request Jan 14, 2022
When fluxcd/website#702 merges, this patch will
have been incorporated into the Image Update Guide. Add a link to the
more directly relevant Image Update Guide here instead, since it has the
example, and it doesn't belong in the API docs nor need to be repeated
here, per the discussion from the earlier PR number fluxcd#193.

Signed-off-by: Kingdon Barrett <kingdon@weave.works>
Workload Identity (the same link repeated from cron-job-image-auth.md)

Signed-off-by: Kingdon Barrett <kingdon@weave.works>
Copy link
Member

@kingdonb kingdonb left a comment

Choose a reason for hiding this comment

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

👍 LGTM

@kingdonb
Copy link
Member

My approval is pending the merge of the following PR:

The draft status of this PR reflects that the depended on PR has not been merged yet.

Co-authored-by: Stefan Prodan <stefan.prodan@gmail.com>
Signed-off-by: Kingdon Barrett <kingdon@weave.works>
kingdonb pushed a commit to kingdonb/image-reflector-controller that referenced this pull request Jan 27, 2022
Squashed commits:

follow the convention of using footer-style links

I noticed that some links might not be good, and all of the new links
added in this PR were not following the convention of link targets in
the footer. Correct those issues.

Signed-off-by: Kingdon Barrett <kingdon@weave.works>

Update docs/spec/v1beta1/imagerepositories.md

Signed-off-by: Kingdon Barrett <kingdon@weave.works>

update docs

Signed-off-by: Kingdon Barrett <kingdon@weave.works>

Link to the Image Update guide

When fluxcd/website#702 merges, this patch will
have been incorporated into the Image Update Guide. Add a link to the
more directly relevant Image Update Guide here instead, since it has the
example, and it doesn't belong in the API docs nor need to be repeated
here, per the discussion from the earlier PR number fluxcd#193.

Signed-off-by: Kingdon Barrett <kingdon@weave.works>

remove orphaned link URLs

Signed-off-by: Kingdon Barrett <kingdon@weave.works>
kingdonb pushed a commit to kingdonb/image-reflector-controller that referenced this pull request Jan 27, 2022
Link to the Image Update guide

When fluxcd/website#702 merges, this patch will
have been incorporated into the Image Update Guide. Add a link to the
more directly relevant Image Update Guide here instead, since it has the
example, and it doesn't belong in the API docs nor need to be repeated
here, per the discussion from the earlier PR number fluxcd#193.

Signed-off-by: Kingdon Barrett <kingdon@weave.works>

remove orphaned link URLs

Signed-off-by: Kingdon Barrett <kingdon@weave.works>
Signed-off-by: Kingdon Barrett <yebyen@gmail.com>
kingdonb pushed a commit to kingdonb/image-reflector-controller that referenced this pull request Jan 27, 2022
Link to the Image Update guide

When fluxcd/website#702 merges, this patch will
have been incorporated into the Image Update Guide. Add a link to the
more directly relevant Image Update Guide here instead, since it has the
example, and it doesn't belong in the API docs nor need to be repeated
here, per the discussion from the earlier PR number fluxcd#193.

Signed-off-by: Kingdon Barrett <kingdon@weave.works>

remove orphaned link URLs

Signed-off-by: Kingdon Barrett <kingdon@weave.works>
Signed-off-by: Kingdon Barrett <yebyen@gmail.com>
kingdonb pushed a commit to kingdonb/image-reflector-controller that referenced this pull request Jan 31, 2022
Link to the Image Update guide

When fluxcd/website#702 merges, this patch will
have been incorporated into the Image Update Guide. Add a link to the
more directly relevant Image Update Guide here instead, since it has the
example, and it doesn't belong in the API docs nor need to be repeated
here, per the discussion from the earlier PR number fluxcd#193.

Signed-off-by: Kingdon Barrett <kingdon@weave.works>

remove orphaned link URLs

Signed-off-by: Kingdon Barrett <kingdon@weave.works>
Signed-off-by: Kingdon Barrett <yebyen@gmail.com>
@@ -0,0 +1,377 @@
---
title: "How to use cron jobs to sync image repository credentials"
linkTitle: "cron jobs for image repository cretentions"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
linkTitle: "cron jobs for image repository cretentions"
linkTitle: "Configure image automation authentication"

@@ -0,0 +1,377 @@
---
title: "How to use cron jobs to sync image repository credentials"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
title: "How to use cron jobs to sync image repository credentials"
title: "Configure image automation authentication"

@stefanprodan stefanprodan marked this pull request as ready for review February 1, 2022 10:58
Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

LGTM

I'll make the title changes in another PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/docs Documentation related issues and pull requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants