Skip to content

Commit a55c502

Browse files
relurelu
committed
Fix HelmChart valuesFile chart path restriction
As part of the feature implementation to support helm chart dependencies, the functionality for allowing values files overwriting from any location scoped to the same source was altered. This should fix the problem by allowing users to load files from any arbitrary location as long as it's in the context of the same source from where the helm chart itself is loaded. Signed-off-by: Aurel Canciu <[email protected]>
1 parent a5032a0 commit a55c502

File tree

6 files changed

+214
-132
lines changed

6 files changed

+214
-132
lines changed

‎config/testdata/helmchart-valuesfile/helmchart_gitrepository.yaml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,4 +8,4 @@ spec:
88
kind: GitRepository
99
name: podinfo
1010
chart: charts/podinfo
11-
valuesFile: values-prod.yaml
11+
valuesFile: charts/podinfo/values-prod.yaml

‎controllers/helmchart_controller.go‎

Lines changed: 33 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import (
2222
"io/ioutil"
2323
"net/url"
2424
"os"
25+
"path/filepath"
2526
"regexp"
2627
"strings"
2728
"time"
@@ -378,8 +379,18 @@ func (r *HelmChartReconciler) reconcileFromHelmRepository(ctx context.Context,
378379
return sourcev1.HelmChartNotReady(chart, sourcev1.StorageOperationFailedReason, err.Error()), err
379380
}
380381

382+
// Find override file and retrieve contents
383+
var valuesData []byte
384+
cfn := filepath.Clean(chart.Spec.ValuesFile)
385+
for _, f := range helmChart.Files {
386+
if f.Name == cfn {
387+
valuesData = f.Data
388+
break
389+
}
390+
}
391+
381392
// Overwrite values file
382-
if changed, err := helm.OverwriteChartDefaultValues(helmChart, chart.Spec.ValuesFile); err != nil {
393+
if changed, err := helm.OverwriteChartDefaultValues(helmChart, valuesData); err != nil {
383394
return sourcev1.HelmChartNotReady(chart, sourcev1.ChartPackageFailedReason, err.Error()), err
384395
} else if !changed {
385396
// No changes, skip to write original package to storage
@@ -483,9 +494,27 @@ func (r *HelmChartReconciler) reconcileFromTarballArtifact(ctx context.Context,
483494
// or write the chart directly to storage.
484495
pkgPath := chartPath
485496
isValuesFileOverriden := false
486-
if chart.Spec.ValuesFile != "" && chart.Spec.ValuesFile != chartutil.ValuesfileName {
487-
// Overwrite default values if configured
488-
isValuesFileOverriden, err = helm.OverwriteChartDefaultValues(helmChart, chart.Spec.ValuesFile)
497+
if chart.Spec.ValuesFile != "" {
498+
srcPath, err := securejoin.SecureJoin(tmpDir, chart.Spec.ValuesFile)
499+
if err != nil {
500+
return sourcev1.HelmChartNotReady(chart, sourcev1.StorageOperationFailedReason, err.Error()), err
501+
}
502+
if f, err := os.Stat(srcPath); os.IsNotExist(err) || !f.Mode().IsRegular() {
503+
err = fmt.Errorf("invalid values file path: %s", chart.Spec.ValuesFile)
504+
return chart, err
505+
}
506+
src, err := os.Open(srcPath)
507+
if err != nil {
508+
err = fmt.Errorf("failed to open values file '%s': %w", chart.Spec.ValuesFile, err)
509+
return chart, err
510+
}
511+
defer src.Close()
512+
513+
var valuesData []byte
514+
if _, err := src.Read(valuesData); err == nil {
515+
isValuesFileOverriden, err = helm.OverwriteChartDefaultValues(helmChart, valuesData)
516+
}
517+
489518
if err != nil {
490519
return sourcev1.HelmChartNotReady(chart, sourcev1.ChartPackageFailedReason, err.Error()), err
491520
}

‎controllers/helmchart_controller_test.go‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -693,6 +693,31 @@ var _ = Describe("HelmChartReconciler", func() {
693693
return got.Status.Artifact != nil &&
694694
storage.ArtifactExist(*got.Status.Artifact)
695695
}, timeout, interval).Should(BeTrue())
696+
697+
When("Setting valid valuesFile attribute", func() {
698+
updated := &sourcev1.HelmChart{}
699+
Expect(k8sClient.Get(context.Background(), key, updated)).To(Succeed())
700+
chart.Spec.ValuesFile = "./charts/helmchart/override.yaml"
701+
Expect(k8sClient.Update(context.Background(), updated)).To(Succeed())
702+
got := &sourcev1.HelmChart{}
703+
Eventually(func() bool {
704+
_ = k8sClient.Get(context.Background(), key, got)
705+
return got.Status.Artifact != nil &&
706+
storage.ArtifactExist(*got.Status.Artifact)
707+
}, timeout, interval).Should(BeTrue())
708+
})
709+
710+
When("Setting invalid valuesFile attribute", func() {
711+
updated := &sourcev1.HelmChart{}
712+
Expect(k8sClient.Get(context.Background(), key, updated)).To(Succeed())
713+
chart.Spec.ValuesFile = "invalid.yaml"
714+
Expect(k8sClient.Update(context.Background(), updated)).To(Succeed())
715+
got := &sourcev1.HelmChart{}
716+
Eventually(func() bool {
717+
_ = k8sClient.Get(context.Background(), key, got)
718+
return got.Status.Artifact != nil && got.Status.Artifact.Revision == updated.Status.Artifact.Revision
719+
}, timeout, interval).Should(BeTrue())
720+
})
696721
})
697722
})
698723

@@ -936,6 +961,31 @@ var _ = Describe("HelmChartReconciler", func() {
936961
return got.Status.Artifact != nil &&
937962
storage.ArtifactExist(*got.Status.Artifact)
938963
}, timeout, interval).Should(BeTrue())
964+
965+
When("Setting valid valuesFile attribute", func() {
966+
updated := &sourcev1.HelmChart{}
967+
Expect(k8sClient.Get(context.Background(), key, updated)).To(Succeed())
968+
chart.Spec.ValuesFile = "override.yaml"
969+
Expect(k8sClient.Update(context.Background(), updated)).To(Succeed())
970+
got := &sourcev1.HelmChart{}
971+
Eventually(func() bool {
972+
_ = k8sClient.Get(context.Background(), key, got)
973+
return got.Status.Artifact != nil &&
974+
storage.ArtifactExist(*got.Status.Artifact)
975+
}, timeout, interval).Should(BeTrue())
976+
})
977+
978+
When("Setting invalid valuesFile attribute", func() {
979+
updated := &sourcev1.HelmChart{}
980+
Expect(k8sClient.Get(context.Background(), key, updated)).To(Succeed())
981+
chart.Spec.ValuesFile = "./charts/helmchart/override.yaml"
982+
Expect(k8sClient.Update(context.Background(), updated)).To(Succeed())
983+
got := &sourcev1.HelmChart{}
984+
Eventually(func() bool {
985+
_ = k8sClient.Get(context.Background(), key, got)
986+
return got.Status.Artifact != nil && got.Status.Artifact.Revision == updated.Status.Artifact.Revision
987+
}, timeout, interval).Should(BeTrue())
988+
})
939989
})
940990
})
941991
})

