(fix): unhandle changes for crd upgrade safety

camilamacedo86 · camilamacedo86 · commit 7cd292399b69 · 2025-09-03T09:00:30.000+01:00
- Keep unhandled spec changes as errors; message: "unhandled changes found"

Assisted-by: Cursor
diff --git a/internal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety.go b/internal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety.go
@@ -108,8 +108,9 @@ func (p *Preflight) runPreflight(ctx context.Context, rel *release.Release) erro
 			resultErrs := crdWideErrors(results)
 			resultErrs = append(resultErrs, sameVersionErrors(results)...)
 			resultErrs = append(resultErrs, servedVersionErrors(results)...)
-
-			validateErrors = append(validateErrors, fmt.Errorf("validating upgrade for CRD %q: %w", newCrd.Name, errors.Join(resultErrs...)))
+			if len(resultErrs) > 0 {
+				validateErrors = append(validateErrors, fmt.Errorf("validating upgrade for CRD %q: %w", newCrd.Name, errors.Join(resultErrs...)))
+			}
 		}
 	}
 
@@ -171,7 +172,11 @@ func sameVersionErrors(results *runner.Results) []error {
 		for property, comparisonResults := range propertyResults {
 			for _, result := range comparisonResults {
 				for _, err := range result.Errors {
-					errs = append(errs, fmt.Errorf("%s: %s: %s: %s", version, property, result.Name, err))
+					msg := err
+					if result.Name == "unhandled" {
+						msg = "unhandled changes found"
+					}
+					errs = append(errs, fmt.Errorf("%s: %s: %s: %s", version, property, result.Name, msg))
 				}
 			}
 		}
@@ -190,7 +195,11 @@ func servedVersionErrors(results *runner.Results) []error {
 		for property, comparisonResults := range propertyResults {
 			for _, result := range comparisonResults {
 				for _, err := range result.Errors {
-					errs = append(errs, fmt.Errorf("%s: %s: %s: %s", version, property, result.Name, err))
+					msg := err
+					if result.Name == "unhandled" {
+						msg = "unhandled changes found"
+					}
+					errs = append(errs, fmt.Errorf("%s: %s: %s: %s", version, property, result.Name, msg))
 				}
 			}
 		}
diff --git a/internal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety_test.go b/internal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety_test.go
@@ -15,6 +15,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
+	crdifyconfig "sigs.k8s.io/crdify/pkg/config"
 
 	"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/preflights/crdupgradesafety"
 	"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/util"
@@ -379,3 +380,37 @@ func TestUpgrade(t *testing.T) {
 		})
 	}
 }
