diff --git a/internal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety.go b/internal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety.go index f058b44825..e7830ce620 100644 --- a/internal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety.go +++ b/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,7 +187,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 = conciseUnhandledMessage(err) + } + errs = append(errs, fmt.Errorf("%s: %s: %s: %s", version, property, result.Name, msg)) } } } @@ -189,3 +199,133 @@ func servedVersionErrors(results *runner.Results) []error { 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 = "" + } + after := entry.after + if after == "" { + after = "" + } + 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 "" + case "\"\"": + return "\"\"" + } + + value = strings.ReplaceAll(value, "v1.", "") + if strings.Contains(value, "JSONSchemaProps") { + return "" + } + + return value +} diff --git a/internal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety_test.go b/internal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety_test.go index d91da7402f..9fb275880d 100644 --- 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/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\"") + }) +} 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 new file mode 100644 index 0000000000..6fed77fc16 --- /dev/null +++ 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 new file mode 100644 index 0000000000..a87fbd5054 --- /dev/null +++ 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" + } + } +} + diff --git a/internal/operator-controller/rukpak/preflights/crdupgradesafety/unhandled_message_test.go b/internal/operator-controller/rukpak/preflights/crdupgradesafety/unhandled_message_test.go new file mode 100644 index 0000000000..59078655a0 --- /dev/null +++ b/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 -> (changed))", conciseUnhandledMessage(raw)) +}