diff --git a/changelogs/unreleased/10275-kaovilai b/changelogs/unreleased/10275-kaovilai new file mode 100644 index 000000000..ad6379cd3 --- /dev/null +++ b/changelogs/unreleased/10275-kaovilai @@ -0,0 +1 @@ +Add includedNamespacesByLabel/excludedNamespacesByLabel and labelSelectorLogic to ResourcePolicy's includeExcludePolicy, allowing namespace selection by Kubernetes label selector without enumerating names in BackupSpec. diff --git a/internal/resourcepolicies/resource_policies.go b/internal/resourcepolicies/resource_policies.go index ad8f06ee4..9b8b1da7d 100644 --- a/internal/resourcepolicies/resource_policies.go +++ b/internal/resourcepolicies/resource_policies.go @@ -223,13 +223,190 @@ type IncludeExcludePolicy struct { ExcludedClusterScopedResources []string `yaml:"excludedClusterScopedResources"` IncludedNamespaceScopedResources []string `yaml:"includedNamespaceScopedResources"` ExcludedNamespaceScopedResources []string `yaml:"excludedNamespaceScopedResources"` + + // IncludedNamespacesByLabel and ExcludedNamespacesByLabel are lists of Kubernetes + // label selector strings (same syntax as `kubectl get ns -l `, parsed via + // labels.Parse). At backup time, each selector is evaluated against the live namespace + // list to dynamically resolve which namespaces to include/exclude, without requiring + // namespaces to be enumerated by name in BackupSpec. + IncludedNamespacesByLabel []string `yaml:"includedNamespacesByLabel,omitempty"` + ExcludedNamespacesByLabel []string `yaml:"excludedNamespacesByLabel,omitempty"` + + // LabelSelectorLogic controls how multiple entries within IncludedNamespacesByLabel are + // combined with each other, and independently how multiple entries within + // ExcludedNamespacesByLabel are combined with each other: "OR" (default) matches a + // namespace against any entry in the list; "AND" requires a namespace to match every + // entry in the list. Empty string is treated as "OR". Matching is case-insensitive + // ("and"/"Or" are accepted the same as "AND"/"OR"). This is unrelated to the + // comma-separated AND semantics within a single selector string, which is standard + // labels.Parse syntax. + LabelSelectorLogic string `yaml:"labelSelectorLogic,omitempty"` } func (p *IncludeExcludePolicy) Validate() error { if err := p.validateIncludeExclude(p.IncludedClusterScopedResources, p.ExcludedClusterScopedResources); err != nil { return err } - return p.validateIncludeExclude(p.IncludedNamespaceScopedResources, p.ExcludedNamespaceScopedResources) + if err := p.validateIncludeExclude(p.IncludedNamespaceScopedResources, p.ExcludedNamespaceScopedResources); err != nil { + return err + } + if err := validateLabelSelectors(p.IncludedNamespacesByLabel); err != nil { + return fmt.Errorf("includedNamespacesByLabel: %w", err) + } + if err := validateLabelSelectors(p.ExcludedNamespacesByLabel); err != nil { + return fmt.Errorf("excludedNamespacesByLabel: %w", err) + } + return validateLabelSelectorLogic(p.LabelSelectorLogic) +} + +// validateLabelSelectors returns an error if any selector string is empty/whitespace-only +// (which labels.Parse would otherwise silently accept as labels.Everything(), matching +// every namespace) or fails to parse as a Kubernetes label selector. +func validateLabelSelectors(selectors []string) error { + for _, s := range selectors { + if strings.TrimSpace(s) == "" { + return fmt.Errorf("label selector cannot be empty") + } + if _, err := labels.Parse(s); err != nil { + return fmt.Errorf("invalid label selector %q: %w", s, err) + } + } + return nil +} + +func validateLabelSelectorLogic(logic string) error { + switch strings.ToUpper(logic) { + case "", "OR", "AND": + return nil + default: + return fmt.Errorf("labelSelectorLogic must be \"OR\" or \"AND\", got %q", logic) + } +} + +// ResolveNamespacesByLabel lists all cluster namespaces and returns two independently +// resolved name sets: those matching includedSelectors, and those matching +// excludedSelectors, combined per logic ("OR": any selector in the list matches; "AND": +// every selector in the list matches; "" defaults to "OR", case-insensitive). It performs no +// cross-suppression between the two sets - the caller decides how to combine them with +// BackupSpec.IncludedNamespaces/ExcludedNamespaces. Although the production path already +// validates selectors and logic before reaching here (see validateLabelSelectors/ +// validateLabelSelectorLogic, called from Validate()), this function re-validates both on +// entry since it is exported: an empty-string selector parses successfully as "match +// everything" (k8s labels.Parse("") is not an error), so skipping this check would let a +// malformed excludedSelectors entry silently exclude nothing instead of failing loudly - +// fail-open, since a namespace meant to be excluded would be backed up instead. +func ResolveNamespacesByLabel( + ctx context.Context, + client crclient.Client, + includedSelectors []string, + excludedSelectors []string, + logic string, +) ([]string, []string, error) { + if err := validateLabelSelectorLogic(logic); err != nil { + return nil, nil, err + } + if err := validateLabelSelectors(includedSelectors); err != nil { + return nil, nil, errors.Wrap(err, "includedNamespacesByLabel") + } + if err := validateLabelSelectors(excludedSelectors); err != nil { + return nil, nil, errors.Wrap(err, "excludedNamespacesByLabel") + } + + nsList := &corev1api.NamespaceList{} + if err := client.List(ctx, nsList); err != nil { + return nil, nil, errors.Wrap(err, "listing namespaces") + } + + matchSet := func(selectors []string) ([]string, error) { + result := sets.NewString() + if len(selectors) == 0 { + return result.List(), nil + } + // Matches validateLabelSelectorLogic's case-insensitive acceptance - "and"/"Or" etc. + // are as valid as "AND"/"OR", so the actual matching must normalize the same way. + isAND := strings.EqualFold(logic, "AND") + parsedSelectors := make([]labels.Selector, 0, len(selectors)) + for _, sel := range selectors { + parsed, err := labels.Parse(sel) + if err != nil { + return nil, fmt.Errorf("invalid label selector %q: %w", sel, err) + } + parsedSelectors = append(parsedSelectors, parsed) + } + for _, ns := range nsList.Items { + nsLabels := labels.Set(ns.Labels) + if isAND { + allMatch := true + for _, parsed := range parsedSelectors { + if !parsed.Matches(nsLabels) { + allMatch = false + break + } + } + if allMatch { + result.Insert(ns.Name) + } + } else { // "OR" (default, including "") + for _, parsed := range parsedSelectors { + if parsed.Matches(nsLabels) { + result.Insert(ns.Name) + break + } + } + } + } + return result.List(), nil + } + + included, err := matchSet(includedSelectors) + if err != nil { + return nil, nil, err + } + excluded, err := matchSet(excludedSelectors) + if err != nil { + return nil, nil, err + } + + return included, excluded, nil +} + +// NoNamespaceMatchesPattern is a namespace glob pattern guaranteed to match zero real +// namespaces - Kubernetes namespace names are RFC 1123 labels that must start and end with an +// alphanumeric character, so no real namespace can ever start with '-' - while still being +// recognized as a wildcard pattern by wildcard.ShouldExpandWildcards/ExpandWildcards. +// +// This matters because a plain empty []string in BackupSpec.IncludedNamespaces is Velero's +// long-standing "include everything" default everywhere else: wildcard.ShouldExpandWildcards +// explicitly treats len(includes)==0 as "equivalent to * (match all) - don't expand". Reusing +// that same empty representation to mean the opposite - "a configured includedNamespacesByLabel +// selector currently matches zero namespaces, so include nothing" - would silently expand to +// "back up every namespace" instead, exactly the opposite of the intended fail-safe. Routing +// through the wildcard-expansion path instead uses the mechanism +// collections.NamespaceIncludesExcludes.ShouldInclude already relies on for "include nothing": +// it returns false for everything once wildcard expansion ran and the expanded includes list +// came back empty, which only happens when the includes list contained an actual wildcard +// pattern (not a plain empty list). +// +// This must be a pattern collections.ValidateNamespaceIncludesExcludes actually accepts, not +// just wildcard.ValidateNamespaceName in isolation: that function replaces glob metacharacters +// (*, ?, [, ]) with a placeholder letter before checking the result against Kubernetes' own +// RFC 1123 namespace-name rules, so a pattern like "[A-Z]*" becomes "xA-Zxx" - the literal +// uppercase A and Z survive that substitution and fail RFC 1123 (lowercase only), even though +// wildcard.ValidateNamespaceName alone would accept it as a syntactically valid glob. "[-]*" +// substitutes to "x-xx", which is a valid RFC 1123 label, so it passes both checks - confirmed +// empirically against collections.ValidateNamespaceIncludesExcludes directly, not just reasoned +// through the substitution rule. +const NoNamespaceMatchesPattern = "[-]*" + +// RepresentNamespaceSelection returns resolved as an effective IncludedNamespaces value, +// substituting NoNamespaceMatchesPattern when resolved is empty so that "the selector matched +// nothing" is represented unambiguously downstream - see NoNamespaceMatchesPattern's doc +// comment for why a plain empty slice cannot be used for this. +func RepresentNamespaceSelection(resolved []string) []string { + if len(resolved) == 0 { + return []string{NoNamespaceMatchesPattern} + } + return resolved } func (p *IncludeExcludePolicy) validateIncludeExclude(includesList, excludesList []string) error { diff --git a/internal/resourcepolicies/resource_policies_test.go b/internal/resourcepolicies/resource_policies_test.go index f75392d6e..6e87c8fe7 100644 --- a/internal/resourcepolicies/resource_policies_test.go +++ b/internal/resourcepolicies/resource_policies_test.go @@ -17,6 +17,7 @@ package resourcepolicies import ( "context" + "fmt" "testing" "github.com/sirupsen/logrus" @@ -27,6 +28,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/client-go/kubernetes/scheme" + crclient "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" @@ -2482,6 +2484,279 @@ includeExcludePolicy: assert.Equal(t, []string{"ClusterRoleBinding"}, iePolicy.ExcludedClusterScopedResources) } +func TestIncludeExcludePolicyValidateNamespacesByLabel(t *testing.T) { + tests := []struct { + name string + policy IncludeExcludePolicy + wantErr string + }{ + { + name: "no label selector fields set is valid", + policy: IncludeExcludePolicy{}, + }, + { + name: "valid included and excluded selectors", + policy: IncludeExcludePolicy{ + IncludedNamespacesByLabel: []string{"team=platform", "team=infra"}, + ExcludedNamespacesByLabel: []string{"env=dev"}, + }, + }, + { + name: "valid AND logic", + policy: IncludeExcludePolicy{ + IncludedNamespacesByLabel: []string{"tier=critical", "compliance=pci"}, + LabelSelectorLogic: "AND", + }, + }, + { + name: "empty string in includedNamespacesByLabel is rejected", + policy: IncludeExcludePolicy{ + IncludedNamespacesByLabel: []string{""}, + }, + wantErr: "includedNamespacesByLabel: label selector cannot be empty", + }, + { + name: "whitespace-only string in excludedNamespacesByLabel is rejected", + policy: IncludeExcludePolicy{ + ExcludedNamespacesByLabel: []string{" "}, + }, + wantErr: "excludedNamespacesByLabel: label selector cannot be empty", + }, + { + name: "invalid selector syntax is rejected", + policy: IncludeExcludePolicy{ + IncludedNamespacesByLabel: []string{"=="}, + }, + wantErr: "includedNamespacesByLabel: invalid label selector", + }, + { + name: "invalid operator is rejected", + policy: IncludeExcludePolicy{ + ExcludedNamespacesByLabel: []string{"env >> prod"}, + }, + wantErr: "excludedNamespacesByLabel: invalid label selector", + }, + { + name: "malformed 'in' clause without parens is rejected", + policy: IncludeExcludePolicy{ + IncludedNamespacesByLabel: []string{"env in prod"}, + }, + wantErr: "includedNamespacesByLabel: invalid label selector", + }, + { + name: "invalid labelSelectorLogic is rejected", + policy: IncludeExcludePolicy{ + LabelSelectorLogic: "XOR", + }, + wantErr: `labelSelectorLogic must be "OR" or "AND", got "XOR"`, + }, + { + name: "lowercase labelSelectorLogic is accepted", + policy: IncludeExcludePolicy{ + IncludedNamespacesByLabel: []string{"team=platform"}, + LabelSelectorLogic: "and", + }, + }, + { + name: "mixed-case labelSelectorLogic is accepted", + policy: IncludeExcludePolicy{ + IncludedNamespacesByLabel: []string{"team=platform"}, + LabelSelectorLogic: "Or", + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + err := tc.policy.Validate() + if tc.wantErr == "" { + assert.NoError(t, err) + } else { + require.Error(t, err) + assert.Contains(t, err.Error(), tc.wantErr) + } + }) + } +} + +func TestResolveNamespacesByLabel(t *testing.T) { + nsWith := func(name string, labels map[string]string) *corev1api.Namespace { + return &corev1api.Namespace{ + ObjectMeta: metav1.ObjectMeta{Name: name, Labels: labels}, + } + } + + namespaces := []crclient.Object{ + nsWith("platform-prod", map[string]string{"team": "platform", "env": "prod"}), + nsWith("platform-dev", map[string]string{"team": "platform", "env": "dev"}), + nsWith("infra", map[string]string{"team": "infra"}), + nsWith("confidential", map[string]string{"confidential": "true"}), + nsWith("unlabeled", nil), + } + + newClient := func() crclient.Client { + return fake.NewClientBuilder().WithScheme(scheme.Scheme).WithObjects(namespaces...).Build() + } + + t.Run("OR logic across included selectors", func(t *testing.T) { + included, excluded, err := ResolveNamespacesByLabel(context.Background(), newClient(), + []string{"team=platform", "team=infra"}, nil, "") + require.NoError(t, err) + assert.ElementsMatch(t, []string{"platform-prod", "platform-dev", "infra"}, included) + assert.Empty(t, excluded) + }) + + t.Run("AND logic across included selectors", func(t *testing.T) { + included, _, err := ResolveNamespacesByLabel(context.Background(), newClient(), + []string{"team=platform", "env=prod"}, nil, "AND") + require.NoError(t, err) + assert.Equal(t, []string{"platform-prod"}, included) + }) + + t.Run("AND logic across excluded selectors", func(t *testing.T) { + // labelSelectorLogic applies independently to each list - covers the excluded half + // of the contract, not just included (which the case above already covers). + _, excluded, err := ResolveNamespacesByLabel(context.Background(), newClient(), + nil, []string{"team=platform", "env=prod"}, "AND") + require.NoError(t, err) + assert.Equal(t, []string{"platform-prod"}, excluded) + }) + + t.Run("AND logic matching is case-insensitive", func(t *testing.T) { + included, _, err := ResolveNamespacesByLabel(context.Background(), newClient(), + []string{"team=platform", "env=prod"}, nil, "and") + require.NoError(t, err) + assert.Equal(t, []string{"platform-prod"}, included) + }) + + t.Run("excluded resolved independently of included", func(t *testing.T) { + included, excluded, err := ResolveNamespacesByLabel(context.Background(), newClient(), + nil, []string{"confidential=true"}, "") + require.NoError(t, err) + assert.Empty(t, included) + assert.Equal(t, []string{"confidential"}, excluded) + }) + + t.Run("configured selector matching zero namespaces returns empty, not all", func(t *testing.T) { + included, _, err := ResolveNamespacesByLabel(context.Background(), newClient(), + []string{"team=nonexistent"}, nil, "") + require.NoError(t, err) + assert.Empty(t, included) + }) + + t.Run("empty selector lists return empty sets", func(t *testing.T) { + included, excluded, err := ResolveNamespacesByLabel(context.Background(), newClient(), nil, nil, "") + require.NoError(t, err) + assert.Empty(t, included) + assert.Empty(t, excluded) + }) + + t.Run("selector on a label key no namespace carries at all resolves to empty", func(t *testing.T) { + included, _, err := ResolveNamespacesByLabel(context.Background(), newClient(), + []string{"nonexistent-key=anything"}, nil, "") + require.NoError(t, err) + assert.Empty(t, included) + }) + + t.Run("existence-check selector (!key) matches namespaces missing that label", func(t *testing.T) { + included, _, err := ResolveNamespacesByLabel(context.Background(), newClient(), + []string{"!confidential"}, nil, "") + require.NoError(t, err) + assert.ElementsMatch(t, []string{"platform-prod", "platform-dev", "infra", "unlabeled"}, included) + }) + + // ResolveNamespacesByLabel is exported and does not itself call Validate() - the + // production path always validates first, but a malformed selector reaching this function + // directly must return an error, not a silent empty result. Silently treating a malformed + // *excluded* selector as "no matches" would be fail-open: a namespace meant to be excluded + // would be backed up instead. + t.Run("malformed included selector returns an error, not a silent empty result", func(t *testing.T) { + _, _, err := ResolveNamespacesByLabel(context.Background(), newClient(), + []string{"=="}, nil, "") + require.Error(t, err) + }) + + t.Run("malformed excluded selector returns an error, not a silent empty result", func(t *testing.T) { + _, _, err := ResolveNamespacesByLabel(context.Background(), newClient(), + nil, []string{"=="}, "") + require.Error(t, err) + }) + + t.Run("empty-string included selector returns an error, not a silent match-everything", func(t *testing.T) { + // k8s labels.Parse("") succeeds and returns a selector that matches everything, so + // without validateLabelSelectors' explicit empty check, this would silently include + // every namespace instead of failing. + _, _, err := ResolveNamespacesByLabel(context.Background(), newClient(), + []string{""}, nil, "") + require.Error(t, err) + }) + + t.Run("empty-string excluded selector returns an error, not a silent match-everything", func(t *testing.T) { + // Same gap as above, but fail-open for excludes: a silently-everything-matching + // excluded selector would exclude every namespace instead of failing loudly. + _, _, err := ResolveNamespacesByLabel(context.Background(), newClient(), + nil, []string{""}, "") + require.Error(t, err) + }) + + t.Run("invalid logic value returns an error, not a silent fall-through to OR", func(t *testing.T) { + // ResolveNamespacesByLabel is exported and does not itself call Validate() - a + // garbage logic value reaching this function directly must be rejected, not silently + // treated as OR (the exact-match comparison a garbage value would otherwise fail, + // widening an intended AND into an OR is fail-open the same way a swallowed selector + // parse error is). + _, _, err := ResolveNamespacesByLabel(context.Background(), newClient(), + []string{"team=platform"}, nil, "XOR") + require.Error(t, err) + }) +} + +// TestResolveNamespacesByLabel_ManyNamespaces is a correctness-at-scale check against a +// cluster with thousands of namespaces - not a timing assertion (BenchmarkResolveNamespacesByLabel +// below covers actual performance). +func TestResolveNamespacesByLabel_ManyNamespaces(t *testing.T) { + fakeClient := manyNamespacesClient() + + included, _, err := ResolveNamespacesByLabel(context.Background(), fakeClient, []string{"team=platform"}, nil, "") + + require.NoError(t, err) + assert.Len(t, included, manyNamespacesMatching) +} + +// manyNamespacesTotal/manyNamespacesMatching/manyNamespacesClient back both +// TestResolveNamespacesByLabel_ManyNamespaces (correctness at scale) and +// BenchmarkResolveNamespacesByLabel (`go test -bench`, not part of a normal `go test` run and +// so can't flake CI the way a fixed wall-clock assertion in a regular test can). +const ( + manyNamespacesTotal = 5000 + manyNamespacesMatching = 137 +) + +func manyNamespacesClient() crclient.Client { + objs := make([]crclient.Object, 0, manyNamespacesTotal) + for i := range manyNamespacesTotal { + nsLabels := map[string]string{"team": "other"} + if i < manyNamespacesMatching { + nsLabels = map[string]string{"team": "platform"} + } + objs = append(objs, &corev1api.Namespace{ + ObjectMeta: metav1.ObjectMeta{Name: fmt.Sprintf("ns-%d", i), Labels: nsLabels}, + }) + } + + return fake.NewClientBuilder().WithScheme(scheme.Scheme).WithObjects(objs...).Build() +} + +func BenchmarkResolveNamespacesByLabel(b *testing.B) { + fakeClient := manyNamespacesClient() + + for range b.N { + if _, _, err := ResolveNamespacesByLabel(context.Background(), fakeClient, []string{"team=platform"}, nil, ""); err != nil { + b.Fatal(err) + } + } +} + func TestFirstMatchSemantics(t *testing.T) { yamlData := `version: v1 namespacedFilterPolicies: diff --git a/pkg/backup/backup_test.go b/pkg/backup/backup_test.go index 8d3e75837..309a2f6ac 100644 --- a/pkg/backup/backup_test.go +++ b/pkg/backup/backup_test.go @@ -5543,6 +5543,32 @@ func TestBackupNamespaces(t *testing.T) { "resources/deployments.apps/v1-preferredversion/namespaces/ns-1/deploy-1.json", }, }, + { + // Regression guard for the design/namespace-label-selector-in-resource-policy_design.md + // Precedence and Interaction trade-off: a namespace admitted into + // BackupSpec.IncludedNamespaces by name (which is exactly what + // resourcepolicies.ResolveNamespacesByLabel + prepareBackupRequest produce for + // includedNamespacesByLabel) still gets its own Namespace object backed up even + // when nothing in it matches a separately configured LabelSelector - + // namespace-selection and resource-selection-by-label are independent axes. + name: "namespace explicitly included is backed up even when nothing inside matches LabelSelector", + backup: defaultBackup().IncludedNamespaces("ns-1"). + LabelSelector(&metav1.LabelSelector{MatchLabels: map[string]string{"team": "platform"}}). + Result(), + apiResources: []*test.APIResource{ + test.Namespaces( + builder.ForNamespace("ns-1").Phase(corev1api.NamespaceActive).Result(), + builder.ForNamespace("ns-2").Phase(corev1api.NamespaceActive).Result(), + ), + test.Deployments( + builder.ForDeployment("ns-1", "deploy-1").Result(), + ), + }, + want: []string{ + "resources/namespaces/cluster/ns-1.json", + "resources/namespaces/v1-preferredversion/cluster/ns-1.json", + }, + }, } itemBlockPool := StartItemBlockWorkerPool(t.Context(), 1, logrus.StandardLogger()) @@ -5571,6 +5597,212 @@ func TestBackupNamespaces(t *testing.T) { } } +// TestBackupWithResourcePolicyNamespaceLabelSelector is an integration-style test spanning +// resourcepolicies.ResolveNamespacesByLabel and the real backup item-collection path: it +// resolves includedNamespacesByLabel against live namespaces the way prepareBackupRequest +// does, combines the result with an explicit BackupSpec.IncludedNamespaces entry the way +// mergeNamespacesByLabel's union branch does, and verifies the resulting backup contains both +// namespaces' resources - simulating a Schedule configured with both a ResourcePolicy label +// selector and an explicit include. +func TestBackupWithResourcePolicyNamespaceLabelSelector(t *testing.T) { + nsPlatform := builder.ForNamespace("platform-ns").ObjectMeta(builder.WithLabels("team", "platform")).Result() + nsOps := builder.ForNamespace("ops-ns").Result() + nsOther := builder.ForNamespace("other-ns").ObjectMeta(builder.WithLabels("team", "infra")).Result() + + fakeClient := test.NewFakeControllerRuntimeClient(t, nsPlatform, nsOps, nsOther) + + resolvedIncluded, _, err := resourcepolicies.ResolveNamespacesByLabel( + t.Context(), fakeClient, []string{"team=platform"}, nil, "") + require.NoError(t, err) + require.Equal(t, []string{"platform-ns"}, resolvedIncluded) + + // "ops-ns" stands in for BackupSpec.IncludedNamespaces already having an explicit entry; + // mergeNamespacesByLabel would union resolvedIncluded into it additively (see + // TestMergeNamespacesByLabel in pkg/controller for that merge decision in isolation). + effectiveIncludes := append([]string{"ops-ns"}, resolvedIncluded...) + + backup := defaultBackup().IncludedNamespaces(effectiveIncludes...).Result() + + itemBlockPool := StartItemBlockWorkerPool(t.Context(), 1, logrus.StandardLogger()) + defer itemBlockPool.Stop() + + h := newHarness(t, itemBlockPool) + req := &Request{ + Backup: backup, + SkippedPVTracker: NewSkipPVTracker(), + BackedUpItems: NewBackedUpItemsMap(), + WorkerPool: itemBlockPool, + } + backupFile := bytes.NewBuffer([]byte{}) + + h.addItems(t, test.Namespaces( + builder.ForNamespace("platform-ns").Phase(corev1api.NamespaceActive).ObjectMeta(builder.WithLabels("team", "platform")).Result(), + builder.ForNamespace("ops-ns").Phase(corev1api.NamespaceActive).Result(), + builder.ForNamespace("other-ns").Phase(corev1api.NamespaceActive).ObjectMeta(builder.WithLabels("team", "infra")).Result(), + )) + h.addItems(t, test.Deployments( + builder.ForDeployment("platform-ns", "app-1").Result(), + builder.ForDeployment("ops-ns", "app-2").Result(), + builder.ForDeployment("other-ns", "app-3").Result(), + )) + + h.backupper.Backup(h.log, req, backupFile, nil, nil, nil) + + assertTarballContents(t, backupFile, + "metadata/version", + "resources/namespaces/cluster/platform-ns.json", + "resources/namespaces/v1-preferredversion/cluster/platform-ns.json", + "resources/namespaces/cluster/ops-ns.json", + "resources/namespaces/v1-preferredversion/cluster/ops-ns.json", + "resources/deployments.apps/namespaces/platform-ns/app-1.json", + "resources/deployments.apps/v1-preferredversion/namespaces/platform-ns/app-1.json", + "resources/deployments.apps/namespaces/ops-ns/app-2.json", + "resources/deployments.apps/v1-preferredversion/namespaces/ops-ns/app-2.json", + ) +} + +// TestBackupResourcePolicyNamespaceLabelSelectorEdgeCases runs several scenarios through the +// real backup item-collection path (not just prepareBackupRequest's intermediate spec value), +// each constructing the same effective IncludedNamespaces/ExcludedNamespaces that +// mergeNamespacesByLabel (pkg/controller) produces for the given resource-policy configuration. +// Guards against an empty include-selector result silently expanding to "back up everything" +// downstream, and covers the explicit-wildcard and exclude-precedence handling. +func TestBackupResourcePolicyNamespaceLabelSelectorEdgeCases(t *testing.T) { + runBackup := func(t *testing.T, backup *velerov1.Backup, namespaces *test.APIResource, resources ...*test.APIResource) *bytes.Buffer { + t.Helper() + + itemBlockPool := StartItemBlockWorkerPool(t.Context(), 1, logrus.StandardLogger()) + defer itemBlockPool.Stop() + + h := newHarness(t, itemBlockPool) + req := &Request{ + Backup: backup, + SkippedPVTracker: NewSkipPVTracker(), + BackedUpItems: NewBackedUpItemsMap(), + WorkerPool: itemBlockPool, + } + backupFile := bytes.NewBuffer([]byte{}) + + h.addItems(t, namespaces) + for _, r := range resources { + h.addItems(t, r) + } + + require.NoError(t, h.backupper.Backup(h.log, req, backupFile, nil, nil, nil)) + return backupFile + } + + t.Run("include selector matching zero namespaces backs up nothing, not everything", func(t *testing.T) { + // mergeNamespacesByLabel's fix: an include selector matching zero namespaces must + // resolve to resourcepolicies.NoNamespaceMatchesPattern, not a bare empty slice + // (which wildcard.ShouldExpandWildcards would otherwise treat as "match everything"). + backup := defaultBackup().IncludedNamespaces(resourcepolicies.NoNamespaceMatchesPattern).Result() + + backupFile := runBackup(t, backup, + test.Namespaces( + builder.ForNamespace("ns-1").Phase(corev1api.NamespaceActive).Result(), + builder.ForNamespace("ns-2").Phase(corev1api.NamespaceActive).Result(), + ), + test.Deployments(builder.ForDeployment("ns-1", "app-1").Result()), + ) + + assertTarballContents(t, backupFile, "metadata/version") + }) + + t.Run("explicit wildcard plus include selector keeps everything, not narrowed", func(t *testing.T) { + // mergeNamespacesByLabel's other fix: an explicitly-configured ["*"] keeps everything + // included regardless of includedNamespacesByLabel, rather than being narrowed down to + // just the label matches. The merge canonicalizes this case back down to ["*"] (see + // TestMergeNamespacesByLabel), which is what's fed in here. + backup := defaultBackup().IncludedNamespaces("*").Result() + + backupFile := runBackup(t, backup, + test.Namespaces( + builder.ForNamespace("platform-ns").Phase(corev1api.NamespaceActive).ObjectMeta(builder.WithLabels("team", "platform")).Result(), + builder.ForNamespace("other-ns").Phase(corev1api.NamespaceActive).Result(), + ), + test.Deployments( + builder.ForDeployment("platform-ns", "app-1").Result(), + builder.ForDeployment("other-ns", "app-2").Result(), + ), + ) + + assertTarballContents(t, backupFile, + "metadata/version", + "resources/namespaces/cluster/platform-ns.json", + "resources/namespaces/v1-preferredversion/cluster/platform-ns.json", + "resources/namespaces/cluster/other-ns.json", + "resources/namespaces/v1-preferredversion/cluster/other-ns.json", + "resources/deployments.apps/namespaces/platform-ns/app-1.json", + "resources/deployments.apps/v1-preferredversion/namespaces/platform-ns/app-1.json", + "resources/deployments.apps/namespaces/other-ns/app-2.json", + "resources/deployments.apps/v1-preferredversion/namespaces/other-ns/app-2.json", + ) + }) + + t.Run("namespace matching both included and excluded label selectors is excluded", func(t *testing.T) { + // A namespace resolved into both resolvedIncluded and resolvedExcluded - exclusion + // wins, same as BackupSpec.ExcludedNamespaces vs IncludedNamespaces always has. + backup := defaultBackup(). + IncludedNamespaces("both-ns", "include-only-ns"). + ExcludedNamespaces("both-ns"). + Result() + + backupFile := runBackup(t, backup, test.Namespaces( + builder.ForNamespace("both-ns").Phase(corev1api.NamespaceActive).Result(), + builder.ForNamespace("include-only-ns").Phase(corev1api.NamespaceActive).Result(), + )) + + assertTarballContents(t, backupFile, + "metadata/version", + "resources/namespaces/cluster/include-only-ns.json", + "resources/namespaces/v1-preferredversion/cluster/include-only-ns.json", + ) + }) + + t.Run("velero.io/exclude-from-backup hard exclusion wins over an include-label match", func(t *testing.T) { + // prepareBackupRequest's ordering guarantee: hard-excluded namespaces are already in + // ExcludedNamespaces by the time includedNamespacesByLabel resolution runs, so a + // namespace that also matches an include selector must still end up excluded. + backup := defaultBackup(). + IncludedNamespaces("hard-excluded-ns", "platform-ns"). + ExcludedNamespaces("hard-excluded-ns"). + Result() + + backupFile := runBackup(t, backup, test.Namespaces( + builder.ForNamespace("hard-excluded-ns").Phase(corev1api.NamespaceActive). + ObjectMeta(builder.WithLabels("velero.io/exclude-from-backup", "true")).Result(), + builder.ForNamespace("platform-ns").Phase(corev1api.NamespaceActive).Result(), + )) + + assertTarballContents(t, backupFile, + "metadata/version", + "resources/namespaces/cluster/platform-ns.json", + "resources/namespaces/v1-preferredversion/cluster/platform-ns.json", + ) + }) + + t.Run("every included namespace also excluded backs up nothing, not everything", func(t *testing.T) { + // mergeNamespacesByLabel's exclude-subtraction step (added to satisfy + // collections.ValidateIncludesExcludes' invariants) can itself empty out the + // included set when every included name is also excluded. When that happens, the + // merge must emit resourcepolicies.NoNamespaceMatchesPattern rather than a bare + // empty include list, which wildcard.ShouldExpandWildcards would otherwise treat as + // "match everything" - the same hazard the zero-match sentinel exists for, reached + // through a different path. + backup := defaultBackup(). + IncludedNamespaces(resourcepolicies.NoNamespaceMatchesPattern). + ExcludedNamespaces("only-ns"). + Result() + + backupFile := runBackup(t, backup, test.Namespaces( + builder.ForNamespace("only-ns").Phase(corev1api.NamespaceActive).Result(), + )) + + assertTarballContents(t, backupFile, "metadata/version") + }) +} + func TestUpdateVolumeInfos(t *testing.T) { timeExample := time.Date(2014, 6, 5, 11, 56, 45, 0, time.Local) now := metav1.NewTime(timeExample) diff --git a/pkg/controller/backup_controller.go b/pkg/controller/backup_controller.go index 7b57bcd89..fec2da678 100644 --- a/pkg/controller/backup_controller.go +++ b/pkg/controller/backup_controller.go @@ -603,7 +603,12 @@ func (b *backupReconciler) prepareBackupRequest(ctx context.Context, backup *vel // Empty IncludedNamespaces means "include all namespaces". Normalize // to ["*"] so that downstream wildcard expansion does not collapse // an empty-includes + wildcard-excludes combination into "back up nothing". - if len(request.Spec.IncludedNamespaces) == 0 { + // Recorded separately from the normalized value below: once normalized, an + // originally-empty list and an explicitly-configured ["*"] are indistinguishable, + // but mergeNamespacesByLabel's replace-vs-union decision needs to tell them apart + // (see its doc comment). + includedNamespacesWereDefaulted := len(request.Spec.IncludedNamespaces) == 0 + if includedNamespacesWereDefaulted { request.Spec.IncludedNamespaces = []string{"*"} } @@ -636,10 +641,130 @@ func (b *backupReconciler) prepareBackupRequest(ctx context.Context, backup *vel request.Status.ValidationErrors = append(request.Status.ValidationErrors, "include-resources, exclude-resources and include-cluster-resources are old filter parameters.\n"+ "They cannot be used with namespace-scoped or fine-grained global filter policies.") } + + // Resolve includedNamespacesByLabel/excludedNamespacesByLabel from the resource policy + // (if configured) against the live namespace list, and merge into the effective + // namespace filter. Must run after the velero.io/exclude-from-backup hard-exclusion + // above, so that the "- BackupSpec.ExcludedNamespaces" term below already carries + // hard-excluded namespaces. + if resourcePolicies != nil && resourcePolicies.GetIncludeExcludePolicy() != nil { + iep := resourcePolicies.GetIncludeExcludePolicy() + if len(iep.IncludedNamespacesByLabel) > 0 || len(iep.ExcludedNamespacesByLabel) > 0 { + resolvedIncluded, resolvedExcluded, err := resourcepolicies.ResolveNamespacesByLabel( + ctx, b.kbClient, + iep.IncludedNamespacesByLabel, iep.ExcludedNamespacesByLabel, iep.LabelSelectorLogic) + if err != nil { + request.Status.ValidationErrors = append(request.Status.ValidationErrors, fmt.Sprintf("error resolving namespace label selectors: %v", err)) + } else { + logger.WithFields(logrus.Fields{ + "includedNamespacesByLabelCount": len(resolvedIncluded), + "excludedNamespacesByLabelCount": len(resolvedExcluded), + }).Info("resolved namespaces by label selector") + logger.WithFields(logrus.Fields{ + "includedNamespacesByLabel": resolvedIncluded, + "excludedNamespacesByLabel": resolvedExcluded, + }).Debug("resolved namespaces by label selector detail") + + request.Spec.IncludedNamespaces, request.Spec.ExcludedNamespaces = mergeNamespacesByLabel( + request.Spec.IncludedNamespaces, + request.Spec.ExcludedNamespaces, + len(iep.IncludedNamespacesByLabel) > 0, + includedNamespacesWereDefaulted, + resolvedIncluded, + resolvedExcluded, + ) + } + } + } + request.ResPolicies = resourcePolicies return request } +// mergeNamespacesByLabel merges a resource policy's resolved includedNamespacesByLabel/ +// excludedNamespacesByLabel name sets into the backup's effective IncludedNamespaces/ +// ExcludedNamespaces, per the design's Precedence and Interaction rules: +// +// - includedNamespaces is assumed already normalized so that an originally-empty +// BackupSpec.IncludedNamespaces reads as the ["*"] wildcard (prepareBackupRequest does +// this normalization earlier, before resource-policy processing runs). +// - includedNamespacesWereDefaulted reports whether that normalization actually fired - +// i.e. whether BackupSpec.IncludedNamespaces was originally empty, as opposed to the user +// having explicitly written ["*"] themselves. The two are indistinguishable by the time +// includedNamespaces reaches this function (both read as ["*"]), so the caller must track +// and pass this separately; inspecting includedNamespaces alone would wrongly narrow an +// explicit ["*"] down to only the label matches instead of leaving it as "everything". +// - labelIncludeActive reports whether includedNamespacesByLabel was *configured* at all +// (not whether it matched anything - a configured selector matching zero namespaces must +// still produce an empty-selection baseline, not fall through to "all namespaces"). +// - When labelIncludeActive and includedNamespacesWereDefaulted, resolvedIncluded REPLACES +// the wildcard baseline (unioning into "all" would still be "all", defeating the feature's +// primary use case of a schedule with no explicit includes) - represented via +// resourcepolicies.RepresentNamespaceSelection so a zero-match result is expressed as a +// wildcard pattern guaranteed to match nothing, not a plain empty slice (which +// wildcard.ShouldExpandWildcards treats as "match everything" - see that function's doc +// comment for why a bare empty list cannot be reused to mean the opposite here). When +// explicit concrete names were already present, resolvedIncluded is unioned in additively +// instead. When the user explicitly wrote ["*"] themselves (includedNamespacesWereDefaulted +// is false but includedNamespaces is already ["*"]), unioning concrete names into it is a +// no-op at match time (IncludesExcludes.ShouldInclude treats a "*" entry as match-everything +// regardless of what else is in the list) but would also violate +// collections.ValidateIncludesExcludes' "'*' must be alone in includes" invariant if the +// merged result were ever re-validated - so this case canonicalizes back down to ["*"] +// instead of widening it. +// - resolvedExcluded is unioned into excludedNamespaces whenever non-empty, regardless of +// labelIncludeActive - excludedNamespacesByLabel is purely subtractive, same role as +// BackupSpec.ExcludedNamespaces today. An empty resolvedExcluded needs no such translation: +// "exclude nothing" is unambiguous as a plain empty list, unlike "include nothing". +// - Finally, unless mergedIncluded is the wildcard, any name present in both merged lists is +// dropped from mergedIncluded (not mergedExcluded) so the two stay mutually exclusive. +// Exclusion already wins over inclusion at match time regardless (same ShouldInclude +// precedence as above), so this changes only the returned representation, not resolved +// backup behavior - it keeps the merged lists satisfying +// collections.ValidateIncludesExcludes' "excludes list cannot contain an item in the +// includes list" invariant too, for the same reason the "*" case above is canonicalized +// rather than left as an invariant-violating pair. +func mergeNamespacesByLabel( + includedNamespaces []string, + excludedNamespaces []string, + labelIncludeActive bool, + includedNamespacesWereDefaulted bool, + resolvedIncluded []string, + resolvedExcluded []string, +) (mergedIncluded []string, mergedExcluded []string) { + mergedIncluded = includedNamespaces + if labelIncludeActive { + switch { + case includedNamespacesWereDefaulted: + mergedIncluded = resourcepolicies.RepresentNamespaceSelection(resolvedIncluded) + case sets.NewString(includedNamespaces...).Has("*"): + mergedIncluded = []string{"*"} + default: + mergedIncluded = sets.NewString(includedNamespaces...).Insert(resolvedIncluded...).List() + } + } + + mergedExcluded = excludedNamespaces + if len(resolvedExcluded) > 0 { + mergedExcluded = sets.NewString(excludedNamespaces...).Insert(resolvedExcluded...).List() + } + + if !sets.NewString(mergedIncluded...).Has("*") { + // Difference can legitimately empty this out entirely (every resolved or explicit + // include also landed in mergedExcluded) - route back through + // RepresentNamespaceSelection so that comes back as the no-match sentinel, not a bare + // empty slice. The same "empty means include everything" hazard that motivated the + // zero-match sentinel above applies here too: an empty result at this point means + // "everything that was included is now excluded", i.e. include nothing, and a plain + // empty []string would be silently reinterpreted downstream as the opposite. + mergedIncluded = resourcepolicies.RepresentNamespaceSelection( + sets.NewString(mergedIncluded...).Difference(sets.NewString(mergedExcluded...)).List(), + ) + } + + return mergedIncluded, mergedExcluded +} + // validateAndGetSnapshotLocations gets a collection of VolumeSnapshotLocation objects that // this backup will use (returned as a map of provider name -> VSL), and ensures: // - each location name in .spec.volumeSnapshotLocations exists as a location diff --git a/pkg/controller/backup_controller_test.go b/pkg/controller/backup_controller_test.go index eb7786f63..453fced37 100644 --- a/pkg/controller/backup_controller_test.go +++ b/pkg/controller/backup_controller_test.go @@ -354,6 +354,417 @@ func TestPrepareBackupRequest_EmptyIncludedNamespacesNormalizedToWildcard(t *tes assert.Equal(t, []string{"*"}, res.Spec.IncludedNamespaces) } +// TestPrepareBackupRequest_IncludedNamespacesByLabel_ReplacesWildcardBaseline verifies that +// when includedNamespacesByLabel is configured and BackupSpec.IncludedNamespaces was left +// empty (normalized to the ["*"] wildcard), the label-resolved namespace set REPLACES the +// wildcard baseline rather than being unioned into it - otherwise "all" unioned with anything +// is still "all", defeating the feature. +func TestPrepareBackupRequest_IncludedNamespacesByLabel_ReplacesWildcardBaseline(t *testing.T) { + formatFlag := logging.FormatText + logger := logging.DefaultLogger(logrus.DebugLevel, formatFlag) + + policyYAML := `version: v1 +includeExcludePolicy: + includedNamespacesByLabel: + - "team=platform" +` + policyConfigMap := &corev1api.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: "ns-label-policy", Namespace: velerov1api.DefaultNamespace}, + Data: map[string]string{"policy": policyYAML}, + } + + backupLocation := builder.ForBackupStorageLocation("velero", "loc-1").Phase(velerov1api.BackupStorageLocationPhaseAvailable).Result() + nsPlatform := builder.ForNamespace("platform-ns").ObjectMeta(builder.WithLabels("team", "platform")).Result() + nsOther := builder.ForNamespace("other-ns").ObjectMeta(builder.WithLabels("team", "infra")).Result() + + fakeClient := velerotest.NewFakeControllerRuntimeClient(t, policyConfigMap, backupLocation, nsPlatform, nsOther) + + apiServer := velerotest.NewAPIServer(t) + discoveryHelper, err := discovery.NewHelper(apiServer.DiscoveryClient, logger) + require.NoError(t, err) + + c := &backupReconciler{ + discoveryHelper: discoveryHelper, + kbClient: fakeClient, + defaultBackupLocation: backupLocation.Name, + clock: &clock.RealClock{}, + formatFlag: formatFlag, + } + + backup := defaultBackup().Result() + backup.Spec.IncludedNamespaces = nil + backup.Spec.ResourcePolicy = &corev1api.TypedLocalObjectReference{Kind: "configmap", Name: "ns-label-policy"} + + res := c.prepareBackupRequest(ctx, backup, logger) + defer res.WorkerPool.Stop() + + assert.Empty(t, res.Status.ValidationErrors) + assert.Equal(t, []string{"platform-ns"}, res.Spec.IncludedNamespaces) +} + +// TestPrepareBackupRequest_IncludedNamespacesByLabel_UnionsWithExplicitIncludes verifies that +// when BackupSpec.IncludedNamespaces already has explicit names, the label-resolved set is +// additive (unioned), not a replacement. +func TestPrepareBackupRequest_IncludedNamespacesByLabel_UnionsWithExplicitIncludes(t *testing.T) { + formatFlag := logging.FormatText + logger := logging.DefaultLogger(logrus.DebugLevel, formatFlag) + + policyYAML := `version: v1 +includeExcludePolicy: + includedNamespacesByLabel: + - "team=platform" +` + policyConfigMap := &corev1api.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: "ns-label-policy", Namespace: velerov1api.DefaultNamespace}, + Data: map[string]string{"policy": policyYAML}, + } + + backupLocation := builder.ForBackupStorageLocation("velero", "loc-1").Phase(velerov1api.BackupStorageLocationPhaseAvailable).Result() + nsPlatform := builder.ForNamespace("platform-ns").ObjectMeta(builder.WithLabels("team", "platform")).Result() + + fakeClient := velerotest.NewFakeControllerRuntimeClient(t, policyConfigMap, backupLocation, nsPlatform) + + apiServer := velerotest.NewAPIServer(t) + discoveryHelper, err := discovery.NewHelper(apiServer.DiscoveryClient, logger) + require.NoError(t, err) + + c := &backupReconciler{ + discoveryHelper: discoveryHelper, + kbClient: fakeClient, + defaultBackupLocation: backupLocation.Name, + clock: &clock.RealClock{}, + formatFlag: formatFlag, + } + + backup := defaultBackup().IncludedNamespaces("explicit-ns").Result() + backup.Spec.ResourcePolicy = &corev1api.TypedLocalObjectReference{Kind: "configmap", Name: "ns-label-policy"} + + res := c.prepareBackupRequest(ctx, backup, logger) + defer res.WorkerPool.Stop() + + assert.Empty(t, res.Status.ValidationErrors) + assert.ElementsMatch(t, []string{"explicit-ns", "platform-ns"}, res.Spec.IncludedNamespaces) +} + +// TestPrepareBackupRequest_IncludedNamespacesByLabel_ZeroMatchesResolvesToNoMatchSentinel +// verifies that a configured includedNamespacesByLabel selector matching zero namespaces +// resolves to resourcepolicies.NoNamespaceMatchesPattern, not a fall-through to "all +// namespaces" - this is deliberate fail-safe behavior, not a bug. +func TestPrepareBackupRequest_IncludedNamespacesByLabel_ZeroMatchesResolvesToNoMatchSentinel(t *testing.T) { + formatFlag := logging.FormatText + logger := logging.DefaultLogger(logrus.DebugLevel, formatFlag) + + policyYAML := `version: v1 +includeExcludePolicy: + includedNamespacesByLabel: + - "team=nonexistent" +` + policyConfigMap := &corev1api.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: "ns-label-policy", Namespace: velerov1api.DefaultNamespace}, + Data: map[string]string{"policy": policyYAML}, + } + + backupLocation := builder.ForBackupStorageLocation("velero", "loc-1").Phase(velerov1api.BackupStorageLocationPhaseAvailable).Result() + nsOther := builder.ForNamespace("other-ns").ObjectMeta(builder.WithLabels("team", "infra")).Result() + + fakeClient := velerotest.NewFakeControllerRuntimeClient(t, policyConfigMap, backupLocation, nsOther) + + apiServer := velerotest.NewAPIServer(t) + discoveryHelper, err := discovery.NewHelper(apiServer.DiscoveryClient, logger) + require.NoError(t, err) + + c := &backupReconciler{ + discoveryHelper: discoveryHelper, + kbClient: fakeClient, + defaultBackupLocation: backupLocation.Name, + clock: &clock.RealClock{}, + formatFlag: formatFlag, + } + + backup := defaultBackup().Result() + backup.Spec.IncludedNamespaces = nil + backup.Spec.ResourcePolicy = &corev1api.TypedLocalObjectReference{Kind: "configmap", Name: "ns-label-policy"} + + res := c.prepareBackupRequest(ctx, backup, logger) + defer res.WorkerPool.Stop() + + assert.Empty(t, res.Status.ValidationErrors) + // Not a plain empty slice - see resourcepolicies.NoNamespaceMatchesPattern's doc comment + // for why a bare empty list here would be silently reinterpreted downstream as "include + // everything" instead of the intended "include nothing". + assert.Equal(t, []string{resourcepolicies.NoNamespaceMatchesPattern}, res.Spec.IncludedNamespaces) +} + +// TestPrepareBackupRequest_IncludedNamespacesByLabel_ExplicitWildcardCanonicalized verifies +// that when BackupSpec.IncludedNamespaces was explicitly set to ["*"] (as opposed to left +// empty and normalized to ["*"]), includedNamespacesByLabel does not narrow the backup down +// to only the label matches - the two must not be conflated (see mergeNamespacesByLabel's doc +// comment). The result stays canonicalized to ["*"] rather than widened to ["*", "platform-ns"]: +// both are equivalent at match time, but only the former satisfies +// collections.ValidateIncludesExcludes' "'*' must be alone in includes" invariant. +func TestPrepareBackupRequest_IncludedNamespacesByLabel_ExplicitWildcardCanonicalized(t *testing.T) { + formatFlag := logging.FormatText + logger := logging.DefaultLogger(logrus.DebugLevel, formatFlag) + + policyYAML := `version: v1 +includeExcludePolicy: + includedNamespacesByLabel: + - "team=platform" +` + policyConfigMap := &corev1api.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: "ns-label-policy", Namespace: velerov1api.DefaultNamespace}, + Data: map[string]string{"policy": policyYAML}, + } + + backupLocation := builder.ForBackupStorageLocation("velero", "loc-1").Phase(velerov1api.BackupStorageLocationPhaseAvailable).Result() + nsPlatform := builder.ForNamespace("platform-ns").ObjectMeta(builder.WithLabels("team", "platform")).Result() + + fakeClient := velerotest.NewFakeControllerRuntimeClient(t, policyConfigMap, backupLocation, nsPlatform) + + apiServer := velerotest.NewAPIServer(t) + discoveryHelper, err := discovery.NewHelper(apiServer.DiscoveryClient, logger) + require.NoError(t, err) + + c := &backupReconciler{ + discoveryHelper: discoveryHelper, + kbClient: fakeClient, + defaultBackupLocation: backupLocation.Name, + clock: &clock.RealClock{}, + formatFlag: formatFlag, + } + + backup := defaultBackup().IncludedNamespaces("*").Result() + backup.Spec.ResourcePolicy = &corev1api.TypedLocalObjectReference{Kind: "configmap", Name: "ns-label-policy"} + + res := c.prepareBackupRequest(ctx, backup, logger) + defer res.WorkerPool.Stop() + + assert.Empty(t, res.Status.ValidationErrors) + assert.ElementsMatch(t, []string{"*"}, res.Spec.IncludedNamespaces) +} + +// TestPrepareBackupRequest_ExcludedNamespacesByLabel_Subtracted verifies excludedNamespacesByLabel +// resolves independently and is merged into BackupSpec.ExcludedNamespaces, regardless of whether +// includedNamespacesByLabel is configured. +func TestPrepareBackupRequest_ExcludedNamespacesByLabel_Subtracted(t *testing.T) { + formatFlag := logging.FormatText + logger := logging.DefaultLogger(logrus.DebugLevel, formatFlag) + + policyYAML := `version: v1 +includeExcludePolicy: + excludedNamespacesByLabel: + - "confidential=true" +` + policyConfigMap := &corev1api.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: "ns-label-policy", Namespace: velerov1api.DefaultNamespace}, + Data: map[string]string{"policy": policyYAML}, + } + + backupLocation := builder.ForBackupStorageLocation("velero", "loc-1").Phase(velerov1api.BackupStorageLocationPhaseAvailable).Result() + nsConfidential := builder.ForNamespace("secret-ns").ObjectMeta(builder.WithLabels("confidential", "true")).Result() + + fakeClient := velerotest.NewFakeControllerRuntimeClient(t, policyConfigMap, backupLocation, nsConfidential) + + apiServer := velerotest.NewAPIServer(t) + discoveryHelper, err := discovery.NewHelper(apiServer.DiscoveryClient, logger) + require.NoError(t, err) + + c := &backupReconciler{ + discoveryHelper: discoveryHelper, + kbClient: fakeClient, + defaultBackupLocation: backupLocation.Name, + clock: &clock.RealClock{}, + formatFlag: formatFlag, + } + + backup := defaultBackup().Result() + backup.Spec.IncludedNamespaces = nil + backup.Spec.ResourcePolicy = &corev1api.TypedLocalObjectReference{Kind: "configmap", Name: "ns-label-policy"} + + res := c.prepareBackupRequest(ctx, backup, logger) + defer res.WorkerPool.Stop() + + assert.Empty(t, res.Status.ValidationErrors) + // includedNamespacesByLabel not configured, so baseline stays the wildcard. + assert.Equal(t, []string{"*"}, res.Spec.IncludedNamespaces) + assert.Equal(t, []string{"secret-ns"}, res.Spec.ExcludedNamespaces) +} + +// TestMergeNamespacesByLabel exercises the union/replacement decision directly, without +// the full prepareBackupRequest scaffold (fake client, discovery helper, resource policy +// ConfigMap, etc.) - see mergeNamespacesByLabel's doc comment for the precedence rules. +func TestMergeNamespacesByLabel(t *testing.T) { + tests := []struct { + name string + includedNamespaces []string + excludedNamespaces []string + labelIncludeActive bool + includedNamespacesWereDefaulted bool + resolvedIncluded []string + resolvedExcluded []string + wantIncluded []string + wantExcluded []string + }{ + { + name: "defaulted wildcard baseline is replaced", + includedNamespaces: []string{"*"}, + labelIncludeActive: true, + includedNamespacesWereDefaulted: true, + resolvedIncluded: []string{"platform-ns"}, + wantIncluded: []string{"platform-ns"}, + wantExcluded: nil, + }, + { + name: "explicit includes union additively", + includedNamespaces: []string{"explicit-ns"}, + labelIncludeActive: true, + includedNamespacesWereDefaulted: false, + resolvedIncluded: []string{"platform-ns"}, + wantIncluded: []string{"explicit-ns", "platform-ns"}, + wantExcluded: nil, + }, + { + // Explicit includes can themselves be a non-"*" wildcard glob (e.g. from + // --include-namespaces 'app-*'), not just concrete names - the union branch must + // pass that through unchanged alongside the resolved concrete names; downstream + // wildcard.ShouldExpandWildcards still expands it normally since it isn't the bare + // "*" special case. + name: "explicit glob-pattern include unions with resolved names unchanged", + includedNamespaces: []string{"app-*"}, + labelIncludeActive: true, + includedNamespacesWereDefaulted: false, + resolvedIncluded: []string{"platform-ns"}, + wantIncluded: []string{"app-*", "platform-ns"}, + wantExcluded: nil, + }, + { + // A bare empty []string here would be interpreted downstream by + // wildcard.ShouldExpandWildcards as "match everything" (its own documented + // behavior), silently defeating the fail-safe. Must come back as + // resourcepolicies.NoNamespaceMatchesPattern instead - see + // mergeNamespacesByLabel's doc comment. + name: "defaulted wildcard matching zero namespaces resolves to the no-match sentinel, not empty", + includedNamespaces: []string{"*"}, + labelIncludeActive: true, + includedNamespacesWereDefaulted: true, + resolvedIncluded: nil, + wantIncluded: []string{resourcepolicies.NoNamespaceMatchesPattern}, + wantExcluded: nil, + }, + { + // The code must not re-derive "was this defaulted" by checking + // includedNamespaces == ["*"], since an explicitly-configured wildcard looks + // identical to the normalized default by this point. An explicit ["*"] must stay + // "everything", never narrow to just the label matches - canonicalized back to + // ["*"] rather than widened to ["*", "platform-ns"], since the latter is + // semantically identical at match time but would violate + // collections.ValidateIncludesExcludes' "'*' must be alone in includes" invariant. + name: "explicitly-configured wildcard canonicalizes to itself instead of widening", + includedNamespaces: []string{"*"}, + labelIncludeActive: true, + includedNamespacesWereDefaulted: false, + resolvedIncluded: []string{"platform-ns"}, + wantIncluded: []string{"*"}, + wantExcluded: nil, + }, + { + name: "explicitly-configured wildcard stays everything even on zero matches", + includedNamespaces: []string{"*"}, + labelIncludeActive: true, + includedNamespacesWereDefaulted: false, + resolvedIncluded: nil, + wantIncluded: []string{"*"}, + wantExcluded: nil, + }, + { + name: "not labelIncludeActive leaves includedNamespaces untouched", + includedNamespaces: []string{"*"}, + labelIncludeActive: false, + includedNamespacesWereDefaulted: true, + resolvedIncluded: []string{"platform-ns"}, // should be ignored + wantIncluded: []string{"*"}, + wantExcluded: nil, + }, + { + name: "resolvedExcluded unions in regardless of labelIncludeActive", + includedNamespaces: []string{"*"}, + excludedNamespaces: []string{"legacy-ns"}, + labelIncludeActive: false, + includedNamespacesWereDefaulted: true, + resolvedExcluded: []string{"secret-ns"}, + wantIncluded: []string{"*"}, + wantExcluded: []string{"legacy-ns", "secret-ns"}, + }, + { + name: "empty resolvedExcluded leaves excludedNamespaces untouched", + includedNamespaces: []string{"*"}, + excludedNamespaces: []string{"legacy-ns"}, + labelIncludeActive: false, + includedNamespacesWereDefaulted: true, + resolvedExcluded: nil, + wantIncluded: []string{"*"}, + wantExcluded: []string{"legacy-ns"}, + }, + { + // A resolved-include name overlapping an already-excluded name violates + // collections.ValidateIncludesExcludes' "excludes list cannot contain an item in + // the includes list" invariant if the merged result were ever re-validated. + // Exclusion already wins at match time regardless (IncludesExcludes.ShouldInclude + // checks excludes first), so dropping the overlap from mergedIncluded changes only + // the returned representation, not resolved backup behavior. + name: "a resolved include overlapping an existing exclude is dropped from the merged includes", + includedNamespaces: []string{"explicit-ns"}, + excludedNamespaces: []string{"both-ns"}, + labelIncludeActive: true, + includedNamespacesWereDefaulted: false, + resolvedIncluded: []string{"both-ns", "platform-ns"}, + wantIncluded: []string{"explicit-ns", "platform-ns"}, + wantExcluded: []string{"both-ns"}, + }, + { + // The exclude-subtraction step itself can produce a bare empty []string when + // every explicit include is also excluded - the same "empty means include + // everything" hazard the zero-match sentinel exists for, reached through a + // different path. + name: "an explicit include fully removed by exclusion resolves to the no-match sentinel, not empty", + includedNamespaces: []string{"explicit-ns"}, + labelIncludeActive: true, + includedNamespacesWereDefaulted: false, + resolvedExcluded: []string{"explicit-ns"}, + wantIncluded: []string{resourcepolicies.NoNamespaceMatchesPattern}, + wantExcluded: []string{"explicit-ns"}, + }, + { + // Same hazard, but for a defaulted-wildcard-replaced resolved include (rather + // than an explicit one) that a same-namespace excludedNamespacesByLabel match + // fully removes. + name: "a resolved include fully removed by exclusion resolves to the no-match sentinel, not empty", + includedNamespaces: []string{"*"}, + labelIncludeActive: true, + includedNamespacesWereDefaulted: true, + resolvedIncluded: []string{"both-ns"}, + resolvedExcluded: []string{"both-ns"}, + wantIncluded: []string{resourcepolicies.NoNamespaceMatchesPattern}, + wantExcluded: []string{"both-ns"}, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + gotIncluded, gotExcluded := mergeNamespacesByLabel( + tc.includedNamespaces, + tc.excludedNamespaces, + tc.labelIncludeActive, + tc.includedNamespacesWereDefaulted, + tc.resolvedIncluded, + tc.resolvedExcluded, + ) + assert.ElementsMatch(t, tc.wantIncluded, gotIncluded) + assert.ElementsMatch(t, tc.wantExcluded, gotExcluded) + }) + } +} + func Test_prepareBackupRequest_BackupStorageLocation(t *testing.T) { var ( defaultBackupTTL = metav1.Duration{Duration: 24 * 30 * time.Hour} diff --git a/site/content/docs/main/resource-filtering.md b/site/content/docs/main/resource-filtering.md index 94838fe99..32e343123 100644 --- a/site/content/docs/main/resource-filtering.md +++ b/site/content/docs/main/resource-filtering.md @@ -266,6 +266,7 @@ Resource policies support both **Backup** and **Restore** operations, though cer | `clusterScopedFilterPolicy` | Fine-grained cluster-scoped filter overlays with per-kind label selectors and resource name patterns. | **Backup** & **Restore** | [Fine-Grained Backup Filters](fine-grained-backup-filters.md) / [Fine-Grained Restore Filters](fine-grained-restore-filters.md) | | `volumePolicies` | Rules to control volume data backup methods (`skip`, `snapshot`, `fs-backup`) based on conditions. | **Backup** only | See [VolumePolicy](#volumepolicy-backup-only) | | `includeExcludePolicy` | Reusable scoped resource include/exclude filters. | **Backup** only | See [IncludeExcludePolicy](#includeexcludepolicy-backup-only) | +| `includeExcludePolicy.includedNamespacesByLabel` / `excludedNamespacesByLabel` / `labelSelectorLogic` | Dynamically include/exclude whole namespaces by label selector; `labelSelectorLogic` picks OR (default, any entry matches) or AND (every entry must match) across multiple entries in the same list. | **Backup** only | See [Namespace selection by label](#namespace-selection-by-label-backup-only) | ### Creating and referencing resource policies @@ -424,6 +425,101 @@ velero backup create --resource-policies-configmap my-policy --inc The backup will include all resources in namespace `my-workload-ns`, including `configmap` and `event`, and all CRDs and `apiservices` in the cluster. +### Namespace selection by label (Backup only) +`includedNamespacesByLabel` and `excludedNamespacesByLabel` let you dynamically include or exclude entire namespaces from +a backup based on Kubernetes label selectors applied to the namespace objects themselves, without enumerating namespace +names in `BackupSpec` or a schedule. This is useful when namespaces are created and labeled dynamically and you don't want +to update `--include-namespaces`/`--exclude-namespaces` (or a schedule) every time. + +Both fields live in `includeExcludePolicy`, alongside the resource-scoped filters above, and are lists of Kubernetes label +selector strings (same syntax as `kubectl get ns -l `). A realistic policy commonly sets both together — include +any namespace opted into a schedule by team, but always exclude confidential ones regardless of team: + +```yaml +version: v1 +includeExcludePolicy: + includedNamespacesByLabel: + - "velero-backup-schedule=weekly" + - "team=platform" + excludedNamespacesByLabel: + - "confidential=true" +``` + +With the policy above, a namespace labeled `velero-backup-schedule=weekly` AND `confidential=true` is still excluded — +`excludedNamespacesByLabel` always wins (see [Precedence](#precedence) below). + +By default, multiple entries within `includedNamespacesByLabel` (and, independently, within `excludedNamespacesByLabel`) +are OR'd together: a namespace matching *any* entry in the list is included/excluded. Set `labelSelectorLogic: "AND"` to +require a namespace to match *every* entry in the list instead: + +```yaml +version: v1 +includeExcludePolicy: + labelSelectorLogic: "AND" + includedNamespacesByLabel: + - "tier=critical" + - "compliance=pci" +``` + +(A single selector string can already express AND via comma-separated requirements, e.g. `"tier=critical,compliance=pci"` +— that's standard `labels.Parse` syntax and unrelated to `labelSelectorLogic`, which only controls how *separate list +entries* combine.) + +#### Precedence + +- If `includedNamespacesByLabel` is configured and `BackupSpec.IncludedNamespaces` was left empty (the common case — a + schedule with no explicit namespace list), the namespaces resolved by label become the entire inclusion baseline + instead of "all namespaces." If the selector currently matches nothing, the backup selects nothing — it does not fall + back to "everything." +- If `BackupSpec.IncludedNamespaces` is also set explicitly, the label-resolved namespaces are added to that list. An + explicit `--include-namespaces '*'` is treated the same way as any other explicit value here — it is **preserved**, + not narrowed down to just the label matches, since `*` already means "every namespace" regardless of what else is in + the list. +- `excludedNamespacesByLabel` always subtracts from the effective set, the same way `--exclude-namespaces` does, whether + or not `includedNamespacesByLabel` is configured. +- `BackupSpec.LabelSelector`/`--selector` is unaffected by any of this — it continues to filter individual resources, not + namespaces. A namespace selected via `includedNamespacesByLabel` gets its own `Namespace` object backed up even if that + namespace doesn't separately match `--selector`; resources inside it are still filtered by `--selector` as usual. This + matches how an explicitly-named `--include-namespaces` entry already behaves today. + +This union-vs-replacement distinction is subtle but matters in practice — the same `includedNamespacesByLabel` policy +produces a different effective namespace set depending on what else is configured on the backup: + +| `BackupSpec.IncludedNamespaces` | `includedNamespacesByLabel` matches | Effective included namespaces | +| --- | --- | --- | +| *(empty)* | `team-a`, `team-b` | `team-a`, `team-b` (**replaces** the "all namespaces" default) | +| `ops` | `team-a`, `team-b` | `ops`, `team-a`, `team-b` (**unions** with the explicit list) | +| `*` (explicit) | `team-a`, `team-b` | `*` (**preserved as-is** — already "every namespace", not narrowed) | +| *(empty)* | *(no matches yet)* | *(none)* — not "all namespaces" | + +Selectors are evaluated once, when the backup starts — a namespace labeled to match *after* that point isn't picked up +until the next backup runs. The resolved names are written into the created Backup's own `spec.includedNamespaces` (and +`spec.excludedNamespaces`), so `velero backup describe` and `kubectl get backup -o yaml` show exactly which +namespaces were actually selected. When a selector matches nothing, that field shows the internal `[-]*` pattern +(`resourcepolicies.NoNamespaceMatchesPattern`) rather than an actual namespace name — a placeholder namespace glob +guaranteed to match nothing, not a sign anything went wrong. + +#### Limitations + +- Not usable in a `RestoreSpec.ResourcePolicy` ConfigMap — like the rest of `includeExcludePolicy`, a ConfigMap containing + these fields is rejected for Restore. +- Not honored in the global `--global-backup-volume-policies-configmap`; set these on a per-backup (or per-schedule) ResourcePolicy + ConfigMap referenced via `--resource-policies-configmap` / `BackupSpec.ResourcePolicy`. +- An empty (or whitespace-only) selector string is rejected at validation time rather than silently matching every + namespace. +- `labelSelectorLogic` applies the same OR/AND choice to both `includedNamespacesByLabel` and + `excludedNamespacesByLabel` when both are set — there's no way to set AND for one and OR for the other. Setting + `"AND"` to narrow which namespaces are included also requires *every* `excludedNamespacesByLabel` entry to match + before a namespace is excluded, which excludes fewer namespaces than the OR default. If you rely on + `excludedNamespacesByLabel` as a safety net, keep it to a single selector entry (comma-separated requirements + within that one entry already express AND, independent of `labelSelectorLogic`) so its behavior doesn't change + based on how `includedNamespacesByLabel` is tuned. +- Narrowing `BackupSpec.IncludedNamespaces` from "all namespaces" down to a resolved subset — whether via + `includedNamespacesByLabel` or a plain explicit namespace list — also stops cluster-scoped resources (CRDs, + ClusterRoles, StorageClasses, etc.) from being backed up by default, per Velero's existing + `IncludeClusterResources` auto-detection (unset means "only include cluster-scoped resources on a full, + all-namespaces backup"). Set `includeClusterResources: true` on the backup explicitly if you still want them. + ### VolumePolicy (Backup only) VolumePolicy is a data structure to control how velero handle the volumes matching certain conditions.