feat: change to load custom certs through a secret rather than configmap

[STollenaar]

• Documentation written ℹ︎
• Commit history is tidy ℹ︎

What this does

Currently if you need to install a custom CA cert for the monitor to work, it is installed through a ConfigMap. The standard for handling certs in Kubernetes is by putting these in a Secret. By instead putting it in a Secret you can also rely on other automated sources that handle these certificates (think of cert-manager, external-secrets).

Notes for the reviewer

Create the Secret for the certs instead of a configmap. Then install the helmchart following steps and set the certsSecret to the name of the Secret

[bgardiner]

@STollenaar Thanks for the PR!

Since the ssl-certs config map is defined outside of the chart, I think the current version of this PR is backwards incompatible with prior installs (i.e., it will break customers that have already installed the chart previously and are just running helm upgrade). How would you feel about migrating the ssl-certs to a projected volume that mounts both the config map and the new secret (both optional). With this refactor, I think whichever resource a user has created (old config map or new secret) will be mounted in.

[STollenaar]

@STollenaar Thanks for the PR!

Since the ssl-certs config map is defined outside of the chart, I think the current version of this PR is backwards incompatible with prior installs (i.e., it will break customers that have already installed the chart previously and are just running helm upgrade). How would you feel about migrating the ssl-certs to a projected volume that mounts both the config map and the new secret (both optional). With this refactor, I think whichever resource a user has created (old config map or new secret) will be mounted in.

that's good suggestion. I changed it to use the projected-volume instead and re-added the configmap value to the values.yaml. With the comment that it's deprecated.

[CLAassistant]

All committers have signed the CLA.

[parker-snyk]

Thanks for the PR, and welcome! Moving custom certs into a Secret is a good move — it works much more nicely with cert-manager and external-secrets.

A few things to fix before it's good to go:

snyk-monitor/README.md

The kubectl create secret tls command here won't work with the way the monitor loads certs. A tls Secret always has exactly two keys, tls.crt and tls.key. The monitor mounts that volume at /srv/app/certs and passes it to skopeo via --cert-dir. skopeo treats *.crt as CA certs, but it also expects every *.key to sit next to a matching *.cert. There's no tls.cert, so it fails with missing certificate tls.cert for key tls.key and the whole cert directory fails to load — which is exactly the private-registry case this feature is for.

A generic Secret built from the certs folder avoids that and keeps the original behavior, where any *.crt, *.cert, or *.key file is keyed by its filename:

kubectl create secret generic snyk-monitor-certs -n snyk-monitor --from-file=<path_to_certs_folder>

One more thing on that same line: small typo, (_.crt,_.cert, *.key only) should read (*.crt, *.cert, *.key only).

snyk-monitor-deployment.yaml

This is the plain manifest we publish and test against, for folks who install with kubectl apply rather than Helm. Its ssl-certs volume is still a plain configMap, so a Secret won't get picked up here. It needs the same projected configMap+secret volume you added to the Helm template:

      - name: ssl-certs
        projected:
          sources:
          - configMap:
              name: snyk-monitor-certs
              optional: true
          - secret:
              name: snyk-monitor-certs
              optional: true

snyk-monitor/templates/deployment.yaml

Nit: in the volume block you added, {{.Values.certsConfigMap}} is missing the spaces the rest of the file uses. {{ .Values.certsConfigMap }} keeps it consistent.

The rest looks good — values.yaml and the projected-volume approach in the template are exactly right. Happy to talk through any of this.

[parker-snyk]

LGTM. Thank you for your contribution to Snyk and open source, @STollenaar !

[snyk-pr-review-bot]

PR Reviewer Guide 🔍

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Deployment Hazard 🟠 [major] The default values for certsSecret and certsConfigMap are both set to snyk-monitor-certs. In templates/deployment.yaml, these are used as sources for a single projected volume. If a user creates the new Secret with the same name as the existing ConfigMap (as suggested in README.md), and both contain the same filenames (e.g., ca.pem), the Kubelet will fail to mount the volume. The Pod will be stuck in ContainerCreating or CrashLoopBackOff because projected volumes do not allow multiple sources to provide the same path. Setting one of these defaults to an empty string or using a logic gate in the template to prioritize one over the other would be safer. certsSecret: snyk-monitor-certs # Deprecated, use the certsSecret instead certsConfigMap: snyk-monitor-certs

📚 Repository Context Analyzed This review considered 23 relevant code sections from 15 files (average relevance: 0.68) snyk-monitor/values.yaml (lines 1-210) snyk-monitor/Chart.yaml (lines 1-6) snyk-monitor-deployment.yaml (lines 1-142) snyk-monitor-cluster-permissions.yaml (lines 1-102) snyk-monitor-namespaced-permissions.yaml (lines 1-94) snyk-monitor/templates/serviceaccount.yaml (lines 1-17) scripts/circleci-jobs/prepare_to_deploy.sh (lines 1-51) ... and 8 more file(s)