From 642a5bdf873be71c0af74e2b2b6b97ea0fb92cc9 Mon Sep 17 00:00:00 2001 From: KrishhnaT Date: Wed, 21 Oct 2026 18:40:17 +0530 Subject: [PATCH] e2e: match --features as the comma-separated list it is (#10661) * e2e: match --features as the comma-separated list it is --features takes a comma-separated list, but every check for EnableCSI in the suite compares the whole flag value against the single feature name. With --features=EnableCSI,EnableAPIGroupVersions none of them match, so a CSI run quietly behaves as a non-CSI one: BeforeSuite skips installing the VolumeSnapshotClass, installKibishii picks the provider's plain overlay rather than its CSI one, and BuildSnapshotCheckPointFromVolumeInfo returns a checkpoint with EnableCSI false and no snapshot handles, so the CSI snapshot count is never compared against the expected one. One site already used strings.Contains instead, which handles a list but matches any feature whose name merely starts with this one, and misses a value that differs only in case. Add FeatureEnabled, which splits on commas and compares each entry whole and case-insensitively, and route all eleven call sites through it. The table test covers the list, case and prefix shapes that the two previous idioms each got wrong. hack/test.sh listed only ./pkg/..., ./internal/... and ./cmd/..., so nothing under test/ was ever unit tested and this table would not have run in CI. Add ./test, which holds the suite's shared types and nothing that needs a cluster. The e2e suites live in subpackages and stay unlisted. Found while reviewing the kind-csi mapping in #10657, which copied the EqualFold form from the Azure and AWS branches beside it. Signed-off-by: krishhna24 * Add changelog for #10661 Signed-off-by: krishhna24 --------- Signed-off-by: krishhna24 --- changelogs/unreleased/10661-krishhna24 | 1 + hack/test.sh | 4 ++ test/e2e/basic/backup-volume-info/base.go | 2 +- test/e2e/e2e_suite_test.go | 4 +- test/e2e/migration/migration.go | 6 +-- test/e2e/upgrade/upgrade.go | 3 +- test/types.go | 15 ++++++++ test/types_test.go | 47 +++++++++++++++++++++++ test/util/kibishii/kibishii_utils.go | 6 +-- test/util/velero/velero_utils.go | 2 +- 10 files changed, 78 insertions(+), 12 deletions(-) create mode 100644 changelogs/unreleased/10661-krishhna24 create mode 100644 test/types_test.go diff --git a/changelogs/unreleased/10661-krishhna24 b/changelogs/unreleased/10661-krishhna24 new file mode 100644 index 000000000..c398c039a --- /dev/null +++ b/changelogs/unreleased/10661-krishhna24 @@ -0,0 +1 @@ +Match --features as the comma-separated list it is when checking for EnableCSI diff --git a/hack/test.sh b/hack/test.sh index 61756b12e..0ab625ba9 100755 --- a/hack/test.sh +++ b/hack/test.sh @@ -24,6 +24,10 @@ export CGO_ENABLED=0 TARGETS=($(go list ./pkg/... ./internal/...| grep -vE "/pkg/builder|pkg/apis|pkg/test|pkg/generated|pkg/plugin/generated|mocks|internal/restartabletest")) TARGETS+=( ./cmd/... + # The test/ root package only holds the e2e suite's shared types and their + # unit tests. The e2e suites themselves live in subpackages and are not + # listed, since running them needs a cluster. + ./test ) if [[ ${#@} -ne 0 ]]; then diff --git a/test/e2e/basic/backup-volume-info/base.go b/test/e2e/basic/backup-volume-info/base.go index 9a21c38a4..f3c059ac8 100644 --- a/test/e2e/basic/backup-volume-info/base.go +++ b/test/e2e/basic/backup-volume-info/base.go @@ -78,7 +78,7 @@ func (v *BackupVolumeInfo) Init() error { } func (v *BackupVolumeInfo) Start() error { - if strings.Contains(v.VeleroCfg.Features, FeatureCSI) { + if FeatureEnabled(v.VeleroCfg.Features, FeatureCSI) { if strings.Contains(v.CaseBaseName, "native-snapshot") { fmt.Printf("Skip native snapshot case %s when the CSI feature is enabled.\n", v.CaseBaseName) Skip("Skip native snapshot case due to CSI feature is enabled.") diff --git a/test/e2e/e2e_suite_test.go b/test/e2e/e2e_suite_test.go index 8ff609a64..7806dce7f 100644 --- a/test/e2e/e2e_suite_test.go +++ b/test/e2e/e2e_suite_test.go @@ -823,7 +823,7 @@ var _ = BeforeSuite(func() { By("Install StorageClass for E2E.") Expect(veleroutil.InstallStorageClasses(test.VeleroCfg.CloudProvider)).To(Succeed()) - if strings.EqualFold(test.VeleroCfg.Features, test.FeatureCSI) && + if test.FeatureEnabled(test.VeleroCfg.Features, test.FeatureCSI) && test.VeleroCfg.UseVolumeSnapshots { By("Install VolumeSnapshotClass for E2E.") Expect( @@ -881,7 +881,7 @@ var _ = AfterSuite(func() { ), ).To(Succeed()) - if strings.EqualFold(test.VeleroCfg.Features, test.FeatureCSI) && + if test.FeatureEnabled(test.VeleroCfg.Features, test.FeatureCSI) && test.VeleroCfg.UseVolumeSnapshots { By("Delete VolumeSnapshotClass created by E2E") Expect( diff --git a/test/e2e/migration/migration.go b/test/e2e/migration/migration.go index c5ba68cf3..6495c4d6e 100644 --- a/test/e2e/migration/migration.go +++ b/test/e2e/migration/migration.go @@ -285,7 +285,7 @@ func (m *migrationE2E) Backup() error { // the snapshots of AWS may be still in pending status when do the restore. // wait for a while to avoid this https://github.com/velero-io/velero/issues/1799 if OriginVeleroCfg.CloudProvider == test.Azure && - strings.EqualFold(OriginVeleroCfg.Features, test.FeatureCSI) || + test.FeatureEnabled(OriginVeleroCfg.Features, test.FeatureCSI) || OriginVeleroCfg.CloudProvider == test.AWS { By("Sleep 5 minutes to avoid snapshot recreated by unknown reason ", func() { time.Sleep(5 * time.Minute) @@ -355,7 +355,7 @@ func (m *migrationE2E) Restore() error { test.VeleroCfg.StandbyClient.Kubebuilder, )).To(Succeed()) - if strings.EqualFold(m.VeleroCfg.Features, test.FeatureCSI) && + if test.FeatureEnabled(m.VeleroCfg.Features, test.FeatureCSI) && m.VeleroCfg.UseVolumeSnapshots { By("Install VolumeSnapshotClass for E2E.") Expect( @@ -488,7 +488,7 @@ func (m *migrationE2E) Clean() error { m.VeleroCfg.ClientToInstallVelero.Kubebuilder, )).To(Succeed()) - if strings.EqualFold(m.VeleroCfg.Features, test.FeatureCSI) && + if test.FeatureEnabled(m.VeleroCfg.Features, test.FeatureCSI) && m.VeleroCfg.UseVolumeSnapshots { By("Delete VolumeSnapshotClass created by E2E") if err := k8sutil.KubectlDeleteByFile( diff --git a/test/e2e/upgrade/upgrade.go b/test/e2e/upgrade/upgrade.go index 03262a8f9..fd39782fd 100644 --- a/test/e2e/upgrade/upgrade.go +++ b/test/e2e/upgrade/upgrade.go @@ -19,7 +19,6 @@ import ( "context" "flag" "fmt" - "strings" "time" "github.com/google/uuid" @@ -224,7 +223,7 @@ func BackupUpgradeRestoreTest(useVolumeSnapshots bool, veleroCLI2Version VeleroC fmt.Sprintf("failed to delete namespace %s", upgradeNamespace)) }) - if useVolumeSnapshots && veleroCfg.CloudProvider == Azure && strings.EqualFold(veleroCfg.Features, FeatureCSI) { + if useVolumeSnapshots && veleroCfg.CloudProvider == Azure && FeatureEnabled(veleroCfg.Features, FeatureCSI) { // Upgrade test is not running daily since no CSI plugin v1.0 released, because builds before // v1.0 have issues to fail upgrade case. By("Sleep 5 minutes to avoid snapshot recreated by unknown reason ", func() { diff --git a/test/types.go b/test/types.go index 2788eb86d..794f9ca49 100644 --- a/test/types.go +++ b/test/types.go @@ -17,6 +17,7 @@ limitations under the License. package test import ( + "strings" "time" "github.com/google/uuid" @@ -52,6 +53,20 @@ func SecondStorageClassInstalled() bool { return !(StorageClassFile2Set && StorageClassFile2 == "") } +// FeatureEnabled reports whether name is one of the features in a --features +// value. The flag takes a comma-separated list, so comparing the whole value +// against a single feature name stops matching as soon as a second feature is +// enabled, and comparing with strings.Contains matches any feature whose name +// merely starts with this one. +func FeatureEnabled(features, name string) bool { + for _, f := range strings.Split(features, ",") { + if strings.EqualFold(strings.TrimSpace(f), name) { + return true + } + } + return false +} + const FeatureCSI = "EnableCSI" const VanillaZFS = "vanilla-zfs" const Kind = "kind" diff --git a/test/types_test.go b/test/types_test.go new file mode 100644 index 000000000..137f74ec8 --- /dev/null +++ b/test/types_test.go @@ -0,0 +1,47 @@ +/* +Copyright the Velero contributors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package test + +import "testing" + +func TestFeatureEnabled(t *testing.T) { + tests := []struct { + name string + features string + feature string + want bool + }{ + {"empty value", "", FeatureCSI, false}, + {"exact match", "EnableCSI", FeatureCSI, true}, + {"different case", "enablecsi", FeatureCSI, true}, + {"first of a list", "EnableCSI,EnableAPIGroupVersions", FeatureCSI, true}, + {"last of a list", "EnableAPIGroupVersions,EnableCSI", FeatureCSI, true}, + {"middle of a list", "A,EnableCSI,B", FeatureCSI, true}, + {"surrounding spaces", " EnableCSI ", FeatureCSI, true}, + {"spaces after a comma", "EnableAPIGroupVersions, EnableCSI", FeatureCSI, true}, + {"absent", "EnableAPIGroupVersions", FeatureCSI, false}, + {"longer name sharing the prefix", "EnableCSIFoo", FeatureCSI, false}, + {"name as a substring of another", "NotEnableCSI", FeatureCSI, false}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := FeatureEnabled(tc.features, tc.feature); got != tc.want { + t.Errorf("FeatureEnabled(%q, %q) = %v, want %v", tc.features, tc.feature, got, tc.want) + } + }) + } +} diff --git a/test/util/kibishii/kibishii_utils.go b/test/util/kibishii/kibishii_utils.go index 651bdde11..bf62c06de 100644 --- a/test/util/kibishii/kibishii_utils.go +++ b/test/util/kibishii/kibishii_utils.go @@ -313,17 +313,17 @@ func installKibishii( workerOS string, ) error { if strings.EqualFold(cloudPlatform, Azure) && - strings.EqualFold(veleroFeatures, FeatureCSI) { + FeatureEnabled(veleroFeatures, FeatureCSI) { cloudPlatform = AzureCSI } if strings.EqualFold(cloudPlatform, AWS) && - strings.EqualFold(veleroFeatures, FeatureCSI) { + FeatureEnabled(veleroFeatures, FeatureCSI) { cloudPlatform = AwsCSI } // Keep this in step with KibishiiVolumesAreHostPath above, which is its // inverse: the cases skip exactly when this branch does not fire. if strings.EqualFold(cloudPlatform, Kind) && - strings.EqualFold(veleroFeatures, FeatureCSI) { + FeatureEnabled(veleroFeatures, FeatureCSI) { cloudPlatform = KindCSI } diff --git a/test/util/velero/velero_utils.go b/test/util/velero/velero_utils.go index 85b3643cb..35f5bc4ec 100644 --- a/test/util/velero/velero_utils.go +++ b/test/util/velero/velero_utils.go @@ -1318,7 +1318,7 @@ func BuildSnapshotCheckPointFromVolumeInfo( snapshotCheckPoint.PodName = KibishiiPVCNameList if (veleroCfg.CloudProvider == Azure || veleroCfg.CloudProvider == AWS || - veleroCfg.CloudProvider == Kind) && strings.EqualFold(veleroCfg.Features, FeatureCSI) { + veleroCfg.CloudProvider == Kind) && FeatureEnabled(veleroCfg.Features, FeatureCSI) { snapshotCheckPoint.EnableCSI = true var VscCount = 0