Multi arch image builds #45

RealHarshThakur · 2021-05-20T21:58:06Z

Fixes #21

Notes to reviewer/maintainer:

Secret named GCR_KEY needs to be created for this repo which should contain the Service Account token
Moving forward, we need to have branches with release names(v0.9, etc) and the image tag will reflect that

k8s-ci-robot · 2021-05-20T21:58:14Z

Welcome @RealHarshThakur!

It looks like this is your first PR to kubernetes-sigs/hierarchical-namespaces 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/hierarchical-namespaces has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2021-05-20T21:58:14Z

Hi @RealHarshThakur. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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.

adrianludwin · 2021-06-04T17:45:30Z

Sorry for the long delay, reviewing this now

adrianludwin · 2021-06-04T18:14:09Z

Secret named GCR_KEY needs to be created for this repo which should contain the Service Account token

@RealHarshThakur what does "secret" mean in this context? Does Github have its own concept of secrets? Sorry, I don't know the GH flow.

RealHarshThakur · 2021-06-04T18:26:35Z

Yes, GitHub can store secrets which will show up as environment variables inside the job and only the owner of the repo can set it. https://docs.github.com/en/actions/reference/encrypted-secrets

.github/workflows/image.yaml

adrianludwin · 2021-06-04T18:17:06Z

.github/workflows/image.yaml

+
+      - name: Set Release Tag
+        run: |
+          TAG="${GITHUB_REF#refs/*/v}"


What does this give you if you're running on master?

It gives it as refs/heads/master whereas when it's pushed to v1, it gives it as 1.

https://github.com/RealHarshThakur/hierarchical-namespaces/runs/2753272153?check_suite_focus=true#step:8:11
https://github.com/RealHarshThakur/hierarchical-namespaces/runs/2753141639?check_suite_focus=true

This means that we'll repeatedly overwrite the same tag, right? I'm not a huge fan of that, especially on the branches. Can we append the commit hash, e.g. something like hnc_manager:master-0a1b2c-multiarch for the master branch, and hnc_manager:0.9-3d4e5f-multiarch for the branches?

For the tags, it's probably ok to just have the tag name, e.g. hnc_manager:0.9.0-multiarch. We'll never build the same tag twice - if we need to do a release candidate, we'll include that in the tag name in Git (e.g. 0.9.0-rc1) and if we need to make a change to an existing tag, we'll just create a new one instead (e.g. 0.9.1).

We could also not push image for master branch. Only when a release is made and a branch is created, we create an image for it. We can have a separate image tag for Github Actions, just hope it doesn't confuse the end users. WDYT?

This applies to the release branches as well. It's only Git tags that only get built once.

I'd prefer if we could append some unique prefix onto each build that comes from a branch (and not a tag). Failing that, can we say -latest on it so people know not to rely on it being constant?

Nit: I suggest doing a +{date} to the tag as these are automatic builds. A 'real' release should likely only be retagging an intermediate build into the proper version. At least that would be my suggestion, especially since our release include more than just an image build. It also includes docs, kustomization files, and soon to be helm charts.

@RealHarshThakur I'm not seeing the date in the tab above, am I missing someething?

@rjbez17 added it here

