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 16, 2022 · ee17502 · ee17502
1 parent 2195310
commit ee17502
Show file tree

Hide file tree

Showing 5 changed files with 239 additions and 39 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,208 @@ invalid`,
 	}
 }
 
+func TestValidation(t *testing.T) {
+	tests := []struct {
+		name       string
+		resources  []runtime.Object
+		references []v2.ValuesReference
+		values     string
+		wantErr    bool
+	}{
+		{
+			name: "valid ValuesKey",
+			resources: []runtime.Object{
+				valuesSecret("values", map[string][]byte{"single": []byte("true")}),
+			},
+			references: []v2.ValuesReference{
+				{
+					Kind:      "Secret",
+					Name:      "values",
+					ValuesKey: "any-key_na.me",
+				},
+			},
+			wantErr: false,
+		},
+		{
+			name: "valid ValuesKey: empty",
+			resources: []runtime.Object{
+				valuesSecret("values", map[string][]byte{"single": []byte("true")}),
+			},
+			references: []v2.ValuesReference{
+				{
+					Kind:      "Secret",
+					Name:      "values",
+					ValuesKey: "",
+				},
+			},
+			wantErr: false,
+		},
+		{
+			name: "valid ValuesKey: long",
+			resources: []runtime.Object{
+				valuesSecret("values", map[string][]byte{"single": []byte("true")}),
+			},
+			references: []v2.ValuesReference{
+				{
+					Kind:      "Secret",
+					Name:      "values",
+					ValuesKey: func() string { return strings.Repeat("a", 253) }(),
+				},
+			},
+			wantErr: false,
+		},
+		{
+			name: "invalid ValuesKey",
+			resources: []runtime.Object{
+				valuesSecret("values", map[string][]byte{"single": []byte("true")}),
+			},
+			references: []v2.ValuesReference{
+				{
+					Kind:      "Secret",
+					Name:      "values",
+					ValuesKey: "a($&^%b",
+				},
+			},
+			wantErr: true,
+		},
+		{
+			name: "invalid ValuesKey: too long",
+			resources: []runtime.Object{
+				valuesSecret("values", map[string][]byte{"single": []byte("true")}),
+			},
+			references: []v2.ValuesReference{
+				{
+					Kind:      "Secret",
+					Name:      "values",
+					ValuesKey: func() string { return strings.Repeat("a", 254) }(),
+				},
+			},
+			wantErr: true,
+		},
+		{
+			name: "valid target path: empty",
+			resources: []runtime.Object{
+				valuesSecret("values", map[string][]byte{"single": []byte("true")}),
+			},
+			references: []v2.ValuesReference{
+				{
+					Kind:       "Secret",
+					Name:       "values",
+					TargetPath: "",
+				},
+			},
+			wantErr: false,
+		},
+		{
+			name: "valid target path",
+			resources: []runtime.Object{
+				valuesSecret("values", map[string][]byte{"single": []byte("true")}),
+			},
+			references: []v2.ValuesReference{
+				{
+					Kind:       "Secret",
+					Name:       "values",
+					TargetPath: "list_with.nested-values.and.index[0]",
+				},
+			},
+			wantErr: false,
+		},
+		{
+			name: "valid target path: long",
+			resources: []runtime.Object{
+				valuesSecret("values", map[string][]byte{"single": []byte("true")}),
+			},
+			references: []v2.ValuesReference{
+				{
+					Kind:       "Secret",
+					Name:       "values",
+					TargetPath: func() string { return strings.Repeat("a", 250) }(),
+				},
+			},
+			wantErr: false,
+		},
+		{
+			name: "invalid target path: too long",
+			resources: []runtime.Object{
+				valuesSecret("values", map[string][]byte{"single": []byte("true")}),
+			},
+			references: []v2.ValuesReference{
+				{
+					Kind:       "Secret",
+					Name:       "values",
+					TargetPath: func() string { return strings.Repeat("a", 251) }(),
+				},
+			},
+			wantErr: true,
+		},
+		{
+			name: "invalid target path: wrong index format",
+			resources: []runtime.Object{
+				valuesSecret("values", map[string][]byte{"single": []byte("true")}),
+			},
+			references: []v2.ValuesReference{
+				{
+					Kind:       "Secret",
+					Name:       "values",
+					ValuesKey:  "single",
+					TargetPath: "a[",
+				},
+			},
+			wantErr: true,
+		},
+		{
+			name: "invalid target path",
+			resources: []runtime.Object{
+				valuesSecret("values", map[string][]byte{"single": []byte("true")}),
+			},
+			references: []v2.ValuesReference{
+				{
+					Kind:       "Secret",
+					Name:       "values",
+					ValuesKey:  "single",
+					TargetPath: "a[",
+				},
+			},
+			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>