Skip to content

OCPBUGS-18948: CRD Compatibility Validation #592

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

Merged
merged 1 commit into from
Oct 21, 2023
Merged

OCPBUGS-18948: CRD Compatibility Validation #592

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
130 changes: 78 additions & 52 deletions staging/operator-lifecycle-manager/pkg/controller/operators/catalog/operator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import (
"k8s.io/apimachinery/pkg/selection"
utilerrors "k8s.io/apimachinery/pkg/util/errors"
utilruntime "k8s.io/apimachinery/pkg/util/runtime"
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/apimachinery/pkg/util/validation/field"
"k8s.io/apimachinery/pkg/util/yaml"
batchv1applyconfigurations "k8s.io/client-go/applyconfigurations/batch/v1"
Expand Down Expand Up @@ -2018,81 +2019,106 @@ func transitionInstallPlanState(log logrus.FieldLogger, transitioner installPlan
func validateV1CRDCompatibility(dynamicClient dynamic.Interface, oldCRD *apiextensionsv1.CustomResourceDefinition, newCRD *apiextensionsv1.CustomResourceDefinition) error {
logrus.Debugf("Comparing %#v to %#v", oldCRD.Spec.Versions, newCRD.Spec.Versions)

// If validation schema is unchanged, return right away
newestSchema := newCRD.Spec.Versions[len(newCRD.Spec.Versions)-1].Schema
for i, oldVersion := range oldCRD.Spec.Versions {
if !reflect.DeepEqual(oldVersion.Schema, newestSchema) {
break
}
if i == len(oldCRD.Spec.Versions)-1 {
// we are on the last iteration
// schema has not changed between versions at this point.
return nil
oldVersionSet := sets.New[string]()
for _, oldVersion := range oldCRD.Spec.Versions {
if !oldVersionSet.Has(oldVersion.Name) && oldVersion.Served {
oldVersionSet.Insert(oldVersion.Name)
}
}

convertedCRD := &apiextensions.CustomResourceDefinition{}
if err := apiextensionsv1.Convert_v1_CustomResourceDefinition_To_apiextensions_CustomResourceDefinition(newCRD, convertedCRD, nil); 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 := validateExistingCRs(dynamicClient, gvr, convertedCRD)
if err != nil {
validationsMap := make(map[string]*apiextensions.CustomResourceValidation, 0)
for _, newVersion := range newCRD.Spec.Versions {
if oldVersionSet.Has(newVersion.Name) && newVersion.Served {
// If the new CRD's version is present in the cluster and still
// served then fill the map entry with the new validation
convertedValidation := &apiextensions.CustomResourceValidation{}
if err := apiextensionsv1.Convert_v1_CustomResourceValidation_To_apiextensions_CustomResourceValidation(newVersion.Schema, convertedValidation, nil); err != nil {
return err
}
validationsMap[newVersion.Name] = convertedValidation
}
}

logrus.Debugf("Successfully validated CRD %s\n", newCRD.Name)
return nil
return validateExistingCRs(dynamicClient, schema.GroupResource{Group: newCRD.Spec.Group, Resource: newCRD.Spec.Names.Plural}, validationsMap)
}

// Validate all existing served versions against new CRD's validation (if changed)
func validateV1Beta1CRDCompatibility(dynamicClient dynamic.Interface, oldCRD *apiextensionsv1beta1.CustomResourceDefinition, newCRD *apiextensionsv1beta1.CustomResourceDefinition) error {
logrus.Debugf("Comparing %#v to %#v", oldCRD.Spec.Validation, newCRD.Spec.Validation)

// TODO return early of all versions are equal
convertedCRD := &apiextensions.CustomResourceDefinition{}
if err := apiextensionsv1beta1.Convert_v1beta1_CustomResourceDefinition_To_apiextensions_CustomResourceDefinition(newCRD, convertedCRD, nil); err != nil {
return err
oldVersionSet := sets.New[string]()
if len(oldCRD.Spec.Versions) == 0 {
// apiextensionsv1beta1 special case: if spec.Versions is empty, use the global version and validation
oldVersionSet.Insert(oldCRD.Spec.Version)
}
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 := validateExistingCRs(dynamicClient, gvr, convertedCRD)
if err != nil {
return err
}
for _, oldVersion := range oldCRD.Spec.Versions {
// collect served versions from spec.Versions if the list is present
if !oldVersionSet.Has(oldVersion.Name) && oldVersion.Served {
oldVersionSet.Insert(oldVersion.Name)
}
}

if oldCRD.Spec.Version != "" {
gvr := schema.GroupVersionResource{Group: oldCRD.Spec.Group, Version: oldCRD.Spec.Version, Resource: oldCRD.Spec.Names.Plural}
err := validateExistingCRs(dynamicClient, gvr, convertedCRD)
if err != nil {
return err
validationsMap := make(map[string]*apiextensions.CustomResourceValidation, 0)
gr := schema.GroupResource{Group: newCRD.Spec.Group, Resource: newCRD.Spec.Names.Plural}
if len(newCRD.Spec.Versions) == 0 {
// apiextensionsv1beta1 special case: if spec.Versions of newCRD is empty, use the global version and validation
if oldVersionSet.Has(newCRD.Spec.Version) {
convertedValidation := &apiextensions.CustomResourceValidation{}
if err := apiextensionsv1beta1.Convert_v1beta1_CustomResourceValidation_To_apiextensions_CustomResourceValidation(newCRD.Spec.Validation, convertedValidation, nil); err != nil {
return err
}
validationsMap[newCRD.Spec.Version] = convertedValidation
}
}
for _, newVersion := range newCRD.Spec.Versions {
if oldVersionSet.Has(newVersion.Name) && newVersion.Served {
// If the new CRD's version is present in the cluster and still
// served then fill the map entry with the new validation
if newCRD.Spec.Validation != nil {
// apiextensionsv1beta1 special case: spec.Validation and spec.Versions[].Schema are mutually exclusive;
// if spec.Versions is non-empty and spec.Validation is set then we can validate once against any
// single existing version.
convertedValidation := &apiextensions.CustomResourceValidation{}
if err := apiextensionsv1beta1.Convert_v1beta1_CustomResourceValidation_To_apiextensions_CustomResourceValidation(newCRD.Spec.Validation, convertedValidation, nil); err != nil {
return err
}
return validateExistingCRs(dynamicClient, gr, map[string]*apiextensions.CustomResourceValidation{newVersion.Name: convertedValidation})
}
convertedValidation := &apiextensions.CustomResourceValidation{}
if err := apiextensionsv1beta1.Convert_v1beta1_CustomResourceValidation_To_apiextensions_CustomResourceValidation(newVersion.Schema, convertedValidation, nil); err != nil {
return err
}
validationsMap[newVersion.Name] = convertedValidation
}
}
logrus.Debugf("Successfully validated CRD %s\n", newCRD.Name)
return nil
return validateExistingCRs(dynamicClient, gr, validationsMap)
}

func validateExistingCRs(dynamicClient dynamic.Interface, gvr schema.GroupVersionResource, newCRD *apiextensions.CustomResourceDefinition) error {
// make dynamic client
crList, err := dynamicClient.Resource(gvr).List(context.TODO(), metav1.ListOptions{})
if err != nil {
return fmt.Errorf("error listing resources in GroupVersionResource %#v: %s", gvr, err)
}
for _, cr := range crList.Items {
validator, _, err := validation.NewSchemaValidator(newCRD.Spec.Validation)
// validateExistingCRs lists all CRs for each version entry in validationsMap, then validates each using the paired validation.
func validateExistingCRs(dynamicClient dynamic.Interface, gr schema.GroupResource, validationsMap map[string]*apiextensions.CustomResourceValidation) error {
for version, schemaValidation := range validationsMap {
// create validator from given crdValidation
validator, _, err := validation.NewSchemaValidator(schemaValidation)
if err != nil {
return fmt.Errorf("error creating validator for schema %#v: %s", newCRD.Spec.Validation, err)
return fmt.Errorf("error creating validator for schema version %s: %s", version, err)
}
err = validation.ValidateCustomResource(field.NewPath(""), cr.UnstructuredContent(), validator).ToAggregate()

gvr := schema.GroupVersionResource{Group: gr.Group, Version: version, Resource: gr.Resource}
crList, err := dynamicClient.Resource(gvr).List(context.TODO(), metav1.ListOptions{})
if err != nil {
return fmt.Errorf("error validating custom resource against new schema for %s %s/%s: %v", newCRD.Spec.Names.Kind, cr.GetNamespace(), cr.GetName(), err)
return fmt.Errorf("error listing resources in GroupVersionResource %#v: %s", gvr, err)
}

// validate each CR against this version schema
for _, cr := range crList.Items {
err = validation.ValidateCustomResource(field.NewPath(""), cr.UnstructuredContent(), validator).ToAggregate()
if err != nil {
var namespacedName string
if cr.GetNamespace() == "" {
namespacedName = cr.GetName()
} else {
namespacedName = fmt.Sprintf("%s/%s", cr.GetNamespace(), cr.GetName())
}
return fmt.Errorf("error validating %s %q: updated validation is too restrictive: %v", cr.GroupVersionKind(), namespacedName, err)
}
}
}
return nil
Expand Down
151 changes: 139 additions & 12 deletions staging/operator-lifecycle-manager/pkg/controller/operators/catalog/operator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import (
"time"

"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/install"
"k8s.io/apiextensions-apiserver/pkg/apis/apiextensions"

"github.com/sirupsen/logrus"
"github.com/stretchr/testify/require"
Expand Down Expand Up @@ -1459,7 +1458,7 @@ func TestCompetingCRDOwnersExist(t *testing.T) {
}
}

func TestValidateExistingCRs(t *testing.T) {
func TestValidateV1Beta1CRDCompatibility(t *testing.T) {
unstructuredForFile := func(file string) *unstructured.Unstructured {
data, err := os.ReadFile(file)
require.NoError(t, err)
Expand All @@ -1469,22 +1468,21 @@ func TestValidateExistingCRs(t *testing.T) {
return k8sFile
}

unversionedCRDForV1beta1File := func(file string) *apiextensions.CustomResourceDefinition {
unversionedCRDForV1beta1File := func(file string) *apiextensionsv1beta1.CustomResourceDefinition {
data, err := os.ReadFile(file)
require.NoError(t, err)
dec := utilyaml.NewYAMLOrJSONDecoder(strings.NewReader(string(data)), 30)
k8sFile := &apiextensionsv1beta1.CustomResourceDefinition{}
require.NoError(t, dec.Decode(k8sFile))
convertedCRD := &apiextensions.CustomResourceDefinition{}
require.NoError(t, apiextensionsv1beta1.Convert_v1beta1_CustomResourceDefinition_To_apiextensions_CustomResourceDefinition(k8sFile, convertedCRD, nil))
return convertedCRD
return k8sFile
}

tests := []struct {
name string
existingObjects []runtime.Object
gvr schema.GroupVersionResource
newCRD *apiextensions.CustomResourceDefinition
oldCRD *apiextensionsv1beta1.CustomResourceDefinition
newCRD *apiextensionsv1beta1.CustomResourceDefinition
want error
}{
{
Expand All @@ -1497,6 +1495,7 @@ func TestValidateExistingCRs(t *testing.T) {
Version: "v1",
Resource: "machinepools",
},
oldCRD: unversionedCRDForV1beta1File("testdata/hivebug/crd.yaml"),
newCRD: unversionedCRDForV1beta1File("testdata/hivebug/crd.yaml"),
},
{
Expand All @@ -1509,16 +1508,144 @@ func TestValidateExistingCRs(t *testing.T) {
Version: "v1",
Resource: "machinepools",
},
oldCRD: unversionedCRDForV1beta1File("testdata/hivebug/crd.yaml"),
newCRD: unversionedCRDForV1beta1File("testdata/hivebug/crd.yaml"),
want: fmt.Errorf("error validating custom resource against new schema for MachinePool /test: [[].spec.clusterDeploymentRef: Invalid value: \"null\": spec.clusterDeploymentRef in body must be of type object: \"null\", [].spec.name: Required value, [].spec.platform: Required value]"),
want: fmt.Errorf("error validating hive.openshift.io/v1, Kind=MachinePool \"test\": updated validation is too restrictive: [[].spec.clusterDeploymentRef: Invalid value: \"null\": spec.clusterDeploymentRef in body must be of type object: \"null\", [].spec.name: Required value, [].spec.platform: Required value]"),
},
{
name: "backwards incompatible change",
existingObjects: []runtime.Object{
unstructuredForFile("testdata/apiextensionsv1beta1/cr.yaml"),
},
gvr: schema.GroupVersionResource{
Group: "cluster.com",
Version: "v1alpha1",
Resource: "testcrd",
},
oldCRD: unversionedCRDForV1beta1File("testdata/apiextensionsv1beta1/crd.old.yaml"),
newCRD: unversionedCRDForV1beta1File("testdata/apiextensionsv1beta1/crd.yaml"),
want: fmt.Errorf("error validating cluster.com/v1alpha1, Kind=testcrd \"my-cr-1\": updated validation is too restrictive: [].spec.scalar: Invalid value: 2: spec.scalar in body should be greater than or equal to 3"),
},
{
name: "unserved version",
existingObjects: []runtime.Object{
unstructuredForFile("testdata/apiextensionsv1beta1/cr.yaml"),
unstructuredForFile("testdata/apiextensionsv1beta1/cr.v2.yaml"),
},
gvr: schema.GroupVersionResource{
Group: "cluster.com",
Version: "v1alpha1",
Resource: "testcrd",
},
oldCRD: unversionedCRDForV1beta1File("testdata/apiextensionsv1beta1/crd.old.yaml"),
newCRD: unversionedCRDForV1beta1File("testdata/apiextensionsv1beta1/crd.unserved.yaml"),
},
{
name: "cr not validated against currently unserved version",
existingObjects: []runtime.Object{
unstructuredForFile("testdata/apiextensionsv1beta1/cr.yaml"),
unstructuredForFile("testdata/apiextensionsv1beta1/cr.v2.yaml"),
},
oldCRD: unversionedCRDForV1beta1File("testdata/apiextensionsv1beta1/crd.unserved.yaml"),
newCRD: unversionedCRDForV1beta1File("testdata/apiextensionsv1beta1/crd.yaml"),
},
{
name: "crd with no versions list",
existingObjects: []runtime.Object{
unstructuredForFile("testdata/apiextensionsv1beta1/cr.yaml"),
unstructuredForFile("testdata/apiextensionsv1beta1/cr.v2.yaml"),
},
oldCRD: unversionedCRDForV1beta1File("testdata/apiextensionsv1beta1/crd.no-versions-list.old.yaml"),
newCRD: unversionedCRDForV1beta1File("testdata/apiextensionsv1beta1/crd.no-versions-list.yaml"),
want: fmt.Errorf("error validating cluster.com/v1alpha1, Kind=testcrd \"my-cr-1\": updated validation is too restrictive: [].spec.scalar: Invalid value: 2: spec.scalar in body should be greater than or equal to 3"),
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
client := fakedynamic.NewSimpleDynamicClient(runtime.NewScheme(), tt.existingObjects...)
require.Equal(t, tt.want, validateV1Beta1CRDCompatibility(client, tt.oldCRD, tt.newCRD))
})
}
}

func TestValidateV1CRDCompatibility(t *testing.T) {
unstructuredForFile := func(file string) *unstructured.Unstructured {
data, err := os.ReadFile(file)
require.NoError(t, err)
dec := utilyaml.NewYAMLOrJSONDecoder(strings.NewReader(string(data)), 30)
k8sFile := &unstructured.Unstructured{}
require.NoError(t, dec.Decode(k8sFile))
return k8sFile
}

unversionedCRDForV1File := func(file string) *apiextensionsv1.CustomResourceDefinition {
data, err := os.ReadFile(file)
require.NoError(t, err)
dec := utilyaml.NewYAMLOrJSONDecoder(strings.NewReader(string(data)), 30)
k8sFile := &apiextensionsv1.CustomResourceDefinition{}
require.NoError(t, dec.Decode(k8sFile))
return k8sFile
}

tests := []struct {
name string
existingCRs []runtime.Object
gvr schema.GroupVersionResource
oldCRD *apiextensionsv1.CustomResourceDefinition
newCRD *apiextensionsv1.CustomResourceDefinition
want error
}{
{
name: "valid",
existingCRs: []runtime.Object{
unstructuredForFile("testdata/apiextensionsv1/crontabs.cr.valid.v1.yaml"),
unstructuredForFile("testdata/apiextensionsv1/crontabs.cr.valid.v2.yaml"),
},
oldCRD: unversionedCRDForV1File("testdata/apiextensionsv1/crontabs.crd.old.yaml"),
newCRD: unversionedCRDForV1File("testdata/apiextensionsv1/crontabs.crd.yaml"),
},
{
name: "validation failure",
existingCRs: []runtime.Object{
unstructuredForFile("testdata/apiextensionsv1/crontabs.cr.valid.v1.yaml"),
unstructuredForFile("testdata/apiextensionsv1/crontabs.cr.fail.v2.yaml"),
},
oldCRD: unversionedCRDForV1File("testdata/apiextensionsv1/crontabs.crd.old.yaml"),
newCRD: unversionedCRDForV1File("testdata/apiextensionsv1/crontabs.crd.yaml"),
want: fmt.Errorf("error validating stable.example.com/v2, Kind=CronTab \"my-crontab\": updated validation is too restrictive: [].spec.replicas: Invalid value: 10: spec.replicas in body should be less than or equal to 9"),
},
{
name: "cr not invalidated by unserved version",
existingCRs: []runtime.Object{
unstructuredForFile("testdata/apiextensionsv1/crontabs.cr.valid.v1.yaml"),
unstructuredForFile("testdata/apiextensionsv1/crontabs.cr.valid.v2.yaml"),
},
oldCRD: unversionedCRDForV1File("testdata/apiextensionsv1/crontabs.crd.old.yaml"),
newCRD: unversionedCRDForV1File("testdata/apiextensionsv1/crontabs.crd.unserved.yaml"),
},
{
name: "cr not validated against currently unserved version",
existingCRs: []runtime.Object{
unstructuredForFile("testdata/apiextensionsv1/crontabs.cr.valid.v1.yaml"),
unstructuredForFile("testdata/apiextensionsv1/crontabs.cr.valid.v2.yaml"),
},
oldCRD: unversionedCRDForV1File("testdata/apiextensionsv1/crontabs.crd.old.unserved.yaml"),
newCRD: unversionedCRDForV1File("testdata/apiextensionsv1/crontabs.crd.yaml"),
},
{
name: "validation failure with single CRD version",
existingCRs: []runtime.Object{
unstructuredForFile("testdata/apiextensionsv1/single-version-cr.yaml"),
},
oldCRD: unversionedCRDForV1File("testdata/apiextensionsv1/single-version-crd.old.yaml"),
newCRD: unversionedCRDForV1File("testdata/apiextensionsv1/single-version-crd.yaml"),
want: fmt.Errorf("error validating cluster.com/v1alpha1, Kind=testcrd \"my-cr-1\": updated validation is too restrictive: [].spec.scalar: Invalid value: 100: spec.scalar in body should be less than or equal to 50"),
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
client := fakedynamic.NewSimpleDynamicClientWithCustomListKinds(runtime.NewScheme(), map[schema.GroupVersionResource]string{
tt.gvr: "UnstructuredList",
}, tt.existingObjects...)
require.Equal(t, tt.want, validateExistingCRs(client, tt.gvr, tt.newCRD))
client := fakedynamic.NewSimpleDynamicClient(runtime.NewScheme(), tt.existingCRs...)
require.Equal(t, tt.want, validateV1CRDCompatibility(client, tt.oldCRD, tt.newCRD))
})
}
}
Expand Down
8 changes: 8 additions & 0 deletions ...anager/pkg/controller/operators/catalog/testdata/apiextensionsv1/crontabs.cr.fail.v2.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
apiVersion: stable.example.com/v2
kind: CronTab
metadata:
name: my-crontab
spec:
cronSpec: "* * * * *"
image: ""
replicas: 10
8 changes: 8 additions & 0 deletions ...nager/pkg/controller/operators/catalog/testdata/apiextensionsv1/crontabs.cr.valid.v1.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
apiVersion: stable.example.com/v1
kind: CronTab
metadata:
name: my-crontab-v1
spec:
cronSpec: "* * * * *"
image: ""
replicas: 9
8 changes: 8 additions & 0 deletions ...nager/pkg/controller/operators/catalog/testdata/apiextensionsv1/crontabs.cr.valid.v2.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
apiVersion: stable.example.com/v2
kind: CronTab
metadata:
name: my-crontab-v2
spec:
cronSpec: "* * * * *"
image: ""
replicas: 9
Loading