Add smoke tests #5

maelvls · 2021-02-05T17:51:58Z

The smoke tests can be seen in ./test-suite.yaml.

TODO:

Find a way to not have location and projectId hardcoded
~~Make the google-cas-issuer test work~~
→ will do that in next milestone
- ~~Find a workaround for the issue with the Kubernetes → Google serviceaccount binding~~
  → will do that in next milestone
- ~~Find a workaround for the issue with the Google → Kubernetes serviceaccount binding~~
  → will do that in next milestone
Make the test suite pass
Need documentation in the readme

maelvls · 2021-02-05T21:17:16Z

Yay, the tests seem to be finally running! They fail... but at least I got them running!

│ I0205 21:10:20.453918       1 main.go:86] >>> Running /test-suite.yaml                                                                                                      │
│ I0205 21:10:20.455308       1 main.go:136]  >   0: kubectl smoke test                                                                                                       │
│ I0205 21:10:20.571113       1 main.go:141]  PASSED                                                                                                                          │
│ I0205 21:10:20.571175       1 main.go:136]  >   1: Create test issuer and self signed cert                                                                                  │
│ E0205 21:10:21.843313       1 main.go:143]  FAILED: Bash test failed > Unexpected exit status code > Should have equaled 0, but was 1                                       │
│ I0205 21:10:21.843363       1 main.go:136]  >   2: Try to get new cert

Issues:

mpdev verify does not stream the logs of the apptest... so the only way is to run this in my own terminal while the build is running:
```
k logs -n $(k get ns -oname | grep apptest) -l marketplace.cloud.google.com/verification=test -f --tail=-1
```
very tricky to do the the "binding" command. Right now I only annotate the google-cas-issuer's Kubernetes serviceaccount with the Kubernetes → Google binding, but I still need to figure out how to do the Google → Kubernetes binding.

I would have to have the following command run inside the deployer...
```
   gcloud iam service-accounts add-iam-policy-binding sa-google-cas-issuer@$(gcloud config get-value project | tr ':' '/').iam.gserviceaccount.com \
   --role roles/iam.workloadIdentityUser \
   --member "serviceAccount:$(gcloud config get-value project | tr ':' '/').svc.id.goog[test-ns/test-google-cas-issuer-serviceaccount-name]"
```
But where do I get the serviceaccount from in order to run this from inside a job in the deployer??

maelvls · 2021-02-08T09:21:05Z

The solutions I thought about so far:

use mpdev verify with a fixed namespace and a fixed release name so that I can do the Google → Kubernetes serviceaccount before-hand and do the Kubernetes → Google SA binding in a job as part of the deployer's Helm chart. I think I'll end up doing that.
do the Google → Kubernetes SA binding during the test itself in test-suite.yaml and make sure to create a Google serviceaccount + credentials.json that has the resourcemanager.projects.setIamPolicy permission (about iam), probably would require a new role since the existing roles are quite permissive:
do both bindings in the job as part of the deployer's Helm chart

maelvls · 2021-02-09T12:52:50Z

I tried implementing solution (1) i.e., using a fixed namespace and release name with mpdev verify. And I realized I can't even use a fixed name and namespace; from the mpdev doc:

The mpdev verify command does not take parameters as in install command. Instead, it uses the default values declared in the schema file.

If a property has no default value declared in /data/schema.yaml, it has to be declared in /data-test/schema.yaml (with exception of NAME and NAMESPACE, which are auto-generated during verification).

maelvls · 2021-02-09T14:56:36Z

Ooook I think I'll just skip the google-cas-issuer testing for now since (2) and (3) are equally painful and I think it is better to have at least cert-manager tested alone than not having any smoke tests at all

maelvls · 2021-02-11T18:32:11Z

After a discussion with James W, I decided to just drop the smoke test for google-cas-issuer for now and just do smoke tests for cert-manager itself

We will do the google-cas-issuer tests later 😅

tbd: create an issue that reminds us to do those google-cas-issuer smoke tests

wallrj

I read through the changes and it all looks really cool, but I guess you've still got to remove the google-cas-issuer smoke test changes, in light of your comment above.

I'll hold off testing it until you're ready.

Dockerfile

maelvls · 2021-02-11T20:01:18Z

Btw, do you know how we could be running that in GitHub Actions? Not sure which jsonkey we should be storing in a GitHub Actions secret: would it be a service account on the jetstack-public project?