+
+func TestUpgrade_UnhandledChanges_InSpec_DefaultPolicy(t *testing.T) {
+	t.Run("unhandled spec changes cause error by default", func(t *testing.T) {
+		preflight := newMockPreflight(getCrdFromManifestFile(t, "crd-unhandled-old.json"), nil)
+		rel := &release.Release{
+			Name:     "test-release",
+			Manifest: getManifestString(t, "crd-unhandled-new.json"),
+		}
+		err := preflight.Upgrade(context.Background(), rel)
+		require.Error(t, err)
+		require.ErrorContains(t, err, "unhandled changes found")
+		require.NotContains(t, err.Error(), ".status.", "status changes should be ignored")
+		require.NotContains(t, err.Error(), "v1.JSONSchemaProps", "error message should be concise without raw diff")
+	})
+}
+
+func TestUpgrade_UnhandledChanges_PolicyError(t *testing.T) {
+	t.Run("unhandled changes error when policy is Error", func(t *testing.T) {
+		oldCrd := getCrdFromManifestFile(t, "crd-unhandled-old.json")
+		preflight := crdupgradesafety.NewPreflight(&MockCRDGetter{oldCrd: oldCrd}, crdupgradesafety.WithConfig(&crdifyconfig.Config{
+			Conversion:           crdifyconfig.ConversionPolicyIgnore,
+			UnhandledEnforcement: crdifyconfig.EnforcementPolicyError,
+		}))
+
+		rel := &release.Release{
+			Name:     "test-release",
+			Manifest: getManifestString(t, "crd-unhandled-new.json"),
+		}
+
+		err := preflight.Upgrade(context.Background(), rel)
+		require.Error(t, err)
+		require.ErrorContains(t, err, "unhandled changes found")
+	})
+}
diff --git a/internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-status-change-new.json b/internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-status-change-new.json
@@ -0,0 +1,46 @@
+{
+  "apiVersion": "apiextensions.k8s.io/v1",
+  "kind": "CustomResourceDefinition",
+  "metadata": {"name": "apps.example.com"},
+  "spec": {
+    "group": "example.com",
+    "versions": [
+      {
+        "name": "v1alpha1",
+        "served": true,
+        "storage": true,
+        "schema": {
+          "openAPIV3Schema": {
+            "type": "object",
+            "properties": {
+              "spec": {
+                "type": "object",
+                "properties": {
+                  "name": {"type": "string"}
+                }
+              },
+              "status": {
+                "type": "object",
+                "properties": {
+                  "resources": {
+                    "type": "array",
+                    "items": {
+                      "type": "object",
+                      "properties": {
+                        "status": {"type": "string"},
+                        "syncWave": {"type": "integer", "format": "int64"}
+                      }
+                    }
+                  }
+                }
+              }
+            }
+          }
+        }
+      }
+    ],
+    "scope": "Namespaced",
+    "names": {"plural": "apps", "singular": "app", "kind": "App"}
+  }
+}
+
diff --git a/internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-status-change-old.json b/internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-status-change-old.json
@@ -0,0 +1,45 @@
+{
+  "apiVersion": "apiextensions.k8s.io/v1",
+  "kind": "CustomResourceDefinition",
+  "metadata": {"name": "apps.example.com"},
+  "spec": {
+    "group": "example.com",
+    "versions": [
+      {
+        "name": "v1alpha1",
+        "served": true,
+        "storage": true,
+        "schema": {
+          "openAPIV3Schema": {
+            "type": "object",
+            "properties": {
+              "spec": {
+                "type": "object",
+                "properties": {
+                  "name": {"type": "string"}
+                }
+              },
+              "status": {
+                "type": "object",
+                "properties": {
+                  "resources": {
+                    "type": "array",
+                    "items": {
+                      "type": "object",
+                      "properties": {
+                        "status": {"type": "string"}
+                      }
+                    }
+                  }
+                }
+              }
+            }
+          }
+        }
+      }
+    ],
+    "scope": "Namespaced",
+    "names": {"plural": "apps", "singular": "app", "kind": "App"}
+  }
+}
+
diff --git a/internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-unhandled-new.json b/internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-unhandled-new.json
@@ -0,0 +1,40 @@
+{
+  "apiVersion": "apiextensions.k8s.io/v1",
+  "kind": "CustomResourceDefinition",
+  "metadata": {
+    "name": "widgets.example.com"
+  },
+  "spec": {
+    "group": "example.com",
+    "versions": [
+      {
+        "name": "v1alpha1",
+        "served": true,
+        "storage": true,
+        "schema": {
+          "openAPIV3Schema": {
+            "type": "object",
+            "properties": {
+              "spec": {
+                "type": "object",
+                "properties": {
+                  "field": {
+                    "type": "string",
+                    "format": "email"
+                  }
+                }
+              }
+            }
+          }
+        }
+      }
+    ],
+    "scope": "Namespaced",
+    "names": {
+      "plural": "widgets",
+      "singular": "widget",
+      "kind": "Widget"
+    }
+  }
+}
+
diff --git a/internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-unhandled-old.json b/internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-unhandled-old.json
@@ -0,0 +1,39 @@
+{
+  "apiVersion": "apiextensions.k8s.io/v1",
+  "kind": "CustomResourceDefinition",
+  "metadata": {
+    "name": "widgets.example.com"
+  },
+  "spec": {
+    "group": "example.com",
+    "versions": [
+      {
+        "name": "v1alpha1",
+        "served": true,
+        "storage": true,
+        "schema": {
+          "openAPIV3Schema": {
+            "type": "object",
+            "properties": {
+              "spec": {
+                "type": "object",
+                "properties": {
+                  "field": {
+                    "type": "string"
+                  }
+                }
+              }
+            }
+          }
+        }
+      }
+    ],
+    "scope": "Namespaced",
+    "names": {
+      "plural": "widgets",
+      "singular": "widget",
+      "kind": "Widget"
+    }
+  }
+}
+

Original file line number	Diff line number	Diff line change
`@@ -108,8 +108,9 @@ func (p Preflight) runPreflight(ctx context.Context, rel release.Release) erro`
`108`	`108`	`resultErrs := crdWideErrors(results)`
`109`	`109`	`resultErrs = append(resultErrs, sameVersionErrors(results)...)`
`110`	`110`	`resultErrs = append(resultErrs, servedVersionErrors(results)...)`
`111`		`-`
`112`		`- validateErrors = append(validateErrors, fmt.Errorf("validating upgrade for CRD %q: %w", newCrd.Name, errors.Join(resultErrs...)))`
	`111`	`+ if len(resultErrs) > 0 {`
	`112`	`+ validateErrors = append(validateErrors, fmt.Errorf("validating upgrade for CRD %q: %w", newCrd.Name, errors.Join(resultErrs...)))`
	`113`	`+ }`
`113`	`114`	`}`
`114`	`115`	`}`
`115`	`116`
`@@ -171,7 +172,11 @@ func sameVersionErrors(results *runner.Results) []error {`
`171`	`172`	`for property, comparisonResults := range propertyResults {`
`172`	`173`	`for _, result := range comparisonResults {`
`173`	`174`	`for _, err := range result.Errors {`
`174`		`- errs = append(errs, fmt.Errorf("%s: %s: %s: %s", version, property, result.Name, err))`
	`175`	`+ msg := err`
	`176`	`+ if result.Name == "unhandled" {`
	`177`	`+ msg = "unhandled changes found"`
	`178`	`+ }`
	`179`	`+ errs = append(errs, fmt.Errorf("%s: %s: %s: %s", version, property, result.Name, msg))`
`175`	`180`	`}`
`176`	`181`	`}`
`177`	`182`	`}`
`@@ -190,7 +195,11 @@ func servedVersionErrors(results *runner.Results) []error {`
`190`	`195`	`for property, comparisonResults := range propertyResults {`
`191`	`196`	`for _, result := range comparisonResults {`
`192`	`197`	`for _, err := range result.Errors {`
`193`		`- errs = append(errs, fmt.Errorf("%s: %s: %s: %s", version, property, result.Name, err))`
	`198`	`+ msg := err`
	`199`	`+ if result.Name == "unhandled" {`
	`200`	`+ msg = "unhandled changes found"`
	`201`	`+ }`
	`202`	`+ errs = append(errs, fmt.Errorf("%s: %s: %s: %s", version, property, result.Name, msg))`
`194`	`203`	`}`
`195`	`204`	`}`
`196`	`205`	`}`