From a501c69a6d006f664afa3a670affaaf592958d6d Mon Sep 17 00:00:00 2001 From: Rashed Kamal Date: Tue, 11 Aug 2026 17:05:35 -0400 Subject: [PATCH 1/3] Validate .spec.name .spec.name is projected into workload containers as a volume mount directory at $SERVICE_BINDING_ROOT/, but was never validated. path.Join cleans its result, so `spec.name: ../../etc` produced MountPath: /etc, mounting the bound Secret over an arbitrary directory in every workload matched by .spec.workload. The specification already requires binding names to match [a-z0-9\-\.]{1,253}; the rule simply was not implemented. Enforce it, anchored, and additionally reject "." and "..". Those are the only values in that character set whose path.Join result can leave the intended directory: the set contains no "/", so the name is always one path element, and Clean gives special meaning to exactly those two. Name validation is ratcheted on update, so ServiceBindings created before this rule remain writable and deletable -- deletion clears a finalizer, which is an UPDATE, and would otherwise be rejected. Signed-off-by: Rashed Kamal --- apis/v1/servicebinding_test.go | 182 +++++++++++++++++++++++++++++- apis/v1/servicebinding_webhook.go | 38 ++++++- 2 files changed, 213 insertions(+), 7 deletions(-) diff --git a/apis/v1/servicebinding_test.go b/apis/v1/servicebinding_test.go index c9794d2..49f3ff0 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 @@ -246,11 +267,88 @@ func TestServiceBindingValidate(t *testing.T) { field.Required(field.NewPath("spec", "env[1]", "key"), ""), }, }, + + // .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. + + { + name: "name valid", + seed: serviceBindingNamed("my-binding"), + expected: field.ErrorList{}, + }, + { + name: "name valid leading hyphen", + seed: serviceBindingNamed("-foo"), + expected: field.ErrorList{}, + }, + { + name: "name valid trailing hyphen", + seed: serviceBindingNamed("foo-"), + expected: field.ErrorList{}, + }, + { + name: "name valid consecutive dots", + seed: serviceBindingNamed("foo..bar"), + expected: field.ErrorList{}, + }, + { + name: "name valid trailing dot", + seed: serviceBindingNamed("foo."), + expected: field.ErrorList{}, + }, + { + name: "name valid max length", + seed: serviceBindingNamed(strings.Repeat("a", 253)), + expected: field.ErrorList{}, + }, + { + name: "name invalid parent directory", + seed: serviceBindingNamed(".."), + expected: field.ErrorList{ + field.Invalid(field.NewPath("spec", "name"), "..", `must not be "." or ".."`), + }, + }, + { + name: "name invalid current directory", + seed: serviceBindingNamed("."), + expected: field.ErrorList{ + field.Invalid(field.NewPath("spec", "name"), ".", `must not be "." or ".."`), + }, + }, + { + name: "name invalid path traversal", + seed: serviceBindingNamed("../../etc"), + expected: field.ErrorList{ + field.Invalid(field.NewPath("spec", "name"), "../../etc", bindingNameErrMsg), + }, + }, + { + name: "name invalid uppercase", + seed: serviceBindingNamed("Foo"), + expected: field.ErrorList{ + field.Invalid(field.NewPath("spec", "name"), "Foo", bindingNameErrMsg), + }, + }, + { + name: "name invalid underscore", + seed: serviceBindingNamed("foo_bar"), + expected: field.ErrorList{ + field.Invalid(field.NewPath("spec", "name"), "foo_bar", bindingNameErrMsg), + }, + }, + { + name: "name invalid too long", + seed: serviceBindingNamed(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) { - if diff := cmp.Diff(c.expected, c.seed.validate()); diff != "" { + if diff := cmp.Diff(c.expected, c.seed.validate(nil)); diff != "" { t.Errorf("validate (-expected, +actual): %s", diff) } @@ -261,7 +359,13 @@ func TestServiceBindingValidate(t *testing.T) { t.Errorf("ValidateCreate (-expected, +actual): %s", diff) } - _, actualUpdateErr := (&ServiceBinding{}).ValidateUpdate(t.Context(), c.seed.DeepCopy(), c.seed.DeepCopy()) + // the old object carries a different .spec.name so that name validation is not + // ratcheted, i.e. these cases assert the rules applied to a newly introduced value. + // Ratcheting itself is covered by TestServiceBindingValidate_RatchetName. + old := c.seed.DeepCopy() + old.Spec.Name = "previous-name" + + _, actualUpdateErr := (&ServiceBinding{}).ValidateUpdate(t.Context(), old, c.seed.DeepCopy()) if diff := cmp.Diff(expectedErr, actualUpdateErr); diff != "" { t.Errorf("ValidateUpdate (-expected, +actual): %s", diff) } @@ -274,6 +378,80 @@ func TestServiceBindingValidate(t *testing.T) { } } +// A ServiceBinding that omits .spec.name is valid: the webhook defaults the field from +// .metadata.name before validating it. Kubernetes constrains .metadata.name to a DNS-1123 +// subdomain, which the binding name pattern always admits, so a defaulted name cannot escape the +// binding root. This is why validating .spec.name at admission is sufficient. +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 + expectErr bool + }{ + {name: "unchanged invalid name is allowed", oldName: "Legacy_Name", newName: "Legacy_Name", expectErr: false}, + {name: "unchanged traversal name is allowed, so it can be deleted", oldName: "..", newName: "..", expectErr: false}, + {name: "changed to another invalid name is rejected", oldName: "Legacy_Name", newName: "Other_Bad", expectErr: true}, + {name: "changed to a valid name is allowed", oldName: "Legacy_Name", newName: "legacy-name", expectErr: false}, + {name: "newly introduced traversal is rejected", oldName: "good-name", newName: "../../etc", expectErr: true}, + {name: "valid name unchanged is allowed", oldName: "good-name", newName: "good-name", expectErr: 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.expectErr && err == nil { + t.Errorf("ValidateUpdate: expected an error, got none") + } + if !c.expectErr && err != nil { + t.Errorf("ValidateUpdate: unexpected error: %s", err) + } + + // creating the same object outright is always validated, never ratcheted + if _, err := (&ServiceBinding{}).ValidateCreate(t.Context(), serviceBindingNamed(c.newName)); err == nil && c.newName != "legacy-name" && c.newName != "good-name" { + t.Errorf("ValidateCreate(%q): expected an error, got none", c.newName) + } + }) + } +} + 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..bea1cc6 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,7 @@ func (*ServiceBinding) ValidateCreate(ctx context.Context, obj *ServiceBinding) log.V(1).Info("Validating Create") (&ServiceBinding{}).Default(ctx, obj) - return nil, obj.validate().ToAggregate() + return nil, obj.validate(nil).ToAggregate() } // ValidateUpdate implements webhook.CustomValidator so a webhook will be registered for the type @@ -78,7 +79,7 @@ func (*ServiceBinding) ValidateUpdate(ctx context.Context, old, obj *ServiceBind } // validate new object - errs = append(errs, obj.validate()...) + errs = append(errs, obj.validate(old)...) return nil, errs.ToAggregate() } @@ -91,19 +92,46 @@ func (*ServiceBinding) ValidateDelete(ctx context.Context, obj *ServiceBinding) return nil, nil } -func (r *ServiceBinding) validate() field.ErrorList { +// validate the ServiceBinding. On update, old is the existing object; on create it is nil. Some +// rules are ratcheted against old so that objects predating a rule are not frozen by it. +func (r *ServiceBinding) validate(old *ServiceBinding) field.ErrorList { errs := field.ErrorList{} - errs = append(errs, r.Spec.validate(field.NewPath("spec"))...) + var oldSpec *ServiceBindingSpec + if old != nil { + oldSpec = &old.Spec + } + errs = append(errs, r.Spec.validate(field.NewPath("spec"), oldSpec)...) return errs } -func (r *ServiceBindingSpec) validate(fldPath *field.Path) field.ErrorList { +// 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}$`) + +func (r *ServiceBindingSpec) validate(fldPath *field.Path, old *ServiceBindingSpec) field.ErrorList { errs := field.ErrorList{} + // the name format is ratcheted: an unchanged value is accepted even if it does not conform, so + // that objects created before this rule existed remain writable. Deletion clears a finalizer, + // which is an update, so rejecting them here would leave them stuck terminating. + nameRatcheted := old != nil && old.Name == r.Name + if r.Name == "" { errs = append(errs, field.Required(fldPath.Child("name"), "")) + } else if nameRatcheted { + // unchanged, accept whatever is already stored + } else if !bindingNameRE.MatchString(r.Name) { + errs = append(errs, field.Invalid(fldPath.Child("name"), 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.Child("name"), r.Name, `must not be "." or ".."`)) } errs = append(errs, r.Service.validate(fldPath.Child("service"))...) errs = append(errs, r.Workload.validate(fldPath.Child("workload"))...) From edca2ef293b4a1640dba71a3ffcc8db61fa549be Mon Sep 17 00:00:00 2001 From: Rashed Kamal Date: Wed, 12 Aug 2026 09:47:55 -0400 Subject: [PATCH 2/3] Check the binding name from the webhook entry points Move the name format check next to the immutable field checks in ValidateUpdate, so the old object no longer needs to be plumbed through validate() and ServiceBindingSpec.validate(). ValidateUpdate applies the check only when the value changes, so that objects predating the rule are not frozen by it: deleting one clears a finalizer, which is an update. Signed-off-by: Rashed Kamal --- apis/v1/servicebinding_test.go | 158 ++++++++++++++++++------------ apis/v1/servicebinding_webhook.go | 58 ++++++----- 2 files changed, 131 insertions(+), 85 deletions(-) diff --git a/apis/v1/servicebinding_test.go b/apis/v1/servicebinding_test.go index 49f3ff0..f5671f0 100644 --- a/apis/v1/servicebinding_test.go +++ b/apis/v1/servicebinding_test.go @@ -267,79 +267,113 @@ func TestServiceBindingValidate(t *testing.T) { field.Required(field.NewPath("spec", "env[1]", "key"), ""), }, }, + } - // .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. + for _, c := range tests { + t.Run(c.name, func(t *testing.T) { + if diff := cmp.Diff(c.expected, c.seed.validate()); diff != "" { + t.Errorf("validate (-expected, +actual): %s", diff) + } + expectedErr := c.expected.ToAggregate() + + _, actualCreateErr := (&ServiceBinding{}).ValidateCreate(t.Context(), c.seed.DeepCopy()) + if diff := cmp.Diff(expectedErr, actualCreateErr); diff != "" { + t.Errorf("ValidateCreate (-expected, +actual): %s", diff) + } + + _, actualUpdateErr := (&ServiceBinding{}).ValidateUpdate(t.Context(), c.seed.DeepCopy(), c.seed.DeepCopy()) + if diff := cmp.Diff(expectedErr, actualUpdateErr); diff != "" { + t.Errorf("ValidateUpdate (-expected, +actual): %s", diff) + } + + _, actualDeleteErr := (&ServiceBinding{}).ValidateDelete(t.Context(), c.seed.DeepCopy()) + if diff := cmp.Diff(nil, actualDeleteErr); diff != "" { + t.Errorf("ValidateDelete (-expected, +actual): %s", diff) + } + }) + } +} + +// .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: "name valid", - seed: serviceBindingNamed("my-binding"), - expected: field.ErrorList{}, + name: "valid", + bindingName: "my-binding", + expected: field.ErrorList{}, }, { - name: "name valid leading hyphen", - seed: serviceBindingNamed("-foo"), - expected: field.ErrorList{}, + name: "valid leading hyphen", + bindingName: "-foo", + expected: field.ErrorList{}, }, { - name: "name valid trailing hyphen", - seed: serviceBindingNamed("foo-"), - expected: field.ErrorList{}, + name: "valid trailing hyphen", + bindingName: "foo-", + expected: field.ErrorList{}, }, { - name: "name valid consecutive dots", - seed: serviceBindingNamed("foo..bar"), - expected: field.ErrorList{}, + name: "valid consecutive dots", + bindingName: "foo..bar", + expected: field.ErrorList{}, }, { - name: "name valid trailing dot", - seed: serviceBindingNamed("foo."), - expected: field.ErrorList{}, + name: "valid trailing dot", + bindingName: "foo.", + expected: field.ErrorList{}, }, { - name: "name valid max length", - seed: serviceBindingNamed(strings.Repeat("a", 253)), - expected: field.ErrorList{}, + name: "valid max length", + bindingName: strings.Repeat("a", 253), + expected: field.ErrorList{}, }, { - name: "name invalid parent directory", - seed: serviceBindingNamed(".."), + name: "invalid parent directory", + bindingName: "..", expected: field.ErrorList{ field.Invalid(field.NewPath("spec", "name"), "..", `must not be "." or ".."`), }, }, { - name: "name invalid current directory", - seed: serviceBindingNamed("."), + name: "invalid current directory", + bindingName: ".", expected: field.ErrorList{ field.Invalid(field.NewPath("spec", "name"), ".", `must not be "." or ".."`), }, }, { - name: "name invalid path traversal", - seed: serviceBindingNamed("../../etc"), + name: "invalid path traversal", + bindingName: "../../etc", expected: field.ErrorList{ field.Invalid(field.NewPath("spec", "name"), "../../etc", bindingNameErrMsg), }, }, { - name: "name invalid uppercase", - seed: serviceBindingNamed("Foo"), + name: "invalid uppercase", + bindingName: "Foo", expected: field.ErrorList{ field.Invalid(field.NewPath("spec", "name"), "Foo", bindingNameErrMsg), }, }, { - name: "name invalid underscore", - seed: serviceBindingNamed("foo_bar"), + name: "invalid underscore", + bindingName: "foo_bar", expected: field.ErrorList{ field.Invalid(field.NewPath("spec", "name"), "foo_bar", bindingNameErrMsg), }, }, { - name: "name invalid too long", - seed: serviceBindingNamed(strings.Repeat("a", 254)), + name: "invalid too long", + bindingName: strings.Repeat("a", 254), expected: field.ErrorList{ field.Invalid(field.NewPath("spec", "name"), strings.Repeat("a", 254), bindingNameErrMsg), }, @@ -348,40 +382,33 @@ func TestServiceBindingValidate(t *testing.T) { for _, c := range tests { t.Run(c.name, func(t *testing.T) { - if diff := cmp.Diff(c.expected, c.seed.validate(nil)); diff != "" { - t.Errorf("validate (-expected, +actual): %s", diff) + 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(), c.seed.DeepCopy()) + _, 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 name validation is not - // ratcheted, i.e. these cases assert the rules applied to a newly introduced value. - // Ratcheting itself is covered by TestServiceBindingValidate_RatchetName. - old := c.seed.DeepCopy() + // 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, c.seed.DeepCopy()) + _, actualUpdateErr := (&ServiceBinding{}).ValidateUpdate(t.Context(), old, seed.DeepCopy()) if diff := cmp.Diff(expectedErr, actualUpdateErr); diff != "" { t.Errorf("ValidateUpdate (-expected, +actual): %s", diff) } - - _, actualDeleteErr := (&ServiceBinding{}).ValidateDelete(t.Context(), c.seed.DeepCopy()) - if diff := cmp.Diff(nil, actualDeleteErr); diff != "" { - t.Errorf("ValidateDelete (-expected, +actual): %s", diff) - } }) } } -// A ServiceBinding that omits .spec.name is valid: the webhook defaults the field from -// .metadata.name before validating it. Kubernetes constrains .metadata.name to a DNS-1123 -// subdomain, which the binding name pattern always admits, so a defaulted name cannot escape the -// binding root. This is why validating .spec.name at admission is sufficient. +// 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 @@ -418,17 +445,18 @@ func TestServiceBindingValidate_DefaultedName(t *testing.T) { // 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 - expectErr bool + name string + oldName string + newName string + expectUpdateErr bool + expectCreateErr bool }{ - {name: "unchanged invalid name is allowed", oldName: "Legacy_Name", newName: "Legacy_Name", expectErr: false}, - {name: "unchanged traversal name is allowed, so it can be deleted", oldName: "..", newName: "..", expectErr: false}, - {name: "changed to another invalid name is rejected", oldName: "Legacy_Name", newName: "Other_Bad", expectErr: true}, - {name: "changed to a valid name is allowed", oldName: "Legacy_Name", newName: "legacy-name", expectErr: false}, - {name: "newly introduced traversal is rejected", oldName: "good-name", newName: "../../etc", expectErr: true}, - {name: "valid name unchanged is allowed", oldName: "good-name", newName: "good-name", expectErr: false}, + {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 { @@ -437,17 +465,21 @@ func TestServiceBindingValidate_RatchetName(t *testing.T) { obj := serviceBindingNamed(c.newName) _, err := (&ServiceBinding{}).ValidateUpdate(t.Context(), old, obj) - if c.expectErr && err == nil { + if c.expectUpdateErr && err == nil { t.Errorf("ValidateUpdate: expected an error, got none") } - if !c.expectErr && err != nil { + if !c.expectUpdateErr && err != nil { t.Errorf("ValidateUpdate: unexpected error: %s", err) } // creating the same object outright is always validated, never ratcheted - if _, err := (&ServiceBinding{}).ValidateCreate(t.Context(), serviceBindingNamed(c.newName)); err == nil && c.newName != "legacy-name" && c.newName != "good-name" { + _, 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) + } }) } } diff --git a/apis/v1/servicebinding_webhook.go b/apis/v1/servicebinding_webhook.go index bea1cc6..bea606b 100644 --- a/apis/v1/servicebinding_webhook.go +++ b/apis/v1/servicebinding_webhook.go @@ -55,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(nil).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 @@ -78,8 +82,16 @@ 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(old)...) + errs = append(errs, obj.validate()...) return nil, errs.ToAggregate() } @@ -92,16 +104,10 @@ func (*ServiceBinding) ValidateDelete(ctx context.Context, obj *ServiceBinding) return nil, nil } -// validate the ServiceBinding. On update, old is the existing object; on create it is nil. Some -// rules are ratcheted against old so that objects predating a rule are not frozen by it. -func (r *ServiceBinding) validate(old *ServiceBinding) field.ErrorList { +func (r *ServiceBinding) validate() field.ErrorList { errs := field.ErrorList{} - var oldSpec *ServiceBindingSpec - if old != nil { - oldSpec = &old.Spec - } - errs = append(errs, r.Spec.validate(field.NewPath("spec"), oldSpec)...) + errs = append(errs, r.Spec.validate(field.NewPath("spec"))...) return errs } @@ -113,25 +119,33 @@ const bindingNameErrMsg = "must consist of lower case alphanumeric characters, ' // bindingNameRE is the binding name pattern required by the specification, anchored. var bindingNameRE = regexp.MustCompile(`^[a-z0-9.-]{1,253}$`) -func (r *ServiceBindingSpec) validate(fldPath *field.Path, old *ServiceBindingSpec) field.ErrorList { +// 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{} - // the name format is ratcheted: an unchanged value is accepted even if it does not conform, so - // that objects created before this rule existed remain writable. Deletion clears a finalizer, - // which is an update, so rejecting them here would leave them stuck terminating. - nameRatcheted := old != nil && old.Name == r.Name - if r.Name == "" { - errs = append(errs, field.Required(fldPath.Child("name"), "")) - } else if nameRatcheted { - // unchanged, accept whatever is already stored - } else if !bindingNameRE.MatchString(r.Name) { - errs = append(errs, field.Invalid(fldPath.Child("name"), r.Name, bindingNameErrMsg)) + 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.Child("name"), r.Name, `must not be "." or ".."`)) + 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{} + + if r.Name == "" { + errs = append(errs, field.Required(fldPath.Child("name"), "")) } errs = append(errs, r.Service.validate(fldPath.Child("service"))...) errs = append(errs, r.Workload.validate(fldPath.Child("workload"))...) From dc8ed841a39416da30ebc0cd78f2aa7d846050c5 Mon Sep 17 00:00:00 2001 From: Scott Andrews Date: Wed, 12 Aug 2026 11:52:50 -0400 Subject: [PATCH 3/3] default old object before comparing defaulted value Signed-off-by: Scott Andrews --- apis/v1/servicebinding_webhook.go | 1 + 1 file changed, 1 insertion(+) diff --git a/apis/v1/servicebinding_webhook.go b/apis/v1/servicebinding_webhook.go index bea606b..e7b68fc 100644 --- a/apis/v1/servicebinding_webhook.go +++ b/apis/v1/servicebinding_webhook.go @@ -67,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{}