From fcb5fbe310ac7c6c3b96fc3e01c5039050caf442 Mon Sep 17 00:00:00 2001 From: Rashmi Gottipati Date: Sun, 21 Apr 2024 21:54:31 -0400 Subject: [PATCH] Add handling of updates to required fields to the CRD Upgrade Safety preflight check Signed-off-by: Rashmi Gottipati --- pkg/kapp/crdupgradesafety/change_validator.go | 42 ++++++ .../crdupgradesafety/change_validator_test.go | 79 +++++++++++ pkg/kapp/crdupgradesafety/preflight.go | 1 + ...d_field_change_requiredfield_added_test.go | 129 ++++++++++++++++++ ...uiredfield_added_when_none_existed_test.go | 126 +++++++++++++++++ ...field_change_requiredfield_removed_test.go | 128 +++++++++++++++++ 6 files changed, 505 insertions(+) create mode 100644 test/e2e/preflight_crdupgradesafety_invalid_field_change_requiredfield_added_test.go create mode 100644 test/e2e/preflight_crdupgradesafety_invalid_field_change_requiredfield_added_when_none_existed_test.go create mode 100644 test/e2e/preflight_crdupgradesafety_valid_field_change_requiredfield_removed_test.go diff --git a/pkg/kapp/crdupgradesafety/change_validator.go b/pkg/kapp/crdupgradesafety/change_validator.go index 4ae3ac936..f346a0549 100644 --- a/pkg/kapp/crdupgradesafety/change_validator.go +++ b/pkg/kapp/crdupgradesafety/change_validator.go @@ -71,6 +71,48 @@ func EnumChangeValidation(diff FieldDiff) (bool, error) { return handled(), nil } +// RequiredFieldChangeValidation adds a validation check to ensure that +// existing required fields can be marked as optional in a CRD schema: +// - No new values can be added as required that did not previously have +// any required fields present +// - Existing values can be removed from the required field +// This function returns: +// - A boolean representation of whether or not the change +// has been fully handled (i.e. the only change was to required field values) +// - An error if either of the above criteria are not met +func RequiredFieldChangeValidation(diff FieldDiff) (bool, error) { + handled := func() bool { + diff.Old.Required = []string{} + diff.New.Required = []string{} + return reflect.DeepEqual(diff.Old, diff.New) + } + + if len(diff.Old.Required) == 0 && len(diff.New.Required) > 0 { + return handled(), fmt.Errorf("new values added as required when previously no required fields existed: %+v", diff.New.Required) + } + + oldSet := sets.NewString() + for _, requiredField := range diff.Old.Required { + if !oldSet.Has(requiredField) { + oldSet.Insert(requiredField) + } + } + + newSet := sets.NewString() + for _, requiredField := range diff.New.Required { + if !newSet.Has(requiredField) { + newSet.Insert(requiredField) + } + } + + diffSet := newSet.Difference(oldSet) + if diffSet.Len() > 0 { + return handled(), fmt.Errorf("new required fields added: %+v", diffSet.UnsortedList()) + } + + return handled(), nil +} + // ChangeValidator is a Validation implementation focused on // handling updates to existing fields in a CRD type ChangeValidator struct { diff --git a/pkg/kapp/crdupgradesafety/change_validator_test.go b/pkg/kapp/crdupgradesafety/change_validator_test.go index d979b0f81..c787ba7a8 100644 --- a/pkg/kapp/crdupgradesafety/change_validator_test.go +++ b/pkg/kapp/crdupgradesafety/change_validator_test.go @@ -429,3 +429,82 @@ func TestChangeValidator(t *testing.T) { }) } } + +func TestRequiredFieldChangeValidation(t *testing.T) { + for _, tc := range []struct { + name string + diff crdupgradesafety.FieldDiff + shouldError bool + shouldHandle bool + }{ + { + name: "no change, no error, marked as handled", + diff: crdupgradesafety.FieldDiff{ + Old: &v1.JSONSchemaProps{ + Required: []string{"foo"}, + }, + New: &v1.JSONSchemaProps{ + Required: []string{"foo"}, + }, + }, + shouldHandle: true, + }, + { + name: "required field removed, no other changes, should be handled, no error", + diff: crdupgradesafety.FieldDiff{ + Old: &v1.JSONSchemaProps{ + Required: []string{"foo", "bar"}, + }, + New: &v1.JSONSchemaProps{ + Required: []string{"foo"}, + }, + }, + shouldHandle: true, + }, + { + name: "new required field added, no other changes, should be handled, error", + diff: crdupgradesafety.FieldDiff{ + Old: &v1.JSONSchemaProps{ + Required: []string{"foo"}, + }, + New: &v1.JSONSchemaProps{ + Required: []string{"foo", "bar"}, + }, + }, + shouldHandle: true, + shouldError: true, + }, + { + name: "no required field change, another field modified, no error, not marked as handled", + diff: crdupgradesafety.FieldDiff{ + Old: &v1.JSONSchemaProps{ + Required: []string{"foo"}, + ID: "abc", + }, + New: &v1.JSONSchemaProps{ + Required: []string{"foo"}, + ID: "xyz", + }, + }, + }, + { + name: "no required fields before, new required fields added, no other changes, error, marked as handled", + diff: crdupgradesafety.FieldDiff{ + Old: &v1.JSONSchemaProps{}, + New: &v1.JSONSchemaProps{ + Required: []string{"foo", "bar"}, + }, + }, + shouldHandle: true, + shouldError: true, + }, + } { + t.Run(tc.name, func(t *testing.T) { + handled, err := crdupgradesafety.RequiredFieldChangeValidation(tc.diff) + assert.Empty(t, tc.diff.Old.Required) + assert.Empty(t, tc.diff.New.Required) + assert.Equal(t, tc.shouldError, err != nil, "should error? - %v", tc.shouldError) + assert.Equal(t, tc.shouldHandle, handled, "should be handled? - %v", tc.shouldHandle) + }) + } +} diff --git a/pkg/kapp/crdupgradesafety/preflight.go b/pkg/kapp/crdupgradesafety/preflight.go index cd3e661cc..72e3aab70 100644 --- a/pkg/kapp/crdupgradesafety/preflight.go +++ b/pkg/kapp/crdupgradesafety/preflight.go @@ -40,6 +40,7 @@ func NewPreflight(df cmdcore.DepsFactory, enabled bool) *Preflight { &ChangeValidator{ Validations: []ChangeValidation{ EnumChangeValidation, + RequiredFieldChangeValidation, }, }, }, diff --git a/test/e2e/preflight_crdupgradesafety_invalid_field_change_requiredfield_added_test.go b/test/e2e/preflight_crdupgradesafety_invalid_field_change_requiredfield_added_test.go new file mode 100644 index 000000000..125c1f803 --- /dev/null +++ b/test/e2e/preflight_crdupgradesafety_invalid_field_change_requiredfield_added_test.go @@ -0,0 +1,129 @@ +// Copyright 2024 The Carvel Authors. +// SPDX-License-Identifier: Apache-2.0 + +package e2e + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestPreflightCRDUpgradeSafetyInvalidFieldChangeRequiredFieldAdded(t *testing.T) { + env := BuildEnv(t) + logger := Logger{} + kapp := Kapp{t, env.Namespace, env.KappBinaryPath, logger} + + testName := "preflightcrdupgradesafetyinvalidfieldchangerequiredfieldadded" + + base := ` +--- +apiVersion: apiextensions.k8s.io/v1 +kind: CustomResourceDefinition +metadata: + annotations: + controller-gen.kubebuilder.io/version: v0.13.0 + name: memcacheds.__test-name__.example.com +spec: + group: __test-name__.example.com + names: + kind: Memcached + listKind: MemcachedList + plural: memcacheds + singular: memcached + scope: Namespaced + versions: + - name: v1alpha1 + schema: + openAPIV3Schema: + properties: + apiVersion: + type: string + kind: + type: string + metadata: + type: object + spec: + type: string + properties: + image: + type: string + pollInterval: + type: string + required: + - image + status: + type: object + type: object + served: true + storage: true + subresources: + status: {} +` + + base = strings.ReplaceAll(base, "__test-name__", testName) + appName := "preflight-crdupgradesafety-app" + + cleanUp := func() { + kapp.Run([]string{"delete", "-a", appName}) + } + cleanUp() + defer cleanUp() + + update := ` +--- +apiVersion: apiextensions.k8s.io/v1 +kind: CustomResourceDefinition +metadata: + annotations: + controller-gen.kubebuilder.io/version: v0.13.0 + name: memcacheds.__test-name__.example.com +spec: + group: __test-name__.example.com + names: + kind: Memcached + listKind: MemcachedList + plural: memcacheds + singular: memcached + scope: Namespaced + versions: + - name: v1alpha1 + schema: + openAPIV3Schema: + properties: + apiVersion: + type: string + kind: + type: string + metadata: + type: object + spec: + type: string + properties: + image: + type: string + pollInterval: + type: string + required: + - image + - pollInterval + status: + type: object + type: object + served: true + storage: true + subresources: + status: {} +` + + update = strings.ReplaceAll(update, "__test-name__", testName) + logger.Section("deploy app with CRD update that adds a required field value, preflight check enabled, should error", func() { + _, err := kapp.RunWithOpts([]string{"deploy", "-a", appName, "-f", "-"}, RunOpts{StdinReader: strings.NewReader(base)}) + require.NoError(t, err) + _, err = kapp.RunWithOpts([]string{"deploy", "--preflight=CRDUpgradeSafety", "-a", appName, "-f", "-"}, + RunOpts{StdinReader: strings.NewReader(update), AllowError: true}) + require.Error(t, err) + require.Contains(t, err.Error(), "new required fields added") + }) +} diff --git a/test/e2e/preflight_crdupgradesafety_invalid_field_change_requiredfield_added_when_none_existed_test.go b/test/e2e/preflight_crdupgradesafety_invalid_field_change_requiredfield_added_when_none_existed_test.go new file mode 100644 index 000000000..20a0c8126 --- /dev/null +++ b/test/e2e/preflight_crdupgradesafety_invalid_field_change_requiredfield_added_when_none_existed_test.go @@ -0,0 +1,126 @@ +// Copyright 2024 The Carvel Authors. +// SPDX-License-Identifier: Apache-2.0 + +package e2e + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestPreflightCRDUpgradeSafetyInvalidFieldChangeRequiredFieldAddedWhenNoneExisted(t *testing.T) { + env := BuildEnv(t) + logger := Logger{} + kapp := Kapp{t, env.Namespace, env.KappBinaryPath, logger} + + testName := "preflightcrdupgradesafetyinvalidfieldchangerequiredfieldaddedwhennonexisted" + + base := ` +--- +apiVersion: apiextensions.k8s.io/v1 +kind: CustomResourceDefinition +metadata: + annotations: + controller-gen.kubebuilder.io/version: v0.13.0 + name: memcacheds.__test-name__.example.com +spec: + group: __test-name__.example.com + names: + kind: Memcached + listKind: MemcachedList + plural: memcacheds + singular: memcached + scope: Namespaced + versions: + - name: v1alpha1 + schema: + openAPIV3Schema: + properties: + apiVersion: + type: string + kind: + type: string + metadata: + type: object + spec: + type: string + properties: + image: + type: string + pollInterval: + type: string + status: + type: object + type: object + served: true + storage: true + subresources: + status: {} +` + + base = strings.ReplaceAll(base, "__test-name__", testName) + appName := "preflight-crdupgradesafety-app" + + cleanUp := func() { + kapp.Run([]string{"delete", "-a", appName}) + } + cleanUp() + defer cleanUp() + + update := ` +--- +apiVersion: apiextensions.k8s.io/v1 +kind: CustomResourceDefinition +metadata: + annotations: + controller-gen.kubebuilder.io/version: v0.13.0 + name: memcacheds.__test-name__.example.com +spec: + group: __test-name__.example.com + names: + kind: Memcached + listKind: MemcachedList + plural: memcacheds + singular: memcached + scope: Namespaced + versions: + - name: v1alpha1 + schema: + openAPIV3Schema: + properties: + apiVersion: + type: string + kind: + type: string + metadata: + type: object + spec: + type: string + properties: + image: + type: string + pollInterval: + type: string + required: + - image + status: + type: object + type: object + served: true + storage: true + subresources: + status: {} +` + + update = strings.ReplaceAll(update, "__test-name__", testName) + logger.Section("deploy app with CRD update that adds a new required field value when none existed prior to this, preflight check enabled, should error", func() { + _, err := kapp.RunWithOpts([]string{"deploy", "-a", appName, "-f", "-"}, RunOpts{StdinReader: strings.NewReader(base)}) + require.NoError(t, err) + _, err = kapp.RunWithOpts([]string{"deploy", "--preflight=CRDUpgradeSafety", "-a", appName, "-f", "-"}, + RunOpts{StdinReader: strings.NewReader(update), AllowError: true}) + require.Error(t, err) + require.Contains(t, err.Error(), "new values added as required when previously no required fields existed") + }) +} diff --git a/test/e2e/preflight_crdupgradesafety_valid_field_change_requiredfield_removed_test.go b/test/e2e/preflight_crdupgradesafety_valid_field_change_requiredfield_removed_test.go new file mode 100644 index 000000000..98831efa8 --- /dev/null +++ b/test/e2e/preflight_crdupgradesafety_valid_field_change_requiredfield_removed_test.go @@ -0,0 +1,128 @@ +// Copyright 2024 The Carvel Authors. +// SPDX-License-Identifier: Apache-2.0 + +package e2e + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestPreflightCRDUpgradeSafetyInvalidFieldChangeRequiredFieldRemoved(t *testing.T) { + env := BuildEnv(t) + logger := Logger{} + kapp := Kapp{t, env.Namespace, env.KappBinaryPath, logger} + + testName := "preflightcrdupgradesafetyinvalidfieldchangerequiredfieldremoved" + + base := ` +--- +apiVersion: apiextensions.k8s.io/v1 +kind: CustomResourceDefinition +metadata: + annotations: + controller-gen.kubebuilder.io/version: v0.13.0 + name: memcacheds.__test-name__.example.com +spec: + group: __test-name__.example.com + names: + kind: Memcached + listKind: MemcachedList + plural: memcacheds + singular: memcached + scope: Namespaced + versions: + - name: v1alpha1 + schema: + openAPIV3Schema: + properties: + apiVersion: + type: string + kind: + type: string + metadata: + type: object + spec: + type: string + properties: + image: + type: string + pollInterval: + type: string + required: + - image + - pollInterval + status: + type: object + type: object + served: true + storage: true + subresources: + status: {} +` + + base = strings.ReplaceAll(base, "__test-name__", testName) + appName := "preflight-crdupgradesafety-app" + + cleanUp := func() { + kapp.Run([]string{"delete", "-a", appName}) + } + cleanUp() + defer cleanUp() + + update := ` +--- +apiVersion: apiextensions.k8s.io/v1 +kind: CustomResourceDefinition +metadata: + annotations: + controller-gen.kubebuilder.io/version: v0.13.0 + name: memcacheds.__test-name__.example.com +spec: + group: __test-name__.example.com + names: + kind: Memcached + listKind: MemcachedList + plural: memcacheds + singular: memcached + scope: Namespaced + versions: + - name: v1alpha1 + schema: + openAPIV3Schema: + properties: + apiVersion: + type: string + kind: + type: string + metadata: + type: object + spec: + type: string + properties: + image: + type: string + pollInterval: + type: string + required: + - image + status: + type: object + type: object + served: true + storage: true + subresources: + status: {} +` + + update = strings.ReplaceAll(update, "__test-name__", testName) + logger.Section("deploy app with CRD update that removes a required field value, preflight check enabled, should not error", func() { + _, err := kapp.RunWithOpts([]string{"deploy", "-a", appName, "-f", "-"}, RunOpts{StdinReader: strings.NewReader(base)}) + require.NoError(t, err) + _, err = kapp.RunWithOpts([]string{"deploy", "--preflight=CRDUpgradeSafety", "-a", appName, "-f", "-"}, + RunOpts{StdinReader: strings.NewReader(update), AllowError: true}) + require.NoError(t, err) + }) +}