From eebe3733022ef20942a8b3f98bdee20d85697667 Mon Sep 17 00:00:00 2001 From: Anik Bhattacharjee Date: Wed, 11 Jun 2025 15:10:54 -0400 Subject: [PATCH] fix(crd-upgrade-safety): Safely handle changes to description fields Motivation: When attempting to upgrade argocd-operator from v0.5.0 to v0.7.0, the upgrade process fails during the preflight CRD safety validation. The validation correctly detects that the `argocds.argoproj.io` CRD has been modified between the two versions. The specific error reported is: ``` CustomResourceDefinition argocds.argoproj.io failed upgrade safety validation. "ChangeValidator" validation failed: version "v1alpha1", field "^.status.applicationController" has unknown change, refusing to determine that change is safe ``` However, changes between the CRD versions in this instance are limited to non-functional updates in the description fields of various properties (e.g., status.applicationController).`ChangeValidator` lacks a specific rule to classify a description-only update as safe, which blocks legitimate and otherwise safe operator upgrades. Solution: This PR enhances the CRD upgrade safety validation logic to correctly handle changes to description fields by introducing a new `ChangeValidation` check for `Description`, and registering the check by adding it to the default list of `ChangeValidations` used by `ChangeValidator`. Result: Non-functional updates to documentation fields are now deemed safe(which resolves the upgrade failure for argocd-operator from v0.5.0 to v0.7.0) --- .../preflights/crdupgradesafety/checks.go | 10 ++ .../crdupgradesafety/checks_test.go | 102 ++++++++++++++++++ .../crdupgradesafety/crdupgradesafety.go | 1 + 3 files changed, 113 insertions(+) diff --git a/internal/operator-controller/rukpak/preflights/crdupgradesafety/checks.go b/internal/operator-controller/rukpak/preflights/crdupgradesafety/checks.go index 669f65e57..61d8b55c3 100644 --- a/internal/operator-controller/rukpak/preflights/crdupgradesafety/checks.go +++ b/internal/operator-controller/rukpak/preflights/crdupgradesafety/checks.go @@ -242,3 +242,13 @@ func Type(diff FieldDiff) (bool, error) { return isHandled(diff, reset), err } + +// Description changes are considered safe and non-breaking. +func Description(diff FieldDiff) (bool, error) { + reset := func(diff FieldDiff) FieldDiff { + diff.Old.Description = "" + diff.New.Description = "" + return diff + } + return isHandled(diff, reset), nil +} diff --git a/internal/operator-controller/rukpak/preflights/crdupgradesafety/checks_test.go b/internal/operator-controller/rukpak/preflights/crdupgradesafety/checks_test.go index 36618b584..ebceed8b4 100644 --- a/internal/operator-controller/rukpak/preflights/crdupgradesafety/checks_test.go +++ b/internal/operator-controller/rukpak/preflights/crdupgradesafety/checks_test.go @@ -904,3 +904,105 @@ func TestType(t *testing.T) { }) } } + +func TestDescription(t *testing.T) { + for _, tc := range []testcase{ + { + name: "no diff, no error, handled", + diff: FieldDiff{ + Old: &apiextensionsv1.JSONSchemaProps{ + Description: "some field", + }, + New: &apiextensionsv1.JSONSchemaProps{ + Description: "some field", + }, + }, + err: nil, + handled: true, + }, + { + name: "description changed, no error, handled", + diff: FieldDiff{ + Old: &apiextensionsv1.JSONSchemaProps{ + Description: "old description", + }, + New: &apiextensionsv1.JSONSchemaProps{ + Description: "new description", + }, + }, + err: nil, + handled: true, + }, + { + name: "description added, no error, handled", + diff: FieldDiff{ + Old: &apiextensionsv1.JSONSchemaProps{}, + New: &apiextensionsv1.JSONSchemaProps{ + Description: "a new description was added", + }, + }, + err: nil, + handled: true, + }, + { + name: "description removed, no error, handled", + diff: FieldDiff{ + Old: &apiextensionsv1.JSONSchemaProps{ + Description: "this description will be removed", + }, + New: &apiextensionsv1.JSONSchemaProps{}, + }, + err: nil, + handled: true, + }, + { + name: "different field changed, no error, not handled", + diff: FieldDiff{ + Old: &apiextensionsv1.JSONSchemaProps{ + ID: "foo", + }, + New: &apiextensionsv1.JSONSchemaProps{ + ID: "bar", + }, + }, + err: nil, + handled: false, + }, + { + name: "different field changed with description, no error, not handled", + diff: FieldDiff{ + Old: &apiextensionsv1.JSONSchemaProps{ + ID: "foo", + Description: "description", + }, + New: &apiextensionsv1.JSONSchemaProps{ + ID: "bar", + Description: "description", + }, + }, + err: nil, + handled: false, + }, + { + name: "description and ID changed, no error, not handled", + diff: FieldDiff{ + Old: &apiextensionsv1.JSONSchemaProps{ + ID: "foo", + Description: "old description", + }, + New: &apiextensionsv1.JSONSchemaProps{ + ID: "bar", + Description: "new description", + }, + }, + err: nil, + handled: false, + }, + } { + t.Run(tc.name, func(t *testing.T) { + handled, err := Description(tc.diff) + require.Equal(t, tc.err, err) + require.Equal(t, tc.handled, handled) + }) + } +} diff --git a/internal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety.go b/internal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety.go index 6bc177cd1..0904bf4d4 100644 --- a/internal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety.go +++ b/internal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety.go @@ -31,6 +31,7 @@ type Preflight struct { func NewPreflight(crdCli apiextensionsv1client.CustomResourceDefinitionInterface, opts ...Option) *Preflight { changeValidations := []ChangeValidation{ + Description, Enum, Required, Maximum,