Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Helm deployment check to only retrieve deployed YAML #5723

Merged
merged 2 commits into from
Apr 27, 2021

Conversation

briandealwis
Copy link
Member

Fixes: #5484

Description
The Helm deployer calls helm get all <chart> to get the release record for the deployed chart and then parses these release record to get the Kubernetes YAML.

Example Helm release record (`helm get all skaffold-helm-default`)
$ helm get all skaffold-helm-default
NAME: skaffold-helm-default
LAST DEPLOYED: Sat Apr 24 00:22:07 2021
NAMESPACE: default
STATUS: deployed
REVISION: 1
TEST SUITE: None
USER-SUPPLIED VALUES:
image: gcr.io/k8s-skaffold/skaffold-helm:0a46319f93bc0edf6bea77fa3e9ab287979d641939645abe1f45d9503550e93b

COMPUTED VALUES:
image: gcr.io/k8s-skaffold/skaffold-helm:0a46319f93bc0edf6bea77fa3e9ab287979d641939645abe1f45d9503550e93b
ingress:
  annotations: null
  enabled: true
  hosts: null
  tls: null
replicaCount: 1
resources: {}
service:
  externalPort: 80
  internalPort: 80
  name: nginx
  type: ClusterIP

HOOKS:
MANIFEST:
---
# Source: skaffold-helm/templates/service.yaml
apiVersion: v1
kind: Service
metadata:
  name: skaffold-helm-default-skaffold-helm
  labels:
    app: skaffold-helm
    chart: skaffold-helm-0.1.0
    release: skaffold-helm-default
    heritage: Helm
spec:
  type: ClusterIP
  ports:
    - port: 80
      targetPort: 80
      protocol: TCP
      name: nginx
  selector:
    app: skaffold-helm
    release: skaffold-helm-default
---
# Source: skaffold-helm/templates/deployment.yaml
apiVersion: apps/v1
kind: Deployment
metadata:
  name: skaffold-helm-default
  labels:
    app: skaffold-helm
    chart: skaffold-helm-0.1.0
    release: skaffold-helm-default
    heritage: Helm
spec:
  replicas: 1
  selector:
    matchLabels:
      app: skaffold-helm
      release: skaffold-helm-default
  template:
    metadata:
      labels:
        app: skaffold-helm
        release: skaffold-helm-default
    spec:
      containers:
        - name: skaffold-helm
          image: gcr.io/k8s-skaffold/skaffold-helm:0a46319f93bc0edf6bea77fa3e9ab287979d641939645abe1f45d9503550e93b
          imagePullPolicy: 
          ports:
            - containerPort: 80
          resources:
            {}
---
# Source: skaffold-helm/templates/ingress.yaml
apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: skaffold-helm-default-skaffold-helm
  labels:
    app: skaffold-helm
    chart: skaffold-helm-0.1.0
    release: skaffold-helm-default
    heritage: Helm
  annotations:
spec:
  rules:
    - http:
        paths:
          - path: /
            backend:
              serviceName: skaffold-helm-default-skaffold-helm
              servicePort: 80

We try parsing YAML documents from this record, but this is a semi-structured format at best. But since the Kubernetes YAML is the last item reported, and is always preceded with an YAML end-doc (---), our parsing usually works without incident.

But a NOTES.txt changes the output such that the NOTES.txt content comes after the Kubernetes YAML.

              serviceName: skaffold-helm-default-skaffold-helm
              servicePort: 80

NOTES:
Hello Helm 

Since Helm does not add an end-doc ---, the last YAML doc fails to parse and we emit an error. We use this release record primarily to extract the deployed namespaces from the Kubernetes YAML; since most projects deploy to a single namespace or have multiple objects across namespaces, this error is usually of no consequence.

This PR changes the code to request only the deployed Kubernetes YAML using helm get all <chart> --template '{{.Release.Manifest}}' which is supported back to Helm 3.0. From the tests, this seems to have been the expected output 🤷

