From fcb57196cde94635ab8770c6b887b4d9d0f10e75 Mon Sep 17 00:00:00 2001 From: Vu Dinh Date: Thu, 1 Aug 2019 06:47:48 -0400 Subject: [PATCH] Bug 1732914: Operator upgrades fail when versions field is not set 1. Fix the ensureCRDVersions to account for `version` field 2. Add test cases for `version` scenarios Signed-off-by: Vu Dinh --- pkg/controller/operators/catalog/operator.go | 45 ++++++++++++------- .../operators/catalog/operator_test.go | 29 ++++++++++++ test/e2e/installplan_e2e_test.go | 33 ++++++++++++++ 3 files changed, 92 insertions(+), 15 deletions(-) diff --git a/pkg/controller/operators/catalog/operator.go b/pkg/controller/operators/catalog/operator.go index 4667536c04..323213ac2e 100644 --- a/pkg/controller/operators/catalog/operator.go +++ b/pkg/controller/operators/catalog/operator.go @@ -1064,23 +1064,35 @@ func (o *Operator) ResolvePlan(plan *v1alpha1.InstallPlan) error { return nil } -// Ensure all existing versions are present in new CRD +// Ensure all existing served versions are present in new CRD func ensureCRDVersions(oldCRD *v1beta1ext.CustomResourceDefinition, newCRD *v1beta1ext.CustomResourceDefinition) error { + newCRDVersions := map[string]struct{}{} + + for _, newVersion := range newCRD.Spec.Versions { + newCRDVersions[newVersion.Name] = struct{}{} + } + if newCRD.Spec.Version != "" { + newCRDVersions[newCRD.Spec.Version] = struct{}{} + } + for _, oldVersion := range oldCRD.Spec.Versions { - var versionPresent bool - for _, newVersion := range newCRD.Spec.Versions { - if oldVersion.Name == newVersion.Name { - versionPresent = true + if oldVersion.Served { + _, ok := newCRDVersions[oldVersion.Name] + if !ok { + return fmt.Errorf("New CRD (%s) must contain existing served versions (%s)", oldCRD.Name, oldVersion.Name) } } - if !versionPresent { - return fmt.Errorf("not allowing CRD (%v) update with unincluded version %v", newCRD.GetName(), oldVersion) + } + if oldCRD.Spec.Version != "" { + _, ok := newCRDVersions[oldCRD.Spec.Version] + if !ok { + return fmt.Errorf("New CRD (%s) must contain existing version (%s)", oldCRD.Name, oldCRD.Spec.Version) } } - return nil } +// Validate all existing served versions against new CRD's validation (if changed) func (o *Operator) validateCustomResourceDefinition(oldCRD *v1beta1ext.CustomResourceDefinition, newCRD *v1beta1ext.CustomResourceDefinition) error { o.logger.Debugf("Comparing %#v to %#v", oldCRD.Spec.Validation, newCRD.Spec.Validation) // If validation schema is unchanged, return right away @@ -1091,11 +1103,13 @@ func (o *Operator) validateCustomResourceDefinition(oldCRD *v1beta1ext.CustomRes if err := v1beta1ext.Convert_v1beta1_CustomResourceDefinition_To_apiextensions_CustomResourceDefinition(newCRD, convertedCRD, nil); err != nil { return err } - for _, oldVersion := range oldCRD.Spec.Versions { - gvr := schema.GroupVersionResource{Group: oldCRD.Spec.Group, Version: oldVersion.Name, Resource: oldCRD.Spec.Names.Plural} - err := o.validateExistingCRs(gvr, convertedCRD) - if err != nil { - return err + for _, version := range oldCRD.Spec.Versions { + if !version.Served { + gvr := schema.GroupVersionResource{Group: oldCRD.Spec.Group, Version: version.Name, Resource: oldCRD.Spec.Names.Plural} + err := o.validateExistingCRs(gvr, convertedCRD) + if err != nil { + return err + } } } @@ -1106,7 +1120,7 @@ func (o *Operator) validateCustomResourceDefinition(oldCRD *v1beta1ext.CustomRes return err } } - + o.logger.Debugf("Successfully validated CRD %s\n", newCRD.Name) return nil } @@ -1198,7 +1212,8 @@ func (o *Operator) ExecutePlan(plan *v1alpha1.InstallPlan) error { } else if len(matchedCSV) > 1 { o.logger.Debugf("Found multiple owners for CRD %v", crd) - if err := ensureCRDVersions(currentCRD, &crd); err != nil { + err := ensureCRDVersions(currentCRD, &crd) + if err != nil { return errorwrap.Wrapf(err, "error missing existing CRD version(s) in new CRD: %s", step.Resource.Name) } diff --git a/pkg/controller/operators/catalog/operator_test.go b/pkg/controller/operators/catalog/operator_test.go index d0f4d0800a..8e74330ea2 100644 --- a/pkg/controller/operators/catalog/operator_test.go +++ b/pkg/controller/operators/catalog/operator_test.go @@ -195,6 +195,35 @@ func TestEnsureCRDVersions(t *testing.T) { }(), expectedFailure: true, }, + { + name: "missing version in new CRD", + oldCRD: func() v1beta1.CustomResourceDefinition { + oldCRD := crd(mainCRDPlural) + oldCRD.Spec.Version = "v1alpha1" + return oldCRD + }(), + newCRD: func() v1beta1.CustomResourceDefinition { + newCRD := crd(mainCRDPlural) + newCRD.Spec.Version = "v1alpha2" + return newCRD + }(), + expectedFailure: true, + }, + { + name: "existing version is present in new CRD's versions", + oldCRD: func() v1beta1.CustomResourceDefinition { + oldCRD := crd(mainCRDPlural) + oldCRD.Spec.Version = "v1alpha1" + return oldCRD + }(), + newCRD: func() v1beta1.CustomResourceDefinition { + newCRD := crd(mainCRDPlural) + newCRD.Spec.Version = "" + newCRD.Spec.Versions = addedVersions + return newCRD + }(), + expectedFailure: false, + }, } for _, tt := range tests { diff --git a/test/e2e/installplan_e2e_test.go b/test/e2e/installplan_e2e_test.go index 9438996bbd..96bb38c648 100644 --- a/test/e2e/installplan_e2e_test.go +++ b/test/e2e/installplan_e2e_test.go @@ -823,6 +823,39 @@ func TestInstallPlanWithCRDSchemaChange(t *testing.T) { return &newCRD }(), }, + { + name: "existing version is present in new CRD (deprecated field)", + expectedPhase: v1alpha1.InstallPlanPhaseComplete, + oldCRD: func() *apiextensions.CustomResourceDefinition { + oldCRD := newCRD(mainCRDPlural + "d") + oldCRD.Spec.Version = "v1alpha1" + return &oldCRD + }(), + newCRD: func() *apiextensions.CustomResourceDefinition { + newCRD := newCRD(mainCRDPlural + "d") + newCRD.Spec.Version = "v1alpha1" + newCRD.Spec.Validation = &apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ + Type: "object", + Properties: map[string]apiextensions.JSONSchemaProps{ + "spec": { + Type: "object", + Description: "Spec of a test object.", + Properties: map[string]apiextensions.JSONSchemaProps{ + "scalar": { + Type: "number", + Description: "Scalar value that should have a min and max.", + Minimum: &min, + Maximum: &max, + }, + }, + }, + }, + }, + } + return &newCRD + }(), + }, } for _, tt := range tests {