Skip to content

Commit 716e2e8

Browse files
committed
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 <hello@hidde.co>
1 parent 82fdc24 commit 716e2e8

File tree

3 files changed

+114
-19
lines changed

3 files changed

+114
-19
lines changed

controllers/helmchart_controller.go

Lines changed: 23 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ package controllers
1919
import (
2020
"context"
2121
"fmt"
22+
"io"
2223
"io/ioutil"
2324
"net/url"
2425
"os"
@@ -374,23 +375,30 @@ func (r *HelmChartReconciler) reconcileFromHelmRepository(ctx context.Context,
374375
if err != nil {
375376
return sourcev1.HelmChartNotReady(chart, sourcev1.ChartPullFailedReason, err.Error()), err
376377
}
378+
tmpFile, err := ioutil.TempFile("", fmt.Sprintf("%s-%s-", chart.Namespace, chart.Name))
379+
if err != nil {
380+
return sourcev1.HelmChartNotReady(chart, sourcev1.ChartPullFailedReason, err.Error()), err
381+
}
382+
defer os.RemoveAll(tmpFile.Name())
383+
if _, err = io.Copy(tmpFile, res); err != nil {
384+
tmpFile.Close()
385+
return sourcev1.HelmChartNotReady(chart, sourcev1.ChartPullFailedReason, err.Error()), err
386+
}
387+
tmpFile.Close()
377388

378-
// Either repackage the chart with the declared default values file,
379-
// or write the chart directly to storage.
389+
// Check if we need to repackage the chart with the declared defaults files.
380390
var (
391+
pkgPath = tmpFile.Name()
381392
readyReason = sourcev1.ChartPullSucceededReason
382393
readyMessage = fmt.Sprintf("Fetched revision: %s", newArtifact.Revision)
383394
)
395+
384396
switch {
385397
case len(chart.GetValuesFiles()) > 0:
386-
var (
387-
tmpDir string
388-
pkgPath string
389-
)
390398
valuesMap := make(map[string]interface{})
391399

392400
// Load the chart
393-
helmChart, err := loader.LoadArchive(res)
401+
helmChart, err := loader.LoadFile(pkgPath)
394402
if err != nil {
395403
err = fmt.Errorf("load chart error: %w", err)
396404
return sourcev1.HelmChartNotReady(chart, sourcev1.StorageOperationFailedReason, err.Error()), err
@@ -435,12 +443,11 @@ func (r *HelmChartReconciler) reconcileFromHelmRepository(ctx context.Context,
435443
if changed, err := helm.OverwriteChartDefaultValues(helmChart, yamlBytes); err != nil {
436444
return sourcev1.HelmChartNotReady(chart, sourcev1.ChartPackageFailedReason, err.Error()), err
437445
} else if !changed {
438-
// No changes, skip to write original package to storage
439-
goto skipToDefault
446+
break
440447
}
441448

442449
// Create temporary working directory
443-
tmpDir, err = ioutil.TempDir("", fmt.Sprintf("%s-%s-", chart.Namespace, chart.Name))
450+
tmpDir, err := ioutil.TempDir("", fmt.Sprintf("%s-%s-", chart.Namespace, chart.Name))
444451
if err != nil {
445452
err = fmt.Errorf("tmp dir error: %w", err)
446453
return sourcev1.HelmChartNotReady(chart, sourcev1.StorageOperationFailedReason, err.Error()), err
@@ -462,15 +469,12 @@ func (r *HelmChartReconciler) reconcileFromHelmRepository(ctx context.Context,
462469

463470
readyMessage = fmt.Sprintf("Fetched and packaged revision: %s", newArtifact.Revision)
464471
readyReason = sourcev1.ChartPackageSucceededReason
465-
break
466-
skipToDefault:
467-
fallthrough
468-
default:
469-
// Write artifact to storage
470-
if err := r.Storage.AtomicWriteFile(&newArtifact, res, 0644); err != nil {
471-
err = fmt.Errorf("unable to write chart file: %w", err)
472-
return sourcev1.HelmChartNotReady(chart, sourcev1.StorageOperationFailedReason, err.Error()), err
473-
}
472+
}
473+
474+
// Write artifact to storage
475+
if err := r.Storage.CopyFromPath(&newArtifact, pkgPath); err != nil {
476+
err = fmt.Errorf("unable to write chart file: %w", err)
477+
return sourcev1.HelmChartNotReady(chart, sourcev1.StorageOperationFailedReason, err.Error()), err
474478
}
475479

476480
// Update symlink

controllers/helmchart_controller_test.go

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -242,6 +242,27 @@ var _ = Describe("HelmChartReconciler", func() {
242242
Expect(helmChart.Values["testOverride"]).To(BeTrue())
243243
})
244244

245+
When("Setting identical valuesFile attribute", func() {
246+
updated := &sourcev1.HelmChart{}
247+
Expect(k8sClient.Get(context.Background(), key, updated)).To(Succeed())
248+
updated.Spec.ValuesFile = "duplicate.yaml"
249+
updated.Spec.ValuesFiles = []string{}
250+
Expect(k8sClient.Update(context.Background(), updated)).To(Succeed())
251+
got := &sourcev1.HelmChart{}
252+
Eventually(func() bool {
253+
_ = k8sClient.Get(context.Background(), key, got)
254+
return got.Status.Artifact.Checksum != updated.Status.Artifact.Checksum &&
255+
storage.ArtifactExist(*got.Status.Artifact)
256+
}, timeout, interval).Should(BeTrue())
257+
f, err := os.Stat(storage.LocalPath(*got.Status.Artifact))
258+
Expect(err).NotTo(HaveOccurred())
259+
Expect(f.Size()).To(BeNumerically(">", 0))
260+
helmChart, err := loader.Load(storage.LocalPath(*got.Status.Artifact))
261+
Expect(err).NotTo(HaveOccurred())
262+
Expect(helmChart.Values["testDefault"]).To(BeTrue())
263+
Expect(helmChart.Values["testOverride"]).To(BeFalse())
264+
})
265+
245266
When("Setting invalid valuesFile attribute", func() {
246267
updated := &sourcev1.HelmChart{}
247268
Expect(k8sClient.Get(context.Background(), key, updated)).To(Succeed())
Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
# Default values for helmchart.
2+
# This is a YAML-formatted file.
3+
# Declare variables to be passed into your templates.
4+
5+
replicaCount: 1
6+
7+
image:
8+
repository: nginx
9+
pullPolicy: IfNotPresent
10+
11+
imagePullSecrets: []
12+
nameOverride: ""
13+
fullnameOverride: ""
14+
15+
serviceAccount:
16+
# Specifies whether a service account should be created
17+
create: true
18+
# The name of the service account to use.
19+
# If not set and create is true, a name is generated using the fullname template
20+
name:
21+
22+
podSecurityContext: {}
23+
# fsGroup: 2000
24+
25+
securityContext: {}
26+
# capabilities:
27+
# drop:
28+
# - ALL
29+
# readOnlyRootFilesystem: true
30+
# runAsNonRoot: true
31+
# runAsUser: 1000
32+
33+
service:
34+
type: ClusterIP
35+
port: 80
36+
37+
ingress:
38+
enabled: false
39+
annotations: {}
40+
# kubernetes.io/ingress.class: nginx
41+
# kubernetes.io/tls-acme: "true"
42+
hosts:
43+
- host: chart-example.local
44+
paths: []
45+
tls: []
46+
# - secretName: chart-example-tls
47+
# hosts:
48+
# - chart-example.local
49+
50+
resources: {}
51+
# We usually recommend not to specify default resources and to leave this as a conscious
52+
# choice for the user. This also increases chances charts run on environments with little
53+
# resources, such as Minikube. If you do want to specify resources, uncomment the following
54+
# lines, adjust them as necessary, and remove the curly braces after 'resources:'.
55+
# limits:
56+
# cpu: 100m
57+
# memory: 128Mi
58+
# requests:
59+
# cpu: 100m
60+
# memory: 128Mi
61+
62+
nodeSelector: {}
63+
64+
tolerations: []
65+
66+
affinity: {}
67+
68+
# Values for tests
69+
testDefault: true
70+
testOverride: false

0 commit comments

Comments
 (0)