Add smoke tests Signed-off-by: Maël Valais <mael@vls.dev> Co-authored-by: Richard Wall <richard.wall@jetstack.io>

Signed-off-by: Maël Valais <mael@vls.dev> Co-authored-by: Richard Wall <richard.wall@jetstack.io>

helm does not like dashs in yaml keys With: {{ .Values.google-cas-issuer.serviceAccount.name }} I would get the error: Error: parse error at googlecasissuer.yaml:40: bad character U+002D '-' The trick is to use the "index" function from the Go templates language: {{ index .Values "google-cas-issuer" "serviceAccount" "name" }} See: helm/helm#2192 (comment) Signed-off-by: Maël Valais <mael@vls.dev> Co-authored-by: Richard Wall <richard.wall@jetstack.io>

tar xf the data-test/chart too Signed-off-by: Maël Valais <mael@vls.dev> Co-authored-by: Richard Wall <richard.wall@jetstack.io>

Signed-off-by: Maël Valais <mael@vls.dev>

This is a Google Marketplace requirement, c.f. schema.md: > Note that the images share a common prefix gcr.io/project/company/app, > which is set externally to the schema.yaml file, when you onboard your > app for publishing. If your app contains a primary image, its repository > must exactly match the common prefix of the images. In our case, the "primary" image is cert-manager-controller and is tagged as: gcr.io/jetstack-public.jetstack-secure-for-cert-manager:1.0.0 For all the other images, it looks like: gcr.io/jetstack-public/jetstack-secure-for-cert-manager/cert-manager-webhook:1.0.0 Signed-off-by: Maël Valais <mael@vls.dev>

Signed-off-by: Maël Valais <mael@vls.dev> Co-authored-by: Richard Wall <wallrj@users.noreply.github.com>

Signed-off-by: Maël Valais <mael@vls.dev> Co-authored-by: Richard Wall <richard.wall@jetstack.io>

wallrj

Looks good @maelvls

Just a few suggestions which you can ignore if you prefer.

I haven't run the cloudbuild script, but I think it makes sense.

Please unhold and merge when ready.

/hold
/lgtm
/unassign
/assign @maelvls

wallrj · 2021-02-16T14:01:05Z

README.md

