From 9a9d23d6bc973532edc3f93e6f7b7ab6f23ef6a1 Mon Sep 17 00:00:00 2001 From: Rashmi Gottipati Date: Thu, 2 May 2024 10:49:52 -0400 Subject: [PATCH] Add handling of updates to default values to the CRD Upgrade Safety preflight check Signed-off-by: Rashmi Gottipati --- pkg/kapp/crdupgradesafety/change_validator.go | 39 ++++++ .../crdupgradesafety/change_validator_test.go | 94 ++++++++++++++ pkg/kapp/crdupgradesafety/preflight.go | 1 + ...invalid_field_change_default_added_test.go | 118 ++++++++++++++++++ ...valid_field_change_default_changed_test.go | 118 ++++++++++++++++++ ...valid_field_change_default_removed_test.go | 117 +++++++++++++++++ ...lid_field_change_default_no_change_test.go | 117 +++++++++++++++++ 7 files changed, 604 insertions(+) create mode 100644 test/e2e/preflight_crdupgradesafety_invalid_field_change_default_added_test.go create mode 100644 test/e2e/preflight_crdupgradesafety_invalid_field_change_default_changed_test.go create mode 100644 test/e2e/preflight_crdupgradesafety_invalid_field_change_default_removed_test.go create mode 100644 test/e2e/preflight_crdupgradesafety_valid_field_change_default_no_change_test.go diff --git a/pkg/kapp/crdupgradesafety/change_validator.go b/pkg/kapp/crdupgradesafety/change_validator.go index 8030fb9ad..f69e67410 100644 --- a/pkg/kapp/crdupgradesafety/change_validator.go +++ b/pkg/kapp/crdupgradesafety/change_validator.go @@ -4,6 +4,7 @@ package crdupgradesafety import ( + "bytes" "errors" "fmt" "reflect" @@ -369,6 +370,44 @@ func MaximumPropertiesChangeValidation(diff FieldDiff) (bool, error) { } } +// DefaultValueChangeValidation adds a validation check to ensure that +// default values are not changed in a CRD schema: +// - No new value can be added as default that did not previously have a +// default value present +// - Default value of a field cannot be changed +// - Existing default value for a field cannot be removed +// This function returns: +// - A boolean representation of whether or not the change +// has been fully handled (i.e. the only change was to a field's default value) +// - An error if either of the above criteria are not met +func DefaultValueChangeValidation(diff FieldDiff) (bool, error) { + handled := func() bool { + diff.Old.Default = &v1.JSON{} + diff.New.Default = &v1.JSON{} + return reflect.DeepEqual(diff.Old, diff.New) + } + + switch { + case diff.Old.Default == nil && diff.New.Default != nil: + newDefault := diff.New.Default + return handled(), fmt.Errorf("new value added as default when previously no default value existed: %+v", newDefault) + + case diff.Old.Default != nil && diff.New.Default == nil: + oldDefault := diff.Old.Default.Raw + return handled(), fmt.Errorf("default value has been removed when previously a default value existed: %+v", oldDefault) + + case diff.Old.Default != nil && diff.New.Default != nil: + oldDefault := diff.Old.Default.Raw + newDefault := diff.New.Default.Raw + if !bytes.Equal(diff.Old.Default.Raw, diff.New.Default.Raw) { + return handled(), fmt.Errorf("default value has been changed from %+v to %+v", oldDefault, newDefault) + } + fallthrough + default: + 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 f990676d3..2a73261aa 100644 --- a/pkg/kapp/crdupgradesafety/change_validator_test.go +++ b/pkg/kapp/crdupgradesafety/change_validator_test.go @@ -1221,3 +1221,97 @@ func TestMaximumPropertiesChangeValidation(t *testing.T) { }) } } + +func TestDefaultChangeValidation(t *testing.T) { + for _, tc := range []struct { + name string + diff crdupgradesafety.FieldDiff + shouldError bool + shouldHandle bool + }{ + { + name: "no change in default value, no error, marked as handled", + diff: crdupgradesafety.FieldDiff{ + Old: &v1.JSONSchemaProps{ + Default: &v1.JSON{ + Raw: []byte("foo"), + }, + }, + New: &v1.JSONSchemaProps{ + Default: &v1.JSON{ + Raw: []byte("foo"), + }, + }, + }, + shouldHandle: true, + }, + { + name: "no default before, default added, no other changes, error, marked as handled", + diff: crdupgradesafety.FieldDiff{ + Old: &v1.JSONSchemaProps{}, + New: &v1.JSONSchemaProps{ + Default: &v1.JSON{ + Raw: []byte("foo"), + }, + }, + }, + shouldHandle: true, + shouldError: true, + }, + { + name: "existing default removed, no other changes, error, should be handled", + diff: crdupgradesafety.FieldDiff{ + Old: &v1.JSONSchemaProps{ + Default: &v1.JSON{ + Raw: []byte("foo"), + }, + }, + New: &v1.JSONSchemaProps{}, + }, + shouldHandle: true, + shouldError: true, + }, + { + name: "default value changed, error, marked as handled", + diff: crdupgradesafety.FieldDiff{ + Old: &v1.JSONSchemaProps{ + Default: &v1.JSON{ + Raw: []byte("foo"), + }, + }, + New: &v1.JSONSchemaProps{ + Default: &v1.JSON{ + Raw: []byte("bar"), + }, + }, + }, + shouldHandle: true, + shouldError: true, + }, + { + name: "no default value change, other changes, no error, not marked as handled", + diff: crdupgradesafety.FieldDiff{ + Old: &v1.JSONSchemaProps{ + Default: &v1.JSON{ + Raw: []byte("foo"), + }, + ID: "abc", + }, + New: &v1.JSONSchemaProps{ + Default: &v1.JSON{ + Raw: []byte("foo"), + }, + ID: "xyz", + }, + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + handled, err := crdupgradesafety.DefaultValueChangeValidation(tc.diff) + assert.Equal(t, tc.shouldError, err != nil, "should error? - %v", tc.shouldError) + assert.Equal(t, tc.shouldHandle, handled, "should be handled? - %v", tc.shouldHandle) + assert.Empty(t, tc.diff.Old.Default) + assert.Empty(t, tc.diff.New.Default) + }) + } +} diff --git a/pkg/kapp/crdupgradesafety/preflight.go b/pkg/kapp/crdupgradesafety/preflight.go index 0a166eac8..482ef5cfc 100644 --- a/pkg/kapp/crdupgradesafety/preflight.go +++ b/pkg/kapp/crdupgradesafety/preflight.go @@ -49,6 +49,7 @@ func NewPreflight(df cmdcore.DepsFactory, enabled bool) *Preflight { MaximumLengthChangeValidation, MaximumItemsChangeValidation, MaximumPropertiesChangeValidation, + DefaultValueChangeValidation, }, }, }, diff --git a/test/e2e/preflight_crdupgradesafety_invalid_field_change_default_added_test.go b/test/e2e/preflight_crdupgradesafety_invalid_field_change_default_added_test.go new file mode 100644 index 000000000..9f6f54c16 --- /dev/null +++ b/test/e2e/preflight_crdupgradesafety_invalid_field_change_default_added_test.go @@ -0,0 +1,118 @@ +// Copyright 2024 The Carvel Authors. +// SPDX-License-Identifier: Apache-2.0 + +package e2e + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/require" +) + +// Adding a new value as default for a field that did not +// previously have a default value present is invalid +// and this test is covering that case. +func TestPreflightCRDUpgradeSafetyInvalidFieldChangeDefaultAdded(t *testing.T) { + env := BuildEnv(t) + logger := Logger{} + kapp := Kapp{t, env.Namespace, env.KappBinaryPath, logger} + + testName := "preflightcrdupgradesafetyinvalidfieldchangedefaultadded" + + 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 + 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 + default: foo + 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 default value that did not exist previously, 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 value added as default when previously no default value existed") + }) +} diff --git a/test/e2e/preflight_crdupgradesafety_invalid_field_change_default_changed_test.go b/test/e2e/preflight_crdupgradesafety_invalid_field_change_default_changed_test.go new file mode 100644 index 000000000..813aaa47c --- /dev/null +++ b/test/e2e/preflight_crdupgradesafety_invalid_field_change_default_changed_test.go @@ -0,0 +1,118 @@ +// Copyright 2024 The Carvel Authors. +// SPDX-License-Identifier: Apache-2.0 + +package e2e + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/require" +) + +// Changing an existing field's default value to a new one is not valid +// and this test is covering that case. +func TestPreflightCRDUpgradeSafetyInvalidFieldChangeDefaultChanged(t *testing.T) { + env := BuildEnv(t) + logger := Logger{} + kapp := Kapp{t, env.Namespace, env.KappBinaryPath, logger} + + testName := "preflightcrdupgradesafetyinvalidfieldchangedefaultchanged" + + 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 + default: foo + 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 + default: bar + 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 changes an existing field's default 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(), "default value has been changed") + }) +} diff --git a/test/e2e/preflight_crdupgradesafety_invalid_field_change_default_removed_test.go b/test/e2e/preflight_crdupgradesafety_invalid_field_change_default_removed_test.go new file mode 100644 index 000000000..5e89e0cc7 --- /dev/null +++ b/test/e2e/preflight_crdupgradesafety_invalid_field_change_default_removed_test.go @@ -0,0 +1,117 @@ +// Copyright 2024 The Carvel Authors. +// SPDX-License-Identifier: Apache-2.0 + +package e2e + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/require" +) + +// Removing a default value for a field when previously a default value existed +// is not valid and this test is covering that case. +func TestPreflightCRDUpgradeSafetyInvalidFieldChangeDefaultRemoved(t *testing.T) { + env := BuildEnv(t) + logger := Logger{} + kapp := Kapp{t, env.Namespace, env.KappBinaryPath, logger} + + testName := "preflightcrdupgradesafetyinvalidfieldchangedefaultremoved" + + 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 + default: foo + 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 + 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 default value that existed previously, 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(), "default value has been removed when previously a default value existed") + }) +} diff --git a/test/e2e/preflight_crdupgradesafety_valid_field_change_default_no_change_test.go b/test/e2e/preflight_crdupgradesafety_valid_field_change_default_no_change_test.go new file mode 100644 index 000000000..d68672aab --- /dev/null +++ b/test/e2e/preflight_crdupgradesafety_valid_field_change_default_no_change_test.go @@ -0,0 +1,117 @@ +// Copyright 2024 The Carvel Authors. +// SPDX-License-Identifier: Apache-2.0 + +package e2e + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/require" +) + +// No change in the default value of a field is a valid case +// and this test covers that. +func TestPreflightCRDUpgradeSafetyValidFieldChangeDefaultNoChange(t *testing.T) { + env := BuildEnv(t) + logger := Logger{} + kapp := Kapp{t, env.Namespace, env.KappBinaryPath, logger} + + testName := "preflightcrdupgradesafetyvalidfieldchangedefaultnochange" + + 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 + default: foo + 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.14.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 + default: foo + 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 has the same default value of a field, 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) + }) +}