‎controllers/testdata/charts/helmchart/override.yaml‎

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
# Override values for helmchart.
2+
# This is a YAML-formatted file.
3+
# Declare variables to be passed into your templates.
4+
5+
replicaCount: 3
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: {}

‎internal/helm/chart.go‎

Lines changed: 7 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -25,46 +25,30 @@ import (
2525
)
2626

2727
// OverwriteChartDefaultValues overwrites the chart default values file with the
28-
// contents of the given valuesFile.
29-
func OverwriteChartDefaultValues(chart *helmchart.Chart, valuesFile string) (bool, error) {
30-
if valuesFile == "" || valuesFile == chartutil.ValuesfileName {
31-
return false, nil
32-
}
33-
34-
// Find override file and retrieve contents
35-
var valuesData []byte
36-
for _, f := range chart.Files {
37-
if f.Name == valuesFile {
38-
valuesData = f.Data
39-
break
40-
}
41-
}
42-
if valuesData == nil {
43-
return false, fmt.Errorf("failed to locate override values file: %s", valuesFile)
44-
}
45-
28+
// given data.
29+
func OverwriteChartDefaultValues(chart *helmchart.Chart, data []byte) (bool, error) {
4630
// Read override values file data
47-
values, err := chartutil.ReadValues(valuesData)
31+
values, err := chartutil.ReadValues(data)
4832
if err != nil {
49-
return false, fmt.Errorf("failed to parse override values file: %s", valuesFile)
33+
return false, fmt.Errorf("failed to parse provided override values file data")
5034
}
5135

5236
// Replace current values file in Raw field
5337
for _, f := range chart.Raw {
5438
if f.Name == chartutil.ValuesfileName {
5539
// Do nothing if contents are equal
56-
if reflect.DeepEqual(f.Data, valuesData) {
40+
if reflect.DeepEqual(f.Data, data) {
5741
return false, nil
5842
}
5943

6044
// Replace in Files field
6145
for _, f := range chart.Files {
6246
if f.Name == chartutil.ValuesfileName {
63-
f.Data = valuesData
47+
f.Data = data
6448
}
6549
}
6650

67-
f.Data = valuesData
51+
f.Data = data
6852
chart.Values = values
6953
return true, nil
7054
}

0 commit comments

Comments
 (0)