Implement registries for Kubernetes backend #4092

meln5674 · 2024-09-07T04:48:00Z

According to the documentation, per-organization and per-pipeline registries are currently unsupported for the Kubernetes backend.

This patch implements this missing functionality by creating and deleting a matching secret for each pod with a matched registry, using the same name, labels, and annotations as the pod, and appending it to its imagePullSecrets list.

This patch adds tests for the new functionality, and has been manually end-to-end-tested in KinD by using a private image hosted in the matching gitea instance.

This will require updating the matching helm charts to add the create/delete permissions to the agent role, which I am happy to do as well.

* For each step that matches an registry, create a secret with the same name as its pod * Delete this same secret when the pod is deleted

6543 · 2024-09-07T05:40:50Z

Please also update the docs :)

zc-devs

Am I correct, that only one image pull secret is supported?
There was registry field in UI, as I remember. You don't use it, right?

https://woodpecker-ci.org/docs/usage/registries#images-from-private-registries

Woodpecker matches the registry hostname to each image in your YAML. If the hostnames match, the registry credentials are used to authenticate to your registry and pull the image.

Seems, registries are checked on the Server. As you create/delete secret each time, it should work as intended and comments above are invalid.
However, I still believe, that creating one secret with all available registries/auths and then "attach" it (via imagePullSecret) to needed steps/pods is a better approach.

zc-devs · 2024-09-07T08:12:36Z

pipeline/backend/kubernetes/image_pull_secret.go

