Declarative validation for ActorTemplate by shrutiyam-glitch · Pull Request #1303 · agent-substrate/substrate

shrutiyam-glitch · 2026-08-28T22:45:51Z

Continues the declarative-validation (DV) migration from #1215, covering the ActorTemplate resource, its full spec tree, the create path, and read/delete verbs.

Key Changes

DV Tags & Hooks: Added validation tags and custom hooks across the ActorTemplate spec (Metadata, SandboxConfig, SnapshotsConfig, Container, Volume, Resources).
Create Path: Replaced hand-written validation with generated validators in CreateActorTemplate. Custom rules (e.g., on_commit ⊆ on_pause) are now handled via custom hooks.
Read/Delete Verbs: Converted Get, List, and Delete requests to use DV.
State Updates: Refactored atepg metadata setters to update in-place.
Regenerated apitool exemptions (documented 15 previously-exempt fields).

Deliberate Behavior Changes
The gRPC path now strictly enforces CRD rules. Specific tightenings include:

Empty worker_selector is now rejected.
EnvVar.name is strictly required.
Negative values in optional enum fields are rejected (minimum=1).
page_token is capped at 256 characters (matching other list requests).

Deliberately Not Done

No update DV / status tags: ActorTemplates are immutable to clients. The reconciler updates the status directly against the store, so tagging the status subtree or adding update validation isn't necessary right now.
config_name matching sandbox_class: Enforcing this requires calling SandboxConfigLister (ServiceImpl), so it's left as a TODO.

Testing

Added ~70 positive and negative test cases in the validator tables.
Store-backed tests (TestCreateActorTemplate*, TestUpdateActorTemplateMetadata, and the atepg suite) verified against real Postgres via testcontainers.

Tests pass
Appropriate changes to documentation are included in the PR

Julian Gutierrez Oschmann (juli4n) · 2026-08-31T15:15:18Z

  Selector worker_selector = 2;

+  // +k8s:required # at least one container
+  // +k8s:listType=atomic


We should probably set a max number of containers (+k8s:maxItems).

We should also give this repeated field map semantics, and designate the container name field as key. Something like +k8s:listType=map and +k8s:listMapKey=name

Julian Gutierrez Oschmann (juli4n) · 2026-08-31T15:37:58Z

  repeated string args = 4;

+  // +k8s:optional
+  // +k8s:listType=atomic


Set a limit? Also question: do we want env to have map semantics and keyed on EnvVar.name?

Julian Gutierrez Oschmann (juli4n) · 2026-08-31T16:01:42Z

  // name may be any printable ASCII character except '='.
+  //
+  // +k8s:required
+  // TODO: validate the printable-ASCII-except-'=' rule


Any reason to not add this in this PR?

There is another TODO validation that needs to be in the .go files - https://github.com/agent-substrate/substrate/pull/1303/changes#diff-54487cd2c9a8b26ec0ca05b934fd914a6c261a8c068b2b9f00e7f95e2b970c6cR66
Since the PR was already big, thought of doing a follow up.
But I guess for this, I can add in this PR itself. Will add it. Thanks.

Julian Gutierrez Oschmann (juli4n) · 2026-08-31T16:52:04Z

+  //
+  // +k8s:optional
+  // +k8s:maxItems=2
+  // +k8s:listType=atomic


Use listType=map, then I dont think you need k8s:unique.

Zoe Zhao (zoez7) · 2026-08-31T19:47:48Z

  // the default capability set.
+  //
+  // +k8s:optional
  SecurityContext security_context = 8;


Can we also copy over the validation for SecurityContext field here

substrate/pkg/api/v1alpha1/actortemplate_types.go

Line 267 in 9bddf66

// +kubebuilder:validation:XValidation:rule="!self.exists(c, c == 'ALL')",message="add does not accept 'ALL'; name the individual capabilities the container needs"

done. thanks!

I don't think the DV framework will understand those annotations.

added a custom validation method to check this - https://github.com/agent-substrate/substrate/pull/1303/changes#diff-54487cd2c9a8b26ec0ca05b934fd914a6c261a8c068b2b9f00e7f95e2b970c6cR319-R324

Julian Gutierrez Oschmann (juli4n) · 2026-09-01T14:22:40Z

+  // +k8s:maxItems=32
+  // +k8s:listType=map # a volume may be mounted at several paths, so the
+  // path, not the name, is the key
+  // +k8s:listMapKey=mount_path


Shouldn't this be name? That's what defines the ID of the volume, right? We can add a validation that mount paths don't clash, but the key of the map seems to be the name.

