From a7119ae1ae26003941465fbb55b46f411dc9d058 Mon Sep 17 00:00:00 2001 From: Vu Dinh Date: Thu, 16 Jan 2020 23:16:06 -0500 Subject: [PATCH 1/2] Refactor validate CRD to fix conversion error 1. Fix conversion error due to incompatible type 1. Refactor validate CRD code to reduce duplication 2. Support v1.CRD type Signed-off-by: Vu Dinh --- pkg/validation/internal/crd.go | 45 +++++++++++++++++++++++++--------- 1 file changed, 34 insertions(+), 11 deletions(-) diff --git a/pkg/validation/internal/crd.go b/pkg/validation/internal/crd.go index 3c2c8bf59..220c0166c 100644 --- a/pkg/validation/internal/crd.go +++ b/pkg/validation/internal/crd.go @@ -8,18 +8,18 @@ import ( "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions" "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/install" + "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1" "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation" "k8s.io/apimachinery/pkg/conversion" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" - "k8s.io/client-go/kubernetes/scheme" ) -var Scheme = scheme.Scheme +var scheme = runtime.NewScheme() func init() { - install.Install(Scheme) + install.Install(scheme) } var CRDValidator interfaces.Validator = interfaces.ValidatorFunc(validateCRDs) @@ -28,31 +28,54 @@ func validateCRDs(objs ...interface{}) (results []errors.ManifestResult) { for _, obj := range objs { switch v := obj.(type) { case *v1beta1.CustomResourceDefinition: - results = append(results, validateCRD(v)) + results = append(results, validateV1Beta1CRD(v)) + case *v1.CustomResourceDefinition: + results = append(results, validateV1CRD(v)) } } return results } -func validateCRD(crd runtime.Object) (result errors.ManifestResult) { - unversionedCRD := apiextensions.CustomResourceDefinition{} - err := Scheme.Converter().Convert(&crd, &unversionedCRD, conversion.SourceToDest, nil) +func validateV1Beta1CRD(crd *v1beta1.CustomResourceDefinition) (result errors.ManifestResult) { + internalCRD := &apiextensions.CustomResourceDefinition{} + v1beta1.SetDefaults_CustomResourceDefinition(crd) + v1beta1.SetDefaults_CustomResourceDefinitionSpec(&crd.Spec) + err := scheme.Converter().Convert(crd, internalCRD, conversion.SourceToDest, nil) if err != nil { - result.Add(errors.ErrInvalidParse("error converting versioned crd to unversioned crd", err)) + result.Add(errors.ErrInvalidParse("error converting crd", err)) return result } + gv := crd.GetObjectKind().GroupVersionKind().GroupVersion() - result = validateCRDUnversioned(&unversionedCRD, gv) - result.Name = unversionedCRD.GetName() + result = validateInternalCRD(internalCRD, gv) return result } -func validateCRDUnversioned(crd *apiextensions.CustomResourceDefinition, gv schema.GroupVersion) (result errors.ManifestResult) { +func validateV1CRD(crd *v1.CustomResourceDefinition) (result errors.ManifestResult) { + internalCRD := &apiextensions.CustomResourceDefinition{} + v1.SetDefaults_CustomResourceDefinition(crd) + v1.SetDefaults_CustomResourceDefinitionSpec(&crd.Spec) + err := scheme.Converter().Convert(crd, internalCRD, conversion.SourceToDest, nil) + if err != nil { + result.Add(errors.ErrInvalidParse("error converting crd", err)) + return result + } + + gv := crd.GetObjectKind().GroupVersionKind().GroupVersion() + result = validateInternalCRD(internalCRD, gv) + return result +} + +func validateInternalCRD(crd *apiextensions.CustomResourceDefinition, gv schema.GroupVersion) (result errors.ManifestResult) { errList := validation.ValidateCustomResourceDefinition(crd, gv) for _, err := range errList { if !strings.Contains(err.Field, "openAPIV3Schema") && !strings.Contains(err.Field, "status") { result.Add(errors.NewError(errors.ErrorType(err.Type), err.Error(), err.Field, err.BadValue)) } } + + if result.HasError() { + result.Name = crd.GetName() + } return result } From fab55ede74153bb9ff5242e02311092f541bd385 Mon Sep 17 00:00:00 2001 From: Vu Dinh Date: Mon, 20 Jan 2020 12:24:26 -0500 Subject: [PATCH 2/2] Add unit test case for CRD validation Signed-off-by: Vu Dinh --- pkg/validation/internal/crd_test.go | 82 +++++++++++++++++++ .../testdata/deprecatedVersion.crd.yaml | 16 ++++ .../testdata/duplicateVersions.crd.yaml | 22 +++++ pkg/validation/internal/testdata/v1.crd.yaml | 19 +++++ .../internal/testdata/v1beta1.crd.yaml | 16 ++++ 5 files changed, 155 insertions(+) create mode 100644 pkg/validation/internal/crd_test.go create mode 100644 pkg/validation/internal/testdata/deprecatedVersion.crd.yaml create mode 100644 pkg/validation/internal/testdata/duplicateVersions.crd.yaml create mode 100644 pkg/validation/internal/testdata/v1.crd.yaml create mode 100644 pkg/validation/internal/testdata/v1beta1.crd.yaml diff --git a/pkg/validation/internal/crd_test.go b/pkg/validation/internal/crd_test.go new file mode 100644 index 000000000..a9ce874be --- /dev/null +++ b/pkg/validation/internal/crd_test.go @@ -0,0 +1,82 @@ +package internal + +import ( + "io/ioutil" + "testing" + + "github.com/operator-framework/api/pkg/validation/errors" + "github.com/stretchr/testify/require" + v1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1" + + "github.com/ghodss/yaml" +) + +func TestValidateCRD(t *testing.T) { + var table = []struct { + description string + directory string + version string + hasError bool + errString string + }{ + { + description: "registryv1 bundle/valid crd", + directory: "./testdata/v1beta1.crd.yaml", + version: "v1beta1", + hasError: false, + errString: "", + }, + { + description: "registryv1 bundle/invalid crd", + directory: "./testdata/duplicateVersions.crd.yaml", + version: "v1beta1", + hasError: true, + errString: "must contain unique version names", + }, + { + description: "registryv1 bundle/invalid crd", + directory: "./testdata/v1.crd.yaml", + version: "v1", + hasError: false, + errString: "", + }, + { + description: "registryv1 bundle/invalid crd", + directory: "./testdata/deprecatedVersion.crd.yaml", + version: "v1", + hasError: true, + errString: "must have exactly one version marked as storage version", + }, + } + + for _, tt := range table { + b, err := ioutil.ReadFile(tt.directory) + if err != nil { + t.Fatalf("Error reading CRD path %s: %v", tt.directory, err) + } + + results := []errors.ManifestResult{} + switch tt.version { + case "v1": + crd := &v1.CustomResourceDefinition{} + if err = yaml.Unmarshal(b, crd); err != nil { + t.Fatalf("Error unmarshalling CRD at path %s: %v", tt.directory, err) + } + results = CRDValidator.Validate(crd) + default: + crd := &v1beta1.CustomResourceDefinition{} + if err = yaml.Unmarshal(b, crd); err != nil { + t.Fatalf("Error unmarshalling CRD at path %s: %v", tt.directory, err) + } + results = CRDValidator.Validate(crd) + } + + if len(results) > 0 { + require.Equal(t, results[0].HasError(), tt.hasError) + if results[0].HasError() { + require.Contains(t, results[0].Errors[0].Error(), tt.errString) + } + } + } +} diff --git a/pkg/validation/internal/testdata/deprecatedVersion.crd.yaml b/pkg/validation/internal/testdata/deprecatedVersion.crd.yaml new file mode 100644 index 000000000..25dcb6d78 --- /dev/null +++ b/pkg/validation/internal/testdata/deprecatedVersion.crd.yaml @@ -0,0 +1,16 @@ +apiVersion: apiextensions.k8s.io/v1 +kind: CustomResourceDefinition +metadata: + name: etcdclusters.etcd.database.coreos.com +spec: + group: etcd.database.coreos.com + names: + kind: EtcdCluster + listKind: EtcdClusterList + plural: etcdclusters + shortNames: + - etcdclus + - etcd + singular: etcdcluster + scope: Namespaced + version: v1beta2 diff --git a/pkg/validation/internal/testdata/duplicateVersions.crd.yaml b/pkg/validation/internal/testdata/duplicateVersions.crd.yaml new file mode 100644 index 000000000..5275ebb7f --- /dev/null +++ b/pkg/validation/internal/testdata/duplicateVersions.crd.yaml @@ -0,0 +1,22 @@ +apiVersion: apiextensions.k8s.io/v1beta1 +kind: CustomResourceDefinition +metadata: + name: etcdclusters.etcd.database.coreos.com +spec: + group: etcd.database.coreos.com + names: + kind: EtcdCluster + listKind: EtcdClusterList + plural: etcdclusters + shortNames: + - etcdclus + - etcd + singular: etcdcluster + scope: Namespaced + versions: + - name: v1beta2 + served: true + storage: true + - name: v1beta2 + served: true + storage: false diff --git a/pkg/validation/internal/testdata/v1.crd.yaml b/pkg/validation/internal/testdata/v1.crd.yaml new file mode 100644 index 000000000..4fef5f3ad --- /dev/null +++ b/pkg/validation/internal/testdata/v1.crd.yaml @@ -0,0 +1,19 @@ +apiVersion: apiextensions.k8s.io/v1 +kind: CustomResourceDefinition +metadata: + name: etcdclusters.etcd.database.coreos.com +spec: + group: etcd.database.coreos.com + names: + kind: EtcdCluster + listKind: EtcdClusterList + plural: etcdclusters + shortNames: + - etcdclus + - etcd + singular: etcdcluster + scope: Namespaced + versions: + - name: v1beta2 + served: true + storage: true diff --git a/pkg/validation/internal/testdata/v1beta1.crd.yaml b/pkg/validation/internal/testdata/v1beta1.crd.yaml new file mode 100644 index 000000000..01111e5c5 --- /dev/null +++ b/pkg/validation/internal/testdata/v1beta1.crd.yaml @@ -0,0 +1,16 @@ +apiVersion: apiextensions.k8s.io/v1beta1 +kind: CustomResourceDefinition +metadata: + name: etcdclusters.etcd.database.coreos.com +spec: + group: etcd.database.coreos.com + names: + kind: EtcdCluster + listKind: EtcdClusterList + plural: etcdclusters + shortNames: + - etcdclus + - etcd + singular: etcdcluster + scope: Namespaced + version: v1beta2