🐛 CRD upgrade safety ratcheting

go.mod

@@ -43,10 +43,12 @@ require (
 	k8s.io/utils v0.0.0-20250604170112-4c0f3b243397
 	sigs.k8s.io/controller-runtime v0.21.0
 	sigs.k8s.io/controller-tools v0.18.0
-	sigs.k8s.io/crdify v0.4.1-0.20250613143457-398e4483fb58
+	sigs.k8s.io/crdify v0.4.1-0.20250724144029-7e14d68ee765
 	sigs.k8s.io/yaml v1.6.0
 )
 
+replace sigs.k8s.io/crdify => github.com/joelanford/crdify v0.4.1-0.20250728172651-d757a012127f
+
 require (
 	k8s.io/component-helpers v0.33.2 // indirect
 	k8s.io/kube-openapi v0.0.0-20250610211856-8b98d1ed966a // indirect

go.sum

@@ -275,6 +275,8 @@ github.com/jmhodges/clock v1.2.0 h1:eq4kys+NI0PLngzaHEe7AmPT90XMGIEySD1JfV1PDIs=
 github.com/jmhodges/clock v1.2.0/go.mod h1:qKjhA7x7u/lQpPB1XAqX1b1lCI/w3/fNuYpI/ZjLynI=
 github.com/jmoiron/sqlx v1.4.0 h1:1PLqN7S1UYp5t4SrVVnt4nUVNemrDAtxlulVe+Qgm3o=
 github.com/jmoiron/sqlx v1.4.0/go.mod h1:ZrZ7UsYB/weZdl2Bxg6jCRO9c3YHl8r3ahlKmRT4JLY=
+github.com/joelanford/crdify v0.4.1-0.20250728172651-d757a012127f h1:VEKhNZW1Rcu2V9QDZm6zvYoMEewHiH8kbTLbj+EBB54=
+github.com/joelanford/crdify v0.4.1-0.20250728172651-d757a012127f/go.mod h1:ZIFxaYNgKYmFtZCLPysncXQ8oqwnNlHQbRUfxJHZwzU=
 github.com/joelanford/ignore v0.1.1 h1:vKky5RDoPT+WbONrbQBgOn95VV/UPh4ejlyAbbzgnQk=
 github.com/joelanford/ignore v0.1.1/go.mod h1:8eho/D8fwQ3rIXrLwE23AaeaGDNXqLE9QJ3zJ4LIPCw=
 github.com/josharian/intern v1.0.0 h1:vlS4z54oSdjm0bgjRigI+G1HpF+tI+9rE5LLzOg8HmY=
@@ -765,8 +767,6 @@ sigs.k8s.io/controller-runtime v0.21.0 h1:CYfjpEuicjUecRk+KAeyYh+ouUBn4llGyDYytI
 sigs.k8s.io/controller-runtime v0.21.0/go.mod h1:OSg14+F65eWqIu4DceX7k/+QRAbTTvxeQSNSOQpukWM=
 sigs.k8s.io/controller-tools v0.18.0 h1:rGxGZCZTV2wJreeRgqVoWab/mfcumTMmSwKzoM9xrsE=
 sigs.k8s.io/controller-tools v0.18.0/go.mod h1:gLKoiGBriyNh+x1rWtUQnakUYEujErjXs9pf+x/8n1U=
-sigs.k8s.io/crdify v0.4.1-0.20250613143457-398e4483fb58 h1:VTvhbqgZMVoDpHHPuZLaOgzjjsJBhO8+vDKA1COuLCY=
-sigs.k8s.io/crdify v0.4.1-0.20250613143457-398e4483fb58/go.mod h1:ZIFxaYNgKYmFtZCLPysncXQ8oqwnNlHQbRUfxJHZwzU=
 sigs.k8s.io/gateway-api v1.1.0 h1:DsLDXCi6jR+Xz8/xd0Z1PYl2Pn0TyaFMOPPZIj4inDM=
 sigs.k8s.io/gateway-api v1.1.0/go.mod h1:ZH4lHrL2sDi0FHZ9jjneb8kKnGzFWyrTya35sWUTrRs=
 sigs.k8s.io/json v0.0.0-20241014173422-cfa47c3a1cc8 h1:gBQPwqORJ8d8/YNZWEjoZs7npUVDpVXUUOFfW6CgAqE=

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

@@ -38,7 +38,7 @@ func newMockPreflight(crd *apiextensionsv1.CustomResourceDefinition, err error)
 	}, preflightOpts...)
 }
 
