Add handling of updates to default values to the CRD Upgrade Safety p…

…reflight check

Signed-off-by: Rashmi Gottipati <[email protected]>

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 {

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)
+		})
+	}
+}

pkg/kapp/crdupgradesafety/preflight.go

@@ -49,6 +49,7 @@ func NewPreflight(df cmdcore.DepsFactory, enabled bool) *Preflight {
 						MaximumLengthChangeValidation,
 						MaximumItemsChangeValidation,
 						MaximumPropertiesChangeValidation,
+						DefaultValueChangeValidation,
 					},
 				},
 			},

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")
+	})
+}

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")
+	})
+}