Skip to content

Commit

Permalink
feat(olm): allow CRD updates with multiple owners
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
Jeff Peeler committed Jun 27, 2019
1 parent f06b3f2 commit 3470dea
Show file tree
Hide file tree
Showing 2 changed files with 240 additions and 2 deletions.
45 changes: 43 additions & 2 deletions pkg/controller/operators/catalog/operator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
Expand Down
197 changes: 197 additions & 0 deletions test/e2e/installplan_e2e_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down

0 comments on commit 3470dea

Please sign in to comment.