Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Bug 1732914: Operator upgrades fail when versions field is not set #973

Merged
merged 1 commit into from
Aug 22, 2019
+92 −15
Merged

Bug 1732914: Operator upgrades fail when versions field is not set #973

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 30 additions & 15 deletions pkg/controller/operators/catalog/operator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{}{}
Copy link
Member

@ecordell ecordell Aug 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the behavior that we want here actually something like:

  1. There must be a nonempty intersection of oldCRDVersions and newCRDVersions
  2. For any version in both oldCRD and newCRD, ensure the validation is unchanged (or has only backwards-compatible changes)

I'm thinking that this used as-is will prevent anyone from ever deprecating an older api version. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Developers can deprecate/disable a version of CRD by simply changing Served field into false. The reason I think we shouldn't allow developers to remove a version straight out without at least disabling it first to avoid the confusion that a CSV may reference that version or an existing CR with that version but the new CRD has no record of that version. Also, if a CSV is still referencing to a version that is not present in new CRD, the InstallPlan won't include a step to upgrade that CRD.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to make sure that this is documented. Can we add some doc's in this PR?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shawn-hurley The doc PR is referenced in this PR. It is #984.


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
Expand All @@ -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
}
}
}

Expand All @@ -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
}

Expand Down Expand Up @@ -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)
}

Expand Down
29 changes: 29 additions & 0 deletions pkg/controller/operators/catalog/operator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
33 changes: 33 additions & 0 deletions test/e2e/installplan_e2e_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down