k8s VolumeMounts are keyed by mountPath - https://github.com/kubernetes/api/blob/master/core/v1/types.go#L3239-L3243
However, for simplicity, we keying by volume name. But, this incorrectly validates the case when volume names are the different but the mount_path are the same. Added a TODO for the same - 43fc468#diff-f29aa971d0e861ade55215208c2d5e2eaa7d9f743969517826128207b1e4095eR842-R845

Michelle Au (@msau42) could you please confirm this behavior?

cc: Julian Gutierrez Oschmann (@juli4n)

Yeah for now lets keep it simple with one mount per volume. I vaguely recall that in Kubernetes there were some drivers/scenarios where having multiple mount paths on the same volume didn't work.

Julian Gutierrez Oschmann (juli4n) · 2026-09-01T18:51:15Z

This PR is getting large and it adds a significant set of validations. I would say let's merge and iterate. If we find (we will :) ) missing validations, we can add them in follow up PRs.

Tim Hockin (thockin)

I have a few comments I will send as a followup PR. Some questions about API direction for Julian Gutierrez Oschmann (@juli4n) :)

Tim Hockin (thockin) · 2026-09-01T22:57:54Z

+}

+// ValidateCustom_ActorTemplate_SnapshotsConfig mirrors the ActorTemplate
+// CRD's CEL rule: on_commit must be a subset of on_pause. UNSPECIFIED means


Why are we allowing UNSPECIFIED to mean full - why not demand a value?

Tim Hockin (thockin) · 2026-09-01T23:05:50Z

+  // +k8s:listType=map
+  // +k8s:listMapKey=name
+  // +k8s:customValidation # names, quantity parse/positivity, cpu bound
  repeated Limits limits = 1;


This is more about API design than validation, but:

K8s did this because we allow 3rd party resources. It seems possible that we will ALSO allow those (e.g. GPU) -- is "map of string to quantity" what we want?

Alternative #1: map[string]string where keys are validated and values are quantities.

Alternative #2: message Limits { cpu string=1; memory string=2}

Julian Gutierrez Oschmann (@juli4n)

