Skip to content

POC: using MutatingAdmissionPolicy to schedule CS pods #1777

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion e2e/nomostest/clusters/kind.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"sync"
"time"

"k8s.io/apiserver/pkg/features"
"kpt.dev/configsync/e2e"
"kpt.dev/configsync/e2e/nomostest/docker"
"kpt.dev/configsync/e2e/nomostest/taskgroup"
Expand Down Expand Up @@ -155,6 +156,12 @@ func createKindCluster(p *cluster.Provider, name, kcfgPath string) error {
fmt.Sprintf(`[plugins."io.containerd.grpc.v1.cri".registry.mirrors."localhost:%d"]
endpoint = ["http://%s:%d"]`, docker.RegistryPort, docker.RegistryName, docker.RegistryPort),
},
FeatureGates: map[string]bool{
string(features.MutatingAdmissionPolicy): true,
},
RuntimeConfig: map[string]string{
"admissionregistration.k8s.io/v1alpha1": "true",
},
// Enable ValidatingAdmissionWebhooks in the Kind cluster, as these
// are disabled by default.
// Also mount etcd to tmpfs for memory-backed storage.
Expand All @@ -166,7 +173,7 @@ etcd:
dataDir: /tmp/etcd
apiServer:
extraArgs:
"enable-admission-plugins": "ValidatingAdmissionWebhook"`,
"enable-admission-plugins": "ValidatingAdmissionWebhook,MutatingAdmissionPolicy,ValidatingAdmissionPolicy"`,
},
}),
// Retain nodes for debugging logs.
Expand Down
17 changes: 17 additions & 0 deletions e2e/nomostest/config_sync.go
Original file line number Diff line number Diff line change
Expand Up @@ -1398,6 +1398,23 @@ func podHasReadyCondition(conditions []corev1.PodCondition) bool {
return false
}

// ValidatePodByLabel validates that all Pods matching the provided label pass the
// provided list of predicates.
func ValidatePodByLabel(nt *NT, label, value string, predicates ...testpredicates.Predicate) error {
newPods := &corev1.PodList{}
if err := nt.KubeClient.List(newPods, client.InNamespace(configmanagement.ControllerNamespace), client.MatchingLabels{label: value}); err != nil {
return err
}
tg := taskgroup.New()
for _, pod := range newPods.Items {
po := pod
tg.Go(func() error {
return nt.Validate(po.Name, po.Namespace, &corev1.Pod{}, predicates...)
})
}
return tg.Wait()
}

// NewPodReady checks if the new created pods are ready.
// It also checks if the new children pods that are managed by the pods are ready.
func NewPodReady(nt *NT, labelName, currentLabel, childLabel string, oldCurrentPods, oldChildPods []corev1.Pod) error {
Expand Down
22 changes: 22 additions & 0 deletions e2e/nomostest/testpredicates/predicates.go
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,28 @@ func HasExactlyLabelKeys(wantKeys ...string) Predicate {
}
}

// HasExactlyNodeAffinity ensures the Pod has the provided nodeAffinity
func HasExactlyNodeAffinity(nodeAffinity *corev1.NodeAffinity) Predicate {
return func(o client.Object) error {
if o == nil {
return ErrObjectNotFound
}
pod, ok := o.(*corev1.Pod)
if !ok {
return WrongTypeErr(pod, &corev1.Pod{})
}
gotAffinity := &corev1.NodeAffinity{}
if pod.Spec.Affinity != nil && pod.Spec.Affinity.NodeAffinity != nil {
gotAffinity = pod.Spec.Affinity.NodeAffinity
}
if !equality.Semantic.DeepEqual(nodeAffinity, gotAffinity) {
return fmt.Errorf("expected %s to have spec.affinity.nodeAffinity: %s, but got %s",
kinds.ObjectSummary(pod), log.AsJSON(nodeAffinity), log.AsJSON(gotAffinity))
}
return nil
}
}

