Repository navigation
atelet: introduce declarative validation for the AteomSupport RPCs - #1903
Julian Gutierrez Oschmann (juli4n) merged 25 commits into
Conversation
Generate DV for the atelet request messages into a new ateletvalidation package (atelet's handlers live in package main, so the generated code cannot sit with them as it does in controlapi). Fields carry their zero-value gates at parity with the hand-written validators; opaqueType defers the deep trees to follow-ups. Not wired to handlers yet.
MintActorCertificate forwarded requests unvalidated and relied on the control plane to reject malformed ones after a round trip; now they fail at the node, after the certificate check so an unauthenticated caller still learns nothing but Unauthenticated. The CSR takes the same 16KiB maxBytes bound as the control-plane request it is forwarded into.
docs/api-validation.md unions are flat optional fields tagged unionMember rather than proto oneofs, which declarative validation cannot see. The conversion is wire-compatible; only the Go wrapper types disappear. The unionMember tags land with the WorkloadSpec validation tags.
Tag the flattened union members with unionMember, so a volume with no source or several sources is now rejected rather than silently tolerated, and bound SystemInfoVolume's data sources with the same paths-unique rule the control plane enforces. Deeper source messages stay opaque for follow-ups; Volume carries a per-type opt-in because no request descends into the WorkloadSpec tree yet.
Bound ExternalVolumeSource and ImageVolumeSource with the same rules the control plane enforces on their template counterparts, and stop marking them opaque under Volume. storage_volume_id is required here, unlike the actor-status field it mirrors: the control plane only sends volumes that finished provisioning.
Bound Capabilities with the control plane's grammar (uppercase, no CAP_ prefix, ALL only in drop, unique within each set) and floor the cgroup limits at zero. Container.name and the containers list are tagged ahead of the rest of Container: the list is keyed by name, and the generator requires presence tags on fields whose types now carry validations.
Mirror the control plane's rules: env names are printable ASCII without '=', set at most once; mount paths are clean absolute Unix paths, unique per container, and must not nest, while a volume may be mounted at multiple paths. The env and volume_mounts lists on Container take their keys and bounds from the template counterparts.
Mirror the template probe's rules: an RFC 3986 path capped at 1024 characters, a port in 1..65535, and a polling timeout of at most an hour. The control plane always sends concrete values, so requiring them here is parity, not a tightening.
The image must be a well-formed reference pinned by digest, matching the template rule it mirrors, and argv strings take the template's guardrail bounds. The container name hook rejects the reserved "pause", the last ValidateContainerNames rule without a declarative equivalent, so the herder handlers can drop that check as they wire up.
ActorMetadataItem and TrustBundleDataSource take the control plane's rules: enum-bounded fields, each identity field projected at most once, and projected paths checked against the shared internal/volumepath rule that atelet also re-applies at host-write time. This removes the last opaque markers inside the WorkloadSpec tree.
Give the request header its formats, open spec so the whole validated WorkloadSpec tree runs on a live RPC, and delete the hand-written validator: every check it made now has a declarative equivalent, including the reserved pause container name. actor_uid tightens from DNS label to UUID, matching the values the control plane mints. The lifecycle test fixture switches to a digest-pinned image, which is what validated specs actually carry.
8859bd0 to
17fa9c6
Compare
atelet checked only that a capacity report was present. It now applies the rules ateapi declares on WorkerResources, so bad reports are rejected at atelet's edge instead of being forwarded. validation-gen cannot follow ateapipb types into atelet, so the field stays opaque and a custom hook calls resources.ValidateWorkerResources, a hand-written copy of those rules. The Resources.limits rule moves to resources.ValidateLimits so both packages share it, and a parity test keeps the copy matching the generated validator. ateapi behavior is unchanged.
Test each nested message through the message that owns it (Volume, Container, WorkloadSpec) so errors are asserted at their real paths and each shared type is covered once. Request tables also check the handler-facing wrapper instead of separate edge tests.
Keep this change to the AteomSupport RPCs: MintActorCertificate, SetWorkerCapacity, and RequestActorSuspend. AteomHerder validation, moves to a folloup.
SetWorkerCapacityRequest reused ateapi's WorkerResources, which made the atelet proto import the public control-plane API and differed from the other atelet methods, which own their messages. Add atelet-owned WorkerResources, Resources and Limits that mirror the control plane's field for field, and convert them when the atelet forwards the request, as it already does for WorkloadSpec. The mirrored messages carry the same validation tags. The limit rules move to resources.ValidateLimits, generic over either Limits type, so the control plane and atelet hooks share one implementation. The SetWorkerCapacity handler tests are folded into one table-driven TestSetWorkerCapacity that checks the returned code and exactly what is forwarded to the control plane.
| type Limit interface { | ||
| GetName() string | ||
| GetQuantity() string | ||
| ProtoReflect() protoreflect.Message |
There was a problem hiding this comment.
This is a bit too complicated. How about we keep the one validator for each api message (under controlapi and apivalidation) packages. If there is something we can reuse (like we do already today with resource.ParseQuantity) we can extract it.
Eventually this will all go away once we can validate these fields with DV tags.
| // proto messages, and reflect.DeepEqual otherwise. This is called by | ||
| // declarative validation's generated code. It is a copy of controlapi's | ||
| // ateDeepEqual: the generated code calls it as a package-level identifier, so | ||
| // each generating package carries its own. |
There was a problem hiding this comment.
This is already defined in cmd/ateapi/internal/controlapi. How about moving it under /internal and then just have a wrapper ateDeepEqual that just calls the one shared?
|
|
||
| func toInvalidArgument(errs field.ErrorList) error { | ||
| if len(errs) == 0 { | ||
| return nil |
There was a problem hiding this comment.
The idiom that we follow in ate-apiserver for errors is that the validator functions return plain field.ErrorList errors, and then the handler does:
if errs := validateFoo(ctx, req); len(errors > 0 {
return nil, toGRPCStatusError(errs)
}
Can we follow the same pattern? That way we can a) reuse toGRPCStatusError (we might need to move it under internal and b) the tests get simplified as they only need to assert using field.ErrorMatcher.
|
|
||
| var createOp = operation.Operation{Type: operation.Create} | ||
|
|
||
| func expectErrors(t *testing.T, want, got field.ErrorList) { |
There was a problem hiding this comment.
Nit: rename to assertValidateErr to be consistent with other validation tests?
|
|
||
| // expectEdge checks the handler-facing wrapper: a valid request passes and an | ||
| // invalid one comes back as InvalidArgument. | ||
| func expectEdge(t *testing.T, err error, wantInvalid bool) { |
There was a problem hiding this comment.
See my comment about making validations return field.ErrorList which would remove the need for this function.
Fixes a part of #1709
This adds declarative validation to atelet's
AteomSupportRPCs. Malformed requests are now rejected at atelet instead of being forwarded to the control plane. Validation runs after authentication.MintActorCertificateRequestActorSuspendSetWorkerCapacitycapacityis required and must follow theWorkerResourcesrules ateapi declares, shared throughinternal/resourcesThe
AteomHerderRPCs andWorkloadSpecget their validation in a follow-up PR.