From e1be79b06030cc582da36d84b6cef84f9adc00ab Mon Sep 17 00:00:00 2001 From: Joe Lanford Date: Tue, 26 Aug 2025 11:29:02 -0400 Subject: [PATCH] =?UTF-8?q?UPSTREAM:=20:=20=F0=9F=90=9B=20CRD=20upg?= =?UTF-8?q?rade=20safety=20fixes=20and=20ratcheting=20(#2123)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * move crd upgrade safety testdata into crdupgradesafety package Signed-off-by: Joe Lanford * fix: bump crdify to fix bugs and regressions regression fixes: 1. correctly handle processing of properties that are OpenAPI items 2. allow enums to have values added. bug fix: crdify's served version validator was updated to actually compare the old CRD with the new CRD so that any issues identified in the old CRD do not continue to be reported when performing comparisons between served versions of the new CRD. This effectively allows issues in the served version validations to be acknowledged once when they are introduced, but then those issues are essentially grandfathered in such that they do not have to be acknowledged again in the future. This issue was actually identified in a case where an operator upgrade was stopped by the CRD upgrade check despite there being no changes whatsoever to the CRD. The "old" and "new" CRDs contained the exact same issues, but since crdify was looking exclusively at the "new" CRD, it found those issues and reported them. --------- Signed-off-by: Joe Lanford --- go.mod | 2 +- go.sum | 4 +- .../crdupgradesafety/crdupgradesafety.go | 8 ++ .../crdupgradesafety/crdupgradesafety_test.go | 89 +++++++++----- ...crd-conversion-no-webhook-extra-issue.json | 76 ++++++++++++ .../manifests/crd-conversion-no-webhook.json | 0 .../manifests/crd-conversion-webhook-old.json | 0 .../manifests/crd-conversion-webhook.json | 0 .../manifests/crd-description-changed.json | 6 +- .../manifests/crd-field-removed.json | 6 +- .../testdata}/manifests/crd-invalid | 0 .../manifests/crd-invalid-upgrade.json | 7 +- .../manifests/crd-unhandled-new.json | 0 .../manifests/crd-unhandled-old.json | 0 .../manifests/crd-valid-upgrade.json | 9 +- .../testdata}/manifests/no-crds.json | 0 .../testdata}/manifests/old-crd.json | 6 +- vendor/modules.txt | 2 +- .../crdify/pkg/validations/util.go | 20 +++- .../pkg/validators/version/served/served.go | 109 ++++++++++++++++-- 20 files changed, 286 insertions(+), 58 deletions(-) create mode 100644 internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-conversion-no-webhook-extra-issue.json 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 (94%) rename {testdata => internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata}/manifests/crd-field-removed.json (96%) 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 (92%) rename {testdata => internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata}/manifests/crd-unhandled-new.json (100%) rename {testdata => internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata}/manifests/crd-unhandled-old.json (100%) rename {testdata => internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata}/manifests/crd-valid-upgrade.json (93%) 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 (94%) diff --git a/go.mod b/go.mod index 7f385942d..125cb7c9e 100644 --- a/go.mod +++ b/go.mod @@ -44,7 +44,7 @@ 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.20250825182107-69e65223aee0 sigs.k8s.io/yaml v1.6.0 ) diff --git a/go.sum b/go.sum index f66d08458..9b4abf20e 100644 --- a/go.sum +++ b/go.sum @@ -765,8 +765,8 @@ 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/crdify v0.4.1-0.20250825182107-69e65223aee0 h1:jfBjW0kwwx2ULnzRrs+Jnepn345JbpAVqzekHBeIGgY= +sigs.k8s.io/crdify v0.4.1-0.20250825182107-69e65223aee0/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= diff --git a/internal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety.go b/internal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety.go index 3905535c6..efa86a191 100644 --- a/internal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety.go +++ b/internal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety.go @@ -15,6 +15,7 @@ import ( "sigs.k8s.io/crdify/pkg/config" "sigs.k8s.io/crdify/pkg/runner" "sigs.k8s.io/crdify/pkg/validations" + "sigs.k8s.io/crdify/pkg/validations/property" "github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/util" ) @@ -131,6 +132,13 @@ func defaultConfig() *config.Config { Name: "description", Enforcement: config.EnforcementPolicyNone, }, + { + Name: "enum", + Enforcement: config.EnforcementPolicyError, + Configuration: map[string]interface{}{ + "additionPolicy": property.AdditionPolicyAllow, + }, + }, }, } } diff --git a/internal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety_test.go b/internal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety_test.go index 71b457de6..2d2a2065f 100644 --- a/internal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety_test.go +++ b/internal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety_test.go @@ -39,7 +39,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 == "" { @@ -67,6 +67,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) { @@ -74,7 +82,7 @@ func TestInstall(t *testing.T) { name string oldCrdPath string release *release.Release - wantErrMsgs []string + requireErr require.ErrorAssertionFunc wantCrdGetErr error }{ { @@ -92,7 +100,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", @@ -108,7 +116,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", @@ -124,7 +132,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", @@ -143,7 +151,7 @@ func TestInstall(t *testing.T) { Name: "test-release", Manifest: getManifestString(t, "crd-invalid-upgrade.json"), }, - wantErrMsgs: []string{ + requireErr: wantErrorMsgs([]string{ `scope:`, `storedVersionRemoval:`, `enum:`, @@ -157,7 +165,7 @@ func TestInstall(t *testing.T) { `minLength:`, `minProperties:`, `default:`, - }, + }), }, { name: "new crd validation failure for existing field removal", @@ -168,9 +176,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", @@ -188,10 +196,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) } @@ -204,7 +210,7 @@ func TestUpgrade(t *testing.T) { name string oldCrdPath string release *release.Release - wantErrMsgs []string + requireErr require.ErrorAssertionFunc wantCrdGetErr error }{ { @@ -222,7 +228,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", @@ -238,7 +244,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", @@ -254,7 +260,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", @@ -273,7 +279,7 @@ func TestUpgrade(t *testing.T) { Name: "test-release", Manifest: getManifestString(t, "crd-invalid-upgrade.json"), }, - wantErrMsgs: []string{ + requireErr: wantErrorMsgs([]string{ `scope:`, `storedVersionRemoval:`, `enum:`, @@ -287,7 +293,7 @@ func TestUpgrade(t *testing.T) { `minLength:`, `minProperties:`, `default:`, - }, + }), }, { name: "new crd validation failure for existing field removal", @@ -298,9 +304,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", @@ -317,9 +323,9 @@ func TestUpgrade(t *testing.T) { Name: "test-release", Manifest: getManifestString(t, "crd-conversion-no-webhook.json"), }, - wantErrMsgs: []string{ - `validating upgrade for CRD "crontabs.stable.example.com": v1 <-> v2: ^.spec.foobarbaz: enum: allowed enum values removed`, - }, + 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", @@ -331,16 +337,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" + ] + } + } +} 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 94% rename from testdata/manifests/crd-description-changed.json rename to internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-description-changed.json index ae30459e3..0e7f9a600 100644 --- a/testdata/manifests/crd-description-changed.json +++ b/internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-description-changed.json @@ -23,7 +23,8 @@ "type":"integer" }, "enum": { - "type":"integer" + "type": "string", + "enum": ["a", "b", "c"] }, "minMaxValue": { "type":"integer" @@ -70,7 +71,8 @@ "type":"integer" }, "enum": { - "type":"integer" + "type": "string", + "enum": ["a", "b", "c"] }, "minMaxValue": { "type":"integer" diff --git a/testdata/manifests/crd-field-removed.json b/internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-field-removed.json similarity index 96% rename from testdata/manifests/crd-field-removed.json rename to internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-field-removed.json index 86ba06e40..650b13fd4 100644 --- a/testdata/manifests/crd-field-removed.json +++ b/internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-field-removed.json @@ -22,7 +22,8 @@ "type":"integer" }, "enum": { - "type":"integer" + "type": "string", + "enum": ["a", "b", "c"] }, "minMaxValue": { "type":"integer" @@ -66,7 +67,8 @@ "type": "object", "properties": { "enum": { - "type":"integer" + "type": "string", + "enum": ["a", "b", "c"] }, "minMaxValue": { "type":"integer" 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 92% rename from testdata/manifests/crd-invalid-upgrade.json rename to internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-invalid-upgrade.json index 4131a68fb..3c95ccb25 100644 --- a/testdata/manifests/crd-invalid-upgrade.json +++ b/internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-invalid-upgrade.json @@ -24,11 +24,8 @@ "type":"integer" }, "enum": { - "type":"integer", - "enum":[ - 1, - 2 - ] + "type": "string", + "enum": ["a", "b"] }, "minMaxValue": { "type":"integer", diff --git a/testdata/manifests/crd-unhandled-new.json b/internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-unhandled-new.json similarity index 100% rename from testdata/manifests/crd-unhandled-new.json rename to internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-unhandled-new.json diff --git a/testdata/manifests/crd-unhandled-old.json b/internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-unhandled-old.json similarity index 100% rename from testdata/manifests/crd-unhandled-old.json rename to internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-unhandled-old.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 93% rename from testdata/manifests/crd-valid-upgrade.json rename to internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-valid-upgrade.json index 52380dc92..cbc2e3ec1 100644 --- a/testdata/manifests/crd-valid-upgrade.json +++ b/internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-valid-upgrade.json @@ -22,7 +22,8 @@ "type":"integer" }, "enum": { - "type":"integer" + "type": "string", + "enum": ["a", "b", "c", "adding-enum-is-allowed"] }, "minMaxValue": { "type":"integer" @@ -69,7 +70,8 @@ "type":"integer" }, "enum": { - "type":"integer" + "type": "string", + "enum": ["a", "b", "c", "adding-enum-is-allowed"] }, "minMaxValue": { "type":"integer" @@ -116,7 +118,8 @@ "type":"integer" }, "enum": { - "type":"integer" + "type": "string", + "enum": ["a", "b", "c", "adding-enum-is-allowed"] }, "minMaxValue": { "type":"integer" 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 94% rename from testdata/manifests/old-crd.json rename to internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/old-crd.json index 1f3ff5a4b..5a8c55b32 100644 --- a/testdata/manifests/old-crd.json +++ b/internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/old-crd.json @@ -23,7 +23,8 @@ "type":"integer" }, "enum": { - "type":"integer" + "type": "string", + "enum": ["a", "b", "c"] }, "minMaxValue": { "type":"integer" @@ -70,7 +71,8 @@ "type":"integer" }, "enum": { - "type":"integer" + "type": "string", + "enum": ["a", "b", "c"] }, "minMaxValue": { "type":"integer" diff --git a/vendor/modules.txt b/vendor/modules.txt index b499ae80b..3d49f8737 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -1995,7 +1995,7 @@ sigs.k8s.io/controller-tools/pkg/genall sigs.k8s.io/controller-tools/pkg/loader sigs.k8s.io/controller-tools/pkg/markers sigs.k8s.io/controller-tools/pkg/version -# sigs.k8s.io/crdify v0.4.1-0.20250613143457-398e4483fb58 +# sigs.k8s.io/crdify v0.4.1-0.20250825182107-69e65223aee0 ## explicit; go 1.24.0 sigs.k8s.io/crdify/pkg/config sigs.k8s.io/crdify/pkg/runner diff --git a/vendor/sigs.k8s.io/crdify/pkg/validations/util.go b/vendor/sigs.k8s.io/crdify/pkg/validations/util.go index 4d10bd738..85a5bb699 100644 --- a/vendor/sigs.k8s.io/crdify/pkg/validations/util.go +++ b/vendor/sigs.k8s.io/crdify/pkg/validations/util.go @@ -77,8 +77,7 @@ func FlattenedCRDVersionDiff(a, b map[string]*apiextensionsv1.JSONSchemaProps) m // based on changes to the children properties. The changes to the children // properties should still be evaluated since we are looping through a flattened // map of all the properties for the CRD version - oldSchemaCopy := oldSchema.DeepCopy() - oldSchemaCopy.Properties = nil + oldSchemaCopy := DropChildrenPropertiesFromJSONSchema(oldSchema) newSchema, ok := b[prop] // In the event the property no longer exists on the new version @@ -91,8 +90,7 @@ func FlattenedCRDVersionDiff(a, b map[string]*apiextensionsv1.JSONSchemaProps) m // Do the same copy and unset logic for the new schema properties // before comparison to ensure we are only comparing the individual properties - newSchemaCopy := newSchema.DeepCopy() - newSchemaCopy.Properties = nil + newSchemaCopy := DropChildrenPropertiesFromJSONSchema(newSchema) if !equality.Semantic.DeepEqual(oldSchemaCopy, newSchemaCopy) { diffMap[prop] = Diff{Old: oldSchemaCopy, New: newSchemaCopy} @@ -102,6 +100,20 @@ func FlattenedCRDVersionDiff(a, b map[string]*apiextensionsv1.JSONSchemaProps) m return diffMap } +// DropChildrenPropertiesFromJSONSchema sets properties on a schema +// associated with children schemas to `nil`. Useful when calculating +// differences between a before and after of a given schema +// without the changes to its children schemas influencing the +// diff calculation. +// Returns a copy of the provided apiextensionsv1.JSONSchemaProps with children schemas dropped. +func DropChildrenPropertiesFromJSONSchema(schema *apiextensionsv1.JSONSchemaProps) *apiextensionsv1.JSONSchemaProps { + schemaCopy := schema.DeepCopy() + schemaCopy.Properties = nil + schemaCopy.Items = nil + + return schemaCopy +} + // SchemaWalkerFunc is a function that walks a JSONSchemaProps. // ancestry is an order list of ancestors of s, where index 0 is the root and index len-1 is the direct parent. type SchemaWalkerFunc func(s *apiextensionsv1.JSONSchemaProps, fldPath, simpleLocation *field.Path, ancestry []*apiextensionsv1.JSONSchemaProps) bool diff --git a/vendor/sigs.k8s.io/crdify/pkg/validators/version/served/served.go b/vendor/sigs.k8s.io/crdify/pkg/validators/version/served/served.go index 36c99c43e..bd6b4ff0d 100644 --- a/vendor/sigs.k8s.io/crdify/pkg/validators/version/served/served.go +++ b/vendor/sigs.k8s.io/crdify/pkg/validators/version/served/served.go @@ -81,7 +81,7 @@ func New(opts ...ValidatorOption) *Validator { } // Validate runs the validations configured in the Validator. -func (v *Validator) Validate(_, b *apiextensionsv1.CustomResourceDefinition) map[string]map[string][]validations.ComparisonResult { +func (v *Validator) Validate(a, b *apiextensionsv1.CustomResourceDefinition) map[string]map[string][]validations.ComparisonResult { result := map[string]map[string][]validations.ComparisonResult{} // If conversion webhook is specified and conversion policy is ignore, pass check @@ -89,24 +89,117 @@ func (v *Validator) Validate(_, b *apiextensionsv1.CustomResourceDefinition) map return result } - servedVersions := []apiextensionsv1.CustomResourceDefinitionVersion{} + aResults := v.compareVersionPairs(a) + bResults := v.compareVersionPairs(b) + subtractExistingIssues(bResults, aResults) - for _, version := range b.Spec.Versions { + return bResults +} + +func (v *Validator) compareVersionPairs(crd *apiextensionsv1.CustomResourceDefinition) map[string]map[string][]validations.ComparisonResult { + result := map[string]map[string][]validations.ComparisonResult{} + + for resultVersionPair, versions := range makeVersionPairs(crd) { + result[resultVersionPair] = validations.CompareVersions(versions[0], versions[1], v.unhandledEnforcement, v.comparators...) + } + + return result +} + +func makeVersionPairs(crd *apiextensionsv1.CustomResourceDefinition) map[string][2]apiextensionsv1.CustomResourceDefinitionVersion { + servedVersions := make([]apiextensionsv1.CustomResourceDefinitionVersion, 0, len(crd.Spec.Versions)) + + for _, version := range crd.Spec.Versions { if version.Served { servedVersions = append(servedVersions, version) } } + if len(servedVersions) < 2 { + return nil + } + slices.SortFunc(servedVersions, func(a, b apiextensionsv1.CustomResourceDefinitionVersion) int { return versionhelper.CompareKubeAwareVersionStrings(a.Name, b.Name) }) - for i, oldVersion := range servedVersions[:len(servedVersions)-1] { - for _, newVersion := range servedVersions[i+1:] { - resultVersion := fmt.Sprintf("%s <-> %s", oldVersion.Name, newVersion.Name) - result[resultVersion] = validations.CompareVersions(oldVersion, newVersion, v.unhandledEnforcement, v.comparators...) + pairs := make(map[string][2]apiextensionsv1.CustomResourceDefinitionVersion, numUnidirectionalPermutations(servedVersions)) + + for i, iVersion := range servedVersions[:len(servedVersions)-1] { + for _, jVersion := range servedVersions[i+1:] { + resultVersionPair := fmt.Sprintf("%s -> %s", iVersion.Name, jVersion.Name) + pairs[resultVersionPair] = [2]apiextensionsv1.CustomResourceDefinitionVersion{iVersion, jVersion} } } - return result + return pairs +} + +func numUnidirectionalPermutations[T any](in []T) int { + n := len(in) + + return (n * (n - 1)) / 2 +} + +// subtractExistingIssues removes errors and warnings from b's results that are also found in a's results. +func subtractExistingIssues(b, a map[string]map[string][]validations.ComparisonResult) { + sliceToMapByName := func(in []validations.ComparisonResult) map[string]*validations.ComparisonResult { + out := make(map[string]*validations.ComparisonResult, len(in)) + + for i := range in { + v := &in[i] + out[v.Name] = v + } + + return out + } + + for versionPair, bVersionPairResults := range b { + aVersionPairResults, ok := a[versionPair] + if !ok { + // If the version pair is not found in a, that means + // b introduced a new version, so we'll keep _all_ + // of the comparison results for this pair + continue + } + + for fieldPath, bFieldPathResults := range bVersionPairResults { + aFieldPathResults, ok := aVersionPairResults[fieldPath] + if !ok { + // If this field path is not found in a's results + // for this version pair, that means b introduced a new field + // in an existing schema, so we'll keep _all_ of the comparison + // results for this field path. + continue + } + + aResultMap := sliceToMapByName(aFieldPathResults) + bResultMap := sliceToMapByName(bFieldPathResults) + + for validationName, bValidationResult := range bResultMap { + aValidationResult, ok := aResultMap[validationName] + if !ok { + // If a's results do not include results for this validation, + // that means we ran a new validation for b that we did not + // run for a. We never intend to do that, so if that is somehow + // the case, let's panic and say what our programmer intent was. + panic(fmt.Sprintf("Validation %q not found in a's results for version pair %q. This should never happen because this validator uses the same validation configuration for CRDs a and b.", validationName, versionPair)) + } + + bValidationResult.Errors = slices.DeleteFunc(bValidationResult.Errors, func(bErr string) bool { + return slices.Contains(aValidationResult.Errors, bErr) + }) + if len(bValidationResult.Errors) == 0 { + bValidationResult.Errors = nil + } + + bValidationResult.Warnings = slices.DeleteFunc(bValidationResult.Warnings, func(bWarn string) bool { + return slices.Contains(aValidationResult.Warnings, bWarn) + }) + if len(bValidationResult.Warnings) == 0 { + bValidationResult.Warnings = nil + } + } + } + } }