sse-secure-systems / connaisseur

An admission controller that integrates Container Image Signature Verification into a Kubernetes cluster
https://sse-secure-systems.github.io/connaisseur/
Apache License 2.0
441 stars 62 forks source link

Passing in kubernetes.deployment.envs to connaisseur chart values for chart version 2.4 upwards breaks helm deployment #1734

Closed edison-vflow closed 2 months ago

edison-vflow commented 2 months ago

Describe the bug

From Connaisseur Helm chart version 2.4 onwards it seems when you have custom environment variables kubernetes.deployment.envs eg https://sse-secure-systems.github.io/connaisseur/latest/validators/sigstore_cosign/#kms-support where you need to support KMS and inject AWS environment variables

kubernetes:
  deployment:
    resources:
      limits:
        cpu: 1000m
        memory: 512Mi
      requests:
        cpu: 100m
        memory: 512Mi
    annotations:
      cluster-autoscaler.kubernetes.io/safe-to-evict: "true"
    envs:
      AWS_DEFAULT_REGION: my-region
      AWS_ACCESS_KEY_ID: my-access-key
      AWS_SECRET_ACCESS_KEY: my-secret-key

  service: {}

Once you have an envs fragment, the helm deployment fails

│ Error: unable to build kubernetes objects from release manifest: error validating "": error validating data: [ValidationError(Deployment.spec.template.spec.containers[0].env[1]): unknown field "secretRef" in io.k8s.api.core.v1.EnvVar, ValidationError(Deployment.spec.template.spec.containers[0].env[1]): missing required field "name" in io.k8s.api.core.v1.EnvVar]

Expected behavior

In version 2.3.4 and downwards, having this envs fragment works without failing

Comparing the default values of chart 2.3.4 and downwards and 2.4 upwards, there was no change to the yaml structure implying the same kubernetes.deployment.envs can be passed.

Optional: To reproduce

Deploy the helm chart using terraform ``` resource "helm_release" "connaisseur" { name = "connaisseur" namespace = kubernetes_namespace.connaisseur.metadata[0].name create_namespace = true atomic = false repository = "https://sse-secure-systems.github.io/connaisseur/charts" chart = "connaisseur" version = 2.6.0 values = [ file("${abspath(path.module)}/values.yaml") ] } ``` * use the default values file `https://artifacthub.io/packages/helm/connaisseur/connaisseur/2.6.0?modal=values` Copy these values to create a values.yaml * First run, notice that you will get a successful deployment * Add a `kubernetes.deployment.envs` fragment like in this example https://sse-secure-systems.github.io/connaisseur/latest/validators/sigstore_cosign/#kms-support ``` envs: VAULT_ADDR: myvault.com VAULT_TOKEN: secrettoken ``` * If we uninstall the first deployment and re-install with the new changes, thats when we get the error. * Its as if introducing the envs fragment breaks the logic that creates the secret that contains these custom env values and the ability to use this secret in the deployment **Optional: Versions (please complete the following information as relevant):** - OS: aws-linux images - Kubernetes Cluster: 1.30 - Notary Server: - Container registry: ECR - Connaisseur: 2.4 , 2.5 and 2.6 are failing - Other: **Optional: Additional context**
edison-vflow commented 2 months ago

Further investigation has been carried out. Since Connaisseur chart version 2.4, a refactor was done that places connaisseur-env-secret under env instead of under envFrom in the deployment In values.yaml when kubernetes.deployment.envs section is populated, this results in an invalid kubernetes manifest

      envFrom:
          - configMapRef:
              name: connaisseur-env
          - secretRef:
              name: connaisseur-redis-secret
          env:
          - name: REDIS_HOST
            value: connaisseur-redis-service
          - secretRef:
              name: connaisseur-env-secret

As shown above, the secretRef is under env but any list item that goes under env should start with name

A fix is suggested in PR https://github.com/sse-secure-systems/connaisseur/pull/1735

This fix is equivalent to reverting the Chart logic that was there pre 2.4 but with the usage of the functions eg

{{ include "connaisseur.name" . }}
xavidop commented 2 months ago

same issue here, can we push a quick fix @phbelitz ?

phbelitz commented 2 months ago

@xavidop @edison-vflow I merged the changes and released a new version. Checkout v3.6.1 🎉