+	return step.AuthConfig.Username != "" && step.AuthConfig.Password != ""
+}
+
+func mkImagePullSecret(step *types.Step, config *config, podName, goos string, options BackendOptions) (*v1.Secret, error) {


Unused goos.

There is secrets.go. Secrets stuff should be there, I think.

Removed goos, moved all secrets-related func's to secrets.go

zc-devs · 2024-09-07T08:18:27Z

pipeline/backend/kubernetes/image_pull_secret.go

+	if err != nil {
+		return nil, err
+	}
+	annotations := podAnnotations(config, options)


Why do you use Pod annotations on secret?

Probably, applied to labels also.

I assumed the user would want to label and annotate the secrets as well, but if you prefer a second pair of env vars for the secrets, I can do that.

Added 4 new env's for labels, annotations, and allow for both for registry secrets. Added separate func's to return labels and annotations specifically for registry secrets.

zc-devs · 2024-09-07T08:21:07Z

pipeline/backend/kubernetes/image_pull_secret_test.go

+				"part-of": "woodpecker-ci",
+				"step": "go-test"
+			},
+			"annotations": {


That's what I'm talking about. They are Pod specific.

zc-devs · 2024-09-07T08:23:30Z

pipeline/backend/kubernetes/image_pull_secret_test.go

+	  }
+	}`
+
+	pullSecret, err := mkImagePullSecret(&types.Step{


You don't need a half of options to test secrets.

Removed all non-relevant fields

zc-devs · 2024-09-07T08:26:56Z

pipeline/backend/kubernetes/pod.go

+			return nil, err
+		}
+		log.Trace().Msgf("creating secret: %s", pullSecret.Name)
+		_, err = engine.client.CoreV1().Secrets(engineConfig.Namespace).Create(ctx, pullSecret, meta_v1.CreateOptions{})


Make a function in image_pull_secrets.

zc-devs · 2024-09-07T08:27:56Z

pipeline/backend/kubernetes/pod.go

@@ -505,6 +514,18 @@ func startPod(ctx context.Context, engine *kube, step *types.Step, options Backe
 		return nil, err
 	}

+	if needsImagePullSecret(step) {


And probably call it in kubernetes.go. See how volumes work.

Created startRegistrySecret and stopRegistrySecret to mirror startPod/Volume and stopPod/Volume

zc-devs · 2024-09-07T08:29:00Z

pipeline/backend/kubernetes/pod.go

@@ -514,12 +535,23 @@ func stopPod(ctx context.Context, engine *kube, step *types.Step, deleteOpts met
 	if err != nil {
 		return err
 	}
+	errs := make([]error, 0, 2)
+	if needsImagePullSecret(step) {


The same as two above.

zc-devs · 2024-09-07T08:39:12Z

#2987

meln5674 · 2024-09-07T16:15:37Z

Please also update the docs :)

Can do, I wasn't sure if the docs site was live at HEAD or if there was some separate release process for them.

Am I correct, that only one image pull secret is supported?

There was registry field in UI, as I remember. You don't use it, right?

https://woodpecker-ci.org/docs/usage/registries#images-from-private-registries

Woodpecker matches the registry hostname to each image in your YAML. If the hostnames match, the registry credentials are used to authenticate to your registry and pull the image.

Seems, registries are checked on the Server. As you create/delete secret each time, it should work as intended and comments above are invalid. However, I still believe, that creating one secret with all available registries/auths and then "attach" it (via imagePullSecret) to needed steps/pods is a better approach.

In Kubernetes? No, any number of secrets is supported, with any number of auths in each one . In woodpecker, I saw that the AuthConfig for a step is a single user:pass pair, which is matched against the list of repo, org, and global registries, so in that sense, a single extra secret is supported. That said, in the future, if sidecars are also supported, it may make sense for this one secret to contain multiple auths. Because you can have as many imagePullSecrets as you want in a pod, it makes much more sense to list the user-managed ones and then the agent-managed ones, instead of reading them from the API server and writing them back out, as the kubelet will do that for you, and retry against each one in sequence in the event of authentication errors.

* Updated docs to remove unsupported functionality note and fix grammar * Use term "registry secret" in source to refer to temporary secrets created to contain registry authentication * Added separate env's to add extra labels and annotations to registry secrets * Consolidated registry secret functions into secrets.go * Moved secret creation/cleanup logic to separate functions in secret.go, called in kubernetes.go * Fixed whitespace errors and unused fields in test code

woodpecker-bot · 2024-09-08T00:15:59Z

Deploying preview to https://woodpecker-ci-woodpecker-pr-4092.surge.sh

zc-devs

Overall, looks very well to me.

Am I correct, that only one image pull secret is supported?
There was registry field in UI, as I remember. You don't use it, right?

I was confused by flashbacks from #3122. I thought, if registry isn't passed within AuthConfig, then how Kubernetes would choose right secret? Or the only one secret/registry would be supported.
Then I reread through my PR

Made the secrets pipeline-wide instead of step-wide as in Docker backend

and understood everything :) ⬇️

registries are checked on the Server. As you create/delete secret each time, it should work as intended

--

if sidecars are also supported, it may make sense for this one secret to contain multiple auths

Good catch.

instead of reading them from the API server and writing them back out

Don't get this. Who reads regcreds from API? Kubernetes API? What is the "back"?

kubelet will do that for you, and retry against each one

That is exactly, what I had in mind, implementing #3122. ⬇️

However, I still believe, that creating one secret with all available registries/auths and then "attach" it (via imagePullSecret) to needed steps/pods is a better approach.

Yeah, maybe even not to "needed" pods, but all of them. And let Kubernetes cope with that.

zc-devs · 2024-09-08T08:18:03Z

pipeline/backend/kubernetes/flags.go

+	&cli.StringFlag{
+		Sources: cli.EnvVars("WOODPECKER_BACKEND_K8S_REGISTRY_SECRET_LABELS"),
+		Name:    "backend-k8s-registry-secret-labels",
+		Usage:   "backend k8s additional Agent-wide registry image pull secret labels",
+		Value:   "",
+	},
+	&cli.BoolFlag{
+		Sources: cli.EnvVars("WOODPECKER_BACKEND_K8S_REGISTRY_SECRET_LABELS_ALLOW_FROM_STEP"),
+		Name:    "backend-k8s-registry-secret-labels-allow-from-step",
+		Usage:   "whether to allow using labels from step's backend options for registry image pull secrets",
+		Value:   false,
+	},
+	&cli.StringFlag{
+		Sources: cli.EnvVars("WOODPECKER_BACKEND_K8S_REGISTRY_SECRET_ANNOTATIONS"),
+		Name:    "backend-k8s-registry-secret-annotations",
+		Usage:   "backend k8s additional Agent-wide registry image pull secret annotations",
+		Value:   "",
+	},
+	&cli.BoolFlag{
+		Sources: cli.EnvVars("WOODPECKER_BACKEND_K8S_REGISTRY_SECRET_ANNOTATIONS_ALLOW_FROM_STEP"),
+		Name:    "backend-k8s-registry-secret-annotations-allow-from-step",
+		Usage:   "whether to allow using annotations from step's backend options for registry image pull secret",
+		Value:   false,
+	},


I assumed the user would want to label and annotate the secrets as well

Added 4 new env's for labels, annotations, and allow for both for registry secrets

I would remove it, don't see a reason to keep it.

For Pod there was some security/functional annotations. I don't know something like that for secrets.

As name of a secret is the same as the name of Pod, we can easily find out which Pod a secret belongs to.

There is no Agent-wide annotations for the volumes or the services.

If there would be a real need, we can easily add it later.

Fair enough. I tend to err on the side of flexibility with these sorts of things, but I guess it is in the commit history if we want it back.

Done.

zc-devs · 2024-09-08T08:40:17Z

pipeline/backend/kubernetes/secrets.go

+		},
+	}
+
+	configFileJSON, err := json.Marshal(authConfig)


Does it work without encoding to Base64?
If works, then OK. Otherwise, maybe we can generalize code with Docker (#3122, func (a Auth) EncodeToBase64() (string, error)).

Yes. If you're thinking of needing to use base64 in secret YAML files, that is accomplished by the encoding/json package "Array and slice values encode as JSON arrays, except that []byte encodes as a base64-encoded string". The serialized JSON form contains a base64 string, but the k8s data structure contains raw bytes.

I've tested prior to each push with a full install into a k8s cluster, both testing that a private image fails without the registry, and succeeds with it, so if it didn't work without base64, that would have caught it. I'm happy to share or commit this test setup script if you'd like.

I meant more auth field in the auths, than entire .dockerconfigjson in the secret.
https://kubernetes.io/docs/tasks/configure-pod-container/pull-image-private-registry/

{"auths":{"your.private.registry.example.com":{"username":"janedoe","password":"xxxxxxxxxxx","email":"[email protected]","auth":"c3R...zE2"}}}

To understand what is in the auth field, convert the base64-encoded data to a readable format:

$ echo "c3R...zE2" | base64 --decode janedoe:xxxxxxxxxxx

Ah, I see. In k8s, as far as I know, that field isn't actually required, and empirically, it works without it.

But don't take my word for it, here's the tests from kubernetes itself where they work with just username and password.

zc-devs

As it is tested and works (#4092 (comment)), no more comments from me.
Thank you for the contribution 🎉

Implement registries for Kubernetes backend

06eef97

* For each step that matches an registry, create a secret with the same name as its pod * Delete this same secret when the pod is deleted

qwerty287 added enhancement improve existing features backend/kubernetes build_pr_images If set, the CI will build images for this PR and push to Dockerhub labels Sep 7, 2024

zc-devs reviewed Sep 7, 2024

View reviewed changes

Fix linter findings

aa11aad

zc-devs reviewed Sep 8, 2024

View reviewed changes

Remove flags for labels/annotations on k8s registry secrets

3ce2091

zc-devs approved these changes Sep 8, 2024

View reviewed changes

meln5674 mentioned this pull request Sep 8, 2024

RBAC for Registry Secrets woodpecker-ci/helm#218

Merged

Merge branch 'main' into feature/kubernetes-image-pull-secrets

6b2390f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement registries for Kubernetes backend #4092

Implement registries for Kubernetes backend #4092

meln5674 commented Sep 7, 2024 •

edited

Loading

6543 commented Sep 7, 2024

zc-devs left a comment •

edited

Loading

zc-devs Sep 7, 2024

zc-devs Sep 7, 2024

meln5674 Sep 7, 2024

zc-devs Sep 7, 2024

zc-devs Sep 7, 2024

meln5674 Sep 7, 2024

meln5674 Sep 7, 2024

zc-devs Sep 7, 2024

zc-devs Sep 7, 2024

meln5674 Sep 7, 2024

zc-devs Sep 7, 2024

meln5674 Sep 7, 2024

zc-devs Sep 7, 2024

meln5674 Sep 7, 2024

zc-devs Sep 7, 2024

meln5674 Sep 7, 2024

zc-devs commented Sep 7, 2024

meln5674 commented Sep 7, 2024

woodpecker-bot commented Sep 8, 2024

zc-devs left a comment

zc-devs Sep 8, 2024

meln5674 Sep 8, 2024

zc-devs Sep 8, 2024

meln5674 Sep 8, 2024

zc-devs Sep 8, 2024

meln5674 Sep 8, 2024 •

edited

Loading

zc-devs left a comment

Implement registries for Kubernetes backend #4092

Are you sure you want to change the base?

Implement registries for Kubernetes backend #4092

Conversation

meln5674 commented Sep 7, 2024 • edited Loading

6543 commented Sep 7, 2024

zc-devs left a comment • 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

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

zc-devs commented Sep 7, 2024

meln5674 commented Sep 7, 2024

woodpecker-bot commented Sep 8, 2024

zc-devs 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

meln5674 Sep 8, 2024 • edited Loading

Choose a reason for hiding this comment

zc-devs left a comment

Choose a reason for hiding this comment

meln5674 commented Sep 7, 2024 •

edited

Loading

zc-devs left a comment •

edited

Loading

meln5674 Sep 8, 2024 •

edited

Loading