add easy pattern for customresource validation #21679

deads2k · 2018-12-17T21:33:38Z

This makes an easy pattern to follow and provide CR validation functions. I'm thinking about our config APIs in particular. This isn't totally pure, but it does solve our ordering issue, makes it easy for teams to add validation, provides for immutable fields, and could maybe be added as a webhook later if we decided against this.

@derekwaynecarr @smarterclayton we've discussed the need for validation
@openshift/sig-master think of something better or we'll have to hold our noses :)

openshift-ci-robot · 2018-12-17T21:34:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~pkg/OWNERS~~ [deads2k]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

deads2k · 2018-12-17T22:57:49Z

/retest

enj · 2018-12-20T21:47:02Z

@openshift/sig-auth

I think I would take anything over no validation.

deads2k · 2019-01-03T21:13:21Z

@sanchezl I could use some help with unit tests here and this is a construct you're likely to need to be familiar with

deads2k · 2019-01-03T21:21:12Z

@sanchezl I could use some help with unit tests here and this is a construct you're likely to need to be familiar with

After the test I think we're ready to move on this.

smarterclayton · 2019-01-04T00:09:05Z

pkg/admission/customresourcevalidation/image/validate_image.go

+// Register registers a plugin
+func Register(plugins *admission.Plugins) {
+	plugins.Register("config.openshift.io/ValidateImage", func(config io.Reader) (admission.Interface, error) {
+		return customresourcevalidation.NewValidator(configv1.Resource("images"), validateObjCreateFn, validateObjUpdateFn, validateObjStatusUpdateFn)


Would be better as an interface rather than passing raw functions around, unless you're going to expect people to define these inline.

Would be better as an interface rather than passing raw functions around, unless you're going to expect people to define these inline.

Yeah, and people probably want/need a slice of them to handle versions.

smarterclayton · 2019-01-04T00:12:37Z

pkg/admission/customresourcevalidation/image/validate_image.go

+}
+
+func validateObjStatusUpdateFn(uncastObj runtime.Object, uncastOldObj runtime.Object) field.ErrorList {
+	allErrs := field.ErrorList{}


If you're only going to call these functions once per type, this is very boilerplate-y and could be made much simpler by just doing the conversion yourself here with a helper that is shared.

by just doing the conversion yourself here with a helper that is shared.

That would make these functions significantly different than "normal" validation functions and doesn't help with the boilerplate since the same defense against errors is needed

smarterclayton · 2019-01-04T00:15:20Z

pkg/admission/customresourcevalidation/attributes.go

+}
+
+// toBestObjectPossible tries to convert to a real object in the config scheme
+func toBestObjectPossible(orig runtime.Object) runtime.Object {


This function really could just be a shared helper that takes the unstructured and the into and then does the type check, verifies the config scheme allows into, converts, and optionally returns invalid errors. That bypasses the cast and make this much easier to read.

var image *configv1.Image if errs := ConvertFromUnstructured(obj, image); len(errs) > 0 { return errs } ...

instead of having to do the very ugly cast.

This function really could just be a shared helper that takes the unstructured and the into

The into isn't known, that's part of what this function is figuring out. If it gets specified then I have reflection trying to clone it. Also the unstructured has to be cast somewhere.

deads2k · 2019-01-07T13:48:42Z

pkg/admission/customresourcevalidation/image/validate_image.go

+// Register registers a plugin
+func Register(plugins *admission.Plugins) {
+	plugins.Register("config.openshift.io/ValidateImage", func(config io.Reader) (admission.Interface, error) {
+		return customresourcevalidation.NewValidator(configv1.Resource("images"), validateObjCreateFn, validateObjUpdateFn, validateObjStatusUpdateFn)


this needs to be on the group resource, not the group version resource

sanchezl · 2019-01-07T16:03:51Z

pkg/admission/customresourcevalidation/customresourcevalidator.go

+			return apierrors.NewInvalid(attributes.GetKind().GroupKind(), attributes.GetName(), errors)
+
+		default:
+			admission.NewForbidden(attributes, fmt.Errorf("unhandled subresource: %v", attributes.GetSubresource()))


Should this error (and the other Forbidden error below) not be returned?

Should this error (and the other Forbidden error below) not be returned?

bug.

For this specific line, should we not just ignore (i.e. return nil), to be consistent with shouldIgnore()?

deads2k · 2019-01-08T17:57:40Z

/assign @sttts

ptal

sttts · 2019-01-11T14:45:06Z

.../customresourcevalidation/customresourcevalidationregistration/cr_validation_registration.go

+	"k8s.io/apiserver/pkg/admission"
+)
+
+var AllCustomResourceValidators = []string{


godoc. what are these strings?

sttts · 2019-01-11T14:46:46Z

pkg/admission/customresourcevalidation/customresourcevalidator.go

+
+var _ admission.ValidationInterface = &validateCustomResource{}
+
+func (a *validateCustomResource) Validate(uncastAttributes admission.Attributes) error {


Godoc. What ae uncastAttributes? Attributes with unstructured old+new object inside?

sttts · 2019-01-11T14:48:22Z

pkg/admission/customresourcevalidation/attributes.go

+)
+
+// unstructuredUnpackingAttributes tries to convert to a real object in the config scheme
+type unstructuredUnpackingAttributes struct {


why this wrapper struct? An explicit conversion in the admission handler would be much simpler. No need for on-the-fly conversion.

why this wrapper struct? An explicit conversion in the admission handler would be much simpler. No need for on-the-fly conversion.

We don't know whether or not we can until point of use. Also, point of use makes sure that that we don't miss a spot.

sttts · 2019-01-11T14:49:25Z

pkg/admission/customresourcevalidation/customresourcevalidator.go

+	"k8s.io/apiserver/pkg/admission"
+)
+
+func RequireNameCluster(name string, prefix bool) []string {


what is this? A helper? Mover to an helper.go file.

sttts · 2019-01-11T14:49:44Z

pkg/admission/customresourcevalidation/image/validate_image.go

+	configv1 "github.com/openshift/api/config/v1"
+
+	"github.com/openshift/origin/pkg/admission/customresourcevalidation"
+	"k8s.io/apimachinery/pkg/api/validation"


sttts · 2019-01-11T14:53:32Z

pkg/admission/customresourcevalidation/image/validate_image.go

+	})
+}
+
+func toImageV1(uncastObj runtime.Object) (*configv1.Image, field.ErrorList) {


do we need this boilerplate in every validation? Can't we check in a generic way in the framework code, if necessary by passing the target kind there.

do we need this boilerplate in every validation? Can't we check in a generic way in the framework code, if necessary by passing the target kind there.

Probably via reflection. I'm open to future refinement.

Probably via reflection. I'm open to future refinement.

you have to use reflection for the actual method calls

sttts · 2019-01-11T14:54:02Z

pkg/admission/customresourcevalidation/image/validate_image.go

+}
+
+func (imageV1) ValidateUpdate(uncastObj runtime.Object, uncastOldObj runtime.Object) field.ErrorList {
+	obj, castErrors := toImageV1(uncastObj)


s/castErrors/errs/

sttts · 2019-01-11T14:54:10Z

pkg/admission/customresourcevalidation/image/validate_image.go

+		return castErrors
+	}
+
+	allErrs := field.ErrorList{}


deads2k · 2019-01-11T20:33:49Z

comments addressed. Some reflective refinement is possible, but I don't see it as blocking.

openshift-bot · 2019-01-13T09:17:40Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot · 2019-01-13T11:58:30Z

@deads2k: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/openshift-jenkins/end_to_end	`ef05639`	link	`/test end_to_end`
ci/prow/e2e-gcp-serial	`ef05639`	link	`/test e2e-gcp-serial`
ci/prow/e2e-gcp	`ef05639`	link	`/test e2e-gcp`
ci/prow/launch-aws	`ef05639`	link	`/test launch-aws`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

openshift-bot · 2019-01-13T13:19:38Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. sig/master size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 17, 2018

openshift-ci-robot requested review from bparees and knobunc December 17, 2018 21:34

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 17, 2018

deads2k mentioned this pull request Dec 17, 2018

Add cloud provider name to infra config openshift/api#145

Merged

deads2k force-pushed the cr-validation branch from ef05639 to 7907dcd Compare January 3, 2019 21:10

smarterclayton reviewed Jan 4, 2019

View reviewed changes

deads2k commented Jan 7, 2019

View reviewed changes

sanchezl reviewed Jan 7, 2019

View reviewed changes

openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 8, 2019

sanchezl force-pushed the cr-validation branch from 59ea688 to 7907dcd Compare January 8, 2019 14:44

deads2k force-pushed the cr-validation branch 2 times, most recently from bc9a508 to fe22771 Compare January 8, 2019 16:22

deads2k changed the title ~~[WIP] add easy pattern for customresource validation~~ add easy pattern for customresource validation Jan 8, 2019

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 8, 2019

add easy pattern for customresource validation

40489fe

return error

e5b2a05

deads2k force-pushed the cr-validation branch from fe22771 to f307440 Compare January 8, 2019 17:57

openshift-ci-robot assigned sttts Jan 8, 2019

sttts reviewed Jan 11, 2019

View reviewed changes

pkg/admission/customresourcevalidation/image/validate_image.go Outdated

return castErrors

}

allErrs := field.ErrorList{}

Copy link

Contributor

sttts Jan 11, 2019

Choose a reason for hiding this comment

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

errs

add customresourcevalidator_test

738fa31

deads2k force-pushed the cr-validation branch from f307440 to 738fa31 Compare January 11, 2019 20:32

deads2k added the lgtm Indicates that a PR is ready to be merged. label Jan 11, 2019

openshift-merge-robot merged commit 2f67e56 into openshift:master Jan 13, 2019


		var _ admission.ValidationInterface = &validateCustomResource{}

		func (a *validateCustomResource) Validate(uncastAttributes admission.Attributes) error {

add easy pattern for customresource validation #21679

add easy pattern for customresource validation #21679

Uh oh!

Conversation

deads2k commented Dec 17, 2018

Uh oh!

openshift-ci-robot commented Dec 17, 2018

Uh oh!

deads2k commented Dec 17, 2018

Uh oh!

enj commented Dec 20, 2018

Uh oh!

deads2k commented Jan 3, 2019

Uh oh!

deads2k commented Jan 3, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smarterclayton Jan 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deads2k commented Jan 8, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deads2k commented Jan 11, 2019

Uh oh!

openshift-bot commented Jan 13, 2019

Uh oh!

openshift-ci-robot commented Jan 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-bot commented Jan 13, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

smarterclayton Jan 4, 2019 •

edited

Loading

openshift-ci-robot commented Jan 13, 2019 •

edited

Loading