// HasExactlyImage ensures a container has the expected image.
func HasExactlyImage(containerName, expectImageName, expectImageTag, expectImageDigest string) Predicate {
return func(o client.Object) error {
Expand Down
77 changes: 77 additions & 0 deletions e2e/testcases/mutating_admission_policy_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
package e2e

import (
"path/filepath"
"testing"
"time"

corev1 "k8s.io/api/core/v1"
"kpt.dev/configsync/e2e/nomostest"
"kpt.dev/configsync/e2e/nomostest/ntopts"
"kpt.dev/configsync/e2e/nomostest/syncsource"
nomostesting "kpt.dev/configsync/e2e/nomostest/testing"
"kpt.dev/configsync/e2e/nomostest/testpredicates"
"kpt.dev/configsync/pkg/api/configsync"
"kpt.dev/configsync/pkg/core/k8sobjects"
"kpt.dev/configsync/pkg/kinds"
"kpt.dev/configsync/pkg/reconcilermanager"
)

// This test currently requires KinD because MutatingAdmissionPolicy is alpha
// and requires a feature gate.
func TestMutatingAdmissionPolicy(t *testing.T) {
nt := nomostest.New(t, nomostesting.Reconciliation2,
ntopts.SyncWithGitSource(nomostest.DefaultRootSyncID, ntopts.Unstructured),
ntopts.RequireKind(t))
rootSyncGitRepo := nt.SyncSourceGitReadWriteRepository(nomostest.DefaultRootSyncID)

mapFile := filepath.Join(".", "..", "..", "examples", "mutating-admission-policies", "config-sync-node-placement.yaml")
nt.T.Cleanup(func() {
nt.Must(nt.Shell.Kubectl("delete", "--ignore-not-found", "-f", mapFile))
})
nt.Must(nt.Shell.Kubectl("apply", "-f", mapFile))
// TODO: is there a way to wait for MutatingAdmissionPolicy readiness? (it doesn't appear so)
// sleep hack to give time to propagate
time.Sleep(5 * time.Second)
Comment on lines +33 to +35

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The use of time.Sleep can introduce flakiness into the test suite. While MutatingAdmissionPolicy currently lacks a status field for readiness, a more reliable approach would be to actively poll for the policy to become effective.

A robust alternative to a fixed sleep duration would be to attempt to create a resource that the policy should mutate, and retry until the mutation is observed. This would provide a more deterministic signal that the admission policy is active and ready.


// expected nodeAffinity from the example MutatingAdmissionPolicy yaml
exampleNodeAffinity := &corev1.NodeAffinity{
PreferredDuringSchedulingIgnoredDuringExecution: []corev1.PreferredSchedulingTerm{
{
Weight: 1,
Preference: corev1.NodeSelectorTerm{
MatchExpressions: []corev1.NodeSelectorRequirement{
{
Key: "another-node-label-key",
Operator: corev1.NodeSelectorOpIn,
Values: []string{"another-node-label-value"},
},
},
},
},
},
}

// bounce reconciler-manager Pod and verify the nodeAffinity is applied by MAP
nt.Must(nomostest.ValidatePodByLabel(nt, "app", reconcilermanager.ManagerName,
testpredicates.HasExactlyNodeAffinity(&corev1.NodeAffinity{})))
nt.T.Log("Replacing the reconciler-manager Pod to validate nodeAffinity is added")
nomostest.DeletePodByLabel(nt, "app", reconcilermanager.ManagerName, false)
nt.Must(nomostest.ValidatePodByLabel(nt, "app", reconcilermanager.ManagerName,
testpredicates.HasExactlyNodeAffinity(exampleNodeAffinity)))

// update the RootSync to trigger a Deployment change, verify new reconciler Pod has nodeAffinity
rootSync := nomostest.RootSyncObjectV1Beta1FromRootRepo(nt, nomostest.DefaultRootSyncID.Name)
rootSync.Spec.Git.Dir = "foo"
nt.Must(nt.KubeClient.Apply(rootSync))
nt.Must(rootSyncGitRepo.Add("foo/ns.yaml", k8sobjects.NamespaceObject("test-map-ns")))
nt.Must(rootSyncGitRepo.CommitAndPush("add foo-ns under foo/ dir"))
nt.Must(nt.WatchForSync(kinds.RootSyncV1Beta1(), rootSync.Name, configsync.ControllerNamespace,
&syncsource.GitSyncSource{
ExpectedCommit: rootSyncGitRepo.MustHash(t),
ExpectedDirectory: "foo",
}))
nt.Must(nt.Validate("test-map-ns", "", &corev1.Namespace{}))
nt.Must(nomostest.ValidatePodByLabel(nt, "app", reconcilermanager.Reconciler,
testpredicates.HasExactlyNodeAffinity(exampleNodeAffinity)))
}
21 changes: 21 additions & 0 deletions examples/mutating-admission-policies/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
# Scheduling Config Sync system pods using MutatingAdmissionPolicies

[MutatingAdmissionPolicy] can be used to configure the way the Config Sync's
system Pods are scheduled.

The [example in this directory](./config-sync-node-placement.yaml) demonstrates
how to inject `nodeAffinity` into all Pods in the `config-management-system` Namespace.

Note that this is just a demonstrative example, and different `matchConstraints` and
`mutations` can be applied depending on the use case.

Caveats:

- MutatingAdmissionPolicy is currently in alpha and requires feature gate enablement. It is [targeted to enter beta in k8s 1.34].
- MutatingAdmissionPolicy does not update existing Pods. Pods created before the policy was applied must be updated/recreated.




[MutatingAdmissionPolicy]: https://kubernetes.io/docs/reference/access-authn-authz/mutating-admission-policy/
[targeted to enter beta in k8s 1.34]: https://github.com/kubernetes/enhancements/issues/3962
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
apiVersion: admissionregistration.k8s.io/v1alpha1
kind: MutatingAdmissionPolicy
metadata:
name: "configsync-nodeplacement"
spec:
# This example uniformly applies the nodeAffinity to *all* Pods in config-management-system.
# Different matchConstraints can be used for more granular mutation.
matchConstraints:
namespaceSelector:
matchLabels:
kubernetes.io/metadata.name: config-management-system
resourceRules:
- apiGroups: [""]
apiVersions: ["v1"]
operations: ["CREATE", "UPDATE"]
resources: ["pods"]
failurePolicy: Fail
reinvocationPolicy: IfNeeded
mutations:
# Simple example of adding nodeAffinity: https://kubernetes.io/docs/concepts/scheduling-eviction/assign-pod-node/
# Similar mutations can be applied for nodeSelector/tolerations/etc
- patchType: "JSONPatch"
jsonPatch:
expression: >
[
JSONPatch{
op: "add", path: "/spec/affinity",
value: Object.spec.affinity{
nodeAffinity: Object.spec.affinity.nodeAffinity{
preferredDuringSchedulingIgnoredDuringExecution: [
Object.spec.affinity.nodeAffinity.preferredDuringSchedulingIgnoredDuringExecution{
weight: 1,
preference: Object.spec.affinity.nodeAffinity.preferredDuringSchedulingIgnoredDuringExecution.preference{
matchExpressions: [
Object.spec.affinity.nodeAffinity.preferredDuringSchedulingIgnoredDuringExecution.preference.matchExpressions{
key: "another-node-label-key",
operator: "In",
values: [
"another-node-label-value"
]
}
]
}
}
]
}
}
}
]
Comment on lines +24 to +49

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

This CEL expression is confusing and appears to use a non-standard syntax for constructing the patch value. Using constructs like Object.spec.affinity as type names is likely incorrect and will be confusing for users trying to adapt this example.

For a user-facing example, it's crucial that the code is clear, robust, and follows documented conventions. A better approach would be to use standard CEL to construct the patch, or if using CEL's proto message construction capabilities, to use the correct, fully-qualified type names (e.g., core.v1.Affinity).

Here is a suggested correction that uses more conventional type names. This assumes the JSONPatch constructor and the core.v1 types are available in the CEL environment.

      expression: >
        [
          JSONPatch{
            op: "add", path: "/spec/affinity",
            value: core.v1.Affinity{
              nodeAffinity: core.v1.NodeAffinity{
                preferredDuringSchedulingIgnoredDuringExecution: [
                  core.v1.PreferredSchedulingTerm{
                    weight: 1,
                    preference: core.v1.NodeSelectorTerm{
                      matchExpressions: [
                        core.v1.NodeSelectorRequirement{
                          key: "another-node-label-key",
                          operator: "In",
                          values: [
                            "another-node-label-value"
                          ]
                        }
                      ]
                    }
                  }
                ]
              }
            }
          }
        ]

---
apiVersion: admissionregistration.k8s.io/v1alpha1
kind: MutatingAdmissionPolicyBinding
metadata:
name: "configsync-nodeplacement"
spec:
policyName: "configsync-nodeplacement"