Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

validate third party resources #25007

Merged
merged 1 commit into from
May 15, 2016
Merged

validate third party resources #25007

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
87 changes: 60 additions & 27 deletions pkg/api/rest/resttest/resttest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,15 +43,28 @@ type Tester struct {
createOnUpdate bool
generatesName bool
returnDeletedObject bool
namer func(int) string
}

func New(t *testing.T, storage rest.Storage) *Tester {
return &Tester{
T: t,
storage: storage,
namer: defaultNamer,
}
}

func defaultNamer(i int) string {
return fmt.Sprintf("foo%d", i)
}

// Namer allows providing a custom name maker
// By default "foo%d" is used
func (t *Tester) Namer(namer func(int) string) *Tester {
t.namer = namer
return t
}

func (t *Tester) ClusterScope() *Tester {
t.clusterScope = true
return t
Expand Down Expand Up @@ -202,14 +215,28 @@ func (t *Tester) TestWatch(
// =============================================================================
// Creation tests.

func (t *Tester) delete(ctx api.Context, obj runtime.Object) error {
objectMeta, err := api.ObjectMetaFor(obj)
if err != nil {
return err
}
deleter, ok := t.storage.(rest.GracefulDeleter)
if !ok {
return fmt.Errorf("Expected deleting storage, got %v", t.storage)
}
_, err = deleter.Delete(ctx, objectMeta.Name, nil)
return err
}

func (t *Tester) testCreateAlreadyExisting(obj runtime.Object, createFn CreateFunc) {
ctx := t.TestContext()

foo := copyOrDie(obj)
t.setObjectMeta(foo, "foo1")
t.setObjectMeta(foo, t.namer(1))
if err := createFn(ctx, foo); err != nil {
t.Errorf("unexpected error: %v", err)
}
defer t.delete(ctx, foo)

_, err := t.storage.(rest.Creater).Create(ctx, foo)
if !errors.IsAlreadyExists(err) {
Expand All @@ -221,12 +248,13 @@ func (t *Tester) testCreateEquals(obj runtime.Object, getFn GetFunc) {
ctx := t.TestContext()

foo := copyOrDie(obj)
t.setObjectMeta(foo, "foo2")
t.setObjectMeta(foo, t.namer(2))

created, err := t.storage.(rest.Creater).Create(ctx, foo)
if err != nil {
t.Errorf("unexpected error: %v", err)
}
defer t.delete(ctx, created)

got, err := getFn(ctx, foo)
if err != nil {
Expand Down Expand Up @@ -254,6 +282,7 @@ func (t *Tester) testCreateDiscardsObjectNamespace(valid runtime.Object) {
if err != nil {
t.Fatalf("Unexpected error: %v", err)
}
defer t.delete(t.TestContext(), created)
createdObjectMeta := t.getObjectMetaOrFail(created)
if createdObjectMeta.Namespace != api.NamespaceNone {
t.Errorf("Expected empty namespace on created object, got '%v'", createdObjectMeta.Namespace)
Expand All @@ -265,19 +294,19 @@ func (t *Tester) testCreateGeneratesName(valid runtime.Object) {
objectMeta.Name = ""
objectMeta.GenerateName = "test-"

_, err := t.storage.(rest.Creater).Create(t.TestContext(), valid)
created, err := t.storage.(rest.Creater).Create(t.TestContext(), valid)
if err != nil {
t.Fatalf("Unexpected error: %v", err)
}
defer t.delete(t.TestContext(), created)
if objectMeta.Name == "test-" || !strings.HasPrefix(objectMeta.Name, "test-") {
t.Errorf("unexpected name: %#v", valid)
}
}

func (t *Tester) testCreateHasMetadata(valid runtime.Object) {
objectMeta := t.getObjectMetaOrFail(valid)
objectMeta.Name = ""
objectMeta.GenerateName = "test-"
objectMeta.Name = t.namer(1)
objectMeta.Namespace = t.TestNamespace()

obj, err := t.storage.(rest.Creater).Create(t.TestContext(), valid)
Expand All @@ -287,6 +316,7 @@ func (t *Tester) testCreateHasMetadata(valid runtime.Object) {
if obj == nil {
t.Fatalf("Unexpected object from result: %#v", obj)
}
defer t.delete(t.TestContext(), obj)
if !api.HasObjectMetaSystemFieldValues(objectMeta) {
t.Errorf("storage did not populate object meta field values")
}
Expand All @@ -301,6 +331,7 @@ func (t *Tester) testCreateIgnoresContextNamespace(valid runtime.Object) {
if err != nil {
t.Fatalf("Unexpected error: %v", err)
}
defer t.delete(ctx, created)
createdObjectMeta := t.getObjectMetaOrFail(created)
if createdObjectMeta.Namespace != api.NamespaceNone {
t.Errorf("Expected empty namespace on created object, got '%v'", createdObjectMeta.Namespace)
Expand All @@ -319,6 +350,7 @@ func (t *Tester) testCreateIgnoresMismatchedNamespace(valid runtime.Object) {
if err != nil {
t.Fatalf("Unexpected error: %v", err)
}
defer t.delete(ctx, created)
createdObjectMeta := t.getObjectMetaOrFail(created)
if createdObjectMeta.Namespace != api.NamespaceNone {
t.Errorf("Expected empty namespace on created object, got '%v'", createdObjectMeta.Namespace)
Expand Down Expand Up @@ -386,6 +418,7 @@ func (t *Tester) testCreateResetsUserData(valid runtime.Object) {
if obj == nil {
t.Fatalf("Unexpected object from result: %#v", obj)
}
defer t.delete(t.TestContext(), obj)
if objectMeta.UID == "bad-uid" || objectMeta.CreationTimestamp == now {
t.Errorf("ObjectMeta did not reset basic fields: %#v", objectMeta)
}
Expand All @@ -398,7 +431,7 @@ func (t *Tester) testUpdateEquals(obj runtime.Object, createFn CreateFunc, getFn
ctx := t.TestContext()

foo := copyOrDie(obj)
t.setObjectMeta(foo, "foo2")
t.setObjectMeta(foo, t.namer(2))
if err := createFn(ctx, foo); err != nil {
t.Errorf("unexpected error: %v", err)
}
Expand Down Expand Up @@ -433,7 +466,7 @@ func (t *Tester) testUpdateFailsOnVersionTooOld(obj runtime.Object, createFn Cre
ctx := t.TestContext()

foo := copyOrDie(obj)
t.setObjectMeta(foo, "foo3")
t.setObjectMeta(foo, t.namer(3))

if err := createFn(ctx, foo); err != nil {
t.Errorf("unexpected error: %v", err)
Expand All @@ -460,7 +493,7 @@ func (t *Tester) testUpdateInvokesValidation(obj runtime.Object, createFn Create
ctx := t.TestContext()

foo := copyOrDie(obj)
t.setObjectMeta(foo, "foo4")
t.setObjectMeta(foo, t.namer(4))
if err := createFn(ctx, foo); err != nil {
t.Errorf("unexpected error: %v", err)
}
Expand All @@ -480,7 +513,7 @@ func (t *Tester) testUpdateInvokesValidation(obj runtime.Object, createFn Create
func (t *Tester) testUpdateWithWrongUID(obj runtime.Object, createFn CreateFunc, getFn GetFunc) {
ctx := t.TestContext()
foo := copyOrDie(obj)
t.setObjectMeta(foo, "foo5")
t.setObjectMeta(foo, t.namer(5))
objectMeta := t.getObjectMetaOrFail(foo)
objectMeta.UID = types.UID("UID0000")
if err := createFn(ctx, foo); err != nil {
Expand All @@ -498,7 +531,7 @@ func (t *Tester) testUpdateWithWrongUID(obj runtime.Object, createFn CreateFunc,
}

func (t *Tester) testUpdateOnNotFound(obj runtime.Object) {
t.setObjectMeta(obj, "foo")
t.setObjectMeta(obj, t.namer(0))
_, created, err := t.storage.(rest.Updater).Update(t.TestContext(), obj)
if t.createOnUpdate {
if err != nil {
Expand All @@ -520,13 +553,13 @@ func (t *Tester) testUpdateRejectsMismatchedNamespace(obj runtime.Object, create
ctx := t.TestContext()

foo := copyOrDie(obj)
t.setObjectMeta(foo, "foo1")
t.setObjectMeta(foo, t.namer(1))
if err := createFn(ctx, foo); err != nil {
t.Errorf("unexpected error: %v", err)
}

objectMeta := t.getObjectMetaOrFail(obj)
objectMeta.Name = "foo1"
objectMeta.Name = t.namer(1)
objectMeta.Namespace = "not-default"

obj, updated, err := t.storage.(rest.Updater).Update(t.TestContext(), obj)
Expand All @@ -547,7 +580,7 @@ func (t *Tester) testDeleteNoGraceful(obj runtime.Object, createFn CreateFunc, g
ctx := t.TestContext()

foo := copyOrDie(obj)
t.setObjectMeta(foo, "foo1")
t.setObjectMeta(foo, t.namer(1))
if err := createFn(ctx, foo); err != nil {
t.Errorf("unexpected error: %v", err)
}
Expand Down Expand Up @@ -586,7 +619,7 @@ func (t *Tester) testDeleteWithUID(obj runtime.Object, createFn CreateFunc, getF
ctx := t.TestContext()

foo := copyOrDie(obj)
t.setObjectMeta(foo, "foo1")
t.setObjectMeta(foo, t.namer(1))
objectMeta := t.getObjectMetaOrFail(foo)
objectMeta.UID = types.UID("UID0000")
if err := createFn(ctx, foo); err != nil {
Expand Down Expand Up @@ -623,7 +656,7 @@ func (t *Tester) testDeleteGracefulHasDefault(obj runtime.Object, createFn Creat
ctx := t.TestContext()

foo := copyOrDie(obj)
t.setObjectMeta(foo, "foo1")
t.setObjectMeta(foo, t.namer(1))
if err := createFn(ctx, foo); err != nil {
t.Errorf("unexpected error: %v", err)
}
Expand All @@ -650,7 +683,7 @@ func (t *Tester) testDeleteGracefulWithValue(obj runtime.Object, createFn Create
ctx := t.TestContext()

foo := copyOrDie(obj)
t.setObjectMeta(foo, "foo2")
t.setObjectMeta(foo, t.namer(2))
if err := createFn(ctx, foo); err != nil {
t.Errorf("unexpected error: %v", err)
}
Expand All @@ -677,7 +710,7 @@ func (t *Tester) testDeleteGracefulExtend(obj runtime.Object, createFn CreateFun
ctx := t.TestContext()

foo := copyOrDie(obj)
t.setObjectMeta(foo, "foo3")
t.setObjectMeta(foo, t.namer(3))
if err := createFn(ctx, foo); err != nil {
t.Errorf("unexpected error: %v", err)
}
Expand Down Expand Up @@ -742,7 +775,7 @@ func (t *Tester) testDeleteGracefulUsesZeroOnNil(obj runtime.Object, createFn Cr
ctx := t.TestContext()

foo := copyOrDie(obj)
t.setObjectMeta(foo, "foo5")
t.setObjectMeta(foo, t.namer(5))
if err := createFn(ctx, foo); err != nil {
t.Errorf("unexpected error: %v", err)
}
Expand All @@ -766,7 +799,7 @@ func (t *Tester) testGetDifferentNamespace(obj runtime.Object) {
}

objMeta := t.getObjectMetaOrFail(obj)
objMeta.Name = "foo5"
objMeta.Name = t.namer(5)

ctx1 := api.WithNamespace(api.NewContext(), "bar3")
objMeta.Namespace = api.NamespaceValue(ctx1)
Expand Down Expand Up @@ -809,15 +842,15 @@ func (t *Tester) testGetDifferentNamespace(obj runtime.Object) {

func (t *Tester) testGetFound(obj runtime.Object) {
ctx := t.TestContext()
t.setObjectMeta(obj, "foo1")
t.setObjectMeta(obj, t.namer(1))

existing, err := t.storage.(rest.Creater).Create(ctx, obj)
if err != nil {
t.Errorf("unexpected error: %v", err)
}
existingMeta := t.getObjectMetaOrFail(existing)

got, err := t.storage.(rest.Getter).Get(ctx, "foo1")
got, err := t.storage.(rest.Getter).Get(ctx, t.namer(1))
if err != nil {
t.Errorf("unexpected error: %v", err)
}
Expand All @@ -832,13 +865,13 @@ func (t *Tester) testGetMimatchedNamespace(obj runtime.Object) {
ctx1 := api.WithNamespace(api.NewContext(), "bar1")
ctx2 := api.WithNamespace(api.NewContext(), "bar2")
objMeta := t.getObjectMetaOrFail(obj)
objMeta.Name = "foo4"
objMeta.Name = t.namer(4)
objMeta.Namespace = api.NamespaceValue(ctx1)
_, err := t.storage.(rest.Creater).Create(ctx1, obj)
if err != nil {
t.Errorf("unexpected error: %v", err)
}
_, err = t.storage.(rest.Getter).Get(ctx2, "foo4")
_, err = t.storage.(rest.Getter).Get(ctx2, t.namer(4))
if t.clusterScope {
if err != nil {
t.Errorf("unexpected error: %v", err)
Expand All @@ -852,12 +885,12 @@ func (t *Tester) testGetMimatchedNamespace(obj runtime.Object) {

func (t *Tester) testGetNotFound(obj runtime.Object) {
ctx := t.TestContext()
t.setObjectMeta(obj, "foo2")
t.setObjectMeta(obj, t.namer(2))
_, err := t.storage.(rest.Creater).Create(ctx, obj)
if err != nil {
t.Errorf("unexpected error: %v", err)
}
_, err = t.storage.(rest.Getter).Get(ctx, "foo3")
_, err = t.storage.(rest.Getter).Get(ctx, t.namer(3))
if !errors.IsNotFound(err) {
t.Errorf("unexpected error returned: %#v", err)
}
Expand Down Expand Up @@ -889,9 +922,9 @@ func (t *Tester) testListFound(obj runtime.Object, assignFn AssignFunc) {
ctx := t.TestContext()

foo1 := copyOrDie(obj)
t.setObjectMeta(foo1, "foo1")
t.setObjectMeta(foo1, t.namer(1))
foo2 := copyOrDie(obj)
t.setObjectMeta(foo2, "foo2")
t.setObjectMeta(foo2, t.namer(2))

existing := assignFn([]runtime.Object{foo1, foo2})

Expand Down
25 changes: 18 additions & 7 deletions pkg/apis/extensions/validation/validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,20 @@ func ValidateThirdPartyResourceUpdate(update, old *extensions.ThirdPartyResource
}

func ValidateThirdPartyResourceName(name string, prefix bool) (bool, string) {
return apivalidation.NameIsDNSSubdomain(name, prefix)
// Make sure it's a valid DNS subdomain
if ok, msg := apivalidation.NameIsDNSSubdomain(name, prefix); !ok {
return ok, msg
}

// Make sure it's at least three segments (kind + two-segment group name)
if !prefix {
parts := strings.Split(name, ".")
if len(parts) < 3 {
return false, "must be at least three segments long: <kind>.<domain>.<tld>"
}
}

return true, ""
}

func ValidateThirdPartyResource(obj *extensions.ThirdPartyResource) field.ErrorList {
Expand All @@ -152,6 +165,8 @@ func ValidateThirdPartyResource(obj *extensions.ThirdPartyResource) field.ErrorL
version := &obj.Versions[ix]
if len(version.Name) == 0 {
allErrs = append(allErrs, field.Invalid(field.NewPath("versions").Index(ix).Child("name"), version, "must not be empty"))
} else if !validation.IsDNS1123Label(version.Name) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very restrictive, what if i want a third party resource that has an underscore in the name? Or a colon?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I'm not sure what the right line to draw is. if there was a way for thirdpartyresources to specify what name formats they allow, that would be better, but in the absence of that, how much should we protect users of this API from bad names? see discussion at #25007 (diff)

@liggitt: should we make the name validation tight (dns label or subdomain, etc) or minimal (anything we can represent as a URL path segment)?

@deads2k: Without actual hooks to allow them to shape their names, I think forcing the label is reasonable.

@liggitt: you don't think most API consumers would be equipped to handle resources named İ ♥ \n?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok with starting tight. But it also encourages clients to make
assumptions "oh, third party is label, I'll just mix in some colons and
underscores and everything will be safe". No objection, otherwise LGTM.

On Mon, May 9, 2016 at 2:55 PM, Jordan Liggitt notifications@github.com
wrote:

In pkg/apis/extensions/validation/validation.go
#25007 (comment)
:

@@ -152,6 +165,8 @@ func ValidateThirdPartyResource(obj *extensions.ThirdPartyResource) field.ErrorL
version := &obj.Versions[ix]
if len(version.Name) == 0 {
allErrs = append(allErrs, field.Invalid(field.NewPath("versions").Index(ix).Child("name"), version, "must not be empty"))

  •   } else if !validation.IsDNS1123Label(version.Name) {

yeah, I'm not sure what the right line to draw is. if there was a way for
thirdpartyresources to specify what name formats they allow, that would be
better, but in the absence of that, how much should we protect users of
this API from bad names? see discussion at #25007 (diff)
#25007 (diff)

@liggitt https://github.com/liggitt: should we make the name validation
tight (dns label or subdomain, etc) or minimal (anything we can represent
as a URL path segment)?

@deads2k https://github.com/deads2k: Without actual hooks to allow them
to shape their names, I think forcing the label is reasonable.

@liggitt https://github.com/liggitt: you don't think most API consumers
would be equipped to handle resources named İ ♥ \n?

—
You are receiving this because you were assigned.
Reply to this email directly or view it on GitHub
https://github.com/kubernetes/kubernetes/pull/25007/files/6c323a4f723cbcb1a11d12edf1c81b8ed908057d#r62552880

allErrs = append(allErrs, field.Invalid(field.NewPath("versions").Index(ix).Child("name"), version, apivalidation.DNS1123LabelErrorMsg))
}
if versions.Has(version.Name) {
allErrs = append(allErrs, field.Duplicate(field.NewPath("versions").Index(ix).Child("name"), version))
Expand Down Expand Up @@ -371,15 +386,11 @@ func ValidateDeploymentRollback(obj *extensions.DeploymentRollback) field.ErrorL
}

func ValidateThirdPartyResourceDataUpdate(update, old *extensions.ThirdPartyResourceData) field.ErrorList {
return ValidateThirdPartyResourceData(update)
return apivalidation.ValidateObjectMetaUpdate(&update.ObjectMeta, &old.ObjectMeta, field.NewPath("metadata"))
}

func ValidateThirdPartyResourceData(obj *extensions.ThirdPartyResourceData) field.ErrorList {
allErrs := field.ErrorList{}
if len(obj.Name) == 0 {
allErrs = append(allErrs, field.Required(field.NewPath("name"), ""))
}
return allErrs
return apivalidation.ValidateObjectMeta(&obj.ObjectMeta, true, apivalidation.NameIsDNSLabel, field.NewPath("metadata"))
}

// ValidateIngress tests if required fields in the Ingress are set.
Expand Down
5 changes: 5 additions & 0 deletions pkg/registry/registrytest/etcd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,11 @@ func (t *Tester) ClusterScope() *Tester {
return t
}

func (t *Tester) Namer(namer func(int) string) *Tester {
t.tester = t.tester.Namer(namer)
return t
}

func (t *Tester) AllowCreateOnUpdate() *Tester {
t.tester = t.tester.AllowCreateOnUpdate()
return t
Expand Down