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

Add simple validation for BackendConfig using OpenAPI schema generation #272

Merged

Conversation

rramkumar1
Copy link
Contributor

@rramkumar1 rramkumar1 commented May 16, 2018

  1. Update codegen scripts to include generation of OpenAPI schema for BackendConfig

  2. Use the generated schema to construct a validation spec which is used to initialize the CRD. If everything works right, when a CR is pushed, the validation spec will kick in and make sure the spec is valid based on the types we have defined for the BackendConfig (pkg/apis/backendconfig/v1beta1/types.go).

  3. Refactor the crd code to be more generic by introducing a CRDMeta type which stores all information needed to build a CRD.

FYI: One of the commits is a vendor update and one is a patch to generated code.

/assign @MrHohn

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 16, 2018
@rramkumar1 rramkumar1 force-pushed the backend-config-schema-validation branch 2 times, most recently from 159cd42 to bd136cc Compare May 16, 2018 05:04
*ref = schemaProps.Ref.String()
}

props = &extensionsobj.JSONSchemaProps{
Copy link
Member

Choose a reason for hiding this comment

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

Oh interesting...Didn't expect it will look like this in codes :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Surprisingly, this is the only way currently to generate the validation spec in code given go structs.

@rramkumar1 rramkumar1 force-pushed the backend-config-schema-validation branch 2 times, most recently from cc46288 to 04df3f5 Compare May 16, 2018 16:19
@rramkumar1 rramkumar1 changed the title Add simple validation for BackendConfig using OpenAPI schema generation [WIP] Add simple validation for BackendConfig using OpenAPI schema generation May 16, 2018
@MrHohn MrHohn self-assigned this May 16, 2018
limitations under the License.
*/

// Note: All credit goes to github.com/ant31/crd-validation for this code.
Copy link
Member

Choose a reason for hiding this comment

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

A quick comment, should we vendor github.com/ant31/crd-validation instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I rewrote it :)

@@ -41,7 +41,7 @@ func NewSimpleClientset(objects ...runtime.Object) *Clientset {
}
}

fakePtr := &testing.Fake{}
Copy link
Member

Choose a reason for hiding this comment

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

BTW we will need to patch 3af3642 everytime when regenerate this client, or go vet will fail...

I0516 16:21:40.714] # k8s.io/ingress-gce/pkg/backendconfig/client/clientset/versioned/fake
I0516 16:21:40.714] pkg/backendconfig/client/clientset/versioned/fake/clientset_generated.go:56: literal copies lock value from fakePtr: k8s.io/ingress-gce/vendor/k8s.io/client-go/testing.Fake

@rramkumar1 rramkumar1 changed the title Add simple validation for BackendConfig using OpenAPI schema generation Add simple validation for BackendConfig using OpenAPI schema generation [WIP] May 17, 2018
@rramkumar1 rramkumar1 force-pushed the backend-config-schema-validation branch 2 times, most recently from 348227f to 2fdbed6 Compare May 17, 2018 18:43
Copy link
Member

@MrHohn MrHohn left a comment

Choose a reason for hiding this comment

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

Overall LGTM, thanks for the work

cmd/glbc/main.go Outdated
@@ -34,7 +34,8 @@ import (
neg "k8s.io/ingress-gce/pkg/neg"

"k8s.io/ingress-gce/cmd/glbc/app"
"k8s.io/ingress-gce/pkg/backendconfig"
backendconfig "k8s.io/ingress-gce/pkg/backendconfig"
Copy link
Member

Choose a reason for hiding this comment

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

Seems to have no effect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

pkg/crd/crd.go Outdated
if meta.typeSource != "" && meta.fn != nil {
validationSpec, err := validation(meta.typeSource, meta.fn)
if err != nil {
glog.V(0).Infof("Error adding simple validation for %v CRD: %v", meta.kind, err)
Copy link
Member

Choose a reason for hiding this comment

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

glog.Errorf and return crd?

Copy link
Member

Choose a reason for hiding this comment

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

And maybe remove the word "simple"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@rramkumar1 rramkumar1 force-pushed the backend-config-schema-validation branch from 2fdbed6 to 6271766 Compare May 17, 2018 20:37
@rramkumar1 rramkumar1 force-pushed the backend-config-schema-validation branch from 6271766 to 51b54d2 Compare May 17, 2018 21:24
@rramkumar1 rramkumar1 changed the title Add simple validation for BackendConfig using OpenAPI schema generation [WIP] Add simple validation for BackendConfig using OpenAPI schema generation May 17, 2018
@rramkumar1
Copy link
Contributor Author

/assign @nicksardo

@nicksardo nicksardo added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 17, 2018
@nicksardo nicksardo merged commit c945ab1 into kubernetes:master May 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants