From 40af5efdd0ebfd19e581c9f5f6bd24eb501219da Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Fri, 18 Sep 2026 23:04:55 -0400 Subject: [PATCH] Implement namespace selection by label in resource policy Add includedNamespacesByLabel, excludedNamespacesByLabel, and labelSelectorLogic to IncludeExcludePolicy in the ResourcePolicy ConfigMap (realizes design in velero-io/velero#9772), letting a backup select or exclude namespaces by label instead of (or in addition to) name/wildcard. The backup controller resolves label selectors against the live namespace list once per backup, merges the results into spec.includedNamespaces/excludedNamespaces, then proceeds through the existing name-based filtering unchanged. A defaulted "*" include list is replaced by the resolved set; an explicitly-configured include list (including an explicit "*") is unioned with it instead, and stays canonical rather than widening. Namespaces matching an exclude selector are always subtracted from the merged includes, regardless of how the includes were populated. Because Velero's namespace-includes/excludes model requires at least one name (an empty list means "match everything"), a selector that resolves to zero namespaces is represented with a sentinel glob pattern ("[-]*") guaranteed to match no real namespace, rather than an empty list that would silently fall back to including/excluding everything. labelSelectorLogic ("AND"/"OR", case-insensitive) controls whether multiple included/excluded label selectors are combined by intersection or union; it is validated up front, including inside ResolveNamespacesByLabel itself, so an invalid value fails fast instead of silently falling through to OR semantics. Namespace-selection-by-label and resource-selection-by-label act as independent axes and do not affect each other, matching the design discussion in #9772. Known limitations: - Selectors are evaluated once per backup against the namespace list at that point in time; namespaces created or relabeled mid-backup are not picked up. - Backup-only for now; restore-side namespace mapping is unaffected. Testing: - Unit coverage in internal/resourcepolicies for validation, selector resolution (including AND/OR logic, case-insensitivity, and malformed-selector/invalid-logic error paths), and the no-match sentinel. - Unit coverage in pkg/controller for the merge logic between resolved label selections and explicit/defaulted includes and excludes. - End-to-end coverage in pkg/backup exercising the full backup pipeline with label-selected namespaces, including the velero.io/exclude-from-backup hard-exclusion interaction and the zero-match/fully-excluded sentinel path. Co-Authored-By: Claude Sonnet 5 Signed-off-by: Tiger Kaovilai --- changelogs/unreleased/10275-kaovilai | 1 + .../resourcepolicies/resource_policies.go | 179 +++++++- .../resource_policies_test.go | 275 ++++++++++++ pkg/backup/backup_test.go | 232 ++++++++++ pkg/controller/backup_controller.go | 127 +++++- pkg/controller/backup_controller_test.go | 411 ++++++++++++++++++ site/content/docs/main/resource-filtering.md | 96 ++++ 7 files changed, 1319 insertions(+), 2 deletions(-) create mode 100644 changelogs/unreleased/10275-kaovilai 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.