From 498338c6266124bdd5653aa9a08f450f5de20649 Mon Sep 17 00:00:00 2001 From: kerthcet Date: Sat, 3 Oct 2026 17:56:11 +0100 Subject: [PATCH 1/5] remove encryptedPort from Modal Signed-off-by: kerthcet --- pkg/provider/modal/client.go | 1 - pkg/provider/modal/modal.go | 11 ++++------- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/pkg/provider/modal/client.go b/pkg/provider/modal/client.go index f1a5c8f..a96b443 100644 --- a/pkg/provider/modal/client.go +++ b/pkg/provider/modal/client.go @@ -180,7 +180,6 @@ func (c *sdkClient) CreateSandbox(ctx context.Context, spec SandboxSpec) (string MemoryMiB: spec.MemoryMiB, CPULimit: spec.CPULimit, MemoryLimitMiB: spec.MemoryLimitMiB, - EncryptedPorts: spec.Ports, // Nil leaves Modal's SchedulerPlacement unset entirely (the SDK only builds one // when Regions is non-empty), which is the unconstrained, un-multiplied case. Regions: spec.Regions, diff --git a/pkg/provider/modal/modal.go b/pkg/provider/modal/modal.go index 9dd272b..ed87586 100644 --- a/pkg/provider/modal/modal.go +++ b/pkg/provider/modal/modal.go @@ -173,10 +173,8 @@ type SandboxSpec struct { // size with an unbounded ceiling — the inverse of what it asked for, and billable. CPULimit float64 MemoryLimitMiB int - // Ports are the container ports to expose, from the Pod's containerPorts. They - // declare to Modal which ports may receive traffic at all, and the connect URL - // routes to the first of them (see firstPort) — one token routes to one port. - // Empty leaves both the exposed set and the routed port to Modal's own default. + // Ports are the Pod's containerPorts. Only the first is reachable from outside, via + // the connect URL (see firstPort); none is opened as a tunnel (see CreateSandbox). Ports []int // Regions constrains where Modal may place the sandbox, in Modal's own // vocabulary — a broad region ("us", "eu", "ap") or a narrow one ("us-east", @@ -791,9 +789,8 @@ func limitQty(c *corev1.Container, name corev1.ResourceName) *resource.Quantity return nil } -// containerPorts collects the container's declared ports, which is what tells Modal -// which ports may receive traffic at all. The connect URL then routes to one of them -// (see firstPort). +// containerPorts collects the container's declared ports; the connect URL routes to +// one of them (see firstPort). func containerPorts(c *corev1.Container) []int { if len(c.Ports) == 0 { return nil From 557e729200b99ec6bbaa982a11f2b195ecbcb8bc Mon Sep 17 00:00:00 2001 From: kerthcet Date: Sat, 3 Oct 2026 20:17:09 +0100 Subject: [PATCH 2/5] add pod validation Signed-off-by: kerthcet --- config/webhook/kustomization.yaml | 7 +- config/webhook/manifests.yaml | 22 ++++- .../mutating_pod_selector.yaml} | 0 .../patches/validating_pod_selector.yaml | 22 +++++ internal/webhook/v1/nodepool_webhook.go | 4 +- internal/webhook/v1/pod_webhook.go | 70 ++++++++++++++ internal/webhook/v1/pod_webhook_test.go | 95 +++++++++++++++++++ pkg/provider/modal/client.go | 6 +- pkg/provider/modal/modal.go | 5 +- pkg/provider/modal/modal_test.go | 17 ++-- test/e2e/e2e_test.go | 2 +- 11 files changed, 226 insertions(+), 24 deletions(-) rename config/webhook/{selector_patch.yaml => patches/mutating_pod_selector.yaml} (100%) create mode 100644 config/webhook/patches/validating_pod_selector.yaml diff --git a/config/webhook/kustomization.yaml b/config/webhook/kustomization.yaml index e74dbed..cc05241 100644 --- a/config/webhook/kustomization.yaml +++ b/config/webhook/kustomization.yaml @@ -5,11 +5,14 @@ resources: # but never Creates it, so without this the manager crash-loops. See secret.yaml. - secret.yaml -# Narrow the generated Pod webhook to opt-in Pods outside system namespaces. +# Narrow the generated Pod webhooks to opt-in Pods outside system namespaces. The patches +# live in a subdirectory because envtest installs every webhook-kind file at THIS level +# (non-recursively), and a patch read as an object breaks its setup. patches: -- path: selector_patch.yaml +- path: patches/mutating_pod_selector.yaml target: kind: MutatingWebhookConfiguration +- path: patches/validating_pod_selector.yaml configurations: - kustomizeconfig.yaml diff --git a/config/webhook/manifests.yaml b/config/webhook/manifests.yaml index bcfe766..9092831 100644 --- a/config/webhook/manifests.yaml +++ b/config/webhook/manifests.yaml @@ -36,7 +36,7 @@ webhooks: name: webhook-service namespace: system path: /validate-nebula-inftyai-com-v1alpha1-nodepool - failurePolicy: Fail + failurePolicy: Ignore name: vnodepool-v1alpha1.nebula.inftyai.com rules: - apiGroups: @@ -49,3 +49,23 @@ webhooks: resources: - nodepools sideEffects: None +- admissionReviewVersions: + - v1 + clientConfig: + service: + name: webhook-service + namespace: system + path: /validate--v1-pod + failurePolicy: Fail + name: vpod-v1.nebula.inftyai.com + rules: + - apiGroups: + - "" + apiVersions: + - v1 + operations: + - CREATE + - UPDATE + resources: + - pods + sideEffects: None diff --git a/config/webhook/selector_patch.yaml b/config/webhook/patches/mutating_pod_selector.yaml similarity index 100% rename from config/webhook/selector_patch.yaml rename to config/webhook/patches/mutating_pod_selector.yaml diff --git a/config/webhook/patches/validating_pod_selector.yaml b/config/webhook/patches/validating_pod_selector.yaml new file mode 100644 index 0000000..c73b0a9 --- /dev/null +++ b/config/webhook/patches/validating_pod_selector.yaml @@ -0,0 +1,22 @@ +# The same narrowing as mutating_pod_selector.yaml, for the Pod VALIDATING webhook: it is +# just as failurePolicy=Fail and on the Pod-CREATE path. Merged by webhook name rather than +# by index, because this configuration also holds the NodePool webhook, which must stay +# unselected. +apiVersion: admissionregistration.k8s.io/v1 +kind: ValidatingWebhookConfiguration +metadata: + name: validating-webhook-configuration +webhooks: +- name: vpod-v1.nebula.inftyai.com + objectSelector: + matchLabels: + nebula.inftyai.com/enabled: "true" + namespaceSelector: + matchExpressions: + - key: kubernetes.io/metadata.name + operator: NotIn + values: + - kube-system + - kube-node-lease + - kube-public + - nebula-system diff --git a/internal/webhook/v1/nodepool_webhook.go b/internal/webhook/v1/nodepool_webhook.go index 12333f2..0845ae0 100644 --- a/internal/webhook/v1/nodepool_webhook.go +++ b/internal/webhook/v1/nodepool_webhook.go @@ -36,7 +36,9 @@ func SetupNodePoolWebhookWithManager(mgr ctrl.Manager) error { Complete() } -// +kubebuilder:webhook:path=/validate-nebula-inftyai-com-v1alpha1-nodepool,mutating=false,failurePolicy=fail,sideEffects=None,groups=nebula.inftyai.com,resources=nodepools,verbs=create;update,versions=v1alpha1,name=vnodepool-v1alpha1.nebula.inftyai.com,admissionReviewVersions=v1 +// failurePolicy=ignore: the validator only warns, so a webhook outage must not block +// NodePool writes. The Pod validator enforces runtime limits and stays Fail. +// +kubebuilder:webhook:path=/validate-nebula-inftyai-com-v1alpha1-nodepool,mutating=false,failurePolicy=ignore,sideEffects=None,groups=nebula.inftyai.com,resources=nodepools,verbs=create;update,versions=v1alpha1,name=vnodepool-v1alpha1.nebula.inftyai.com,admissionReviewVersions=v1 // NodePoolCustomValidator validates NodePools on create and update. A setting a listed // provider cannot serve (Spot on Modal, a restricted egress on RunPod) is NOT rejected: diff --git a/internal/webhook/v1/pod_webhook.go b/internal/webhook/v1/pod_webhook.go index 3c43a80..9f688d1 100644 --- a/internal/webhook/v1/pod_webhook.go +++ b/internal/webhook/v1/pod_webhook.go @@ -25,6 +25,7 @@ import ( ctrl "sigs.k8s.io/controller-runtime" logf "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/webhook" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" nebulav1alpha1 "github.com/InftyAI/Nebula/api/v1alpha1" ) @@ -35,6 +36,7 @@ var podlog = logf.Log.WithName("pod-webhook") func SetupPodWebhookWithManager(mgr ctrl.Manager) error { return ctrl.NewWebhookManagedBy(mgr).For(&corev1.Pod{}). WithDefaulter(&PodCustomDefaulter{}). + WithValidator(&PodCustomValidator{}). Complete() } @@ -107,6 +109,74 @@ func (d *PodCustomDefaulter) Default(_ context.Context, obj runtime.Object) erro return nil } +// +kubebuilder:webhook:path=/validate--v1-pod,mutating=false,failurePolicy=fail,sideEffects=None,groups="",resources=pods,verbs=create;update,versions=v1,name=vpod-v1.nebula.inftyai.com,admissionReviewVersions=v1 + +// PodCustomValidator rejects an opted-in Pod the providers cannot run as written: they +// launch one container and no init containers, and expose at most one port (Modal's +// connect URL routes to one; see modal.firstPort). Admitting more would drop the rest +// silently. +type PodCustomValidator struct{} + +var _ webhook.CustomValidator = &PodCustomValidator{} + +// ValidateCreate implements webhook.CustomValidator. +func (v *PodCustomValidator) ValidateCreate(_ context.Context, obj runtime.Object) (admission.Warnings, error) { + pod, ok := obj.(*corev1.Pod) + if !ok { + return nil, fmt.Errorf("expected a Pod object but got %T", obj) + } + return nil, validatePod(pod) +} + +// ValidateUpdate implements webhook.CustomValidator. The shape is immutable but the opt-in +// label is not, so only a transition INTO opted-in is judged. An already opted-in Pod is +// left alone: one admitted before this check existed would otherwise have every later +// write rejected, placement's gate release among them, and stay gated forever. +func (v *PodCustomValidator) ValidateUpdate(_ context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) { + oldPod, ok := oldObj.(*corev1.Pod) + if !ok { + return nil, fmt.Errorf("expected a Pod object but got %T", oldObj) + } + newPod, ok := newObj.(*corev1.Pod) + if !ok { + return nil, fmt.Errorf("expected a Pod object but got %T", newObj) + } + if optedIn(oldPod) { + return nil, nil + } + return nil, validatePod(newPod) +} + +// ValidateDelete implements webhook.CustomValidator. Not registered for deletes. +func (v *PodCustomValidator) ValidateDelete(_ context.Context, _ runtime.Object) (admission.Warnings, error) { + return nil, nil +} + +func optedIn(pod *corev1.Pod) bool { + return pod.Labels[nebulav1alpha1.EnabledLabel] == nebulav1alpha1.EnabledValue +} + +// validatePod enforces PodCustomValidator's rules on an opted-in Pod placement would see. +func validatePod(pod *corev1.Pod) error { + if !optedIn(pod) { + return nil + } + if pod.Spec.NodeName != "" { + return nil // pre-bound: never placed, so never run by a provider (see needsPlacement) + } + if n := len(pod.Spec.Containers); n != 1 { + return fmt.Errorf("nebula runs exactly one container per Pod, got %d", n) + } + // Native sidecars are init containers too, so this rejects them as well. + if n := len(pod.Spec.InitContainers); n > 0 { + return fmt.Errorf("nebula does not run init containers, got %d", n) + } + if c := pod.Spec.Containers[0]; len(c.Ports) > 1 { + return fmt.Errorf("nebula exposes at most one port per Pod, container %q declares %d", c.Name, len(c.Ports)) + } + return nil +} + // hasGate reports whether the Pod already carries the named scheduling gate. func hasGate(pod *corev1.Pod, name string) bool { for _, g := range pod.Spec.SchedulingGates { diff --git a/internal/webhook/v1/pod_webhook_test.go b/internal/webhook/v1/pod_webhook_test.go index 93784e1..a7ec9b8 100644 --- a/internal/webhook/v1/pod_webhook_test.go +++ b/internal/webhook/v1/pod_webhook_test.go @@ -18,6 +18,7 @@ package v1 import ( "context" + "strings" "testing" corev1 "k8s.io/api/core/v1" @@ -172,3 +173,97 @@ func TestDefault_RejectsNonPod(t *testing.T) { t.Fatal("expected an error for a non-Pod object") } } + +func TestValidateCreate_OneContainerOnePort(t *testing.T) { + optedIn := map[string]string{nebulav1alpha1.EnabledLabel: "true"} + container := func(name string, ports ...int32) corev1.Container { + c := corev1.Container{Name: name} + for _, p := range ports { + c.Ports = append(c.Ports, corev1.ContainerPort{ContainerPort: p}) + } + return c + } + cases := []struct { + name string + labels map[string]string + containers []corev1.Container + inits []corev1.Container + wantErr string + }{ + {"one container, one port", optedIn, []corev1.Container{container("main", 8080)}, nil, ""}, + {"one container, no port", optedIn, []corev1.Container{container("main")}, nil, ""}, + {"two ports", optedIn, []corev1.Container{container("main", 8080, 9090)}, nil, "at most one port"}, + {"two containers", optedIn, []corev1.Container{container("main", 8080), container("sidecar")}, nil, "exactly one container"}, + {"no container", optedIn, nil, nil, "exactly one container"}, + {"init container", optedIn, []corev1.Container{container("main")}, []corev1.Container{container("setup")}, "init containers"}, + // Pods outside Nebula are never Nebula's to judge, whatever the selector says. + {"not opted in", nil, []corev1.Container{container("a", 1, 2), container("b")}, []corev1.Container{container("i")}, ""}, + } + v := &PodCustomValidator{} + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + pod := podWith(tc.labels, "") + pod.Spec.Containers = tc.containers + pod.Spec.InitContainers = tc.inits + _, err := v.ValidateCreate(context.Background(), pod) + switch { + case tc.wantErr == "" && err != nil: + t.Fatalf("ValidateCreate err = %v, want nil", err) + case tc.wantErr != "" && (err == nil || !strings.Contains(err.Error(), tc.wantErr)): + t.Fatalf("ValidateCreate err = %v, want it to contain %q", err, tc.wantErr) + } + }) + } +} + +func TestValidateCreate_RejectsNonPod(t *testing.T) { + if _, err := (&PodCustomValidator{}).ValidateCreate(context.Background(), &corev1.Service{}); err == nil { + t.Fatal("expected an error for a non-Pod object") + } +} + +// A pre-bound Pod skips placement (see needsPlacement), so no provider ever runs it and its +// shape is not Nebula's to judge, even when it carries the opt-in label. +func TestValidateCreate_SkipsPreBoundPod(t *testing.T) { + pod := podWith(map[string]string{nebulav1alpha1.EnabledLabel: "true"}, "node-1") + pod.Spec.Containers = []corev1.Container{ + {Name: "main", Ports: []corev1.ContainerPort{{ContainerPort: 8080}, {ContainerPort: 9090}}}, + {Name: "sidecar"}, + } + pod.Spec.InitContainers = []corev1.Container{{Name: "setup"}} + if _, err := (&PodCustomValidator{}).ValidateCreate(context.Background(), pod); err != nil { + t.Fatalf("ValidateCreate err = %v, want nil for a pre-bound Pod", err) + } +} + +func TestValidateUpdate_JudgesOnlyTransitionsIntoOptedIn(t *testing.T) { + optedIn := map[string]string{nebulav1alpha1.EnabledLabel: "true"} + sidecarPod := func(labels map[string]string) *corev1.Pod { + pod := podWith(labels, "", nebulav1alpha1.ProviderSelectionGate) + pod.Spec.Containers = []corev1.Container{{Name: "main"}, {Name: "sidecar"}} + return pod + } + cases := []struct { + name string + old, new *corev1.Pod + wantErr bool + }{ + // The bypass: created unlabelled (so no webhook ran) with the gate already set, then + // relabelled, which would hand placement a Pod it cannot run. + {"relabelled into opted-in", sidecarPod(nil), sidecarPod(optedIn), true}, + // An opted-in Pod admitted before this check existed: rejecting its later writes + // would leave it gated forever. + {"already opted in", sidecarPod(optedIn), sidecarPod(optedIn), false}, + {"still not opted in", sidecarPod(nil), sidecarPod(map[string]string{"other": "x"}), false}, + {"opting out", sidecarPod(optedIn), sidecarPod(nil), false}, + } + v := &PodCustomValidator{} + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + _, err := v.ValidateUpdate(context.Background(), tc.old, tc.new) + if (err != nil) != tc.wantErr { + t.Fatalf("ValidateUpdate err = %v, want error: %t", err, tc.wantErr) + } + }) + } +} diff --git a/pkg/provider/modal/client.go b/pkg/provider/modal/client.go index a96b443..b84e30f 100644 --- a/pkg/provider/modal/client.go +++ b/pkg/provider/modal/client.go @@ -330,8 +330,6 @@ func (c *sdkClient) registrySecret(ctx context.Context, kv map[string]string) (* // provider off rather than be scoped to one request. func (c *sdkClient) mintCredential(ctx context.Context, sb *modal.Sandbox, port int) (Credential, error) { creds, err := sb.CreateConnectToken(ctx, &modal.SandboxCreateConnectTokenParams{ - // Derived from the exposed set rather than carried separately, so the routed - // port cannot name one the sandbox was never told to accept traffic on. Port: port, }) if err != nil { @@ -792,9 +790,7 @@ func (c *sdkClient) observe(ctx context.Context, sb *modal.Sandbox) (Sandbox, er // minted at create and already persisted on the Pod's endpoint annotation, so // re-deriving it per tick would be a round trip for a value the API server holds. // - // The alternative — a tunnel URL — is worse than nothing: a tunnel is PUBLIC to - // whoever learns it, so substituting one for an authenticated URL silently downgrades - // access. A sandbox whose mint failed reports no address at all, and that stays a FACT + // A sandbox whose mint failed reports no address at all, and that stays a FACT // rather than an error, since observe's errors fail the entire List. return out, nil } diff --git a/pkg/provider/modal/modal.go b/pkg/provider/modal/modal.go index ed87586..dbc72a8 100644 --- a/pkg/provider/modal/modal.go +++ b/pkg/provider/modal/modal.go @@ -174,7 +174,7 @@ type SandboxSpec struct { CPULimit float64 MemoryLimitMiB int // Ports are the Pod's containerPorts. Only the first is reachable from outside, via - // the connect URL (see firstPort); none is opened as a tunnel (see CreateSandbox). + // the connect URL (see firstPort); none is opened as a tunnel. Ports []int // Regions constrains where Modal may place the sandbox, in Modal's own // vocabulary — a broad region ("us", "eu", "ap") or a narrow one ("us-east", @@ -244,8 +244,7 @@ func (s SandboxSpec) GoString() string { return s.String() } // No endpoint here, deliberately: Modal's reachable address is the connect URL, minted at // CREATE time and published to the Pod's endpoint annotation, where it persists for the // sandbox's life. Re-deriving it per read would be a round trip for a value the API server -// already holds. The only other candidate, a tunnel URL, is PUBLIC to anyone who learns -// it, so substituting it for an authenticated URL would downgrade access. +// already holds. type Sandbox struct { ID string Tags map[string]string diff --git a/pkg/provider/modal/modal_test.go b/pkg/provider/modal/modal_test.go index c15a5e0..2be983f 100644 --- a/pkg/provider/modal/modal_test.go +++ b/pkg/provider/modal/modal_test.go @@ -869,11 +869,6 @@ func TestPlanProbe(t *testing.T) { } } -// The spec carries the container's declared ports verbatim: they are the set Modal is -// told to accept traffic on, and the connect URL routes to the first of them (the -// client derives it, so the routed port can never name one outside the set). No -// declared port is not "no endpoint" — every workload is credentialed — it means Modal -// picks, defaulting to 8080. func TestProvision_CarriesRegion(t *testing.T) { for _, tc := range []struct { name string @@ -1215,6 +1210,9 @@ func TestClassifyProvisionError_ImageBuildNeverDeniesTheProvider(t *testing.T) { } } +// The spec carries the declared ports verbatim and the connect URL routes to the first; +// none is opened as a tunnel, so the rest have no outside address. No declared port still +// gets a credential, routed to Modal's default 8080. func TestProvision_CarriesDeclaredPorts(t *testing.T) { for _, tc := range []struct { name string @@ -1226,10 +1224,9 @@ func TestProvision_CarriesDeclaredPorts(t *testing.T) { }{ {"no ports leaves the port to Modal", nil, nil, 0}, {"single port", []corev1.ContainerPort{{ContainerPort: 8000}}, []int{8000}, 8000}, - // Modal routes one port per token, so the first declared port wins — but the - // whole set is still exposed. + // Modal routes one port per token, so the first declared port wins. { - "all exposed, first routed", + "first declared port is routed", []corev1.ContainerPort{{ContainerPort: 8000}, {ContainerPort: 9090}}, []int{8000, 9090}, 8000, @@ -1390,9 +1387,7 @@ func TestProvision_IdempotentReturnsNoCredential(t *testing.T) { // Modal reports NO observed endpoint. Its address is the connect URL, published from // the create path onto the Pod's annotation, where it persists; re-deriving it per tick -// would be a round trip for a value the API server already holds. The alternative — -// falling back to a tunnel URL — is worse than nothing, since a tunnel is public to -// whoever learns it. +// would be a round trip for a value the API server already holds. func TestToInstance_ReportsNoEndpoint(t *testing.T) { p := newTestProvider(&fakeClient{}) diff --git a/test/e2e/e2e_test.go b/test/e2e/e2e_test.go index d7d1d2d..f5e42c9 100644 --- a/test/e2e/e2e_test.go +++ b/test/e2e/e2e_test.go @@ -55,7 +55,7 @@ const ( fakeWorkloadPod = "e2e-fake-workload" // fakeWorkloadNS is a dedicated namespace for the placement-flow workload. It // must NOT be the manager namespace: the mutating webhook's namespaceSelector - // excludes nebula-system (see config/webhook/selector_patch.yaml), so a Pod + // excludes nebula-system (see config/webhook/patches/mutating_pod_selector.yaml), so a Pod // there would never get the scheduling gate and placement would never run. fakeWorkloadNS = "nebula-e2e-workload" From 95f0bc22453e2cf378b6651305dec293f5026c02 Mon Sep 17 00:00:00 2001 From: kerthcet Date: Sat, 3 Oct 2026 20:27:59 +0100 Subject: [PATCH 3/5] update webhook Signed-off-by: kerthcet --- internal/webhook/v1/pod_webhook.go | 13 ++++++++++--- internal/webhook/v1/pod_webhook_test.go | 11 +++++++++-- 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/internal/webhook/v1/pod_webhook.go b/internal/webhook/v1/pod_webhook.go index 9f688d1..be5c8bd 100644 --- a/internal/webhook/v1/pod_webhook.go +++ b/internal/webhook/v1/pod_webhook.go @@ -129,9 +129,10 @@ func (v *PodCustomValidator) ValidateCreate(_ context.Context, obj runtime.Objec } // ValidateUpdate implements webhook.CustomValidator. The shape is immutable but the opt-in -// label is not, so only a transition INTO opted-in is judged. An already opted-in Pod is -// left alone: one admitted before this check existed would otherwise have every later -// write rejected, placement's gate release among them, and stay gated forever. +// label is not, so only label transitions are judged: opting in gets the CREATE rules, and +// opting out must not strand Nebula's gate. A Pod staying opted in is left alone: one +// admitted before this check existed would otherwise have every later write rejected, +// placement's gate release among them, and stay gated forever. func (v *PodCustomValidator) ValidateUpdate(_ context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) { oldPod, ok := oldObj.(*corev1.Pod) if !ok { @@ -142,6 +143,12 @@ func (v *PodCustomValidator) ValidateUpdate(_ context.Context, oldObj, newObj ru return nil, fmt.Errorf("expected a Pod object but got %T", newObj) } if optedIn(oldPod) { + // Placement ignores a Pod without the label, so it would never release the gate. + if !optedIn(newPod) && hasGate(newPod, nebulav1alpha1.ProviderSelectionGate) { + return nil, fmt.Errorf("removing label %s would leave the Pod gated forever: also remove "+ + "scheduling gate %q in the same update, or keep the label", + nebulav1alpha1.EnabledLabel, nebulav1alpha1.ProviderSelectionGate) + } return nil, nil } return nil, validatePod(newPod) diff --git a/internal/webhook/v1/pod_webhook_test.go b/internal/webhook/v1/pod_webhook_test.go index a7ec9b8..10cff3d 100644 --- a/internal/webhook/v1/pod_webhook_test.go +++ b/internal/webhook/v1/pod_webhook_test.go @@ -236,13 +236,17 @@ func TestValidateCreate_SkipsPreBoundPod(t *testing.T) { } } -func TestValidateUpdate_JudgesOnlyTransitionsIntoOptedIn(t *testing.T) { +func TestValidateUpdate_JudgesLabelTransitions(t *testing.T) { optedIn := map[string]string{nebulav1alpha1.EnabledLabel: "true"} sidecarPod := func(labels map[string]string) *corev1.Pod { pod := podWith(labels, "", nebulav1alpha1.ProviderSelectionGate) pod.Spec.Containers = []corev1.Container{{Name: "main"}, {Name: "sidecar"}} return pod } + released := func(pod *corev1.Pod) *corev1.Pod { + pod.Spec.SchedulingGates = nil + return pod + } cases := []struct { name string old, new *corev1.Pod @@ -255,7 +259,10 @@ func TestValidateUpdate_JudgesOnlyTransitionsIntoOptedIn(t *testing.T) { // would leave it gated forever. {"already opted in", sidecarPod(optedIn), sidecarPod(optedIn), false}, {"still not opted in", sidecarPod(nil), sidecarPod(map[string]string{"other": "x"}), false}, - {"opting out", sidecarPod(optedIn), sidecarPod(nil), false}, + // Placement ignores an unlabelled Pod, so nothing would ever release the gate it keeps. + {"opting out while gated", sidecarPod(optedIn), sidecarPod(nil), true}, + {"opting out and releasing the gate", sidecarPod(optedIn), released(sidecarPod(nil)), false}, + {"opting out after placement", released(sidecarPod(optedIn)), released(sidecarPod(nil)), false}, } v := &PodCustomValidator{} for _, tc := range cases { From 245dc8fe68b1545898c8c2caf5d56a368e364179 Mon Sep 17 00:00:00 2001 From: kerthcet Date: Sat, 3 Oct 2026 20:37:26 +0100 Subject: [PATCH 4/5] update webhook Signed-off-by: kerthcet --- internal/webhook/v1/pod_webhook.go | 16 +++++++++---- internal/webhook/v1/pod_webhook_test.go | 32 +++++++++++++++++-------- 2 files changed, 33 insertions(+), 15 deletions(-) diff --git a/internal/webhook/v1/pod_webhook.go b/internal/webhook/v1/pod_webhook.go index be5c8bd..2b76e9f 100644 --- a/internal/webhook/v1/pod_webhook.go +++ b/internal/webhook/v1/pod_webhook.go @@ -97,7 +97,7 @@ func (d *PodCustomDefaulter) Default(_ context.Context, obj runtime.Object) erro }) } - if hasGate(pod, nebulav1alpha1.ProviderSelectionGate) { + if hasNebulaGate(pod) { return nil // already gated; nothing more to do } @@ -144,13 +144,19 @@ func (v *PodCustomValidator) ValidateUpdate(_ context.Context, oldObj, newObj ru } if optedIn(oldPod) { // Placement ignores a Pod without the label, so it would never release the gate. - if !optedIn(newPod) && hasGate(newPod, nebulav1alpha1.ProviderSelectionGate) { + if !optedIn(newPod) && hasNebulaGate(newPod) { return nil, fmt.Errorf("removing label %s would leave the Pod gated forever: also remove "+ "scheduling gate %q in the same update, or keep the label", nebulav1alpha1.EnabledLabel, nebulav1alpha1.ProviderSelectionGate) } return nil, nil } + // Placement needs the gate, and only the CREATE defaulter can add it. + if optedIn(newPod) && newPod.Spec.NodeName == "" && !hasNebulaGate(newPod) { + return nil, fmt.Errorf("opt in at creation: label %s takes effect only on a new Pod, since "+ + "Kubernetes cannot add scheduling gate %q afterwards", + nebulav1alpha1.EnabledLabel, nebulav1alpha1.ProviderSelectionGate) + } return nil, validatePod(newPod) } @@ -184,10 +190,10 @@ func validatePod(pod *corev1.Pod) error { return nil } -// hasGate reports whether the Pod already carries the named scheduling gate. -func hasGate(pod *corev1.Pod, name string) bool { +// hasNebulaGate reports whether the Pod carries Nebula's provider-selection scheduling gate. +func hasNebulaGate(pod *corev1.Pod) bool { for _, g := range pod.Spec.SchedulingGates { - if g.Name == name { + if g.Name == nebulav1alpha1.ProviderSelectionGate { return true } } diff --git a/internal/webhook/v1/pod_webhook_test.go b/internal/webhook/v1/pod_webhook_test.go index 10cff3d..7aead31 100644 --- a/internal/webhook/v1/pod_webhook_test.go +++ b/internal/webhook/v1/pod_webhook_test.go @@ -28,7 +28,7 @@ import ( ) func gated(pod *corev1.Pod) bool { - return hasGate(pod, nebulav1alpha1.ProviderSelectionGate) + return hasNebulaGate(pod) } func podWith(labels map[string]string, nodeName string, gates ...string) *corev1.Pod { @@ -243,6 +243,12 @@ func TestValidateUpdate_JudgesLabelTransitions(t *testing.T) { pod.Spec.Containers = []corev1.Container{{Name: "main"}, {Name: "sidecar"}} return pod } + // ungatedPod is valid in shape but was created without the label, so it never got the gate. + ungatedPod := func(labels map[string]string, nodeName string) *corev1.Pod { + pod := podWith(labels, nodeName) + pod.Spec.Containers = []corev1.Container{{Name: "main"}} + return pod + } released := func(pod *corev1.Pod) *corev1.Pod { pod.Spec.SchedulingGates = nil return pod @@ -250,26 +256,32 @@ func TestValidateUpdate_JudgesLabelTransitions(t *testing.T) { cases := []struct { name string old, new *corev1.Pod - wantErr bool + wantErr string // substring; empty means admitted }{ // The bypass: created unlabelled (so no webhook ran) with the gate already set, then // relabelled, which would hand placement a Pod it cannot run. - {"relabelled into opted-in", sidecarPod(nil), sidecarPod(optedIn), true}, + {"relabelled into opted-in", sidecarPod(nil), sidecarPod(optedIn), "exactly one container"}, + // Without the gate placement never sees it, and the gate cannot be added after CREATE. + {"relabelled without the gate", ungatedPod(nil, ""), ungatedPod(optedIn, ""), "opt in at creation"}, + {"relabelled without the gate but bound", ungatedPod(nil, "node-1"), ungatedPod(optedIn, "node-1"), ""}, // An opted-in Pod admitted before this check existed: rejecting its later writes // would leave it gated forever. - {"already opted in", sidecarPod(optedIn), sidecarPod(optedIn), false}, - {"still not opted in", sidecarPod(nil), sidecarPod(map[string]string{"other": "x"}), false}, + {"already opted in", sidecarPod(optedIn), sidecarPod(optedIn), ""}, + {"still not opted in", sidecarPod(nil), sidecarPod(map[string]string{"other": "x"}), ""}, // Placement ignores an unlabelled Pod, so nothing would ever release the gate it keeps. - {"opting out while gated", sidecarPod(optedIn), sidecarPod(nil), true}, - {"opting out and releasing the gate", sidecarPod(optedIn), released(sidecarPod(nil)), false}, - {"opting out after placement", released(sidecarPod(optedIn)), released(sidecarPod(nil)), false}, + {"opting out while gated", sidecarPod(optedIn), sidecarPod(nil), "gated forever"}, + {"opting out and releasing the gate", sidecarPod(optedIn), released(sidecarPod(nil)), ""}, + {"opting out after placement", released(sidecarPod(optedIn)), released(sidecarPod(nil)), ""}, } v := &PodCustomValidator{} for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { _, err := v.ValidateUpdate(context.Background(), tc.old, tc.new) - if (err != nil) != tc.wantErr { - t.Fatalf("ValidateUpdate err = %v, want error: %t", err, tc.wantErr) + switch { + case tc.wantErr == "" && err != nil: + t.Fatalf("ValidateUpdate err = %v, want nil", err) + case tc.wantErr != "" && (err == nil || !strings.Contains(err.Error(), tc.wantErr)): + t.Fatalf("ValidateUpdate err = %v, want it to contain %q", err, tc.wantErr) } }) } From ea18a15936afec36e73663318af839925090389b Mon Sep 17 00:00:00 2001 From: kerthcet Date: Sat, 3 Oct 2026 20:46:39 +0100 Subject: [PATCH 5/5] change logic Signed-off-by: kerthcet --- internal/webhook/v1/pod_webhook.go | 14 +++++++------- internal/webhook/v1/pod_webhook_test.go | 20 +++++++++++--------- 2 files changed, 18 insertions(+), 16 deletions(-) diff --git a/internal/webhook/v1/pod_webhook.go b/internal/webhook/v1/pod_webhook.go index 2b76e9f..c09c95b 100644 --- a/internal/webhook/v1/pod_webhook.go +++ b/internal/webhook/v1/pod_webhook.go @@ -129,7 +129,7 @@ func (v *PodCustomValidator) ValidateCreate(_ context.Context, obj runtime.Objec } // ValidateUpdate implements webhook.CustomValidator. The shape is immutable but the opt-in -// label is not, so only label transitions are judged: opting in gets the CREATE rules, and +// label is not, so only label transitions are judged: opting in is refused outright, and // opting out must not strand Nebula's gate. A Pod staying opted in is left alone: one // admitted before this check existed would otherwise have every later write rejected, // placement's gate release among them, and stay gated forever. @@ -151,13 +151,13 @@ func (v *PodCustomValidator) ValidateUpdate(_ context.Context, oldObj, newObj ru } return nil, nil } - // Placement needs the gate, and only the CREATE defaulter can add it. - if optedIn(newPod) && newPod.Spec.NodeName == "" && !hasNebulaGate(newPod) { - return nil, fmt.Errorf("opt in at creation: label %s takes effect only on a new Pod, since "+ - "Kubernetes cannot add scheduling gate %q afterwards", - nebulav1alpha1.EnabledLabel, nebulav1alpha1.ProviderSelectionGate) + // Opting in is CREATE-only: the defaulter's gate and toleration cannot be added later, + // and checking for hand-copied ones would drift whenever the defaulter adds another. + if optedIn(newPod) { + return nil, fmt.Errorf("opt in at creation: label %s takes effect only on a new Pod", + nebulav1alpha1.EnabledLabel) } - return nil, validatePod(newPod) + return nil, nil } // ValidateDelete implements webhook.CustomValidator. Not registered for deletes. diff --git a/internal/webhook/v1/pod_webhook_test.go b/internal/webhook/v1/pod_webhook_test.go index 7aead31..305552e 100644 --- a/internal/webhook/v1/pod_webhook_test.go +++ b/internal/webhook/v1/pod_webhook_test.go @@ -243,9 +243,9 @@ func TestValidateUpdate_JudgesLabelTransitions(t *testing.T) { pod.Spec.Containers = []corev1.Container{{Name: "main"}, {Name: "sidecar"}} return pod } - // ungatedPod is valid in shape but was created without the label, so it never got the gate. - ungatedPod := func(labels map[string]string, nodeName string) *corev1.Pod { - pod := podWith(labels, nodeName) + // validPod is valid in shape but was created without the label, so no defaulter ran on it. + validPod := func(labels map[string]string, nodeName string, gates ...string) *corev1.Pod { + pod := podWith(labels, nodeName, gates...) pod.Spec.Containers = []corev1.Container{{Name: "main"}} return pod } @@ -258,12 +258,14 @@ func TestValidateUpdate_JudgesLabelTransitions(t *testing.T) { old, new *corev1.Pod wantErr string // substring; empty means admitted }{ - // The bypass: created unlabelled (so no webhook ran) with the gate already set, then - // relabelled, which would hand placement a Pod it cannot run. - {"relabelled into opted-in", sidecarPod(nil), sidecarPod(optedIn), "exactly one container"}, - // Without the gate placement never sees it, and the gate cannot be added after CREATE. - {"relabelled without the gate", ungatedPod(nil, ""), ungatedPod(optedIn, ""), "opt in at creation"}, - {"relabelled without the gate but bound", ungatedPod(nil, "node-1"), ungatedPod(optedIn, "node-1"), ""}, + // Created unlabelled, so the defaulter never ran: whatever it hand-copied, the Pod + // may lack the gate or the toleration placement needs. + {"relabelled with a bad shape", sidecarPod(nil), sidecarPod(optedIn), "opt in at creation"}, + {"relabelled without the gate", validPod(nil, ""), validPod(optedIn, ""), "opt in at creation"}, + {"relabelled with the gate but no toleration", + validPod(nil, "", nebulav1alpha1.ProviderSelectionGate), + validPod(optedIn, "", nebulav1alpha1.ProviderSelectionGate), "opt in at creation"}, + {"relabelled while bound", validPod(nil, "node-1"), validPod(optedIn, "node-1"), "opt in at creation"}, // An opted-in Pod admitted before this check existed: rejecting its later writes // would leave it gated forever. {"already opted in", sidecarPod(optedIn), sidecarPod(optedIn), ""},