🐛 (fix): unhandle changes for crd upgrade safety ( OCPBUGS-59518 ) (#2179)

* (fix): unhandle changes for crd upgrade safety

- Keep unhandled spec changes as errors; message: "unhandled changes found"

Assisted-by: Cursor

* enhance to extract details from unhandle issue

Author: camilamacedo86

internal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"strings"
 
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	apiextensionsv1client "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset/typed/apiextensions/v1"
@@ -99,8 +100,9 @@ func (p *Preflight) runPreflight(ctx context.Context, relObjects []client.Object
 			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...)))
+			}
 		}
 	}
 
@@ -162,7 +164,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 = conciseUnhandledMessage(err)
+					}
+					errs = append(errs, fmt.Errorf("%s: %s: %s: %s", version, property, result.Name, msg))
 				}
 			}
 		}
@@ -181,11 +187,145 @@ 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 = conciseUnhandledMessage(err)
+					}
+					errs = append(errs, fmt.Errorf("%s: %s: %s: %s", version, property, result.Name, msg))
 				}
 			}
 		}
 	}
 
 	return errs
 }
+
+const unhandledSummaryPrefix = "unhandled changes found"
+
+// conciseUnhandledMessage trims the CRD diff emitted by crdify's "unhandled" comparator
+// into a short human readable description so operators get a hint of the change without
+// the unreadable Go struct dump.
+func conciseUnhandledMessage(raw string) string {
+	if !strings.Contains(raw, unhandledSummaryPrefix) {
+		return raw
+	}
+
+	details := extractUnhandledDetails(raw)
+	if len(details) == 0 {
+		return unhandledSummaryPrefix
+	}
+
+	return fmt.Sprintf("%s (%s)", unhandledSummaryPrefix, strings.Join(details, "; "))
+}
+
+func extractUnhandledDetails(raw string) []string {
+	type diffEntry struct {
+		before    string
+		after     string
+		beforeRaw string
+		afterRaw  string
+	}
+
+	entries := map[string]*diffEntry{}
+	order := []string{}
+
+	for _, line := range strings.Split(raw, "\n") {
+		trimmed := strings.TrimSpace(line)
+		if len(trimmed) < 2 {
+			continue
+		}
+
+		sign := trimmed[0]
+		if sign != '-' && sign != '+' {
+			continue
+		}
+
+		field, value, rawValue := parseUnhandledDiffValue(trimmed[1:])
+		if field == "" {
+			continue
+		}
+
+		entry, ok := entries[field]
+		if !ok {
+			entry = &diffEntry{}
+			entries[field] = entry
+			order = append(order, field)
+		}
+
+		if sign == '-' {
+			entry.before = value
+			entry.beforeRaw = rawValue
+		} else {
+			entry.after = value
+			entry.afterRaw = rawValue
+		}
+	}
+
+	details := []string{}
+	for _, field := range order {
+		entry := entries[field]
+		if entry.before == "" && entry.after == "" {
+			continue
+		}
+		if entry.before == entry.after && entry.beforeRaw == entry.afterRaw {
+			continue
+		}
+
+		before := entry.before
+		if before == "" {
+			before = "<empty>"
+		}
+		after := entry.after
+		if after == "" {
+			after = "<empty>"
+		}
+		if entry.before == entry.after && entry.beforeRaw != entry.afterRaw {
+			after = after + " (changed)"
+		}
+
+		details = append(details, fmt.Sprintf("%s %s -> %s", field, before, after))
+	}
+
+	return details
+}
+
+func parseUnhandledDiffValue(fragment string) (string, string, string) {
+	cleaned := strings.TrimSpace(fragment)
+	cleaned = strings.TrimPrefix(cleaned, "\t")
+	cleaned = strings.TrimSpace(cleaned)
+	cleaned = strings.TrimSuffix(cleaned, ",")
+
+	parts := strings.SplitN(cleaned, ":", 2)
+	if len(parts) != 2 {
+		return "", "", ""
+	}
+
+	field := strings.TrimSpace(parts[0])
+	rawValue := strings.TrimSpace(parts[1])
+	value := normalizeUnhandledValue(rawValue)
+
+	if field == "" {
+		return "", "", ""
+	}
+
+	return field, value, rawValue
+}
+
+func normalizeUnhandledValue(value string) string {
+	value = strings.TrimSuffix(value, ",")
+	value = strings.TrimSpace(value)
+
+	switch value {
+	case "":
+		return "<empty>"
+	case "\"\"":
+		return "\"\""
+	}
+
+	value = strings.ReplaceAll(value, "v1.", "")
+	if strings.Contains(value, "JSONSchemaProps") {
+		return "<complex value>"
+	}
+
+	return value
+}

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/applier"
 	"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/preflights/crdupgradesafety"
@@ -386,3 +387,42 @@ 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"),
+		}
+		objs, err := applier.HelmReleaseToObjectsConverter{}.GetObjectsFromRelease(rel)
+		require.NoError(t, err)
+		err = preflight.Upgrade(context.Background(), objs)
+		require.Error(t, err)
+		require.ErrorContains(t, err, "unhandled changes found")
+		require.ErrorContains(t, err, "Format \"\" -> \"email\"")
+		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"),
+		}
+
+		objs, err := applier.HelmReleaseToObjectsConverter{}.GetObjectsFromRelease(rel)
+		require.NoError(t, err)
+		err = preflight.Upgrade(context.Background(), objs)
+		require.Error(t, err)
+		require.ErrorContains(t, err, "unhandled changes found")
+		require.ErrorContains(t, err, "Format \"\" -> \"email\"")
+	})
+}

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"
+    }
+  }
+}
+

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"
+    }
+  }
+}
+

internal/operator-controller/rukpak/preflights/crdupgradesafety/unhandled_message_test.go

@@ -0,0 +1,28 @@
+package crdupgradesafety
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestConciseUnhandledMessage_NoPrefix(t *testing.T) {
+	raw := "some other error"
+	require.Equal(t, raw, conciseUnhandledMessage(raw))
+}
+
+func TestConciseUnhandledMessage_SingleChange(t *testing.T) {
+	raw := "unhandled changes found :\n- Format: \"\"\n+ Format: \"email\"\n"
+	require.Equal(t, "unhandled changes found (Format \"\" -> \"email\")", conciseUnhandledMessage(raw))
+}
+
+func TestConciseUnhandledMessage_MultipleChanges(t *testing.T) {
+	raw := "unhandled changes found :\n- Format: \"\"\n+ Format: \"email\"\n- Default: nil\n+ Default: \"value\"\n- Title: \"\"\n+ Title: \"Widget\"\n- Description: \"old\"\n+ Description: \"new\"\n"
+	got := conciseUnhandledMessage(raw)
+	require.Equal(t, "unhandled changes found (Format \"\" -> \"email\"; Default nil -> \"value\"; Title \"\" -> \"Widget\"; Description \"old\" -> \"new\")", got)
+}
+
+func TestConciseUnhandledMessage_SkipComplexValues(t *testing.T) {
+	raw := "unhandled changes found :\n- Default: &v1.JSONSchemaProps{}\n+ Default: &v1.JSONSchemaProps{Type: \"string\"}\n"
+	require.Equal(t, "unhandled changes found (Default <complex value> -> <complex value> (changed))", conciseUnhandledMessage(raw))
+}