Tim Hockin (thockin) · 2026-09-01T23:24:18Z


 message SnapshotsConfig {
-  // on_pause selects what is captured during pause actor.
+  // on_pause selects what is captured during pause actor. UNSPECIFIED is


Why do we need to allow UNSPECIFIED? We don't really have a compat problem yet.

Tim Hockin (thockin) · 2026-09-01T23:59:33Z

+  // +k8s:optional
+  // +k8s:maxItems=32
+  // +k8s:listType=map # each variable is set at most once
+  // +k8s:listMapKey=name


Julian Gutierrez Oschmann (@juli4n) should we use actual maps for things that are fundamentally map-like ?

Tim Hockin (thockin) · 2026-09-02T00:10:32Z

  HTTPGetAction http_get = 1;

  // timeout_seconds bounds how long to poll http_get before failing the
  // actor start. 0 means the server-applied default (30s).


Julian Gutierrez Oschmann (@juli4n) I asked elsewhere in a different PR - do you think we should keep this semantic or should we reify default values when they are stored? If we do not, then we can never really change a default.

Tim Hockin (thockin) · 2026-09-02T00:12:50Z

  // type discriminates the source union: "DurableDir",
  // "ExternalVolumeTemplate", "Image" or "SystemInfo".
+  //
+  // +k8s:optional


Julian Gutierrez Oschmann (@juli4n) We discussed this one - I thought we agreed to not use discriminators - did I misremember? Optional discrims are weird

shrutiyam-glitch added 13 commits August 27, 2026 17:25

Add dv for ActorTemplate

e7dd16f

Add dv for SandboxConfig

504314e

Add dv for SnapshotsCOnfig and OnResumeConfig

4f4947d

Merge branch 'main' into at-dv

f9eaded

Add DV for Container and EnvVar

86dd4ed

Add DV for ContainerReadyz, HTTPGetAction, and VolumeMount

30c9423

Add DV for Volume and its volume sources

01e5965

Add DV for resources and limits

72028ad

convert CreateActorTemplate to use dv

7a301d3

set template metadata in place instead of reallocating

03cb5f5

convert get/list/delete methods to use dv

929e63f

Update exemptions

719adfe

Merge branch 'main' into at-dv

8796d7c

shrutiyam-glitch requested review from Julian Gutierrez Oschmann (juli4n) and Zoe Zhao (zoez7) August 28, 2026 22:45

Merge branch 'main' into at-dv

db944d8

Julian Gutierrez Oschmann (juli4n) reviewed Aug 31, 2026

View reviewed changes

Comment thread pkg/proto/ateapipb/ateapi.proto

Julian Gutierrez Oschmann (juli4n) reviewed Aug 31, 2026

View reviewed changes

Comment thread pkg/proto/ateapipb/ateapi.proto

Julian Gutierrez Oschmann (juli4n) reviewed Aug 31, 2026

View reviewed changes

Comment thread pkg/proto/ateapipb/ateapi.proto

Julian Gutierrez Oschmann (juli4n) reviewed Aug 31, 2026

View reviewed changes

Comment thread pkg/proto/ateapipb/ateapi.proto

Julian Gutierrez Oschmann (juli4n) reviewed Aug 31, 2026

View reviewed changes

Comment thread pkg/proto/ateapipb/ateapi.proto

Julian Gutierrez Oschmann (juli4n) reviewed Aug 31, 2026

View reviewed changes

Comment thread pkg/proto/ateapipb/ateapi.proto

Julian Gutierrez Oschmann (juli4n) reviewed Aug 31, 2026

View reviewed changes

Comment thread pkg/proto/ateapipb/ateapi.proto

Julian Gutierrez Oschmann (juli4n) reviewed Aug 31, 2026

View reviewed changes

Comment thread pkg/proto/ateapipb/ateapi.proto

Address comments

ed66eb4

Zoe Zhao (zoez7) reviewed Aug 31, 2026

View reviewed changes

shrutiyam-glitch added 2 commits August 31, 2026 14:03

Merge branch 'main' into at-dv

262696b

Add bounds and custom validations

c83e636

shrutiyam-glitch requested a review from Julian Gutierrez Oschmann (juli4n) August 31, 2026 21:56

Julian Gutierrez Oschmann (juli4n) reviewed Sep 1, 2026

View reviewed changes

Comment thread pkg/proto/ateapipb/ateapi.proto Outdated

Julian Gutierrez Oschmann (juli4n) reviewed Sep 1, 2026

View reviewed changes

Comment thread pkg/proto/ateapipb/ateapi.proto Outdated

Julian Gutierrez Oschmann (juli4n) reviewed Sep 1, 2026

View reviewed changes

Comment thread pkg/proto/ateapipb/ateapi.proto Outdated

Julian Gutierrez Oschmann (juli4n) reviewed Sep 1, 2026

View reviewed changes

Comment thread pkg/proto/ateapipb/ateapi.proto Outdated

Julian Gutierrez Oschmann (juli4n) reviewed Sep 1, 2026

View reviewed changes

Comment thread pkg/proto/ateapipb/ateapi.proto Outdated

Julian Gutierrez Oschmann (juli4n) reviewed Sep 1, 2026

View reviewed changes

Comment thread pkg/proto/ateapipb/ateapi.proto Outdated

Julian Gutierrez Oschmann (juli4n) reviewed Sep 1, 2026

View reviewed changes

Comment thread pkg/proto/ateapipb/ateapi.proto Outdated

Julian Gutierrez Oschmann (juli4n) reviewed Sep 1, 2026

View reviewed changes

Comment thread pkg/proto/ateapipb/ateapi.proto Outdated

Julian Gutierrez Oschmann (juli4n) reviewed Sep 1, 2026

View reviewed changes

Comment thread pkg/proto/ateapipb/ateapi.proto

Julian Gutierrez Oschmann (juli4n) reviewed Sep 1, 2026

View reviewed changes

Comment thread pkg/proto/ateapipb/ateapi.proto

Julian Gutierrez Oschmann (juli4n) reviewed Sep 1, 2026

View reviewed changes

Comment thread pkg/proto/ateapipb/ateapi.proto

Merge branch 'main' into at-dv

ce191a5

shrutiyam-glitch force-pushed the at-dv branch from 43fc468 to 688dbf3 Compare September 1, 2026 16:00

Address comments

5863cea

shrutiyam-glitch force-pushed the at-dv branch from 688dbf3 to 5863cea Compare September 1, 2026 16:35

Merge branch 'main' into at-dv

9eb8cd4

Julian Gutierrez Oschmann (juli4n) approved these changes Sep 1, 2026

View reviewed changes

Julian Gutierrez Oschmann (juli4n) merged commit 159de80 into agent-substrate:main Sep 1, 2026
9 checks passed

shrutiyam-glitch mentioned this pull request Sep 1, 2026

atepg(cleanup): update the in-place metadata setter migration #1377

Open

2 tasks

Tim Hockin (thockin) reviewed Sep 2, 2026

View reviewed changes

Uh oh!

Conversation

shrutiyam-glitch commented Aug 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Julian Gutierrez Oschmann (juli4n) commented Sep 1, 2026

Uh oh!

Uh oh!

Tim Hockin (thockin) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

shrutiyam-glitch commented Aug 28, 2026 •

edited

Loading