E2E checking Docker registry credentials in package repo #4927

castelblanque · 2022-06-15T12:35:17Z

Description of the change

Until now, E2E test '03-create-private-package-repository.spec.js' was testing two things:

Deploying a chart hosted in a repo with authentication
Adding Docker credentials and making an API request to check that imagePullSecrets was being filled in the CRD

Apart from that, E2E tests where failing when running from forked PRs. This was partially solved by @dlaloue-vmware with reorganizing the E2E script parameters.

This PR separates the 03 test into two separate ones, introducing the 10-deploy-package-with-private-image.spec.js.
The test makes use of a cluster local installed Docker registry with authentication.

Benefits

Better granularity of tests: one for app repo auth, one for Docker credentials
Docker credentials for a repo checked with a real deployment
No more DOCKER_USERNAME and DOCKER_PASSWORD needed from the CI environment
The above brings the ability for forked PRs to run E2E without needing CI context data

Possible drawbacks

Creation of local authenticated Docker registry brings some scripts using port-forward, which could introduce some instability when running.

Applicable issues

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

This reverts commit 03e85cf. Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

netlify · 2022-06-15T12:35:23Z

✅ Deploy Preview for kubeapps-dev canceled.

Name	Link
🔨 Latest commit	`8860d2e`
🔍 Latest deploy log	https://app.netlify.com/sites/kubeapps-dev/deploys/62bc1eaf84f1160008073050

castelblanque · 2022-06-15T12:35:54Z

.circleci/config.yml

@@ -398,7 +398,8 @@ load_kind_images: &load_kind_images
    name: "Load needed images into Kind"
    command:
      ./script/load-kind-image.sh docker.io/bitnami/apache:2.4.48-debian-10-r74 kubeapps-ci kubeapps-ci-additional &&
-      ./script/load-kind-image.sh docker.io/bitnami/apache:2.4.48-debian-10-r75 kubeapps-ci kubeapps-ci-additional
+      ./script/load-kind-image.sh docker.io/bitnami/apache:2.4.48-debian-10-r75 kubeapps-ci kubeapps-ci-additional &&
+      ./script/load-kind-image.sh registry:2 kubeapps-ci kubeapps-ci-additional


Preload registry image into Kind

castelblanque · 2022-06-15T12:36:10Z

.circleci/config.yml

@@ -501,7 +502,7 @@ run_e2e_tests: &run_e2e_tests
        DEV_TAG=${latest/v/}
        IMG_MODIFIER=""
      fi
-      if ./script/e2e-test.sh $USE_MULTICLUSTER_OIDC_ENV $OLM_VERSION $DEV_TAG $IMG_MODIFIER $TEST_TIMEOUT_MINUTES $DEFAULT_DEX_IP $ADDITIONAL_CLUSTER_IP $DOCKER_USERNAME $DOCKER_PASSWORD $KAPP_CONTROLLER_VERSION; then


Getting rid of this CI context variables

castelblanque · 2022-06-15T12:36:46Z

integration/charts/simplechart/values.yaml

@@ -0,0 +1,16 @@
+image:
+  repository: local-docker-registry:5600/nginx


Adding a test chart that makes use of an authenticated container image

castelblanque · 2022-06-15T12:37:01Z

integration/registry/local-registry.yaml

+  name: registry-auth
+  namespace: ci
+data:
+  htpasswd: dGVzdHVzZXI6JDJ5JDA1JHVLUnAvdUV1aEMxRXpmbzQ1a1NCbk96eUlnV2tKNmk3R1VKTzNhLjVvY0JrZUNBNTZjRWxXCgo=


Credentials for Docker registry

Perhaps a comment in the file just so it's obvious this is a local credential for the instantiated docker registry, so we don't panic in the future if we find this :P

Or another thought: I see below that you create the registry-tls secret as part of the script to setup the registry: why not also create this simple opaque secret there so that we only have the one location where the secret is defined and used? Just a thought, fine either way.

registry-tls is created from the script as it needs some externally generated input (the key file). I prefer having everything possible put in a static yaml file, that is why registry-auth is there, it does not depend on anything.
But agree with your suggestion, I'll add a comment.

castelblanque · 2022-06-15T12:38:18Z

integration/tests/main/03-create-private-package-repository.spec.js

-  const axInstance = await utils.getAxiosInstance(page, k.token);
-  const resourceResp = await axInstance.get(
-    `/apis/plugins/resources/v1alpha1/helm.packages/v1alpha1/c/default/ns/default/${appName}`,
-  );
-  expect(resourceResp.status).toEqual(200);
-
-  let deployment;
-  resourceResp.data
-    .trim()
-    .split(/\r?\n/)
-    .forEach(r => {
-      // Axios doesn't provide streaming responses, so splitting on new line works
-      // but gives us a string, not JSON, and may leave a blank line at the end.
-      const response = JSON.parse(r);
-      const resourceRef = response.result?.resourceRef;
-      if (resourceRef.kind === "Deployment" && resourceRef.name.match(appName)) {
-        deployment = JSON.parse(response.result?.manifest);
-      }
-    });
-
-  expect(deployment?.spec?.template?.spec?.imagePullSecrets).toEqual([{ name: secretName }]);


