Add validation to TargetPath and ValuesKey

Formalises the API requirements around TargetPath and ValuesKey, which were the two fields missing validation within ValuesReference. In both cases the validation was introduced at CRD level, so that the apiserver will enforce it. ValuesKey must be a valid Data Key. Therefore the same logic used by upstream Kubernetes is reused here to ensure a valid key is being used. For TargetPath a loose regex is being used to largely represent the expected format. A max length of 250 is now being enforced. This is a breaking change, as invalid TargetPath and ValuesKey will now be rejected by the apiserver, instead of being accepted and potentially failing at reconciliation time. Signed-off-by: Paulo Gomes <[email protected]>
fluxcd · Aug 17, 2022 · 067854e · 067854e
1 parent 2195310
commit 067854e
Show file tree

Hide file tree

Showing 7 changed files with 205 additions and 47 deletions.
diff --git a/api/v2beta1/reference_types.go b/api/v2beta1/reference_types.go
@@ -59,12 +59,18 @@ type ValuesReference struct {
 
 	// ValuesKey is the data key where the values.yaml or a specific value can be
 	// found at. Defaults to 'values.yaml'.
+	// When set, must be a valid Data Key, consisting of alphanumeric characters,
+	// '-', '_' or '.'.
+	// +kubebuilder:validation:MaxLength=253
+	// +kubebuilder:validation:Pattern=`^[\-._a-zA-Z0-9]+$`
 	// +optional
 	ValuesKey string `json:"valuesKey,omitempty"`
 
 	// TargetPath is the YAML dot notation path the value should be merged at. When
 	// set, the ValuesKey is expected to be a single flat value. Defaults to 'None',
 	// which results in the values getting merged at the root.
+	// +kubebuilder:validation:MaxLength=250
+	// +kubebuilder:validation:Pattern=`^([a-zA-Z0-9_\-.\\\/]|\[[0-9]{1,5}\])+$`
 	// +optional
 	TargetPath string `json:"targetPath,omitempty"`
 

diff --git a/config/crd/bases/helm.toolkit.fluxcd.io_helmreleases.yaml b/config/crd/bases/helm.toolkit.fluxcd.io_helmreleases.yaml
@@ -697,10 +697,16 @@ spec:
                         should be merged at. When set, the ValuesKey is expected to
                         be a single flat value. Defaults to 'None', which results
                         in the values getting merged at the root.
+                      maxLength: 250
+                      pattern: ^([a-zA-Z0-9_\-.\\\/]|\[[0-9]{1,5}\])+$
                       type: string
                     valuesKey:
                       description: ValuesKey is the data key where the values.yaml
                         or a specific value can be found at. Defaults to 'values.yaml'.
+                        When set, must be a valid Data Key, consisting of alphanumeric
+                        characters, '-', '_' or '.'.
+                      maxLength: 253
+                      pattern: ^[\-._a-zA-Z0-9]+$
                       type: string
                   required:
                   - kind

diff --git a/controllers/helmrelease_controller_test.go b/controllers/helmrelease_controller_test.go
@@ -19,14 +19,17 @@ package controllers
 import (
 	"context"
 	"reflect"
+	"strings"
 	"testing"
+	"time"
 
 	"github.com/go-logr/logr"
 	"helm.sh/helm/v3/pkg/chartutil"
 	corev1 "k8s.io/api/core/v1"
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 	"sigs.k8s.io/yaml"
 
@@ -278,6 +281,174 @@ invalid`,
 	}
 }
 
+func TestValuesReferenceValidation(t *testing.T) {
+	tests := []struct {
+		name       string
+		references []v2.ValuesReference
+		values     string
+		wantErr    bool
+	}{
+		{
+			name: "valid ValuesKey",
+			references: []v2.ValuesReference{
+				{
+					Kind:      "Secret",
+					Name:      "values",
+					ValuesKey: "any-key_na.me",
+				},
+			},
+			wantErr: false,
+		},
+		{
+			name: "valid ValuesKey: empty",
+			references: []v2.ValuesReference{
+				{
+					Kind:      "Secret",
+					Name:      "values",
+					ValuesKey: "",
+				},
+			},
+			wantErr: false,
+		},
+		{
+			name: "valid ValuesKey: long",
+			references: []v2.ValuesReference{
+				{
+					Kind:      "Secret",
+					Name:      "values",
+					ValuesKey: strings.Repeat("a", 253),
+				},
+			},
+			wantErr: false,
+		},
+		{
+			name: "invalid ValuesKey",
+			references: []v2.ValuesReference{
+				{
+					Kind:      "Secret",
+					Name:      "values",
+					ValuesKey: "a($&^%b",
+				},
+			},
+			wantErr: true,
+		},
+		{
+			name: "invalid ValuesKey: too long",
+			references: []v2.ValuesReference{
+				{
+					Kind:      "Secret",
+					Name:      "values",
+					ValuesKey: strings.Repeat("a", 254),
+				},
+			},
+			wantErr: true,
+		},
+		{
+			name: "valid target path: empty",
+			references: []v2.ValuesReference{
+				{
+					Kind:       "Secret",
+					Name:       "values",
+					TargetPath: "",
+				},
+			},
+			wantErr: false,
+		},
+		{
+			name: "valid target path",
+			references: []v2.ValuesReference{
+				{
+					Kind:       "Secret",
+					Name:       "values",
+					TargetPath: "list_with.nested-values.and.index[0]",
+				},
+			},
+			wantErr: false,
+		},
+		{
+			name: "valid target path: long",
+			references: []v2.ValuesReference{
+				{
+					Kind:       "Secret",
+					Name:       "values",
+					TargetPath: func() string { return strings.Repeat("a", 250) }(),
+				},
+			},
+			wantErr: false,
+		},
+		{
+			name: "invalid target path: too long",
+			references: []v2.ValuesReference{
+				{
+					Kind:       "Secret",
+					Name:       "values",
+					TargetPath: func() string { return strings.Repeat("a", 251) }(),
+				},
+			},
+			wantErr: true,
+		},
+		{
+			name: "invalid target path: opened index",
+			references: []v2.ValuesReference{
+				{
+					Kind:       "Secret",
+					Name:       "values",
+					ValuesKey:  "single",
+					TargetPath: "a[",
+				},
+			},
+			wantErr: true,
+		},
+		{
+			name: "invalid target path: incorrect index syntax",
+			references: []v2.ValuesReference{
+				{
+					Kind:       "Secret",
+					Name:       "values",
+					ValuesKey:  "single",
+					TargetPath: "a]0[",
+				},
+			},
+			wantErr: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			var values *apiextensionsv1.JSON
+			if tt.values != "" {
+				v, _ := yaml.YAMLToJSON([]byte(tt.values))
+				values = &apiextensionsv1.JSON{Raw: v}
+			}
+
+			hr := v2.HelmRelease{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test",
+					Namespace: "default",
+				},
+				Spec: v2.HelmReleaseSpec{
+					Interval: metav1.Duration{Duration: 5 * time.Minute},
+					Chart: v2.HelmChartTemplate{
+						Spec: v2.HelmChartTemplateSpec{
+							SourceRef: v2.CrossNamespaceObjectReference{
+								Name: "something",
+							},
+						},
+					},
+					ValuesFrom: tt.references,
+					Values:     values,
+				},
+			}
+
+			err := k8sClient.Create(context.TODO(), &hr, client.DryRunAll)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("composeValues() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+		})
+	}
+}
+
 func valuesSecret(name string, data map[string][]byte) *corev1.Secret {
 	return &corev1.Secret{
 		ObjectMeta: metav1.ObjectMeta{Name: name},

diff --git a/controllers/suite_test.go b/controllers/suite_test.go
@@ -17,67 +17,48 @@ limitations under the License.
 package controllers
 
 import (
+	"fmt"
+	"os"
 	"path/filepath"
 	"testing"
 
-	. "github.com/onsi/ginkgo"
-	. "github.com/onsi/gomega"
+	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 	"k8s.io/client-go/kubernetes/scheme"
 	"k8s.io/client-go/rest"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/envtest"
-	"sigs.k8s.io/controller-runtime/pkg/envtest/printer"
-	logf "sigs.k8s.io/controller-runtime/pkg/log"
-	"sigs.k8s.io/controller-runtime/pkg/log/zap"
 
 	"github.com/fluxcd/helm-controller/api/v2beta1"
 	// +kubebuilder:scaffold:imports
 )
 
-// These tests use Ginkgo (BDD-style Go testing framework). Refer to
-// http://onsi.github.io/ginkgo/ to learn more about Ginkgo.
-
 var cfg *rest.Config
 var k8sClient client.Client
 var testEnv *envtest.Environment
 
-func TestAPIs(t *testing.T) {
-	RegisterFailHandler(Fail)
-
-	RunSpecsWithDefaultAndCustomReporters(t,
-		"Controller Suite",
-		[]Reporter{printer.NewlineReporter{}})
-}
-
-var _ = BeforeSuite(func(done Done) {
-	logf.SetLogger(
-		zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true)),
-	)
-
-	By("bootstrapping test environment")
+func TestMain(m *testing.M) {
 	testEnv = &envtest.Environment{
 		CRDDirectoryPaths: []string{filepath.Join("..", "config", "crd", "bases")},
 	}
 
 	var err error
 	cfg, err = testEnv.Start()
-	Expect(err).ToNot(HaveOccurred())
-	Expect(cfg).ToNot(BeNil())
-
-	err = v2beta1.AddToScheme(scheme.Scheme)
-	Expect(err).NotTo(HaveOccurred())
-
-	// +kubebuilder:scaffold:scheme
+	if err != nil {
+		panic(fmt.Errorf("failed to start testenv: %v", err))
+	}
 
+	utilruntime.Must(v2beta1.AddToScheme(scheme.Scheme))
 	k8sClient, err = client.New(cfg, client.Options{Scheme: scheme.Scheme})
-	Expect(err).ToNot(HaveOccurred())
-	Expect(k8sClient).ToNot(BeNil())
+	if err != nil {
+		panic(fmt.Errorf("failed to create k8s client: %v", err))
+	}
 
-	close(done)
-}, 60)
+	code := m.Run()
 
-var _ = AfterSuite(func() {
-	By("tearing down the test environment")
-	err := testEnv.Stop()
-	Expect(err).ToNot(HaveOccurred())
-})
+	err = testEnv.Stop()
+	if err != nil {
+		panic(fmt.Errorf("failed to stop testenv: %v", err))
+	}
+
+	os.Exit(code)
+}
diff --git a/docs/api/helmrelease.md b/docs/api/helmrelease.md
@@ -2092,7 +2092,9 @@ string
 <td>
 <em>(Optional)</em>
 <p>ValuesKey is the data key where the values.yaml or a specific value can be
-found at. Defaults to &lsquo;values.yaml&rsquo;.</p>
+found at. Defaults to &lsquo;values.yaml&rsquo;.
+When set, must be a valid Data Key, consisting of alphanumeric characters,
+&lsquo;-&rsquo;, &lsquo;_&rsquo; or &lsquo;.&rsquo;.</p>
 </td>
 </tr>
 <tr>

diff --git a/go.mod b/go.mod
@@ -13,7 +13,6 @@ require (
 	github.com/fluxcd/source-controller/api v0.25.9
 	github.com/go-logr/logr v1.2.3
 	github.com/hashicorp/go-retryablehttp v0.7.1
-	github.com/onsi/ginkgo v1.16.5
 	github.com/onsi/gomega v1.19.0
 	github.com/spf13/pflag v1.0.5
 	helm.sh/helm/v3 v3.9.0
@@ -120,7 +119,6 @@ require (
 	github.com/monochromegane/go-gitignore v0.0.0-20200626010858-205db1a8cc00 // indirect
 	github.com/morikuni/aec v1.0.0 // indirect
 	github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect
-	github.com/nxadm/tail v1.4.8 // indirect
 	github.com/opencontainers/go-digest v1.0.0 // indirect
 	github.com/opencontainers/image-spec v1.0.3-0.20211202183452-c5a74bcca799 // indirect
 	github.com/peterbourgon/diskv v2.0.1+incompatible // indirect
@@ -157,7 +155,6 @@ require (
 	google.golang.org/grpc v1.43.0 // indirect
 	google.golang.org/protobuf v1.28.0 // indirect
 	gopkg.in/inf.v0 v0.9.1 // indirect
-	gopkg.in/tomb.v1 v1.0.0-20141024135613-dd632973f1e7 // indirect
 	gopkg.in/yaml.v2 v2.4.0 // indirect
 	gopkg.in/yaml.v3 v3.0.0-20210107192922-496545a6307b // indirect
 	k8s.io/apiserver v0.24.1 // indirect

diff --git a/go.sum b/go.sum
@@ -259,7 +259,6 @@ github.com/go-sql-driver/mysql v1.4.1/go.mod h1:zAC/RDZ24gD3HViQzih4MyKcchzm+sOG
 github.com/go-sql-driver/mysql v1.5.0 h1:ozyZYNQW3x3HtqT1jira07DN2PArx2v7/mN66gGcHOs=
 github.com/go-sql-driver/mysql v1.5.0/go.mod h1:DCzpHaOWr8IXmIStZouvnhqoel9Qv2LBy8hT2VhHyBg=
 github.com/go-stack/stack v1.8.0/go.mod h1:v0f6uXyyMGvRgIKkXu+yp6POWl0qKG85gN/melR3HDY=
-github.com/go-task/slim-sprig v0.0.0-20210107165309-348f09dbbbc0/go.mod h1:fyg7847qk6SyHyPtNmDHnmrv/HOrqktSC+C9fM+CJOE=
 github.com/gobuffalo/logger v1.0.6 h1:nnZNpxYo0zx+Aj9RfMPBm+x9zAU2OayFh/xrAWi34HU=
 github.com/gobuffalo/logger v1.0.6/go.mod h1:J31TBEHR1QLV2683OXTAItYIg8pv2JMHnF/quuAbMjs=
 github.com/gobuffalo/packd v1.0.1 h1:U2wXfRr4E9DH8IdsDLlRFwTZTK7hLfq9qT/QHXGVe/0=
@@ -550,7 +549,6 @@ github.com/mxk/go-flowrate v0.0.0-20140419014527-cca7078d478f/go.mod h1:ZdcZmHo+
 github.com/niemeyer/pretty v0.0.0-20200227124842-a10e7caefd8e/go.mod h1:zD1mROLANZcx1PVRCS0qkT7pwLkGfwJo4zjcN/Tysno=
 github.com/nxadm/tail v1.4.4/go.mod h1:kenIhsEOeOJmVchQTgglprH7qJGnHDVpk1VPCcaMI8A=
 github.com/nxadm/tail v1.4.8 h1:nPr65rt6Y5JFSKQO7qToXr7pePgD6Gwiw05lkbyAQTE=
-github.com/nxadm/tail v1.4.8/go.mod h1:+ncqLTQzXmGhMZNUePPaPqPvBxHAIsmXswZKocGu+AU=
 github.com/oklog/ulid v1.3.1/go.mod h1:CirwcVhetQ6Lv90oh/F+FBtV6XMibvdAFo93nm5qn4U=
 github.com/olekukonko/tablewriter v0.0.4/go.mod h1:zq6QwlOf5SlnkVbMSr5EoBv3636FWnp+qbPhuoO21uA=
 github.com/olekukonko/tablewriter v0.0.5/go.mod h1:hPp6KlRPjbx+hW8ykQs1w3UBbZlj6HuIJcUGPhkA7kY=
@@ -559,7 +557,6 @@ github.com/onsi/ginkgo v1.6.0/go.mod h1:lLunBs/Ym6LB5Z9jYTR76FiuTmxDTDusOGeTQH+W
 github.com/onsi/ginkgo v1.12.1/go.mod h1:zj2OWP4+oCPe1qIXoGWkgMRwljMUYCdkwsT2108oapk=
 github.com/onsi/ginkgo v1.14.0/go.mod h1:iSB4RoI2tjJc9BBv4NKIKWKya62Rps+oPG/Lv9klQyY=
 github.com/onsi/ginkgo v1.16.5 h1:8xi0RTUf59SOSfEtZMvwTvXYMzG4gV23XVHOZiXNtnE=
-github.com/onsi/ginkgo v1.16.5/go.mod h1:+E8gABHa3K6zRBolWtd+ROzc/U5bkGt0FwiG042wbpU=
 github.com/onsi/ginkgo/v2 v2.1.3 h1:e/3Cwtogj0HA+25nMP1jCMDIf8RtRYbGwGGuBIFztkc=
 github.com/onsi/gomega v0.0.0-20170829124025-dcabb60a477c/go.mod h1:C1qb7wdrVGGVU+Z6iS04AVkA3Q65CEZX59MT0QO5uiA=
 github.com/onsi/gomega v1.7.1/go.mod h1:XdKZgCCFLUoM/7CFJVPcG8C1xQ1AJ0vpAezJrB7JYyY=
@@ -933,7 +930,6 @@ golang.org/x/sys v0.0.0-20200930185726-fdedc70b468f/go.mod h1:h1NjWce9XRLGQEsW7w
 golang.org/x/sys v0.0.0-20201119102817-f84b799fce68/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
 golang.org/x/sys v0.0.0-20201201145000-ef89a241ccb3/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
 golang.org/x/sys v0.0.0-20210104204734-6f8348627aad/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
-golang.org/x/sys v0.0.0-20210112080510-489259a85091/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
 golang.org/x/sys v0.0.0-20210119212857-b64e53b001e4/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
 golang.org/x/sys v0.0.0-20210124154548-22da62e12c0c/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
 golang.org/x/sys v0.0.0-20210220050731-9a76102bfb43/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
@@ -1034,7 +1030,6 @@ golang.org/x/tools v0.0.0-20200904185747-39188db58858/go.mod h1:Cj7w3i3Rnn0Xh82u
 golang.org/x/tools v0.0.0-20201110124207-079ba7bd75cd/go.mod h1:emZCQorbCU4vsT4fOWvOPXz4eW1wZW4PmDk9uLelYpA=
 golang.org/x/tools v0.0.0-20201201161351-ac6f37ff4c2a/go.mod h1:emZCQorbCU4vsT4fOWvOPXz4eW1wZW4PmDk9uLelYpA=
 golang.org/x/tools v0.0.0-20201208233053-a543418bbed2/go.mod h1:emZCQorbCU4vsT4fOWvOPXz4eW1wZW4PmDk9uLelYpA=
-golang.org/x/tools v0.0.0-20201224043029-2b0845dc783e/go.mod h1:emZCQorbCU4vsT4fOWvOPXz4eW1wZW4PmDk9uLelYpA=
 golang.org/x/tools v0.0.0-20210105154028-b0ab187a4818/go.mod h1:emZCQorbCU4vsT4fOWvOPXz4eW1wZW4PmDk9uLelYpA=
 golang.org/x/tools v0.0.0-20210106214847-113979e3529a/go.mod h1:emZCQorbCU4vsT4fOWvOPXz4eW1wZW4PmDk9uLelYpA=
 golang.org/x/tools v0.1.0/go.mod h1:xkSsbof2nBLbhDlRMhhhyNLN/zl3eTqcnHD5viDpcZ0=