Add validation to TargetPath and ValuesKey

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 and a max length is enforced.

The validation is only checked within the reconciliation, to avoid
causing issues to users upgrading into a new version and have
HelmReleases objects in an existing cluster, that would no longer be
accepted by the API server.

Signed-off-by: Paulo Gomes <paulo.gomes@weave.works>

Author: Paulo Gomes

api/v2beta1/reference_types.go

@@ -59,6 +59,8 @@ 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 '.'.
 	// +optional
 	ValuesKey string `json:"valuesKey,omitempty"`
 

config/crd/bases/helm.toolkit.fluxcd.io_helmreleases.yaml

@@ -701,6 +701,8 @@ spec:
                     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 '.'.
                       type: string
                   required:
                   - kind

controllers/helmrelease_controller.go

@@ -20,6 +20,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"regexp"
 	"strings"
 	"time"
 
@@ -34,6 +35,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/apimachinery/pkg/util/validation"
 	"k8s.io/cli-runtime/pkg/genericclioptions"
 	"k8s.io/client-go/rest"
 	kuberecorder "k8s.io/client-go/tools/record"
@@ -65,6 +67,11 @@ import (
 	"github.com/fluxcd/helm-controller/internal/util"
 )
 
+var (
+	targetPathMaxLen = 250
+	targetPathRegex  = regexp.MustCompilePOSIX(`^([a-zA-Z0-9_.]|\[[0-9]{1,5}\])+$`)
+)
+
 // +kubebuilder:rbac:groups=helm.toolkit.fluxcd.io,resources=helmreleases,verbs=get;list;watch;create;update;patch;delete
 // +kubebuilder:rbac:groups=helm.toolkit.fluxcd.io,resources=helmreleases/status,verbs=get;update;patch
 // +kubebuilder:rbac:groups=helm.toolkit.fluxcd.io,resources=helmreleases/finalizers,verbs=get;create;update;patch;delete
@@ -512,6 +519,14 @@ func (r *HelmReleaseReconciler) composeValues(ctx context.Context, hr v2.HelmRel
 		namespacedName := types.NamespacedName{Namespace: hr.Namespace, Name: v.Name}
 		var valuesData []byte
 
+		if !isValidValuesKey(v.ValuesKey) {
+			return nil, fmt.Errorf("invalid ValuesKey: %s", v.ValuesKey)
+		}
+
+		if !isValidTargetPath(v.TargetPath) {
+			return nil, fmt.Errorf("invalid TargetPath: %s", v.TargetPath)
+		}
+
 		switch v.Kind {
 		case "ConfigMap":
 			resource, ok := configMaps[namespacedName.String()]
@@ -772,3 +787,21 @@ func (r *HelmReleaseReconciler) recordReadiness(ctx context.Context, hr v2.HelmR
 		}, !hr.DeletionTimestamp.IsZero())
 	}
 }
+
+func isValidTargetPath(targetPath string) bool {
+	if len(targetPath) > targetPathMaxLen {
+		return false
+	}
+	if targetPath == "" {
+		return true
+	}
+	return targetPathRegex.MatchString(targetPath)
+}
+
+func isValidValuesKey(valuesKey string) bool {
+	if valuesKey == "" {
+		return true
+	}
+	// validation.IsConfigMapKey returns an array of errors found.
+	return len(validation.IsConfigMapKey(valuesKey)) == 0
+}

controllers/helmrelease_controller_test.go

@@ -19,6 +19,7 @@ package controllers
 import (
 	"context"
 	"reflect"
+	"strings"
 	"testing"
 
 	"github.com/go-logr/logr"
@@ -249,6 +250,21 @@ invalid`,
 			},
 			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 {
@@ -278,6 +294,52 @@ invalid`,
 	}
 }
 
+func TestTargetPath(t *testing.T) {
+	tests := []struct {
+		name  string
+		input []string
+		want  bool
+	}{
+		{
+			name: "valid TargetPaths",
+			input: []string{
+				"list2",
+				"with_underscore",
+				"list.example[2]",
+				"list[2][2].test",
+				"some.longer.multi.level.target.path.that.is.too.long.to.see.but.still.within.a.reasonable.limit",
+			},
+			want: true,
+		},
+		{
+			name: "invalid TargetPaths",
+			input: []string{
+				func() string { return strings.Repeat("a", 251) }(),
+				"test=",
+				"test%",
+				"test[623221].a",
+				"test[a]",
+				"test]0[",
+				"test[0[",
+				"test[0",
+				"test$",
+			},
+			want: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			for _, targetPath := range tt.input {
+				got := isValidTargetPath(targetPath)
+				if got != tt.want {
+					t.Errorf("%s: got = %v, want %v", targetPath, got, tt.want)
+				}
+			}
+		})
+	}
+}
+
 func valuesSecret(name string, data map[string][]byte) *corev1.Secret {
 	return &corev1.Secret{
 		ObjectMeta: metav1.ObjectMeta{Name: name},

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>