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
- CRD is the same version without an openapi schema change
  • Loading branch information
Jeff Peeler committed Jun 14, 2019
1 parent 92b1ee8 commit ce813e2
Show file tree
Hide file tree
Showing 2 changed files with 176 additions and 113 deletions.
37 changes: 35 additions & 2 deletions pkg/controller/operators/catalog/operator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1031,6 +1031,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.Log.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 @@ -1077,9 +1087,32 @@ 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.Log.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.Log.Debugf("Found multiple owners for CRD %v", crd)
for _, oldVersion := range currentCRD.Spec.Versions {
for _, newVersion := range crd.Spec.Versions {
if oldVersion.Name == newVersion.Name {
if !o.checkCRDSchemas(currentCRD, &crd) {
return fmt.Errorf("not allowing CRD (%v) update with multiple owners with schema change on version %v", crd.GetName(), oldVersion)
}
}
}
}

if currentCRD.Spec.Version == crd.Spec.Version {
if !o.checkCRDSchemas(currentCRD, &crd) {
return fmt.Errorf("not allowing CRD (%v) update with multiple owners with schema change on 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
252 changes: 141 additions & 111 deletions test/e2e/installplan_e2e_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -498,148 +498,178 @@ func TestCreateInstallPlanWithPreExistingCRDOwners(t *testing.T) {
// Should have removed every matching step
require.Equal(t, 0, len(expectedSteps), "Actual resource steps do not match expected")
})
}

t.Run("PreExistingCRDOwnerIsReplaced", func(t *testing.T) {
defer cleaner.NotifyTestComplete(t, true)
func TestInstallPlanWithCRDSchemaChange(t *testing.T) {
defer cleaner.NotifyTestComplete(t, true)

mainPackageName := genName("nginx-")
dependentPackageName := genName("nginx-dep-")
tests := []struct {
name string
version string
expectedPhase v1alpha1.InstallPlanPhase
}{
{
name: "CRD same version",
version: "v1alpha1",
expectedPhase: v1alpha1.InstallPlanPhaseFailed,
},
{
name: "CRD different version",
version: "v1alpha2",
expectedPhase: v1alpha1.InstallPlanPhaseComplete,
},
}

mainPackageStable := fmt.Sprintf("%s-stable", mainPackageName)
mainPackageBeta := fmt.Sprintf("%s-beta", mainPackageName)
dependentPackageStable := fmt.Sprintf("%s-stable", dependentPackageName)
dependentPackageBeta := fmt.Sprintf("%s-beta", dependentPackageName)
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
defer cleaner.NotifyTestComplete(t, true)

stableChannel := "stable"
betaChannel := "beta"
mainPackageName := genName("nginx-")
mainPackageStable := fmt.Sprintf("%s-stable", mainPackageName)
mainPackageBeta := fmt.Sprintf("%s-beta", mainPackageName)

// Create manifests
mainManifests := []registry.PackageManifest{
{
PackageName: mainPackageName,
Channels: []registry.PackageChannel{
{Name: stableChannel, CurrentCSVName: mainPackageStable},
{Name: betaChannel, CurrentCSVName: mainPackageBeta},
},
DefaultChannelName: stableChannel,
},
{
PackageName: dependentPackageName,
Channels: []registry.PackageChannel{
{Name: stableChannel, CurrentCSVName: dependentPackageStable},
{Name: betaChannel, CurrentCSVName: dependentPackageBeta},
stableChannel := "stable"
betaChannel := "beta"

// Create manifests
mainManifests := []registry.PackageManifest{
{
PackageName: mainPackageName,
Channels: []registry.PackageChannel{
{Name: stableChannel, CurrentCSVName: mainPackageStable},
{Name: betaChannel, CurrentCSVName: mainPackageBeta},
},
DefaultChannelName: stableChannel,
},
DefaultChannelName: stableChannel,
},
}
}

// Create new CRDs
mainCRDPlural := genName("ins-")
mainCRD := newCRD(mainCRDPlural)
// Create new CRDs
mainCRDPlural := genName("ins-main-")
mainCRD := newCRD(mainCRDPlural)

// Create a new named install strategy
mainNamedStrategy := newNginxInstallStrategy(genName("dep-"), nil, nil)
dependentNamedStrategy := newNginxInstallStrategy(genName("dep-"), nil, nil)
// Create a new named install strategy
mainNamedStrategy := newNginxInstallStrategy(genName("dep-"), nil, nil)

dependentCRDPlural := genName("ins-")
dependentCRD := newCRD(dependentCRDPlural)
// 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)

// Create new CSVs
mainStableCSV := newCSV(mainPackageStable, testNamespace, "", semver.MustParse("0.1.0"), []apiextensions.CustomResourceDefinition{mainCRD}, []apiextensions.CustomResourceDefinition{dependentCRD}, mainNamedStrategy)
mainBetaCSV := newCSV(mainPackageBeta, testNamespace, mainPackageStable, semver.MustParse("0.2.0"), []apiextensions.CustomResourceDefinition{mainCRD}, []apiextensions.CustomResourceDefinition{dependentCRD}, mainNamedStrategy)
dependentStableCSV := newCSV(dependentPackageStable, testNamespace, "", semver.MustParse("0.1.0"), []apiextensions.CustomResourceDefinition{dependentCRD}, nil, dependentNamedStrategy)
dependentBetaCSV := newCSV(dependentPackageBeta, testNamespace, dependentPackageStable, semver.MustParse("0.2.0"), []apiextensions.CustomResourceDefinition{dependentCRD}, nil, dependentNamedStrategy)
c := newKubeClient(t)
crc := newCRClient(t)

c := newKubeClient(t)
crc := newCRClient(t)
defer func() {
require.NoError(t, crc.OperatorsV1alpha1().Subscriptions(testNamespace).DeleteCollection(&metav1.DeleteOptions{}, metav1.ListOptions{}))
}()
// Create the catalog source
mainCatalogSourceName := genName("mock-ocs-main-")
// catalog source clean up is called explicitly below
_, cleanupCatalogSource := createInternalCatalogSource(t, c, crc, mainCatalogSourceName, testNamespace, mainManifests, []apiextensions.CustomResourceDefinition{mainCRD}, []v1alpha1.ClusterServiceVersion{mainStableCSV, mainBetaCSV})

// Create the catalog source
mainCatalogSourceName := genName("mock-ocs-main-" + strings.ToLower(t.Name()) + "-")
_, cleanupCatalogSource := createInternalCatalogSource(t, c, crc, mainCatalogSourceName, testNamespace, mainManifests, []apiextensions.CustomResourceDefinition{dependentCRD, mainCRD}, []v1alpha1.ClusterServiceVersion{dependentBetaCSV, dependentStableCSV, 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)
// 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-")
subscriptionCleanup := createSubscriptionForCatalog(t, crc, testNamespace, subscriptionName, mainCatalogSourceName, mainPackageName, stableChannel, "", v1alpha1.ApprovalAutomatic)
defer subscriptionCleanup()
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)
subscription, err := fetchSubscription(t, crc, testNamespace, subscriptionName, subscriptionHasInstallPlanChecker)
require.NoError(t, err)
require.NotNil(t, subscription)

installPlanName := subscription.Status.Install.Name
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)
// 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: dependentCRD.Name, Kind: "CustomResourceDefinition"}: {},
registry.ResourceKey{Name: dependentPackageStable, Kind: v1alpha1.ClusterServiceVersionKind}: {},
registry.ResourceKey{Name: mainPackageStable, Kind: v1alpha1.ClusterServiceVersionKind}: {},
registry.ResourceKey{Name: strings.Join([]string{dependentPackageStable, mainCatalogSourceName, testNamespace}, "-"), Kind: v1alpha1.SubscriptionKind}: {},
}
// 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")
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,
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)
}
_, 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")

// Should have removed every matching step
require.Equal(t, 0, len(expectedSteps), "Actual resource steps do not match expected")
cleanupCatalogSource()
// CRD schema change
var min float64 = 2
var max float64 = 256
mainCRD.Spec.Version = tt.version
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,
},
},
},
},
},
}
_, 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)

// 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)
// 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)
// 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)
subscription, err = fetchSubscription(t, crc, testNamespace, subscriptionName, subscriptionHasInstallPlanChecker)
require.NoError(t, err)
require.NotNil(t, subscription)

installPlanName = subscription.Status.Install.Name
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)
t.Logf("Install plan %s fetched with status %s", fetchedInstallPlan.GetName(), fetchedInstallPlan.Status.Phase)
// 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, v1alpha1.InstallPlanPhaseComplete, 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)
// 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)
// 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)
})
t.Logf("All expected resources resolved %s", fetchedCSV.Status.Phase)
})
}
}

func TestUpdateInstallPlan(t *testing.T) {
Expand Down

0 comments on commit ce813e2

Please sign in to comment.