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.

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

Separate validation concern from rest of RESTStorage #2977

Closed
derekwaynecarr opened this issue Dec 16, 2014 · 7 comments
Closed

Separate validation concern from rest of RESTStorage #2977

derekwaynecarr opened this issue Dec 16, 2014 · 7 comments

Comments

@derekwaynecarr
Copy link
Member

@derekwaynecarr derekwaynecarr commented Dec 16, 2014

The majority of unique code per resource type in our RESTStorage implementations deals with populating default values on an object and validating the object prior to persistence. The other 99% of the code is pretty common across all of our resource types and could be generalized. When we look at our tests, the majority of our rest_test.go files are just ensuring that validation occurs prior to create/update. There is a lot of duplicate effort where we could just mandate that the framework that calls the RESTStorage guarantees that the object is valid.

I would like to suggest that the resthandler.go logic enforces that prior to calling a RESTStorage Create and Update operation, it makes a call to something like the following:

type Validator interface {
  // ValidateCreate will populate default values on the object, and ensure its valid for persistence
  ValidateCreate(ctx api.Context, obj runtime.Object) error
  // ValidateUpdate will validate the newObj and copy values required from oldObject prior to persistence
ValidateUpdate(ctx api.Context, newObj runtime.Object, oldObj runtime.Object) error
}

In addition, as we look to the future, there are some additional concepts that I would like the resthandler.go to enforce uniformly across all resources without adding more boilerplate into each RESTStorage implementation. Specifically, I do not want to invoke any Admission control style logic for invalid input, but I do not want to modify each RESTStorage implementation to have to make an explicit call-out to perform admission control as it just increases more boiler plate code.

@derekwaynecarr
Copy link
Member Author

@derekwaynecarr derekwaynecarr commented Dec 16, 2014

@smarterclayton @lavalamp @liggitt - this is related to the topic we discussed at meet-up on simplifying the RESTStorage tier. I think centralizing call-outs for object validation has huge benefits and will let us write more focused tests. Let me know your thoughts.

@smarterclayton
Copy link
Contributor

@smarterclayton smarterclayton commented Dec 16, 2014

You used the four letter b-word

On Dec 16, 2014, at 3:40 PM, Derek Carr notifications@github.com wrote:

@smarterclayton @lavalamp @liggitt - this is related to the topic we discussed at meet-up on simplifying the RESTStorage tier. I think centralizing call-outs for bean validation has huge benefits and will let us write more focused tests. Let me know your thoughts.


Reply to this email directly or view it on GitHub.

@derekwaynecarr
Copy link
Member Author

@derekwaynecarr derekwaynecarr commented Dec 16, 2014

@smarterclayton - github update comment let's me fix my mistake ;-)

@liggitt
Copy link
Member

@liggitt liggitt commented Dec 16, 2014

"Boilerplate" has far more than four letters :)

@bgrant0607
Copy link
Member

@bgrant0607 bgrant0607 commented Dec 17, 2014

Yes, the apiserver and RESTStorage implementations should be generic.

@smarterclayton
Copy link
Contributor

@smarterclayton smarterclayton commented Mar 4, 2015

This is now possible, when people switch to genericetcd they must implement a strategy (which hides this)

@derekwaynecarr
Copy link
Member Author

@derekwaynecarr derekwaynecarr commented Mar 20, 2015

Closing this issue because I agree it's now possible. Thanks @smarterclayton

seans3 pushed a commit to seans3/kubernetes that referenced this issue Apr 10, 2019
KEP: Create a `k8s.io/component` repo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.