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 diff --git a/pkg/admissionpolicymanager/commons.go b/pkg/admissionpolicymanager/commons.go new file mode 100644 index 000000000..da120f940 --- /dev/null +++ b/pkg/admissionpolicymanager/commons.go @@ -0,0 +1,84 @@ +/* +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" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + + "go.goms.io/fleet/pkg/utils/errors" +) + +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..f753dc335 --- /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 ( + "go.goms.io/fleet/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..1699654fb --- /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" + + "go.goms.io/fleet/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..d2c0e28b8 --- /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" + + "go.goms.io/fleet/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..346fd9ad2 --- /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" + + "go.goms.io/fleet/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..97874f2f9 --- /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" + + "go.goms.io/fleet/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 8a3764e24..f16aa7125 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. diff --git a/pkg/utils/controller/resource_selector_resolver.go b/pkg/utils/controller/resource_selector_resolver.go index a405917c1..6fa1b083f 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 65eee8788..f2efa9f3b 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 { @@ -201,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) @@ -450,17 +496,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 +581,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 c7852bf2c..2ac9176c6 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" @@ -535,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{ @@ -1930,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", @@ -1946,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) }, }, }, @@ -1954,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{ @@ -2049,3 +2048,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) + } + } +} diff --git a/test/e2e/actuals_test.go b/test/e2e/actuals_test.go index 6a212c07a..a526f91df 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 a6f8acb97..15baef4d8 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 7aca5bfb2..326d08c4a 100644 --- a/test/e2e/webhook_test.go +++ b/test/e2e/webhook_test.go @@ -77,7 +77,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())