Render and Apply - different behaviour with Helm namespaces

[davidjsanders]

Expected behavior

Supplying the following skaffold.yaml to Cloud Deploy, the render and apply stages would create the namespace(s) and include in the rendered manifests (as per https://skaffold.dev/docs/references/yaml/):

apiVersion: skaffold/v2beta22
kind: Config
profiles:
...
- name: dev
  deploy:
    helm:
      releases:
...
        - chartPath: ./d2chart
          name: d2chart
          imageStrategy:
            helm: {}
          artifactOverrides:
            image: nginx2
          namespace: "ns-d2chart-dev"
          createNamespace: true
          valuesFiles:
            - values-files/dev-values.yaml
            - values-files/common-values.yaml
...

Actual behavior

The namespace and create namespace fields are ignored during render and apply. Using skaffold deploy ... works (because it's using Helm to deploy; however, with render and apply this is ignored. Could the rendering stage be updated to include the namespace flag (e.g. helm template -f values-files/common-values.yaml -f values-files/dev-values.yaml test ./demochart --namespace foobar --set image.repository=nginx --set image.tag=1.21) which would allow the namespace to be updated in the manifest during render?

Information

• Skaffold version: 1.35.2
• Operating system: Debian 10 and Cloud Deploy
• Installed via: skaffold.dev and version used in Cloud Deploy
• Contents of skaffold.yaml:

# Copyright 2022 Google LLC. 
# This software is provided as-is, without warranty or representation 
# for any use or purpose. Your use of it is subject to your agreements 
# with Google.
apiVersion: skaffold/v2beta22
kind: Config
profiles:
- name: dev
  deploy:
    helm:
      releases:
        - chartPath: ./demochart
          name: demochart
          imageStrategy:
            helm: {}
          artifactOverrides:
            image: nginx
          valuesFiles:
            - values-files/dev-values.yaml
            - values-files/common-values.yaml
        - chartPath: ./d2chart
          name: d2chart
          imageStrategy:
            helm: {}
          artifactOverrides:
            image: nginx2
          namespace: "ns-d2chart-dev"
          createNamespace: true
          valuesFiles:
            - values-files/dev-values.yaml
            - values-files/common-values.yaml
- name: test
  deploy:
    helm:
      releases:
        - chartPath: ./demochart
          name: demochart
          imageStrategy:
            helm: {}
          artifactOverrides:
            image: nginx
          valuesFiles:
            - values-files/test-values.yaml
            - values-files/common-values.yaml
        - chartPath: ./d2chart
          name: d2chart
          imageStrategy:
            helm: {}
          artifactOverrides:
            image: nginx2
          namespace: "ns-d2chart-test"
          createNamespace: true
          valuesFiles:
            - values-files/test-values.yaml
            - values-files/common-values.yaml
- name: prod
  deploy:
    helm:
      releases:
        - chartPath: ./demochart
          name: demochart
          imageStrategy:
            helm: {}
          artifactOverrides:
            image: nginx
          valuesFiles:
            - values-files/prod-values.yaml
            - values-files/common-values.yaml
        - chartPath: ./d2chart
          name: d2chart
          imageStrategy:
            helm: {}
          artifactOverrides:
            image: nginx2
          namespace: "ns-d2chart-prod"
          createNamespace: true
          valuesFiles:
            - values-files/prod-values.yaml
            - values-files/common-values.yaml

You can work around this issue by baking the namespace into the manifest(s)...

apiVersion: apps/v1
kind: Deployment
metadata:
  name: {{ include "demochart.fullname" . }}
  labels:
    {{- include "demochart.labels" . | nindent 4 }}
  namespace: {{ .Values.namespace }}
...

However, that a) adds fragility by baking in the namespace and b) requires the namespace exists before the apply stage is run.

Steps to reproduce the behavior

1. Create two default Helm charts
2. skaffold render -p dev -a build.artifacts > manifest.yaml
3. skaffold apply --filename=skaffold.yaml manifest.yaml

[briandealwis]

This seems to be a mismatch between skaffold deploy and skaffold render && skaffold apply. helm template does support --namespace so this seems an oversight.

[aaron-prindle]

I am still investigating the root cause of this issue but currently the --namespace flag is passed to the helm template call IIUC. The code that adds --namespace to the helm template args:
https://github.com/GoogleContainerTools/skaffold/blob/main/pkg/skaffold/deploy/helm/deploy.go#L428-L434

