Write chart data on identitical values overwrite

This likely happened because the byte buffer response was already
being read by the chart loader, making it empty by the time the
artifact was written to storage.

As an alternative, and because it makes the code a tiny bit less
obnoxious: write the data to a temp file first, and later decide
what file to copy over and use as an stored artifact.

Signed-off-by: Hidde Beydals <[email protected]>

Author: hiddeco

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