We no longer need this. Test '10' actually checks that the chart is correctly deployed using a container image from the authenticated Docker registry.

Right, so this test here is just testing repository authentication, whereas the other will test the image pull secrets. Nice.

castelblanque · 2022-06-15T12:39:24Z

script/install-local-registry.sh

@@ -0,0 +1,84 @@
+#!/bin/bash


Logic for setting up the Docker registry is placed in this separate script.

Might be worth still specifying

set -o errexit set -o nounset set -o pipefail

even though this script is currently sourced from the main e2e which already does this?

Did a quick check with errexit, and functions from sourced files get the same flags applied if invoked from the "sourcer". Therefore we don't need to add it again.

castelblanque · 2022-06-15T12:40:47Z

script/install-local-registry.sh

+    echo "127.0.0.1  $DOCKER_REGISTRY_HOST" | sudo tee -a /etc/hosts
+
+    docker pull nginx
+    docker tag nginx $DOCKER_REGISTRY/nginx
+
+    /bin/sh -c "kubectl -n ${REGISTRY_NS} port-forward service/docker-registry 5600:5600 &"


Solution far from elegant, but it does the trick for pushing from CI host into Docker registry inside cluster.

setting up a port-forward temporarily is OK I think, we do the same for chartmuseum. But the sleeps are more worrying in my opinion (as the time required is variable). It'd be nice if we could instead wait until the service is available (with a simple curl or something).

castelblanque · 2022-06-15T12:41:32Z

script/install-local-registry.sh

+    docker exec --user root $CONTROL_PLANE_CONTAINER update-ca-certificates -f
+
+    # Restart containerd to make the CA addition effective
+    docker exec --user root $CONTROL_PLANE_CONTAINER systemctl restart containerd


Key step in order to get CA certificates refreshed for Kubelet.

Nice - I bet that one line cost a bit... sometimes I wish it were possible to show how much one line costs after feeling like I've been banging my head against a wall :)

Woah, that's totally true... it seems a tricky one.

I thought this change in the CA could be done as a kubeadm config yaml during the cluster creation in kind (below), but you'll probably have explored many paths and this one is the simplest one for our use case, I guess, so +1 and thanks for coming up with it :P

apiVersion: kubeadm.k8s.io/v1beta3 kind: ClusterConfiguration controllerManager: extraArgs: cluster-signing-cert-file: /etc/kubernetes/pki/ca.crt cluster-signing-key-file: /etc/kubernetes/pki/ca.key

| I bet that one line cost a bit.

Finding that out unlocked the whole thing to me after banging my head against the wall.

| you'll probably have explored many paths

Indeed, tried many ways to get it working, including the use of /etc/kubernetes/pki/ca* for generating the new certificate. Didn't work.
It seemed to me that the issue is when the "kubelet" establishes a TLS connection to contact the image repository. Acting as a client, it uses a set of CAs available form the underlying OS in which the /etc/kubernetes/pki/ca.crt, for some reason, is not included. Therefore, I needed to add a new CA to sign the certificate.

castelblanque · 2022-06-15T12:43:46Z

integration/registry/local-registry.yaml

+        secret:
+          secretName: registry-tls
+      containers:
+        - image: registry:2


@antgamdia I guess this new dependency needs to be added to OSM.

Yep, you're right. So let's refrain from merging this PR until we trigger the release process, 🙏

absoludity

Great, thanks for this Rafa... much nicer than just checking the actual secret is included in the yaml.

absoludity · 2022-06-16T05:15:52Z

.circleci/config.yml

@@ -398,7 +398,8 @@ load_kind_images: &load_kind_images
    name: "Load needed images into Kind"
    command:
      ./script/load-kind-image.sh docker.io/bitnami/apache:2.4.48-debian-10-r74 kubeapps-ci kubeapps-ci-additional &&
-      ./script/load-kind-image.sh docker.io/bitnami/apache:2.4.48-debian-10-r75 kubeapps-ci kubeapps-ci-additional
+      ./script/load-kind-image.sh docker.io/bitnami/apache:2.4.48-debian-10-r75 kubeapps-ci kubeapps-ci-additional &&
+      ./script/load-kind-image.sh registry:2 kubeapps-ci kubeapps-ci-additional


