From 28d60940e2e7cdc59645285668ab60927b6fcdd7 Mon Sep 17 00:00:00 2001 From: everettraven Date: Wed, 13 Mar 2024 10:04:02 -0400 Subject: [PATCH] Add stored version removal validation to CRD preflight check Signed-off-by: everettraven --- pkg/kapp/crdupgradesafety/preflight.go | 3 +- pkg/kapp/crdupgradesafety/validator.go | 20 ++- pkg/kapp/crdupgradesafety/validator_test.go | 74 ++++++++++- ...light_crdupgradesafety_scopechange_test.go | 2 +- ...upgradesafety_storedversionremoval_test.go | 116 ++++++++++++++++++ 5 files changed, 210 insertions(+), 5 deletions(-) create mode 100644 test/e2e/preflight_crdupgradesafety_storedversionremoval_test.go diff --git a/pkg/kapp/crdupgradesafety/preflight.go b/pkg/kapp/crdupgradesafety/preflight.go index 3677574e7..3b1e5f22b 100644 --- a/pkg/kapp/crdupgradesafety/preflight.go +++ b/pkg/kapp/crdupgradesafety/preflight.go @@ -34,7 +34,8 @@ func NewPreflight(df cmdcore.DepsFactory, enabled bool) *Preflight { enabled: enabled, validator: &Validator{ Validations: []Validation{ - NewValidationFunc("NoScopeChange", NoScopeChangeValidateFunc), + NewValidationFunc("NoScopeChange", NoScopeChange), + NewValidationFunc("NoStoredVersionRemoved", NoStoredVersionRemoved), }, }, } diff --git a/pkg/kapp/crdupgradesafety/validator.go b/pkg/kapp/crdupgradesafety/validator.go index 91f6b51c7..8ed2faaf6 100644 --- a/pkg/kapp/crdupgradesafety/validator.go +++ b/pkg/kapp/crdupgradesafety/validator.go @@ -8,6 +8,7 @@ import ( "fmt" v1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + "k8s.io/apimachinery/pkg/util/sets" ) // Validation is a representation of a validation to run @@ -67,9 +68,26 @@ func (v *Validator) Validate(old, new v1.CustomResourceDefinition) error { return nil } -var NoScopeChangeValidateFunc ValidateFunc = func(old, new v1.CustomResourceDefinition) error { +func NoScopeChange(old, new v1.CustomResourceDefinition) error { if old.Spec.Scope != new.Spec.Scope { return fmt.Errorf("scope changed from %q to %q", old.Spec.Scope, new.Spec.Scope) } return nil } + +func NoStoredVersionRemoved(old, new v1.CustomResourceDefinition) error { + newVersions := sets.New[string]() + for _, version := range new.Spec.Versions { + if !newVersions.Has(version.Name) { + newVersions.Insert(version.Name) + } + } + + for _, storedVersion := range old.Status.StoredVersions { + if !newVersions.Has(storedVersion) { + return fmt.Errorf("stored version %q removed", storedVersion) + } + } + + return nil +} diff --git a/pkg/kapp/crdupgradesafety/validator_test.go b/pkg/kapp/crdupgradesafety/validator_test.go index 98ffe657e..7bb5eed5b 100644 --- a/pkg/kapp/crdupgradesafety/validator_test.go +++ b/pkg/kapp/crdupgradesafety/validator_test.go @@ -63,7 +63,7 @@ func TestValidator(t *testing.T) { } } -func TestNoScopeChangeValidateFunc(t *testing.T) { +func TestNoScopeChange(t *testing.T) { for _, tc := range []struct { name string old v1.CustomResourceDefinition @@ -99,7 +99,77 @@ func TestNoScopeChangeValidateFunc(t *testing.T) { }, } { t.Run(tc.name, func(t *testing.T) { - err := NoScopeChangeValidateFunc(tc.old, tc.new) + err := NoScopeChange(tc.old, tc.new) + require.Equal(t, tc.shouldError, err != nil) + }) + } +} + +func TestNoStoredVersionRemoved(t *testing.T) { + for _, tc := range []struct { + name string + old v1.CustomResourceDefinition + new v1.CustomResourceDefinition + shouldError bool + }{ + { + name: "no stored versions, no error", + new: v1.CustomResourceDefinition{ + Spec: v1.CustomResourceDefinitionSpec{ + Versions: []v1.CustomResourceDefinitionVersion{ + { + Name: "v1alpha1", + }, + }, + }, + }, + old: v1.CustomResourceDefinition{}, + }, + { + name: "stored versions, no stored version removed, no error", + new: v1.CustomResourceDefinition{ + Spec: v1.CustomResourceDefinitionSpec{ + Versions: []v1.CustomResourceDefinitionVersion{ + { + Name: "v1alpha1", + }, + { + Name: "v1alpha2", + }, + }, + }, + }, + old: v1.CustomResourceDefinition{ + Status: v1.CustomResourceDefinitionStatus{ + StoredVersions: []string{ + "v1alpha1", + }, + }, + }, + }, + { + name: "stored versions, stored version removed, error", + new: v1.CustomResourceDefinition{ + Spec: v1.CustomResourceDefinitionSpec{ + Versions: []v1.CustomResourceDefinitionVersion{ + { + Name: "v1alpha2", + }, + }, + }, + }, + old: v1.CustomResourceDefinition{ + Status: v1.CustomResourceDefinitionStatus{ + StoredVersions: []string{ + "v1alpha1", + }, + }, + }, + shouldError: true, + }, + } { + t.Run(tc.name, func(t *testing.T) { + err := NoStoredVersionRemoved(tc.old, tc.new) require.Equal(t, tc.shouldError, err != nil) }) } diff --git a/test/e2e/preflight_crdupgradesafety_scopechange_test.go b/test/e2e/preflight_crdupgradesafety_scopechange_test.go index 133982cfd..6a3fbcaa5 100644 --- a/test/e2e/preflight_crdupgradesafety_scopechange_test.go +++ b/test/e2e/preflight_crdupgradesafety_scopechange_test.go @@ -107,7 +107,7 @@ spec: ` update = strings.ReplaceAll(update, "__test-name__", testName) - logger.Section("deploy app with breaking change to CRD and preflight check enabled, should error", func() { + logger.Section("deploy app with CRD update that changes scope from namespace to cluster, preflight check enabled, should error", func() { _, err := kapp.RunWithOpts([]string{"deploy", "--preflight=CRDUpgradeSafety", "-a", appName, "-f", "-"}, RunOpts{StdinReader: strings.NewReader(update), AllowError: true}) require.Error(t, err) diff --git a/test/e2e/preflight_crdupgradesafety_storedversionremoval_test.go b/test/e2e/preflight_crdupgradesafety_storedversionremoval_test.go new file mode 100644 index 000000000..324e3d323 --- /dev/null +++ b/test/e2e/preflight_crdupgradesafety_storedversionremoval_test.go @@ -0,0 +1,116 @@ +// Copyright 2024 VMware, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package e2e + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestPreflightCRDUpgradeSafetyStoredVersionRemoved(t *testing.T) { + env := BuildEnv(t) + logger := Logger{} + kapp := Kapp{t, env.Namespace, env.KappBinaryPath, logger} + kubectl := Kubectl{t, env.Namespace, logger} + + testName := "preflightcrdupgradesafetystoredversionremoved" + + 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: object + 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}) + RemoveClusterResource(t, "ns", testName, "", kubectl) + } + cleanUp() + defer cleanUp() + + kapp.RunWithOpts([]string{"deploy", "-a", appName, "-f", "-"}, RunOpts{StdinReader: strings.NewReader(base)}) + + 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: v1alpha2 + schema: + openAPIV3Schema: + properties: + apiVersion: + type: string + kind: + type: string + metadata: + type: object + spec: + type: object + status: + type: object + type: object + served: true + storage: true + subresources: + status: {} +` + + update = strings.ReplaceAll(update, "__test-name__", testName) + logger.Section("deploy app with CRD that removes an existing stored version, preflight check enabled, should error", func() { + _, 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(), "\"NoStoredVersionRemoved\" validation failed: stored version \"v1alpha1\" removed") + }) +}