Logs in my own repro attempt show that --namespace is populated in the helm template call:

$ skaffold render -p dev -a build.artifacts -v debug
...
DEBU[0002] Running command: [helm --kube-context gke_aprindle-test-cluster_us-central1-c_cluster-1 template skaffold-helm charts --set-string image=gcr.io/aprindle-test-cluster/skaffold-helm:latest@sha256:617aa9d694b12dd1e08ae600c8e9c38f297003ae54343d76ba279e3473be168f --namespace skaffold-helm-dev]  subtask=-1 task=DevLoop

^^^^^ helm template ... --namespace skaffold-helm-dev

Should helm be adding a namespace in the generated manifest? When I run helm template with --namespace standalone the manifest has no namespace: field:

$ helm --kube-context gke_aprindle-test-cluster_us-central1-c_cluster-1 template skaffold-helm charts --set-string image=gcr.io/aprindle-test-cluster/skaffold-helm:latest@sha256:617aa9d694b12dd1e08ae600c8e9c38f297003ae54343d76ba279e3473be168f --namespace skaffold-helm-dev

---
# Source: skaffold-helm/templates/deployment.yaml
apiVersion: apps/v1
kind: Deployment
metadata:
  name: skaffold-helm
  labels:
    app: skaffold-helm
spec:
  selector:
    matchLabels:
      app: skaffold-helm
  replicas: 2
  template:
    metadata:
      labels:
        app: skaffold-helm
    spec:
      containers:
      - name: skaffold-helm
        image: gcr.io/aprindle-test-cluster/skaffold-helm:latest@sha256:617aa9d694b12dd1e08ae600c8e9c38f297003ae54343d76ba279e3473be168f

[aaron-prindle]

Here is my repro steps:

skaffold.yaml

apiVersion: skaffold/v2beta27
kind: Config
build:
  artifacts:
  - image: skaffold-helm
profiles:
- name: dev
  deploy:
    helm:
      releases:
      - name: skaffold-helm
        chartPath: charts
        artifactOverrides:
          image: skaffold-helm
        namespace: "skaffold-helm-dev"
        createNamespace: true
- name: prod
  deploy:
    helm:
      releases:
      - name: skaffold-helm
        chartPath: charts
        artifactOverrides:
          image: skaffold-helm
        namespace: "skaffold-helm-prod"
        createNamespace: true

commands:

$ skaffold build --file-output build.artifacts --cache-artifacts=false --default-repo=gcr.io/aprindle-test-cluster
$ skaffold render -p dev -a build.artifacts > manifest.yaml
$ skaffold apply --filename=skaffold.yaml manifest.yaml

I am able to reproduce this issue, the deployment (in this case skaffold-helm) is deployed to the default namespace, not the skaffold-helm-dev namespace as expected. I am a bit confused though how this should work I am not able to actually get helm template to generate a manifest with a namespace, even when setting --namespace and --createNamespace:

$ helm --kube-context gke_aprindle-test-cluster_us-central1-c_cluster-1 template skaffold-helm charts --set-string image=gcr.io/aprindle-test-cluster/skaffold-helm:latest@sha256:617aa9d694b12dd1e08ae600c8e9c38f297003ae54343d76ba279e3473be168f --namespace skaffold-helm-dev --create-namespace
---
# Source: skaffold-helm/templates/deployment.yaml
apiVersion: apps/v1
kind: Deployment
metadata:
  name: skaffold-helm
  labels:
    app: skaffold-helm
spec:
  selector:
    matchLabels:
      app: skaffold-helm
  replicas: 2
  template:
    metadata:
      labels:
        app: skaffold-helm
    spec:
      containers:
      - name: skaffold-helm
        image: gcr.io/aprindle-test-cluster/skaffold-helm:latest@sha256:617aa9d694b12dd1e08ae600c8e9c38f297003ae54343d76ba279e3473be168f

[aaron-prindle]

Related helm issue:
helm/helm#3553

I'm not sure if helm actually adds namespaces to the generated manifest, still trying to have it work for me locally via helm. Trying to see if updating helm to latest supports this, there was one comment stating it might be supported in that thread - helm/helm#3553 (comment)

[msonnleitner]

I don't think that Helm/Skaffold should add namespaces to the manifest, but skaffold apply should use the namespace given in the deploy section. It is possible to explicitly specify something like skaffold apply manifest.yaml --namespace=skaffold-helm-dev, but the default should probably be the one given in that config