Why not use the 2.8 tag or something more specific than the major version? (here and below in the deployment yaml) Maybe one less thing to update. Fine either way.

+1, and also I'd suggest: can we replace this registry:2 with sth like << pipeline.parameters.DOCKER_REGISTRY_VERSION>> so that upgrading the versions becomes as easy as changing a single param at the very beginning of the file?

absoludity · 2022-06-16T05:20:10Z

integration/registry/local-registry.yaml

+  name: registry-auth
+  namespace: ci
+data:
+  htpasswd: dGVzdHVzZXI6JDJ5JDA1JHVLUnAvdUV1aEMxRXpmbzQ1a1NCbk96eUlnV2tKNmk3R1VKTzNhLjVvY0JrZUNBNTZjRWxXCgo=


Perhaps a comment in the file just so it's obvious this is a local credential for the instantiated docker registry, so we don't panic in the future if we find this :P

absoludity · 2022-06-16T05:24:59Z

integration/tests/main/03-create-private-package-repository.spec.js

-  const axInstance = await utils.getAxiosInstance(page, k.token);
-  const resourceResp = await axInstance.get(
-    `/apis/plugins/resources/v1alpha1/helm.packages/v1alpha1/c/default/ns/default/${appName}`,
-  );
-  expect(resourceResp.status).toEqual(200);
-
-  let deployment;
-  resourceResp.data
-    .trim()
-    .split(/\r?\n/)
-    .forEach(r => {
-      // Axios doesn't provide streaming responses, so splitting on new line works
-      // but gives us a string, not JSON, and may leave a blank line at the end.
-      const response = JSON.parse(r);
-      const resourceRef = response.result?.resourceRef;
-      if (resourceRef.kind === "Deployment" && resourceRef.name.match(appName)) {
-        deployment = JSON.parse(response.result?.manifest);
-      }
-    });
-
-  expect(deployment?.spec?.template?.spec?.imagePullSecrets).toEqual([{ name: secretName }]);


Right, so this test here is just testing repository authentication, whereas the other will test the image pull secrets. Nice.

absoludity · 2022-06-16T05:43:39Z

script/install-local-registry.sh

@@ -0,0 +1,84 @@
+#!/bin/bash


Might be worth still specifying

set -o errexit set -o nounset set -o pipefail

even though this script is currently sourced from the main e2e which already does this?

absoludity · 2022-06-16T06:05:21Z

script/install-local-registry.sh

+    docker exec --user root $CONTROL_PLANE_CONTAINER update-ca-certificates -f
+
+    # Restart containerd to make the CA addition effective
+    docker exec --user root $CONTROL_PLANE_CONTAINER systemctl restart containerd


Nice - I bet that one line cost a bit... sometimes I wish it were possible to show how much one line costs after feeling like I've been banging my head against a wall :)

absoludity · 2022-06-16T06:12:21Z

integration/registry/local-registry.yaml

+  name: registry-auth
+  namespace: ci
+data:
+  htpasswd: dGVzdHVzZXI6JDJ5JDA1JHVLUnAvdUV1aEMxRXpmbzQ1a1NCbk96eUlnV2tKNmk3R1VKTzNhLjVvY0JrZUNBNTZjRWxXCgo=


Or another thought: I see below that you create the registry-tls secret as part of the script to setup the registry: why not also create this simple opaque secret there so that we only have the one location where the secret is defined and used? Just a thought, fine either way.

absoludity · 2022-06-16T06:24:01Z

script/install-local-registry.sh

+    # Generate new certificate for registry
+    mkcert -key-file $PROJECT_PATH/devel/localhost-key.pem -cert-file $PROJECT_PATH/devel/docker-registry-cert.pem $DOCKER_REGISTRY_HOST "docker-registry.$REGISTRY_NS.svc.cluster.local"
+    kubectl -n $REGISTRY_NS delete secret registry-tls --ignore-not-found=true
+    kubectl -n $REGISTRY_NS create secret tls registry-tls --key $PROJECT_PATH/devel/localhost-key.pem --cert $PROJECT_PATH/devel/docker-registry-cert.pem


As above, I wonder if it's worth creating the registry-auth here as well so you don't need to repeat the encoded secret in the yaml.

absoludity · 2022-06-16T06:26:23Z

script/install-local-registry.sh

+    echo "127.0.0.1  $DOCKER_REGISTRY_HOST" | sudo tee -a /etc/hosts
+
+    docker pull nginx
+    docker tag nginx $DOCKER_REGISTRY/nginx
+
+    /bin/sh -c "kubectl -n ${REGISTRY_NS} port-forward service/docker-registry 5600:5600 &"


setting up a port-forward temporarily is OK I think, we do the same for chartmuseum. But the sleeps are more worrying in my opinion (as the time required is variable). It'd be nice if we could instead wait until the service is available (with a simple curl or something).

