From a28e6e89fca4b2fe0a689e1ea514e86a4ba6fa3d Mon Sep 17 00:00:00 2001 From: Luiz Oliveira Date: Wed, 5 Aug 2026 11:06:38 -0400 Subject: [PATCH] Make UpdateActorSnapshot follow substrate API guidelines --- .../internal/controlapi/actor_snapshot.go | 82 +++++-- .../controlapi/actor_snapshot_test.go | 226 ++++++++++++++++++ .../internal/controlapi/functional_test.go | 212 +++++++++++++++- .../internal/controlapi/update_actor.go | 60 +---- .../internal/controlapi/update_actor_test.go | 47 ---- cmd/ateapi/internal/controlapi/update_mask.go | 60 +++++ .../internal/controlapi/update_mask_test.go | 184 ++++++++++++++ .../internal/cmd/actor_snapshots.go | 8 +- internal/e2e/suites/demo/demo_test.go | 7 +- internal/resources/validate.go | 32 +++ internal/resources/validate_test.go | 64 +++++ pkg/proto/ateapipb/ateapi.pb.go | 38 ++- pkg/proto/ateapipb/ateapi.proto | 16 +- 13 files changed, 900 insertions(+), 136 deletions(-) create mode 100644 cmd/ateapi/internal/controlapi/actor_snapshot_test.go create mode 100644 cmd/ateapi/internal/controlapi/update_mask.go create mode 100644 cmd/ateapi/internal/controlapi/update_mask_test.go diff --git a/cmd/ateapi/internal/controlapi/actor_snapshot.go b/cmd/ateapi/internal/controlapi/actor_snapshot.go index 27de85482..e83734291 100644 --- a/cmd/ateapi/internal/controlapi/actor_snapshot.go +++ b/cmd/ateapi/internal/controlapi/actor_snapshot.go @@ -18,6 +18,7 @@ import ( "context" "errors" "fmt" + "slices" "github.com/agent-substrate/substrate/cmd/ateapi/internal/store" "github.com/agent-substrate/substrate/internal/resources" @@ -27,6 +28,21 @@ import ( "k8s.io/apimachinery/pkg/util/validation/field" ) +// actorSnapshotTagScopes lists the scopes a client may set on an ActorSnapshotTag. +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() + } + return names +}() + func (s *Service) GetActorSnapshot(ctx context.Context, req *ateapipb.GetActorSnapshotRequest) (*ateapipb.ActorSnapshot, error) { if err := validateActorSnapshotRef(req.GetSnapshot(), "snapshot"); err != nil { return nil, err @@ -100,23 +116,41 @@ func (s *Service) TagActorSnapshot(ctx context.Context, req *ateapipb.TagActorSn return tag, nil } +// actorSnapshotTagMutableFields lists the ActorSnapshotTag field paths a client +// may name in an UpdateActorSnapshotTag update_mask. +var actorSnapshotTagMutableFields = mutableFields[*ateapipb.ActorSnapshotTag]{ + "scope": func(dst, src *ateapipb.ActorSnapshotTag) { dst.Scope = src.GetScope() }, +} + func (s *Service) UpdateActorSnapshotTag(ctx context.Context, req *ateapipb.UpdateActorSnapshotTagRequest) (*ateapipb.ActorSnapshotTag, error) { - if errs := resources.ValidateObjectRef(req.GetTag(), field.NewPath("tag")); len(errs) > 0 { - return nil, status.Error(codes.InvalidArgument, errs.ToAggregate().Error()) - } - if err := validateActorSnapshotTagScope(req.GetScope()); err != nil { - return nil, err + if errs := validateUpdateActorSnapshotTagRequest(req); len(errs) > 0 { + return nil, toGRPCStatusError(errs) } - _, _, current, err := s.persistence.GetActorSnapshotByTag(ctx, req.GetTag().GetAtespace(), req.GetTag().GetName()) + in := req.GetTag() + atespace, name := in.GetMetadata().GetAtespace(), in.GetMetadata().GetName() + _, _, current, err := s.persistence.GetActorSnapshotByTag(ctx, atespace, name) if errors.Is(err, store.ErrNotFound) { - return nil, status.Errorf(codes.NotFound, "ActorSnapshot tag %s/%s not found", req.GetTag().GetAtespace(), req.GetTag().GetName()) + return nil, status.Errorf(codes.NotFound, "ActorSnapshot tag %s/%s not found", atespace, name) } if err != nil { return nil, fmt.Errorf("while getting actor snapshot tag: %w", err) } - tag, err := s.persistence.UpdateActorSnapshotTag(ctx, req.GetTag().GetAtespace(), req.GetTag().GetName(), req.GetScope(), current.GetMetadata().GetVersion()) + + // UID and version preconditions. + if uid := in.GetMetadata().GetUid(); uid != "" && uid != current.GetMetadata().GetUid() { + return nil, status.Errorf(codes.Aborted, "ActorSnapshot tag %s/%s has uid %s, not %s", atespace, name, current.GetMetadata().GetUid(), uid) + } + + expectedVersion := current.GetMetadata().GetVersion() + if version := in.GetMetadata().GetVersion(); version != 0 { + expectedVersion = version + } + + applyUpdateMask(current, in, req.GetUpdateMask(), actorSnapshotTagMutableFields) + + updatedTag, err := s.persistence.UpdateActorSnapshotTag(ctx, atespace, name, current.GetScope(), expectedVersion) if errors.Is(err, store.ErrNotFound) { - return nil, status.Errorf(codes.NotFound, "ActorSnapshot tag %s/%s not found", req.GetTag().GetAtespace(), req.GetTag().GetName()) + return nil, status.Errorf(codes.NotFound, "ActorSnapshot tag %s/%s not found", atespace, name) } if errors.Is(err, store.ErrVersionConflict) { return nil, status.Error(codes.Aborted, "concurrent update conflict, please retry") @@ -124,7 +158,28 @@ func (s *Service) UpdateActorSnapshotTag(ctx context.Context, req *ateapipb.Upda if err != nil { return nil, fmt.Errorf("while updating actor snapshot tag: %w", err) } - return tag, nil + return updatedTag, nil +} + +func validateUpdateActorSnapshotTagRequest(req *ateapipb.UpdateActorSnapshotTagRequest) field.ErrorList { + var fldPath *field.Path + var errs field.ErrorList + + tag := req.GetTag() + tagPath := fldPath.Child("tag") + if tag == nil { + return field.ErrorList{field.Required(tagPath, "")} + } + + errs = append(errs, resources.ValidateResourceMetadataRef(tag.GetMetadata(), tagPath.Child("metadata"))...) + + errs = append(errs, validateUpdateMask(req.GetUpdateMask(), actorSnapshotTagMutableFields)...) + + if scope, p := tag.GetScope(), tagPath.Child("scope"); validateActorSnapshotTagScope(scope) != nil { + errs = append(errs, field.NotSupported(p, scope.String(), actorSnapshotTagScopeNames)) + } + + return errs } func (s *Service) DeleteActorSnapshotTag(ctx context.Context, req *ateapipb.DeleteActorSnapshotTagRequest) (*ateapipb.ActorSnapshotTag, error) { @@ -231,11 +286,8 @@ func validateActorSnapshotTag(tag *ateapipb.ActorSnapshotTag, name string) error } func validateActorSnapshotTagScope(scope ateapipb.ActorSnapshotTagScope) error { - switch scope { - case ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_ATESPACE, - ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED: + if slices.Contains(actorSnapshotTagScopes, scope) { return nil - default: - return status.Error(codes.InvalidArgument, "invalid ActorSnapshot tag scope") } + return status.Error(codes.InvalidArgument, "invalid ActorSnapshot tag scope") } diff --git a/cmd/ateapi/internal/controlapi/actor_snapshot_test.go b/cmd/ateapi/internal/controlapi/actor_snapshot_test.go new file mode 100644 index 000000000..fa6f051ed --- /dev/null +++ b/cmd/ateapi/internal/controlapi/actor_snapshot_test.go @@ -0,0 +1,226 @@ +// Copyright 2026 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package controlapi + +import ( + "testing" + + "google.golang.org/protobuf/types/known/fieldmaskpb" + "k8s.io/apimachinery/pkg/util/validation/field" + + "github.com/agent-substrate/substrate/pkg/proto/ateapipb" +) + +func TestValidateUpdateActorSnapshotTagRequest(t *testing.T) { + mutableFields := []string{"scope"} + scopes := []string{ + ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_ATESPACE.String(), + ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED.String(), + } + + tests := []struct { + name string + req *ateapipb.UpdateActorSnapshotTagRequest + wantError field.ErrorList + }{ + { + name: "valid", + req: &ateapipb.UpdateActorSnapshotTagRequest{ + Tag: &ateapipb.ActorSnapshotTag{ + Metadata: &ateapipb.ResourceMetadata{Atespace: "ns1", Name: "tag1"}, + Scope: ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED, + }, + UpdateMask: &fieldmaskpb.FieldMask{Paths: []string{"scope"}}, + }, + wantError: nil, + }, + { + name: "missing tag", + req: &ateapipb.UpdateActorSnapshotTagRequest{UpdateMask: &fieldmaskpb.FieldMask{Paths: []string{"scope"}}}, + wantError: field.ErrorList{field.Required(field.NewPath("tag"), "")}, + }, + { + name: "missing tag.metadata.atespace", + req: &ateapipb.UpdateActorSnapshotTagRequest{ + Tag: &ateapipb.ActorSnapshotTag{ + Metadata: &ateapipb.ResourceMetadata{Name: "tag1"}, + Scope: ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED, + }, + UpdateMask: &fieldmaskpb.FieldMask{Paths: []string{"scope"}}, + }, + wantError: field.ErrorList{field.Required(field.NewPath("tag", "metadata", "atespace"), "")}, + }, + { + name: "invalid tag.metadata.atespace", + req: &ateapipb.UpdateActorSnapshotTagRequest{ + Tag: &ateapipb.ActorSnapshotTag{ + Metadata: &ateapipb.ResourceMetadata{Atespace: "NS1", Name: "tag1"}, + Scope: ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED, + }, + UpdateMask: &fieldmaskpb.FieldMask{Paths: []string{"scope"}}, + }, + wantError: field.ErrorList{field.Invalid(field.NewPath("tag", "metadata", "atespace"), "NS1", "")}, + }, + { + name: "missing tag.metadata.name", + req: &ateapipb.UpdateActorSnapshotTagRequest{ + Tag: &ateapipb.ActorSnapshotTag{ + Metadata: &ateapipb.ResourceMetadata{Atespace: "ns1"}, + Scope: ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED, + }, + UpdateMask: &fieldmaskpb.FieldMask{Paths: []string{"scope"}}, + }, + wantError: field.ErrorList{field.Required(field.NewPath("tag", "metadata", "name"), "")}, + }, + { + name: "invalid tag.metadata.name", + req: &ateapipb.UpdateActorSnapshotTagRequest{ + Tag: &ateapipb.ActorSnapshotTag{ + Metadata: &ateapipb.ResourceMetadata{Atespace: "ns1", Name: "TAG1"}, + Scope: ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED, + }, + UpdateMask: &fieldmaskpb.FieldMask{Paths: []string{"scope"}}, + }, + wantError: field.ErrorList{field.Invalid(field.NewPath("tag", "metadata", "name"), "TAG1", "")}, + }, + { + name: "valid tag.metadata.uid precondition", + req: &ateapipb.UpdateActorSnapshotTagRequest{ + Tag: &ateapipb.ActorSnapshotTag{ + Metadata: &ateapipb.ResourceMetadata{ + Atespace: "ns1", Name: "tag1", Uid: "2a5f8c1e-9b3d-4f7a-8e6c-1d0b4a7f2e93", + }, + Scope: ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED, + }, + UpdateMask: &fieldmaskpb.FieldMask{Paths: []string{"scope"}}, + }, + wantError: nil, + }, + { + name: "invalid tag.metadata.uid precondition", + req: &ateapipb.UpdateActorSnapshotTagRequest{ + Tag: &ateapipb.ActorSnapshotTag{ + Metadata: &ateapipb.ResourceMetadata{Atespace: "ns1", Name: "tag1", Uid: "not-a-uuid"}, + Scope: ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED, + }, + UpdateMask: &fieldmaskpb.FieldMask{Paths: []string{"scope"}}, + }, + wantError: field.ErrorList{field.Invalid(field.NewPath("tag", "metadata", "uid"), "not-a-uuid", "")}, + }, + { + name: "valid tag.metadata.version precondition", + req: &ateapipb.UpdateActorSnapshotTagRequest{ + Tag: &ateapipb.ActorSnapshotTag{ + Metadata: &ateapipb.ResourceMetadata{Atespace: "ns1", Name: "tag1", Version: 7}, + Scope: ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED, + }, + UpdateMask: &fieldmaskpb.FieldMask{Paths: []string{"scope"}}, + }, + wantError: nil, + }, + { + name: "negative tag.metadata.version precondition", + req: &ateapipb.UpdateActorSnapshotTagRequest{ + Tag: &ateapipb.ActorSnapshotTag{ + Metadata: &ateapipb.ResourceMetadata{Atespace: "ns1", Name: "tag1", Version: -1}, + Scope: ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED, + }, + UpdateMask: &fieldmaskpb.FieldMask{Paths: []string{"scope"}}, + }, + wantError: field.ErrorList{field.Invalid(field.NewPath("tag", "metadata", "version"), int64(-1), "")}, + }, + { + name: "missing update_mask", + req: &ateapipb.UpdateActorSnapshotTagRequest{ + Tag: &ateapipb.ActorSnapshotTag{ + Metadata: &ateapipb.ResourceMetadata{Atespace: "ns1", Name: "tag1"}, + Scope: ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED, + }, + }, + wantError: field.ErrorList{field.Required(field.NewPath("update_mask"), "")}, + }, + { + name: "empty update_mask", + req: &ateapipb.UpdateActorSnapshotTagRequest{ + Tag: &ateapipb.ActorSnapshotTag{ + Metadata: &ateapipb.ResourceMetadata{Atespace: "ns1", Name: "tag1"}, + Scope: ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED, + }, + UpdateMask: &fieldmaskpb.FieldMask{}, + }, + wantError: field.ErrorList{field.Required(field.NewPath("update_mask"), "")}, + }, + { + name: "wildcard update_mask", + req: &ateapipb.UpdateActorSnapshotTagRequest{ + Tag: &ateapipb.ActorSnapshotTag{ + Metadata: &ateapipb.ResourceMetadata{Atespace: "ns1", Name: "tag1"}, + Scope: ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED, + }, + UpdateMask: &fieldmaskpb.FieldMask{Paths: []string{"*"}}, + }, + wantError: field.ErrorList{field.NotSupported(field.NewPath("update_mask"), "*", mutableFields)}, + }, + { + name: "output-only field in update_mask", + req: &ateapipb.UpdateActorSnapshotTagRequest{ + Tag: &ateapipb.ActorSnapshotTag{ + Metadata: &ateapipb.ResourceMetadata{Atespace: "ns1", Name: "tag1"}, + Scope: ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED, + }, + UpdateMask: &fieldmaskpb.FieldMask{Paths: []string{"metadata.version"}}, + }, + wantError: field.ErrorList{field.NotSupported(field.NewPath("update_mask"), "metadata.version", mutableFields)}, + }, + { + name: "immutable field in update_mask", + req: &ateapipb.UpdateActorSnapshotTagRequest{ + Tag: &ateapipb.ActorSnapshotTag{ + Metadata: &ateapipb.ResourceMetadata{Atespace: "ns1", Name: "tag1"}, + Scope: ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED, + }, + UpdateMask: &fieldmaskpb.FieldMask{Paths: []string{"snapshot"}}, + }, + wantError: field.ErrorList{field.NotSupported(field.NewPath("update_mask"), "snapshot", mutableFields)}, + }, + { + // The zero value is ATESPACE, so leaving scope unset unpublishes the tag. + name: "unset tag.scope", + req: &ateapipb.UpdateActorSnapshotTagRequest{ + Tag: &ateapipb.ActorSnapshotTag{ + Metadata: &ateapipb.ResourceMetadata{Atespace: "ns1", Name: "tag1"}, + }, + UpdateMask: &fieldmaskpb.FieldMask{Paths: []string{"scope"}}, + }, + wantError: nil, + }, + { + name: "tag.scope outside the enum", + req: &ateapipb.UpdateActorSnapshotTagRequest{ + Tag: &ateapipb.ActorSnapshotTag{ + Metadata: &ateapipb.ResourceMetadata{Atespace: "ns1", Name: "tag1"}, + Scope: ateapipb.ActorSnapshotTagScope(7), + }, + UpdateMask: &fieldmaskpb.FieldMask{Paths: []string{"scope"}}, + }, + wantError: field.ErrorList{field.NotSupported(field.NewPath("tag", "scope"), "7", scopes)}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assertValidateErr(t, validateUpdateActorSnapshotTagRequest(tt.req), tt.wantError) + }) + } +} diff --git a/cmd/ateapi/internal/controlapi/functional_test.go b/cmd/ateapi/internal/controlapi/functional_test.go index 84bb5c92a..967d535d5 100644 --- a/cmd/ateapi/internal/controlapi/functional_test.go +++ b/cmd/ateapi/internal/controlapi/functional_test.go @@ -440,6 +440,44 @@ func createAtespace(t *testing.T, tc *testContext, name string) { } } +// createActorSnapshot seeds an ActorSnapshot in testAtespace directly through +// the store, so tag tests do not need a full resume/suspend lifecycle. +func createActorSnapshot(t *testing.T, tc *testContext, name string) *ateapipb.ObjectRef { + t.Helper() + if _, err := tc.persistence.CreateActorSnapshot(context.Background(), &ateapipb.ActorSnapshot{ + Metadata: &ateapipb.ResourceMetadata{Atespace: testAtespace, Name: name}, + }, "gs://my-bucket/"+name); err != nil { + t.Fatalf("CreateActorSnapshot(%s) failed: %v", name, err) + } + return &ateapipb.ObjectRef{Atespace: testAtespace, Name: name} +} + +// tagActorSnapshot points tagName at snapshotRef with atespace scope. +func tagActorSnapshot(t *testing.T, tc *testContext, snapshotRef *ateapipb.ObjectRef, tagName string) *ateapipb.ActorSnapshotTag { + t.Helper() + tag, err := tc.client.TagActorSnapshot(context.Background(), &ateapipb.TagActorSnapshotRequest{ + Snapshot: &ateapipb.ActorSnapshotRef{Reference: &ateapipb.ActorSnapshotRef_Snapshot{Snapshot: snapshotRef}}, + Tag: &ateapipb.ActorSnapshotTag{ + Metadata: &ateapipb.ResourceMetadata{Atespace: testAtespace, Name: tagName}, + Scope: ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_ATESPACE, + }, + }) + if err != nil { + t.Fatalf("TagActorSnapshot(%s) failed: %v", tagName, err) + } + return tag +} + +// updateActorSnapshotTagScope sets tagName's scope, carrying meta as the +// optional uid/version preconditions. +func updateActorSnapshotTagScope(tc *testContext, tagName string, meta *ateapipb.ResourceMetadata, scope ateapipb.ActorSnapshotTagScope) (*ateapipb.ActorSnapshotTag, error) { + meta.Atespace, meta.Name = testAtespace, tagName + return tc.client.UpdateActorSnapshotTag(context.Background(), &ateapipb.UpdateActorSnapshotTagRequest{ + Tag: &ateapipb.ActorSnapshotTag{Metadata: meta, Scope: scope}, + UpdateMask: &fieldmaskpb.FieldMask{Paths: []string{"scope"}}, + }) +} + const poolLabelKey = "pool" func createTemplateWithContainers(t *testing.T, tc *testContext, ns string, containers []atev1alpha1.Container) { @@ -2109,8 +2147,11 @@ func TestSuspendActor(t *testing.T) { t.Fatalf("cross-atespace CreateActor status = %v, want FailedPrecondition", status.Code(err)) } updated, err := tc.client.UpdateActorSnapshotTag(context.Background(), &ateapipb.UpdateActorSnapshotTagRequest{ - Tag: tagRef, - Scope: ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED, + Tag: &ateapipb.ActorSnapshotTag{ + Metadata: &ateapipb.ResourceMetadata{Atespace: tagRef.GetAtespace(), Name: tagRef.GetName()}, + Scope: ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED, + }, + UpdateMask: &fieldmaskpb.FieldMask{Paths: []string{"scope"}}, }) if err != nil || updated.GetScope() != ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED { t.Fatalf("UpdateActorSnapshotTag = (%v, %v), want published", updated, err) @@ -2441,6 +2482,173 @@ func TestUpdateActor_NotFound(t *testing.T) { assertGrpcError(t, err, codes.NotFound, "Actor test-atespace/does-not-exist not found") } +func TestUpdateActorSnapshotTag_Success(t *testing.T) { + ns := namespaceForTest("ns-update-tag") + tc := setupTest(t, ns) + defer tc.cleanup() + + createTemplate(t, tc, ns) + + ctx := context.Background() + const snapshotName, tagName = "snapshot-1", "before-upgrade" + snapshotRef := createActorSnapshot(t, tc, snapshotName) + tagActorSnapshot(t, tc, snapshotRef, tagName) + + updateResp, err := tc.client.UpdateActorSnapshotTag(ctx, &ateapipb.UpdateActorSnapshotTagRequest{ + Tag: &ateapipb.ActorSnapshotTag{ + Metadata: &ateapipb.ResourceMetadata{Atespace: testAtespace, Name: tagName}, + Scope: ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED, + Snapshot: &ateapipb.ObjectRef{Atespace: testAtespace, Name: "some-other-snapshot"}, + }, + UpdateMask: &fieldmaskpb.FieldMask{Paths: []string{"scope"}}, + }) + if err != nil { + t.Fatalf("UpdateActorSnapshotTag failed: %v", err) + } + + wantTag := &ateapipb.ActorSnapshotTag{ + Metadata: &ateapipb.ResourceMetadata{Atespace: testAtespace, Name: tagName, Version: 2}, + Snapshot: snapshotRef, + Scope: ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED, + } + if diff := cmp.Diff(wantTag, updateResp, protocmp.Transform(), ignoreUID, ignoreTimestamps); diff != "" { + t.Errorf("UpdateActorSnapshotTag response mismatch (-want +got):\n%s", diff) + } + + _, _, storedTag, err := tc.persistence.GetActorSnapshotByTag(ctx, testAtespace, tagName) + if err != nil { + t.Fatalf("GetActorSnapshotByTag failed: %v", err) + } + if diff := cmp.Diff(wantTag, storedTag, protocmp.Transform(), ignoreUID, ignoreTimestamps); diff != "" { + t.Errorf("stored tag mismatch after UpdateActorSnapshotTag (-want +got):\n%s", diff) + } +} + +// TestUpdateActorSnapshotTag_Preconditions verifies the optional version and uid +// guards carried in the tag's metadata. +func TestUpdateActorSnapshotTag_Preconditions(t *testing.T) { + ns := namespaceForTest("ns-update-tag-preconditions") + tc := setupTest(t, ns) + defer tc.cleanup() + + createTemplate(t, tc, ns) + + ctx := context.Background() + const snapshotName, tagName = "snapshot-1", "before-upgrade" + snapshotRef := createActorSnapshot(t, tc, snapshotName) + + // Each call to update() flips the scope, so every accepted update is an + // observable write that bumps the version. + update := func(meta *ateapipb.ResourceMetadata, scope ateapipb.ActorSnapshotTagScope) (*ateapipb.ActorSnapshotTag, error) { + return updateActorSnapshotTagScope(tc, tagName, meta, scope) + } + + // Delete and recreate the same atespace/name tag, so the first lifecycle's + // uid becomes stale. + staleUID := tagActorSnapshot(t, tc, snapshotRef, tagName).GetMetadata().GetUid() + if _, err := tc.client.DeleteActorSnapshotTag(ctx, &ateapipb.DeleteActorSnapshotTagRequest{ + Tag: &ateapipb.ObjectRef{Atespace: testAtespace, Name: tagName}, + }); err != nil { + t.Fatalf("DeleteActorSnapshotTag failed: %v", err) + } + + tagged := tagActorSnapshot(t, tc, snapshotRef, tagName) + staleVersion := tagged.GetMetadata().GetVersion() + uid := tagged.GetMetadata().GetUid() + if uid == staleUID { + t.Fatalf("recreated tag reused uid %s, want a fresh one", uid) + } + // The uid from the deleted lifecycle must be rejected, even though the + // atespace/name it was observed under still resolves. + _, err := update(&ateapipb.ResourceMetadata{Uid: staleUID}, ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED) + assertGrpcError(t, err, codes.Aborted, fmt.Sprintf("ActorSnapshot tag %s/%s has uid %s, not %s", testAtespace, tagName, uid, staleUID)) + + // An unguarded update is last-writer-wins, and moves the tag past the + // version observed above. + unguarded, err := update(&ateapipb.ResourceMetadata{}, ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED) + if err != nil { + t.Fatalf("UpdateActorSnapshotTag(no guards) failed: %v", err) + } + currentVersion := unguarded.GetMetadata().GetVersion() + if currentVersion <= staleVersion { + t.Fatalf("version = %d, want greater than %d after an update", currentVersion, staleVersion) + } + if got, want := unguarded.GetScope(), ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED; got != want { + t.Errorf("scope = %v, want %v", got, want) + } + + // The version observed before that write is now stale: rejected rather than + // silently overwriting the concurrent change. + _, err = update(&ateapipb.ResourceMetadata{Version: staleVersion}, ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_ATESPACE) + assertGrpcError(t, err, codes.Aborted, "concurrent update conflict, please retry") + + // Both uid and version matching the observed state: the update goes through. + updated, err := update(&ateapipb.ResourceMetadata{Uid: uid, Version: currentVersion}, ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_ATESPACE) + if err != nil { + t.Fatalf("UpdateActorSnapshotTag(matching guards) failed: %v", err) + } + if got, want := updated.GetScope(), ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_ATESPACE; got != want { + t.Errorf("scope = %v, want %v", got, want) + } + if updated.GetMetadata().GetVersion() <= currentVersion { + t.Errorf("version = %d, want greater than %d", updated.GetMetadata().GetVersion(), currentVersion) + } + + // The guard the client just satisfied is now stale in turn. + _, err = update(&ateapipb.ResourceMetadata{Version: currentVersion}, ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED) + assertGrpcError(t, err, codes.Aborted, "concurrent update conflict, please retry") +} + +// TestUpdateActorSnapshotTag_ClearsMaskedField verifies that a masked field +// left unset on the request resets to its default. ATESPACE is the zero value +// of ActorSnapshotTagScope, so masking scope without setting it unpublishes the +// tag. +func TestUpdateActorSnapshotTag_ClearsMaskedField(t *testing.T) { + ns := namespaceForTest("ns-update-tag-clear") + tc := setupTest(t, ns) + defer tc.cleanup() + + createTemplate(t, tc, ns) + + const tagName = "before-upgrade" + snapshotRef := createActorSnapshot(t, tc, "snapshot-1") + tagActorSnapshot(t, tc, snapshotRef, tagName) + + published, err := updateActorSnapshotTagScope(tc, tagName, &ateapipb.ResourceMetadata{}, ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED) + if err != nil { + t.Fatalf("UpdateActorSnapshotTag(publish) failed: %v", err) + } + if got, want := published.GetScope(), ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED; got != want { + t.Fatalf("scope = %v, want %v", got, want) + } + + cleared, err := tc.client.UpdateActorSnapshotTag(context.Background(), &ateapipb.UpdateActorSnapshotTagRequest{ + Tag: &ateapipb.ActorSnapshotTag{Metadata: &ateapipb.ResourceMetadata{Atespace: testAtespace, Name: tagName}}, + UpdateMask: &fieldmaskpb.FieldMask{Paths: []string{"scope"}}, + }) + if err != nil { + t.Fatalf("UpdateActorSnapshotTag(masked clear) failed: %v", err) + } + if got, want := cleared.GetScope(), ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_ATESPACE; got != want { + t.Errorf("scope = %v, want %v after masked clear", got, want) + } +} + +func TestUpdateActorSnapshotTag_NotFound(t *testing.T) { + ns := namespaceForTest("ns-update-tag-notfound") + tc := setupTest(t, ns) + defer tc.cleanup() + + _, err := tc.client.UpdateActorSnapshotTag(context.Background(), &ateapipb.UpdateActorSnapshotTagRequest{ + Tag: &ateapipb.ActorSnapshotTag{ + Metadata: &ateapipb.ResourceMetadata{Atespace: testAtespace, Name: "does-not-exist"}, + Scope: ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED, + }, + UpdateMask: &fieldmaskpb.FieldMask{Paths: []string{"scope"}}, + }) + assertGrpcError(t, err, codes.NotFound, "ActorSnapshot tag test-atespace/does-not-exist not found") +} + // TestResumeActor_ReleasesStaleWorkerWhenPoolBecomesIneligible verifies that // a worker claimed by a failed resume attempt is released back to the free // pool if, by the next resume attempt, the actor's worker_selector has diff --git a/cmd/ateapi/internal/controlapi/update_actor.go b/cmd/ateapi/internal/controlapi/update_actor.go index f6e342bd5..c6faefab2 100644 --- a/cmd/ateapi/internal/controlapi/update_actor.go +++ b/cmd/ateapi/internal/controlapi/update_actor.go @@ -18,23 +18,18 @@ import ( "context" "errors" "fmt" - "maps" - "slices" "github.com/agent-substrate/substrate/cmd/ateapi/internal/store" "github.com/agent-substrate/substrate/internal/resources" "github.com/agent-substrate/substrate/pkg/proto/ateapipb" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" - "google.golang.org/protobuf/types/known/fieldmaskpb" "k8s.io/apimachinery/pkg/util/validation/field" ) -// actorMutableFields maps the Actor field paths a client may name in an -// UpdateActor update_mask to the setter that applies them. -// Every other field is either output-only (server-managed), immutable or -// unsupported (e.g. '*'), and naming one is an error. -var actorMutableFields = map[string]func(dst, src *ateapipb.Actor){ +// actorMutableFields lists the Actor field paths a client may name in an +// UpdateActor update_mask. +var actorMutableFields = mutableFields[*ateapipb.Actor]{ "worker_selector": func(dst, src *ateapipb.Actor) { dst.WorkerSelector = src.GetWorkerSelector() }, } @@ -64,7 +59,7 @@ func (s *Service) UpdateActor(ctx context.Context, req *ateapipb.UpdateActorRequ expectedVersion = version } - applyActorUpdateMask(actor, in, req.GetUpdateMask()) + applyUpdateMask(actor, in, req.GetUpdateMask(), actorMutableFields) updated, err := s.persistence.UpdateActor(ctx, actor, expectedVersion) if err != nil { @@ -78,18 +73,6 @@ func (s *Service) UpdateActor(ctx context.Context, req *ateapipb.UpdateActorRequ return updated, nil } -// applyActorUpdateMask copies the masked fields from src onto dst. Fields set on -// src but absent from the mask are ignored, and a masked field that is unset on -// src is cleared on dst. -func applyActorUpdateMask(dst, src *ateapipb.Actor, mask *fieldmaskpb.FieldMask) { - for _, path := range mask.GetPaths() { - apply, ok := actorMutableFields[path] - if ok { - apply(dst, src) - } - } -} - func validateUpdateActorRequest(req *ateapipb.UpdateActorRequest) field.ErrorList { var fldPath *field.Path var errs field.ErrorList @@ -100,40 +83,9 @@ func validateUpdateActorRequest(req *ateapipb.UpdateActorRequest) field.ErrorLis return field.ErrorList{field.Required(actorPath, "")} } - // atespace and name identify the resource to update; uid and version are - // optional preconditions. - metaPath := actorPath.Child("metadata") - if atespace, p := actor.GetMetadata().GetAtespace(), metaPath.Child("atespace"); atespace == "" { - errs = append(errs, field.Required(p, "")) - } else { - errs = append(errs, resources.ValidateResourceName(atespace, p)...) - } - - if name, p := actor.GetMetadata().GetName(), metaPath.Child("name"); name == "" { - errs = append(errs, field.Required(p, "")) - } else { - errs = append(errs, resources.ValidateResourceName(name, p)...) - } + errs = append(errs, resources.ValidateResourceMetadataRef(actor.GetMetadata(), actorPath.Child("metadata"))...) - if uid, p := actor.GetMetadata().GetUid(), metaPath.Child("uid"); uid != "" { - errs = append(errs, resources.ValidateUUID(uid, p)...) - } - - if version, p := actor.GetMetadata().GetVersion(), metaPath.Child("version"); version < 0 { - errs = append(errs, field.Invalid(p, version, "must not be negative")) - } - - maskPath := fldPath.Child("update_mask") - if paths := req.GetUpdateMask().GetPaths(); len(paths) == 0 { - errs = append(errs, field.Required(maskPath, "must name at least one field to update")) - } else { - supportedMutableFields := slices.Sorted(maps.Keys(actorMutableFields)) - for _, path := range paths { - if _, ok := actorMutableFields[path]; !ok { - errs = append(errs, field.NotSupported(maskPath, path, supportedMutableFields)) - } - } - } + errs = append(errs, validateUpdateMask(req.GetUpdateMask(), actorMutableFields)...) if selector := actor.GetWorkerSelector(); selector != nil { errs = append(errs, validateSelector(selector, actorPath.Child("worker_selector"))...) diff --git a/cmd/ateapi/internal/controlapi/update_actor_test.go b/cmd/ateapi/internal/controlapi/update_actor_test.go index 5a2772877..2d5e1335d 100644 --- a/cmd/ateapi/internal/controlapi/update_actor_test.go +++ b/cmd/ateapi/internal/controlapi/update_actor_test.go @@ -18,11 +18,9 @@ import ( "context" "testing" - "github.com/google/go-cmp/cmp" "go.opentelemetry.io/otel/attribute" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" - "google.golang.org/protobuf/testing/protocmp" "google.golang.org/protobuf/types/known/fieldmaskpb" "k8s.io/apimachinery/pkg/util/validation/field" @@ -132,51 +130,6 @@ func TestValidateUpdateActorRequest(t *testing.T) { } } -func TestApplyActorUpdateMask(t *testing.T) { - selector := &ateapipb.Selector{MatchLabels: map[string]string{"tier": "paid"}} - - tests := []struct { - name string - src *ateapipb.Actor - dst *ateapipb.Actor - paths []string - want *ateapipb.Actor - }{{ - name: "sets a masked field", - src: &ateapipb.Actor{WorkerSelector: selector}, - dst: &ateapipb.Actor{}, - paths: []string{"worker_selector"}, - want: &ateapipb.Actor{WorkerSelector: selector}, - }, { - name: "clears a masked field left unset on src", - src: &ateapipb.Actor{}, - dst: &ateapipb.Actor{WorkerSelector: selector}, - paths: []string{"worker_selector"}, - want: &ateapipb.Actor{}, - }, { - name: "ignores fields set on src but absent from the mask", - src: &ateapipb.Actor{Status: ateapipb.Actor_STATUS_RUNNING, WorkerSelector: selector}, - dst: &ateapipb.Actor{Status: ateapipb.Actor_STATUS_SUSPENDED}, - paths: []string{"worker_selector"}, - want: &ateapipb.Actor{Status: ateapipb.Actor_STATUS_SUSPENDED, WorkerSelector: selector}, - }, { - // Unreachable through the RPC, which rejects the path during validation. - name: "skips a path outside the mutable set", - src: &ateapipb.Actor{Status: ateapipb.Actor_STATUS_RUNNING}, - dst: &ateapipb.Actor{Status: ateapipb.Actor_STATUS_SUSPENDED}, - paths: []string{"status"}, - want: &ateapipb.Actor{Status: ateapipb.Actor_STATUS_SUSPENDED}, - }} - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - applyActorUpdateMask(tt.dst, tt.src, &fieldmaskpb.FieldMask{Paths: tt.paths}) - if diff := cmp.Diff(tt.want, tt.dst, protocmp.Transform()); diff != "" { - t.Errorf("actor mismatch (-want +got):\n%s", diff) - } - }) - } -} - // TestUpdateActor_ClearsMaskedField verifies that naming a field in the mask // while leaving it unset on the request clears it, which is the whole point of // requiring an explicit mask. diff --git a/cmd/ateapi/internal/controlapi/update_mask.go b/cmd/ateapi/internal/controlapi/update_mask.go new file mode 100644 index 000000000..96e52665d --- /dev/null +++ b/cmd/ateapi/internal/controlapi/update_mask.go @@ -0,0 +1,60 @@ +// Copyright 2026 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package controlapi + +import ( + "maps" + "slices" + + "google.golang.org/protobuf/types/known/fieldmaskpb" + "k8s.io/apimachinery/pkg/util/validation/field" +) + +// mutableFields maps the field paths a client may set in an update_mask to its update function. +// Every other field is either output-only (server-managed), immutable or +// unsupported (e.g. '*'), and setting one is an error. +type mutableFields[T any] map[string]func(dst, src T) + +// applyUpdateMask copies the masked fields from src onto dst. Fields set on src +// but absent from the mask are ignored, and a masked field that is unset on src +// is cleared on dst. Paths that set no mutable field are skipped. +func applyUpdateMask[T any](dst, src T, mask *fieldmaskpb.FieldMask, fields mutableFields[T]) { + for _, path := range mask.GetPaths() { + if apply, ok := fields[path]; ok { + apply(dst, src) + } + } +} + +// updateMaskPath is the update_mask request field that's required for +// update requests. +var updateMaskPath = field.NewPath("update_mask") + +// validateUpdateMask checks that mask sets at least one field and that every +// path it sets is mutable. +func validateUpdateMask[T any](mask *fieldmaskpb.FieldMask, fields mutableFields[T]) field.ErrorList { + paths := mask.GetPaths() + if len(paths) == 0 { + return field.ErrorList{field.Required(updateMaskPath, "must name at least one field to update")} + } + var errs field.ErrorList + supported := slices.Sorted(maps.Keys(fields)) + for _, path := range paths { + if _, ok := fields[path]; !ok { + errs = append(errs, field.NotSupported(updateMaskPath, path, supported)) + } + } + return errs +} diff --git a/cmd/ateapi/internal/controlapi/update_mask_test.go b/cmd/ateapi/internal/controlapi/update_mask_test.go new file mode 100644 index 000000000..6ae1b36f1 --- /dev/null +++ b/cmd/ateapi/internal/controlapi/update_mask_test.go @@ -0,0 +1,184 @@ +// Copyright 2026 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package controlapi + +import ( + "testing" + + "github.com/google/go-cmp/cmp" + "google.golang.org/protobuf/types/known/fieldmaskpb" + "k8s.io/apimachinery/pkg/util/validation/field" +) + +// testResource stands in for a generic message with two mutable fields and one +// the client may not touch. +type testResource struct { + Mutable string + Other string + Immutable string +} + +var resourceMutableFields = mutableFields[*testResource]{ + "mutable": func(dst, src *testResource) { dst.Mutable = src.Mutable }, + "other": func(dst, src *testResource) { dst.Other = src.Other }, +} + +func TestApplyUpdateMask(t *testing.T) { + tests := []struct { + name string + src *testResource + dst *testResource + paths []string + want *testResource + }{ + { + name: "sets a masked field", + src: &testResource{Mutable: "new"}, + dst: &testResource{}, + paths: []string{"mutable"}, + want: &testResource{Mutable: "new"}, + }, + { + name: "overwrites a masked field", + src: &testResource{Mutable: "new"}, + dst: &testResource{Mutable: "old"}, + paths: []string{"mutable"}, + want: &testResource{Mutable: "new"}, + }, + { + name: "clears a masked field left unset on src", + src: &testResource{}, + dst: &testResource{Mutable: "old"}, + paths: []string{"mutable"}, + want: &testResource{}, + }, + { + name: "ignores fields set on src but absent from the mask", + src: &testResource{Mutable: "new", Other: "ignored"}, + dst: &testResource{Mutable: "old", Other: "keep"}, + paths: []string{"mutable"}, + want: &testResource{Mutable: "new", Other: "keep"}, + }, + { + name: "applies every masked field", + src: &testResource{Mutable: "new-mutable", Other: "new-other"}, + dst: &testResource{Mutable: "old", Other: "old"}, + paths: []string{"mutable", "other"}, + want: &testResource{Mutable: "new-mutable", Other: "new-other"}, + }, + { + name: "skips a path outside the mutable set", + src: &testResource{Immutable: "ignored"}, + dst: &testResource{Immutable: "keep"}, + paths: []string{"immutable-path-is-ignored"}, + want: &testResource{Immutable: "keep"}, + }, + { + name: "leaves dst untouched for a nil mask", + src: &testResource{Mutable: "new"}, + dst: &testResource{Mutable: "old"}, + want: &testResource{Mutable: "old"}, + }, + { + name: "applies a repeated path once per occurrence", + src: &testResource{Mutable: "new"}, + dst: &testResource{}, + paths: []string{"mutable", "mutable"}, + want: &testResource{Mutable: "new"}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var mask *fieldmaskpb.FieldMask + if tt.paths != nil { + mask = &fieldmaskpb.FieldMask{Paths: tt.paths} + } + // applyUpdateMask writes to dst in place, so we make a copy + // of the inputs to check for unwanted mutations to src. + src := *tt.src + + applyUpdateMask(tt.dst, tt.src, mask, resourceMutableFields) + if diff := cmp.Diff(tt.want, tt.dst); diff != "" { + t.Errorf("applyUpdateMask(%+v, %+v, %v) dst mismatch (-want +got):\n%s", *tt.dst, src, tt.paths, diff) + } + if diff := cmp.Diff(&src, tt.src); diff != "" { + t.Errorf("applyUpdateMask(%+v, %+v, %v) mutated src (-want +got):\n%s", *tt.dst, src, tt.paths, diff) + } + }) + } +} + +func TestValidateUpdateMask(t *testing.T) { + // Errors are always reported against the request's update_mask field. + fieldPath := field.NewPath("update_mask") + supported := []string{"mutable", "other"} + + tests := []struct { + name string + mask *fieldmaskpb.FieldMask + want field.ErrorList + }{ + { + name: "nil mask", + want: field.ErrorList{field.Required(fieldPath, "")}, + }, + { + name: "empty mask", + mask: &fieldmaskpb.FieldMask{}, + want: field.ErrorList{field.Required(fieldPath, "")}, + }, + { + name: "single mutable path", + mask: &fieldmaskpb.FieldMask{Paths: []string{"mutable"}}, + }, + { + name: "every mutable path", + mask: &fieldmaskpb.FieldMask{Paths: []string{"mutable", "other"}}, + }, + { + name: "wildcard", + mask: &fieldmaskpb.FieldMask{Paths: []string{"*"}}, + want: field.ErrorList{field.NotSupported(fieldPath, "*", supported)}, + }, + { + name: "path outside the mutable set", + mask: &fieldmaskpb.FieldMask{Paths: []string{"immutable"}}, + want: field.ErrorList{field.NotSupported(fieldPath, "immutable", supported)}, + }, + { + name: "nested path under a mutable field", + mask: &fieldmaskpb.FieldMask{Paths: []string{"mutable.nested"}}, + want: field.ErrorList{field.NotSupported(fieldPath, "mutable.nested", supported)}, + }, + { + name: "empty path", + mask: &fieldmaskpb.FieldMask{Paths: []string{""}}, + want: field.ErrorList{field.NotSupported(fieldPath, "", supported)}, + }, + { + name: "reports every unsupported path", + mask: &fieldmaskpb.FieldMask{Paths: []string{"immutable", "mutable", "unknown"}}, + want: field.ErrorList{ + field.NotSupported(fieldPath, "immutable", supported), + field.NotSupported(fieldPath, "unknown", supported), + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assertValidateErr(t, validateUpdateMask(tt.mask, resourceMutableFields), tt.want) + }) + } +} diff --git a/cmd/kubectl-ate/internal/cmd/actor_snapshots.go b/cmd/kubectl-ate/internal/cmd/actor_snapshots.go index fd50d18ef..7f6a8e834 100644 --- a/cmd/kubectl-ate/internal/cmd/actor_snapshots.go +++ b/cmd/kubectl-ate/internal/cmd/actor_snapshots.go @@ -22,6 +22,7 @@ import ( "github.com/agent-substrate/substrate/internal/ateclient" "github.com/agent-substrate/substrate/pkg/proto/ateapipb" "github.com/spf13/cobra" + "google.golang.org/protobuf/types/known/fieldmaskpb" ) var ( @@ -144,8 +145,11 @@ var updateActorSnapshotTagCmd = &cobra.Command{ defer client.Close() resp, err := client.UpdateActorSnapshotTag(ctx, &ateapipb.UpdateActorSnapshotTagRequest{ - Tag: &ateapipb.ObjectRef{Atespace: updateTagAtespaceFlag, Name: args[0]}, - Scope: scope, + Tag: &ateapipb.ActorSnapshotTag{ + Metadata: &ateapipb.ResourceMetadata{Atespace: updateTagAtespaceFlag, Name: args[0]}, + Scope: scope, + }, + UpdateMask: &fieldmaskpb.FieldMask{Paths: []string{"scope"}}, }) if err != nil { return fmt.Errorf("failed to update actor snapshot tag: %w", err) diff --git a/internal/e2e/suites/demo/demo_test.go b/internal/e2e/suites/demo/demo_test.go index dcb910688..ca893a879 100644 --- a/internal/e2e/suites/demo/demo_test.go +++ b/internal/e2e/suites/demo/demo_test.go @@ -29,6 +29,7 @@ import ( "github.com/agent-substrate/substrate/internal/resources" "github.com/agent-substrate/substrate/pkg/api/v1alpha1" "github.com/agent-substrate/substrate/pkg/proto/ateapipb" + "google.golang.org/protobuf/types/known/fieldmaskpb" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" @@ -165,7 +166,11 @@ func TestActorSnapshotLifecycle(t *testing.T) { t.Fatalf("failed to tag ActorSnapshot: %v", err) } if _, err := clients.SubstrateAPI.UpdateActorSnapshotTag(ctx, &ateapipb.UpdateActorSnapshotTagRequest{ - Tag: tagRef, Scope: ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED, + Tag: &ateapipb.ActorSnapshotTag{ + Metadata: &ateapipb.ResourceMetadata{Atespace: tagRef.GetAtespace(), Name: tagRef.GetName()}, + Scope: ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED, + }, + UpdateMask: &fieldmaskpb.FieldMask{Paths: []string{"scope"}}, }); err != nil { t.Fatalf("failed to publish ActorSnapshot tag: %v", err) } diff --git a/internal/resources/validate.go b/internal/resources/validate.go index 09c985df7..ef02f889c 100644 --- a/internal/resources/validate.go +++ b/internal/resources/validate.go @@ -71,6 +71,38 @@ func ValidateObjectRef(ref *ateapipb.ObjectRef, fldPath *field.Path) field.Error return errs } +// ValidateResourceMetadataRef checks the metadata a mutating request uses to +// name the resource it acts on: atespace and name identify the resource and +// are required, while uid and version are optional preconditions. It does not +// check the server-managed timestamps, which clients may not set. Unlike +// ValidateObjectRef, nil metadata is an error rather than a no-op: a request +// that names no resource cannot be served. +func ValidateResourceMetadataRef(meta *ateapipb.ResourceMetadata, fldPath *field.Path) field.ErrorList { + var errs field.ErrorList + + if val, fldPath := meta.GetAtespace(), fldPath.Child("atespace"); val == "" { + errs = append(errs, field.Required(fldPath, "")) + } else { + errs = append(errs, ValidateResourceName(val, fldPath)...) + } + + if val, fldPath := meta.GetName(), fldPath.Child("name"); val == "" { + errs = append(errs, field.Required(fldPath, "")) + } else { + errs = append(errs, ValidateResourceName(val, fldPath)...) + } + + if val, fldPath := meta.GetUid(), fldPath.Child("uid"); val != "" { + errs = append(errs, ValidateUUID(val, fldPath)...) + } + + if val, fldPath := meta.GetVersion(), fldPath.Child("version"); val < 0 { + errs = append(errs, field.Invalid(fldPath, val, "must not be negative")) + } + + return errs +} + // ValidateGlobalObjectRef checks that a reference to a global-scoped resource is // well-formed: its atespace must be empty (global resources do not belong to an // atespace) and its name must be a valid resource name. It does not check that diff --git a/internal/resources/validate_test.go b/internal/resources/validate_test.go index b25de1d55..cfa08550f 100644 --- a/internal/resources/validate_test.go +++ b/internal/resources/validate_test.go @@ -90,6 +90,70 @@ func TestValidateObjectRef(t *testing.T) { } } +func TestValidateResourceMetadataRef(t *testing.T) { + const uid = "8bf5b1a2-3c4d-4e5f-8a9b-0c1d2e3f4a5b" + tests := []struct { + name string + input *ateapipb.ResourceMetadata + wantError field.ErrorList + }{ + { + name: "valid without preconditions", + input: &ateapipb.ResourceMetadata{Atespace: "ns1", Name: "id1"}, + wantError: nil, + }, + { + name: "valid with preconditions", + input: &ateapipb.ResourceMetadata{Atespace: "ns1", Name: "id1", Uid: uid, Version: 7}, + wantError: nil, + }, + { + name: "nil metadata", + input: nil, + wantError: field.ErrorList{ + field.Required(field.NewPath("path", "atespace"), ""), + field.Required(field.NewPath("path", "name"), ""), + }, + }, + { + name: "missing atespace", + input: &ateapipb.ResourceMetadata{Name: "id1"}, + wantError: field.ErrorList{field.Required(field.NewPath("path", "atespace"), "")}, + }, + { + name: "invalid atespace", + input: &ateapipb.ResourceMetadata{Atespace: "NS1", Name: "id1"}, + wantError: field.ErrorList{field.Invalid(field.NewPath("path", "atespace"), "NS1", "")}, + }, + { + name: "missing name", + input: &ateapipb.ResourceMetadata{Atespace: "ns1"}, + wantError: field.ErrorList{field.Required(field.NewPath("path", "name"), "")}, + }, + { + name: "invalid name", + input: &ateapipb.ResourceMetadata{Atespace: "ns1", Name: "ID1"}, + wantError: field.ErrorList{field.Invalid(field.NewPath("path", "name"), "ID1", "")}, + }, + { + name: "invalid uid", + input: &ateapipb.ResourceMetadata{Atespace: "ns1", Name: "id1", Uid: "not-a-uuid"}, + wantError: field.ErrorList{field.Invalid(field.NewPath("path", "uid"), "not-a-uuid", "")}, + }, + { + name: "negative version", + input: &ateapipb.ResourceMetadata{Atespace: "ns1", Name: "id1", Version: -1}, + wantError: field.ErrorList{field.Invalid(field.NewPath("path", "version"), int64(-1), "")}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := ValidateResourceMetadataRef(tt.input, field.NewPath("path")) + field.ErrorMatcher{}.ByType().ByField().ByValue().Test(t, tt.wantError, got) + }) + } +} + func TestValidateGlobalObjectRef(t *testing.T) { tests := []struct { name string diff --git a/pkg/proto/ateapipb/ateapi.pb.go b/pkg/proto/ateapipb/ateapi.pb.go index fdae6906f..dc8cdf6b3 100644 --- a/pkg/proto/ateapipb/ateapi.pb.go +++ b/pkg/proto/ateapipb/ateapi.pb.go @@ -2086,10 +2086,21 @@ func (x *TagActorSnapshotRequest) GetTag() *ActorSnapshotTag { return nil } +// Request to update mutable fields on an existing ActorSnapshotTag. +// The tag keeps its address: the snapshot it points at cannot be changed. type UpdateActorSnapshotTagRequest struct { - state protoimpl.MessageState `protogen:"open.v1"` - Tag *ObjectRef `protobuf:"bytes,1,opt,name=tag,proto3" json:"tag,omitempty"` - Scope ActorSnapshotTagScope `protobuf:"varint,2,opt,name=scope,proto3,enum=ateapi.ActorSnapshotTagScope" json:"scope,omitempty"` + state protoimpl.MessageState `protogen:"open.v1"` + // The tag to update. + // tag.metadata.atespace and tag.metadata.name identify which resource to + // update. + // tag.metadata.version and tag.metadata.uid are optional preconditions + // and zero values skip the check. + Tag *ActorSnapshotTag `protobuf:"bytes,1,opt,name=tag,proto3" json:"tag,omitempty"` + // The set of fields to update. Required. + // + // Only the following fields are supported: + // - scope + UpdateMask *fieldmaskpb.FieldMask `protobuf:"bytes,2,opt,name=update_mask,json=updateMask,proto3" json:"update_mask,omitempty"` unknownFields protoimpl.UnknownFields sizeCache protoimpl.SizeCache } @@ -2124,18 +2135,18 @@ func (*UpdateActorSnapshotTagRequest) Descriptor() ([]byte, []int) { return file_ateapi_proto_rawDescGZIP(), []int{30} } -func (x *UpdateActorSnapshotTagRequest) GetTag() *ObjectRef { +func (x *UpdateActorSnapshotTagRequest) GetTag() *ActorSnapshotTag { if x != nil { return x.Tag } return nil } -func (x *UpdateActorSnapshotTagRequest) GetScope() ActorSnapshotTagScope { +func (x *UpdateActorSnapshotTagRequest) GetUpdateMask() *fieldmaskpb.FieldMask { if x != nil { - return x.Scope + return x.UpdateMask } - return ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_ATESPACE + return nil } type DeleteActorSnapshotTagRequest struct { @@ -3103,10 +3114,11 @@ const file_ateapi_proto_rawDesc = "" + "\x0fnext_page_token\x18\x02 \x01(\tR\rnextPageToken\"{\n" + "\x17TagActorSnapshotRequest\x124\n" + "\bsnapshot\x18\x01 \x01(\v2\x18.ateapi.ActorSnapshotRefR\bsnapshot\x12*\n" + - "\x03tag\x18\x02 \x01(\v2\x18.ateapi.ActorSnapshotTagR\x03tag\"y\n" + - "\x1dUpdateActorSnapshotTagRequest\x12#\n" + - "\x03tag\x18\x01 \x01(\v2\x11.ateapi.ObjectRefR\x03tag\x123\n" + - "\x05scope\x18\x02 \x01(\x0e2\x1d.ateapi.ActorSnapshotTagScopeR\x05scope\"D\n" + + "\x03tag\x18\x02 \x01(\v2\x18.ateapi.ActorSnapshotTagR\x03tag\"\x88\x01\n" + + "\x1dUpdateActorSnapshotTagRequest\x12*\n" + + "\x03tag\x18\x01 \x01(\v2\x18.ateapi.ActorSnapshotTagR\x03tag\x12;\n" + + "\vupdate_mask\x18\x02 \x01(\v2\x1a.google.protobuf.FieldMaskR\n" + + "updateMask\"D\n" + "\x1dDeleteActorSnapshotTagRequest\x12#\n" + "\x03tag\x18\x01 \x01(\v2\x11.ateapi.ObjectRefR\x03tag\"P\n" + "\x12ListWorkersRequest\x12\x1b\n" + @@ -3320,8 +3332,8 @@ var file_ateapi_proto_depIdxs = []int32{ 11, // 37: ateapi.ListActorSnapshotsResponse.snapshots:type_name -> ateapi.ActorSnapshot 15, // 38: ateapi.TagActorSnapshotRequest.snapshot:type_name -> ateapi.ActorSnapshotRef 12, // 39: ateapi.TagActorSnapshotRequest.tag:type_name -> ateapi.ActorSnapshotTag - 14, // 40: ateapi.UpdateActorSnapshotTagRequest.tag:type_name -> ateapi.ObjectRef - 1, // 41: ateapi.UpdateActorSnapshotTagRequest.scope:type_name -> ateapi.ActorSnapshotTagScope + 12, // 40: ateapi.UpdateActorSnapshotTagRequest.tag:type_name -> ateapi.ActorSnapshotTag + 53, // 41: ateapi.UpdateActorSnapshotTagRequest.update_mask:type_name -> google.protobuf.FieldMask 14, // 42: ateapi.DeleteActorSnapshotTagRequest.tag:type_name -> ateapi.ObjectRef 41, // 43: ateapi.ListWorkersResponse.workers:type_name -> ateapi.Worker 9, // 44: ateapi.ListActorsResponse.actors:type_name -> ateapi.Actor diff --git a/pkg/proto/ateapipb/ateapi.proto b/pkg/proto/ateapipb/ateapi.proto index bc576cfc9..6fa2044c1 100644 --- a/pkg/proto/ateapipb/ateapi.proto +++ b/pkg/proto/ateapipb/ateapi.proto @@ -385,9 +385,21 @@ message TagActorSnapshotRequest { ActorSnapshotTag tag = 2; } +// Request to update mutable fields on an existing ActorSnapshotTag. +// The tag keeps its address: the snapshot it points at cannot be changed. message UpdateActorSnapshotTagRequest { - ObjectRef tag = 1; - ActorSnapshotTagScope scope = 2; + // The tag to update. + // tag.metadata.atespace and tag.metadata.name identify which resource to + // update. + // tag.metadata.version and tag.metadata.uid are optional preconditions + // and zero values skip the check. + ActorSnapshotTag tag = 1; + + // The set of fields to update. Required. + // + // Only the following fields are supported: + // - scope + google.protobuf.FieldMask update_mask = 2; } message DeleteActorSnapshotTagRequest {