From 050f85868c615ec4f4409dba0c751c25f8aaba8f Mon Sep 17 00:00:00 2001 From: younsl Date: Tue, 28 Jul 2026 19:06:45 +0900 Subject: [PATCH 1/3] fix: reject nodeSelector on sandbox agents instead of silently dropping it Substrate ActorTemplates carry no node placement (actors are scheduled onto WorkerPool workers), so a per-agent deployment.nodeSelector on a SandboxAgent was silently discarded by the sandbox backend and the workload could land on an unintended node. Reject the configuration in ValidateSubstrateSandboxAgentSpec so the API server, reconciler, and compiler all surface an explicit error pointing at the WorkerPool's nodeSelector as the supported alternative. Fixes #2306 Signed-off-by: younsl --- go/api/v1alpha2/agent_spec_validation.go | 27 +++++++- go/api/v1alpha2/agent_spec_validation_test.go | 61 +++++++++++++++++++ 2 files changed, 85 insertions(+), 3 deletions(-) diff --git a/go/api/v1alpha2/agent_spec_validation.go b/go/api/v1alpha2/agent_spec_validation.go index 5e46592fb..5e108d9d0 100644 --- a/go/api/v1alpha2/agent_spec_validation.go +++ b/go/api/v1alpha2/agent_spec_validation.go @@ -6,8 +6,9 @@ import ( ) const ( - substrateSandboxSkillsUnsupportedMsg = "spec.skills is not supported for sandbox agents" - substrateSandboxBYOMissingCommandMsg = "BYO agents on substrate must set spec.byo.deployment.cmd (substrate does not fall back to the image entrypoint)" + substrateSandboxSkillsUnsupportedMsg = "spec.skills is not supported for sandbox agents" + substrateSandboxBYOMissingCommandMsg = "BYO agents on substrate must set spec.byo.deployment.cmd (substrate does not fall back to the image entrypoint)" + substrateSandboxNodeSelectorUnsupportedMsg = "deployment.nodeSelector is not supported for sandbox agents: substrate schedules actors onto WorkerPool workers, so set the WorkerPool's nodeSelector instead" ) // AgentSpecHasSkills reports whether the spec configures any skill sources. @@ -23,7 +24,9 @@ func AgentSpecHasSkills(spec *AgentSpec) bool { // does not support on Agent Substrate (for example declarative skills). Declarative // Python/Go and BYO (Go/Python) agents are supported; BYO agents must provide an explicit // command because substrate copies the container Command verbatim with no image-entrypoint -// fallback. +// fallback. A per-agent deployment.nodeSelector is rejected: substrate ActorTemplates carry +// no node placement (actors run on WorkerPool workers), so the selector would otherwise be +// silently dropped. func ValidateSubstrateSandboxAgentSpec(agent *SandboxAgent) error { if agent == nil { return nil @@ -32,6 +35,9 @@ func ValidateSubstrateSandboxAgentSpec(agent *SandboxAgent) error { if AgentSpecHasSkills(spec) { return fmt.Errorf("%s", substrateSandboxSkillsUnsupportedMsg) } + if len(agentSpecNodeSelector(spec)) > 0 { + return fmt.Errorf("%s", substrateSandboxNodeSelectorUnsupportedMsg) + } if spec.Type == AgentType_BYO { dep := spec.BYO // Trim so a whitespace-only cmd is rejected like an empty one (substrate would treat it @@ -42,3 +48,18 @@ func ValidateSubstrateSandboxAgentSpec(agent *SandboxAgent) error { } return nil } + +// agentSpecNodeSelector returns the per-agent deployment nodeSelector, whichever agent +// type carries it. +func agentSpecNodeSelector(spec *AgentSpec) map[string]string { + if spec == nil { + return nil + } + if spec.Declarative != nil && spec.Declarative.Deployment != nil { + return spec.Declarative.Deployment.NodeSelector + } + if spec.BYO != nil && spec.BYO.Deployment != nil { + return spec.BYO.Deployment.NodeSelector + } + return nil +} diff --git a/go/api/v1alpha2/agent_spec_validation_test.go b/go/api/v1alpha2/agent_spec_validation_test.go index 704edad3a..ea9b09b76 100644 --- a/go/api/v1alpha2/agent_spec_validation_test.go +++ b/go/api/v1alpha2/agent_spec_validation_test.go @@ -81,6 +81,67 @@ func TestValidateSubstrateSandboxAgentSpec(t *testing.T) { require.NoError(t, ValidateSubstrateSandboxAgentSpec(agent)) }) + t.Run("rejects declarative agents with a nodeSelector", func(t *testing.T) { + agent := &SandboxAgent{ + Spec: SandboxAgentSpec{ + AgentSpec: AgentSpec{ + Type: AgentType_Declarative, + Declarative: &DeclarativeAgentSpec{ + Runtime: DeclarativeRuntime_Go, + Deployment: &DeclarativeDeploymentSpec{ + SharedDeploymentSpec: SharedDeploymentSpec{ + NodeSelector: map[string]string{"kubernetes.io/arch": "amd64", "topology.kubernetes.io/zone": "z1"}, + }, + }, + }, + }, + }, + } + err := ValidateSubstrateSandboxAgentSpec(agent) + require.Error(t, err) + require.Contains(t, err.Error(), substrateSandboxNodeSelectorUnsupportedMsg) + }) + + t.Run("rejects BYO agents with a nodeSelector", func(t *testing.T) { + cmd := "/app" + agent := &SandboxAgent{ + Spec: SandboxAgentSpec{ + AgentSpec: AgentSpec{ + Type: AgentType_BYO, + BYO: &BYOAgentSpec{Deployment: &ByoDeploymentSpec{ + Image: "example/agent:latest", + Cmd: &cmd, + SharedDeploymentSpec: SharedDeploymentSpec{ + NodeSelector: map[string]string{"kubernetes.io/arch": "amd64"}, + }, + }}, + }, + }, + } + err := ValidateSubstrateSandboxAgentSpec(agent) + require.Error(t, err) + require.Contains(t, err.Error(), substrateSandboxNodeSelectorUnsupportedMsg) + }) + + t.Run("allows declarative agents with an empty nodeSelector", func(t *testing.T) { + agent := &SandboxAgent{ + Spec: SandboxAgentSpec{ + AgentSpec: AgentSpec{ + Type: AgentType_Declarative, + Declarative: &DeclarativeAgentSpec{ + Runtime: DeclarativeRuntime_Go, + Deployment: &DeclarativeDeploymentSpec{ + SharedDeploymentSpec: SharedDeploymentSpec{ + NodeSelector: map[string]string{}, + }, + }, + }, + }, + }, + } + require.NoError(t, ValidateSubstrateSandboxAgentSpec(agent)) + }) + t.Run("allows go runtime", func(t *testing.T) { agent := &SandboxAgent{ Spec: SandboxAgentSpec{ From cfa89067e855af3c19852bbe7bcab9559fd2695b Mon Sep 17 00:00:00 2001 From: younsl Date: Tue, 28 Jul 2026 23:22:26 +0900 Subject: [PATCH 2/3] fix: enforce sandbox nodeSelector rejection with CEL rules at admission Per review feedback, reject deployment.nodeSelector on SandboxAgent via CEL XValidation rules on SandboxAgentSpec so the API server rejects the config at admission instead of the controller discovering it at reconcile time. Covers both declarative and BYO deployments; an empty map is still accepted, matching the Go check. The Go-side ValidateSubstrateSandboxAgentSpec check is kept (mirroring the existing spec.skills pattern) for objects created before the rules shipped and callers that bypass the API server. Adds an envtest-backed CEL test pinning the rules against the shipped CRD YAML. Signed-off-by: younsl --- .../crd/bases/kagent.dev_sandboxagents.yaml | 11 ++ go/api/v1alpha2/agent_spec_validation.go | 4 +- go/api/v1alpha2/sandboxagent_cel_test.go | 155 ++++++++++++++++++ go/api/v1alpha2/sandboxagent_types.go | 2 + .../templates/kagent.dev_sandboxagents.yaml | 11 ++ 5 files changed, 182 insertions(+), 1 deletion(-) create mode 100644 go/api/v1alpha2/sandboxagent_cel_test.go diff --git a/go/api/config/crd/bases/kagent.dev_sandboxagents.yaml b/go/api/config/crd/bases/kagent.dev_sandboxagents.yaml index 8dd8560b8..e0cf48ec8 100644 --- a/go/api/config/crd/bases/kagent.dev_sandboxagents.yaml +++ b/go/api/config/crd/bases/kagent.dev_sandboxagents.yaml @@ -11369,6 +11369,17 @@ spec: x-kubernetes-validations: - message: spec.skills is not supported for sandbox agents rule: '!has(self.skills)' + - message: 'deployment.nodeSelector is not supported for sandbox agents: + substrate schedules actors onto WorkerPool workers, so set the WorkerPool''s + nodeSelector instead' + rule: '!has(self.declarative) || !has(self.declarative.deployment) || + !has(self.declarative.deployment.nodeSelector) || size(self.declarative.deployment.nodeSelector) + == 0' + - message: 'deployment.nodeSelector is not supported for sandbox agents: + substrate schedules actors onto WorkerPool workers, so set the WorkerPool''s + nodeSelector instead' + rule: '!has(self.byo) || !has(self.byo.deployment) || !has(self.byo.deployment.nodeSelector) + || size(self.byo.deployment.nodeSelector) == 0' - message: type must be specified rule: has(self.type) - message: type must be either Declarative or BYO diff --git a/go/api/v1alpha2/agent_spec_validation.go b/go/api/v1alpha2/agent_spec_validation.go index 5e108d9d0..a7c6b328a 100644 --- a/go/api/v1alpha2/agent_spec_validation.go +++ b/go/api/v1alpha2/agent_spec_validation.go @@ -26,7 +26,9 @@ func AgentSpecHasSkills(spec *AgentSpec) bool { // command because substrate copies the container Command verbatim with no image-entrypoint // fallback. A per-agent deployment.nodeSelector is rejected: substrate ActorTemplates carry // no node placement (actors run on WorkerPool workers), so the selector would otherwise be -// silently dropped. +// silently dropped. The skills and nodeSelector checks are also enforced at admission by CEL +// rules on SandboxAgentSpec; this function keeps them effective for objects created before +// those rules shipped and for callers that bypass the API server. func ValidateSubstrateSandboxAgentSpec(agent *SandboxAgent) error { if agent == nil { return nil diff --git a/go/api/v1alpha2/sandboxagent_cel_test.go b/go/api/v1alpha2/sandboxagent_cel_test.go new file mode 100644 index 000000000..cd2316416 --- /dev/null +++ b/go/api/v1alpha2/sandboxagent_cel_test.go @@ -0,0 +1,155 @@ +/* +Copyright 2025. + +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 v1alpha2 + +import ( + "context" + "testing" + + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + ctrl_client "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/envtest" +) + +// TestSandboxAgentCELValidation pins the SandboxAgentSpec CEL rules against a +// real kube-apiserver loaded with the shipped CRDs, so admission rejects +// unsupported configuration instead of the controller discovering it at +// reconcile time. ValidateSubstrateSandboxAgentSpec mirrors these rules in Go +// for objects that predate them. +func TestSandboxAgentCELValidation(t *testing.T) { + testEnv := &envtest.Environment{ + BinaryAssetsDirectory: envtestAssetsDir(t), + CRDDirectoryPaths: []string{crdBasesDir(t)}, + ErrorIfCRDPathMissing: true, + } + cfg, err := testEnv.Start() + require.NoError(t, err) + t.Cleanup(func() { _ = testEnv.Stop() }) + + scheme := runtime.NewScheme() + require.NoError(t, corev1.AddToScheme(scheme)) + require.NoError(t, AddToScheme(scheme)) + cl, err := ctrl_client.New(cfg, ctrl_client.Options{Scheme: scheme}) + require.NoError(t, err) + + ctx := context.Background() + const ns = "sandbox-cel" + require.NoError(t, cl.Create(ctx, &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: ns}})) + + cmd := "/app" + cases := []struct { + name string + build func() ctrl_client.Object + wantReject string // substring in admission error; empty means accept + }{ + { + name: "declarative nodeSelector rejected", + build: func() ctrl_client.Object { + return &SandboxAgent{ + ObjectMeta: metav1.ObjectMeta{Name: "sa-decl-nodeselector", Namespace: ns}, + Spec: SandboxAgentSpec{ + AgentSpec: AgentSpec{ + Type: AgentType_Declarative, + Declarative: &DeclarativeAgentSpec{ + Runtime: DeclarativeRuntime_Go, + Deployment: &DeclarativeDeploymentSpec{ + SharedDeploymentSpec: SharedDeploymentSpec{ + NodeSelector: map[string]string{"kubernetes.io/arch": "amd64"}, + }, + }, + }, + }, + }, + } + }, + wantReject: "deployment.nodeSelector is not supported for sandbox agents", + }, + { + name: "byo nodeSelector rejected", + build: func() ctrl_client.Object { + return &SandboxAgent{ + ObjectMeta: metav1.ObjectMeta{Name: "sa-byo-nodeselector", Namespace: ns}, + Spec: SandboxAgentSpec{ + AgentSpec: AgentSpec{ + Type: AgentType_BYO, + BYO: &BYOAgentSpec{Deployment: &ByoDeploymentSpec{ + Image: "example/agent:latest", + Cmd: &cmd, + SharedDeploymentSpec: SharedDeploymentSpec{ + NodeSelector: map[string]string{"kubernetes.io/arch": "amd64"}, + }, + }}, + }, + }, + } + }, + wantReject: "deployment.nodeSelector is not supported for sandbox agents", + }, + { + name: "declarative empty nodeSelector accepted", + build: func() ctrl_client.Object { + return &SandboxAgent{ + ObjectMeta: metav1.ObjectMeta{Name: "sa-decl-empty-nodeselector", Namespace: ns}, + Spec: SandboxAgentSpec{ + AgentSpec: AgentSpec{ + Type: AgentType_Declarative, + Declarative: &DeclarativeAgentSpec{ + Runtime: DeclarativeRuntime_Go, + Deployment: &DeclarativeDeploymentSpec{ + SharedDeploymentSpec: SharedDeploymentSpec{ + NodeSelector: map[string]string{}, + }, + }, + }, + }, + }, + } + }, + }, + { + name: "declarative without nodeSelector accepted", + build: func() ctrl_client.Object { + return &SandboxAgent{ + ObjectMeta: metav1.ObjectMeta{Name: "sa-decl-no-nodeselector", Namespace: ns}, + Spec: SandboxAgentSpec{ + AgentSpec: AgentSpec{ + Type: AgentType_Declarative, + Declarative: &DeclarativeAgentSpec{ + Runtime: DeclarativeRuntime_Go, + }, + }, + }, + } + }, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + err := cl.Create(ctx, c.build()) + if c.wantReject == "" { + require.NoError(t, err) + return + } + require.Error(t, err) + require.Contains(t, err.Error(), c.wantReject) + }) + } +} diff --git a/go/api/v1alpha2/sandboxagent_types.go b/go/api/v1alpha2/sandboxagent_types.go index b1bceeeb1..fdefbf88b 100644 --- a/go/api/v1alpha2/sandboxagent_types.go +++ b/go/api/v1alpha2/sandboxagent_types.go @@ -38,6 +38,8 @@ type SandboxAgent struct { } // +kubebuilder:validation:XValidation:rule="!has(self.skills)",message="spec.skills is not supported for sandbox agents" +// +kubebuilder:validation:XValidation:rule="!has(self.declarative) || !has(self.declarative.deployment) || !has(self.declarative.deployment.nodeSelector) || size(self.declarative.deployment.nodeSelector) == 0",message="deployment.nodeSelector is not supported for sandbox agents: substrate schedules actors onto WorkerPool workers, so set the WorkerPool's nodeSelector instead" +// +kubebuilder:validation:XValidation:rule="!has(self.byo) || !has(self.byo.deployment) || !has(self.byo.deployment.nodeSelector) || size(self.byo.deployment.nodeSelector) == 0",message="deployment.nodeSelector is not supported for sandbox agents: substrate schedules actors onto WorkerPool workers, so set the WorkerPool's nodeSelector instead" type SandboxAgentSpec struct { AgentSpec `json:",inline"` diff --git a/helm/kagent-crds/templates/kagent.dev_sandboxagents.yaml b/helm/kagent-crds/templates/kagent.dev_sandboxagents.yaml index 8dd8560b8..e0cf48ec8 100644 --- a/helm/kagent-crds/templates/kagent.dev_sandboxagents.yaml +++ b/helm/kagent-crds/templates/kagent.dev_sandboxagents.yaml @@ -11369,6 +11369,17 @@ spec: x-kubernetes-validations: - message: spec.skills is not supported for sandbox agents rule: '!has(self.skills)' + - message: 'deployment.nodeSelector is not supported for sandbox agents: + substrate schedules actors onto WorkerPool workers, so set the WorkerPool''s + nodeSelector instead' + rule: '!has(self.declarative) || !has(self.declarative.deployment) || + !has(self.declarative.deployment.nodeSelector) || size(self.declarative.deployment.nodeSelector) + == 0' + - message: 'deployment.nodeSelector is not supported for sandbox agents: + substrate schedules actors onto WorkerPool workers, so set the WorkerPool''s + nodeSelector instead' + rule: '!has(self.byo) || !has(self.byo.deployment) || !has(self.byo.deployment.nodeSelector) + || size(self.byo.deployment.nodeSelector) == 0' - message: type must be specified rule: has(self.type) - message: type must be either Declarative or BYO From 3bef8d29f9e284dcb219ac4ba01b893e187900df Mon Sep 17 00:00:00 2001 From: younsl Date: Wed, 29 Jul 2026 07:40:56 +0900 Subject: [PATCH 3/3] chore: retrigger CI after transient python-build-standalone download failure Signed-off-by: younsl