OCPBUGS-53312: [release-4.18] 🐛 fix crdupgradesafety diff when items schema differ (#1863)

go.mod

@@ -3,7 +3,6 @@ module github.com/operator-framework/operator-controller
 go 1.22.5
 
 require (
-	carvel.dev/kapp v0.63.3
 	github.com/BurntSushi/toml v1.4.0
 	github.com/Masterminds/semver/v3 v3.3.0
 	github.com/blang/semver/v4 v4.0.0
@@ -16,6 +15,7 @@ require (
 	github.com/onsi/ginkgo/v2 v2.21.0
 	github.com/onsi/gomega v1.35.1
 	github.com/opencontainers/go-digest v1.0.0
+	github.com/openshift/crd-schema-checker v0.0.0-20240404194209-35a9033b1d11
 	github.com/operator-framework/api v0.27.0
 	github.com/operator-framework/catalogd v1.0.0-rc1
 	github.com/operator-framework/helm-operator-plugins v0.7.0
@@ -38,7 +38,6 @@ require (
 )
 
 require (
-	carvel.dev/vendir v0.40.0 // indirect
 	dario.cat/mergo v1.0.1 // indirect
 	github.com/AdaLogics/go-fuzz-headers v0.0.0-20230811130428-ced1acdcaa24 // indirect
 	github.com/Azure/go-ansiterm v0.0.0-20230124172434-306776ec8161 // indirect
@@ -69,9 +68,6 @@ require (
 	github.com/containers/libtrust v0.0.0-20230121012942-c1716e8a8d01 // indirect
 	github.com/containers/ocicrypt v1.2.0 // indirect
 	github.com/containers/storage v1.55.0 // indirect
-	github.com/cppforlife/cobrautil v0.0.0-20221130162803-acdfead391ef // indirect
-	github.com/cppforlife/color v1.9.1-0.20200716202919-6706ac40b835 // indirect
-	github.com/cppforlife/go-cli-ui v0.0.0-20220425131040-94f26b16bc14 // indirect
 	github.com/cyberphone/json-canonicalization v0.0.0-20231217050601-ba74d44ecf5f // indirect
 	github.com/cyphar/filepath-securejoin v0.3.6 // indirect
 	github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc // indirect
@@ -128,7 +124,6 @@ require (
 	github.com/h2non/go-is-svg v0.0.0-20160927212452-35e8c4b0612c // indirect
 	github.com/hashicorp/errwrap v1.1.0 // indirect
 	github.com/hashicorp/go-multierror v1.1.1 // indirect
-	github.com/hashicorp/go-version v1.6.0 // indirect
 	github.com/huandu/xstrings v1.5.0 // indirect
 	github.com/imdario/mergo v0.3.16 // indirect
 	github.com/inconshreveable/mousetrap v1.1.0 // indirect
@@ -137,8 +132,6 @@ require (
 	github.com/joelanford/ignore v0.1.1 // indirect
 	github.com/josharian/intern v1.0.0 // indirect
 	github.com/json-iterator/go v1.1.12 // indirect
-	github.com/k14s/starlark-go v0.0.0-20200720175618-3a5c849cc368 // indirect
-	github.com/k14s/ytt v0.36.0 // indirect
 	github.com/klauspost/compress v1.17.11 // indirect
 	github.com/klauspost/pgzip v1.2.6 // indirect
 	github.com/lann/builder v0.0.0-20180802200727-47ae307949d0 // indirect
@@ -172,7 +165,6 @@ require (
 	github.com/oklog/ulid v1.3.1 // indirect
 	github.com/opencontainers/image-spec v1.1.0 // indirect
 	github.com/opencontainers/runtime-spec v1.2.0 // indirect
-	github.com/openshift/crd-schema-checker v0.0.0-20240404194209-35a9033b1d11 // indirect
 	github.com/operator-framework/operator-lib v0.15.0 // indirect
 	github.com/otiai10/copy v1.14.0 // indirect
 	github.com/peterbourgon/diskv v2.0.1+incompatible // indirect
@@ -202,8 +194,6 @@ require (
 	github.com/ulikunitz/xz v0.5.12 // indirect
 	github.com/vbatts/tar-split v0.11.5 // indirect
 	github.com/vbauerster/mpb/v8 v8.7.5 // indirect
-	github.com/vito/go-interact v1.0.1 // indirect
-	github.com/vmware-tanzu/carvel-kapp-controller v0.51.0 // indirect
 	github.com/x448/float16 v0.8.4 // indirect
 	github.com/xeipuuv/gojsonpointer v0.0.0-20190905194746-02993c407bfb // indirect
 	github.com/xeipuuv/gojsonreference v0.0.0-20180127040603-bd5ef7bd5415 // indirect

internal/rukpak/preflights/crdupgradesafety/change_validator.go

@@ -0,0 +1,167 @@
+// Originally copied from https://github.com/carvel-dev/kapp/tree/d7fc2e15439331aa3a379485bb124e91a0829d2e
+// Attribution:
+//   Copyright 2024 The Carvel Authors.
+//   SPDX-License-Identifier: Apache-2.0
+
+package crdupgradesafety
+
+import (
+	"errors"
+	"fmt"
+	"reflect"
+
+	"github.com/openshift/crd-schema-checker/pkg/manifestcomparators"
+	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
+	"k8s.io/apimachinery/pkg/util/validation/field"
+)
+
+// ChangeValidation is a function that accepts a FieldDiff
+// as a parameter and should return:
+// - a boolean representation of whether or not the change
+// - an error if the change would be unsafe
+// has been fully handled (i.e no additional changes exist)
+type ChangeValidation func(diff FieldDiff) (bool, error)
+
+// ChangeValidator is a Validation implementation focused on
+// handling updates to existing fields in a CRD
+type ChangeValidator struct {
+	// Validations is a slice of ChangeValidations
+	// to run against each changed field
+	Validations []ChangeValidation
+}
+
+func (cv *ChangeValidator) Name() string {
+	return "ChangeValidator"
+}
+
+// Validate will compare each version in the provided existing and new CRDs.
+// Since the ChangeValidator is tailored to handling updates to existing fields in
+// each version of a CRD. As such the following is assumed:
+// - Validating the removal of versions during an update is handled outside of this
+// validator. If a version in the existing version of the CRD does not exist in the new
+// version that version of the CRD is skipped in this validator.
+// - Removal of existing fields is unsafe. Regardless of whether or not this is handled
+// by a validator outside this one, if a field is present in a version provided by the existing CRD
+// but not present in the same version provided by the new CRD this validation will fail.
+//
+// Additionally, any changes that are not validated and handled by the known ChangeValidations
+// are deemed as unsafe and returns an error.
+func (cv *ChangeValidator) Validate(old, new apiextensionsv1.CustomResourceDefinition) error {
+	errs := []error{}
+	for _, version := range old.Spec.Versions {
+		newVersion := manifestcomparators.GetVersionByName(&new, version.Name)
+		if newVersion == nil {
+			// if the new version doesn't exist skip this version
+			continue
+		}
+		flatOld := FlattenSchema(version.Schema.OpenAPIV3Schema)
+		flatNew := FlattenSchema(newVersion.Schema.OpenAPIV3Schema)
+
+		diffs, err := CalculateFlatSchemaDiff(flatOld, flatNew)
+		if err != nil {
+			errs = append(errs, fmt.Errorf("calculating schema diff for CRD version %q", version.Name))
+			continue
+		}
+
+		for field, diff := range diffs {
+			handled := false
+			for _, validation := range cv.Validations {
+				ok, err := validation(diff)
+				if err != nil {
+					errs = append(errs, fmt.Errorf("version %q, field %q: %w", version.Name, field, err))
+				}
+				if ok {
+					handled = true
+					break
+				}
+			}
+
+			if !handled {
+				errs = append(errs, fmt.Errorf("version %q, field %q has unknown change, refusing to determine that change is safe", version.Name, field))
+			}
+		}
+	}
+
+	if len(errs) > 0 {
+		return errors.Join(errs...)
+	}
+	return nil
+}
+
+type FieldDiff struct {
+	Old *apiextensionsv1.JSONSchemaProps
+	New *apiextensionsv1.JSONSchemaProps
+}
+
+// FlatSchema is a flat representation of a CRD schema.
+type FlatSchema map[string]*apiextensionsv1.JSONSchemaProps
+
+// FlattenSchema takes in a CRD version OpenAPIV3Schema and returns
+// a flattened representation of it. For example, a CRD with a schema of:
+// ```yaml
+//
+//	...
+//	spec:
+//	  type: object
+//	  properties:
+//	    foo:
+//	      type: string
+//	    bar:
+//	      type: string
+//	...
+//
+// ```
+// would be represented as:
+//
+//	map[string]*apiextensionsv1.JSONSchemaProps{
+//	   "^": {},
+//	   "^.spec": {},
+//	   "^.spec.foo": {},
+//	   "^.spec.bar": {},
+//	}
+//
+// where "^" represents the "root" schema
+func FlattenSchema(schema *apiextensionsv1.JSONSchemaProps) FlatSchema {
+	fieldMap := map[string]*apiextensionsv1.JSONSchemaProps{}
+
+	manifestcomparators.SchemaHas(schema,
+		field.NewPath("^"),
+		field.NewPath("^"),
+		nil,
+		func(s *apiextensionsv1.JSONSchemaProps, _, simpleLocation *field.Path, _ []*apiextensionsv1.JSONSchemaProps) bool {
+			fieldMap[simpleLocation.String()] = s.DeepCopy()
+			return false
+		})
+
+	return fieldMap
+}
+
+// CalculateFlatSchemaDiff finds fields in a FlatSchema that are different
+// and returns a mapping of field --> old and new field schemas. If a field
+// exists in the old FlatSchema but not the new an empty diff mapping and an error is returned.
+func CalculateFlatSchemaDiff(o, n FlatSchema) (map[string]FieldDiff, error) {
+	diffMap := map[string]FieldDiff{}
+	for field, schema := range o {
+		if _, ok := n[field]; !ok {
+			return diffMap, fmt.Errorf("field %q in existing not found in new", field)
+		}
+		newSchema := n[field]
+
+		// Copy the schemas and remove any child properties for comparison.
+		// In theory this will focus in on detecting changes for only the
+		// field we are looking at and ignore changes in the children fields.
+		// Since we are iterating through the map that should have all fields
+		// we should still detect changes in the children fields.
+		oldCopy := schema.DeepCopy()
+		newCopy := newSchema.DeepCopy()
+		oldCopy.Properties, oldCopy.Items = nil, nil
+		newCopy.Properties, newCopy.Items = nil, nil
+		if !reflect.DeepEqual(oldCopy, newCopy) {
+			diffMap[field] = FieldDiff{
+				Old: oldCopy,
+				New: newCopy,
+			}
+		}
+	}
+	return diffMap, nil
+}