Add handling of updates to required fields to the CRD Upgrade Safety …

…preflight check (#933)

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

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 {

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

pkg/kapp/crdupgradesafety/preflight.go

@@ -40,6 +40,7 @@ func NewPreflight(df cmdcore.DepsFactory, enabled bool) *Preflight {
 				&ChangeValidator{
 					Validations: []ChangeValidation{
 						EnumChangeValidation,
+						RequiredFieldChangeValidation,
 					},
 				},
 			},

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

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