From 0ae57c9a2f00243027ac40b975cb1bc87785b735 Mon Sep 17 00:00:00 2001 From: shrutiyam-glitch Date: Thu, 27 Aug 2026 01:13:06 -0700 Subject: [PATCH 1/6] add dv to ActorSnapshotTag --- .../internal/controlapi/actor_snapshot.go | 37 ++++- .../controlapi/actor_snapshot_test.go | 60 +++++++- .../controlapi/zz_generated.validation.go | 144 ++++++++++++++++++ cmd/ateapi/internal/store/atepg/atepg.go | 26 +--- cmd/ateapi/internal/store/store.go | 5 +- pkg/proto/ateapipb/ateapi.pb.go | 22 ++- pkg/proto/ateapipb/ateapi.proto | 22 ++- 7 files changed, 279 insertions(+), 37 deletions(-) diff --git a/cmd/ateapi/internal/controlapi/actor_snapshot.go b/cmd/ateapi/internal/controlapi/actor_snapshot.go index 4f0ed83409..4e7691c3c8 100644 --- a/cmd/ateapi/internal/controlapi/actor_snapshot.go +++ b/cmd/ateapi/internal/controlapi/actor_snapshot.go @@ -27,6 +27,7 @@ import ( "google.golang.org/grpc/codes" "google.golang.org/grpc/status" "google.golang.org/protobuf/proto" + "k8s.io/apimachinery/pkg/api/operation" "k8s.io/apimachinery/pkg/util/validation/field" ) @@ -224,9 +225,6 @@ func (s *RPCService) UpdateActorSnapshotTag(ctx context.Context, req *ateapipb.U return nil }) if err != nil { - if errors.Is(err, store.ErrImmutableField) { - return nil, status.Errorf(codes.InvalidArgument, "while updating actor snapshot tag %s/%s: %v", atespace, name, err) - } if errors.Is(err, store.ErrVersionConflict) { return nil, status.Error(codes.Aborted, "concurrent update conflict, please retry") } @@ -245,8 +243,37 @@ func (s *RPCService) UpdateActorSnapshotTag(ctx context.Context, req *ateapipb.U } func (s *ServiceImpl) UpdateActorSnapshotTag(ctx context.Context, tagRef resources.ActorSnapshotTagRef, precondition store.Precondition, mutate func(toUpdate *ateapipb.ActorSnapshotTag) error) (*ateapipb.ActorSnapshotTag, error) { - // TODO: implement this - return s.store.UpdateActorSnapshotTag(ctx, tagRef, precondition, mutate) + return s.store.UpdateActorSnapshotTag(ctx, tagRef, precondition, func(toUpdate *ateapipb.ActorSnapshotTag) error { + // Apply the mutation function to the stored value. + oldVal := proto.CloneOf(toUpdate) + if err := mutate(toUpdate); err != nil { + return err + } + newVal := toUpdate + + // Validate the mutated value before doing any further work. This is + // what enforces the immutable fields, since only the stored tag gives + // declarative validation an old value to compare against. + if errs := validateActorSnapshotTagUpdate(ctx, field.NewPath("actor_snapshot_tag"), newVal, oldVal); len(errs) > 0 { + return toGRPCStatusError(errs) + } + + // Do any further work on the resource. Unlike Actor and Worker there + // is no server-owned field to re-require, so until work lands here a + // second validation pass would repeat the one above verbatim; add it + // (mapping to toGRPCInternalError) together with the first such work. + + return nil + }) +} + +// validateActorSnapshotTagUpdate validates an ActorSnapshotTag against the +// previous stored value. It is what enforces the immutable fields, which need +// an old value to compare against. +func validateActorSnapshotTagUpdate(ctx context.Context, fldPath *field.Path, newVal, oldVal *ateapipb.ActorSnapshotTag) field.ErrorList { + // Call the generated validation. + op := operation.Operation{Type: operation.Update} + return Validate_ActorSnapshotTag(ctx, op, fldPath, newVal, oldVal) } func validateUpdateActorSnapshotTagRequest(req *ateapipb.UpdateActorSnapshotTagRequest) field.ErrorList { diff --git a/cmd/ateapi/internal/controlapi/actor_snapshot_test.go b/cmd/ateapi/internal/controlapi/actor_snapshot_test.go index 9d0cf2821d..0293c428e3 100644 --- a/cmd/ateapi/internal/controlapi/actor_snapshot_test.go +++ b/cmd/ateapi/internal/controlapi/actor_snapshot_test.go @@ -205,7 +205,7 @@ func TestCreateActorSnapshotTag_MissingSnapshotIsNotFound(t *testing.T) { persistence, cleanup := storetest.SetupTestStore(t) t.Cleanup(cleanup) storetest.MustCreateAtespace(t, context.Background(), persistence, "team-a") - s := &RPCService{impl: persistence} + s := &RPCService{impl: newServiceImpl(persistence, nil, nil)} _, err := s.CreateActorSnapshotTag(context.Background(), &ateapipb.CreateActorSnapshotTagRequest{ ActorSnapshotTag: &ateapipb.ActorSnapshotTag{ @@ -357,7 +357,7 @@ func rpcServiceWithActorSnapshotTag(t *testing.T, tag *ateapipb.ActorSnapshotTag if err != nil { t.Fatalf("Failed to CreateActorSnapshotTag: %v", err) } - return &RPCService{impl: persistence}, created + return &RPCService{impl: newServiceImpl(persistence, nil, nil)}, created } // TestUpdateActorSnapshotTag_DeleteRecreateRace checks that an update is not @@ -402,7 +402,7 @@ func TestUpdateActorSnapshotTag_DeleteRecreateRace(t *testing.T) { } }, } - svc := &RPCService{impl: racing} + svc := &RPCService{impl: newServiceImpl(racing, nil, nil)} // The client asserts "only update the tag with uid A". Its version guard is // satisfied by B as well, because re-tagging resets the version to 1: the @@ -463,7 +463,7 @@ func TestUpdateActorSnapshotTag_ConcurrentUpdate(t *testing.T) { } }, } - svc := &RPCService{impl: racing} + svc := &RPCService{impl: newServiceImpl(racing, nil, nil)} originalTag.Scope = ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED _, err = svc.UpdateActorSnapshotTag(ctx, &ateapipb.UpdateActorSnapshotTagRequest{ @@ -566,3 +566,55 @@ func TestValidateCreateActorSnapshotTagRequestUnknownFields(t *testing.T) { validateCreateActorSnapshotTagRequest(&ateapipb.CreateActorSnapshotTagRequest{ActorSnapshotTag: withUnknown(validTag(), 9999)}), field.ErrorList{field.Invalid(field.NewPath("actor_snapshot_tag"), field.OmitValueType{}, "")}) } + +// TestServiceImplUpdateActorSnapshotTag_ImmutableFields pins the +// immutable-snapshot rule at the layer that now owns it: declarative +// validation in ServiceImpl, which every write path shares. The store no +// longer enforces it. +func TestServiceImplUpdateActorSnapshotTag_ImmutableFields(t *testing.T) { + ctx := context.Background() + persistence, cleanup := storetest.SetupTestStore(t) + t.Cleanup(cleanup) + impl := newServiceImpl(persistence, nil, nil) + + snapshot := storetest.MustCreateActorSnapshot(t, ctx, persistence, &ateapipb.ActorSnapshot{ + Metadata: &ateapipb.ResourceMetadata{Atespace: testAtespace, Name: "snap-1"}, + Status: &ateapipb.ActorSnapshotStatus{SnapshotUri: "gs://my-bucket/snap-1"}, + }) + created, err := persistence.CreateActorSnapshotTag(ctx, + resources.ActorSnapshotRef{Atespace: testAtespace, Name: snapshot.GetMetadata().GetName()}, + &ateapipb.ActorSnapshotTag{ + Metadata: &ateapipb.ResourceMetadata{Atespace: testAtespace, Name: "tag-1"}, + Snapshot: &ateapipb.ObjectRef{Atespace: testAtespace, Name: snapshot.GetMetadata().GetName()}, + Scope: ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_ATESPACE, + }) + if err != nil { + t.Fatalf("CreateActorSnapshotTag failed: %v", err) + } + + tagRef := resources.ActorSnapshotTagRef{Atespace: testAtespace, Name: "tag-1"} + for _, tc := range []struct { + name string + mutate func(*ateapipb.ActorSnapshotTag) + }{ + {"snapshot changed", func(tag *ateapipb.ActorSnapshotTag) { tag.Snapshot.Name = "some-other-snapshot" }}, + {"snapshot cleared", func(tag *ateapipb.ActorSnapshotTag) { tag.Snapshot = nil }}, + } { + t.Run(tc.name, func(t *testing.T) { + _, err := impl.UpdateActorSnapshotTag(ctx, tagRef, store.PreconditionFrom(created), func(toUpdate *ateapipb.ActorSnapshotTag) error { + tc.mutate(toUpdate) + return nil + }) + if got := status.Code(err); got != codes.InvalidArgument { + t.Fatalf("%s returned %v (err %v), want %v", tc.name, got, err, codes.InvalidArgument) + } + got, err := persistence.GetActorSnapshotTag(ctx, tagRef) + if err != nil { + t.Fatalf("GetActorSnapshotTag failed: %v", err) + } + if got.GetMetadata().GetVersion() != 1 { + t.Errorf("rejected mutation bumped the version to %d, want 1", got.GetMetadata().GetVersion()) + } + }) + } +} diff --git a/cmd/ateapi/internal/controlapi/zz_generated.validation.go b/cmd/ateapi/internal/controlapi/zz_generated.validation.go index e0f76e32a7..d0e849678d 100644 --- a/cmd/ateapi/internal/controlapi/zz_generated.validation.go +++ b/cmd/ateapi/internal/controlapi/zz_generated.validation.go @@ -451,6 +451,150 @@ func Validate_ActorMetadataItem( return errs } +// Validate_ActorSnapshotTag validates an instance of ActorSnapshotTag according +// to declarative validation rules in the API schema. +func Validate_ActorSnapshotTag( + ctx context.Context, op operation.Operation, fldPath *field.Path, + obj, oldObj *ateapipb.ActorSnapshotTag) (errs field.ErrorList) { + + { // field ateapipb.ActorSnapshotTag.Metadata + fn := func( + fldPath *field.Path, + obj, oldObj *ateapipb.ResourceMetadata, + oldValueCorrelated bool) (errs field.ErrorList) { + // don't revalidate unchanged data + if oldValueCorrelated && op.Type == operation.Update { + if ateDeepEqual(obj, oldObj) { + return nil + } + } + // call field-attached validations + earlyReturn := false + if e := validate.RequiredPointer(ctx, op, fldPath, obj, oldObj).MarkShortCircuit(); len(e) != 0 { + errs = append(errs, e...) + earlyReturn = true + } + if earlyReturn { + return // do not proceed + } + func() { // cohort = "atespace" + earlyReturn := false + if e := validate.Subfield(ctx, op, fldPath, obj, oldObj, "atespace", + func(o *ateapipb.ResourceMetadata) *string { return &o.Atespace }, validate.DirectEqual, validate.RequiredValue).MarkShortCircuit(); len(e) != 0 { + errs = append(errs, e...) + earlyReturn = true + } + if e := validate.Subfield(ctx, op, fldPath, obj, oldObj, "atespace", + func(o *ateapipb.ResourceMetadata) *string { return &o.Atespace }, validate.DirectEqual, validate.Immutable).MarkShortCircuit(); len(e) != 0 { + earlyReturn = true + } + if e := validate.Subfield(ctx, op, fldPath, obj, oldObj, "atespace", + func(o *ateapipb.ResourceMetadata) *string { return &o.Atespace }, validate.DirectEqual, validate.OptionalValue).MarkShortCircuit(); len(e) != 0 { + earlyReturn = true + } + if earlyReturn { + return // do not proceed + } + }() + // call the type's validation function + errs = append(errs, Validate_ResourceMetadata(ctx, op, fldPath, obj, oldObj)...) + return + } + oldVal := safe.Field(oldObj, + func(oldObj *ateapipb.ActorSnapshotTag) *ateapipb.ResourceMetadata { + return oldObj.Metadata + }) + errs = append(errs, fn(fldPath.Child("metadata"), obj.Metadata, oldVal, oldObj != nil)...) + } + + { // field ateapipb.ActorSnapshotTag.Snapshot + fn := func( + fldPath *field.Path, + obj, oldObj *ateapipb.ObjectRef, + oldValueCorrelated bool) (errs field.ErrorList) { + // don't revalidate unchanged data + if oldValueCorrelated && op.Type == operation.Update { + if ateDeepEqual(obj, oldObj) { + return nil + } + } + // call field-attached validations + earlyReturn := false + if e := validate.Immutable(ctx, op, fldPath, obj, oldObj).MarkShortCircuit(); len(e) != 0 { + errs = append(errs, e...) + earlyReturn = true + } + if e := validate.RequiredPointer(ctx, op, fldPath, obj, oldObj).MarkShortCircuit(); len(e) != 0 { + errs = append(errs, e...) + earlyReturn = true + } + if earlyReturn { + return // do not proceed + } + func() { // cohort = "atespace" + earlyReturn := false + if e := validate.Subfield(ctx, op, fldPath, obj, oldObj, "atespace", + func(o *ateapipb.ObjectRef) *string { return &o.Atespace }, validate.DirectEqual, validate.RequiredValue).MarkShortCircuit(); len(e) != 0 { + errs = append(errs, e...) + earlyReturn = true + } + if e := validate.Subfield(ctx, op, fldPath, obj, oldObj, "atespace", + func(o *ateapipb.ObjectRef) *string { return &o.Atespace }, validate.DirectEqual, validate.OptionalValue).MarkShortCircuit(); len(e) != 0 { + earlyReturn = true + } + if earlyReturn { + return // do not proceed + } + }() + // call the type's validation function + errs = append(errs, Validate_ObjectRef(ctx, op, fldPath, obj, oldObj)...) + return + } + oldVal := safe.Field(oldObj, + func(oldObj *ateapipb.ActorSnapshotTag) *ateapipb.ObjectRef { + return oldObj.Snapshot + }) + errs = append(errs, fn(fldPath.Child("snapshot"), obj.Snapshot, oldVal, oldObj != nil)...) + } + + { // field ateapipb.ActorSnapshotTag.Scope + fn := func( + fldPath *field.Path, + obj, oldObj *ateapipb.ActorSnapshotTagScope, + oldValueCorrelated bool) (errs field.ErrorList) { + // don't revalidate unchanged data + if oldValueCorrelated && op.Type == operation.Update { + if obj == oldObj || (obj != nil && oldObj != nil && *obj == *oldObj) { + return nil + } + } + // call field-attached validations + earlyReturn := false + if e := validate.RequiredValue(ctx, op, fldPath, obj, oldObj).MarkShortCircuit(); len(e) != 0 { + errs = append(errs, e...) + earlyReturn = true + } + if earlyReturn { + return // do not proceed + } + if e := validate.Maximum(ctx, op, fldPath, obj, oldObj, 2); len(e) != 0 { + errs = append(errs, e...) + } + if e := validate.Minimum(ctx, op, fldPath, obj, oldObj, 1); len(e) != 0 { + errs = append(errs, e...) + } + return + } + oldVal := safe.Field(oldObj, + func(oldObj *ateapipb.ActorSnapshotTag) *ateapipb.ActorSnapshotTagScope { + return &oldObj.Scope + }) + errs = append(errs, fn(fldPath.Child("scope"), &obj.Scope, oldVal, oldObj != nil)...) + } + + return errs +} + // Validate_ActorStatus validates an instance of ActorStatus according // to declarative validation rules in the API schema. func Validate_ActorStatus( diff --git a/cmd/ateapi/internal/store/atepg/atepg.go b/cmd/ateapi/internal/store/atepg/atepg.go index a841087053..92eba8b514 100644 --- a/cmd/ateapi/internal/store/atepg/atepg.go +++ b/cmd/ateapi/internal/store/atepg/atepg.go @@ -1070,22 +1070,6 @@ func (p *Persistence) CreateActorSnapshotTag(ctx context.Context, snapshotRef re return existing, nil } -func validateUpdateActorSnapshotTagMutation(storedTag, mutatedTag *ateapipb.ActorSnapshotTag) error { - if stored, mutated := storedTag.GetMetadata().GetAtespace(), mutatedTag.GetMetadata().GetAtespace(); stored != mutated { - return fmt.Errorf("metadata.atespace is immutable: mutation changed it from %q to %q", stored, mutated) - } - if stored, mutated := storedTag.GetMetadata().GetName(), mutatedTag.GetMetadata().GetName(); stored != mutated { - return fmt.Errorf("metadata.name is immutable: mutation changed it from %q to %q", stored, mutated) - } - if stored, mutated := storedTag.GetSnapshot().GetAtespace(), mutatedTag.GetSnapshot().GetAtespace(); stored != mutated { - return fmt.Errorf("snapshot.atespace is immutable: mutation changed it from %q to %q", stored, mutated) - } - if stored, mutated := storedTag.GetSnapshot().GetName(), mutatedTag.GetSnapshot().GetName(); stored != mutated { - return fmt.Errorf("snapshot.name is immutable: mutation changed it from %q to %q", stored, mutated) - } - return nil -} - func (p *Persistence) UpdateActorSnapshotTag(ctx context.Context, tagRef resources.ActorSnapshotTagRef, precondition store.Precondition, mutate func(*ateapipb.ActorSnapshotTag) error) (*ateapipb.ActorSnapshotTag, error) { if err := precondition.Validate(); err != nil { return nil, err @@ -1113,16 +1097,16 @@ func (p *Persistence) UpdateActorSnapshotTag(ctx context.Context, tagRef resourc if err := precondition.Check(dbTag.GetMetadata()); err != nil { return nil, err } - tagBeforeMutation := proto.Clone(dbTag).(*ateapipb.ActorSnapshotTag) + // Snapshot the stored metadata before handing the tag to mutate. mutate is + // free to edit anything it is given; immutable fields are the service + // layer's to enforce, via declarative validation. + oldMeta := proto.CloneOf(dbTag.GetMetadata()) if err := mutate(dbTag); err != nil { return nil, err } - if err := validateUpdateActorSnapshotTagMutation(tagBeforeMutation, dbTag); err != nil { - return nil, fmt.Errorf("%w: %w", store.ErrImmutableField, err) - } // Stored metadata is authoritative; discard any metadata edits made by the // closure and derive the next revision from the state this attempt read. - dbTag.Metadata = newUpdateMetadata(tagBeforeMutation.GetMetadata()) + dbTag.Metadata = newUpdateMetadata(oldMeta) updatedBytes, err := proto.Marshal(dbTag) if err != nil { diff --git a/cmd/ateapi/internal/store/store.go b/cmd/ateapi/internal/store/store.go index 194e449de5..fb60ea76d7 100644 --- a/cmd/ateapi/internal/store/store.go +++ b/cmd/ateapi/internal/store/store.go @@ -141,8 +141,9 @@ type Interface interface { // Returns ErrPreconditionRequired if the precondition omits either guard, // ErrNotFound if missing, ErrUIDConflict or ErrVersionConflict if the // precondition no longer holds, ErrVersionConflict if the retry budget is - // exhausted, ErrImmutableField if the mutated tag changed a field that is - // immutable for its lifetime, or the mutate's error verbatim otherwise. + // exhausted, or the mutate's error verbatim otherwise. Immutable fields + // are not checked here; the service layer enforces them via declarative + // validation before the write. UpdateActorSnapshotTag(ctx context.Context, tagRef resources.ActorSnapshotTagRef, precondition Precondition, mutate func(toUpdate *ateapipb.ActorSnapshotTag) error) (*ateapipb.ActorSnapshotTag, error) // Deletes and returns a tag. diff --git a/pkg/proto/ateapipb/ateapi.pb.go b/pkg/proto/ateapipb/ateapi.pb.go index 657060758d..6b34cbf442 100644 --- a/pkg/proto/ateapipb/ateapi.pb.go +++ b/pkg/proto/ateapipb/ateapi.pb.go @@ -1516,10 +1516,23 @@ func (x *ActorSnapshotStatus) GetActorTemplate() *ObjectRef { type ActorSnapshotTag struct { state protoimpl.MessageState `protogen:"open.v1"` // Common resource metadata: name, uid, version, timestamps. - // +k8s:opaqueType + // + // +k8s:required + // +k8s:subfield(atespace)=+k8s:required Metadata *ResourceMetadata `protobuf:"bytes,1,opt,name=metadata,proto3" json:"metadata,omitempty"` - // +k8s:opaqueType - Snapshot *ObjectRef `protobuf:"bytes,2,opt,name=snapshot,proto3" json:"snapshot,omitempty"` + // The ActorSnapshot this tag names. The tag keeps its address: the snapshot + // it points at cannot be changed. + // + // +k8s:required + // +k8s:subfield(atespace)=+k8s:required + // +k8s:immutable + Snapshot *ObjectRef `protobuf:"bytes,2,opt,name=snapshot,proto3" json:"snapshot,omitempty"` + // scope is the only mutable field: UpdateActorSnapshotTag publishes or + // unpublishes the tag by changing it. + // + // +k8s:required + // +k8s:minimum=1 + // +k8s:maximum=2 # keep this in sync with the ActorSnapshotTagScope enum Scope ActorSnapshotTagScope `protobuf:"varint,3,opt,name=scope,proto3,enum=ateapi.ActorSnapshotTagScope" json:"scope,omitempty"` unknownFields protoimpl.UnknownFields sizeCache protoimpl.SizeCache @@ -4317,6 +4330,7 @@ func (x *ListActorSnapshotsResponse) GetNextPageToken() string { type CreateActorSnapshotTagRequest struct { state protoimpl.MessageState `protogen:"open.v1"` // The tag to create. + // +k8s:opaqueType # until this request is converted to declarative validation ActorSnapshotTag *ActorSnapshotTag `protobuf:"bytes,1,opt,name=actor_snapshot_tag,json=actorSnapshotTag,proto3" json:"actor_snapshot_tag,omitempty"` unknownFields protoimpl.UnknownFields sizeCache protoimpl.SizeCache @@ -4368,6 +4382,8 @@ type UpdateActorSnapshotTagRequest struct { // identify which resource to update. // actor_snapshot_tag.metadata.version and actor_snapshot_tag.metadata.uid // are required preconditions + // + // +k8s:opaqueType # updates are handled in 2 steps, do not descend ActorSnapshotTag *ActorSnapshotTag `protobuf:"bytes,1,opt,name=actor_snapshot_tag,json=actorSnapshotTag,proto3" json:"actor_snapshot_tag,omitempty"` unknownFields protoimpl.UnknownFields sizeCache protoimpl.SizeCache diff --git a/pkg/proto/ateapipb/ateapi.proto b/pkg/proto/ateapipb/ateapi.proto index b574e69da5..56f6eab31a 100644 --- a/pkg/proto/ateapipb/ateapi.proto +++ b/pkg/proto/ateapipb/ateapi.proto @@ -438,10 +438,25 @@ message ActorSnapshotStatus { // Its owning Atespace cannot be deleted until the tag is removed. message ActorSnapshotTag { // Common resource metadata: name, uid, version, timestamps. - // +k8s:opaqueType + // + // +k8s:required + // +k8s:subfield(atespace)=+k8s:required ResourceMetadata metadata = 1; - // +k8s:opaqueType + + // The ActorSnapshot this tag names. The tag keeps its address: the snapshot + // it points at cannot be changed. + // + // +k8s:required + // +k8s:subfield(atespace)=+k8s:required + // +k8s:immutable ObjectRef snapshot = 2; + + // scope is the only mutable field: UpdateActorSnapshotTag publishes or + // unpublishes the tag by changing it. + // + // +k8s:required + // +k8s:minimum=1 + // +k8s:maximum=2 # keep this in sync with the ActorSnapshotTagScope enum ActorSnapshotTagScope scope = 3; } @@ -973,6 +988,7 @@ message ListActorSnapshotsResponse { message CreateActorSnapshotTagRequest { // The tag to create. + // +k8s:opaqueType # until this request is converted to declarative validation ActorSnapshotTag actor_snapshot_tag = 1; } @@ -984,6 +1000,8 @@ message UpdateActorSnapshotTagRequest { // identify which resource to update. // actor_snapshot_tag.metadata.version and actor_snapshot_tag.metadata.uid // are required preconditions + // + // +k8s:opaqueType # updates are handled in 2 steps, do not descend ActorSnapshotTag actor_snapshot_tag = 1; } From 17c45a38d938f80d5af3e11b914a88641cd00a02 Mon Sep 17 00:00:00 2001 From: shrutiyam-glitch Date: Thu, 27 Aug 2026 02:28:29 -0700 Subject: [PATCH 2/6] Add dv for ActorSnapshotTag create,update and delete requests --- .../internal/controlapi/actor_snapshot.go | 198 +++----- .../controlapi/actor_snapshot_test.go | 465 +++++++++--------- .../functionaltest/actor_snapshot_test.go | 5 +- .../controlapi/zz_generated.validation.go | 148 ++++++ pkg/proto/ateapipb/ateapi.pb.go | 9 +- pkg/proto/ateapipb/ateapi.proto | 9 +- 6 files changed, 486 insertions(+), 348 deletions(-) diff --git a/cmd/ateapi/internal/controlapi/actor_snapshot.go b/cmd/ateapi/internal/controlapi/actor_snapshot.go index 4e7691c3c8..23206a9ad7 100644 --- a/cmd/ateapi/internal/controlapi/actor_snapshot.go +++ b/cmd/ateapi/internal/controlapi/actor_snapshot.go @@ -18,8 +18,6 @@ import ( "context" "errors" "fmt" - "slices" - "strings" "github.com/agent-substrate/substrate/cmd/ateapi/internal/store" "github.com/agent-substrate/substrate/internal/resources" @@ -28,25 +26,25 @@ import ( "google.golang.org/grpc/status" "google.golang.org/protobuf/proto" "k8s.io/apimachinery/pkg/api/operation" + "k8s.io/apimachinery/pkg/api/validate" "k8s.io/apimachinery/pkg/util/validation/field" ) -// actorSnapshotTagScopes lists the scopes a client may set on an ActorSnapshotTag. -// ACTOR_SNAPSHOT_TAG_SCOPE_UNSPECIFIED is deliberately absent: scope is required -// on the wire, not defaulted. See validateActorSnapshotTagScope. -var actorSnapshotTagScopes = []ateapipb.ActorSnapshotTagScope{ - ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_ATESPACE, - ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED, -} - -// actorSnapshotTagScopeNames names actorSnapshotTagScopes for error messages. -var actorSnapshotTagScopeNames = func() []string { - names := make([]string, len(actorSnapshotTagScopes)) - for i, scope := range actorSnapshotTagScopes { - names[i] = scope.String() +// This exists only because nested subfield tags are not supported yet. +func ValidateCustom_UpdateActorSnapshotTagRequest_ActorSnapshotTag(ctx context.Context, op operation.Operation, fldPath *field.Path, tag, _ *ateapipb.ActorSnapshotTag) field.ErrorList { + if tag == nil || tag.Metadata == nil { + return nil // handled by DV } - return names -}() + + // Updates are validated in 2 steps: first the update request and then the + // resource itself. DV for the request doesn't descend into the resource + // metadata. Once DV supports nested subfield tags, this can be changed to + // something like: + // +k8s:subfield(metadata)=+k8s:subfield(atespace)=+k8s:required + errs := Validate_ResourceMetadata(ctx, op, fldPath.Child("metadata"), tag.Metadata, nil) + errs = append(errs, validate.RequiredValue(ctx, op, fldPath.Child("metadata", "atespace"), &tag.Metadata.Atespace, nil)...) + return errs +} func (s *ServiceImpl) CreateActorSnapshot(ctx context.Context, snapshot *ateapipb.ActorSnapshot) (*ateapipb.ActorSnapshot, error) { // TODO: implement this @@ -150,76 +148,76 @@ func validateListActorSnapshotsRequest(req *ateapipb.ListActorSnapshotsRequest) } func (s *RPCService) CreateActorSnapshotTag(ctx context.Context, req *ateapipb.CreateActorSnapshotTagRequest) (*ateapipb.ActorSnapshotTag, error) { - if errs := validateCreateActorSnapshotTagRequest(req); len(errs) > 0 { + // First scrub any fields that users are not allowed to set. + inTag := req.ActorSnapshotTag + if inTag != nil { // otherwise validation will flag it + scrubResourceMetadataForCreate(inTag.Metadata) + } + + // Validate the request, including the object within it. + if errs := validateCreateActorSnapshotTagRequest(ctx, req); len(errs) > 0 { return nil, toGRPCStatusError(errs) } - ref := req.GetActorSnapshotTag().GetSnapshot() - if req.GetActorSnapshotTag().GetMetadata().GetAtespace() != ref.GetAtespace() { + + // Handle the creation, including validation of the final stored object. + return s.impl.CreateActorSnapshotTag(ctx, resources.ActorSnapshotRefFromObjectRef(inTag.GetSnapshot()), inTag) +} + +func (s *ServiceImpl) CreateActorSnapshotTag(ctx context.Context, snapshotRef resources.ActorSnapshotRef, tag *ateapipb.ActorSnapshotTag) (*ateapipb.ActorSnapshotTag, error) { + // A tag pins its snapshot against garbage collection through the owning + // Atespace, so the two must live in the same one. This is a cross-field + // rule declarative validation cannot express. + atespace, name := tag.GetMetadata().GetAtespace(), tag.GetMetadata().GetName() + if atespace != snapshotRef.Atespace { return nil, status.Error(codes.FailedPrecondition, "ActorSnapshot tags must belong to the snapshot's Atespace") } - tag, err := s.impl.CreateActorSnapshotTag(ctx, resources.ActorSnapshotRefFromObjectRef(ref), req.GetActorSnapshotTag()) - if errors.Is(err, store.ErrNotFound) { - return nil, status.Error(codes.NotFound, "ActorSnapshot not found") - } - if errors.Is(err, store.ErrFailedPrecondition) { - return nil, status.Errorf(codes.FailedPrecondition, "Atespace %s not found", req.GetActorSnapshotTag().GetMetadata().GetAtespace()) - } - if errors.Is(err, store.ErrAlreadyExists) { - return nil, status.Errorf(codes.AlreadyExists, "ActorSnapshot tag %s/%s already exists", req.GetActorSnapshotTag().GetMetadata().GetAtespace(), req.GetActorSnapshotTag().GetMetadata().GetName()) - } + + // Save the data in the storage layer. + stored, err := s.store.CreateActorSnapshotTag(ctx, snapshotRef, tag) if err != nil { + if errors.Is(err, store.ErrNotFound) { + return nil, status.Error(codes.NotFound, "ActorSnapshot not found") + } + if errors.Is(err, store.ErrFailedPrecondition) { + return nil, status.Errorf(codes.FailedPrecondition, "Atespace %s not found", atespace) + } + if errors.Is(err, store.ErrAlreadyExists) { + return nil, status.Errorf(codes.AlreadyExists, "ActorSnapshot tag %s/%s already exists", atespace, name) + } return nil, fmt.Errorf("while tagging actor snapshot: %w", err) } - return tag, nil + return stored, nil } -func (s *ServiceImpl) CreateActorSnapshotTag(ctx context.Context, snapshotRef resources.ActorSnapshotRef, tag *ateapipb.ActorSnapshotTag) (*ateapipb.ActorSnapshotTag, error) { - // TODO: implement this - return s.store.CreateActorSnapshotTag(ctx, snapshotRef, tag) +func validateCreateActorSnapshotTagRequest(ctx context.Context, req *ateapipb.CreateActorSnapshotTagRequest) field.ErrorList { + // Call the generated validation. + op := operation.Operation{Type: operation.Create} + return Validate_CreateActorSnapshotTagRequest(ctx, op, nil, req, nil) } -func validateCreateActorSnapshotTagRequest(req *ateapipb.CreateActorSnapshotTagRequest) field.ErrorList { - var fldPath *field.Path - var errs field.ErrorList - - tag := req.ActorSnapshotTag - tagPath := fldPath.Child("actor_snapshot_tag") - if tag == nil { - errs = append(errs, field.Required(tagPath, "")) - return errs - } - - errs = append(errs, validateNoUnknownFields(tag, tagPath)...) - - errs = append(errs, resources.ValidateObjectRef(&ateapipb.ObjectRef{Atespace: tag.GetMetadata().GetAtespace(), Name: tag.GetMetadata().GetName()}, tagPath.Child("metadata"))...) - - if val, p := tag.Snapshot, tagPath.Child("snapshot"); val == nil { - errs = append(errs, field.Required(p, "")) - } else { - errs = append(errs, resources.ValidateObjectRef(val, p)...) +func (s *RPCService) UpdateActorSnapshotTag(ctx context.Context, req *ateapipb.UpdateActorSnapshotTagRequest) (*ateapipb.ActorSnapshotTag, error) { + // First scrub any fields that users are not allowed to set. + inTag := req.ActorSnapshotTag + if inTag != nil { // otherwise validation will flag it + scrubResourceMetadataForUpdate(inTag.Metadata) } - errs = append(errs, validateActorSnapshotTagScope(tag.GetScope(), tagPath.Child("scope"))...) - - return errs -} - -func (s *RPCService) UpdateActorSnapshotTag(ctx context.Context, req *ateapipb.UpdateActorSnapshotTagRequest) (*ateapipb.ActorSnapshotTag, error) { - if errs := validateUpdateActorSnapshotTagRequest(req); len(errs) > 0 { + // Validate the request. + if errs := validateUpdateActorSnapshotTagRequest(ctx, req); len(errs) > 0 { return nil, toGRPCStatusError(errs) } - in := req.GetActorSnapshotTag() - atespace, name := in.GetMetadata().GetAtespace(), in.GetMetadata().GetName() + + atespace, name := inTag.GetMetadata().GetAtespace(), inTag.GetMetadata().GetName() tagRef := resources.ActorSnapshotTagRef{Atespace: atespace, Name: name} - storedTag, err := s.impl.UpdateActorSnapshotTag(ctx, tagRef, store.PreconditionFrom(in), func(toUpdate *ateapipb.ActorSnapshotTag) error { + storedTag, err := s.impl.UpdateActorSnapshotTag(ctx, tagRef, store.PreconditionFrom(inTag), func(toUpdate *ateapipb.ActorSnapshotTag) error { // Metadata is a server-owned field. metadata := toUpdate.GetMetadata() // Whole-object replace: clear first, so a field the client left unset is // cleared rather than kept from the stored tag. Merge cannot smuggle in // unknown fields because validation already rejected them. proto.Reset(toUpdate) - proto.Merge(toUpdate, in) + proto.Merge(toUpdate, inTag) // Restore metadata from the server. toUpdate.Metadata = metadata return nil @@ -229,7 +227,7 @@ func (s *RPCService) UpdateActorSnapshotTag(ctx context.Context, req *ateapipb.U return nil, status.Error(codes.Aborted, "concurrent update conflict, please retry") } if errors.Is(err, store.ErrUIDConflict) { - return nil, status.Errorf(codes.Aborted, "ActorSnapshot tag %s/%s not found with uid %s", atespace, name, in.GetMetadata().GetUid()) + return nil, status.Errorf(codes.Aborted, "ActorSnapshot tag %s/%s not found with uid %s", atespace, name, inTag.GetMetadata().GetUid()) } if errors.Is(err, store.ErrNotFound) { return nil, status.Errorf(codes.NotFound, "ActorSnapshot tag %s/%s not found", atespace, name) @@ -258,45 +256,22 @@ func (s *ServiceImpl) UpdateActorSnapshotTag(ctx context.Context, tagRef resourc return toGRPCStatusError(errs) } - // Do any further work on the resource. Unlike Actor and Worker there - // is no server-owned field to re-require, so until work lands here a - // second validation pass would repeat the one above verbatim; add it - // (mapping to toGRPCInternalError) together with the first such work. - return nil }) } -// validateActorSnapshotTagUpdate validates an ActorSnapshotTag against the -// previous stored value. It is what enforces the immutable fields, which need -// an old value to compare against. -func validateActorSnapshotTagUpdate(ctx context.Context, fldPath *field.Path, newVal, oldVal *ateapipb.ActorSnapshotTag) field.ErrorList { +func validateUpdateActorSnapshotTagRequest(ctx context.Context, req *ateapipb.UpdateActorSnapshotTagRequest) field.ErrorList { // Call the generated validation. - op := operation.Operation{Type: operation.Update} - return Validate_ActorSnapshotTag(ctx, op, fldPath, newVal, oldVal) -} - -func validateUpdateActorSnapshotTagRequest(req *ateapipb.UpdateActorSnapshotTagRequest) field.ErrorList { - var fldPath *field.Path - var errs field.ErrorList - - tag := req.GetActorSnapshotTag() - tagPath := fldPath.Child("actor_snapshot_tag") - if tag == nil { - return field.ErrorList{field.Required(tagPath, "")} - } - - errs = append(errs, validateNoUnknownFields(tag, tagPath)...) - - errs = append(errs, resources.ValidateUpdateMetadataRef(tag.GetMetadata(), tagPath.Child("metadata"))...) - - errs = append(errs, validateActorSnapshotTagScope(tag.GetScope(), tagPath.Child("scope"))...) - - return errs + // We model this as a create rather than an update because updates assume + // the existence of a "current" value, which we do not have yet. This is + // validating the request itself. The result will be validated later, after + // we have a current value to compare against. + op := operation.Operation{Type: operation.Create} + return Validate_UpdateActorSnapshotTagRequest(ctx, op, nil, req, nil) } func (s *RPCService) DeleteActorSnapshotTag(ctx context.Context, req *ateapipb.DeleteActorSnapshotTagRequest) (*ateapipb.ActorSnapshotTag, error) { - if errs := validateDeleteActorSnapshotTagRequest(req); len(errs) > 0 { + if errs := validateDeleteActorSnapshotTagRequest(ctx, req); len(errs) > 0 { return nil, toGRPCStatusError(errs) } tag, err := s.impl.DeleteActorSnapshotTag(ctx, resources.ActorSnapshotTagRefFromObjectRef(req.GetActorSnapshotTag())) @@ -314,26 +289,17 @@ func (s *ServiceImpl) DeleteActorSnapshotTag(ctx context.Context, tagRef resourc return s.store.DeleteActorSnapshotTag(ctx, tagRef) } -func validateDeleteActorSnapshotTagRequest(req *ateapipb.DeleteActorSnapshotTagRequest) field.ErrorList { - var fldPath *field.Path - var errs field.ErrorList - - if val, fldPath := req.ActorSnapshotTag, fldPath.Child("actor_snapshot_tag"); val == nil { - errs = append(errs, field.Required(fldPath, "")) - } else { - errs = append(errs, resources.ValidateObjectRef(val, fldPath)...) - } - - return errs +func validateDeleteActorSnapshotTagRequest(ctx context.Context, req *ateapipb.DeleteActorSnapshotTagRequest) field.ErrorList { + // Call the generated validation. + op := operation.Operation{Type: operation.Create} + return Validate_DeleteActorSnapshotTagRequest(ctx, op, nil, req, nil) } -// validateActorSnapshotTagScope checks that scope is one a client may set. -func validateActorSnapshotTagScope(scope ateapipb.ActorSnapshotTagScope, p *field.Path) field.ErrorList { - switch { - case scope == ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_UNSPECIFIED: - return field.ErrorList{field.Required(p, "must be one of: "+strings.Join(actorSnapshotTagScopeNames, ", "))} - case !slices.Contains(actorSnapshotTagScopes, scope): - return field.ErrorList{field.NotSupported(p, scope.String(), actorSnapshotTagScopeNames)} - } - return nil +// validateActorSnapshotTagUpdate validates an ActorSnapshotTag against the +// previous stored value. It is what enforces the immutable fields, which need +// an old value to compare against. +func validateActorSnapshotTagUpdate(ctx context.Context, fldPath *field.Path, newVal, oldVal *ateapipb.ActorSnapshotTag) field.ErrorList { + // Call the generated validation. + op := operation.Operation{Type: operation.Update} + return Validate_ActorSnapshotTag(ctx, op, fldPath, newVal, oldVal) } diff --git a/cmd/ateapi/internal/controlapi/actor_snapshot_test.go b/cmd/ateapi/internal/controlapi/actor_snapshot_test.go index 0293c428e3..28f5be1bc1 100644 --- a/cmd/ateapi/internal/controlapi/actor_snapshot_test.go +++ b/cmd/ateapi/internal/controlapi/actor_snapshot_test.go @@ -21,7 +21,6 @@ import ( "github.com/google/go-cmp/cmp" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" - "google.golang.org/protobuf/proto" "google.golang.org/protobuf/testing/protocmp" "k8s.io/apimachinery/pkg/util/validation/field" @@ -32,171 +31,75 @@ import ( ) func TestValidateUpdateActorSnapshotTagRequest(t *testing.T) { - // validUID is a well-formed uid to pass validation. + // This test verifies validation of user input for update. The tag body is + // deliberately not descended into here (updates are validated in two + // steps); only the metadata that addresses the resource is checked. Scope + // and snapshot rules are enforced against the stored tag inside + // ServiceImpl.UpdateActorSnapshotTag, which the RPC-level tests cover. const validUID = "2a5f8c1e-9b3d-4f7a-8e6c-1d0b4a7f2e93" - scopes := []string{ - ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_ATESPACE.String(), - ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED.String(), + validReq := func(mods ...func(md *ateapipb.ResourceMetadata)) *ateapipb.UpdateActorSnapshotTagRequest { + md := &ateapipb.ResourceMetadata{Atespace: "ns1", Name: "tag1", Uid: validUID, Version: 7} + for _, m := range mods { + m(md) + } + return &ateapipb.UpdateActorSnapshotTagRequest{ + ActorSnapshotTag: &ateapipb.ActorSnapshotTag{ + Metadata: md, + Scope: ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED, + }, + } } - // Every case carries a uid and version guard, because an update that carries - // neither is rejected as a blind write before anything else is checked. + tests := []struct { name string req *ateapipb.UpdateActorSnapshotTagRequest wantError field.ErrorList - }{ - { - name: "valid", - req: &ateapipb.UpdateActorSnapshotTagRequest{ - ActorSnapshotTag: &ateapipb.ActorSnapshotTag{ - Metadata: &ateapipb.ResourceMetadata{Atespace: "ns1", Name: "tag1", Uid: validUID, Version: 7}, - Scope: ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED, - }, - }, - wantError: nil, - }, - { - name: "missing tag", - req: &ateapipb.UpdateActorSnapshotTagRequest{}, - wantError: field.ErrorList{field.Required(field.NewPath("actor_snapshot_tag"), "")}, - }, - { - name: "missing tag.metadata.atespace", - req: &ateapipb.UpdateActorSnapshotTagRequest{ - ActorSnapshotTag: &ateapipb.ActorSnapshotTag{ - Metadata: &ateapipb.ResourceMetadata{Name: "tag1", Uid: validUID, Version: 7}, - Scope: ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED, - }, - }, - wantError: field.ErrorList{field.Required(field.NewPath("actor_snapshot_tag", "metadata", "atespace"), "")}, - }, - { - name: "invalid tag.metadata.atespace", - req: &ateapipb.UpdateActorSnapshotTagRequest{ - ActorSnapshotTag: &ateapipb.ActorSnapshotTag{ - Metadata: &ateapipb.ResourceMetadata{Atespace: "NS1", Name: "tag1", Uid: validUID, Version: 7}, - Scope: ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED, - }, - }, - wantError: field.ErrorList{field.Invalid(field.NewPath("actor_snapshot_tag", "metadata", "atespace"), "NS1", "")}, - }, - { - name: "missing tag.metadata.name", - req: &ateapipb.UpdateActorSnapshotTagRequest{ - ActorSnapshotTag: &ateapipb.ActorSnapshotTag{ - Metadata: &ateapipb.ResourceMetadata{Atespace: "ns1", Uid: validUID, Version: 7}, - Scope: ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED, - }, - }, - wantError: field.ErrorList{field.Required(field.NewPath("actor_snapshot_tag", "metadata", "name"), "")}, - }, - { - name: "invalid tag.metadata.name", - req: &ateapipb.UpdateActorSnapshotTagRequest{ - ActorSnapshotTag: &ateapipb.ActorSnapshotTag{ - Metadata: &ateapipb.ResourceMetadata{Atespace: "ns1", Name: "TAG1", Uid: validUID, Version: 7}, - Scope: ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED, - }, - }, - wantError: field.ErrorList{field.Invalid(field.NewPath("actor_snapshot_tag", "metadata", "name"), "TAG1", "")}, - }, - { - name: "missing tag.metadata.uid precondition", - req: &ateapipb.UpdateActorSnapshotTagRequest{ - ActorSnapshotTag: &ateapipb.ActorSnapshotTag{ - Metadata: &ateapipb.ResourceMetadata{Atespace: "ns1", Name: "tag1", Version: 7}, - Scope: ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED, - }, - }, - wantError: field.ErrorList{field.Required(field.NewPath("actor_snapshot_tag", "metadata", "uid"), "")}, - }, - { - name: "invalid tag.metadata.uid precondition", - req: &ateapipb.UpdateActorSnapshotTagRequest{ - ActorSnapshotTag: &ateapipb.ActorSnapshotTag{ - Metadata: &ateapipb.ResourceMetadata{Atespace: "ns1", Name: "tag1", Uid: "not-a-uuid", Version: 7}, - Scope: ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED, - }, - }, - wantError: field.ErrorList{field.Invalid(field.NewPath("actor_snapshot_tag", "metadata", "uid"), "not-a-uuid", "")}, - }, - { - name: "missing tag.metadata.version precondition", - req: &ateapipb.UpdateActorSnapshotTagRequest{ - ActorSnapshotTag: &ateapipb.ActorSnapshotTag{ - Metadata: &ateapipb.ResourceMetadata{Atespace: "ns1", Name: "tag1", Uid: validUID}, - Scope: ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED, - }, - }, - wantError: field.ErrorList{field.Required(field.NewPath("actor_snapshot_tag", "metadata", "version"), "")}, - }, - { - name: "negative tag.metadata.version precondition", - req: &ateapipb.UpdateActorSnapshotTagRequest{ - ActorSnapshotTag: &ateapipb.ActorSnapshotTag{ - Metadata: &ateapipb.ResourceMetadata{Atespace: "ns1", Name: "tag1", Uid: validUID, Version: -1}, - Scope: ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED, - }, - }, - wantError: field.ErrorList{field.Invalid(field.NewPath("actor_snapshot_tag", "metadata", "version"), int64(-1), "")}, - }, - { - // A blind write: the caller never read the tag it is updating. - name: "guards on neither uid nor version", - req: &ateapipb.UpdateActorSnapshotTagRequest{ - ActorSnapshotTag: &ateapipb.ActorSnapshotTag{ - Metadata: &ateapipb.ResourceMetadata{Atespace: "ns1", Name: "tag1"}, - Scope: ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED, - }, - }, - wantError: field.ErrorList{ - field.Required(field.NewPath("actor_snapshot_tag", "metadata", "uid"), ""), - field.Required(field.NewPath("actor_snapshot_tag", "metadata", "version"), ""), - }, - }, - { - name: "unset tag.scope", - req: &ateapipb.UpdateActorSnapshotTagRequest{ - ActorSnapshotTag: &ateapipb.ActorSnapshotTag{ - Metadata: &ateapipb.ResourceMetadata{Atespace: "ns1", Name: "tag1", Uid: validUID, Version: 7}, - }, - }, - wantError: field.ErrorList{field.Required(field.NewPath("actor_snapshot_tag", "scope"), "")}, - }, - { - name: "explicit tag.scope UNSPECIFIED", - req: &ateapipb.UpdateActorSnapshotTagRequest{ - ActorSnapshotTag: &ateapipb.ActorSnapshotTag{ - Metadata: &ateapipb.ResourceMetadata{Atespace: "ns1", Name: "tag1", Uid: validUID, Version: 7}, - Scope: ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_UNSPECIFIED, - }, - }, - wantError: field.ErrorList{field.Required(field.NewPath("actor_snapshot_tag", "scope"), "")}, - }, - { - name: "tag.scope ATESPACE explicitly unpublishes", - req: &ateapipb.UpdateActorSnapshotTagRequest{ - ActorSnapshotTag: &ateapipb.ActorSnapshotTag{ - Metadata: &ateapipb.ResourceMetadata{Atespace: "ns1", Name: "tag1", Uid: validUID, Version: 7}, - Scope: ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_ATESPACE, - }, - }, - wantError: nil, - }, - { - name: "tag.scope outside the enum", - req: &ateapipb.UpdateActorSnapshotTagRequest{ - ActorSnapshotTag: &ateapipb.ActorSnapshotTag{ - Metadata: &ateapipb.ResourceMetadata{Atespace: "ns1", Name: "tag1", Uid: validUID, Version: 7}, - Scope: ateapipb.ActorSnapshotTagScope(7), - }, - }, - wantError: field.ErrorList{field.NotSupported(field.NewPath("actor_snapshot_tag", "scope"), "7", scopes)}, - }, - } + }{{ + "valid", + validReq(), + nil, + }, { + // uid and version are preconditions the store requires; the request + // validation deliberately leaves their presence to the store. + "missing uid and version pass request validation", + validReq(func(md *ateapipb.ResourceMetadata) { md.Uid = ""; md.Version = 0 }), + nil, + }, { + "missing tag", + &ateapipb.UpdateActorSnapshotTagRequest{}, + field.ErrorList{field.Required(field.NewPath("actor_snapshot_tag"), "")}, + }, { + "missing tag.metadata", + &ateapipb.UpdateActorSnapshotTagRequest{ActorSnapshotTag: &ateapipb.ActorSnapshotTag{}}, + field.ErrorList{field.Required(field.NewPath("actor_snapshot_tag", "metadata"), "")}, + }, { + "missing tag.metadata.atespace", + validReq(func(md *ateapipb.ResourceMetadata) { md.Atespace = "" }), + field.ErrorList{field.Required(field.NewPath("actor_snapshot_tag", "metadata", "atespace"), "")}, + }, { + "invalid tag.metadata.atespace", + validReq(func(md *ateapipb.ResourceMetadata) { md.Atespace = "NS1" }), + field.ErrorList{field.Invalid(field.NewPath("actor_snapshot_tag", "metadata", "atespace"), nil, "").WithOrigin("format=k8s-short-name")}, + }, { + "missing tag.metadata.name", + validReq(func(md *ateapipb.ResourceMetadata) { md.Name = "" }), + field.ErrorList{field.Required(field.NewPath("actor_snapshot_tag", "metadata", "name"), "")}, + }, { + "invalid tag.metadata.name", + validReq(func(md *ateapipb.ResourceMetadata) { md.Name = "TAG1" }), + field.ErrorList{field.Invalid(field.NewPath("actor_snapshot_tag", "metadata", "name"), nil, "").WithOrigin("format=k8s-short-name")}, + }, { + "invalid tag.metadata.uid precondition", + validReq(func(md *ateapipb.ResourceMetadata) { md.Uid = "not-a-uuid" }), + field.ErrorList{field.Invalid(field.NewPath("actor_snapshot_tag", "metadata", "uid"), nil, "").WithOrigin("format=k8s-uuid")}, + }, { + "negative tag.metadata.version precondition", + validReq(func(md *ateapipb.ResourceMetadata) { md.Version = -1 }), + field.ErrorList{field.Invalid(field.NewPath("actor_snapshot_tag", "metadata", "version"), nil, "").WithOrigin("minimum")}, + }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - assertValidateErr(t, validateUpdateActorSnapshotTagRequest(tt.req), tt.wantError) + assertValidateErr(t, validateUpdateActorSnapshotTagRequest(context.Background(), tt.req), tt.wantError) }) } } @@ -487,84 +390,115 @@ func TestUpdateActorSnapshotTag_ConcurrentUpdate(t *testing.T) { } } -// TestUpdateActorSnapshotTag_RejectsUnknownFields checks that an update carrying -// a field this binary has no descriptor for is refused. -// Update replaces the whole object, so a field the server cannot see would -// otherwise be persisted unexamined. -func TestUpdateActorSnapshotTag_RejectsUnknownFields(t *testing.T) { - ctx := context.Background() +func TestValidateCreateActorSnapshotTagRequest(t *testing.T) { + // This test verifies validation of user input for creation. + validReq := func(mods ...func(tag *ateapipb.ActorSnapshotTag)) *ateapipb.CreateActorSnapshotTagRequest { + tag := &ateapipb.ActorSnapshotTag{ + Metadata: &ateapipb.ResourceMetadata{Atespace: "team-a", Name: "tag-1"}, + Snapshot: &ateapipb.ObjectRef{Atespace: "team-a", Name: "snap-1"}, + Scope: ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_ATESPACE, + } + for _, m := range mods { + m(tag) + } + return &ateapipb.CreateActorSnapshotTagRequest{ActorSnapshotTag: tag} + } tests := []struct { name string - // injectUnknownField attaches the unknown field somewhere in the request's tag. - injectUnknownField func(*ateapipb.ActorSnapshotTag) - // wantPath is where the resulting error points. - wantPath *field.Path - }{ - { - name: "at the top level", - injectUnknownField: func(tag *ateapipb.ActorSnapshotTag) { tag.ProtoReflect().SetUnknown(unknownField(9999)) }, - wantPath: field.NewPath("actor_snapshot_tag"), - }, - { - name: "nested in metadata", - injectUnknownField: func(tag *ateapipb.ActorSnapshotTag) { tag.Metadata.ProtoReflect().SetUnknown(unknownField(9999)) }, - wantPath: field.NewPath("actor_snapshot_tag", "metadata"), - }, - { - name: "nested in snapshot", - injectUnknownField: func(tag *ateapipb.ActorSnapshotTag) { tag.Snapshot.ProtoReflect().SetUnknown(unknownField(9999)) }, - wantPath: field.NewPath("actor_snapshot_tag", "snapshot"), - }, - } + req *ateapipb.CreateActorSnapshotTagRequest + want field.ErrorList + }{{ + "valid", + validReq(), + nil, + }, { + "missing tag", + &ateapipb.CreateActorSnapshotTagRequest{}, + field.ErrorList{field.Required(field.NewPath("actor_snapshot_tag"), "")}, + }, { + "missing metadata", + validReq(func(tag *ateapipb.ActorSnapshotTag) { tag.Metadata = nil }), + field.ErrorList{field.Required(field.NewPath("actor_snapshot_tag", "metadata"), "")}, + }, { + "missing metadata.atespace", + validReq(func(tag *ateapipb.ActorSnapshotTag) { tag.Metadata.Atespace = "" }), + field.ErrorList{field.Required(field.NewPath("actor_snapshot_tag", "metadata", "atespace"), "")}, + }, { + "invalid metadata.name", + validReq(func(tag *ateapipb.ActorSnapshotTag) { tag.Metadata.Name = "TAG-1" }), + field.ErrorList{field.Invalid(field.NewPath("actor_snapshot_tag", "metadata", "name"), nil, "").WithOrigin("format=k8s-short-name")}, + }, { + "missing snapshot", + validReq(func(tag *ateapipb.ActorSnapshotTag) { tag.Snapshot = nil }), + field.ErrorList{field.Required(field.NewPath("actor_snapshot_tag", "snapshot"), "")}, + }, { + "missing snapshot.atespace", + validReq(func(tag *ateapipb.ActorSnapshotTag) { tag.Snapshot.Atespace = "" }), + field.ErrorList{field.Required(field.NewPath("actor_snapshot_tag", "snapshot", "atespace"), "")}, + }, { + "invalid snapshot.name", + validReq(func(tag *ateapipb.ActorSnapshotTag) { tag.Snapshot.Name = "SNAP 1" }), + field.ErrorList{field.Invalid(field.NewPath("actor_snapshot_tag", "snapshot", "name"), nil, "").WithOrigin("format=k8s-short-name")}, + }, { + "unset scope", + validReq(func(tag *ateapipb.ActorSnapshotTag) { + tag.Scope = ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_UNSPECIFIED + }), + field.ErrorList{field.Required(field.NewPath("actor_snapshot_tag", "scope"), "")}, + }, { + "scope outside the enum", + validReq(func(tag *ateapipb.ActorSnapshotTag) { tag.Scope = ateapipb.ActorSnapshotTagScope(7) }), + field.ErrorList{field.Invalid(field.NewPath("actor_snapshot_tag", "scope"), nil, "").WithOrigin("maximum")}, + }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - svc, stored := rpcServiceWithActorSnapshotTag(t, &ateapipb.ActorSnapshotTag{ - Metadata: &ateapipb.ResourceMetadata{Atespace: testAtespace, Name: "tag-1"}, - Scope: ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_ATESPACE, - }) - - in := proto.Clone(stored).(*ateapipb.ActorSnapshotTag) - tt.injectUnknownField(in) - - _, err := svc.UpdateActorSnapshotTag(ctx, &ateapipb.UpdateActorSnapshotTagRequest{ActorSnapshotTag: in}) - wantErr := toGRPCStatusError(field.ErrorList{ - field.Invalid(tt.wantPath, field.OmitValueType{}, ""), - }) - if got, want := status.Code(err), status.Code(wantErr); got != want { - t.Fatalf("UpdateActorSnapshotTag() error code = %v, want %v (error: %v)", got, want, err) - } - if got, want := status.Convert(err).Message(), status.Convert(wantErr).Message(); got != want { - t.Errorf("UpdateActorSnapshotTag() error message = %q, want %q", got, want) - } - - // The rejection happens before the store is touched, so the tag is - // left exactly as it was. - after, err := svc.GetActorSnapshotTag(ctx, &ateapipb.GetActorSnapshotTagRequest{ - ActorSnapshotTag: &ateapipb.ObjectRef{Atespace: testAtespace, Name: "tag-1"}, - }) - if err != nil { - t.Fatalf("GetActorSnapshotTag() error = %v", err) - } - if diff := cmp.Diff(stored, after, protocmp.Transform()); diff != "" { - t.Errorf("tag changed despite the rejection (-want +got):\n%s", diff) - } + assertValidateErr(t, validateCreateActorSnapshotTagRequest(context.Background(), tt.req), tt.want) }) } } -func TestValidateCreateActorSnapshotTagRequestUnknownFields(t *testing.T) { - validTag := func() *ateapipb.ActorSnapshotTag { - return &ateapipb.ActorSnapshotTag{ - Metadata: &ateapipb.ResourceMetadata{Atespace: "team-a", Name: "tag-1"}, - Snapshot: &ateapipb.ObjectRef{Atespace: "team-a", Name: "snap-1"}, - Scope: ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_ATESPACE, - } +func TestValidateActorSnapshotRefRequests(t *testing.T) { + // Get/Delete requests carry a single atespaced ref; all three share the + // same generated rules, so one table drives them all. + type refCase struct { + name string + ref *ateapipb.ObjectRef + want func(root string) field.ErrorList + } + cases := []refCase{{ + "valid", + &ateapipb.ObjectRef{Atespace: "team-a", Name: "obj-1"}, + func(string) field.ErrorList { return nil }, + }, { + "missing ref", + nil, + func(root string) field.ErrorList { return field.ErrorList{field.Required(field.NewPath(root), "")} }, + }, { + "missing atespace", + &ateapipb.ObjectRef{Name: "obj-1"}, + func(root string) field.ErrorList { + return field.ErrorList{field.Required(field.NewPath(root, "atespace"), "")} + }, + }, { + "missing name", + &ateapipb.ObjectRef{Atespace: "team-a"}, + func(root string) field.ErrorList { + return field.ErrorList{field.Required(field.NewPath(root, "name"), "")} + }, + }, { + "invalid name", + &ateapipb.ObjectRef{Atespace: "team-a", Name: "OBJ 1"}, + func(root string) field.ErrorList { + return field.ErrorList{field.Invalid(field.NewPath(root, "name"), nil, "").WithOrigin("format=k8s-short-name")} + }, + }} + for _, tc := range cases { + t.Run("DeleteActorSnapshotTag/"+tc.name, func(t *testing.T) { + got := validateDeleteActorSnapshotTagRequest(context.Background(), &ateapipb.DeleteActorSnapshotTagRequest{ActorSnapshotTag: tc.ref}) + assertValidateErr(t, got, tc.want("actor_snapshot_tag")) + }) } - assertValidateErr(t, validateCreateActorSnapshotTagRequest(&ateapipb.CreateActorSnapshotTagRequest{ActorSnapshotTag: validTag()}), nil) - assertValidateErr(t, - validateCreateActorSnapshotTagRequest(&ateapipb.CreateActorSnapshotTagRequest{ActorSnapshotTag: withUnknown(validTag(), 9999)}), - field.ErrorList{field.Invalid(field.NewPath("actor_snapshot_tag"), field.OmitValueType{}, "")}) } // TestServiceImplUpdateActorSnapshotTag_ImmutableFields pins the @@ -618,3 +552,82 @@ func TestServiceImplUpdateActorSnapshotTag_ImmutableFields(t *testing.T) { }) } } + +// A blind write: the caller never read the tag it is updating. Presence of +// the uid and version guards is the store's to enforce, so the rejection +// arrives from below the request validation, with the same code as before. +func TestUpdateActorSnapshotTag_BlindWriteRejected(t *testing.T) { + ctx := context.Background() + svc, stored := rpcServiceWithActorSnapshotTag(t, &ateapipb.ActorSnapshotTag{ + Metadata: &ateapipb.ResourceMetadata{Atespace: testAtespace, Name: "tag-1"}, + Scope: ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_ATESPACE, + }) + + req := &ateapipb.UpdateActorSnapshotTagRequest{ + ActorSnapshotTag: &ateapipb.ActorSnapshotTag{ + Metadata: &ateapipb.ResourceMetadata{Atespace: testAtespace, Name: "tag-1"}, + Snapshot: stored.GetSnapshot(), + Scope: ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED, + }, + } + _, err := svc.UpdateActorSnapshotTag(ctx, req) + if got := status.Code(err); got != codes.InvalidArgument { + t.Errorf("UpdateActorSnapshotTag() code = %v (err %v), want %v", got, err, codes.InvalidArgument) + } +} + +// TestCreateActorSnapshotTag_RejectsCrossAtespace pins the cross-field rule +// that declarative validation cannot express: a tag must be created in its +// snapshot's Atespace. ServiceImpl rejects it before the store is touched, so +// no store is needed here. +func TestCreateActorSnapshotTag_RejectsCrossAtespace(t *testing.T) { + svc := &RPCService{impl: newServiceImpl(nil, nil, nil)} + + _, err := svc.CreateActorSnapshotTag(context.Background(), &ateapipb.CreateActorSnapshotTagRequest{ + ActorSnapshotTag: &ateapipb.ActorSnapshotTag{ + Metadata: &ateapipb.ResourceMetadata{Atespace: "team-a", Name: "tag-1"}, + Snapshot: &ateapipb.ObjectRef{Atespace: "team-b", Name: "snap-1"}, + Scope: ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_ATESPACE, + }, + }) + if got := status.Code(err); got != codes.FailedPrecondition { + t.Fatalf("CreateActorSnapshotTag() code = %v (err %v), want %v", got, err, codes.FailedPrecondition) + } +} + +// Server-assigned metadata carried on a create request is scrubbed rather than +// rejected: the fields are documented as ignored on input, so even garbage in +// them must not fail validation. +func TestCreateActorSnapshotTag_IgnoresRequestMetadataServerFields(t *testing.T) { + ctx := context.Background() + persistence, cleanup := storetest.SetupTestStore(t) + t.Cleanup(cleanup) + svc := &RPCService{impl: newServiceImpl(persistence, nil, nil)} + + snapshot := storetest.MustCreateActorSnapshot(t, ctx, persistence, &ateapipb.ActorSnapshot{ + Metadata: &ateapipb.ResourceMetadata{Atespace: testAtespace, Name: "snap-1"}, + Status: &ateapipb.ActorSnapshotStatus{SnapshotUri: "gs://my-bucket/snap-1"}, + }) + + got, err := svc.CreateActorSnapshotTag(ctx, &ateapipb.CreateActorSnapshotTagRequest{ + ActorSnapshotTag: &ateapipb.ActorSnapshotTag{ + Metadata: &ateapipb.ResourceMetadata{ + Atespace: testAtespace, + Name: "tag-1", + Uid: "not-a-uuid", + Version: -5, + }, + Snapshot: &ateapipb.ObjectRef{Atespace: testAtespace, Name: snapshot.GetMetadata().GetName()}, + Scope: ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_ATESPACE, + }, + }) + if err != nil { + t.Fatalf("CreateActorSnapshotTag() failed: %v", err) + } + if uid := got.GetMetadata().GetUid(); uid == "" || uid == "not-a-uuid" { + t.Errorf("created tag uid = %q, want a server-assigned uid", uid) + } + if got.GetMetadata().GetVersion() != 1 { + t.Errorf("created tag version = %d, want 1", got.GetMetadata().GetVersion()) + } +} diff --git a/cmd/ateapi/internal/controlapi/functionaltest/actor_snapshot_test.go b/cmd/ateapi/internal/controlapi/functionaltest/actor_snapshot_test.go index a7512bf6f3..33878630d4 100644 --- a/cmd/ateapi/internal/controlapi/functionaltest/actor_snapshot_test.go +++ b/cmd/ateapi/internal/controlapi/functionaltest/actor_snapshot_test.go @@ -82,9 +82,10 @@ func TestUpdateActorSnapshotTag_Preconditions(t *testing.T) { if uid == staleUID { t.Fatalf("recreated tag reused uid %s, want a fresh one", uid) } - // No preconditions + // No preconditions. Presence of the guards is the store's to enforce, so + // the rejection carries its wording rather than field paths. _, err := update(&ateapipb.ResourceMetadata{}, ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED) - assertGrpcError(t, err, codes.InvalidArgument, "[actor_snapshot_tag.metadata.uid: Required value, actor_snapshot_tag.metadata.version: Required value]") + assertGrpcError(t, err, codes.InvalidArgument, fmt.Sprintf("while updating actor snapshot tag %s/%s: persistence: precondition required: uid", testAtespace, tagName)) // The uid from the deleted lifecycle must be rejected, even though the // atespace/name it was observed under still resolves and the version it diff --git a/cmd/ateapi/internal/controlapi/zz_generated.validation.go b/cmd/ateapi/internal/controlapi/zz_generated.validation.go index d0e849678d..207a0747e4 100644 --- a/cmd/ateapi/internal/controlapi/zz_generated.validation.go +++ b/cmd/ateapi/internal/controlapi/zz_generated.validation.go @@ -836,6 +836,46 @@ func Validate_CreateActorRequest( return errs } +// Validate_CreateActorSnapshotTagRequest validates an instance of CreateActorSnapshotTagRequest according +// to declarative validation rules in the API schema. +func Validate_CreateActorSnapshotTagRequest( + ctx context.Context, op operation.Operation, fldPath *field.Path, + obj, oldObj *ateapipb.CreateActorSnapshotTagRequest) (errs field.ErrorList) { + + { // field ateapipb.CreateActorSnapshotTagRequest.ActorSnapshotTag + fn := func( + fldPath *field.Path, + obj, oldObj *ateapipb.ActorSnapshotTag, + oldValueCorrelated bool) (errs field.ErrorList) { + // don't revalidate unchanged data + if oldValueCorrelated && op.Type == operation.Update { + if ateDeepEqual(obj, oldObj) { + return nil + } + } + // call field-attached validations + earlyReturn := false + if e := validate.RequiredPointer(ctx, op, fldPath, obj, oldObj).MarkShortCircuit(); len(e) != 0 { + errs = append(errs, e...) + earlyReturn = true + } + if earlyReturn { + return // do not proceed + } + // call the type's validation function + errs = append(errs, Validate_ActorSnapshotTag(ctx, op, fldPath, obj, oldObj)...) + return + } + oldVal := safe.Field(oldObj, + func(oldObj *ateapipb.CreateActorSnapshotTagRequest) *ateapipb.ActorSnapshotTag { + return oldObj.ActorSnapshotTag + }) + errs = append(errs, fn(fldPath.Child("actor_snapshot_tag"), obj.ActorSnapshotTag, oldVal, oldObj != nil)...) + } + + return errs +} + // Validate_CreateActorTemplateRequest validates an instance of CreateActorTemplateRequest according // to declarative validation rules in the API schema. func Validate_CreateActorTemplateRequest( @@ -916,6 +956,61 @@ func Validate_CreateAtespaceRequest( return errs } +// Validate_DeleteActorSnapshotTagRequest validates an instance of DeleteActorSnapshotTagRequest according +// to declarative validation rules in the API schema. +func Validate_DeleteActorSnapshotTagRequest( + ctx context.Context, op operation.Operation, fldPath *field.Path, + obj, oldObj *ateapipb.DeleteActorSnapshotTagRequest) (errs field.ErrorList) { + + { // field ateapipb.DeleteActorSnapshotTagRequest.ActorSnapshotTag + fn := func( + fldPath *field.Path, + obj, oldObj *ateapipb.ObjectRef, + oldValueCorrelated bool) (errs field.ErrorList) { + // don't revalidate unchanged data + if oldValueCorrelated && op.Type == operation.Update { + if ateDeepEqual(obj, oldObj) { + return nil + } + } + // call field-attached validations + earlyReturn := false + if e := validate.RequiredPointer(ctx, op, fldPath, obj, oldObj).MarkShortCircuit(); len(e) != 0 { + errs = append(errs, e...) + earlyReturn = true + } + if earlyReturn { + return // do not proceed + } + func() { // cohort = "atespace" + earlyReturn := false + if e := validate.Subfield(ctx, op, fldPath, obj, oldObj, "atespace", + func(o *ateapipb.ObjectRef) *string { return &o.Atespace }, validate.DirectEqual, validate.RequiredValue).MarkShortCircuit(); len(e) != 0 { + errs = append(errs, e...) + earlyReturn = true + } + if e := validate.Subfield(ctx, op, fldPath, obj, oldObj, "atespace", + func(o *ateapipb.ObjectRef) *string { return &o.Atespace }, validate.DirectEqual, validate.OptionalValue).MarkShortCircuit(); len(e) != 0 { + earlyReturn = true + } + if earlyReturn { + return // do not proceed + } + }() + // call the type's validation function + errs = append(errs, Validate_ObjectRef(ctx, op, fldPath, obj, oldObj)...) + return + } + oldVal := safe.Field(oldObj, + func(oldObj *ateapipb.DeleteActorSnapshotTagRequest) *ateapipb.ObjectRef { + return oldObj.ActorSnapshotTag + }) + errs = append(errs, fn(fldPath.Child("actor_snapshot_tag"), obj.ActorSnapshotTag, oldVal, oldObj != nil)...) + } + + return errs +} + // Validate_DeleteAtespaceRequest validates an instance of DeleteAtespaceRequest according // to declarative validation rules in the API schema. func Validate_DeleteAtespaceRequest( @@ -1725,6 +1820,59 @@ func Validate_UpdateActorRequest( return errs } +// Validate_UpdateActorSnapshotTagRequest validates an instance of UpdateActorSnapshotTagRequest according +// to declarative validation rules in the API schema. +func Validate_UpdateActorSnapshotTagRequest( + ctx context.Context, op operation.Operation, fldPath *field.Path, + obj, oldObj *ateapipb.UpdateActorSnapshotTagRequest) (errs field.ErrorList) { + + { // field ateapipb.UpdateActorSnapshotTagRequest.ActorSnapshotTag + fn := func( + fldPath *field.Path, + obj, oldObj *ateapipb.ActorSnapshotTag, + oldValueCorrelated bool) (errs field.ErrorList) { + // don't revalidate unchanged data + if oldValueCorrelated && op.Type == operation.Update { + if ateDeepEqual(obj, oldObj) { + return nil + } + } + // call field-attached validations + earlyReturn := false + if e := validate.RequiredPointer(ctx, op, fldPath, obj, oldObj).MarkShortCircuit(); len(e) != 0 { + errs = append(errs, e...) + earlyReturn = true + } + if earlyReturn { + return // do not proceed + } + // custom validation + if e := ValidateCustom_UpdateActorSnapshotTagRequest_ActorSnapshotTag(ctx, op, fldPath, obj, oldObj); len(e) != 0 { + errs = append(errs, e...) + } + func() { // cohort = "metadata" + earlyReturn := false + if e := validate.Subfield(ctx, op, fldPath, obj, oldObj, "metadata", + func(o *ateapipb.ActorSnapshotTag) *ateapipb.ResourceMetadata { return o.Metadata }, deepEqualImpl_, validate.RequiredPointer).MarkShortCircuit(); len(e) != 0 { + errs = append(errs, e...) + earlyReturn = true + } + if earlyReturn { + return // do not proceed + } + }() + return + } + oldVal := safe.Field(oldObj, + func(oldObj *ateapipb.UpdateActorSnapshotTagRequest) *ateapipb.ActorSnapshotTag { + return oldObj.ActorSnapshotTag + }) + errs = append(errs, fn(fldPath.Child("actor_snapshot_tag"), obj.ActorSnapshotTag, oldVal, oldObj != nil)...) + } + + return errs +} + // Validate_Volume validates an instance of Volume according // to declarative validation rules in the API schema. func Validate_Volume( diff --git a/pkg/proto/ateapipb/ateapi.pb.go b/pkg/proto/ateapipb/ateapi.pb.go index 6b34cbf442..24d215b327 100644 --- a/pkg/proto/ateapipb/ateapi.pb.go +++ b/pkg/proto/ateapipb/ateapi.pb.go @@ -4330,7 +4330,8 @@ func (x *ListActorSnapshotsResponse) GetNextPageToken() string { type CreateActorSnapshotTagRequest struct { state protoimpl.MessageState `protogen:"open.v1"` // The tag to create. - // +k8s:opaqueType # until this request is converted to declarative validation + // + // +k8s:required ActorSnapshotTag *ActorSnapshotTag `protobuf:"bytes,1,opt,name=actor_snapshot_tag,json=actorSnapshotTag,proto3" json:"actor_snapshot_tag,omitempty"` unknownFields protoimpl.UnknownFields sizeCache protoimpl.SizeCache @@ -4383,7 +4384,10 @@ type UpdateActorSnapshotTagRequest struct { // actor_snapshot_tag.metadata.version and actor_snapshot_tag.metadata.uid // are required preconditions // + // +k8s:required // +k8s:opaqueType # updates are handled in 2 steps, do not descend + // +k8s:subfield(metadata)=+k8s:required + // +k8s:customValidation # TODO: when we get nested subfields, require metadata.atespace ActorSnapshotTag *ActorSnapshotTag `protobuf:"bytes,1,opt,name=actor_snapshot_tag,json=actorSnapshotTag,proto3" json:"actor_snapshot_tag,omitempty"` unknownFields protoimpl.UnknownFields sizeCache protoimpl.SizeCache @@ -4428,7 +4432,8 @@ func (x *UpdateActorSnapshotTagRequest) GetActorSnapshotTag() *ActorSnapshotTag type DeleteActorSnapshotTagRequest struct { state protoimpl.MessageState `protogen:"open.v1"` - // +k8s:opaqueType + // +k8s:required + // +k8s:subfield(atespace)=+k8s:required ActorSnapshotTag *ObjectRef `protobuf:"bytes,1,opt,name=actor_snapshot_tag,json=actorSnapshotTag,proto3" json:"actor_snapshot_tag,omitempty"` unknownFields protoimpl.UnknownFields sizeCache protoimpl.SizeCache diff --git a/pkg/proto/ateapipb/ateapi.proto b/pkg/proto/ateapipb/ateapi.proto index 56f6eab31a..7b275f6415 100644 --- a/pkg/proto/ateapipb/ateapi.proto +++ b/pkg/proto/ateapipb/ateapi.proto @@ -988,7 +988,8 @@ message ListActorSnapshotsResponse { message CreateActorSnapshotTagRequest { // The tag to create. - // +k8s:opaqueType # until this request is converted to declarative validation + // + // +k8s:required ActorSnapshotTag actor_snapshot_tag = 1; } @@ -1001,12 +1002,16 @@ message UpdateActorSnapshotTagRequest { // actor_snapshot_tag.metadata.version and actor_snapshot_tag.metadata.uid // are required preconditions // + // +k8s:required // +k8s:opaqueType # updates are handled in 2 steps, do not descend + // +k8s:subfield(metadata)=+k8s:required + // +k8s:customValidation # TODO: when we get nested subfields, require metadata.atespace ActorSnapshotTag actor_snapshot_tag = 1; } message DeleteActorSnapshotTagRequest { - // +k8s:opaqueType + // +k8s:required + // +k8s:subfield(atespace)=+k8s:required ObjectRef actor_snapshot_tag = 1; } From 909ab9c723262ae16b308113c7bdb0bfb2fae800 Mon Sep 17 00:00:00 2001 From: shrutiyam-glitch Date: Thu, 27 Aug 2026 02:44:23 -0700 Subject: [PATCH 3/6] Add DV for the ActorSnapshot and tag read and list requests --- .../internal/controlapi/actor_snapshot.go | 54 ++--- .../controlapi/actor_snapshot_test.go | 123 ++++++++++ .../controlapi/zz_generated.validation.go | 212 ++++++++++++++++++ pkg/proto/ateapipb/ateapi.pb.go | 29 ++- pkg/proto/ateapipb/ateapi.proto | 23 +- 5 files changed, 394 insertions(+), 47 deletions(-) diff --git a/cmd/ateapi/internal/controlapi/actor_snapshot.go b/cmd/ateapi/internal/controlapi/actor_snapshot.go index 23206a9ad7..5639ff3609 100644 --- a/cmd/ateapi/internal/controlapi/actor_snapshot.go +++ b/cmd/ateapi/internal/controlapi/actor_snapshot.go @@ -52,7 +52,7 @@ func (s *ServiceImpl) CreateActorSnapshot(ctx context.Context, snapshot *ateapip } func (s *RPCService) GetActorSnapshot(ctx context.Context, req *ateapipb.GetActorSnapshotRequest) (*ateapipb.ActorSnapshot, error) { - if errs := validateGetActorSnapshotRequest(req); len(errs) > 0 { + if errs := validateGetActorSnapshotRequest(ctx, req); len(errs) > 0 { return nil, toGRPCStatusError(errs) } snapshot, err := s.impl.GetActorSnapshot(ctx, resources.ActorSnapshotRefFromObjectRef(req.GetActorSnapshot())) @@ -70,21 +70,14 @@ func (s *ServiceImpl) GetActorSnapshot(ctx context.Context, snapshotRef resource return s.store.GetActorSnapshot(ctx, snapshotRef) } -func validateGetActorSnapshotRequest(req *ateapipb.GetActorSnapshotRequest) field.ErrorList { - var fldPath *field.Path - var errs field.ErrorList - - if val, fldPath := req.ActorSnapshot, fldPath.Child("actor_snapshot"); val == nil { - errs = append(errs, field.Required(fldPath, "")) - } else { - errs = append(errs, resources.ValidateObjectRef(val, fldPath)...) - } - - return errs +func validateGetActorSnapshotRequest(ctx context.Context, req *ateapipb.GetActorSnapshotRequest) field.ErrorList { + // Call the generated validation. + op := operation.Operation{Type: operation.Create} + return Validate_GetActorSnapshotRequest(ctx, op, nil, req, nil) } func (s *RPCService) GetActorSnapshotTag(ctx context.Context, req *ateapipb.GetActorSnapshotTagRequest) (*ateapipb.ActorSnapshotTag, error) { - if errs := validateGetActorSnapshotTagRequest(req); len(errs) > 0 { + if errs := validateGetActorSnapshotTagRequest(ctx, req); len(errs) > 0 { return nil, toGRPCStatusError(errs) } tag, err := s.impl.GetActorSnapshotTag(ctx, resources.ActorSnapshotTagRefFromObjectRef(req.GetActorSnapshotTag())) @@ -102,21 +95,14 @@ func (s *ServiceImpl) GetActorSnapshotTag(ctx context.Context, tagRef resources. return s.store.GetActorSnapshotTag(ctx, tagRef) } -func validateGetActorSnapshotTagRequest(req *ateapipb.GetActorSnapshotTagRequest) field.ErrorList { - var fldPath *field.Path - var errs field.ErrorList - - if val, fldPath := req.ActorSnapshotTag, fldPath.Child("actor_snapshot_tag"); val == nil { - errs = append(errs, field.Required(fldPath, "")) - } else { - errs = append(errs, resources.ValidateObjectRef(val, fldPath)...) - } - - return errs +func validateGetActorSnapshotTagRequest(ctx context.Context, req *ateapipb.GetActorSnapshotTagRequest) field.ErrorList { + // Call the generated validation. + op := operation.Operation{Type: operation.Create} + return Validate_GetActorSnapshotTagRequest(ctx, op, nil, req, nil) } func (s *RPCService) ListActorSnapshots(ctx context.Context, req *ateapipb.ListActorSnapshotsRequest) (*ateapipb.ListActorSnapshotsResponse, error) { - if errs := validateListActorSnapshotsRequest(req); len(errs) > 0 { + if errs := validateListActorSnapshotsRequest(ctx, req); len(errs) > 0 { return nil, toGRPCStatusError(errs) } page, err := s.impl.ListActorSnapshots(ctx, req.GetAtespace(), store.ListOptions{PageSize: effectivePageSize(req.GetPageSize()), PageToken: req.GetPageToken()}) @@ -131,20 +117,10 @@ func (s *ServiceImpl) ListActorSnapshots(ctx context.Context, atespace string, o return s.store.ListActorSnapshots(ctx, atespace, opts) } -func validateListActorSnapshotsRequest(req *ateapipb.ListActorSnapshotsRequest) field.ErrorList { - var fldPath *field.Path - var errs field.ErrorList - - // An empty atespace is allowed here and means "all atespaces". - if val, fldPath := req.Atespace, fldPath.Child("atespace"); val != "" { - errs = append(errs, resources.ValidateResourceName(val, fldPath)...) - } - - if val, fldPath := req.PageSize, fldPath.Child("page_size"); val < 0 { - errs = append(errs, field.Invalid(fldPath, val, "must be greater than or equal to 0")) - } - - return errs +func validateListActorSnapshotsRequest(ctx context.Context, req *ateapipb.ListActorSnapshotsRequest) field.ErrorList { + // Call the generated validation. + op := operation.Operation{Type: operation.Create} + return Validate_ListActorSnapshotsRequest(ctx, op, nil, req, nil) } func (s *RPCService) CreateActorSnapshotTag(ctx context.Context, req *ateapipb.CreateActorSnapshotTagRequest) (*ateapipb.ActorSnapshotTag, error) { diff --git a/cmd/ateapi/internal/controlapi/actor_snapshot_test.go b/cmd/ateapi/internal/controlapi/actor_snapshot_test.go index 28f5be1bc1..dd8de510ae 100644 --- a/cmd/ateapi/internal/controlapi/actor_snapshot_test.go +++ b/cmd/ateapi/internal/controlapi/actor_snapshot_test.go @@ -16,6 +16,7 @@ package controlapi import ( "context" + "strings" "testing" "github.com/google/go-cmp/cmp" @@ -480,6 +481,12 @@ func TestValidateActorSnapshotRefRequests(t *testing.T) { func(root string) field.ErrorList { return field.ErrorList{field.Required(field.NewPath(root, "atespace"), "")} }, + }, { + "invalid atespace", + &ateapipb.ObjectRef{Atespace: "TEAM A", Name: "obj-1"}, + func(root string) field.ErrorList { + return field.ErrorList{field.Invalid(field.NewPath(root, "atespace"), nil, "").WithOrigin("format=k8s-short-name")} + }, }, { "missing name", &ateapipb.ObjectRef{Atespace: "team-a"}, @@ -494,6 +501,14 @@ func TestValidateActorSnapshotRefRequests(t *testing.T) { }, }} for _, tc := range cases { + t.Run("GetActorSnapshot/"+tc.name, func(t *testing.T) { + got := validateGetActorSnapshotRequest(context.Background(), &ateapipb.GetActorSnapshotRequest{ActorSnapshot: tc.ref}) + assertValidateErr(t, got, tc.want("actor_snapshot")) + }) + t.Run("GetActorSnapshotTag/"+tc.name, func(t *testing.T) { + got := validateGetActorSnapshotTagRequest(context.Background(), &ateapipb.GetActorSnapshotTagRequest{ActorSnapshotTag: tc.ref}) + assertValidateErr(t, got, tc.want("actor_snapshot_tag")) + }) t.Run("DeleteActorSnapshotTag/"+tc.name, func(t *testing.T) { got := validateDeleteActorSnapshotTagRequest(context.Background(), &ateapipb.DeleteActorSnapshotTagRequest{ActorSnapshotTag: tc.ref}) assertValidateErr(t, got, tc.want("actor_snapshot_tag")) @@ -501,6 +516,43 @@ func TestValidateActorSnapshotRefRequests(t *testing.T) { } } +func TestValidateListActorSnapshotsRequest(t *testing.T) { + tests := []struct { + name string + req *ateapipb.ListActorSnapshotsRequest + want field.ErrorList + }{{ + "valid, atespace scoped", + &ateapipb.ListActorSnapshotsRequest{Atespace: "team-a"}, + nil, + }, { + "valid, empty atespace means all atespaces", + &ateapipb.ListActorSnapshotsRequest{}, + nil, + }, { + "invalid atespace", + &ateapipb.ListActorSnapshotsRequest{Atespace: "TEAM-A"}, + field.ErrorList{field.Invalid(field.NewPath("atespace"), nil, "").WithOrigin("format=k8s-short-name")}, + }, { + "negative page_size", + &ateapipb.ListActorSnapshotsRequest{PageSize: -1}, + field.ErrorList{field.Invalid(field.NewPath("page_size"), nil, "").WithOrigin("minimum")}, + }, { + "valid page_token", + &ateapipb.ListActorSnapshotsRequest{PageToken: strings.Repeat("x", 256)}, + nil, + }, { + "too-large page_token", + &ateapipb.ListActorSnapshotsRequest{PageToken: strings.Repeat("x", 257)}, + field.ErrorList{field.TooLongCharacters(field.NewPath("page_token"), "", 256).WithOrigin("maxLength")}, + }} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assertValidateErr(t, validateListActorSnapshotsRequest(context.Background(), tt.req), tt.want) + }) + } +} + // TestServiceImplUpdateActorSnapshotTag_ImmutableFields pins the // immutable-snapshot rule at the layer that now owns it: declarative // validation in ServiceImpl, which every write path shares. The store no @@ -631,3 +683,74 @@ func TestCreateActorSnapshotTag_IgnoresRequestMetadataServerFields(t *testing.T) t.Errorf("created tag version = %d, want 1", got.GetMetadata().GetVersion()) } } + +// TestReadActorSnapshotRPCs pins the Get and Delete RPC paths end to end: +// a present resource round-trips, invalid refs are rejected by the generated +// validation, and absent resources map to NOT_FOUND. +func TestReadActorSnapshotRPCs(t *testing.T) { + ctx := context.Background() + svc, stored := rpcServiceWithActorSnapshotTag(t, &ateapipb.ActorSnapshotTag{ + Metadata: &ateapipb.ResourceMetadata{Atespace: testAtespace, Name: "tag-1"}, + Scope: ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_ATESPACE, + }) + + if _, err := svc.GetActorSnapshot(ctx, &ateapipb.GetActorSnapshotRequest{ActorSnapshot: stored.GetSnapshot()}); err != nil { + t.Errorf("GetActorSnapshot(existing) failed: %v", err) + } + got, err := svc.GetActorSnapshotTag(ctx, &ateapipb.GetActorSnapshotTagRequest{ + ActorSnapshotTag: &ateapipb.ObjectRef{Atespace: testAtespace, Name: "tag-1"}, + }) + if err != nil { + t.Errorf("GetActorSnapshotTag(existing) failed: %v", err) + } else if got.GetMetadata().GetUid() != stored.GetMetadata().GetUid() { + t.Errorf("GetActorSnapshotTag() uid = %q, want the stored %q", got.GetMetadata().GetUid(), stored.GetMetadata().GetUid()) + } + + absent := &ateapipb.ObjectRef{Atespace: testAtespace, Name: "no-such-thing"} + for _, tc := range []struct { + name string + call func() error + want codes.Code + }{{ + "GetActorSnapshot absent", + func() error { + _, err := svc.GetActorSnapshot(ctx, &ateapipb.GetActorSnapshotRequest{ActorSnapshot: absent}) + return err + }, + codes.NotFound, + }, { + "GetActorSnapshot no ref", + func() error { + _, err := svc.GetActorSnapshot(ctx, &ateapipb.GetActorSnapshotRequest{}) + return err + }, + codes.InvalidArgument, + }, { + "GetActorSnapshotTag absent", + func() error { + _, err := svc.GetActorSnapshotTag(ctx, &ateapipb.GetActorSnapshotTagRequest{ActorSnapshotTag: absent}) + return err + }, + codes.NotFound, + }, { + "GetActorSnapshotTag no ref", + func() error { + _, err := svc.GetActorSnapshotTag(ctx, &ateapipb.GetActorSnapshotTagRequest{}) + return err + }, + codes.InvalidArgument, + }, { + "DeleteActorSnapshotTag absent", + func() error { + _, err := svc.DeleteActorSnapshotTag(ctx, &ateapipb.DeleteActorSnapshotTagRequest{ActorSnapshotTag: absent}) + return err + }, + codes.NotFound, + }} { + t.Run(tc.name, func(t *testing.T) { + if got := status.Code(tc.call()); got != tc.want { + t.Errorf("code = %v, want %v", got, tc.want) + } + }) + } +} diff --git a/cmd/ateapi/internal/controlapi/zz_generated.validation.go b/cmd/ateapi/internal/controlapi/zz_generated.validation.go index 207a0747e4..9a70d6d354 100644 --- a/cmd/ateapi/internal/controlapi/zz_generated.validation.go +++ b/cmd/ateapi/internal/controlapi/zz_generated.validation.go @@ -1070,6 +1070,116 @@ func Validate_DeleteAtespaceRequest( return errs } +// Validate_GetActorSnapshotRequest validates an instance of GetActorSnapshotRequest according +// to declarative validation rules in the API schema. +func Validate_GetActorSnapshotRequest( + ctx context.Context, op operation.Operation, fldPath *field.Path, + obj, oldObj *ateapipb.GetActorSnapshotRequest) (errs field.ErrorList) { + + { // field ateapipb.GetActorSnapshotRequest.ActorSnapshot + fn := func( + fldPath *field.Path, + obj, oldObj *ateapipb.ObjectRef, + oldValueCorrelated bool) (errs field.ErrorList) { + // don't revalidate unchanged data + if oldValueCorrelated && op.Type == operation.Update { + if ateDeepEqual(obj, oldObj) { + return nil + } + } + // call field-attached validations + earlyReturn := false + if e := validate.RequiredPointer(ctx, op, fldPath, obj, oldObj).MarkShortCircuit(); len(e) != 0 { + errs = append(errs, e...) + earlyReturn = true + } + if earlyReturn { + return // do not proceed + } + func() { // cohort = "atespace" + earlyReturn := false + if e := validate.Subfield(ctx, op, fldPath, obj, oldObj, "atespace", + func(o *ateapipb.ObjectRef) *string { return &o.Atespace }, validate.DirectEqual, validate.RequiredValue).MarkShortCircuit(); len(e) != 0 { + errs = append(errs, e...) + earlyReturn = true + } + if e := validate.Subfield(ctx, op, fldPath, obj, oldObj, "atespace", + func(o *ateapipb.ObjectRef) *string { return &o.Atespace }, validate.DirectEqual, validate.OptionalValue).MarkShortCircuit(); len(e) != 0 { + earlyReturn = true + } + if earlyReturn { + return // do not proceed + } + }() + // call the type's validation function + errs = append(errs, Validate_ObjectRef(ctx, op, fldPath, obj, oldObj)...) + return + } + oldVal := safe.Field(oldObj, + func(oldObj *ateapipb.GetActorSnapshotRequest) *ateapipb.ObjectRef { + return oldObj.ActorSnapshot + }) + errs = append(errs, fn(fldPath.Child("actor_snapshot"), obj.ActorSnapshot, oldVal, oldObj != nil)...) + } + + return errs +} + +// Validate_GetActorSnapshotTagRequest validates an instance of GetActorSnapshotTagRequest according +// to declarative validation rules in the API schema. +func Validate_GetActorSnapshotTagRequest( + ctx context.Context, op operation.Operation, fldPath *field.Path, + obj, oldObj *ateapipb.GetActorSnapshotTagRequest) (errs field.ErrorList) { + + { // field ateapipb.GetActorSnapshotTagRequest.ActorSnapshotTag + fn := func( + fldPath *field.Path, + obj, oldObj *ateapipb.ObjectRef, + oldValueCorrelated bool) (errs field.ErrorList) { + // don't revalidate unchanged data + if oldValueCorrelated && op.Type == operation.Update { + if ateDeepEqual(obj, oldObj) { + return nil + } + } + // call field-attached validations + earlyReturn := false + if e := validate.RequiredPointer(ctx, op, fldPath, obj, oldObj).MarkShortCircuit(); len(e) != 0 { + errs = append(errs, e...) + earlyReturn = true + } + if earlyReturn { + return // do not proceed + } + func() { // cohort = "atespace" + earlyReturn := false + if e := validate.Subfield(ctx, op, fldPath, obj, oldObj, "atespace", + func(o *ateapipb.ObjectRef) *string { return &o.Atespace }, validate.DirectEqual, validate.RequiredValue).MarkShortCircuit(); len(e) != 0 { + errs = append(errs, e...) + earlyReturn = true + } + if e := validate.Subfield(ctx, op, fldPath, obj, oldObj, "atespace", + func(o *ateapipb.ObjectRef) *string { return &o.Atespace }, validate.DirectEqual, validate.OptionalValue).MarkShortCircuit(); len(e) != 0 { + earlyReturn = true + } + if earlyReturn { + return // do not proceed + } + }() + // call the type's validation function + errs = append(errs, Validate_ObjectRef(ctx, op, fldPath, obj, oldObj)...) + return + } + oldVal := safe.Field(oldObj, + func(oldObj *ateapipb.GetActorSnapshotTagRequest) *ateapipb.ObjectRef { + return oldObj.ActorSnapshotTag + }) + errs = append(errs, fn(fldPath.Child("actor_snapshot_tag"), obj.ActorSnapshotTag, oldVal, oldObj != nil)...) + } + + return errs +} + // Validate_GetAtespaceRequest validates an instance of GetAtespaceRequest according // to declarative validation rules in the API schema. func Validate_GetAtespaceRequest( @@ -1129,6 +1239,108 @@ func Validate_GetAtespaceRequest( return errs } +// Validate_ListActorSnapshotsRequest validates an instance of ListActorSnapshotsRequest according +// to declarative validation rules in the API schema. +func Validate_ListActorSnapshotsRequest( + ctx context.Context, op operation.Operation, fldPath *field.Path, + obj, oldObj *ateapipb.ListActorSnapshotsRequest) (errs field.ErrorList) { + + { // field ateapipb.ListActorSnapshotsRequest.Atespace + fn := func( + fldPath *field.Path, + obj, oldObj *string, + oldValueCorrelated bool) (errs field.ErrorList) { + // don't revalidate unchanged data + if oldValueCorrelated && op.Type == operation.Update { + if obj == oldObj || (obj != nil && oldObj != nil && *obj == *oldObj) { + return nil + } + } + // call field-attached validations + earlyReturn := false + if e := validate.OptionalValue(ctx, op, fldPath, obj, oldObj).MarkShortCircuit(); len(e) != 0 { + earlyReturn = true + } + if earlyReturn { + return // do not proceed + } + if e := validate.ShortName(ctx, op, fldPath, obj, oldObj); len(e) != 0 { + errs = append(errs, e...) + } + return + } + oldVal := safe.Field(oldObj, + func(oldObj *ateapipb.ListActorSnapshotsRequest) *string { + return &oldObj.Atespace + }) + errs = append(errs, fn(fldPath.Child("atespace"), &obj.Atespace, oldVal, oldObj != nil)...) + } + + { // field ateapipb.ListActorSnapshotsRequest.PageSize + fn := func( + fldPath *field.Path, + obj, oldObj *int32, + oldValueCorrelated bool) (errs field.ErrorList) { + // don't revalidate unchanged data + if oldValueCorrelated && op.Type == operation.Update { + if obj == oldObj || (obj != nil && oldObj != nil && *obj == *oldObj) { + return nil + } + } + // call field-attached validations + earlyReturn := false + if e := validate.OptionalValue(ctx, op, fldPath, obj, oldObj).MarkShortCircuit(); len(e) != 0 { + earlyReturn = true + } + if earlyReturn { + return // do not proceed + } + if e := validate.Minimum(ctx, op, fldPath, obj, oldObj, 1); len(e) != 0 { + errs = append(errs, e...) + } + return + } + oldVal := safe.Field(oldObj, + func(oldObj *ateapipb.ListActorSnapshotsRequest) *int32 { + return &oldObj.PageSize + }) + errs = append(errs, fn(fldPath.Child("page_size"), &obj.PageSize, oldVal, oldObj != nil)...) + } + + { // field ateapipb.ListActorSnapshotsRequest.PageToken + fn := func( + fldPath *field.Path, + obj, oldObj *string, + oldValueCorrelated bool) (errs field.ErrorList) { + // don't revalidate unchanged data + if oldValueCorrelated && op.Type == operation.Update { + if obj == oldObj || (obj != nil && oldObj != nil && *obj == *oldObj) { + return nil + } + } + // call field-attached validations + earlyReturn := false + if e := validate.OptionalValue(ctx, op, fldPath, obj, oldObj).MarkShortCircuit(); len(e) != 0 { + earlyReturn = true + } + if earlyReturn { + return // do not proceed + } + if e := validate.MaxLength(ctx, op, fldPath, obj, oldObj, 256); len(e) != 0 { + errs = append(errs, e...) + } + return + } + oldVal := safe.Field(oldObj, + func(oldObj *ateapipb.ListActorSnapshotsRequest) *string { + return &oldObj.PageToken + }) + errs = append(errs, fn(fldPath.Child("page_token"), &obj.PageToken, oldVal, oldObj != nil)...) + } + + return errs +} + // Validate_ListAtespacesRequest validates an instance of ListAtespacesRequest according // to declarative validation rules in the API schema. func Validate_ListAtespacesRequest( diff --git a/pkg/proto/ateapipb/ateapi.pb.go b/pkg/proto/ateapipb/ateapi.pb.go index 1331cfbc3d..d5becec7ce 100644 --- a/pkg/proto/ateapipb/ateapi.pb.go +++ b/pkg/proto/ateapipb/ateapi.pb.go @@ -4110,7 +4110,8 @@ func (x *DeleteActorRequest) GetAnyState() bool { type GetActorSnapshotRequest struct { state protoimpl.MessageState `protogen:"open.v1"` - // +k8s:opaqueType + // +k8s:required + // +k8s:subfield(atespace)=+k8s:required ActorSnapshot *ObjectRef `protobuf:"bytes,1,opt,name=actor_snapshot,json=actorSnapshot,proto3" json:"actor_snapshot,omitempty"` unknownFields protoimpl.UnknownFields sizeCache protoimpl.SizeCache @@ -4155,7 +4156,8 @@ func (x *GetActorSnapshotRequest) GetActorSnapshot() *ObjectRef { type GetActorSnapshotTagRequest struct { state protoimpl.MessageState `protogen:"open.v1"` - // +k8s:opaqueType + // +k8s:required + // +k8s:subfield(atespace)=+k8s:required ActorSnapshotTag *ObjectRef `protobuf:"bytes,1,opt,name=actor_snapshot_tag,json=actorSnapshotTag,proto3" json:"actor_snapshot_tag,omitempty"` unknownFields protoimpl.UnknownFields sizeCache protoimpl.SizeCache @@ -4199,10 +4201,25 @@ func (x *GetActorSnapshotTagRequest) GetActorSnapshotTag() *ObjectRef { } type ListActorSnapshotsRequest struct { - state protoimpl.MessageState `protogen:"open.v1"` - Atespace string `protobuf:"bytes,1,opt,name=atespace,proto3" json:"atespace,omitempty"` - PageSize int32 `protobuf:"varint,2,opt,name=page_size,json=pageSize,proto3" json:"page_size,omitempty"` - PageToken string `protobuf:"bytes,3,opt,name=page_token,json=pageToken,proto3" json:"page_token,omitempty"` + state protoimpl.MessageState `protogen:"open.v1"` + // The atespace to list snapshots from. Empty lists across all atespaces. + // + // +k8s:optional + // +k8s:format=k8s-short-name + Atespace string `protobuf:"bytes,1,opt,name=atespace,proto3" json:"atespace,omitempty"` + // Requested page size; the server may return fewer, or occasionally + // slightly more. If unspecified, defaults to a server-chosen value; + // values above 1000 are coerced to 1000. + // + // +k8s:optional + // +k8s:minimum=1 + PageSize int32 `protobuf:"varint,2,opt,name=page_size,json=pageSize,proto3" json:"page_size,omitempty"` + // Pagination token from a previous ListActorSnapshots response. + // Omit or leave empty for the first request. + // + // +k8s:optional + // +k8s:maxLength=256 + PageToken string `protobuf:"bytes,3,opt,name=page_token,json=pageToken,proto3" json:"page_token,omitempty"` unknownFields protoimpl.UnknownFields sizeCache protoimpl.SizeCache } diff --git a/pkg/proto/ateapipb/ateapi.proto b/pkg/proto/ateapipb/ateapi.proto index 6f624abe3c..e44d3bedcf 100644 --- a/pkg/proto/ateapipb/ateapi.proto +++ b/pkg/proto/ateapipb/ateapi.proto @@ -958,18 +958,37 @@ message DeleteActorRequest { } message GetActorSnapshotRequest { - // +k8s:opaqueType + // +k8s:required + // +k8s:subfield(atespace)=+k8s:required ObjectRef actor_snapshot = 1; } message GetActorSnapshotTagRequest { - // +k8s:opaqueType + // +k8s:required + // +k8s:subfield(atespace)=+k8s:required ObjectRef actor_snapshot_tag = 1; } message ListActorSnapshotsRequest { + // The atespace to list snapshots from. Empty lists across all atespaces. + // + // +k8s:optional + // +k8s:format=k8s-short-name string atespace = 1; + + // Requested page size; the server may return fewer, or occasionally + // slightly more. If unspecified, defaults to a server-chosen value; + // values above 1000 are coerced to 1000. + // + // +k8s:optional + // +k8s:minimum=1 int32 page_size = 2; + + // Pagination token from a previous ListActorSnapshots response. + // Omit or leave empty for the first request. + // + // +k8s:optional + // +k8s:maxLength=256 string page_token = 3; } From a4e616355d612200f4fad2480cc245f577ea12d7 Mon Sep 17 00:00:00 2001 From: shrutiyam-glitch Date: Thu, 27 Aug 2026 09:32:30 -0700 Subject: [PATCH 4/6] Use setMetadata and add comments --- .../internal/controlapi/actor_snapshot.go | 32 +++++++++---------- cmd/ateapi/internal/store/atepg/atepg.go | 7 ++-- pkg/proto/ateapipb/ateapi.pb.go | 2 +- pkg/proto/ateapipb/ateapi.proto | 1 + 4 files changed, 23 insertions(+), 19 deletions(-) diff --git a/cmd/ateapi/internal/controlapi/actor_snapshot.go b/cmd/ateapi/internal/controlapi/actor_snapshot.go index 5639ff3609..2ab5d8c4d6 100644 --- a/cmd/ateapi/internal/controlapi/actor_snapshot.go +++ b/cmd/ateapi/internal/controlapi/actor_snapshot.go @@ -30,22 +30,6 @@ import ( "k8s.io/apimachinery/pkg/util/validation/field" ) -// This exists only because nested subfield tags are not supported yet. -func ValidateCustom_UpdateActorSnapshotTagRequest_ActorSnapshotTag(ctx context.Context, op operation.Operation, fldPath *field.Path, tag, _ *ateapipb.ActorSnapshotTag) field.ErrorList { - if tag == nil || tag.Metadata == nil { - return nil // handled by DV - } - - // Updates are validated in 2 steps: first the update request and then the - // resource itself. DV for the request doesn't descend into the resource - // metadata. Once DV supports nested subfield tags, this can be changed to - // something like: - // +k8s:subfield(metadata)=+k8s:subfield(atespace)=+k8s:required - errs := Validate_ResourceMetadata(ctx, op, fldPath.Child("metadata"), tag.Metadata, nil) - errs = append(errs, validate.RequiredValue(ctx, op, fldPath.Child("metadata", "atespace"), &tag.Metadata.Atespace, nil)...) - return errs -} - func (s *ServiceImpl) CreateActorSnapshot(ctx context.Context, snapshot *ateapipb.ActorSnapshot) (*ateapipb.ActorSnapshot, error) { // TODO: implement this return s.store.CreateActorSnapshot(ctx, snapshot) @@ -279,3 +263,19 @@ func validateActorSnapshotTagUpdate(ctx context.Context, fldPath *field.Path, ne op := operation.Operation{Type: operation.Update} return Validate_ActorSnapshotTag(ctx, op, fldPath, newVal, oldVal) } + +// This exists only because nested subfield tags are not supported yet. +func ValidateCustom_UpdateActorSnapshotTagRequest_ActorSnapshotTag(ctx context.Context, op operation.Operation, fldPath *field.Path, tag, _ *ateapipb.ActorSnapshotTag) field.ErrorList { + if tag == nil || tag.Metadata == nil { + return nil // handled by DV + } + + // Updates are validated in 2 steps: first the update request and then the + // resource itself. DV for the request doesn't descend into the resource + // metadata. Once DV supports nested subfield tags, this can be changed to + // something like: + // +k8s:subfield(metadata)=+k8s:subfield(atespace)=+k8s:required + errs := Validate_ResourceMetadata(ctx, op, fldPath.Child("metadata"), tag.Metadata, nil) + errs = append(errs, validate.RequiredValue(ctx, op, fldPath.Child("metadata", "atespace"), &tag.Metadata.Atespace, nil)...) + return errs +} diff --git a/cmd/ateapi/internal/store/atepg/atepg.go b/cmd/ateapi/internal/store/atepg/atepg.go index 92eba8b514..3d31b00e79 100644 --- a/cmd/ateapi/internal/store/atepg/atepg.go +++ b/cmd/ateapi/internal/store/atepg/atepg.go @@ -1011,7 +1011,10 @@ func (p *Persistence) CreateActorSnapshotTag(ctx context.Context, snapshotRef re tagAtespace := tag.GetMetadata().GetAtespace() tagName := tag.GetMetadata().GetName() dbTag := proto.Clone(tag).(*ateapipb.ActorSnapshotTag) - dbTag.Metadata = newCreateMetadata(tagAtespace, tagName) + if dbTag.Metadata == nil { + dbTag.Metadata = &ateapipb.ResourceMetadata{} + } + setCreateMetadata(dbTag.Metadata) dbTag.Snapshot = &ateapipb.ObjectRef{Atespace: snapshotAtespace, Name: snapshotName} protoBytes, err := proto.Marshal(dbTag) if err != nil { @@ -1106,7 +1109,7 @@ func (p *Persistence) UpdateActorSnapshotTag(ctx context.Context, tagRef resourc } // Stored metadata is authoritative; discard any metadata edits made by the // closure and derive the next revision from the state this attempt read. - dbTag.Metadata = newUpdateMetadata(oldMeta) + setUpdateMetadata(dbTag.Metadata, oldMeta) updatedBytes, err := proto.Marshal(dbTag) if err != nil { diff --git a/pkg/proto/ateapipb/ateapi.pb.go b/pkg/proto/ateapipb/ateapi.pb.go index d5becec7ce..7db9074055 100644 --- a/pkg/proto/ateapipb/ateapi.pb.go +++ b/pkg/proto/ateapipb/ateapi.pb.go @@ -97,7 +97,7 @@ const ( ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_ATESPACE ActorSnapshotTagScope = 1 // Published for use by Actors in any Atespace. The tag remains addressed // through its owning Atespace. - ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED ActorSnapshotTagScope = 2 + ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED ActorSnapshotTagScope = 2 // Keep this in sync with ActorSnapshotTag.scope's maximum. ) // Enum value maps for ActorSnapshotTagScope. diff --git a/pkg/proto/ateapipb/ateapi.proto b/pkg/proto/ateapipb/ateapi.proto index e44d3bedcf..aca5a71a22 100644 --- a/pkg/proto/ateapipb/ateapi.proto +++ b/pkg/proto/ateapipb/ateapi.proto @@ -139,6 +139,7 @@ enum ActorSnapshotTagScope { // Published for use by Actors in any Atespace. The tag remains addressed // through its owning Atespace. ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED = 2; + // Keep this in sync with ActorSnapshotTag.scope's maximum. } // Selector matches worker pools by label. From ebb65b0968974c0f49f96ed1e4eccb53aae2b43d Mon Sep 17 00:00:00 2001 From: shrutiyam-glitch Date: Thu, 27 Aug 2026 17:40:08 -0700 Subject: [PATCH 5/6] fix exemptions --- tools/apitool/exemptions.json | 45 ----------------------------------- 1 file changed, 45 deletions(-) diff --git a/tools/apitool/exemptions.json b/tools/apitool/exemptions.json index f7938123e6..fb032203a0 100644 --- a/tools/apitool/exemptions.json +++ b/tools/apitool/exemptions.json @@ -59,11 +59,6 @@ "subject": "ateapi.ActorSnapshotStatus.source_actor_version", "message": "field has no doc comment" }, - { - "rule": "documented", - "subject": "ateapi.ActorSnapshotTag.scope", - "message": "field has no doc comment" - }, { "rule": "documented", "subject": "ateapi.ActorSnapshotTagScope", @@ -114,16 +109,6 @@ "subject": "ateapi.ArchAssets", "message": "message has no doc comment" }, - { - "rule": "documented", - "subject": "ateapi.Capabilities.add", - "message": "field has no doc comment" - }, - { - "rule": "documented", - "subject": "ateapi.Capabilities.drop", - "message": "field has no doc comment" - }, { "rule": "documented", "subject": "ateapi.Container.env", @@ -274,11 +259,6 @@ "subject": "ateapi.HTTPGetAction.port", "message": "field has no doc comment" }, - { - "rule": "documented", - "subject": "ateapi.ImageVolumeSource.reference", - "message": "field has no doc comment" - }, { "rule": "documented", "subject": "ateapi.KubeNamespacedObjectRef", @@ -314,21 +294,6 @@ "subject": "ateapi.ListActorSnapshotsRequest", "message": "message has no doc comment" }, - { - "rule": "documented", - "subject": "ateapi.ListActorSnapshotsRequest.atespace", - "message": "field has no doc comment" - }, - { - "rule": "documented", - "subject": "ateapi.ListActorSnapshotsRequest.page_size", - "message": "field has no doc comment" - }, - { - "rule": "documented", - "subject": "ateapi.ListActorSnapshotsRequest.page_token", - "message": "field has no doc comment" - }, { "rule": "documented", "subject": "ateapi.ListActorSnapshotsResponse", @@ -429,11 +394,6 @@ "subject": "ateapi.SandboxAssets.sandbox_class", "message": "field has no doc comment" }, - { - "rule": "documented", - "subject": "ateapi.SecurityContext.capabilities", - "message": "field has no doc comment" - }, { "rule": "documented", "subject": "ateapi.SnapshotContentScope", @@ -479,11 +439,6 @@ "subject": "ateapi.Volume.external_volume_template", "message": "field has no doc comment" }, - { - "rule": "documented", - "subject": "ateapi.Volume.image", - "message": "field has no doc comment" - }, { "rule": "documented", "subject": "ateapi.Worker.ip", From aee21273b36af6f08c29dd5fe148b408b968d427 Mon Sep 17 00:00:00 2001 From: shrutiyam-glitch Date: Fri, 28 Aug 2026 10:43:41 -0700 Subject: [PATCH 6/6] Update exemptions --- tools/apitool/exemptions.json | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/tools/apitool/exemptions.json b/tools/apitool/exemptions.json index 2ccb0093eb..2d65402935 100644 --- a/tools/apitool/exemptions.json +++ b/tools/apitool/exemptions.json @@ -94,16 +94,6 @@ "subject": "ateapi.ActorSnapshotStatus.source_actor_version", "message": "field has no doc comment" }, - { - "rule": "documented", - "subject": "ateapi.ActorSnapshotTag.scope", - "message": "field has no doc comment" - }, - { - "rule": "documented", - "subject": "ateapi.ActorSnapshotTag.snapshot", - "message": "field has no doc comment" - }, { "rule": "documented", "subject": "ateapi.ActorSnapshotTagScope", @@ -624,4 +614,4 @@ "subject": "ateapi.WorkerStatus.state", "message": "field has no doc comment" } -] \ No newline at end of file +]