Extend declarative validation to Actor - #1244
Extend declarative validation to Actor #1244Julian Gutierrez Oschmann (juli4n) merged 24 commits into
Conversation
shrutiyam-glitch
changed the title
Tim Hockin (thockin)
left a comment
Tim Hockin (thockin)
left a comment
There was a problem hiding this comment.
Mostly about testing and bounds checking. Thanks for picking this up!
| // Node VMs that have local snapshots for this actor, while it's PAUSED. | ||
| // | ||
| // +k8s:optional | ||
| // +k8s:listType=atomic |
There was a problem hiding this comment.
Is it OK to have duplicate values in this list? If no, set listType=set
There was a problem hiding this comment.
Every list should come with a +k8s:maxItems That IMMEDIATELY brings this whole aspect of the API into question. Is it allowed to have a snapshot propagate to all machines? I would think yes -- are we really going to update this field to have 1000 or 10000 or 100000 entries?
Julian Gutierrez Oschmann (@juli4n) Benjamin Elder (@BenTheElder) Dmitry Berkovich (@dberkov)
I think, for now, set it to something like 256, but we need to revisit this design
There was a problem hiding this comment.
Every list should come with some sort of per-item validation, too. +k8s:maxLength=128 or something?
There was a problem hiding this comment.
And tests! :)
There was a problem hiding this comment.
set maxItems as 256 with a TODO.
| // TODO: Add DV (optional, need to recurse into this type, immutable?) | ||
| // | ||
| // +k8s:optional | ||
| // +k8s:listType=atomic |
There was a problem hiding this comment.
we probably do not need to declare atomic lists - that is the default
There was a problem hiding this comment.
Actually, let's poke at this one.
I am assuming that this list should not have 2 entries for the same volume_name, right? Michelle Au (@msau42)
If that is right, you can do something like:
// +k8s:listType=map
// +k8s:listMapKey=volume_name
Julian Gutierrez Oschmann (@juli4n) should we venture into list-maps or actually write these things as literal maps? That would be a different PR, of course
There was a problem hiding this comment.
Also needs a maxLength (or maxProperties if map)
There was a problem hiding this comment.
For now, considered actor_volumes as map with a todo.
Adopt volume validation rules and constraints. Co-authored-by: Michelle Au <msau42@users.noreply.github.com>
|
LGTM but it has an actual error flagged Error: cmd/ateapi/internal/controlapi/actor_test.go:100:23: undefined: refOnly |
| // +k8s:required | ||
| // +k8s:format=k8s-short-name | ||
| // +k8s:update=NoModify | ||
| // +k8s:update=NoUnset # set-once; immutable would reject the create |
There was a problem hiding this comment.
this has to be set on creation so immutable is fine
There was a problem hiding this comment.
Right, but the way we are verifying the creation object doesn't allow us to use immutable in the DV.
Related github issue - #1359
Maybe we could handle this separately ? And update the DV all of such fields in a separate PR ?
There was a problem hiding this comment.
The way we use DV today has an issue with immutable in status. We will have to fix that and then fix this. This will have to be "close enough" for now.
| // +k8s:required | ||
| // +k8s:maxLength=253 | ||
| // +k8s:update=NoModify | ||
| // +k8s:update=NoUnset # set-once, like volume_name above |
There was a problem hiding this comment.
This also needs to be set at creation
8a1eccf to
310c222
Compare
310c222 to
cf5ba1d
Compare
|
LGTM |
880456d
into
agent-substrate:main
4 participants
Follow up on issue #1168 and the base PR #1215
In this PR:
Actor read/lifecycle verbs — completes Actor end-to-end
Get/Delete/Suspend/Pause/ResumeActorRequest: dropped the+k8s:opaqueTypefence on the actor ref, added+k8s:required++k8s:subfield(atespace)=+k8s:required. Combined with the recursion intoValidate_ObjectRefthis reproduces exactly whatresources.ValidateObjectRefenforced (ref required, atespace required, name required, both DNS-1123 labels).ListActorsRequest: tagged to matchListAtespacesRequest(atespace optional + short-name format,page_sizeminimum 1,page_tokenmax length 256).validate*Request()functions are now one-line calls to the generated validators. With this, every RPC verb onActorandAtespaceis fully declarative;resources.ValidateObjectRefhas no callers left inactor.go.Note:
ListActorsRequest.page_tokennow capped at 256 chars (parity withListAtespaces).Testing
field.ErrorMatcherwith origin matching).page_tokenboundary cases forListActors;controlapi,functionaltest,actoridentity,store, andatepgsuites all pass;go vetandgofmtclean.