Merge pull request #345 from fluxcd/empty-chart-fix

Write chart data on identitical values overwrite

Author: stefanprodan

controllers/helmchart_controller.go

@@ -19,6 +19,7 @@ package controllers
 import (
 	"context"
 	"fmt"
+	"io"
 	"io/ioutil"
 	"net/url"
 	"os"
@@ -374,23 +375,30 @@ func (r *HelmChartReconciler) reconcileFromHelmRepository(ctx context.Context,
 	if err != nil {
 		return sourcev1.HelmChartNotReady(chart, sourcev1.ChartPullFailedReason, err.Error()), err
 	}
+	tmpFile, err := ioutil.TempFile("", fmt.Sprintf("%s-%s-", chart.Namespace, chart.Name))
+	if err != nil {
+		return sourcev1.HelmChartNotReady(chart, sourcev1.ChartPullFailedReason, err.Error()), err
+	}
+	defer os.RemoveAll(tmpFile.Name())
+	if _, err = io.Copy(tmpFile, res); err != nil {
+		tmpFile.Close()
+		return sourcev1.HelmChartNotReady(chart, sourcev1.ChartPullFailedReason, err.Error()), err
+	}
+	tmpFile.Close()
 
-	// Either repackage the chart with the declared default values file,
-	// or write the chart directly to storage.
+	// Check if we need to repackage the chart with the declared defaults files.
 	var (
+		pkgPath      = tmpFile.Name()
 		readyReason  = sourcev1.ChartPullSucceededReason
 		readyMessage = fmt.Sprintf("Fetched revision: %s", newArtifact.Revision)
 	)
+
 	switch {
 	case len(chart.GetValuesFiles()) > 0:
-		var (
-			tmpDir  string
-			pkgPath string
-		)
 		valuesMap := make(map[string]interface{})
 
 		// Load the chart
-		helmChart, err := loader.LoadArchive(res)
+		helmChart, err := loader.LoadFile(pkgPath)
 		if err != nil {
 			err = fmt.Errorf("load chart error: %w", err)
 			return sourcev1.HelmChartNotReady(chart, sourcev1.StorageOperationFailedReason, err.Error()), err
@@ -435,12 +443,11 @@ func (r *HelmChartReconciler) reconcileFromHelmRepository(ctx context.Context,
 		if changed, err := helm.OverwriteChartDefaultValues(helmChart, yamlBytes); err != nil {
 			return sourcev1.HelmChartNotReady(chart, sourcev1.ChartPackageFailedReason, err.Error()), err
 		} else if !changed {
-			// No changes, skip to write original package to storage
-			goto skipToDefault
+			break
 		}
 
 		// Create temporary working directory
-		tmpDir, err = ioutil.TempDir("", fmt.Sprintf("%s-%s-", chart.Namespace, chart.Name))
+		tmpDir, err := ioutil.TempDir("", fmt.Sprintf("%s-%s-", chart.Namespace, chart.Name))
 		if err != nil {
 			err = fmt.Errorf("tmp dir error: %w", err)
 			return sourcev1.HelmChartNotReady(chart, sourcev1.StorageOperationFailedReason, err.Error()), err
@@ -462,15 +469,12 @@ func (r *HelmChartReconciler) reconcileFromHelmRepository(ctx context.Context,
 
 		readyMessage = fmt.Sprintf("Fetched and packaged revision: %s", newArtifact.Revision)
 		readyReason = sourcev1.ChartPackageSucceededReason
-		break
-	skipToDefault:
-		fallthrough
-	default:
-		// Write artifact to storage
-		if err := r.Storage.AtomicWriteFile(&newArtifact, res, 0644); err != nil {
-			err = fmt.Errorf("unable to write chart file: %w", err)
-			return sourcev1.HelmChartNotReady(chart, sourcev1.StorageOperationFailedReason, err.Error()), err
-		}
+	}
+
+	// Write artifact to storage
+	if err := r.Storage.CopyFromPath(&newArtifact, pkgPath); err != nil {
+		err = fmt.Errorf("unable to write chart file: %w", err)
+		return sourcev1.HelmChartNotReady(chart, sourcev1.StorageOperationFailedReason, err.Error()), err
 	}
 
 	// Update symlink

controllers/helmchart_controller_test.go

@@ -242,6 +242,27 @@ var _ = Describe("HelmChartReconciler", func() {
 				Expect(helmChart.Values["testOverride"]).To(BeTrue())
 			})
 
+			When("Setting identical valuesFile attribute", func() {
+				updated := &sourcev1.HelmChart{}
+				Expect(k8sClient.Get(context.Background(), key, updated)).To(Succeed())
+				updated.Spec.ValuesFile = "duplicate.yaml"
+				updated.Spec.ValuesFiles = []string{}
+				Expect(k8sClient.Update(context.Background(), updated)).To(Succeed())
+				got := &sourcev1.HelmChart{}
+				Eventually(func() bool {
+					_ = k8sClient.Get(context.Background(), key, got)
+					return got.Status.Artifact.Checksum != updated.Status.Artifact.Checksum &&
+						storage.ArtifactExist(*got.Status.Artifact)
+				}, timeout, interval).Should(BeTrue())
+				f, err := os.Stat(storage.LocalPath(*got.Status.Artifact))
+				Expect(err).NotTo(HaveOccurred())
+				Expect(f.Size()).To(BeNumerically(">", 0))
+				helmChart, err := loader.Load(storage.LocalPath(*got.Status.Artifact))
+				Expect(err).NotTo(HaveOccurred())
+				Expect(helmChart.Values["testDefault"]).To(BeTrue())
+				Expect(helmChart.Values["testOverride"]).To(BeFalse())
+			})
+
 			When("Setting invalid valuesFile attribute", func() {
 				updated := &sourcev1.HelmChart{}
 				Expect(k8sClient.Get(context.Background(), key, updated)).To(Succeed())
@@ -259,9 +280,8 @@ var _ = Describe("HelmChartReconciler", func() {
 				Expect(f.Size()).To(BeNumerically(">", 0))
 				helmChart, err := loader.Load(storage.LocalPath(*got.Status.Artifact))
 				Expect(err).NotTo(HaveOccurred())
-				_, exists := helmChart.Values["testDefault"]
-				Expect(exists).To(BeFalse())
-				Expect(helmChart.Values["testOverride"]).To(BeTrue())
+				Expect(helmChart.Values["testDefault"]).To(BeTrue())
+				Expect(helmChart.Values["testOverride"]).To(BeFalse())
 			})
 
 			By("Expecting missing HelmRepository error")

controllers/testdata/charts/helmchart/duplicate.yaml

@@ -0,0 +1,70 @@
+# Default values for helmchart.
+# This is a YAML-formatted file.
+# Declare variables to be passed into your templates.
+
+replicaCount: 1
+
+image:
+  repository: nginx
+  pullPolicy: IfNotPresent
+
+imagePullSecrets: []
+nameOverride: ""
+fullnameOverride: ""
+
+serviceAccount:
+  # Specifies whether a service account should be created
+  create: true
+  # The name of the service account to use.
+  # If not set and create is true, a name is generated using the fullname template
+  name:
+
+podSecurityContext: {}
+  # fsGroup: 2000
+
+securityContext: {}
+  # capabilities:
+  #   drop:
+  #   - ALL
+  # readOnlyRootFilesystem: true
+  # runAsNonRoot: true
+  # runAsUser: 1000
+
+service:
+  type: ClusterIP
+  port: 80
+
+ingress:
+  enabled: false
+  annotations: {}
+    # kubernetes.io/ingress.class: nginx
+    # kubernetes.io/tls-acme: "true"
+  hosts:
+    - host: chart-example.local
+      paths: []
+  tls: []
+  #  - secretName: chart-example-tls
+  #    hosts:
+  #      - chart-example.local
+
+resources: {}
+  # We usually recommend not to specify default resources and to leave this as a conscious
+  # choice for the user. This also increases chances charts run on environments with little
+  # resources, such as Minikube. If you do want to specify resources, uncomment the following
+  # lines, adjust them as necessary, and remove the curly braces after 'resources:'.
+  # limits:
+  #   cpu: 100m
+  #   memory: 128Mi
+  # requests:
+  #   cpu: 100m
+  #   memory: 128Mi
+
+nodeSelector: {}
+
+tolerations: []
+
+affinity: {}
+
+# Values for tests
+testDefault: true
+testOverride: false