Repository navigation
Resources carry immutable labels, with actor defaults from templates - #3208
Quentin Bisson (QuentinBisson) wants to merge 3 commits into
Conversation
Bowei Du (bowei)
added
area/api
1330288 to
6e4dbc7
Compare
Actors get labels at creation, in Kubernetes label format, with at most 64 entries and 4096 bytes of keys and values together; keys under ate.dev/ are reserved. An ActorTemplate's actor_labels are defaults, and the actor's own value wins on a key conflict. Labels are immutable after creation. Part of agent-substrate#186. Signed-off-by: QuentinBisson <quentin@giantswarm.io>
…ed-key wording Signed-off-by: QuentinBisson <quentin@giantswarm.io>
6e4dbc7 to
8ed0bf1
Compare
| // +k8s:eachVal=+k8s:format=k8s-label-value | ||
| // +k8s:customValidation # reserved keys and total size | ||
| // +k8s:immutable | ||
| map<string, string> labels = 6; |
There was a problem hiding this comment.
I think we should add this as a ResourceMetadata field instead , so other resources can have labels as well.
substrate/api/ateapi/v1/ateapi.proto
Line 412 in 5809059
| // labels identify the Actor for its creator, for example the object of | ||
| // the creating platform it belongs to. At creation they are merged over | ||
| // the actor_labels of the ActorTemplate, the Actor's own value winning on | ||
| // a key conflict; a later change of actor_template does not touch them. |
There was a problem hiding this comment.
🤖 nit 🟢 – Follow docs/api-style-guide.md section 13.2 for proto doc comments (rendered by apitool generate-docs): wrap field names (labels, actor_labels, actor_template), gRPC status codes (INVALID_ARGUMENT), and literal domains (ate.dev) in backticks, here and on ActorTemplate.actor_labels at line 1235.
| // +k8s:eachKey=+k8s:format=k8s-label-key | ||
| // +k8s:eachVal=+k8s:format=k8s-label-value | ||
| // +k8s:customValidation # reserved keys and total size | ||
| // +k8s:immutable |
There was a problem hiding this comment.
🤖 blocking 🔴 – Actor.labels is our first +k8s:immutable map field, and it exposes a bug in resources.DeepEqual (internal/resources/validate.go:119): it falls back to reflect.DeepEqual, which treats nil and map[string]string{} as different.
For example, calling CreateActor in-process with Labels: map[string]string{} (and no template actor_labels) fails with codes.Internal because proto.CloneOf turns {} into nil, and the final ValidateActorUpdate(outActor, inActor) check in cmd/ateapi/internal/controlapi/actor.go:158 rejects nil vs {} as an immutable change.
Update resources.DeepEqual to treat empty maps (and slices) as equal whether nil or empty, matching docs/api-validation.md, and add a map[string]string{} test case.
| // actor_labels are the default labels of Actors created from this | ||
| // template. An Actor's own labels win on a key conflict. Keys in ate.dev | ||
| // and its subdomains are reserved, and keys and values together may not | ||
| // exceed 4096 bytes. |
There was a problem hiding this comment.
4K bytes will significantly increase the size of the actor record in Postgres, can we measure the impact and start with a smaller limit, then expand as we need? (maybe maxProperties = 16) Similar concern as #3617 (comment)
Signed-off-by: QuentinBisson <quentin@giantswarm.io>
Quentin Bisson (QuentinBisson)
changed the title
|
Thank you, your comments are addressed |
Problem
Resources cannot carry their creator's identity, so downstream integrations cannot attribute actors beyond their names. Part of #186, scoped to creation-time labels.
Change
Add immutable
metadata.labelsin Kubernetes label format, bounded to 16 entries and 1024 bytes of keys and values together.ActorTemplate.actor_labelssupplies defaults, actor values win, and the merged set is validated. Updates reject label changes and omission;kubectl ate create actor --labelssets them.Treat nil and empty maps and slices equally during validation. PostgreSQL benchmarks use distinct keys and values: the snapshot fixture used TOAST with 4 KiB of labels and stayed inline at 1 KiB. Selectors and label mutation remain follow-ups.