From 3470deacd58c3f45c9297e7d44399acfbd9fde9e Mon Sep 17 00:00:00 2001 From: Jeff Peeler Date: Mon, 24 Jun 2019 20:18:29 -0400 Subject: [PATCH] feat(olm): allow CRD updates with multiple owners Currently CRD updates aren't done for existing CRDs, so this enables that functionality in the cases of: - New CRD is a different non-existing version - New CRD is the same version without an openapi schema change --- pkg/controller/operators/catalog/operator.go | 45 ++++- test/e2e/installplan_e2e_test.go | 197 +++++++++++++++++++ 2 files changed, 240 insertions(+), 2 deletions(-) diff --git a/pkg/controller/operators/catalog/operator.go b/pkg/controller/operators/catalog/operator.go index 06954584b22..e45770cb35a 100644 --- a/pkg/controller/operators/catalog/operator.go +++ b/pkg/controller/operators/catalog/operator.go @@ -1021,6 +1021,16 @@ func (o *Operator) ResolvePlan(plan *v1alpha1.InstallPlan) error { return nil } +// TODO: in the future this will be extended to verify more than just whether or not the schema changed +func (o *Operator) checkCRDSchemas(oldCRD *v1beta1ext.CustomResourceDefinition, newCRD *v1beta1ext.CustomResourceDefinition) bool { + o.logger.Debugf("Comparing %#v to %#v", oldCRD.Spec.Validation, newCRD.Spec.Validation) + if reflect.DeepEqual(oldCRD.Spec.Validation, newCRD.Spec.Validation) { + return true + } + + return false +} + // ExecutePlan applies a planned InstallPlan to a namespace. func (o *Operator) ExecutePlan(plan *v1alpha1.InstallPlan) error { if plan.Status.Phase != v1alpha1.InstallPlanPhaseInstalling { @@ -1067,9 +1077,40 @@ func (o *Operator) ExecutePlan(plan *v1alpha1.InstallPlan) error { if err != nil { return errorwrap.Wrapf(err, "error find matched CSV: %s", step.Resource.Name) } + crd.SetResourceVersion(currentCRD.GetResourceVersion()) if len(matchedCSV) == 1 { - // Attempt to update CRD - crd.SetResourceVersion(currentCRD.GetResourceVersion()) + o.logger.Debugf("Found one owner for CRD %v", crd) + _, err = o.opClient.ApiextensionsV1beta1Interface().ApiextensionsV1beta1().CustomResourceDefinitions().Update(&crd) + if err != nil { + return errorwrap.Wrapf(err, "error updating CRD: %s", step.Resource.Name) + } + } else if len(matchedCSV) > 1 { + o.logger.Debugf("Found multiple owners for CRD %v", crd) + shouldCheckSchema := len(currentCRD.Spec.Versions) == len(crd.Spec.Versions) // no version bump + + // 1) if there's no version change, verify that schemas match 2) ensure all old versions are present + for _, oldVersion := range currentCRD.Spec.Versions { + var versionPresent bool + for _, newVersion := range crd.Spec.Versions { + if oldVersion.Name == newVersion.Name { + if shouldCheckSchema && !o.checkCRDSchemas(currentCRD, &crd) { + return fmt.Errorf("not allowing CRD (%v) update with multiple owners with schema change on version %v", crd.GetName(), oldVersion) + } + versionPresent = true + } + } + if !versionPresent { + return fmt.Errorf("not allowing CRD (%v) update with unincluded version %v", crd.GetName(), oldVersion) + } + } + + // ensure a CRD with multiple versions isn't checked + if crd.Spec.Version != "" { + if !o.checkCRDSchemas(currentCRD, &crd) { + return fmt.Errorf("not allowing CRD (%v) update with multiple owners with schema change on (single) version %v", crd.GetName(), crd.Spec.Version) + } + } + _, err = o.opClient.ApiextensionsV1beta1Interface().ApiextensionsV1beta1().CustomResourceDefinitions().Update(&crd) if err != nil { return errorwrap.Wrapf(err, "error update CRD: %s", step.Resource.Name) diff --git a/test/e2e/installplan_e2e_test.go b/test/e2e/installplan_e2e_test.go index 016860b3f47..4340b4618a8 100644 --- a/test/e2e/installplan_e2e_test.go +++ b/test/e2e/installplan_e2e_test.go @@ -642,6 +642,203 @@ func TestCreateInstallPlanWithPreExistingCRDOwners(t *testing.T) { }) } +func TestInstallPlanWithCRDSchemaChange(t *testing.T) { + defer cleaner.NotifyTestComplete(t, true) + + // excluded: new CRD, same version, same schema - won't trigger a CRD update + tests := []struct { + name string + version string + multipleVersions []string // if specified, version above will be ignored + expectedPhase v1alpha1.InstallPlanPhase + changeSchema bool + }{ + { + name: "New CRD, same version, different schema", + version: "v1alpha1", + expectedPhase: v1alpha1.InstallPlanPhaseFailed, + changeSchema: true, + }, + { + name: "New CRD, different version, different schema", + expectedPhase: v1alpha1.InstallPlanPhaseComplete, + multipleVersions: []string{"v1alpha1", "v1alpha2"}, + changeSchema: true, + }, + { + name: "New CRD, different version, same schema", + expectedPhase: v1alpha1.InstallPlanPhaseComplete, + multipleVersions: []string{"v1alpha1", "v1alpha2"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + defer cleaner.NotifyTestComplete(t, true) + + mainPackageName := genName("nginx-") + mainPackageStable := fmt.Sprintf("%s-stable", mainPackageName) + mainPackageBeta := fmt.Sprintf("%s-beta", mainPackageName) + + stableChannel := "stable" + betaChannel := "beta" + + // Create manifests + mainManifests := []registry.PackageManifest{ + { + PackageName: mainPackageName, + Channels: []registry.PackageChannel{ + {Name: stableChannel, CurrentCSVName: mainPackageStable}, + {Name: betaChannel, CurrentCSVName: mainPackageBeta}, + }, + DefaultChannelName: stableChannel, + }, + } + + // Create new CRDs + mainCRDPlural := genName("ins-main-") + mainCRD := newCRD(mainCRDPlural) + + // Create a new named install strategy + mainNamedStrategy := newNginxInstallStrategy(genName("dep-"), nil, nil) + + // Create new CSVs + mainStableCSV := newCSV(mainPackageStable, testNamespace, "", semver.MustParse("0.1.0"), []apiextensions.CustomResourceDefinition{mainCRD}, nil, mainNamedStrategy) + mainBetaCSV := newCSV(mainPackageBeta, testNamespace, mainPackageStable, semver.MustParse("0.2.0"), []apiextensions.CustomResourceDefinition{mainCRD}, nil, mainNamedStrategy) + + c := newKubeClient(t) + crc := newCRClient(t) + + // Create the catalog source + mainCatalogSourceName := genName("mock-ocs-main-") + _, cleanupCatalogSource := createInternalCatalogSource(t, c, crc, mainCatalogSourceName, testNamespace, mainManifests, []apiextensions.CustomResourceDefinition{mainCRD}, []v1alpha1.ClusterServiceVersion{mainStableCSV, mainBetaCSV}) + defer cleanupCatalogSource() + + // Attempt to get the catalog source before creating install plan(s) + _, err := fetchCatalogSource(t, crc, mainCatalogSourceName, testNamespace, catalogSourceRegistryPodSynced) + require.NoError(t, err) + + subscriptionName := genName("sub-nginx-") + // this subscription will be cleaned up below without the clean up function + createSubscriptionForCatalog(t, crc, testNamespace, subscriptionName, mainCatalogSourceName, mainPackageName, stableChannel, "", v1alpha1.ApprovalAutomatic) + + subscription, err := fetchSubscription(t, crc, testNamespace, subscriptionName, subscriptionHasInstallPlanChecker) + require.NoError(t, err) + require.NotNil(t, subscription) + + installPlanName := subscription.Status.Install.Name + + // Wait for InstallPlan to be status: Complete or failed before checking resource presence + completeOrFailedFunc := buildInstallPlanPhaseCheckFunc(v1alpha1.InstallPlanPhaseComplete, v1alpha1.InstallPlanPhaseFailed) + fetchedInstallPlan, err := fetchInstallPlan(t, crc, installPlanName, completeOrFailedFunc) + require.NoError(t, err) + t.Logf("Install plan %s fetched with status %s", fetchedInstallPlan.GetName(), fetchedInstallPlan.Status.Phase) + require.Equal(t, v1alpha1.InstallPlanPhaseComplete, fetchedInstallPlan.Status.Phase) + + // Ensure that the desired resources have been created + expectedSteps := map[registry.ResourceKey]struct{}{ + registry.ResourceKey{Name: mainCRD.Name, Kind: "CustomResourceDefinition"}: {}, + registry.ResourceKey{Name: mainPackageStable, Kind: v1alpha1.ClusterServiceVersionKind}: {}, + } + + require.Equal(t, len(expectedSteps), len(fetchedInstallPlan.Status.Plan), "number of expected steps does not match installed") + + for _, step := range fetchedInstallPlan.Status.Plan { + key := registry.ResourceKey{ + Name: step.Resource.Name, + Kind: step.Resource.Kind, + } + _, ok := expectedSteps[key] + require.True(t, ok, "couldn't find %v in expected steps: %#v", key, expectedSteps) + + // Remove the entry from the expected steps set (to ensure no duplicates in resolved plan) + delete(expectedSteps, key) + } + + // Should have removed every matching step + require.Equal(t, 0, len(expectedSteps), "Actual resource steps do not match expected") + + // new CRD changes + if tt.multipleVersions != nil { + for _, version := range tt.multipleVersions { + crdVersion := apiextensions.CustomResourceDefinitionVersion{ + Name: version, + Served: true, + Storage: false, + } + mainCRD.Spec.Versions = append(mainCRD.Spec.Versions, crdVersion) + } + mainCRD.Spec.Version = "" + mainCRD.Spec.Versions[len(tt.multipleVersions)-1].Storage = true + } else { + mainCRD.Spec.Version = tt.version + } + + var min float64 = 2 + var max float64 = 256 + if tt.changeSchema == true { + mainCRD.Spec.Validation = &apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ + 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, + }, + }, + }, + }, + }, + } + } + + updateInternalCatalog(t, c, crc, mainCatalogSourceName, testNamespace, []apiextensions.CustomResourceDefinition{mainCRD}, []v1alpha1.ClusterServiceVersion{mainStableCSV, mainBetaCSV}, mainManifests) + // Attempt to get the catalog source before creating install plan(s) + _, err = fetchCatalogSource(t, crc, mainCatalogSourceName, testNamespace, catalogSourceRegistryPodSynced) + require.NoError(t, err) + + // Update the subscription resource to point to the beta CSV + err = crc.OperatorsV1alpha1().Subscriptions(testNamespace).DeleteCollection(metav1.NewDeleteOptions(0), metav1.ListOptions{}) + require.NoError(t, err) + + // existing cleanup should remove this + createSubscriptionForCatalog(t, crc, testNamespace, subscriptionName, mainCatalogSourceName, mainPackageName, betaChannel, "", v1alpha1.ApprovalAutomatic) + + subscription, err = fetchSubscription(t, crc, testNamespace, subscriptionName, subscriptionHasInstallPlanChecker) + require.NoError(t, err) + require.NotNil(t, subscription) + + installPlanName = subscription.Status.Install.Name + + // Wait for InstallPlan to be status: Complete or Failed before checking resource presence + fetchedInstallPlan, err = fetchInstallPlan(t, crc, installPlanName, buildInstallPlanPhaseCheckFunc(v1alpha1.InstallPlanPhaseComplete, v1alpha1.InstallPlanPhaseFailed)) + require.NoError(t, err) + fmt.Printf("Phase = %v", fetchedInstallPlan.Status.Phase) + t.Logf("Install plan %s fetched with status %s", fetchedInstallPlan.GetName(), fetchedInstallPlan.Status.Phase) + + require.Equal(t, tt.expectedPhase, fetchedInstallPlan.Status.Phase) + + // Fetch installplan again to check for unnecessary control loops + fetchedInstallPlan, err = fetchInstallPlan(t, crc, fetchedInstallPlan.GetName(), func(fip *v1alpha1.InstallPlan) bool { + compareResources(t, fetchedInstallPlan, fip) + return true + }) + require.NoError(t, err) + + // Ensure correct in-cluster resource(s) + fetchedCSV, err := fetchCSV(t, crc, mainBetaCSV.GetName(), testNamespace, csvSucceededChecker) + require.NoError(t, err) + + t.Logf("All expected resources resolved %s", fetchedCSV.Status.Phase) + }) + } +} + func TestUpdateInstallPlan(t *testing.T) { defer cleaner.NotifyTestComplete(t, true) t.Run("UpdateSingleExistingCRDOwner", func(t *testing.T) {