From 99653bd6afad8fa811c4945035a2df5fc1524732 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Sat, 16 May 2026 15:20:50 -0700 Subject: [PATCH 1/4] chore: bump step-security/harden-runner from 2.19.1 to 2.19.2 (#710) Bumps [step-security/harden-runner](https://github.com/step-security/harden-runner) from 2.19.1 to 2.19.2. - [Release notes](https://github.com/step-security/harden-runner/releases) - [Commits](https://github.com/step-security/harden-runner/compare/a5ad31d6a139d249332a2605b85202e8c0b78450...9ca718d3bf646d6534007c269a635b3e54cadf99) --- updated-dependencies: - dependency-name: step-security/harden-runner dependency-version: 2.19.2 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- .github/workflows/codespell.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/codespell.yml b/.github/workflows/codespell.yml index e00e92948..8a710484b 100644 --- a/.github/workflows/codespell.yml +++ b/.github/workflows/codespell.yml @@ -12,7 +12,7 @@ jobs: runs-on: ubuntu-latest steps: - name: Harden Runner - uses: step-security/harden-runner@a5ad31d6a139d249332a2605b85202e8c0b78450 # v2.19.1 + uses: step-security/harden-runner@9ca718d3bf646d6534007c269a635b3e54cadf99 # v2.19.2 with: egress-policy: audit From 803d3068a0b4ffd973d39a074d216a7564f2d5e0 Mon Sep 17 00:00:00 2001 From: Yetkin Timocin Date: Mon, 18 May 2026 11:58:38 -0700 Subject: [PATCH 2/4] feat: harden property selector validation (#668) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat: harden property selector validation Address the four validator-hardening TODOs from issue #646. Two were stale (the placement validator already enforced them at the API boundary); two are real and now have implementations + thorough tests. What's new: - Bundle operator and value validation into validateOperatorAndValues driven by a single supportedPropertyOperators table. Replaces the prior split validateOperator / validateValues. Preserves the existing "operator X requires exactly one value, got N" wording so existing tests stay stable, and only emits "exactly N values" if a future operator declares requiredValueCount > 1. - Add per-property logical-contradiction detection (validateRequirementsConsistency). Split into named helpers per reviewer feedback: collectRequirementBounds, tightenLower / tightenUpper (with a strict-preferred tiebreak), checkEqVsNe, checkInterval, checkEqInsideBounds. Detects: * two Eq with different values, * Eq + Ne on the same value, * empty intervals from the most-restrictive lower vs the most-restrictive upper, including boundary cases such as Gt x + Lt x, Gt x + Lte x, Gte x + Lt x, * Eq value violating the most-restrictive lower or upper bound. More elaborate cases (sparse Ne exclusion sets, etc.) are left for the property-selector evaluator at scheduling time. What's gone: - Removed two stale TODOs in resource_selector_resolver.go (lines 526, 556). Mutual-exclusiveness of Name vs LabelSelector and labelSelector validity are already enforced in pkg/utils/validator/placement.go (validatePlacement / validateLabelSelector). Replaced with comments pointing readers at the validator. Tests: - TestValidateOperatorAndValues (7 cases including unsupported operator, malformed quantity, count mismatch). - TestValidateRequirementsConsistency (~20 cases: each contradiction variant plus negative controls and malformed-input skipping). - TestRequirementBoundsTighten (4 cases for the "most restrictive wins" + tie-breaks). Boy Scout: switched two interface{} to any in the Service nodePort stripping block. Fixes #646 Co-Authored-By: Claude Code Signed-off-by: Yetkin Timocin * Address feedback Signed-off-by: Yetkin Timocin * Address feedback Consolidate the per-operator bounds-folding behaviour into the supportedPropertyOperators registry so the spec table is the single source of truth for operator validity, required value count, and how each requirement folds into requirementBounds. collectRequirementBounds no longer carries a switch; it dispatches via spec.applyToBounds. - Add operatorSpec{ requiredValueCount, applyToBounds } and rewire validateOperatorAndValues and collectRequirementBounds to read from it. Drop the speculative multi-value error-wording branch in validateOperatorAndValues — no operator declares a count > 1, and a future multi-value operator can update the wording at the time. - Extract (*requirementBounds).applyEq as a method so the spec table can reference it as (*requirementBounds).applyEq. - Add TestApplyEq covering first call / second-equal / second-different branches, with postconditions that eqVal carries the expected value including preservation on the error path. - Add cross-format consistency rows that exercise Cmp through the validator's own logic paths (Eq+Ne and Eq vs upper bound), not apimachinery primitives. - Add TestSupportedPropertyOperatorsRegistryComplete to fail at test time if a future entry is added with a non-positive requiredValueCount or nil applyToBounds. Signed-off-by: Yetkin Timocin * fix: codespell typo (unparseable -> unparsable) Signed-off-by: Yetkin Timocin --------- Signed-off-by: Yetkin Timocin Co-authored-by: Claude Code --- .../controller/resource_selector_resolver.go | 11 +- pkg/utils/validator/placement.go | 233 ++++++++++++++++-- pkg/utils/validator/placement_test.go | 200 +++++++++++++++ 3 files changed, 421 insertions(+), 23 deletions(-) diff --git a/pkg/utils/controller/resource_selector_resolver.go b/pkg/utils/controller/resource_selector_resolver.go index 154ec53a6..1b72b662f 100644 --- a/pkg/utils/controller/resource_selector_resolver.go +++ b/pkg/utils/controller/resource_selector_resolver.go @@ -201,9 +201,9 @@ func generateRawContent(object *unstructured.Unstructured) ([]byte, error) { vals, found, err := unstructured.NestedFieldNoCopy(object.Object, "spec", "ports") if found && err == nil { - if ports, ok := vals.([]interface{}); ok { + if ports, ok := vals.([]any); ok { for i := range ports { - if each, ok := ports[i].(map[string]interface{}); ok { + if each, ok := ports[i].(map[string]any); ok { delete(each, "nodePort") } } @@ -523,7 +523,8 @@ func (rs *ResourceSelectorResolver) fetchResources(selector placementv1beta1.Res lister := rs.InformerManager.Lister(gvr) - // TODO: validator should enforce the mutual exclusiveness between the `name` and `labelSelector` fields + // Mutual exclusiveness between `name` and `labelSelector` is enforced by the placement + // validator (see pkg/utils/validator/placement.go validatePlacement). if len(selector.Name) != 0 { var obj runtime.Object var err error @@ -553,7 +554,9 @@ func (rs *ResourceSelectorResolver) fetchResources(selector placementv1beta1.Res if selector.LabelSelector == nil { labelSelector = labels.Everything() } else { - // TODO: validator should enforce the validity of the labelSelector + // LabelSelector validity is enforced by the placement validator (see + // pkg/utils/validator/placement.go validateLabelSelector). The conversion error path + // remains as a defensive guard. labelSelector, err = metav1.LabelSelectorAsSelector(selector.LabelSelector) if err != nil { return nil, NewUnexpectedBehaviorError(fmt.Errorf("cannot convert the label selector to a selector: %w", err)) diff --git a/pkg/utils/validator/placement.go b/pkg/utils/validator/placement.go index 6c75c89cb..11364e1c8 100644 --- a/pkg/utils/validator/placement.go +++ b/pkg/utils/validator/placement.go @@ -66,6 +66,55 @@ var ( resourceCapacityTypes = supportedResourceCapacityTypes() ) +type operatorSpec struct { + requiredValueCount int + applyToBounds func(*requirementBounds, resource.Quantity) error +} + +// supportedPropertyOperators is the single source of truth for which PropertySelector operators +// are accepted and how each one folds into requirementBounds. +var supportedPropertyOperators = map[placementv1beta1.PropertySelectorOperator]operatorSpec{ + placementv1beta1.PropertySelectorGreaterThan: { + requiredValueCount: 1, + applyToBounds: func(rb *requirementBounds, q resource.Quantity) error { + rb.tightenLower(boundary{q: q, strict: true}) + return nil + }, + }, + placementv1beta1.PropertySelectorGreaterThanOrEqualTo: { + requiredValueCount: 1, + applyToBounds: func(rb *requirementBounds, q resource.Quantity) error { + rb.tightenLower(boundary{q: q, strict: false}) + return nil + }, + }, + placementv1beta1.PropertySelectorLessThan: { + requiredValueCount: 1, + applyToBounds: func(rb *requirementBounds, q resource.Quantity) error { + rb.tightenUpper(boundary{q: q, strict: true}) + return nil + }, + }, + placementv1beta1.PropertySelectorLessThanOrEqualTo: { + requiredValueCount: 1, + applyToBounds: func(rb *requirementBounds, q resource.Quantity) error { + rb.tightenUpper(boundary{q: q, strict: false}) + return nil + }, + }, + placementv1beta1.PropertySelectorEqualTo: { + requiredValueCount: 1, + applyToBounds: (*requirementBounds).applyEq, + }, + placementv1beta1.PropertySelectorNotEqualTo: { + requiredValueCount: 1, + applyToBounds: func(rb *requirementBounds, q resource.Quantity) error { + rb.neVals = append(rb.neVals, q) + return nil + }, + }, +} + // hasNamespaceWithResourceSelectorsMode checks if any namespace selector has NamespaceWithResourceSelectors mode. func hasNamespaceWithResourceSelectorsMode(resourceSelectors []placementv1beta1.ResourceSelectorTerm) bool { for _, selector := range resourceSelectors { @@ -450,17 +499,22 @@ func validatePropertySelector(propertySelector *placementv1beta1.PropertySelecto func validatePropertySelectorRequirements(propertySelectorRequirements []placementv1beta1.PropertySelectorRequirement) error { var allErr []error + // Group requirements by property name so we can also check cross-requirement + // contradictions (e.g., Eq 5 alongside Eq 10, or Gt 10 alongside Lt 5). + byName := make(map[string][]placementv1beta1.PropertySelectorRequirement, len(propertySelectorRequirements)) for _, req := range propertySelectorRequirements { if err := validateName(req.Name); err != nil { allErr = append(allErr, fmt.Errorf("invalid property name %s: %w", req.Name, err)) } - if err := validateOperator(req.Operator, req.Values); err != nil { - allErr = append(allErr, err) + if err := validateOperatorAndValues(req.Operator, req.Values); err != nil { + allErr = append(allErr, fmt.Errorf("invalid requirement on property %s: %w", req.Name, err)) } - if err := validateValues(req.Values); err != nil { - allErr = append(allErr, fmt.Errorf("invalid values for property %s: %w", req.Name, err)) + byName[req.Name] = append(byName[req.Name], req) + } + for name, reqs := range byName { + if err := validateRequirementsConsistency(reqs); err != nil { + allErr = append(allErr, fmt.Errorf("inconsistent requirements on property %s: %w", name, err)) } - // TODO: Check for logical contradictions } return apiErrors.NewAggregate(allErr) } @@ -530,26 +584,167 @@ func validateName(name string) error { return nil } -func validateOperator(op placementv1beta1.PropertySelectorOperator, values []string) error { - // TODO: Restructure for Eq (bundle operator and value validation logic) - validOperators := map[placementv1beta1.PropertySelectorOperator]bool{ - placementv1beta1.PropertySelectorGreaterThan: true, - placementv1beta1.PropertySelectorGreaterThanOrEqualTo: true, - placementv1beta1.PropertySelectorLessThan: true, - placementv1beta1.PropertySelectorLessThanOrEqualTo: true, - placementv1beta1.PropertySelectorEqualTo: true, - placementv1beta1.PropertySelectorNotEqualTo: true, +func validateOperatorAndValues(op placementv1beta1.PropertySelectorOperator, values []string) error { + spec, ok := supportedPropertyOperators[op] + if !ok { + return fmt.Errorf("unsupported operator %s", op) } - if validOperators[op] && len(values) != 1 { + if len(values) != spec.requiredValueCount { return fmt.Errorf("operator %s requires exactly one value, got %d", op, len(values)) } + for _, value := range values { + if _, err := resource.ParseQuantity(value); err != nil { + return fmt.Errorf("value %q is not a valid resource.Quantity: %w", value, err) + } + } return nil } -func validateValues(values []string) error { - for _, value := range values { - if _, err := resource.ParseQuantity(value); err != nil { - return fmt.Errorf("value %s is not a valid resource.Quantity: %w", value, err) +// boundary represents a numeric bound on a property selector requirement. `strict` is true for +// strict-inequality operators (Gt, Lt) and false for inclusive ones (Gte, Lte). +type boundary struct { + q resource.Quantity + strict bool +} + +// requirementBounds is the parsed, normalised view of all requirements on a single property. +// `lower` is the most-restrictive (largest) lower bound, `upper` the most-restrictive (smallest) +// upper bound. `eqVal` is the unique Eq target if any. `neVals` collects all Ne values. +type requirementBounds struct { + lower *boundary + upper *boundary + eqVal *resource.Quantity + neVals []resource.Quantity +} + +// validateRequirementsConsistency reports an error when a set of requirements on the same property +// is logically unsatisfiable. Only requirements that pass validateOperatorAndValues are considered; +// malformed inputs are skipped so callers see one error per requirement rather than cascades. +// +// Cases detected: +// - two Eq requirements with different values +// - Eq and Ne requirements with the same value +// - the most-restrictive lower bound exceeds the most-restrictive upper bound (empty interval), +// including boundary cases Gt x + Lt x, Gt x + Lte x, Gte x + Lt x +// - an Eq value that violates the most-restrictive lower or upper bound +func validateRequirementsConsistency(reqs []placementv1beta1.PropertySelectorRequirement) error { + if len(reqs) < 2 { + return nil + } + bounds, err := collectRequirementBounds(reqs) + if err != nil { + return err + } + if err := bounds.checkEqVsNe(); err != nil { + return err + } + if err := bounds.checkInterval(); err != nil { + return err + } + return bounds.checkEqInsideBounds() +} + +// collectRequirementBounds parses each well-formed requirement into the normalised bounds view by +// dispatching to the operator's applyToBounds. Malformed inputs (unknown operator, wrong value +// count, unparsable quantity) are skipped so validateOperatorAndValues remains the sole source +// of those errors. +func collectRequirementBounds(reqs []placementv1beta1.PropertySelectorRequirement) (*requirementBounds, error) { + out := &requirementBounds{} + for _, req := range reqs { + spec, ok := supportedPropertyOperators[req.Operator] + if !ok || len(req.Values) != spec.requiredValueCount { + continue + } + q, err := resource.ParseQuantity(req.Values[0]) + if err != nil { + continue + } + if spec.applyToBounds == nil { + return nil, fmt.Errorf("internal: operator %s is in supportedPropertyOperators but has no bounds handler", req.Operator) + } + if err := spec.applyToBounds(out, q); err != nil { + return nil, err + } + } + return out, nil +} + +// applyEq is a method (not a closure) so the spec table can reference it as +// (*requirementBounds).applyEq. +func (rb *requirementBounds) applyEq(q resource.Quantity) error { + if rb.eqVal != nil && rb.eqVal.Cmp(q) != 0 { + return fmt.Errorf("conflicting Eq values %s and %s", rb.eqVal.String(), q.String()) + } + rb.eqVal = &q + return nil +} + +// tightenLower keeps the most restrictive (largest, strict-preferred at ties) lower bound. +func (rb *requirementBounds) tightenLower(b boundary) { + if rb.lower == nil || isMoreRestrictiveLower(b, *rb.lower) { + rb.lower = &b + } +} + +// tightenUpper keeps the most restrictive (smallest, strict-preferred at ties) upper bound. +func (rb *requirementBounds) tightenUpper(b boundary) { + if rb.upper == nil || isMoreRestrictiveUpper(b, *rb.upper) { + rb.upper = &b + } +} + +func isMoreRestrictiveLower(a, b boundary) bool { + if c := a.q.Cmp(b.q); c != 0 { + return c > 0 + } + return a.strict && !b.strict +} + +func isMoreRestrictiveUpper(a, b boundary) bool { + if c := a.q.Cmp(b.q); c != 0 { + return c < 0 + } + return a.strict && !b.strict +} + +func (rb *requirementBounds) checkEqVsNe() error { + if rb.eqVal == nil { + return nil + } + for _, ne := range rb.neVals { + if rb.eqVal.Cmp(ne) == 0 { + return fmt.Errorf("conflicting Eq and Ne on same value %s", rb.eqVal.String()) + } + } + return nil +} + +func (rb *requirementBounds) checkInterval() error { + if rb.lower == nil || rb.upper == nil { + return nil + } + cmp := rb.lower.q.Cmp(rb.upper.q) + if cmp > 0 || (cmp == 0 && (rb.lower.strict || rb.upper.strict)) { + return fmt.Errorf("lower bound (%s) and upper bound (%s) exclude all values", + rb.lower.q.String(), rb.upper.q.String()) + } + return nil +} + +func (rb *requirementBounds) checkEqInsideBounds() error { + if rb.eqVal == nil { + return nil + } + if rb.lower != nil { + cmp := rb.eqVal.Cmp(rb.lower.q) + if cmp < 0 || (cmp == 0 && rb.lower.strict) { + return fmt.Errorf("eq value %s violates lower bound %s", rb.eqVal.String(), rb.lower.q.String()) + } + } + if rb.upper != nil { + cmp := rb.eqVal.Cmp(rb.upper.q) + if cmp > 0 || (cmp == 0 && rb.upper.strict) { + return fmt.Errorf("eq value %s violates upper bound %s", rb.eqVal.String(), rb.upper.q.String()) } } return nil diff --git a/pkg/utils/validator/placement_test.go b/pkg/utils/validator/placement_test.go index 81a828123..1c532ecea 100644 --- a/pkg/utils/validator/placement_test.go +++ b/pkg/utils/validator/placement_test.go @@ -21,6 +21,7 @@ import ( "testing" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/util/intstr" @@ -2049,3 +2050,202 @@ func TestValidateResourcePlacement(t *testing.T) { }) } } + +func TestValidateOperatorAndValues(t *testing.T) { + tests := []struct { + name string + op placementv1beta1.PropertySelectorOperator + values []string + wantErr bool + }{ + {name: "Eq with one valid value", op: placementv1beta1.PropertySelectorEqualTo, values: []string{"5"}, wantErr: false}, + {name: "Gt with one valid value", op: placementv1beta1.PropertySelectorGreaterThan, values: []string{"100Mi"}, wantErr: false}, + {name: "Lte with one valid value", op: placementv1beta1.PropertySelectorLessThanOrEqualTo, values: []string{"2.5"}, wantErr: false}, + {name: "unsupported operator", op: placementv1beta1.PropertySelectorOperator("In"), values: []string{"5"}, wantErr: true}, + {name: "Eq with zero values", op: placementv1beta1.PropertySelectorEqualTo, values: nil, wantErr: true}, + {name: "Eq with two values", op: placementv1beta1.PropertySelectorEqualTo, values: []string{"5", "10"}, wantErr: true}, + {name: "Lt with malformed quantity", op: placementv1beta1.PropertySelectorLessThan, values: []string{"five"}, wantErr: true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateOperatorAndValues(tt.op, tt.values) + if (err != nil) != tt.wantErr { + t.Errorf("validateOperatorAndValues(%v, %v) error = %v, wantErr %v", tt.op, tt.values, err, tt.wantErr) + } + }) + } +} + +func TestValidateRequirementsConsistency(t *testing.T) { + req := func(op placementv1beta1.PropertySelectorOperator, value string) placementv1beta1.PropertySelectorRequirement { + return placementv1beta1.PropertySelectorRequirement{Name: "p", Operator: op, Values: []string{value}} + } + + tests := []struct { + name string + reqs []placementv1beta1.PropertySelectorRequirement + wantErr bool + errSub string + }{ + // Trivially-satisfied inputs. + {name: "empty input", reqs: nil, wantErr: false}, + {name: "single requirement", reqs: []placementv1beta1.PropertySelectorRequirement{req(placementv1beta1.PropertySelectorEqualTo, "5")}, wantErr: false}, + {name: "two compatible Eq with same value", reqs: []placementv1beta1.PropertySelectorRequirement{req(placementv1beta1.PropertySelectorEqualTo, "5"), req(placementv1beta1.PropertySelectorEqualTo, "5")}, wantErr: false}, + {name: "Eq + Ne with different values", reqs: []placementv1beta1.PropertySelectorRequirement{req(placementv1beta1.PropertySelectorEqualTo, "5"), req(placementv1beta1.PropertySelectorNotEqualTo, "10")}, wantErr: false}, + {name: "two Ne", reqs: []placementv1beta1.PropertySelectorRequirement{req(placementv1beta1.PropertySelectorNotEqualTo, "5"), req(placementv1beta1.PropertySelectorNotEqualTo, "10")}, wantErr: false}, + {name: "Gt + Lt with non-empty interval", reqs: []placementv1beta1.PropertySelectorRequirement{req(placementv1beta1.PropertySelectorGreaterThan, "5"), req(placementv1beta1.PropertySelectorLessThan, "10")}, wantErr: false}, + {name: "Gte x + Lte x pinpoints x", reqs: []placementv1beta1.PropertySelectorRequirement{req(placementv1beta1.PropertySelectorGreaterThanOrEqualTo, "10"), req(placementv1beta1.PropertySelectorLessThanOrEqualTo, "10")}, wantErr: false}, + {name: "Eq inside bounds", reqs: []placementv1beta1.PropertySelectorRequirement{req(placementv1beta1.PropertySelectorGreaterThanOrEqualTo, "5"), req(placementv1beta1.PropertySelectorEqualTo, "7"), req(placementv1beta1.PropertySelectorLessThanOrEqualTo, "10")}, wantErr: false}, + {name: "redundant lower bounds keep most restrictive (compatible)", reqs: []placementv1beta1.PropertySelectorRequirement{req(placementv1beta1.PropertySelectorGreaterThan, "5"), req(placementv1beta1.PropertySelectorGreaterThan, "10"), req(placementv1beta1.PropertySelectorLessThan, "20")}, wantErr: false}, + {name: "value with units (memory quantity)", reqs: []placementv1beta1.PropertySelectorRequirement{req(placementv1beta1.PropertySelectorGreaterThanOrEqualTo, "100Mi"), req(placementv1beta1.PropertySelectorLessThan, "1Gi")}, wantErr: false}, + + // Conflicts. + {name: "two Eq with different values", reqs: []placementv1beta1.PropertySelectorRequirement{req(placementv1beta1.PropertySelectorEqualTo, "5"), req(placementv1beta1.PropertySelectorEqualTo, "10")}, wantErr: true, errSub: "conflicting Eq values"}, + {name: "Eq + Ne with same value", reqs: []placementv1beta1.PropertySelectorRequirement{req(placementv1beta1.PropertySelectorEqualTo, "5"), req(placementv1beta1.PropertySelectorNotEqualTo, "5")}, wantErr: true, errSub: "conflicting Eq and Ne"}, + {name: "Gt 10 + Lt 5", reqs: []placementv1beta1.PropertySelectorRequirement{req(placementv1beta1.PropertySelectorGreaterThan, "10"), req(placementv1beta1.PropertySelectorLessThan, "5")}, wantErr: true, errSub: "exclude all values"}, + {name: "Gt x + Lt x boundary", reqs: []placementv1beta1.PropertySelectorRequirement{req(placementv1beta1.PropertySelectorGreaterThan, "10"), req(placementv1beta1.PropertySelectorLessThan, "10")}, wantErr: true, errSub: "exclude all values"}, + {name: "Gt x + Lte x boundary", reqs: []placementv1beta1.PropertySelectorRequirement{req(placementv1beta1.PropertySelectorGreaterThan, "10"), req(placementv1beta1.PropertySelectorLessThanOrEqualTo, "10")}, wantErr: true, errSub: "exclude all values"}, + {name: "Gte x + Lt x boundary", reqs: []placementv1beta1.PropertySelectorRequirement{req(placementv1beta1.PropertySelectorGreaterThanOrEqualTo, "10"), req(placementv1beta1.PropertySelectorLessThan, "10")}, wantErr: true, errSub: "exclude all values"}, + {name: "least-restrictive lower hides contradiction (still detected via most-restrictive)", + reqs: []placementv1beta1.PropertySelectorRequirement{req(placementv1beta1.PropertySelectorGreaterThan, "10"), req(placementv1beta1.PropertySelectorGreaterThan, "5"), req(placementv1beta1.PropertySelectorLessThan, "7")}, + wantErr: true, errSub: "exclude all values"}, + {name: "Eq below lower bound", reqs: []placementv1beta1.PropertySelectorRequirement{req(placementv1beta1.PropertySelectorGreaterThanOrEqualTo, "10"), req(placementv1beta1.PropertySelectorEqualTo, "5")}, wantErr: true, errSub: "violates lower bound"}, + {name: "Eq above upper bound", reqs: []placementv1beta1.PropertySelectorRequirement{req(placementv1beta1.PropertySelectorLessThanOrEqualTo, "10"), req(placementv1beta1.PropertySelectorEqualTo, "20")}, wantErr: true, errSub: "violates upper bound"}, + {name: "Eq equals strict lower bound", reqs: []placementv1beta1.PropertySelectorRequirement{req(placementv1beta1.PropertySelectorGreaterThan, "10"), req(placementv1beta1.PropertySelectorEqualTo, "10")}, wantErr: true, errSub: "violates lower bound"}, + {name: "Eq equals strict upper bound", reqs: []placementv1beta1.PropertySelectorRequirement{req(placementv1beta1.PropertySelectorLessThan, "10"), req(placementv1beta1.PropertySelectorEqualTo, "10")}, wantErr: true, errSub: "violates upper bound"}, + + // Malformed inputs are skipped, not surfaced — that's validateOperatorAndValues' job. + {name: "malformed value is ignored for consistency check", reqs: []placementv1beta1.PropertySelectorRequirement{req(placementv1beta1.PropertySelectorEqualTo, "not-a-number"), req(placementv1beta1.PropertySelectorEqualTo, "5")}, wantErr: false}, + + // checkEqVsNe and checkEqInsideBounds must use Quantity.Cmp, not string equality, so + // canonically-equal cross-format inputs still produce the right conflict verdict. + {name: "Eq + Ne with same canonical value but different format", reqs: []placementv1beta1.PropertySelectorRequirement{req(placementv1beta1.PropertySelectorEqualTo, "1000m"), req(placementv1beta1.PropertySelectorNotEqualTo, "1")}, wantErr: true, errSub: "conflicting Eq and Ne"}, + {name: "Eq + Lt bound with same canonical value but different format", reqs: []placementv1beta1.PropertySelectorRequirement{req(placementv1beta1.PropertySelectorLessThan, "1024"), req(placementv1beta1.PropertySelectorEqualTo, "1Ki")}, wantErr: true, errSub: "violates upper bound"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateRequirementsConsistency(tt.reqs) + if (err != nil) != tt.wantErr { + t.Errorf("validateRequirementsConsistency(%v) error = %v, wantErr %v", tt.reqs, err, tt.wantErr) + } + if tt.wantErr && err != nil && !strings.Contains(err.Error(), tt.errSub) { + t.Errorf("validateRequirementsConsistency(%v) error = %q, want substring %q", tt.reqs, err.Error(), tt.errSub) + } + }) + } +} + +func TestRequirementBoundsTighten(t *testing.T) { + q := func(s string) resource.Quantity { return resource.MustParse(s) } + + t.Run("lower keeps largest value", func(t *testing.T) { + rb := &requirementBounds{} + rb.tightenLower(boundary{q: q("5")}) + rb.tightenLower(boundary{q: q("10")}) + rb.tightenLower(boundary{q: q("3")}) + if rb.lower.q.Cmp(q("10")) != 0 { + t.Errorf("lower = %s, want 10", rb.lower.q.String()) + } + }) + t.Run("lower prefers strict at tie", func(t *testing.T) { + rb := &requirementBounds{} + rb.tightenLower(boundary{q: q("10"), strict: false}) + rb.tightenLower(boundary{q: q("10"), strict: true}) + if !rb.lower.strict { + t.Errorf("lower.strict = false, want true") + } + }) + t.Run("upper keeps smallest value", func(t *testing.T) { + rb := &requirementBounds{} + rb.tightenUpper(boundary{q: q("10")}) + rb.tightenUpper(boundary{q: q("5")}) + rb.tightenUpper(boundary{q: q("8")}) + if rb.upper.q.Cmp(q("5")) != 0 { + t.Errorf("upper = %s, want 5", rb.upper.q.String()) + } + }) + t.Run("upper prefers strict at tie", func(t *testing.T) { + rb := &requirementBounds{} + rb.tightenUpper(boundary{q: q("10"), strict: false}) + rb.tightenUpper(boundary{q: q("10"), strict: true}) + if !rb.upper.strict { + t.Errorf("upper.strict = false, want true") + } + }) +} + +func TestApplyEq(t *testing.T) { + q := func(s string) resource.Quantity { return resource.MustParse(s) } + + t.Run("first call sets eqVal", func(t *testing.T) { + rb := &requirementBounds{} + if err := rb.applyEq(q("5")); err != nil { + t.Fatalf("applyEq(5) error = %v, want nil", err) + } + if rb.eqVal == nil || rb.eqVal.Cmp(q("5")) != 0 { + t.Errorf("eqVal = %v, want 5", rb.eqVal) + } + }) + t.Run("second call with equal value returns nil", func(t *testing.T) { + rb := &requirementBounds{} + if err := rb.applyEq(q("1")); err != nil { + t.Fatalf("setup applyEq(1) error = %v, want nil", err) + } + if err := rb.applyEq(q("1000m")); err != nil { + t.Errorf("applyEq(1000m) on rb with eqVal=1 error = %v, want nil", err) + } + if rb.eqVal == nil || rb.eqVal.Cmp(q("1")) != 0 { + t.Errorf("eqVal = %v, want value equal to 1", rb.eqVal) + } + }) + t.Run("second call with different value errors", func(t *testing.T) { + rb := &requirementBounds{} + if err := rb.applyEq(q("5")); err != nil { + t.Fatalf("setup applyEq(5) error = %v, want nil", err) + } + err := rb.applyEq(q("10")) + if err == nil || !strings.Contains(err.Error(), "conflicting Eq values") { + t.Errorf("applyEq(10) on rb with eqVal=5 error = %v, want 'conflicting Eq values'", err) + } + // eqVal must be preserved on the error path. + if rb.eqVal == nil || rb.eqVal.Cmp(q("5")) != 0 { + t.Errorf("eqVal after failed applyEq(10) = %v, want value equal to 5", rb.eqVal) + } + }) +} + +// TestCollectRequirementBoundsUnhandledOperator covers the nil-applyToBounds runtime guard. +// Mutates the package-level registry, which is safe only because no test in this package runs +// with t.Parallel(). +func TestCollectRequirementBoundsUnhandledOperator(t *testing.T) { + const fakeOp placementv1beta1.PropertySelectorOperator = "FakeOpForTest" + supportedPropertyOperators[fakeOp] = operatorSpec{requiredValueCount: 1} + t.Cleanup(func() { delete(supportedPropertyOperators, fakeOp) }) + + reqs := []placementv1beta1.PropertySelectorRequirement{ + {Name: "p", Operator: placementv1beta1.PropertySelectorEqualTo, Values: []string{"5"}}, + {Name: "p", Operator: fakeOp, Values: []string{"7"}}, + } + _, err := collectRequirementBounds(reqs) + if err == nil { + t.Fatalf("collectRequirementBounds(%v) error = nil, want non-nil for unhandled operator", reqs) + } + for _, wantSub := range []string{"internal:", "no bounds handler"} { + if !strings.Contains(err.Error(), wantSub) { + t.Errorf("collectRequirementBounds(%v) error = %q, want substring %q", reqs, err.Error(), wantSub) + } + } +} + +// TestSupportedPropertyOperatorsRegistryComplete fails at test time if any spec has a +// non-positive requiredValueCount or a nil applyToBounds. +func TestSupportedPropertyOperatorsRegistryComplete(t *testing.T) { + for op, spec := range supportedPropertyOperators { + if spec.requiredValueCount <= 0 { + t.Errorf("supportedPropertyOperators[%s].requiredValueCount = %d, want > 0", op, spec.requiredValueCount) + } + if spec.applyToBounds == nil { + t.Errorf("supportedPropertyOperators[%s].applyToBounds is nil, want non-nil", op) + } + } +} From 28401aeb9147e6894c61bf5d60789f312b66f7bd Mon Sep 17 00:00:00 2001 From: Wei Weng Date: Wed, 20 May 2026 09:27:33 -0400 Subject: [PATCH 3/4] feat: allow PickFixed placement with empty ClusterNames for scale-to-zero (#719) --- pkg/utils/validator/placement.go | 3 -- pkg/utils/validator/placement_test.go | 12 ++--- test/e2e/actuals_test.go | 37 ++++++++++++++ test/e2e/placement_pickfixed_test.go | 74 +++++++++++++++++++++++++++ test/e2e/webhook_test.go | 1 - 5 files changed, 116 insertions(+), 11 deletions(-) diff --git a/pkg/utils/validator/placement.go b/pkg/utils/validator/placement.go index 11364e1c8..00390e11c 100644 --- a/pkg/utils/validator/placement.go +++ b/pkg/utils/validator/placement.go @@ -250,9 +250,6 @@ func validatePlacementPolicy(policy *placementv1beta1.PlacementPolicy) error { func validatePolicyForPickFixedPlacementType(policy *placementv1beta1.PlacementPolicy) error { allErr := make([]error, 0) - if len(policy.ClusterNames) == 0 { - allErr = append(allErr, fmt.Errorf("cluster names cannot be empty for policy type %s", placementv1beta1.PickFixedPlacementType)) - } uniqueClusterNames := make(map[string]bool) for _, name := range policy.ClusterNames { nameErr := validation.IsDNS1123Subdomain(name) diff --git a/pkg/utils/validator/placement_test.go b/pkg/utils/validator/placement_test.go index 1c532ecea..1d0b9b0c1 100644 --- a/pkg/utils/validator/placement_test.go +++ b/pkg/utils/validator/placement_test.go @@ -536,12 +536,11 @@ func TestValidateClusterResourcePlacement_PickFixedPlacementPolicy(t *testing.T) }, wantErr: false, }, - "invalid placement policy - PickFixed with empty cluster names": { + "valid placement policy - PickFixed with empty cluster names": { policy: &placementv1beta1.PlacementPolicy{ PlacementType: placementv1beta1.PickFixedPlacementType, }, - wantErr: true, - wantErrMsg: "cluster names cannot be empty for policy type PickFixed", + wantErr: false, }, "invalid placement policy - PickFixed with non-unique cluster names": { policy: &placementv1beta1.PlacementPolicy{ @@ -1931,7 +1930,7 @@ func TestValidateResourcePlacement(t *testing.T) { wantErr bool wantErrMsg string }{ - "RP with invalid placement policy": { + "RP with valid PickFixed policy and empty cluster names": { rp: &placementv1beta1.ResourcePlacement{ ObjectMeta: metav1.ObjectMeta{ Name: "test-rp", @@ -1947,7 +1946,7 @@ func TestValidateResourcePlacement(t *testing.T) { }, Policy: &placementv1beta1.PlacementPolicy{ PlacementType: placementv1beta1.PickFixedPlacementType, - ClusterNames: []string{}, // Empty cluster names for PickFixed type + ClusterNames: []string{}, // Empty cluster names for PickFixed type (scale to zero) }, }, }, @@ -1955,8 +1954,7 @@ func TestValidateResourcePlacement(t *testing.T) { APIResources: map[schema.GroupVersionKind]bool{utils.DeploymentGVK: true}, IsClusterScopedResource: false, }, - wantErr: true, - wantErrMsg: "cluster names cannot be empty for policy type PickFixed", + wantErr: false, }, "RP with invalid rollout strategy": { rp: &placementv1beta1.ResourcePlacement{ diff --git a/test/e2e/actuals_test.go b/test/e2e/actuals_test.go index 7dcd45994..44fb6dc79 100644 --- a/test/e2e/actuals_test.go +++ b/test/e2e/actuals_test.go @@ -444,6 +444,17 @@ func rpWorkSynchronizedFailedConditions(generation int64, hasOverrides bool) []m } } +func crpSchedulePendingConditions(generation int64) []metav1.Condition { + return []metav1.Condition{ + { + Type: string(placementv1beta1.ClusterResourcePlacementScheduledConditionType), + Status: metav1.ConditionUnknown, + ObservedGeneration: generation, + Reason: condition.SchedulingUnknownReason, + }, + } +} + func crpScheduledConditions(generation int64) []metav1.Condition { return []metav1.Condition{ { @@ -1348,6 +1359,32 @@ func crpStatusUpdatedActual(wantSelectedResourceIdentifiers []placementv1beta1.R return customizedPlacementStatusUpdatedActual(crpKey, wantSelectedResourceIdentifiers, wantSelectedClusters, wantUnselectedClusters, wantObservedResourceIndex, true) } +// crpStatusWithCustomConditionsUpdatedActual checks CRP status with caller-supplied conditions, +// useful when the default condition derivation (based on selected/unselected clusters) does not apply. +func crpStatusWithCustomConditionsUpdatedActual( + wantSelectedResourceIdentifiers []placementv1beta1.ResourceIdentifier, + wantConditions []metav1.Condition, + wantObservedResourceIndex string, +) func() error { + crpKey := types.NamespacedName{Name: fmt.Sprintf(crpNameTemplate, GinkgoParallelProcess())} + return func() error { + placement, err := retrievePlacement(crpKey) + if err != nil { + return fmt.Errorf("failed to get placement %s: %w", crpKey, err) + } + wantStatus := &placementv1beta1.PlacementStatus{ + Conditions: wantConditions, + PerClusterPlacementStatuses: []placementv1beta1.PerClusterPlacementStatus{}, + SelectedResources: wantSelectedResourceIdentifiers, + ObservedResourceIndex: wantObservedResourceIndex, + } + if diff := cmp.Diff(placement.GetPlacementStatus(), wantStatus, placementStatusCmpOptionsOnCreate...); diff != "" { + return fmt.Errorf("Placement status diff (-got, +want): %s for placement %v", diff, crpKey) + } + return nil + } +} + func rpStatusUpdatedActual(wantSelectedResourceIdentifiers []placementv1beta1.ResourceIdentifier, wantSelectedClusters, wantUnselectedClusters []string, wantObservedResourceIndex string) func() error { rpKey := types.NamespacedName{Name: fmt.Sprintf(rpNameTemplate, GinkgoParallelProcess()), Namespace: appNamespace().Name} return customizedPlacementStatusUpdatedActual(rpKey, wantSelectedResourceIdentifiers, wantSelectedClusters, wantUnselectedClusters, wantObservedResourceIndex, true) diff --git a/test/e2e/placement_pickfixed_test.go b/test/e2e/placement_pickfixed_test.go index 5f6919abb..c0d589d58 100644 --- a/test/e2e/placement_pickfixed_test.go +++ b/test/e2e/placement_pickfixed_test.go @@ -335,4 +335,78 @@ var _ = Describe("placing resources using a CRP of PickFixed placement type", fu ensureCRPAndRelatedResourcesDeleted(crpName, []*framework.Cluster{memberCluster1EastProd, memberCluster2EastCanary}) }) }) + + Context("scale from zero to one cluster", Ordered, func() { + crpName := fmt.Sprintf(crpNameTemplate, GinkgoParallelProcess()) + + BeforeAll(func() { + // Create the resources. + createWorkResources() + + // Create the CRP with PickFixed and no clusters (scale to zero). + crp := &placementv1beta1.ClusterResourcePlacement{ + ObjectMeta: metav1.ObjectMeta{ + Name: crpName, + // Add a custom finalizer; this would allow us to better observe + // the behavior of the controllers. + Finalizers: []string{customDeletionBlockerFinalizer}, + }, + Spec: placementv1beta1.PlacementSpec{ + ResourceSelectors: workResourceSelector(), + Strategy: placementv1beta1.RolloutStrategy{ + Type: placementv1beta1.RollingUpdateRolloutStrategyType, + RollingUpdate: &placementv1beta1.RollingUpdateConfig{ + UnavailablePeriodSeconds: ptr.To(2), + }, + }, + Policy: &placementv1beta1.PlacementPolicy{ + PlacementType: placementv1beta1.PickFixedPlacementType, + ClusterNames: []string{}, + }, + }, + } + Expect(hubClient.Create(ctx, crp)).To(Succeed(), "Failed to create CRP") + }) + + It("should update CRP status as expected with no clusters selected", func() { + // The scheduler returns early when there are no target clusters, + // so the scheduling condition stays at SchedulePending (Unknown). + statusActual := crpStatusWithCustomConditionsUpdatedActual( + workResourceIdentifiers(), + crpSchedulePendingConditions(1), + "0", + ) + Eventually(statusActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update CRP status as expected") + }) + + It("should not place resources on any cluster", func() { + checkIfRemovedWorkResourcesFromMemberClusters(allMemberClusters) + }) + + It("should scale up to one cluster", func() { + // Update the CRP to add a cluster. + Eventually(func() error { + crp := &placementv1beta1.ClusterResourcePlacement{} + if err := hubClient.Get(ctx, types.NamespacedName{Name: crpName}, crp); err != nil { + return err + } + crp.Spec.Policy.ClusterNames = []string{memberCluster1EastProdName} + return hubClient.Update(ctx, crp) + }, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update CRP") + }) + + It("should update CRP status as expected after scale up", func() { + crpStatusUpdatedActual := crpStatusUpdatedActual(workResourceIdentifiers(), []string{memberCluster1EastProdName}, nil, "0") + Eventually(crpStatusUpdatedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update CRP status as expected") + }) + + It("should place resources on the newly specified cluster", func() { + resourcePlacedActual := workNamespaceAndConfigMapPlacedOnClusterActual(memberCluster1EastProd) + Eventually(resourcePlacedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to place resources on specified clusters") + }) + + AfterAll(func() { + ensureCRPAndRelatedResourcesDeleted(crpName, []*framework.Cluster{memberCluster1EastProd}) + }) + }) }) diff --git a/test/e2e/webhook_test.go b/test/e2e/webhook_test.go index b5d429441..2c2d4e5c2 100644 --- a/test/e2e/webhook_test.go +++ b/test/e2e/webhook_test.go @@ -78,7 +78,6 @@ var _ = Describe("webhook tests for CRP CREATE operations", func() { err := hubClient.Create(ctx, &crp) var statusErr *k8sErrors.StatusError g.Expect(errors.As(err, &statusErr)).To(BeTrue(), fmt.Sprintf("Create CRP call produced error %s. Error type wanted is %s.", reflect.TypeOf(err), reflect.TypeOf(&k8sErrors.StatusError{}))) - g.Expect(statusErr.ErrStatus.Message).Should(MatchRegexp("cluster names cannot be empty for policy type")) g.Expect(statusErr.ErrStatus.Message).Should(MatchRegexp("number of clusters must be nil for policy type PickFixed")) return nil }, eventuallyDuration, eventuallyInterval).Should(Succeed()) From 515a7443f7be83f7a5997c8c5422258179c0dc3c Mon Sep 17 00:00:00 2001 From: michaelawyu Date: Wed, 27 May 2026 15:52:46 +1000 Subject: [PATCH 4/4] feat: migrating to validating admission policies for our validating logic (1/) (#708) --- pkg/admissionpolicymanager/commons.go | 83 +++++ pkg/admissionpolicymanager/configs.go | 41 +++ pkg/admissionpolicymanager/manager.go | 324 ++++++++++++++++++ .../manager_integration_test.go | 272 +++++++++++++++ .../podsnreplicasets.go | 146 ++++++++ pkg/admissionpolicymanager/suite_test.go | 119 +++++++ .../svcaccountsntokenreqs.go | 203 +++++++++++ pkg/utils/common.go | 18 +- 8 files changed, 1197 insertions(+), 9 deletions(-) create mode 100644 pkg/admissionpolicymanager/commons.go create mode 100644 pkg/admissionpolicymanager/configs.go create mode 100644 pkg/admissionpolicymanager/manager.go create mode 100644 pkg/admissionpolicymanager/manager_integration_test.go create mode 100644 pkg/admissionpolicymanager/podsnreplicasets.go create mode 100644 pkg/admissionpolicymanager/suite_test.go create mode 100644 pkg/admissionpolicymanager/svcaccountsntokenreqs.go diff --git a/pkg/admissionpolicymanager/commons.go b/pkg/admissionpolicymanager/commons.go new file mode 100644 index 000000000..6a2bc7fe5 --- /dev/null +++ b/pkg/admissionpolicymanager/commons.go @@ -0,0 +1,83 @@ +/* +Copyright 2026 The KubeFleet Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package admissionpolicymanager + +import ( + "context" + "regexp" + "strings" + + "github.com/kubefleet-dev/kubefleet/pkg/utils/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +const ( + // illegalCELStringChars is a string of characters that should not be used in CEL string literals. + illegalCELStringChars = `'"\` +) + +var ( + // reservedNamespacePrefixRegexp matches valid namespace prefix characters (DNS label subset). + reservedNamespacePrefixRegexp = regexp.MustCompile(`^[a-z0-9-]+$`) +) + +var ( + managedByAndPartOfKubeFleetLabelSelector = client.MatchingLabels{ + VAPManagedByKubeFleetLabelKey: VAPManagedByKubeFleetLabelValue, + VAPPartOfKubeFleetLabelKey: VAPPartOfKubeFleetLabelValue, + VAPComponentKubeFleetLabelKey: VAPComponentAdmissionPolicyManagerLabelValue, + } +) + +var ( + // buildRetryUnlessCtxErr returns a function that is used with retry.OnError to stop + // retrying if the parent context has been cancelled or is erred. + buildRetryUnlessCtxErr = func(ctx context.Context) func(error) bool { + return func(err error) bool { + if err := ctx.Err(); err != nil { + return false + } + return true + } + } +) + +// addManagedByPartOfAndComponentLabels adds labels to the given object to indicate that it is managed by +// KubeFleet and belongs to the admission policy manager component. +func addManagedByPartOfAndComponentLabels(obj metav1.Object) { + labels := obj.GetLabels() + if labels == nil { + labels = make(map[string]string) + } + + labels[VAPManagedByKubeFleetLabelKey] = VAPManagedByKubeFleetLabelValue + labels[VAPPartOfKubeFleetLabelKey] = VAPPartOfKubeFleetLabelValue + labels[VAPComponentKubeFleetLabelKey] = VAPComponentAdmissionPolicyManagerLabelValue + obj.SetLabels(labels) +} + +// validateCELStringLiterals checks if any of the provided strings contains characters that are illegal +// in CEL string literals (e.g., backslash, double quotes, and single quotes). +func validateCELStringLiterals(strs ...string) error { + for _, str := range strs { + if strings.ContainsAny(str, illegalCELStringChars) { + return errors.NewUserError(nil, "string literal contains illegal characters for a CEL expression", "value", str) + } + } + return nil +} diff --git a/pkg/admissionpolicymanager/configs.go b/pkg/admissionpolicymanager/configs.go new file mode 100644 index 000000000..cd9106e22 --- /dev/null +++ b/pkg/admissionpolicymanager/configs.go @@ -0,0 +1,41 @@ +/* +Copyright 2026 The KubeFleet Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package admissionpolicymanager + +import ( + "github.com/kubefleet-dev/kubefleet/pkg/utils" +) + +// PolicyGeneratorConfigs holds the configurations for all available admission policy +// generators. +// +// This type is exposed so that users can provide a configuration object (in its serialized form) +// that specifies individual configurations for each generator. +type PolicyGeneratorConfigs struct { + PodsAndReplicaSetsVAPGeneratorConfig *PodsAndReplicaSetsValidatingAdmissionPolicyGenerator `json:"denyPodsAndReplicaSetsOutsideReservedNamespaces,omitempty"` + SvcAccountsAndTokenRequestsVAPGeneratorConfig *ServiceAccountsAndTokenRequestsValidatingAdmissionPolicyGenerator `json:"denyServiceAccountsAndTokenRequestsInReservedNamespaces,omitempty"` +} + +// DefaultPolicyGeneratorConfigs is the default configuration for all available admission policy generators. +var DefaultPolicyGeneratorConfigs = &PolicyGeneratorConfigs{ + PodsAndReplicaSetsVAPGeneratorConfig: &PodsAndReplicaSetsValidatingAdmissionPolicyGenerator{ + ReservedNamespacePrefixes: []string{utils.FleetNSNamePrefix, utils.KubeNSNamePrefix}, + }, + SvcAccountsAndTokenRequestsVAPGeneratorConfig: &ServiceAccountsAndTokenRequestsValidatingAdmissionPolicyGenerator{ + ReservedNamespacePrefixes: []string{utils.FleetNSNamePrefix, utils.KubeNSNamePrefix}, + }, +} diff --git a/pkg/admissionpolicymanager/manager.go b/pkg/admissionpolicymanager/manager.go new file mode 100644 index 000000000..8d4c7b936 --- /dev/null +++ b/pkg/admissionpolicymanager/manager.go @@ -0,0 +1,324 @@ +/* +Copyright 2026 The KubeFleet Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package admissionpolicymanager + +import ( + "context" + "reflect" + "time" + + admissionregistrationv1 "k8s.io/api/admissionregistration/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/apimachinery/pkg/util/wait" + "k8s.io/client-go/util/retry" + "k8s.io/klog/v2" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + + "github.com/kubefleet-dev/kubefleet/pkg/utils/errors" +) + +const ( + // The following labels are added to all policies created by the policy manager, + // so that the agent can track the lifecycle of created policies across different runs + // and act accordingly. + VAPManagedByKubeFleetLabelKey = "app.kubernetes.io/managed-by" + VAPManagedByKubeFleetLabelValue = "fleet-hub-agent" + VAPPartOfKubeFleetLabelKey = "app.kubernetes.io/part-of" + VAPPartOfKubeFleetLabelValue = "fleet" + VAPComponentKubeFleetLabelKey = "app.kubernetes.io/component" + VAPComponentAdmissionPolicyManagerLabelValue = "admission-policy-manager" +) + +var ( + // A list of all available policy generators. + allGenerators = sets.Set[string]{} +) + +func init() { + // Add all available generators to the set. + v := reflect.ValueOf(DefaultPolicyGeneratorConfigs).Elem() + for i := range v.NumField() { + field := v.Field(i) + if field.IsNil() { + continue + } + gen, ok := field.Interface().(ValidatingAdmissionPolicyGenerator) + if !ok { + continue + } + allGenerators.Insert(gen.Name()) + } +} + +// AllGenerators returns a copy of all available policy generators. +func AllGenerators() sets.Set[string] { + return allGenerators.Clone() +} + +var ( + policyRWOpBackoff = wait.Backoff{ + Steps: 3, + Duration: 1 * time.Second, + Factor: 2.0, + Jitter: 0.1, + } +) + +type PolicyWithBindings struct { + Policy *admissionregistrationv1.ValidatingAdmissionPolicy + Bindings []*admissionregistrationv1.ValidatingAdmissionPolicyBinding +} + +type ValidatingAdmissionPolicyGenerator interface { + Name() string + Validate() error + PoliciesWithBindings() []PolicyWithBindings +} + +type PolicyManager struct { + Client client.Client + + enabledPolicyGenerators map[string]ValidatingAdmissionPolicyGenerator +} + +func New(client client.Client, policyGeneratorConfigs *PolicyGeneratorConfigs, enabledPolicyNames []string) (*PolicyManager, error) { + if policyGeneratorConfigs == nil { + klog.V(2).Info("No admission policy generator configuration provided, falling back to the default configuration") + policyGeneratorConfigs = DefaultPolicyGeneratorConfigs + } + // Prepare a set of generators based on the list of enabled policies. + enabledPolicyGenerators, err := preparePolicyGenerators(policyGeneratorConfigs, enabledPolicyNames) + if err != nil { + return nil, errors.Wraps(err, "failed to create policy manager") + } + + return &PolicyManager{ + Client: client, + enabledPolicyGenerators: enabledPolicyGenerators, + }, nil +} + +func preparePolicyGenerators( + policyGeneratorConfigs *PolicyGeneratorConfigs, + enabledPolicyNames []string, +) (map[string]ValidatingAdmissionPolicyGenerator, error) { + enabledPolicyNameSet := sets.New(enabledPolicyNames...) + enabledPolicyGenerators := make(map[string]ValidatingAdmissionPolicyGenerator) + + v := reflect.ValueOf(policyGeneratorConfigs).Elem() + for i := range v.NumField() { + field := v.Field(i) + if field.IsNil() { + continue + } + gen, ok := field.Interface().(ValidatingAdmissionPolicyGenerator) + if !ok { + continue + } + if enabledPolicyNameSet.Has(gen.Name()) { + enabledPolicyGenerators[gen.Name()] = gen + } + } + + if len(enabledPolicyNameSet) != len(enabledPolicyGenerators) { + configuredPolicyNames := make([]string, 0, len(enabledPolicyGenerators)) + for name := range enabledPolicyGenerators { + configuredPolicyNames = append(configuredPolicyNames, name) + } + return nil, errors.NewUserError(nil, "some enabled policy generators are not configured properly", "enabledPolicies", enabledPolicyNames, "configuredPolicies", configuredPolicyNames) + } + return enabledPolicyGenerators, nil +} + +func (m *PolicyManager) Start(ctx context.Context) error { + // Generate all policies and policy bindings from the enabled generators, and apply them to the cluster. + createdOrUpdatedPolicyNames, createdOrUpdatedPolicyBindingNames, err := m.createOrUpdatePoliciesAndBindingsForEnabledGenerators(ctx) + if err != nil { + return errors.Wraps(err, "failed to create or update validating admission policies and bindings for enabled generators") + } + + // List all existing policies and policy bindings created by the manager, and delete those that are no longer needed. + if err := m.garbageCollectUnusedPoliciesAndBindings(ctx, createdOrUpdatedPolicyNames, createdOrUpdatedPolicyBindingNames); err != nil { + return errors.Wraps(err, "failed to garbage collect unused validating admission policies and bindings") + } + return nil +} + +// createOrUpdatePoliciesAndBindingsForEnabledGenerators creates or updates validating admission +// policies and their bindings for all enabled generators, and returns the names of +// created or updated policies and policy bindings. +func (m *PolicyManager) createOrUpdatePoliciesAndBindingsForEnabledGenerators(ctx context.Context) (sets.Set[string], sets.Set[string], error) { + createdOrUpdatedPolicyNames := sets.New[string]() + createdOrUpdatedPolicyBindingNames := sets.New[string]() + + for _, gen := range m.enabledPolicyGenerators { + // As a sanity check, do one more round of validation. + // + // Normally this check would never fail as the generators have been validated before + // the manager initializes. + if err := gen.Validate(); err != nil { + return nil, nil, errors.Wraps(err, "policy generator is invalid", "generator", gen.Name()) + } + + policiesWithBindings := gen.PoliciesWithBindings() + + for _, pb := range policiesWithBindings { + policy := pb.Policy + policyBindings := pb.Bindings + + // Create the policy. + addManagedByPartOfAndComponentLabels(policy) + policyToCreateOrUpdate := &admissionregistrationv1.ValidatingAdmissionPolicy{ + ObjectMeta: policy.ObjectMeta, + } + + err := retry.OnError(policyRWOpBackoff, buildRetryUnlessCtxErr(ctx), func() error { + opRes, err := controllerutil.CreateOrUpdate(ctx, m.Client, policyToCreateOrUpdate, func() error { + policyCopy := policy.DeepCopy() + policyToCreateOrUpdate.Spec = policyCopy.Spec + policyToCreateOrUpdate.Labels = policyCopy.Labels + return nil + }) + if err != nil { + return errors.NewAPIServerError(err, + "failed to create/update validating admission policy", + false, + "op", opRes, "policyName", policy.Name, "policyGenerator", gen.Name()) + } + return nil + }) + if err != nil { + // No need to wrap this for another time. The inner error already contains sufficient context about the failure. + return nil, nil, err + } + createdOrUpdatedPolicyNames.Insert(policy.Name) + klog.V(2).InfoS("Successfully created or updated validating admission policy", "policyName", policy.Name, "policyGenerator", gen.Name()) + + // Create the bindings. + for idx := range policyBindings { + policyBinding := policyBindings[idx] + + addManagedByPartOfAndComponentLabels(policyBinding) + policyBindingToCreateOrUpdate := &admissionregistrationv1.ValidatingAdmissionPolicyBinding{ + ObjectMeta: policyBinding.ObjectMeta, + } + + err := retry.OnError(policyRWOpBackoff, buildRetryUnlessCtxErr(ctx), func() error { + opRes, err := controllerutil.CreateOrUpdate(ctx, m.Client, policyBindingToCreateOrUpdate, func() error { + policyBindingCopy := policyBinding.DeepCopy() + policyBindingToCreateOrUpdate.Spec = policyBindingCopy.Spec + policyBindingToCreateOrUpdate.Labels = policyBindingCopy.Labels + return nil + }) + if err != nil { + return errors.NewAPIServerError(err, + "failed to create/update validating admission policy binding", + false, + "op", opRes, "policyBindingName", policyBinding.Name, "policyGenerator", gen.Name()) + } + return nil + }) + if err != nil { + // No need to wrap this for another time. The inner error already contains sufficient context about the failure. + return nil, nil, err + } + + createdOrUpdatedPolicyBindingNames.Insert(policyBinding.Name) + klog.V(2).InfoS("Successfully created or updated validating admission policy binding", "policyBindingName", policyBinding.Name, "policyGenerator", gen.Name()) + } + } + } + + return createdOrUpdatedPolicyNames, createdOrUpdatedPolicyBindingNames, nil +} + +func (m *PolicyManager) garbageCollectUnusedPoliciesAndBindings(ctx context.Context, createdOrUpdatedPolicyNames sets.Set[string], createdOrUpdatedPolicyBindingNames sets.Set[string]) error { + // List all existing policies and policy bindings created by the manager. + existingPolicyList := &admissionregistrationv1.ValidatingAdmissionPolicyList{} + existingPolicyBindingList := &admissionregistrationv1.ValidatingAdmissionPolicyBindingList{} + + err := retry.OnError(policyRWOpBackoff, buildRetryUnlessCtxErr(ctx), func() error { + if err := m.Client.List(ctx, existingPolicyList, managedByAndPartOfKubeFleetLabelSelector); err != nil { + return errors.NewAPIServerError(err, "failed to list all validating admission policies managed by KubeFleet", false) + } + return nil + }) + if err != nil { + // No need to wrap this for another time. The inner error already contains sufficient context about the failure. + return err + } + + err = retry.OnError(policyRWOpBackoff, buildRetryUnlessCtxErr(ctx), func() error { + if err := m.Client.List(ctx, existingPolicyBindingList, managedByAndPartOfKubeFleetLabelSelector); err != nil { + return errors.NewAPIServerError(err, "failed to list all validating admission policy bindings managed by KubeFleet", false) + } + return nil + }) + if err != nil { + // No need to wrap this for another time. The inner error already contains sufficient context about the failure. + return err + } + + // Delete policies that are created by the manager but no longer needed. + for i := range existingPolicyList.Items { + policy := &existingPolicyList.Items[i] + if !createdOrUpdatedPolicyNames.Has(policy.Name) { + err := retry.OnError(policyRWOpBackoff, buildRetryUnlessCtxErr(ctx), func() error { + if err := m.Client.Delete(ctx, policy); err != nil && !apierrors.IsNotFound(err) { + return errors.NewAPIServerError(err, + "failed to delete validating admission policy", + false, + "policyName", policy.Name) + } + return nil + }) + if err != nil { + // No need to wrap this for another time. The inner error already contains sufficient context about the failure. + return err + } + + klog.V(2).InfoS("Successfully deleted validating admission policy", "policyName", policy.Name) + } + } + + // Delete policy bindings that are created by the manager but no longer needed. + for i := range existingPolicyBindingList.Items { + policyBinding := &existingPolicyBindingList.Items[i] + if !createdOrUpdatedPolicyBindingNames.Has(policyBinding.Name) { + err := retry.OnError(policyRWOpBackoff, buildRetryUnlessCtxErr(ctx), func() error { + if err := m.Client.Delete(ctx, policyBinding); err != nil && !apierrors.IsNotFound(err) { + return errors.NewAPIServerError(err, + "failed to delete validating admission policy binding", + false, + "policyBindingName", policyBinding.Name) + } + return nil + }) + + if err != nil { + // No need to wrap this for another time. The inner error already contains sufficient context about the failure. + return err + } + + klog.V(2).InfoS("Successfully deleted validating admission policy binding", "policyBindingName", policyBinding.Name) + } + } + + return nil +} diff --git a/pkg/admissionpolicymanager/manager_integration_test.go b/pkg/admissionpolicymanager/manager_integration_test.go new file mode 100644 index 000000000..6c9d076ed --- /dev/null +++ b/pkg/admissionpolicymanager/manager_integration_test.go @@ -0,0 +1,272 @@ +/* +Copyright 2026 The KubeFleet Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package admissionpolicymanager + +import ( + "fmt" + + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + admissionregistrationv1 "k8s.io/api/admissionregistration/v1" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/ptr" + "sigs.k8s.io/controller-runtime/pkg/client" + + "github.com/kubefleet-dev/kubefleet/pkg/utils" +) + +const ( + nsNameTmpl = "work-%s" +) + +var ( + ignoreSystemManagedObjectMetaFields = cmpopts.IgnoreFields(metav1.ObjectMeta{}, "UID", "ResourceVersion", "Generation", "CreationTimestamp", "ManagedFields") + + lessFuncValidatingAdmissionPolicy = func(i, j admissionregistrationv1.ValidatingAdmissionPolicy) bool { + return i.Name < j.Name + } + lessFuncValidatingAdmissionPolicyBinding = func(i, j admissionregistrationv1.ValidatingAdmissionPolicyBinding) bool { + return i.Name < j.Name + } +) + +var _ = Describe("Policies, Policy Bindings and their Effects", Ordered, func() { + nsName := fmt.Sprintf(nsNameTmpl, utils.RandStr()) + + var ns *corev1.Namespace + BeforeAll(func() { + ns = &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: nsName, + }, + } + // Note: due to test environment restrictions (restrictions from the envtest package); + // namespaces created cannot be deleted. As a result, this test node will not perform + // any cleanup. + Expect(hubUncachedClient.Create(ctx, ns)).To(Succeed()) + }) + + It("should have all the expected policies", func() { + wantPolicies := []admissionregistrationv1.ValidatingAdmissionPolicy{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: podsAndReplicaSetsVAPPolicyName, + Labels: map[string]string{ + VAPManagedByKubeFleetLabelKey: VAPManagedByKubeFleetLabelValue, + VAPPartOfKubeFleetLabelKey: VAPPartOfKubeFleetLabelValue, + VAPComponentKubeFleetLabelKey: VAPComponentAdmissionPolicyManagerLabelValue, + }, + }, + Spec: admissionregistrationv1.ValidatingAdmissionPolicySpec{ + FailurePolicy: ptr.To(admissionregistrationv1.Fail), + MatchConstraints: &admissionregistrationv1.MatchResources{ + NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, + ResourceRules: []admissionregistrationv1.NamedRuleWithOperations{ + { + RuleWithOperations: admissionregistrationv1.RuleWithOperations{ + Operations: []admissionregistrationv1.OperationType{ + admissionregistrationv1.Create, + }, + Rule: admissionregistrationv1.Rule{ + APIGroups: []string{""}, + APIVersions: []string{"v1"}, + Resources: []string{"pods"}, + Scope: ptr.To(admissionregistrationv1.ScopeType("*")), // The system-enforced default. + }, + }, + }, + { + RuleWithOperations: admissionregistrationv1.RuleWithOperations{ + Operations: []admissionregistrationv1.OperationType{ + admissionregistrationv1.Create, + }, + Rule: admissionregistrationv1.Rule{ + APIGroups: []string{"apps"}, + APIVersions: []string{"v1"}, + Resources: []string{"replicasets"}, + Scope: ptr.To(admissionregistrationv1.ScopeType("*")), // The system-enforced default. + }, + }, + }, + }, + MatchPolicy: ptr.To(admissionregistrationv1.Equivalent), // The system-enforced default. + }, + Validations: []admissionregistrationv1.Validation{ + { + Expression: `request.namespace.startsWith("fleet-") || request.namespace.startsWith("kube-")`, + Message: "creating pods and replicas is disallowed in the fleet hub cluster", + Reason: ptr.To(metav1.StatusReasonForbidden), + }, + }, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: svcAccountsAndTokenRequestsVAPPolicyName, + Labels: map[string]string{ + VAPManagedByKubeFleetLabelKey: VAPManagedByKubeFleetLabelValue, + VAPPartOfKubeFleetLabelKey: VAPPartOfKubeFleetLabelValue, + VAPComponentKubeFleetLabelKey: VAPComponentAdmissionPolicyManagerLabelValue, + }, + }, + Spec: admissionregistrationv1.ValidatingAdmissionPolicySpec{ + FailurePolicy: ptr.To(admissionregistrationv1.Fail), + MatchConstraints: &admissionregistrationv1.MatchResources{ + NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, + ResourceRules: []admissionregistrationv1.NamedRuleWithOperations{ + { + RuleWithOperations: admissionregistrationv1.RuleWithOperations{ + Operations: []admissionregistrationv1.OperationType{ + admissionregistrationv1.Create, + admissionregistrationv1.Update, + admissionregistrationv1.Delete, + }, + Rule: admissionregistrationv1.Rule{ + APIGroups: []string{""}, + APIVersions: []string{"v1"}, + Resources: []string{"serviceaccounts"}, + Scope: ptr.To(admissionregistrationv1.ScopeType("*")), // The system-enforced default. + }, + }, + }, + { + RuleWithOperations: admissionregistrationv1.RuleWithOperations{ + Operations: []admissionregistrationv1.OperationType{ + admissionregistrationv1.Create, + }, + Rule: admissionregistrationv1.Rule{ + APIGroups: []string{""}, + APIVersions: []string{"v1"}, + Resources: []string{"serviceaccounts/token"}, + Scope: ptr.To(admissionregistrationv1.ScopeType("*")), // The system-enforced default. + }, + }, + }, + }, + MatchPolicy: ptr.To(admissionregistrationv1.Equivalent), // The system-enforced default. + }, + Validations: []admissionregistrationv1.Validation{ + { + Expression: `!(request.namespace.startsWith("fleet-") || request.namespace.startsWith("kube-")) || (request.userInfo.username == "system:kube-scheduler" || request.userInfo.username == "system:kube-controller-manager" || "system:nodes" in request.userInfo.groups || "system:masters" in request.userInfo.groups)`, + Message: "writing service accounts in reserved namespaces or requesting tokens from such service accounts is disallowed", + Reason: ptr.To(metav1.StatusReasonForbidden), + }, + }, + }, + }, + } + + Eventually(ctx, func() error { + policyList := &admissionregistrationv1.ValidatingAdmissionPolicyList{} + matchingLabels := map[string]string{ + VAPManagedByKubeFleetLabelKey: VAPManagedByKubeFleetLabelValue, + VAPPartOfKubeFleetLabelKey: VAPPartOfKubeFleetLabelValue, + VAPComponentKubeFleetLabelKey: VAPComponentAdmissionPolicyManagerLabelValue, + } + if err := hubUncachedClient.List(ctx, policyList, client.MatchingLabels(matchingLabels)); err != nil { + return fmt.Errorf("failed to list matching policies: %w", err) + } + + policies := policyList.Items + if len(policies) != len(wantPolicies) { + return fmt.Errorf("number of policies mismatch: got %d, want %d", len(policies), len(wantPolicies)) + } + + if diff := cmp.Diff( + policies, wantPolicies, + cmpopts.EquateEmpty(), + cmpopts.SortSlices(lessFuncValidatingAdmissionPolicy), + ignoreSystemManagedObjectMetaFields, + ); diff != "" { + return fmt.Errorf("policies mismatch (-got +want):\n%s", diff) + } + return nil + }, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Received unexpected list of policies or an error has occurred") + }) + + It("should have all the expected bindings", func() { + wantPolicyBindings := []admissionregistrationv1.ValidatingAdmissionPolicyBinding{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: podsAndReplicaSetsVAPPolicyBindingName, + Labels: map[string]string{ + VAPManagedByKubeFleetLabelKey: VAPManagedByKubeFleetLabelValue, + VAPPartOfKubeFleetLabelKey: VAPPartOfKubeFleetLabelValue, + VAPComponentKubeFleetLabelKey: VAPComponentAdmissionPolicyManagerLabelValue, + }, + }, + Spec: admissionregistrationv1.ValidatingAdmissionPolicyBindingSpec{ + PolicyName: podsAndReplicaSetsVAPPolicyName, + ValidationActions: []admissionregistrationv1.ValidationAction{ + admissionregistrationv1.Deny, + }, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: svcAccountsAndTokenRequestsVAPPolicyBindingName, + Labels: map[string]string{ + VAPManagedByKubeFleetLabelKey: VAPManagedByKubeFleetLabelValue, + VAPPartOfKubeFleetLabelKey: VAPPartOfKubeFleetLabelValue, + VAPComponentKubeFleetLabelKey: VAPComponentAdmissionPolicyManagerLabelValue, + }, + }, + Spec: admissionregistrationv1.ValidatingAdmissionPolicyBindingSpec{ + PolicyName: svcAccountsAndTokenRequestsVAPPolicyName, + ValidationActions: []admissionregistrationv1.ValidationAction{ + admissionregistrationv1.Deny, + }, + }, + }, + } + + Eventually(ctx, func() error { + bindingList := &admissionregistrationv1.ValidatingAdmissionPolicyBindingList{} + matchingLabels := map[string]string{ + VAPManagedByKubeFleetLabelKey: VAPManagedByKubeFleetLabelValue, + VAPPartOfKubeFleetLabelKey: VAPPartOfKubeFleetLabelValue, + VAPComponentKubeFleetLabelKey: VAPComponentAdmissionPolicyManagerLabelValue, + } + if err := hubUncachedClient.List(ctx, bindingList, client.MatchingLabels(matchingLabels)); err != nil { + return fmt.Errorf("failed to list matching policy bindings: %w", err) + } + + bindings := bindingList.Items + if len(bindings) != len(wantPolicyBindings) { + return fmt.Errorf("number of policy bindings mismatch: got %d, want %d", len(bindings), len(wantPolicyBindings)) + } + + if diff := cmp.Diff( + bindings, wantPolicyBindings, + cmpopts.EquateEmpty(), + cmpopts.SortSlices(lessFuncValidatingAdmissionPolicyBinding), + ignoreSystemManagedObjectMetaFields, + ); diff != "" { + return fmt.Errorf("policy bindings mismatch (-got +want):\n%s", diff) + } + return nil + }, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Received unexpected list of policy bindings or an error has occurred") + }) + + // Note: in the integration test environment, it appears that validating admission policies have no effect or are + // not being enforced as expected. As a result the effects are validated using E2E tests instead. +}) diff --git a/pkg/admissionpolicymanager/podsnreplicasets.go b/pkg/admissionpolicymanager/podsnreplicasets.go new file mode 100644 index 000000000..f53dd4c12 --- /dev/null +++ b/pkg/admissionpolicymanager/podsnreplicasets.go @@ -0,0 +1,146 @@ +/* +Copyright 2026 The KubeFleet Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +// TO-DO (chenyu1): refactor the building logic from the current state (static generation) +// to CEL expression trees after the initial set of VAPs are validated to be working as expected. + +package admissionpolicymanager + +import ( + "fmt" + "strings" + + admissionregistrationv1 "k8s.io/api/admissionregistration/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/ptr" + + "github.com/kubefleet-dev/kubefleet/pkg/utils/errors" +) + +const ( + PodsAndReplicaSetsVAPGeneratorName = "DenyPodsAndReplicaSetsOutsideReservedNamespaces" +) + +const ( + podsAndReplicaSetsVAPPolicyName = "deny-pods-and-replicasets-outside-reserved-namespaces" + podsAndReplicaSetsVAPPolicyBindingName = "deny-pods-and-replicasets-outside-reserved-namespaces-binding" +) + +// Verify that PodsAndReplicaSetsValidatingAdmissionPolicyGenerator implements +// the ValidatingAdmissionPolicyGenerator interface. +var _ ValidatingAdmissionPolicyGenerator = &PodsAndReplicaSetsValidatingAdmissionPolicyGenerator{} + +// PodsAndReplicaSetsValidatingAdmissionPolicyGenerator generates a ValidatingAdmissionPolicy +// and its binding that denies creation of pods and replicasets in non-reserved namespaces. +type PodsAndReplicaSetsValidatingAdmissionPolicyGenerator struct { + ReservedNamespacePrefixes []string +} + +// Name returns the name of the generator, which is used to determine if a specific generator +// has been enabled or not. +func (g *PodsAndReplicaSetsValidatingAdmissionPolicyGenerator) Name() string { + return PodsAndReplicaSetsVAPGeneratorName +} + +// Validate validates the configuration of the generator. +func (g *PodsAndReplicaSetsValidatingAdmissionPolicyGenerator) Validate() error { + if len(g.ReservedNamespacePrefixes) == 0 { + return errors.NewUserError(nil, "at least one prefix must be specified") + } + // Check if any of the prefixes includes illegal characters. + for _, prefix := range g.ReservedNamespacePrefixes { + if !reservedNamespacePrefixRegexp.MatchString(prefix) { + return errors.NewUserError(nil, "prefix contains illegal characters; only lowercase alphanumeric characters and hyphens are allowed", "prefix", prefix) + } + } + return nil +} + +// PoliciesWithBindings generates a ValidatingAdmissionPolicy and its policy binding that denies creation of pods and +// replicasets in non-reserved namespaces. +// +// For simplicity reasons, the code here assumes that the generator has been validated before PoliciesWithBindings() is called. +func (g *PodsAndReplicaSetsValidatingAdmissionPolicyGenerator) PoliciesWithBindings() []PolicyWithBindings { + celExprSegs := []string{} + for _, prefix := range g.ReservedNamespacePrefixes { + celExprSegs = append(celExprSegs, fmt.Sprintf(`request.namespace.startsWith("%s")`, prefix)) + } + celExpr := strings.Join(celExprSegs, " || ") + + policy := &admissionregistrationv1.ValidatingAdmissionPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: podsAndReplicaSetsVAPPolicyName, + }, + Spec: admissionregistrationv1.ValidatingAdmissionPolicySpec{ + FailurePolicy: ptr.To(admissionregistrationv1.Fail), + MatchConstraints: &admissionregistrationv1.MatchResources{ + ResourceRules: []admissionregistrationv1.NamedRuleWithOperations{ + { + RuleWithOperations: admissionregistrationv1.RuleWithOperations{ + Operations: []admissionregistrationv1.OperationType{ + admissionregistrationv1.Create, + }, + Rule: admissionregistrationv1.Rule{ + APIGroups: []string{""}, + APIVersions: []string{"v1"}, + Resources: []string{"pods"}, + }, + }, + }, + { + RuleWithOperations: admissionregistrationv1.RuleWithOperations{ + Operations: []admissionregistrationv1.OperationType{ + admissionregistrationv1.Create, + }, + Rule: admissionregistrationv1.Rule{ + APIGroups: []string{"apps"}, + APIVersions: []string{"v1"}, + Resources: []string{"replicasets"}, + }, + }, + }, + }, + }, + Validations: []admissionregistrationv1.Validation{ + { + Expression: celExpr, + // The error message has been set to match with the checks in some of the E2E tests + // in our existing release pipeline. + Message: "creating pods and replicas is disallowed in the fleet hub cluster", + Reason: ptr.To(metav1.StatusReasonForbidden), + }, + }, + }, + } + + binding := &admissionregistrationv1.ValidatingAdmissionPolicyBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: podsAndReplicaSetsVAPPolicyBindingName, + }, + Spec: admissionregistrationv1.ValidatingAdmissionPolicyBindingSpec{ + PolicyName: podsAndReplicaSetsVAPPolicyName, + ValidationActions: []admissionregistrationv1.ValidationAction{ + admissionregistrationv1.Deny, + }, + }, + } + return []PolicyWithBindings{ + { + Policy: policy, + Bindings: []*admissionregistrationv1.ValidatingAdmissionPolicyBinding{binding}, + }, + } +} diff --git a/pkg/admissionpolicymanager/suite_test.go b/pkg/admissionpolicymanager/suite_test.go new file mode 100644 index 000000000..45df92091 --- /dev/null +++ b/pkg/admissionpolicymanager/suite_test.go @@ -0,0 +1,119 @@ +/* +Copyright 2026 The KubeFleet Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package admissionpolicymanager + +import ( + "context" + "flag" + "testing" + "time" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + "k8s.io/client-go/kubernetes/scheme" + "k8s.io/client-go/rest" + "k8s.io/klog/v2" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/envtest" + "sigs.k8s.io/controller-runtime/pkg/log/zap" + "sigs.k8s.io/controller-runtime/pkg/metrics/server" +) + +// These tests use Ginkgo (BDD-style Go testing framework). Refer to +// http://onsi.github.io/ginkgo/ to learn more about Ginkgo. +var ( + cfg *rest.Config + testEnv *envtest.Environment + hubUncachedClient client.Client + hubMgr ctrl.Manager + + ctx context.Context + cancel context.CancelFunc +) + +var ( + eventuallyDuration = time.Second * 10 + eventuallyInterval = time.Millisecond * 500 +) + +func TestAPIs(t *testing.T) { + RegisterFailHandler(Fail) + + RunSpecs(t, "Admission Policy Manager Integration Test Suite") +} + +var _ = BeforeSuite(func() { + ctx, cancel = context.WithCancel(context.TODO()) + + By("Setup klog") + fs := flag.NewFlagSet("klog", flag.ContinueOnError) + klog.InitFlags(fs) + Expect(fs.Parse([]string{"--v", "5", "-add_dir_header", "true"})).Should(Succeed()) + + logger := zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true)) + klog.SetLogger(logger) + ctrl.SetLogger(logger) + + By("Bootstrapping test environment") + testEnv = &envtest.Environment{} + + var err error + cfg, err = testEnv.Start() + Expect(err).ToNot(HaveOccurred()) + Expect(cfg).ToNot(BeNil()) + + By("Setting up the controller manager") + hubMgr, err = ctrl.NewManager(cfg, ctrl.Options{ + Scheme: scheme.Scheme, + Metrics: server.Options{ + BindAddress: "0", // disable the metrics server. + }, + Logger: logger, + }) + Expect(err).ToNot(HaveOccurred()) + Expect(hubMgr).ToNot(BeNil()) + + By("Building the K8s client") + hubUncachedClient, err = client.New(cfg, client.Options{Scheme: scheme.Scheme}) + Expect(err).ToNot(HaveOccurred()) + Expect(hubUncachedClient).ToNot(BeNil()) + + By("Setting up the policy manager") + enabledGeneratorNames := []string{} + for name := range allGenerators { + enabledGeneratorNames = append(enabledGeneratorNames, name) + } + policyManager, err := New(hubUncachedClient, DefaultPolicyGeneratorConfigs, enabledGeneratorNames) + Expect(err).ToNot(HaveOccurred()) + Expect(policyManager).ToNot(BeNil()) + Expect(policyManager.Start(ctx)).To(Succeed()) + + By("Starting the controller manager") + go func() { + defer GinkgoRecover() + Expect(hubMgr.Start(ctx)).To(Succeed()) + }() +}) + +var _ = AfterSuite(func() { + defer klog.Flush() + + cancel() + By("Tearing down the test environment") + Expect(testEnv.Stop()).To(Succeed()) +}) diff --git a/pkg/admissionpolicymanager/svcaccountsntokenreqs.go b/pkg/admissionpolicymanager/svcaccountsntokenreqs.go new file mode 100644 index 000000000..16aa46d4b --- /dev/null +++ b/pkg/admissionpolicymanager/svcaccountsntokenreqs.go @@ -0,0 +1,203 @@ +/* +Copyright 2026 The KubeFleet Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +// TO-DO (chenyu1): refactor the building logic from the current state (static generation) +// to CEL expression trees after the initial set of VAPs are validated to be working as expected. + +package admissionpolicymanager + +import ( + "fmt" + "strings" + + admissionregistrationv1 "k8s.io/api/admissionregistration/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/ptr" + + "github.com/kubefleet-dev/kubefleet/pkg/utils/errors" +) + +const ( + // Exempt the value from the linter as it miscategorizes it as a credential. + SvcAccountsAndTokenRequestsVAPGeneratorName = "DenyServiceAccountsAndTokenRequestsInReservedNamespaces" //nolint:gosec +) + +const ( + svcAccountsAndTokenRequestsVAPPolicyName = "deny-serviceaccounts-and-tokenrequests-in-reserved-namespaces" + svcAccountsAndTokenRequestsVAPPolicyBindingName = "deny-serviceaccounts-and-tokenrequests-in-reserved-namespaces-binding" +) + +const ( + kubeSchedulerUserName = "system:kube-scheduler" + kubeControllerManagerUserName = "system:kube-controller-manager" + + kubeNodeUserGroup = "system:nodes" + adminUserGroup = "system:masters" +) + +// Verify that ServiceAccountsAndTokenRequestsValidatingAdmissionPolicyGenerator implements +// the ValidatingAdmissionPolicyGenerator interface. +var _ ValidatingAdmissionPolicyGenerator = &ServiceAccountsAndTokenRequestsValidatingAdmissionPolicyGenerator{} + +// ServiceAccountsAndTokenRequestsValidatingAdmissionPolicyGenerator generates a +// ValidatingAdmissionPolicy and its binding that denies creation/update/deletion of service accounts +// and creation of token requests in reserved namespaces, except for requests from certain +// whitelisted users and user groups. +// +// TO-DO (chenyu1): evaluate if it is appropriate to ban service accounts ops under +// all namespaces. +type ServiceAccountsAndTokenRequestsValidatingAdmissionPolicyGenerator struct { + WhitelistedUsernames []string + WhitelistedUserGroups []string + ReservedNamespacePrefixes []string +} + +// Name returns the name of the generator, which is used to determine if a specific generator +// has been enabled or not. +func (g *ServiceAccountsAndTokenRequestsValidatingAdmissionPolicyGenerator) Name() string { + return SvcAccountsAndTokenRequestsVAPGeneratorName +} + +// Validate validates the configuration of the generator. +func (g *ServiceAccountsAndTokenRequestsValidatingAdmissionPolicyGenerator) Validate() error { + if len(g.ReservedNamespacePrefixes) == 0 { + return errors.NewUserError(nil, "at least one prefix must be specified") + } + // Check if any of the prefixes includes illegal characters. + for _, prefix := range g.ReservedNamespacePrefixes { + if !reservedNamespacePrefixRegexp.MatchString(prefix) { + return errors.NewUserError(nil, "prefix contains illegal characters; only lowercase alphanumeric characters and hyphens are allowed", "prefix", prefix) + } + } + // Check if any whitelisted username or user group contains characters that are + // illegal in a CEL string literal. + if err := validateCELStringLiterals(g.WhitelistedUsernames...); err != nil { + return errors.Wraps(err, "invalid string literals", "field", "whitelistedUsernames") + } + if err := validateCELStringLiterals(g.WhitelistedUserGroups...); err != nil { + return errors.Wraps(err, "invalid string literals", "field", "whitelistedUserGroups") + } + return nil +} + +// PoliciesWithBindings generates a ValidatingAdmissionPolicy and its policy binding that denies +// creation/update/deletion of service accounts and creation of token requests in reserved namespaces, +// except for requests from certain whitelisted users and user groups. +// +// For simplicity reasons, the code here assumes that the generator has been validated before PoliciesWithBindings() is called. +func (g *ServiceAccountsAndTokenRequestsValidatingAdmissionPolicyGenerator) PoliciesWithBindings() []PolicyWithBindings { + celExprAccSegs := []string{} + + // Exempt whitelisted users from this admission policy. + for _, username := range g.WhitelistedUsernames { + celExprAccSegs = append(celExprAccSegs, fmt.Sprintf(`request.userInfo.username == "%s"`, username)) + } + // Exempt whitelisted user groups from this admission policy. + for _, userGroup := range g.WhitelistedUserGroups { + celExprAccSegs = append(celExprAccSegs, fmt.Sprintf(`"%s" in request.userInfo.groups`, userGroup)) + } + // Exempt requests from the Kubernetes scheduler, any of the nodes, and (esp.) the + // Kubernetes controller manager from this admission policy. + // + // Important: the Kubernetes controller manager, when deployed with the option + // --use-service-account-credentials=true, creates a service account token for many of its controllers + // and uses those tokens to authenticate to the Kubernetes API server. It retrieves a token + // via the TokenRequest API; failure to exempt this scenario may lead to critical errors. + celExprAccSegs = append(celExprAccSegs, fmt.Sprintf(`request.userInfo.username == "%s"`, kubeSchedulerUserName)) + celExprAccSegs = append(celExprAccSegs, fmt.Sprintf(`request.userInfo.username == "%s"`, kubeControllerManagerUserName)) + celExprAccSegs = append(celExprAccSegs, fmt.Sprintf(`"%s" in request.userInfo.groups`, kubeNodeUserGroup)) + // Exempt requests from admin users from this admission policy. + celExprAccSegs = append(celExprAccSegs, fmt.Sprintf(`"%s" in request.userInfo.groups`, adminUserGroup)) + + celExprAcc := strings.Join(celExprAccSegs, " || ") + + celExprNSSegs := []string{} + for _, prefix := range g.ReservedNamespacePrefixes { + celExprNSSegs = append(celExprNSSegs, fmt.Sprintf(`request.namespace.startsWith("%s")`, prefix)) + } + celExprNS := strings.Join(celExprNSSegs, " || ") + + celExpr := fmt.Sprintf("!(%s) || (%s)", celExprNS, celExprAcc) + + policy := &admissionregistrationv1.ValidatingAdmissionPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: svcAccountsAndTokenRequestsVAPPolicyName, + }, + Spec: admissionregistrationv1.ValidatingAdmissionPolicySpec{ + FailurePolicy: ptr.To(admissionregistrationv1.Fail), + MatchConstraints: &admissionregistrationv1.MatchResources{ + ResourceRules: []admissionregistrationv1.NamedRuleWithOperations{ + { + RuleWithOperations: admissionregistrationv1.RuleWithOperations{ + Operations: []admissionregistrationv1.OperationType{ + admissionregistrationv1.Create, + admissionregistrationv1.Update, + admissionregistrationv1.Delete, + }, + Rule: admissionregistrationv1.Rule{ + APIGroups: []string{""}, + APIVersions: []string{"v1"}, + Resources: []string{"serviceaccounts"}, + }, + }, + }, + { + RuleWithOperations: admissionregistrationv1.RuleWithOperations{ + Operations: []admissionregistrationv1.OperationType{ + admissionregistrationv1.Create, + }, + Rule: admissionregistrationv1.Rule{ + APIGroups: []string{""}, + APIVersions: []string{"v1"}, + // TokenRequest API is implemented as a subresource (token) of service + // accounts. It only supports the Create operation. + Resources: []string{"serviceaccounts/token"}, + }, + }, + }, + }, + }, + Validations: []admissionregistrationv1.Validation{ + { + Expression: celExpr, + // The error message has been set to match with the checks in some of the E2E tests + // in our existing release pipeline. + Message: "writing service accounts in reserved namespaces or requesting tokens from such service accounts is disallowed", + Reason: ptr.To(metav1.StatusReasonForbidden), + }, + }, + }, + } + + binding := &admissionregistrationv1.ValidatingAdmissionPolicyBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: svcAccountsAndTokenRequestsVAPPolicyBindingName, + }, + Spec: admissionregistrationv1.ValidatingAdmissionPolicyBindingSpec{ + PolicyName: svcAccountsAndTokenRequestsVAPPolicyName, + ValidationActions: []admissionregistrationv1.ValidationAction{ + admissionregistrationv1.Deny, + }, + }, + } + + return []PolicyWithBindings{ + { + Policy: policy, + Bindings: []*admissionregistrationv1.ValidatingAdmissionPolicyBinding{binding}, + }, + } +} diff --git a/pkg/utils/common.go b/pkg/utils/common.go index 50fdc74fb..c772f8c9f 100644 --- a/pkg/utils/common.go +++ b/pkg/utils/common.go @@ -49,13 +49,13 @@ import ( ) const ( - kubePrefix = "kube-" - fleetPrefix = "fleet-" - fleetMemberNamespacePrefix = fleetPrefix + "member-" - FleetSystemNamespace = fleetPrefix + "system" - NamespaceNameFormat = fleetMemberNamespacePrefix + "%s" - RoleNameFormat = fleetPrefix + "role-%s" - RoleBindingNameFormat = fleetPrefix + "rolebinding-%s" + KubeNSNamePrefix = "kube-" + FleetNSNamePrefix = "fleet-" + FleetMemberNamespacePrefix = FleetNSNamePrefix + "member-" + FleetSystemNamespace = FleetNSNamePrefix + "system" + NamespaceNameFormat = FleetMemberNamespacePrefix + "%s" + RoleNameFormat = FleetNSNamePrefix + "role-%s" + RoleBindingNameFormat = FleetNSNamePrefix + "rolebinding-%s" ValidationPathFmt = "/validate-%s-%s-%s" MutatingPathFmt = "/mutate-%s-%s-%s" lessGroupsStringFormat = "groups: %v" @@ -501,12 +501,12 @@ func CheckCRDInstalled(discoveryClient discovery.DiscoveryInterface, gvk schema. // IsReservedNamespace indicates if an argued namespace is reserved. func IsReservedNamespace(namespace string) bool { - return strings.HasPrefix(namespace, fleetPrefix) || strings.HasPrefix(namespace, kubePrefix) + return strings.HasPrefix(namespace, FleetNSNamePrefix) || strings.HasPrefix(namespace, KubeNSNamePrefix) } // IsFleetMemberNamespace indicates if an argued namespace is a fleet member namespace. func IsFleetMemberNamespace(namespace string) bool { - return strings.HasPrefix(namespace, fleetMemberNamespacePrefix) + return strings.HasPrefix(namespace, FleetMemberNamespacePrefix) } // ShouldPropagateNamespace decides if we should propagate the resources in the namespace.