Skip to content

Commit

Permalink
Correct CRD validation to validate CRs against each version of the up…
Browse files Browse the repository at this point in the history 
…dated CRD

Signed-off-by: Daniel Franz <[email protected]>
  • Loading branch information
@dtfranz
dtfranz committed Oct 5, 2023
1 parent 44935f1 commit 15dd508
Show file tree
Hide file tree
Showing 19 changed files with 690 additions and 64 deletions.
130 changes: 78 additions & 52 deletions pkg/controller/operators/catalog/operator.go
View file
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 @@ -2013,81 +2014,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 pkg/controller/operators/catalog/operator_test.go
View file
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 pkg/controller/operators/catalog/testdata/apiextensionsv1/crontabs.cr.fail.v2.yaml
View file
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 pkg/controller/operators/catalog/testdata/apiextensionsv1/crontabs.cr.valid.v1.yaml
View file
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 pkg/controller/operators/catalog/testdata/apiextensionsv1/crontabs.cr.valid.v2.yaml
View file
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

0 comments on commit 15dd508

Please sign in to comment.