From c6762502f10331b2f940e0121fb4674f79443564 Mon Sep 17 00:00:00 2001 From: Joe Lanford Date: Mon, 28 Jul 2025 20:45:52 -0400 Subject: [PATCH 1/3] move crd upgrade safety testdata into crdupgradesafety package Signed-off-by: Joe Lanford --- .../rukpak/preflights/crdupgradesafety/crdupgradesafety_test.go | 2 +- .../testdata}/manifests/crd-conversion-no-webhook.json | 0 .../testdata}/manifests/crd-conversion-webhook-old.json | 0 .../testdata}/manifests/crd-conversion-webhook.json | 0 .../testdata}/manifests/crd-description-changed.json | 0 .../crdupgradesafety/testdata}/manifests/crd-field-removed.json | 0 .../preflights/crdupgradesafety/testdata}/manifests/crd-invalid | 0 .../testdata}/manifests/crd-invalid-upgrade.json | 0 .../crdupgradesafety/testdata}/manifests/crd-valid-upgrade.json | 0 .../crdupgradesafety/testdata}/manifests/no-crds.json | 0 .../crdupgradesafety/testdata}/manifests/old-crd.json | 0 11 files changed, 1 insertion(+), 1 deletion(-) rename {testdata => internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata}/manifests/crd-conversion-no-webhook.json (100%) rename {testdata => internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata}/manifests/crd-conversion-webhook-old.json (100%) rename {testdata => internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata}/manifests/crd-conversion-webhook.json (100%) rename {testdata => internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata}/manifests/crd-description-changed.json (100%) rename {testdata => internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata}/manifests/crd-field-removed.json (100%) rename {testdata => internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata}/manifests/crd-invalid (100%) rename {testdata => internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata}/manifests/crd-invalid-upgrade.json (100%) rename {testdata => internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata}/manifests/crd-valid-upgrade.json (100%) rename {testdata => internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata}/manifests/no-crds.json (100%) rename {testdata => internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata}/manifests/old-crd.json (100%) diff --git a/internal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety_test.go b/internal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety_test.go index 73db9673b..25720009c 100644 --- a/internal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety_test.go +++ b/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 == "" { diff --git a/testdata/manifests/crd-conversion-no-webhook.json b/internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-conversion-no-webhook.json similarity index 100% rename from testdata/manifests/crd-conversion-no-webhook.json rename to internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-conversion-no-webhook.json diff --git a/testdata/manifests/crd-conversion-webhook-old.json b/internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-conversion-webhook-old.json similarity index 100% rename from testdata/manifests/crd-conversion-webhook-old.json rename to internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-conversion-webhook-old.json diff --git a/testdata/manifests/crd-conversion-webhook.json b/internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-conversion-webhook.json similarity index 100% rename from testdata/manifests/crd-conversion-webhook.json rename to internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-conversion-webhook.json diff --git a/testdata/manifests/crd-description-changed.json b/internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-description-changed.json similarity index 100% rename from testdata/manifests/crd-description-changed.json rename to internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-description-changed.json diff --git a/testdata/manifests/crd-field-removed.json b/internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-field-removed.json similarity index 100% rename from testdata/manifests/crd-field-removed.json rename to internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-field-removed.json diff --git a/testdata/manifests/crd-invalid b/internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-invalid similarity index 100% rename from testdata/manifests/crd-invalid rename to internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-invalid diff --git a/testdata/manifests/crd-invalid-upgrade.json b/internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-invalid-upgrade.json similarity index 100% rename from testdata/manifests/crd-invalid-upgrade.json rename to internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-invalid-upgrade.json diff --git a/testdata/manifests/crd-valid-upgrade.json b/internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-valid-upgrade.json similarity index 100% rename from testdata/manifests/crd-valid-upgrade.json rename to internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-valid-upgrade.json diff --git a/testdata/manifests/no-crds.json b/internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/no-crds.json similarity index 100% rename from testdata/manifests/no-crds.json rename to internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/no-crds.json diff --git a/testdata/manifests/old-crd.json b/internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/old-crd.json similarity index 100% rename from testdata/manifests/old-crd.json rename to internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/old-crd.json From 9bdd9ec07eaf96b89d0b78b39bf8063283832a1d Mon Sep 17 00:00:00 2001 From: Joe Lanford Date: Mon, 28 Jul 2025 20:49:14 -0400 Subject: [PATCH 2/3] add broken test --- .../crdupgradesafety/crdupgradesafety_test.go | 85 +++++++++++++------ ...crd-conversion-no-webhook-extra-issue.json | 76 +++++++++++++++++ 2 files changed, 135 insertions(+), 26 deletions(-) create mode 100644 internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-conversion-no-webhook-extra-issue.json diff --git a/internal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety_test.go b/internal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety_test.go index 25720009c..4fa840975 100644 --- a/internal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety_test.go +++ b/internal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety_test.go @@ -66,6 +66,14 @@ 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) { @@ -73,7 +81,7 @@ func TestInstall(t *testing.T) { 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) } diff --git a/internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-conversion-no-webhook-extra-issue.json b/internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-conversion-no-webhook-extra-issue.json new file mode 100644 index 000000000..0bfd13384 --- /dev/null +++ b/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" + ] + } + } +} From e27ff468c77f4539cb5293cd620fd836abe58676 Mon Sep 17 00:00:00 2001 From: Joe Lanford Date: Mon, 28 Jul 2025 20:49:36 -0400 Subject: [PATCH 3/3] bump crdify to fix broken test --- go.mod | 4 +++- go.sum | 4 ++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index d9c0283a6..ad79fa011 100644 --- a/go.mod +++ b/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 diff --git a/go.sum b/go.sum index 82201375d..a47f90ee8 100644 --- a/go.sum +++ b/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=