[aaron-prindle]

@msonnleitner I agree with the proposed solution of making skaffold apply honor deploy: related namespace configuration (the helm namespace configuration in this specific issue).

Currently skaffold apply only looks at namespace information from manifests directly and does not incorporate any namespace/deploy-configuration information from the skaffold.yaml file:
https://github.com/GoogleContainerTools/skaffold/blob/main/pkg/skaffold/deploy/kubectl/kubectl.go#L184-L192
https://github.com/GoogleContainerTools/skaffold/blob/main/pkg/skaffold/deploy/kubectl/kubectl.go#L224-L230

Additionally when skaffold apply is called, under the hood it uses the kubectl deployer (regardless of the deploy type - [kubectl|helm|kpt|kustomize|etc.]) and essentially does kubectl apply -f manifest.yaml
https://github.com/GoogleContainerTools/skaffold/blob/main/pkg/skaffold/deploy/kubectl/kubectl.go#L239-L243

For helm deployments, when using skaffold deploy helm install is actually used with the params in the helm: field of the skaffold.yaml passed in as args to helm install as expected. When using skaffold apply as noted, kubectl apply is used for the entire manifest.yaml file so there is not the same CLI arguments to map to and the specific yamls that the helm: deployment (+ namespace) is tied to is lost.

Currently the way that skaffold apply handles deploying things into different namespaces is via the namespace set in the manifest and a global namespace that is initialized via a config-option/flag that in skaffold’s source sets every kubectl CLI command to use that namespace:
https://github.com/GoogleContainerTools/skaffold/blob/main/pkg/skaffold/kubectl/cli.go#L47

From the comments here it seems that the suggested fix is that skaffold apply should check the associated skaffold.yaml file, if there is a deploy.helm.namespace value set then the result should be identical to skaffold apply -n “deploy.helm.namespace-value”.

While this can be done, the issue is that for skaffold configuration with multiple deployments in a ‘deploy:’ the namespaces would be different for skaffold deploy vs skaffold render + skaffold apply (the same issue now just for the other deployments). An example can be seen in the skaffold.yaml here:

apiVersion: skaffold/v2beta27
kind: Config
build:
  artifacts:
  - image: skaffold-helm
profiles:
- name: dev
  deploy:
    helm:
      releases:
      - name: skaffold-helm
        chartPath: charts
        artifactOverrides:
          image: skaffold-helm
        namespace: "skaffold-helm-dev"
        createNamespace: true
    kubectl:
      manifests:
        - k8s-*
…

With the suggested fix, the kubectl: deployment will now go to namespace skaffold-helm-dev when it would be expected to go to default and goes to default with skaffold deploy

From my reading of this these are the possible solutions:

• Do the suggested fix that might error for skaffold.yaml files with multiple deploys in a deploy stanza (eg helm + any-deploy-type) and just assume most users using helm: have only a single deploy - deploy.helm specified.

• Do a more complicated fix that involves

  • Storing/recomputing the skaffold.yaml -> manifest.yaml mappings s.t. skaffold knows which deployments belong to which namespace
  • This could be done either by
    • recomputing what yaml objects a skaffold render would generate and then putting those in the namespace specified in the skaffold.yaml
    • Embedding skaffold.yaml information into the manifests generated by skaffold render eg: adding a label like skaffold-render-info: “{namespace: foo}”

@briandealwis do you agree with the following assessment and have any opinion on the appropriate solution going forward? Thanks

[aaron-prindle]

For a simple fix here, the plan is update skaffold apply to do the following:

• When using skaffold apply for skaffold.yaml(s) that only have one deploy entry, that entry is helm and there is one release (criteria met by the sample yaml in this issue) - skaffold will automatically set the namespace flag to be the value of deploy.helm.releases.0.namespace - if the namespace value is set.
• When using skaffold apply for skaffold.yaml(s) that do not meet the above criteria and there is a deploy.helm.releases..namespace value present, skaffold will error stating something to the effect - "skaffold found deploy.helm.releases..namespace while attempting skaffold apply. skaffold apply does not support the helm namespace field. Please remove the defined namespace value in your helm deployment configuration and try again"

[aaron-prindle]

NOTE: In the simple fix suggested above, the profile would have to be passed to skaffold apply as well to work as expected for the namespace:
skaffold apply -p dev --filename=skaffold.yaml manifest.yaml