+
+```sh
+# The primary image "cert-manager-controller":
+gcr.io/jetstack-public/jetstack-secure-for-cert-manager:1.0.0


In https://github.com/GoogleCloudPlatform/marketplace-k8s-app-tools/blob/master/docs/schema.md#image-declaration-and-parameterization I noticed that it says:

x-google-marketplace: images: '': # Primary image has no name.

Perhaps we should do the same.

Done 👍

images: # The Marketplace requires us to use a "primary image". In our case, # this is cert-manager-controller. See "primary image" in # https://github.com/GoogleCloudPlatform/marketplace-k8s-app-tools/blob/d9d3a6/docs/schema.md "": # This is cert-manager-controller. properties: cert-manager.image.repository: type: REPO_WITH_REGISTRY cert-manager.image.tag: type: TAG

chart/jetstacksecure-mp/charts/preflight/templates/deployment.yaml

wallrj · 2021-02-16T14:04:17Z

cloudbuild.yaml

      - tag
      - quay.io/jetstack/cert-manager-controller:v${_CERT_MANAGER_VERSION}
-      - gcr.io/$PROJECT_ID/${_SOLUTION_NAME}/cert-manager-controller:${_APP_VERSION}
+      - gcr.io/$PROJECT_ID/${_SOLUTION_NAME}:${_APP_VERSION}


Ah, this explains the comment that I hightlighed above, in schema.md

wallrj · 2021-02-16T14:08:38Z

cloudbuild.yaml

+        pod=$(kubectl -n "$ns" get pods -oname | grep "apptest-.*-deployer" | cut -d/ -f2)
+
+        kubectl wait -n "$ns" --for=condition=ready pod $pod
+        kubectl logs -n "$ns" $pod -f --tail=-1


You could do kubectl logs deploy/my-deployment, assuming that the code above is to get the name of a Deployment controlled Pod.
A suggestion only. Leave it as is if you prefer to get this merged.

🤯 I wansnt aware that we could request logs for a given deployment!! Thank you for the tip!!

Unfortunately the deployer is running as a job... but I could also very well use

kubectl logs job/apptest-1aef45-deployer-job

wallrj · 2021-02-16T14:11:36Z

smoke-test.yaml

+    bashTest:
+      script: |
+        kubectl apply --namespace ${NAMESPACE} -f - <<EOF
+        apiVersion: cert-manager.io/v1alpha2


Maybe use cert-manager v1 API here.

wallrj · 2021-02-16T14:13:12Z

smoke-test.yaml

+                  -o=jsonpath='{.status.conditions[0].status}' \
+                  | grep -qz True);
+            do sleep 2;
+          done'


Maybe use kubectl wait --for condition=Ready certificate selfsigned-cert

I might have copy-pasted this test case from click-to-deploy... looks like they forgot about kubectl wait 😅

wallrj · 2021-02-16T14:14:00Z

smoke-test.yaml

+  # the two together using the "workload identity" feature.
+  #
+  # Right now, the above GoogleCASIssuer does nothing, and the certificate
+  # will never be created.


This is due to a Docker limitation. Signed-off-by: Maël Valais <mael@vls.dev>

Signed-off-by: Maël Valais <mael@vls.dev>

Signed-off-by: Maël Valais <mael@vls.dev> Co-authored-by: Richard Wall <richard.wall@jetstack.io>

maelvls · 2021-02-16T15:51:44Z

/unhold
/unassign
/assign @wallrj

wallrj

Just one tiny nit, but please unhold if you think it works as-is

/lgtm
/hold
/unassign
/assign @maelvls

wallrj · 2021-02-16T16:07:10Z

smoke-test.yaml

-                  | grep -qz True);
-            do sleep 2;
-          done'
+        kubectl wait -n ${NAMESPACE} --for=condition=ready certificate selfsigned-cert


I thought the condition name was case-sensitive, but I might be wrong.
See https://manpages.debian.org/testing/kubernetes-client/kubectl-wait.1.en.html#EXAMPLE

Suggested change

kubectl wait -n ${NAMESPACE} --for=condition=ready certificate selfsigned-cert

kubectl wait -n ${NAMESPACE} --for=condition=Ready certificate selfsigned-cert

I think you are right 😅

The lower-case version also seems to work though; digging into the kubectl codebase, it seems like the comparison between the condition types (e.g. Ready) is done in here using string.EqualFold:

EqualFold reports whether s and t, interpreted as UTF-8 strings, are equal under Unicode case-folding, which is a more general form of case-insensitivity.

Signed-off-by: Maël Valais <mael@vls.dev> Co-authored-by: Richard Wall <wallrj@users.noreply.github.com>

maelvls · 2021-02-16T16:12:42Z

/unhold

wallrj

/lgtm

jetstack-bot added the size/XXL label Feb 5, 2021

maelvls mentioned this pull request Feb 8, 2021

Add the google-cas-issuer to the Google Marketplace release #4

Merged

maelvls added this to the Beta milestone Feb 9, 2021

maelvls self-assigned this Feb 9, 2021

maelvls force-pushed the add-smoke-tests branch from a7fe4c2 to c4252aa Compare February 10, 2021 15:52

jetstack-bot added size/L and removed size/XXL labels Feb 10, 2021

wallrj mentioned this pull request Feb 10, 2021

Deploy the preflight agent #15

Merged

maelvls force-pushed the add-smoke-tests branch from f371301 to 68b99e0 Compare February 11, 2021 18:33

jetstack-bot added the needs-rebase label Feb 11, 2021

maelvls force-pushed the add-smoke-tests branch from 68b99e0 to a0d038a Compare February 11, 2021 18:35

jetstack-bot removed the needs-rebase label Feb 11, 2021

wallrj reviewed Feb 11, 2021

View reviewed changes

Dockerfile Show resolved Hide resolved

maelvls and others added 4 commits February 12, 2021 17:34

8c6aa03

Add smoke tests Signed-off-by: Maël Valais <mael@vls.dev> Co-authored-by: Richard Wall <richard.wall@jetstack.io>

Add job for annotating the cas-issuer serviceaccount

be3e960

Signed-off-by: Maël Valais <mael@vls.dev> Co-authored-by: Richard Wall <richard.wall@jetstack.io>

f34b96c

tar xf the data-test/chart too Signed-off-by: Maël Valais <mael@vls.dev> Co-authored-by: Richard Wall <richard.wall@jetstack.io>

maelvls force-pushed the add-smoke-tests branch from a0d038a to ef9fbf0 Compare February 12, 2021 16:35

maelvls added 2 commits February 12, 2021 19:22

Pass a jsonkey to the smoketest job for CAS issuer

0c2dc1b

Signed-off-by: Maël Valais <mael@vls.dev>

Remove the google-cas-issuer test

9fe9a1b

Signed-off-by: Maël Valais <mael@vls.dev>

maelvls force-pushed the add-smoke-tests branch from 17bd9c3 to 6be10ae Compare February 12, 2021 18:24

maelvls and others added 3 commits February 12, 2021 19:29

Typo in Docker comment

73dc4e1

Signed-off-by: Maël Valais <mael@vls.dev> Co-authored-by: Richard Wall <wallrj@users.noreply.github.com>

Solution name "cert-manager" -> "jetstack-secure-for-cert-manager"

f667cb5

Signed-off-by: Maël Valais <mael@vls.dev> Co-authored-by: Richard Wall <richard.wall@jetstack.io>

jetstack-bot assigned wallrj Feb 16, 2021

wallrj approved these changes Feb 16, 2021

View reviewed changes

jetstack-bot unassigned wallrj Feb 16, 2021

jetstack-bot added the do-not-merge/hold label Feb 16, 2021

jetstack-bot assigned maelvls Feb 16, 2021

jetstack-bot added the lgtm label Feb 16, 2021

maelvls and others added 5 commits February 16, 2021 16:14

Use "1.1.0-gcm.1" instead of "1.1.0+gcm.1"

c5df070

This is due to a Docker limitation. Signed-off-by: Maël Valais <mael@vls.dev>

Use the deployer's serviceaccount for the smoke tests

cb7bd87

Signed-off-by: Maël Valais <mael@vls.dev>

Preflight: missing EOF newline

1883d82

Signed-off-by: Maël Valais <mael@vls.dev> Co-authored-by: Richard Wall <richard.wall@jetstack.io>

Smoke test: use cert-manager v1 instead of v1alpha2

44c55ad

Signed-off-by: Maël Valais <mael@vls.dev> Co-authored-by: Richard Wall <richard.wall@jetstack.io>

Add a link to the TODO about the missing smoke test

a30bbe9

Signed-off-by: Maël Valais <mael@vls.dev> Co-authored-by: Richard Wall <richard.wall@jetstack.io>

jetstack-bot removed the lgtm label Feb 16, 2021

jetstack-bot unassigned maelvls Feb 16, 2021

jetstack-bot removed the do-not-merge/hold label Feb 16, 2021

jetstack-bot assigned wallrj Feb 16, 2021

wallrj approved these changes Feb 16, 2021

View reviewed changes

jetstack-bot unassigned wallrj Feb 16, 2021

jetstack-bot added do-not-merge/hold lgtm labels Feb 16, 2021

jetstack-bot assigned maelvls Feb 16, 2021

PR comment: --for=condition expects a Uppercase condition

95b450e

Signed-off-by: Maël Valais <mael@vls.dev> Co-authored-by: Richard Wall <wallrj@users.noreply.github.com>

jetstack-bot removed the lgtm label Feb 16, 2021

jetstack-bot removed the do-not-merge/hold label Feb 16, 2021

wallrj approved these changes Feb 16, 2021

View reviewed changes

jetstack-bot assigned wallrj Feb 16, 2021

jetstack-bot added the lgtm label Feb 16, 2021

jetstack-bot merged commit 4778e7e into main Feb 16, 2021

maelvls deleted the add-smoke-tests branch February 16, 2021 18:13

	kubectl wait -n ${NAMESPACE} --for=condition=ready certificate selfsigned-cert
	kubectl wait -n ${NAMESPACE} --for=condition=Ready certificate selfsigned-cert

Add smoke tests #5

Add smoke tests #5

Uh oh!

Conversation

maelvls commented Feb 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maelvls commented Feb 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maelvls commented Feb 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maelvls commented Feb 9, 2021

Uh oh!

maelvls commented Feb 9, 2021

Uh oh!

maelvls commented Feb 11, 2021

Uh oh!

wallrj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maelvls commented Feb 11, 2021

Uh oh!

wallrj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maelvls Feb 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maelvls commented Feb 16, 2021

Uh oh!

wallrj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maelvls Feb 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maelvls commented Feb 16, 2021

Uh oh!

wallrj left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

maelvls commented Feb 5, 2021 •

edited

Loading

maelvls commented Feb 5, 2021 •

edited

Loading

maelvls commented Feb 8, 2021 •

edited

Loading

maelvls Feb 16, 2021 •

edited

Loading

maelvls Feb 16, 2021 •

edited

Loading