Merge pull request #4 from everettraven/no-stored-version-removal-val…

…idation Add stored version removal validation to CRD preflight check
carvel-dev · Mar 25, 2024 · 6b9d2e7 · 6b9d2e7
2 parents a0d4327 + 28d6094
commit 6b9d2e7
Show file tree

Hide file tree

Showing 5 changed files with 210 additions and 5 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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")
+	})
+}