I think that sets the Docker tag so that it includes the date, but RELEASE_TAG (which I'm guessing is the Github release tag) still includes only the branch name, correct? E.g., I'd prefer for this to be something like:

GITREF="${GITHUB_REF#refs/*/v}" echo "RELEASE_TAG=${GITREF}-${{ steps.date.outputs.HNC_BUILD_TIMESTAMP }}" >> $GITHUB_ENV

In other words, line up the RELEASE_TAG and the Docker tags as closely as possible.

@RealHarshThakur if I've misinterpreted please feel free to remove the hold. We can always clean it up later anyway.

.github/workflows/image.yaml

Makefile

adrianludwin · 2021-06-04T18:28:14Z

config/default/manager_auth_proxy_patch.yaml

-        image: gcr.io/kubebuilder/kube-rbac-proxy:v0.4.0
+        image: quay.io/brancz/kube-rbac-proxy:v0.10.0 


I'm not sure about this one. I trust the kubebuilder build process; I don't know who quay.io/brancz is or if we should trust their images. I'd prefer something like make manifests-multi so that people have to explicitly opt into using this version of kube-rbac-proxy from an untrusted source. Or even just tell people to modify the manifest themselves (e.g. change gcr.io/k8s-staging-multitenancy/hnc-manager:v0.9.0 to gcr.io/k8s-staging-multitenancy/hnc-manager:v0.9.0-multi, and change gcr.io/kubebuilder/kube-rbac-proxy:v0.4.0 to quay.io/brancz/kube-rbac-proxy:v0.10.0 at your own risk).

Is there a bug against kubebuilder to produce multiarch images?

I think the ownership has been passed and KubeBuilder maintainers are contributing to the same repo. Here's the repo and here's a comment by one of the maintainers suggesting to use the image(kubernetes-sigs/kubebuilder#1637 (comment)) .

Ah, ok, I see that github.com/brancz is the source-of-truth for kube-rbac-proxy. I'm just not sure about their supply chain. Let me ask and find out why kubebuilder thought it was necessary to have their own copies in the first place.

So I asked around, and apparently gcr.io/kubebuilder/kube-rbac-proxy:v0.8.0 is multi-architecture. I think I'd prefer to use that instead if it works.

The reason it's in gcr.io/kubebuilder is that this registry is owned by a Kubernetes SIG, while quay.io is not. This isn't an absolutely massive deal but it's why I'd prefer to follow kubebuilder's lead and use that image if it's reasonable to.

hack/buildx.sh

cloudbuild.yaml

RealHarshThakur · 2021-06-04T18:38:01Z

Thanks for the review @adrianludwin , will work on it tomorrow/day after tomorrow.

adrianludwin · 2021-06-04T18:50:46Z

Thanks! And I'll read up a bit more on GH actions.

…

On Fri, Jun 4, 2021 at 2:38 PM Harsh Thakur ***@***.***> wrote: Thanks for the review @adrianludwin <https://github.com/adrianludwin> , will work on it tomorrow/day after tomorrow. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#45 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AE43PZAU2V6POXFLOY6FZ43TREMRVANCNFSM45H6TNLQ> .

RealHarshThakur · 2021-06-05T15:17:01Z

@adrianludwin I made the changes you recommended at most places. Replied on the others, PTAL

.github/workflows/image.yaml

adrianludwin · 2021-06-07T13:28:38Z

.github/workflows/image.yaml

+
+      - name: Set Release Tag
+        run: |
+          TAG="${GITHUB_REF#refs/*/v}"


This means that we'll repeatedly overwrite the same tag, right? I'm not a huge fan of that, especially on the branches. Can we append the commit hash, e.g. something like hnc_manager:master-0a1b2c-multiarch for the master branch, and hnc_manager:0.9-3d4e5f-multiarch for the branches?

For the tags, it's probably ok to just have the tag name, e.g. hnc_manager:0.9.0-multiarch. We'll never build the same tag twice - if we need to do a release candidate, we'll include that in the tag name in Git (e.g. 0.9.0-rc1) and if we need to make a change to an existing tag, we'll just create a new one instead (e.g. 0.9.1).

.github/workflows/image.yaml

cloudbuild.yaml

Makefile

adrianludwin · 2021-06-07T14:25:44Z

config/default/manager_auth_proxy_patch.yaml

-        image: gcr.io/kubebuilder/kube-rbac-proxy:v0.4.0
+        image: quay.io/brancz/kube-rbac-proxy:v0.10.0 


Ah, ok, I see that github.com/brancz is the source-of-truth for kube-rbac-proxy. I'm just not sure about their supply chain. Let me ask and find out why kubebuilder thought it was necessary to have their own copies in the first place.

adrianludwin · 2021-06-07T14:29:38Z

/ok-to-test

Signed-off-by: Harsh Thakur <harshthakur9030@gmail.com>

RealHarshThakur · 2021-06-21T17:58:07Z

@adrianludwin sorry for the delay. I think I addressed most comments, PTAL.

adrianludwin

A couple last nits and then I think this looks good to start experimenting with.

/cc @rjbez17 - wdyt?

adrianludwin · 2021-06-24T16:20:22Z

.github/workflows/image.yaml

+    tags:
+      - 'v*'
+
+# TODO: Migrate building binaries from Cloud Build


s/binaries/manifests and kubectl plugins/

adrianludwin · 2021-06-24T16:22:47Z

.github/workflows/image.yaml

+
+      - name: Set Release Tag
+        run: |
+          TAG="${GITHUB_REF#refs/*/v}"


This applies to the release branches as well. It's only Git tags that only get built once.

I'd prefer if we could append some unique prefix onto each build that comes from a branch (and not a tag). Failing that, can we say -latest on it so people know not to rely on it being constant?

Dockerfile

adrianludwin · 2021-06-24T16:28:22Z

.github/workflows/image.yaml

+        # GCR_KEY is the service account key in json which is base64 encoded and stored in the secrets section of the repo.
+          service_account_key: ${{ secrets.GCR_KEY }}


Can you say anything about what this service account needs to be able to do? E.g., it must have permission to push to gcr.io/k8s-staging-multitenancy? If that's the requirement, how did you test this - by pushing to some different GCR repo?

I used the Default compute service account as shown in this blog and yes, I tested with a different repo.

Ah. That's bad advice, the default compute service account is terrible and generally shouldn't be used :D But there was no way for you to know that! I take it that the account was in the same GCP project as the repo you used to test?

Maybe just add the comment that we think the requirement for the service accounts it that it can push to the required registry (sorry, "registry" not "repo") but that we're not certain. We'll find out soon enough!

rjbez17

In the future it'd be nice if we split up seperate things into different PRs or at least commits. This PR does a bit more than just multiarch builds.

rjbez17 · 2021-06-24T18:35:22Z

.github/workflows/image.yaml

+
+      - name: Set Release Tag
+        run: |
+          TAG="${GITHUB_REF#refs/*/v}"


Nit: I suggest doing a +{date} to the tag as these are automatic builds. A 'real' release should likely only be retagging an intermediate build into the proper version. At least that would be my suggestion, especially since our release include more than just an image build. It also includes docs, kustomization files, and soon to be helm charts.

rjbez17 · 2021-06-24T18:39:59Z

lgtm other than the nit around the tagging. I personally think a 'real' release tag should be somewhat manual but open to discussion, but having intermediate builds pushed up will be very helpful.

adrianludwin · 2021-06-24T19:41:23Z

lgtm other than the nit around the tagging. I personally think a 'real' release tag should be somewhat manual but open to discussion, but having intermediate builds pushed up will be very helpful.

+1 to the +{date} suggestion for all automatically generated builds for now.

How would you have broken this up? I thought the majority was just "get github actions working" so maybe it's just the wrong title? The actual multiarch builds part seems pretty small.

rjbez17 · 2021-06-24T19:52:26Z

lgtm other than the nit around the tagging. I personally think a 'real' release tag should be somewhat manual but open to discussion, but having intermediate builds pushed up will be very helpful.

+1 to the +{date} suggestion for all automatically generated builds for now.

How would you have broken this up? I thought the majority was just "get github actions working" so maybe it's just the wrong title? The actual multiarch builds part seems pretty small.

Yeah so I mean there are three things, one really unrelated, that I see here:

Adding github actions to auto-build images based on triggers
Actual mutli-arch builds
Updating the rbac-proxy version (unless it is needed for multi-arch?) seemed like more of a 'while i'm here' thing.

Not a huge deal but in this case since it isn't a huge amount of code changes here but yeah.

rjbez17 · 2021-06-25T03:00:56Z

regardless, lgtm after the +{date} addition

Signed-off-by: Harsh Thakur <harshthakur9030@gmail.com>

rjbez17 · 2021-07-03T01:39:04Z

/lgtm
/approve
/hold

@adrianludwin want feel free to remove the hold

k8s-ci-robot · 2021-07-03T01:39:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RealHarshThakur, rjbez17

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~~ [rjbez17]

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

adrianludwin

One last comment, otherwise lgtm. Please feel free to remove the hold yourself (/hold remove) if I've misinterpreted, or if you just want to make that change in a followup PR.

adrianludwin · 2021-07-05T14:53:00Z

.github/workflows/image.yaml

+
+      - name: Set Release Tag
+        run: |
+          TAG="${GITHUB_REF#refs/*/v}"


I think that sets the Docker tag so that it includes the date, but RELEASE_TAG (which I'm guessing is the Github release tag) still includes only the branch name, correct? E.g., I'd prefer for this to be something like:

GITREF="${GITHUB_REF#refs/*/v}" echo "RELEASE_TAG=${GITREF}-${{ steps.date.outputs.HNC_BUILD_TIMESTAMP }}" >> $GITHUB_ENV

In other words, line up the RELEASE_TAG and the Docker tags as closely as possible.

@RealHarshThakur if I've misinterpreted please feel free to remove the hold. We can always clean it up later anyway.

RealHarshThakur · 2021-07-05T15:19:55Z

@adrianludwin so we're using branch name + timestamp for image tags. The release tag itself is being used in the metadata during docker build. The reason for not using release tag in the image was because branch names like master don't convert well on TAG="${GITHUB_REF#refs/*/v} . Please let me know if that's not okay

adrianludwin · 2021-07-05T15:22:22Z

Ok let's see what it looks like in the GH UI, that should clarify a lot and will tell us if we need more work. Thanks for being patient as we went through all these reviews, and congrats on getting this in! I know that was a lot of work but build systems and procedures are hard. This is a great contribution!

…

On Mon, Jul 5, 2021 at 11:20 AM Harsh Thakur ***@***.***> wrote: /hold remove — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#45 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AE43PZB2G4VRZO3ICCDIVHLTWHETHANCNFSM45H6TNLQ> .

RealHarshThakur · 2021-07-05T15:24:03Z

/remove-hold

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 20, 2021

k8s-ci-robot requested review from adrianludwin and srampal May 20, 2021 21:58

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 20, 2021

RealHarshThakur force-pushed the multi-arch branch from aa6f05a to b3527a2 Compare May 20, 2021 21:59

adrianludwin reviewed Jun 4, 2021

View reviewed changes

RealHarshThakur force-pushed the multi-arch branch from ceab51c to c6ae91b Compare June 5, 2021 15:14

adrianludwin reviewed Jun 7, 2021

View reviewed changes

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 Jun 7, 2021

RealHarshThakur force-pushed the multi-arch branch 3 times, most recently from 682f4c7 to 3ced647 Compare June 21, 2021 17:53

GHA for building and pushing images

39ff519

Signed-off-by: Harsh Thakur <harshthakur9030@gmail.com>

RealHarshThakur force-pushed the multi-arch branch from 3ced647 to 39ff519 Compare June 21, 2021 17:56

adrianludwin reviewed Jun 24, 2021

View reviewed changes

rjbez17 reviewed Jun 24, 2021

View reviewed changes

adrianludwin mentioned this pull request Jun 24, 2021

Disable kube-rbac-proxy by default #50

Merged

RealHarshThakur force-pushed the multi-arch branch from b8d5d1a to 7b3d33a Compare June 26, 2021 20:11

Add date to image tag

2f61d91

Signed-off-by: Harsh Thakur <harshthakur9030@gmail.com>

RealHarshThakur force-pushed the multi-arch branch from 7b3d33a to 2f61d91 Compare June 26, 2021 20:14

RealHarshThakur requested review from adrianludwin and rjbez17 July 2, 2021 05:20

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

k8s-ci-robot assigned rjbez17 Jul 3, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 3, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 3, 2021

adrianludwin reviewed Jul 5, 2021

View reviewed changes

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

k8s-ci-robot merged commit 8486518 into kubernetes-sigs:master Jul 5, 2021

JohannesLamberts mentioned this pull request Apr 19, 2022

HNC: Multiarch images (e.g. arm64) #201

Closed

		image: gcr.io/kubebuilder/kube-rbac-proxy:v0.4.0
		image: quay.io/brancz/kube-rbac-proxy:v0.10.0

		# GCR_KEY is the service account key in json which is base64 encoded and stored in the secrets section of the repo.
		service_account_key: ${{ secrets.GCR_KEY }}

Multi arch image builds #45

Multi arch image builds #45

Conversation

RealHarshThakur commented May 20, 2021 • edited Loading

k8s-ci-robot commented May 20, 2021

k8s-ci-robot commented May 20, 2021

adrianludwin commented Jun 4, 2021

adrianludwin commented Jun 4, 2021

RealHarshThakur commented Jun 4, 2021 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RealHarshThakur commented Jun 4, 2021

adrianludwin commented Jun 4, 2021 via email

RealHarshThakur commented Jun 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrianludwin commented Jun 7, 2021

RealHarshThakur commented Jun 21, 2021

adrianludwin left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjbez17 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjbez17 commented Jun 24, 2021

adrianludwin commented Jun 24, 2021

rjbez17 commented Jun 24, 2021

rjbez17 commented Jun 25, 2021

rjbez17 commented Jul 3, 2021

k8s-ci-robot commented Jul 3, 2021

adrianludwin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RealHarshThakur commented Jul 5, 2021

adrianludwin commented Jul 5, 2021 via email

RealHarshThakur commented Jul 5, 2021

RealHarshThakur commented May 20, 2021 •

edited

Loading

RealHarshThakur commented Jun 4, 2021 •

edited

Loading