@briandealwis briandealwis requested a review from nkubala April 26, 2021 14:29
@briandealwis briandealwis requested a review from a team as a code owner April 26, 2021 14:29
@google-cla google-cla bot added the cla: yes label Apr 26, 2021
@codecov
Copy link

codecov bot commented Apr 26, 2021

Codecov Report

Merging #5723 (439a1be) into master (5e2d86a) will decrease coverage by 0.00%.
The diff coverage is 75.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5723      +/-   ##
==========================================
- Coverage   70.82%   70.81%   -0.01%     
==========================================
  Files         421      421              
  Lines       16043    16045       +2     
==========================================
  Hits        11363    11363              
- Misses       3846     3847       +1     
- Partials      834      835       +1     
Impacted Files Coverage Δ
pkg/skaffold/deploy/helm/deploy.go 76.27% <75.00%> (+0.22%) ⬆️
pkg/skaffold/util/tar.go 50.66% <0.00%> (-5.34%) ⬇️
pkg/skaffold/docker/parse.go 87.14% <0.00%> (+0.95%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5e2d86a...439a1be. Read the comment docs.

@tejal29
Copy link
Contributor

tejal29 commented Apr 26, 2021

Thanks Manually verified by adding templates/NOTES.txt as mentioned here https://helm.sh/docs/chart_template_guide/notes_files/

Example Last Helm release record (`helm get all skaffold-helm-default`)
# Source: skaffold-helm/templates/deployment.yaml
apiVersion: apps/v1
kind: Deployment
metadata:
  name: skaffold-helm
  labels:
    app: skaffold-helm
    chart: skaffold-helm-0.1.0
    release: skaffold-helm
    heritage: Helm
spec:
  selector:
    matchLabels:
      app: skaffold-helm
      release: skaffold-helm
  replicas: 1
  template:
    metadata:
      labels:
        app: skaffold-helm
        release: skaffold-helm
    spec:
      volumes:
        - name: static-assets
          configMap:
            name: skaffold-helm
            defaultMode: 420
      containers:
        - name: skaffold-helm
          image: gcr.io/tejal-gke1/skaffold-helm:latest@sha256:9dc6c65944f0970ffd6ffc3c80e4c4d5e01e683e8acf75a2b4ee0350fb62fc76
          imagePullPolicy: 
          ports:
            - containerPort: 80
          volumeMounts:
            - mountPath: /usr/share/nginx/html/
              name: static-assets
          resources:
            {}

NOTES:
Thank you for installing skaffold-helm.

Your release is named skaffold-helm.

vs

Example Last Helm release record (`helm get all skaffold-helm-default --template '{{.Release.Manifest}}'`)
# Source: skaffold-helm/templates/deployment.yaml
apiVersion: apps/v1
kind: Deployment
metadata:
  name: skaffold-helm
  labels:
    app: skaffold-helm
    chart: skaffold-helm-0.1.0
    release: skaffold-helm
    heritage: Helm
spec:
  selector:
    matchLabels:
      app: skaffold-helm
      release: skaffold-helm
  replicas: 1
  template:
    metadata:
      labels:
        app: skaffold-helm
        release: skaffold-helm
    spec:
      volumes:
        - name: static-assets
          configMap:
            name: skaffold-helm
            defaultMode: 420
      containers:
        - name: skaffold-helm
          image: gcr.io/tejal-gke1/skaffold-helm:latest@sha256:9dc6c65944f0970ffd6ffc3c80e4c4d5e01e683e8acf75a2b4ee0350fb62fc76
          imagePullPolicy: 
          ports:
            - containerPort: 80
          volumeMounts:
            - mountPath: /usr/share/nginx/html/
              name: static-assets
          resources:
            {}

@briandealwis briandealwis added the kokoro:force-run forces a kokoro re-run on a PR label Apr 27, 2021
@kokoro-team kokoro-team removed the kokoro:force-run forces a kokoro re-run on a PR label Apr 27, 2021
@briandealwis briandealwis merged commit e119e27 into GoogleContainerTools:master Apr 27, 2021
@briandealwis briandealwis deleted the i5484 branch April 27, 2021 04:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants