Skip to content

Commit

Permalink
Add handling of updates to required fields to the CRD Upgrade Safety …
Browse files Browse the repository at this point in the history
…preflight check

Signed-off-by: Rashmi Gottipati <chowdary.grashmi@gmail.com>
  • Loading branch information
rashmigottipati committed Apr 26, 2024
1 parent 9580fa3 commit fcb5fbe
Show file tree
Hide file tree
Showing 6 changed files with 505 additions and 0 deletions.
42 changes: 42 additions & 0 deletions pkg/kapp/crdupgradesafety/change_validator.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
79 changes: 79 additions & 0 deletions pkg/kapp/crdupgradesafety/change_validator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
})
}
}
1 change: 1 addition & 0 deletions pkg/kapp/crdupgradesafety/preflight.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ func NewPreflight(df cmdcore.DepsFactory, enabled bool) *Preflight {
&ChangeValidator{
Validations: []ChangeValidation{
EnumChangeValidation,
RequiredFieldChangeValidation,
},
},
},
Expand Down
Original file line number Diff line number Diff line change
@@ -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")
})
}
Original file line number Diff line number Diff line change
@@ -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")
})
}
Loading

0 comments on commit fcb5fbe

Please sign in to comment.