diff --git a/apis/v1/servicebinding_test.go b/apis/v1/servicebinding_test.go index c9794d2..f5671f0 100644 --- a/apis/v1/servicebinding_test.go +++ b/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/. 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 diff --git a/apis/v1/servicebinding_webhook.go b/apis/v1/servicebinding_webhook.go index b7072f2..e7b68fc 100644 --- a/apis/v1/servicebinding_webhook.go +++ b/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,7 +55,11 @@ 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 @@ -62,6 +67,7 @@ func (*ServiceBinding) ValidateUpdate(ctx context.Context, old, obj *ServiceBind 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{}