-const crdFolder string = "../../../../../testdata/manifests"
+const crdFolder string = "testdata/manifests"
 
 func getCrdFromManifestFile(t *testing.T, oldCrdFile string) *apiextensionsv1.CustomResourceDefinition {
 	if oldCrdFile == "" {
@@ -66,14 +66,22 @@ func getManifestString(t *testing.T, crdFile string) string {
 	return string(buff)
 }
 
+func wantErrorMsgs(wantMsgs []string) require.ErrorAssertionFunc {
+	return func(t require.TestingT, haveErr error, _ ...interface{}) {
+		for _, wantMsg := range wantMsgs {
+			require.ErrorContains(t, haveErr, wantMsg)
+		}
+	}
+}
+
 // TestInstall exists only for completeness as Install() is currently a no-op. It can be used as
 // a template for real tests in the future if the func is implemented.
 func TestInstall(t *testing.T) {
 	tests := []struct {
 		name          string
 		oldCrdPath    string
 		release       *release.Release
-		wantErrMsgs   []string
+		requireErr    require.ErrorAssertionFunc
 		wantCrdGetErr error
 	}{
 		{
@@ -91,7 +99,7 @@ func TestInstall(t *testing.T) {
 				Name:     "test-release",
 				Manifest: "abcd",
 			},
-			wantErrMsgs: []string{"json: cannot unmarshal string into Go value of type unstructured.detector"},
+			requireErr: wantErrorMsgs([]string{"json: cannot unmarshal string into Go value of type unstructured.detector"}),
 		},
 		{
 			name: "release with no CRD objects",
@@ -107,7 +115,7 @@ func TestInstall(t *testing.T) {
 				Manifest: getManifestString(t, "crd-valid-upgrade.json"),
 			},
 			wantCrdGetErr: fmt.Errorf("error!"),
-			wantErrMsgs:   []string{"error!"},
+			requireErr:    wantErrorMsgs([]string{"error!"}),
 		},
 		{
 			name: "fail to get old crd, not found error",
@@ -123,7 +131,7 @@ func TestInstall(t *testing.T) {
 				Name:     "test-release",
 				Manifest: getManifestString(t, "crd-invalid"),
 			},
-			wantErrMsgs: []string{"json: cannot unmarshal"},
+			requireErr: wantErrorMsgs([]string{"json: cannot unmarshal"}),
 		},
 		{
 			name:       "valid upgrade",
@@ -142,7 +150,7 @@ func TestInstall(t *testing.T) {
 				Name:     "test-release",
 				Manifest: getManifestString(t, "crd-invalid-upgrade.json"),
 			},
-			wantErrMsgs: []string{
+			requireErr: wantErrorMsgs([]string{
 				`scope:`,
 				`storedVersionRemoval:`,
 				`enum:`,
@@ -156,7 +164,7 @@ func TestInstall(t *testing.T) {
 				`minLength:`,
 				`minProperties:`,
 				`default:`,
-			},
+			}),
 		},
 		{
 			name: "new crd validation failure for existing field removal",
@@ -167,9 +175,9 @@ func TestInstall(t *testing.T) {
 				Name:     "test-release",
 				Manifest: getManifestString(t, "crd-field-removed.json"),
 			},
-			wantErrMsgs: []string{
+			requireErr: wantErrorMsgs([]string{
 				`existingFieldRemoval:`,
-			},
+			}),
 		},
 		{
 			name: "new crd validation should not fail on description changes",
@@ -187,10 +195,8 @@ func TestInstall(t *testing.T) {
 		t.Run(tc.name, func(t *testing.T) {
 			preflight := newMockPreflight(getCrdFromManifestFile(t, tc.oldCrdPath), tc.wantCrdGetErr)
 			err := preflight.Install(context.Background(), tc.release)
-			if len(tc.wantErrMsgs) != 0 {
-				for _, expectedErrMsg := range tc.wantErrMsgs {
-					require.ErrorContains(t, err, expectedErrMsg)
-				}
+			if tc.requireErr != nil {
+				tc.requireErr(t, err)
 			} else {
 				require.NoError(t, err)
 			}
@@ -203,7 +209,7 @@ func TestUpgrade(t *testing.T) {
 		name          string
 		oldCrdPath    string
 		release       *release.Release
-		wantErrMsgs   []string
+		requireErr    require.ErrorAssertionFunc
 		wantCrdGetErr error
 	}{
 		{
@@ -221,7 +227,7 @@ func TestUpgrade(t *testing.T) {
 				Name:     "test-release",
 				Manifest: "abcd",
 			},
-			wantErrMsgs: []string{"json: cannot unmarshal string into Go value of type unstructured.detector"},
+			requireErr: wantErrorMsgs([]string{"json: cannot unmarshal string into Go value of type unstructured.detector"}),
 		},
 		{
 			name: "release with no CRD objects",
@@ -237,7 +243,7 @@ func TestUpgrade(t *testing.T) {
 				Manifest: getManifestString(t, "crd-valid-upgrade.json"),
 			},
 			wantCrdGetErr: fmt.Errorf("error!"),
-			wantErrMsgs:   []string{"error!"},
+			requireErr:    wantErrorMsgs([]string{"error!"}),
 		},
 		{
 			name: "fail to get old crd, not found error",
@@ -253,7 +259,7 @@ func TestUpgrade(t *testing.T) {
 				Name:     "test-release",
 				Manifest: getManifestString(t, "crd-invalid"),
 			},
-			wantErrMsgs: []string{"json: cannot unmarshal"},
+			requireErr: wantErrorMsgs([]string{"json: cannot unmarshal"}),
 		},
 		{
 			name:       "valid upgrade",
@@ -272,7 +278,7 @@ func TestUpgrade(t *testing.T) {
 				Name:     "test-release",
 				Manifest: getManifestString(t, "crd-invalid-upgrade.json"),
 			},
-			wantErrMsgs: []string{
+			requireErr: wantErrorMsgs([]string{
 				`scope:`,
 				`storedVersionRemoval:`,
 				`enum:`,
@@ -286,7 +292,7 @@ func TestUpgrade(t *testing.T) {
 				`minLength:`,
 				`minProperties:`,
 				`default:`,
-			},
+			}),
 		},
 		{
 			name: "new crd validation failure for existing field removal",
@@ -297,9 +303,9 @@ func TestUpgrade(t *testing.T) {
 				Name:     "test-release",
 				Manifest: getManifestString(t, "crd-field-removed.json"),
 			},
-			wantErrMsgs: []string{
+			requireErr: wantErrorMsgs([]string{
 				`existingFieldRemoval:`,
-			},
+			}),
 		},
 		{
 			name:       "webhook conversion strategy exists",
@@ -316,9 +322,9 @@ func TestUpgrade(t *testing.T) {
 				Name:     "test-release",
 				Manifest: getManifestString(t, "crd-conversion-no-webhook.json"),
 			},
-			wantErrMsgs: []string{
+			requireErr: wantErrorMsgs([]string{
 				`validating upgrade for CRD "crontabs.stable.example.com": v1 <-> v2: ^.spec.foobarbaz: enum: allowed enum values removed`,
-			},
+			}),
 		},
 		{
 			name: "new crd validation should not fail on description changes",
@@ -330,16 +336,43 @@ func TestUpgrade(t *testing.T) {
 				Manifest: getManifestString(t, "crd-description-changed.json"),
 			},
 		},
+		{
+			name:       "success when old crd and new crd contain the exact same validation issues",
+			oldCrdPath: "crd-conversion-no-webhook.json",
+			release: &release.Release{
+				Name:     "test-release",
+				Manifest: getManifestString(t, "crd-conversion-no-webhook.json"),
+			},
+		},
+		{
+			name:       "failure when old crd and new crd contain the exact same validation issues, but new crd introduces another validation issue",
+			oldCrdPath: "crd-conversion-no-webhook.json",
+			release: &release.Release{
+				Name:     "test-release",
+				Manifest: getManifestString(t, "crd-conversion-no-webhook-extra-issue.json"),
+			},
+			requireErr: func(t require.TestingT, err error, _ ...interface{}) {
+				require.ErrorContains(t, err,
+					`validating upgrade for CRD "crontabs.stable.example.com":`,
+				)
+				// The newly introduced issue is reported
+				require.Contains(t, err.Error(),
+					`v1 <-> v2: ^.spec.extraField: type: type changed : "boolean" -> "string"`,
+				)
+				// The existing issue is not reported
+				require.NotContains(t, err.Error(),
+					`v1 <-> v2: ^.spec.foobarbaz: enum: allowed enum values removed`,
+				)
+			},
+		},
 	}
 
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {
 			preflight := newMockPreflight(getCrdFromManifestFile(t, tc.oldCrdPath), tc.wantCrdGetErr)
 			err := preflight.Upgrade(context.Background(), tc.release)
-			if len(tc.wantErrMsgs) != 0 {
-				for _, expectedErrMsg := range tc.wantErrMsgs {
-					require.ErrorContains(t, err, expectedErrMsg)
-				}
+			if tc.requireErr != nil {
+				tc.requireErr(t, err)
 			} else {
 				require.NoError(t, err)
 			}

internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-conversion-no-webhook-extra-issue.json

@@ -0,0 +1,76 @@
+{
+    "apiVersion": "apiextensions.k8s.io/v1",
+    "kind": "CustomResourceDefinition",
+    "metadata": {
+        "name": "crontabs.stable.example.com"
+    },
+    "spec": {
+        "group": "stable.example.com",
+        "versions": [
+            {
+                "name": "v2",
+                "served": true,
+                "storage": false,
+                "schema": {
+                    "openAPIV3Schema": {
+                        "type": "object",
+                        "properties": {
+                            "spec": {
+                                "type": "object",
+                                "properties": {
+                                    "foobarbaz": {
+                                        "type":"string",
+                                        "enum":[
+                                            "bark",
+                                            "woof"
+                                        ]
+                                    },
+                                    "extraField": {
+                                        "type":"string"
+                                    }
+                                }
+                            }
+                        }
+                    }
+                }
+            },
+            {
+                "name": "v1",
+                "served": true,
+                "storage": false,
+                "schema": {
+                    "openAPIV3Schema": {
+                        "type": "object",
+                        "properties": {
+                            "spec": {
+                                "type": "object",
+                                "properties": {
+                                    "foobarbaz": {
+                                        "type":"string",
+                                        "enum":[
+                                            "foo",
+                                            "bar",
+                                            "baz"
+                                        ]
+                                    },
+                                    "extraField": {
+                                        "type":"boolean"
+                                    }
+                                }
+                            }
+                        }
+                    }
+                }
+            }
+        ],
+        "scope": "Cluster",
+        "names": {
+            "plural": "crontabs",
+            "singular": "crontab",
+            "kind": "CronTab",
+            "shortNames": [
+                "ct"
+            ]
+        }
+    }
+}