Repository navigation
atelet: introduce declarative validation for the AteomHerder RPCs - #1997
shrutiyam-glitch wants to merge 25 commits into
Conversation
Move the image, capability, mount path, probe path, env var name, projected path and external volume rules out of controlapi's custom validation hooks into internal/resources, so a component receiving a workload spec can apply the same rules. The hooks now call into them; behavior is unchanged.
TerminateRequest's identity fields get declarative validation and the handler converts the errors with resources.ToGRPCStatusError, replacing the hand-written validateTerminateRequest. The workload spec stays opaque until its tree is tagged; until then the handler keeps checking container names by hand.
Terminate now validates spec.containers declaratively: names, pinned images, env var names, volume mounts, security context capabilities, resource limits and wakeup probes. The shared rules come from internal/resources. Volumes are still left unchecked. The hand-written container-name check is removed because the declarative rules now cover it.
Validation tags cannot be placed on oneof members, so the Volume source and SystemInfoDataSource oneofs become plain message fields with the same field numbers, leaving the wire format unchanged. Callers switch from type switches on the oneof wrapper to checking which field is set. No validation behavior changes here; the union rules come with the Volume and system-info validation.
Terminate now validates spec.volumes declaratively. Volume becomes a union requiring exactly one source, with a required short name. External volume sources require a storage volume ID and check the volume type and context bounds, and image volume sources must be pinned by digest. The field rules are shared with the control plane through internal/resources. The system-info source is left unchecked here.
Terminate now validates system-info volumes declaratively. Each data source is a union of actor metadata and trust bundle. Actor metadata items must name a known field at most once, and trust bundles require a name. Projected paths must be clean relative paths, sharing the control plane's rule through internal/resources, and must be unique across the volume's data sources.
The ActorTemplate CRD rejected more than one actorMetadata entry in a system-info volume. When the volume was mirrored into the substrate API that rule was deferred to the handlers and never added, so a template could project the same identity field twice through separate entries. Both the control plane and atelet now reject every actor_metadata entry after the first.
Run now validates its request with declarative validation, replacing the hand-written check. The actor and template identity fields use the same formats as Terminate, the full workload spec is validated, and the CPU and memory sizes are bounded. The egress gateway address must be host:port, checked by a new shared resources.ValidateHostPort.
Run now requires sandbox_assets and validates it declaratively, mirroring the SandboxConfig CRD: the sandbox class must be gvisor or microvm, the pause image must be pinned by digest, and every asset file needs a URL and a 64-character hex sha256. The hash is checked before atelet builds a cache path from it. The per-class asset names and the entry for the node's architecture are still checked when the assets are resolved.
UploadPausedCheckpoint now validates its request with declarative validation, replacing the hand-written check. The identity fields use the same formats as Run and Terminate, the golden atespace is still rejected, the local snapshot name must be a short name, the destination must be a parseable snapshot URI, and the desired scope must be FULL or DATA.
Checkpoint now validates its request with declarative validation, replacing the hand-written check. Validation tags cannot be placed on oneof members, so the config oneof becomes plain local_config and external_config fields with the same field numbers, leaving the wire format unchanged, and they are validated as a union. A message-level check requires the config that matches the checkpoint type. The identity fields use the same formats as Run and Terminate, the spec is validated in full, the local snapshot name must be a short name, the external snapshot URI must be parseable, and the scope must be FULL or DATA.
Restore now validates its request with declarative validation, replacing the hand-written check. The config oneof becomes plain local_config and external_config fields with the same field numbers, leaving the wire format unchanged, and they are validated as a union. A message-level check requires the config that matches the checkpoint type, which is now shared with Checkpoint, and requires base_config exactly when the scope is DATA_ON_GOLDEN. The identity fields, spec, sizes, egress gateway and sandbox assets use the same rules as Run, and the external and base snapshot URIs must be parseable. With every atelet request now validated, the generator selects them by the Request suffix, as controlapi does. resources.ValidateContainerNames has no callers left and is removed.
f00533e to
34669e0
Compare
Bowei Du (bowei)
added
area/node
area/security
|
Needs rebase |
| message Container { | ||
| // +k8s:required | ||
| // +k8s:format=k8s-short-name | ||
| // +k8s:customValidation # "pause" is reserved for sandbox infrastructure |
There was a problem hiding this comment.
I don't think this is a limitation. We removed this #1496 . Just drop the validation.
dc1ff23 to
771ac07
Compare
771ac07 to
6f69a7a
Compare
| // +k8s:format=k8s-short-name | ||
| string actor_template_name = 6; | ||
|
|
||
| // +k8s:optional |
There was a problem hiding this comment.
🤖 should-fix 🟡 – Mark spec as +k8s:required here (and update the "unset spec is allowed" test case in cmd/atelet/internal/apivalidation/validation_test.go:251). CheckpointRequest.spec and RestoreRequest.spec are both +k8s:required, and TerminateRequest.spec is the only one that intentionally tolerates a nil spec for template-less actors on delete. With +k8s:optional, a RunRequest with spec: nil passes ValidateRunRequest, resets actor dirs, writes the sandbox record, prepares only the pause bundle, and calls ateom.RunWorkload with an empty container list.
| } | ||
|
|
||
| // WorkloadSpec parallels Pod, but with far fewer configurable fields. | ||
| message WorkloadSpec { |
There was a problem hiding this comment.
🤖 should-fix 🟡 – Add +k8s:customValidation on WorkloadSpec to verify that every containers[i].volume_mounts[j].name references a volume declared in volumes (matching ValidateCustom_CreateActorTemplateRequest_ActorTemplate in cmd/ateapi/internal/apivalidation/actor_template.go:61). Currently, an undeclared volume mount passes ValidateRunRequest and ValidateRestoreRequest and is only caught later in buildAteomWorkloadSpec (cmd/atelet/main.go:1604) after resetActorDirs, mountExternalVolumes, and prepareOCIBundles have already mutated node state.
# Conflicts: # cmd/ateapi/internal/controlapi/workload_spec.go # cmd/ateapi/internal/controlapi/workload_spec_test.go # cmd/atelet/internal/apivalidation/validate.go # cmd/atelet/internal/apivalidation/validation_test.go # cmd/atelet/internal/apivalidation/zz_generated.validation.go # cmd/atelet/main.go # cmd/atelet/systeminfovolume.go # cmd/atelet/systeminfovolume_test.go # internal/proto/ateletpb/atelet.pb.go # internal/proto/ateletpb/atelet.proto # internal/resources/validate.go # internal/resources/validate_test.go
dc4c82a to
9002824
Compare
| // | ||
| // +k8s:optional | ||
| // +k8s:minimum=0 | ||
| // +k8s:maximum=999999 # the control plane caps cpu limits strictly below 1000 cores |
There was a problem hiding this comment.
🤖 should-fix 🟡 – ateapi accepts limits that convert outside these bounds. A cpu limit of "999.9995" passes resources.ValidateLimit, but MilliValue() rounds it up to 1000000. A memory limit of "9223372036854775808" passes too, and Value() wraps it to a negative number, which fails minimum=0.
Run and Restore then crash the actor. Terminate validates the same template spec, so the actor can never be deleted. Please bound the converted values in ValidateLimit so these templates are rejected at creation. The same applies to cpu_milli and memory_bytes on RunRequest and RestoreRequest.
| // | ||
| // +k8s:required | ||
| // +k8s:maxLength=261 # a bracketed or 253-character host, ':' and a 5-digit port | ||
| // +k8s:customValidation # host:port shape |
There was a problem hiding this comment.
🤖 should-fix 🟡 – This rejects egress addresses that worked before, such as a trailing-dot FQDN (atenet-egress.ate-system.svc.cluster.local.:443) or a named port (:https). ateapi sends --default-egress-gateway-address unchecked, and the InvalidArgument crashes every actor on Run and Restore. Please validate the flag with resources.ValidateHostPort at ateapi startup, so a bad value fails the rollout instead.
atelet's request validation was stricter than the values ateapi produces, and because an InvalidArgument from atelet crashes the actor, each gap was a crash loop rather than a rejection. Resource limits were checked as Quantities at template creation but travel as integers: MilliValue rounds up, so a cpu limit of 999.9995 arrives as 1000000 millicores, and Value wraps, so a memory limit of 2^63 bytes arrives negative. ValidateLimit now bounds the Quantity by what those integers can hold (999.999 cores, 2^63-1 bytes), comparing Quantities because the conversions themselves wrap for very large values. The default egress gateway address came from an unchecked ateapi flag; ateapi now applies the same host:port rule at startup so a bad value fails the rollout, and the rule accepts a trailing-dot FQDN. CSI volume and publish contexts were capped per entry even though they come from the driver; both are now held to the CSI size limit of 4 KiB in total, the only bound a compliant driver observes. target_ateom_uid is the worker pod UID and is validated as a UUID. sha256 and sandbox_class get explicit length bounds beside their custom rules.
c76f1f3 to
91fe3a1
Compare
| // the node plugin needs to complete the mount. Bounded like volume_context. | ||
| // | ||
| // +k8s:optional | ||
| // +k8s:customValidation # at most 4 KiB in total, the CSI limit for map fields |
There was a problem hiding this comment.
ExternalVolume.volume_context in ateapi.proto now validates this differently:
substrate/pkg/proto/ateapipb/ateapi.proto
Lines 453 to 462 in 9b2ecb9
So we should also update that to use +k8s:customValidation with resources.ValidateCSIMap.
There was a problem hiding this comment.
But, I'm afraid the custom validator limiting this to 4KB is fragile. The limits are defined here: https://github.com/container-storage-interface/spec/blob/v1.12.0/spec.md#size-limits
But, what if this changes in the future? Can we have some room (perhaps 10x - 40KiB just in case)?
2ed9c38 to
8087e3a
Compare
ExternalVolume.volume_context on the public API still capped the driver's map per entry while atelet had moved to a total-size bound, so the same driver response was judged by two different rules depending on the hop. Both now go through resources.ValidateCSIMap. The bound is 40 KiB, ten times the CSI spec's limit for map fields: it exists to keep input finite, not to hold drivers to the spec's exact number, and the headroom means a future revision of that number does not turn compliant drivers away.
8087e3a to
827029c
Compare
4 participants
Fixes part of #1709
This adds declarative validation to atelet's AteomHerder RPCs and to the WorkloadSpec they carry. Malformed requests are now rejected with
InvalidArgumentbefore atelet builds any host path or touches an ateom. The hand-written validators they replace are removed. With this, every atelet RPC request is validated, so the generator now selects them by theRequestsuffix, as controlapi does.cpu_milliis 0..999999 andmemory_bytes≥ 0; egress gateway address, when set, is a host:port; sandbox assets are requiredbase_configis required exactly then; same size, egress and sandbox asset rules as RunWorkloadSpec
pauseis reserved. Images are pinned by digest, the same check ateapi applies when a template is created. Args, env, working dir, probes and mounts are bounded.actor_metadatadata source is allowed (the same rule is added on the ateapi side).Sandbox assets: the sandbox class is gvisor or microvm; the pause image is pinned by digest (stricter than the SandboxConfig CRD's
@check); asset URLs are bounded; hashes are SHA-256.Wire format: the
Volume,SystemInfoDataSource, Checkpointconfigand Restoreconfigoneofs become plain fields with the same field numbers, so the wire format doesn't change. They're validated as unions, because validation tags can't be placed on oneof members.