antgamdia

Awesome, thanks! That's great we are finally testing repos with auth , it's gonna be a key aspect for properly testing the new repos API!

As I said in a comment, please do not merge the PR until we trigger the release, so that it does not interfere with the already reported deps to OSM.

antgamdia · 2022-06-18T15:51:42Z

.circleci/config.yml

@@ -398,7 +398,8 @@ load_kind_images: &load_kind_images
    name: "Load needed images into Kind"
    command:
      ./script/load-kind-image.sh docker.io/bitnami/apache:2.4.48-debian-10-r74 kubeapps-ci kubeapps-ci-additional &&
-      ./script/load-kind-image.sh docker.io/bitnami/apache:2.4.48-debian-10-r75 kubeapps-ci kubeapps-ci-additional
+      ./script/load-kind-image.sh docker.io/bitnami/apache:2.4.48-debian-10-r75 kubeapps-ci kubeapps-ci-additional &&
+      ./script/load-kind-image.sh registry:2 kubeapps-ci kubeapps-ci-additional


+1, and also I'd suggest: can we replace this registry:2 with sth like << pipeline.parameters.DOCKER_REGISTRY_VERSION>> so that upgrading the versions becomes as easy as changing a single param at the very beginning of the file?

integration/charts/simplechart/.helmignore

integration/charts/simplechart/Chart.yaml

integration/charts/simplechart/templates/_helpers.tpl

integration/charts/simplechart/templates/deployment.yaml

integration/charts/simplechart/values.yaml

integration/registry/local-registry.yaml

antgamdia · 2022-06-18T15:58:15Z

integration/registry/local-registry.yaml

+        secret:
+          secretName: registry-tls
+      containers:
+        - image: registry:2


Yep, you're right. So let's refrain from merging this PR until we trigger the release process, 🙏

script/install-local-registry.sh

antgamdia · 2022-06-18T16:07:31Z

script/install-local-registry.sh

+    docker exec --user root $CONTROL_PLANE_CONTAINER update-ca-certificates -f
+
+    # Restart containerd to make the CA addition effective
+    docker exec --user root $CONTROL_PLANE_CONTAINER systemctl restart containerd


Woah, that's totally true... it seems a tricky one.

I thought this change in the CA could be done as a kubeadm config yaml during the cluster creation in kind (below), but you'll probably have explored many paths and this one is the simplest one for our use case, I guess, so +1 and thanks for coming up with it :P

apiVersion: kubeadm.k8s.io/v1beta3 kind: ClusterConfiguration controllerManager: extraArgs: cluster-signing-cert-file: /etc/kubernetes/pki/ca.crt cluster-signing-key-file: /etc/kubernetes/pki/ca.key

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

Rafa Castelblanque added 11 commits June 13, 2022 11:49

Disable E2E tests for forked PRs

03e85cf

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

Revert "Disable E2E tests for forked PRs"

9a17cef

This reverts commit 03e85cf. Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

Disable E2E tests for forked PRs

d423435

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

Script to create local Docker registry with auth

eab49dd

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

Removed E2E ban for forked PRs

1a19aa1

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

Adding local Docker registry installation

d6db346

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

Small fixes

8cd8114

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

Merge branch 'main' into ci-disable-forked-e2e

dbd20de

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

Small fixes

0c4eb44

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

Added specific E2E test

697a239

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

Fixed simplechart deployment

da77153

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

vmwclabot added the cla-not-required label Jun 15, 2022

castelblanque commented Jun 15, 2022

View reviewed changes

absoludity approved these changes Jun 16, 2022

View reviewed changes

antgamdia approved these changes Jun 18, 2022

View reviewed changes

Rafa Castelblanque added 6 commits June 20, 2022 11:16

Code review changes

040dfe3

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

Removed sleep for scan waiting

ebecb23

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

Added some logging

69ecf30

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

Fixed applying the right input in kubectl

2a1c045

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

Removed debug log

eaea7af

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

Added missing header

8860d2e

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

castelblanque merged commit 33a46d7 into main Jun 29, 2022

castelblanque deleted the ci-disable-forked-e2e branch June 29, 2022 10:53

		@@ -0,0 +1,16 @@
		image:
		repository: local-docker-registry:5600/nginx

E2E checking Docker registry credentials in package repo #4927

E2E checking Docker registry credentials in package repo #4927

Conversation

castelblanque commented Jun 15, 2022

Description of the change

Benefits

Possible drawbacks

Applicable issues

netlify bot commented Jun 15, 2022 • edited

✅ Deploy Preview for kubeapps-dev canceled.

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

absoludity 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

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

antgamdia 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

netlify bot commented Jun 15, 2022 •

edited