validate third party resources #25007

liggitt · 2016-04-30T05:53:22Z

addresses validation portion of #22768

ThirdPartyResource: validates name (3 segment DNS subdomain) and version names (single segment DNS label)
ThirdPartyResourceData: validates objectmeta (name is validated as a DNS label)
removes ability to use GenerateName with thirdpartyresources (kind and api group should not be randomized, in my opinion)

test improvements:

updates resttest to clean up after create tests (so the same valid object can be used)
updates resttest to take a name generator (in case "foo1" isn't a valid name for the object under test)

action required for alpha thirdpartyresource users:

existing thirdpartyresource objects that do not match these validation rules will need to be removed/updated (after removing thirdpartyresourcedata objects stored under the disallowed versions, kind, or group names)
existing thirdpartyresourcedata objects that do not match the name validation rule will not be able to be updated, but can be removed

liggitt · 2016-04-30T05:55:26Z

pkg/apis/extensions/validation/validation.go

@@ -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) {


not sure how tight we should make this... we've never really defined what is allowed for a version. it definitely needs to be single-segment, but not sure beyond that. Pretty sure @thockin would be happy making this tight to begin with.

@lavalamp @caesarxuchao @deads2k @kubernetes/sig-api-machinery opinions on allowed format for an api version?

We definitely need to restrict special separator characters to preserve flexibility on CLI. We could allow dash and slash - not sure we can safely allow anything else.

No slash, versions have to take one URL segment for API server routing sanity

Sorry, I was referring to apiVersion validation

@lavalamp @caesarxuchao @deads2k @kubernetes/sig-api-machinery opinions on allowed format for an api version?

I like the tight rules. Definitely no dots or slashes we'll hose the cli.

Starting with IsDNS1123Label LGTM.

lavalamp · 2016-05-02T18:00:43Z

Hold on-- why should these be cluster scoped?

lavalamp · 2016-05-02T18:02:42Z

Never mind. I see you're not talking about the data objects. I agree the schemas need to be cluster scoped.

liggitt · 2016-05-02T18:02:58Z

cluster scope change is in #25006, see discussion there

deads2k · 2016-05-02T18:03:26Z

Hold on-- why should these be cluster scoped?

When you add a third party resources, they affect the types that are available across the entire cluster, effectively reserving that name.

smarterclayton · 2016-05-03T22:07:10Z

Validation looks ok to me.

thockin · 2016-05-04T00:18:19Z

no red flags from me

liggitt · 2016-05-09T18:26:01Z

/cc @lavalamp @caesarxuchao @deads2k

smarterclayton · 2016-05-09T18:49:08Z

pkg/apis/extensions/validation/validation.go

@@ -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) {


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

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?

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

lavalamp · 2016-05-09T20:42:42Z

this LGTM

k8s-bot · 2016-05-13T14:51:56Z

GCE e2e build/test passed for commit 6c323a4.

k8s-github-robot · 2016-05-15T09:58:19Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2016-05-15T10:36:03Z

GCE e2e build/test passed for commit 6c323a4.

k8s-github-robot · 2016-05-15T10:38:29Z

Automatic merge from submit-queue

Bug 1825915: UPSTREAM: 90985: Set session scanning to manual to avoid discovering all iSCSI devices during login Origin-commit: afc841de5012eb1cb721be2ce6c70ea4c584ae9f

googlebot added the cla: yes label Apr 30, 2016

k8s-github-robot assigned smarterclayton Apr 30, 2016

k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels Apr 30, 2016

liggitt reviewed Apr 30, 2016
View reviewed changes

This was referenced Apr 30, 2016

Requirements for ThirdPartyResource to graduate to beta #22768

Closed

enable third party resources openshift/origin#8622

Closed

liggitt force-pushed the third-party-resource-validation branch from 8ad7e9e to 1bfd234 Compare May 2, 2016 19:48

googlebot added the cla: yes label May 2, 2016

liggitt changed the title ~~WIP - validate third party resources~~ validate third party resources May 4, 2016

liggitt force-pushed the third-party-resource-validation branch from 1bfd234 to a17967f Compare May 7, 2016 03:53

Remove name generation from thirdpartyresource

6c323a4

liggitt force-pushed the third-party-resource-validation branch from a17967f to 6c323a4 Compare May 9, 2016 13:27

smarterclayton reviewed May 9, 2016
View reviewed changes

liggitt added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 9, 2016

k8s-github-robot merged commit 272674b into kubernetes:master May 15, 2016

liggitt deleted the third-party-resource-validation branch May 18, 2016 04:27

erictune mentioned this pull request Jul 2, 2016

Need "action required" notes in CHANGELOG.md #28405

Closed

erictune removed the release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. label Jul 2, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

validate third party resources #25007

validate third party resources #25007

liggitt commented Apr 30, 2016 •

edited

Loading

liggitt Apr 30, 2016 •

edited

Loading

liggitt Apr 30, 2016

smarterclayton Apr 30, 2016 via email

liggitt Apr 30, 2016

smarterclayton Apr 30, 2016 via email

deads2k May 2, 2016

caesarxuchao May 2, 2016

lavalamp commented May 2, 2016

lavalamp commented May 2, 2016

liggitt commented May 2, 2016

deads2k commented May 2, 2016

smarterclayton commented May 3, 2016

thockin commented May 4, 2016

liggitt commented May 9, 2016

smarterclayton May 9, 2016

liggitt May 9, 2016

smarterclayton May 9, 2016

lavalamp commented May 9, 2016

k8s-bot commented May 13, 2016

k8s-github-robot commented May 15, 2016

k8s-bot commented May 15, 2016

k8s-github-robot commented May 15, 2016

validate third party resources #25007

validate third party resources #25007

Conversation

liggitt commented Apr 30, 2016 • edited Loading

liggitt Apr 30, 2016 • edited Loading

Choose a reason for hiding this comment

liggitt Apr 30, 2016

Choose a reason for hiding this comment

smarterclayton Apr 30, 2016 via email

Choose a reason for hiding this comment

liggitt Apr 30, 2016

Choose a reason for hiding this comment

smarterclayton Apr 30, 2016 via email

Choose a reason for hiding this comment

deads2k May 2, 2016

Choose a reason for hiding this comment

caesarxuchao May 2, 2016

Choose a reason for hiding this comment

lavalamp commented May 2, 2016

lavalamp commented May 2, 2016

liggitt commented May 2, 2016

deads2k commented May 2, 2016

smarterclayton commented May 3, 2016

thockin commented May 4, 2016

liggitt commented May 9, 2016

smarterclayton May 9, 2016

Choose a reason for hiding this comment

liggitt May 9, 2016

Choose a reason for hiding this comment

smarterclayton May 9, 2016

Choose a reason for hiding this comment

lavalamp commented May 9, 2016

k8s-bot commented May 13, 2016

k8s-github-robot commented May 15, 2016

k8s-bot commented May 15, 2016

k8s-github-robot commented May 15, 2016

liggitt commented Apr 30, 2016 •

edited

Loading

liggitt Apr 30, 2016 •

edited

Loading