Validate .spec.name and apply its default consistently

apis/v1/servicebinding_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package v1
 
 import (
+	"strings"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
@@ -78,6 +79,26 @@ func TestServiceBindingDefault(t *testing.T) {
 	}
 }
 
+// serviceBindingNamed returns a ServiceBinding that is valid other than .spec.name, which is set to
+// the provided value. Used by the .spec.name validation cases below.
+func serviceBindingNamed(name string) *ServiceBinding {
+	return &ServiceBinding{
+		Spec: ServiceBindingSpec{
+			Name: name,
+			Service: ServiceBindingServiceReference{
+				APIVersion: "v1",
+				Kind:       "Secret",
+				Name:       "my-service",
+			},
+			Workload: ServiceBindingWorkloadReference{
+				APIVersion: "apps/v1",
+				Kind:       "Deployment",
+				Name:       "my-workload",
+			},
+		},
+	}
+}
+
 func TestServiceBindingValidate(t *testing.T) {
 	tests := []struct {
 		name     string
@@ -274,6 +295,195 @@ func TestServiceBindingValidate(t *testing.T) {
 	}
 }
 
+// .spec.name is projected into the workload as a volume mount directory at
+// $SERVICE_BINDING_ROOT/<name>. The spec requires binding names to match [a-z0-9\-\.]{1,253}; "." and
+// ".." additionally escape the binding root. The rule is applied by the webhook entry points rather
+// than by validate, so both are exercised here: on update the name is changed, since an unchanged
+// name is not revalidated.
+func TestServiceBindingValidateName(t *testing.T) {
+	tests := []struct {
+		name        string
+		bindingName string
+		expected    field.ErrorList
+	}{
+		{
+			name:        "valid",
+			bindingName: "my-binding",
+			expected:    field.ErrorList{},
+		},
+		{
+			name:        "valid leading hyphen",
+			bindingName: "-foo",
+			expected:    field.ErrorList{},
+		},
+		{
+			name:        "valid trailing hyphen",
+			bindingName: "foo-",
+			expected:    field.ErrorList{},
+		},
+		{
+			name:        "valid consecutive dots",
+			bindingName: "foo..bar",
+			expected:    field.ErrorList{},
+		},
+		{
+			name:        "valid trailing dot",
+			bindingName: "foo.",
+			expected:    field.ErrorList{},
+		},
+		{
+			name:        "valid max length",
+			bindingName: strings.Repeat("a", 253),
+			expected:    field.ErrorList{},
+		},
+		{
+			name:        "invalid parent directory",
+			bindingName: "..",
+			expected: field.ErrorList{
+				field.Invalid(field.NewPath("spec", "name"), "..", `must not be "." or ".."`),
+			},
+		},
+		{
+			name:        "invalid current directory",
+			bindingName: ".",
+			expected: field.ErrorList{
+				field.Invalid(field.NewPath("spec", "name"), ".", `must not be "." or ".."`),
+			},
+		},
+		{
+			name:        "invalid path traversal",
+			bindingName: "../../etc",
+			expected: field.ErrorList{
+				field.Invalid(field.NewPath("spec", "name"), "../../etc", bindingNameErrMsg),
+			},
+		},
+		{
+			name:        "invalid uppercase",
+			bindingName: "Foo",
+			expected: field.ErrorList{
+				field.Invalid(field.NewPath("spec", "name"), "Foo", bindingNameErrMsg),
+			},
+		},
+		{
+			name:        "invalid underscore",
+			bindingName: "foo_bar",
+			expected: field.ErrorList{
+				field.Invalid(field.NewPath("spec", "name"), "foo_bar", bindingNameErrMsg),
+			},
+		},
+		{
+			name:        "invalid too long",
+			bindingName: strings.Repeat("a", 254),
+			expected: field.ErrorList{
+				field.Invalid(field.NewPath("spec", "name"), strings.Repeat("a", 254), bindingNameErrMsg),
+			},
+		},
+	}
+
+	for _, c := range tests {
+		t.Run(c.name, func(t *testing.T) {
+			seed := serviceBindingNamed(c.bindingName)
+
+			if diff := cmp.Diff(c.expected, seed.Spec.validateName(field.NewPath("spec", "name"))); diff != "" {
+				t.Errorf("validateName (-expected, +actual): %s", diff)
+			}
+
+			expectedErr := c.expected.ToAggregate()
+
+			_, actualCreateErr := (&ServiceBinding{}).ValidateCreate(t.Context(), seed.DeepCopy())
+			if diff := cmp.Diff(expectedErr, actualCreateErr); diff != "" {
+				t.Errorf("ValidateCreate (-expected, +actual): %s", diff)
+			}
+
+			// the old object carries a different .spec.name so that the name is revalidated
+			old := seed.DeepCopy()
+			old.Spec.Name = "previous-name"
+
+			_, actualUpdateErr := (&ServiceBinding{}).ValidateUpdate(t.Context(), old, seed.DeepCopy())
+			if diff := cmp.Diff(expectedErr, actualUpdateErr); diff != "" {
+				t.Errorf("ValidateUpdate (-expected, +actual): %s", diff)
+			}
+		})
+	}
+}
+
+// A ServiceBinding that omits .spec.name is valid: the webhook defaults the field from .metadata.name
+// before validating it, so a defaulted value is validated exactly like an explicit one.
+func TestServiceBindingValidate_DefaultedName(t *testing.T) {
+	tests := []struct {
+		name     string
+		metaName string
+	}{
+		{name: "simple", metaName: "my-binding"},
+		{name: "dotted", metaName: "a.b.c"},
+		{name: "max length", metaName: strings.Repeat("a", 253)},
+	}
+
+	for _, c := range tests {
+		t.Run(c.name, func(t *testing.T) {
+			seed := serviceBindingNamed("")
+			seed.ObjectMeta = metav1.ObjectMeta{Name: c.metaName}
+
+			obj := seed.DeepCopy()
+			if _, err := (&ServiceBinding{}).ValidateCreate(t.Context(), obj); err != nil {
+				t.Errorf("ValidateCreate: unexpected error: %s", err)
+			}
+			if obj.Spec.Name != c.metaName {
+				t.Errorf("expected .spec.name defaulted to %q, got %q", c.metaName, obj.Spec.Name)
+			}
+
+			if _, err := (&ServiceBinding{}).ValidateUpdate(t.Context(), seed.DeepCopy(), seed.DeepCopy()); err != nil {
+				t.Errorf("ValidateUpdate: unexpected error: %s", err)
+			}
+		})
+	}
+}
+
+// .spec.name validation is ratcheted on update: a value that predates this validation does not by
+// itself block writes to the object. Deleting a ServiceBinding requires clearing its finalizer,
+// which is an UPDATE, so rejecting an unchanged name would leave objects created before this
+// validation stuck terminating. Introducing or changing to an invalid name is still rejected.
+func TestServiceBindingValidate_RatchetName(t *testing.T) {
+	tests := []struct {
+		name            string
+		oldName         string
+		newName         string
+		expectUpdateErr bool
+		expectCreateErr bool
+	}{
+		{name: "unchanged invalid name is allowed", oldName: "Legacy_Name", newName: "Legacy_Name", expectUpdateErr: false, expectCreateErr: true},
+		{name: "unchanged traversal name is allowed, so it can be deleted", oldName: "..", newName: "..", expectUpdateErr: false, expectCreateErr: true},
+		{name: "changed to another invalid name is rejected", oldName: "Legacy_Name", newName: "Other_Bad", expectUpdateErr: true, expectCreateErr: true},
+		{name: "changed to a valid name is allowed", oldName: "Legacy_Name", newName: "legacy-name", expectUpdateErr: false, expectCreateErr: false},
+		{name: "newly introduced traversal is rejected", oldName: "good-name", newName: "../../etc", expectUpdateErr: true, expectCreateErr: true},
+		{name: "valid name unchanged is allowed", oldName: "good-name", newName: "good-name", expectUpdateErr: false, expectCreateErr: false},
+	}
+
+	for _, c := range tests {
+		t.Run(c.name, func(t *testing.T) {
+			old := serviceBindingNamed(c.oldName)
+			obj := serviceBindingNamed(c.newName)
+
+			_, err := (&ServiceBinding{}).ValidateUpdate(t.Context(), old, obj)
+			if c.expectUpdateErr && err == nil {
+				t.Errorf("ValidateUpdate: expected an error, got none")
+			}
+			if !c.expectUpdateErr && err != nil {
+				t.Errorf("ValidateUpdate: unexpected error: %s", err)
+			}
+
+			// creating the same object outright is always validated, never ratcheted
+			_, err = (&ServiceBinding{}).ValidateCreate(t.Context(), serviceBindingNamed(c.newName))
+			if c.expectCreateErr && err == nil {
+				t.Errorf("ValidateCreate(%q): expected an error, got none", c.newName)
+			}
+			if !c.expectCreateErr && err != nil {
+				t.Errorf("ValidateCreate(%q): unexpected error: %s", c.newName, err)
+			}
+		})
+	}
+}
+
 func TestServiceBindingValidate_Immutable(t *testing.T) {
 	tests := []struct {
 		name     string

apis/v1/servicebinding_webhook.go

@@ -18,6 +18,7 @@ package v1
 
 import (
 	"context"
+	"regexp"
 
 	"github.com/go-logr/logr"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -54,14 +55,19 @@ func (*ServiceBinding) ValidateCreate(ctx context.Context, obj *ServiceBinding)
 	log.V(1).Info("Validating Create")
 
 	(&ServiceBinding{}).Default(ctx, obj)
-	return nil, obj.validate().ToAggregate()
+
+	errs := obj.Spec.validateName(field.NewPath("spec", "name"))
+	errs = append(errs, obj.validate()...)
+
+	return nil, errs.ToAggregate()
 }
 
 // ValidateUpdate implements webhook.CustomValidator so a webhook will be registered for the type
 func (*ServiceBinding) ValidateUpdate(ctx context.Context, old, obj *ServiceBinding) (admission.Warnings, error) {
 	log := logr.FromContextOrDiscard(ctx)
 	log.V(1).Info("Validating Update")
 
+	(&ServiceBinding{}).Default(ctx, old)
 	(&ServiceBinding{}).Default(ctx, obj)
 	errs := field.ErrorList{}
 
@@ -77,6 +83,14 @@ func (*ServiceBinding) ValidateUpdate(ctx context.Context, old, obj *ServiceBind
 		)
 	}
 
+	// the binding name is projected as a directory under $SERVICE_BINDING_ROOT, so its format is
+	// restricted. Like the immutable fields above, it is only checked when the value changes so that
+	// objects predating this rule are not frozen by it. Deleting one clears a finalizer, which is an
+	// update, so rejecting an unchanged name would leave them stuck terminating.
+	if obj.Spec.Name != old.Spec.Name {
+		errs = append(errs, obj.Spec.validateName(field.NewPath("spec", "name"))...)
+	}
+
 	// validate new object
 	errs = append(errs, obj.validate()...)
 
@@ -99,6 +113,35 @@ func (r *ServiceBinding) validate() field.ErrorList {
 	return errs
 }
 
+// bindingNameErrMsg describes the format the ServiceBinding specification requires of .spec.name:
+// "Binding names MUST match [a-z0-9\-\.]{1,253}".
+const bindingNameErrMsg = "must consist of lower case alphanumeric characters, '-' or '.', and must be no more than 253 characters"
+
+// bindingNameRE is the binding name pattern required by the specification, anchored.
+var bindingNameRE = regexp.MustCompile(`^[a-z0-9.-]{1,253}$`)
+
+// validateName checks the format the specification requires of a binding name. An empty name is
+// reported as required by validate, not here. The check is applied by the webhook entry points, which
+// skip it for an unchanged value on update.
+func (r *ServiceBindingSpec) validateName(fldPath *field.Path) field.ErrorList {
+	errs := field.ErrorList{}
+
+	if r.Name == "" {
+		return errs
+	}
+
+	if !bindingNameRE.MatchString(r.Name) {
+		errs = append(errs, field.Invalid(fldPath, r.Name, bindingNameErrMsg))
+	} else if r.Name == "." || r.Name == ".." {
+		// the name is projected as a directory under $SERVICE_BINDING_ROOT. Since the pattern above
+		// admits no path separator, these are the only two values that can resolve outside that
+		// directory: "." to the binding root itself and ".." to its parent.
+		errs = append(errs, field.Invalid(fldPath, r.Name, `must not be "." or ".."`))
+	}
+
+	return errs
+}
+
 func (r *ServiceBindingSpec) validate(fldPath *field.Path) field.ErrorList {
 	errs := field.ErrorList{}