Skip to content

ateapi: align ActorSnapshot request validation #976

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Yiqing Wang (YQ-Wang) wants to merge 1 commit into
base: main
Choose a base branch
from
+269 −8
Open

ateapi: align ActorSnapshot request validation #976

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions cmd/ateapi/internal/controlapi/actor_snapshot.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,9 +133,6 @@ func (s *Service) CreateActorSnapshotTag(ctx context.Context, req *ateapipb.Crea
return nil, toGRPCStatusError(errs)
}
ref := req.GetActorSnapshotTag().GetSnapshot()
if req.GetActorSnapshotTag().GetMetadata().GetAtespace() != ref.GetAtespace() {
return nil, status.Error(codes.FailedPrecondition, "ActorSnapshot tags must belong to the snapshot's Atespace")
}
tag, err := s.persistence.CreateActorSnapshotTag(ctx, ref.GetAtespace(), ref.GetName(), req.GetActorSnapshotTag())
if errors.Is(err, store.ErrNotFound) {
return nil, status.Error(codes.NotFound, "ActorSnapshot not found")
Expand All @@ -156,7 +153,7 @@ func validateCreateActorSnapshotTagRequest(req *ateapipb.CreateActorSnapshotTagR
var fldPath *field.Path
var errs field.ErrorList

tag := req.ActorSnapshotTag
tag := req.GetActorSnapshotTag()
tagPath := fldPath.Child("actor_snapshot_tag")
if tag == nil {
errs = append(errs, field.Required(tagPath, ""))
Expand All @@ -165,14 +162,19 @@ func validateCreateActorSnapshotTagRequest(req *ateapipb.CreateActorSnapshotTagR

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 {
if val, p := tag.GetSnapshot(), tagPath.Child("snapshot"); val == nil {
errs = append(errs, field.Required(p, ""))
} else {
errs = append(errs, resources.ValidateObjectRef(val, p)...)
}

errs = append(errs, validateActorSnapshotTagScope(tag.GetScope(), tagPath.Child("scope"))...)

metaAtespace, snapAtespace := tag.GetMetadata().GetAtespace(), tag.GetSnapshot().GetAtespace()
if metaAtespace != snapAtespace && resources.IsValidResourceName(metaAtespace) && resources.IsValidResourceName(snapAtespace) {
errs = append(errs, field.Invalid(tagPath.Child("metadata").Child("atespace"), metaAtespace, "must match snapshot.atespace"))
}

return errs
}

Expand Down Expand Up @@ -218,7 +220,7 @@ func validateUpdateActorSnapshotTagRequest(req *ateapipb.UpdateActorSnapshotTagR

errs = append(errs, resources.ValidateResourceMetadataRef(tag.GetMetadata(), tagPath.Child("metadata"))...)

errs = append(errs, fieldmask.Validate(req.GetUpdateMask(), actorSnapshotTagMutableFields, field.NewPath("update_mask"))...)
errs = append(errs, fieldmask.Validate(req.GetUpdateMask(), actorSnapshotTagMutableFields, fldPath.Child("update_mask"))...)

errs = append(errs, validateActorSnapshotTagScope(tag.GetScope(), tagPath.Child("scope"))...)

Expand Down
259 changes: 259 additions & 0 deletions cmd/ateapi/internal/controlapi/actor_snapshot_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,244 @@ import (
"github.com/agent-substrate/substrate/pkg/proto/ateapipb"
)

func TestValidateGetActorSnapshotRequest(t *testing.T) {
tests := []struct {
name string
req *ateapipb.GetActorSnapshotRequest
want field.ErrorList
}{{
"valid",
&ateapipb.GetActorSnapshotRequest{Snapshot: &ateapipb.ObjectRef{Atespace: "ns1", Name: "snap1"}},
nil,
}, {
"missing snapshot",
&ateapipb.GetActorSnapshotRequest{},
field.ErrorList{field.Required(field.NewPath("snapshot"), "")},
}, {
"missing snapshot.atespace",
&ateapipb.GetActorSnapshotRequest{Snapshot: &ateapipb.ObjectRef{Name: "snap1"}},
field.ErrorList{field.Required(field.NewPath("snapshot", "atespace"), "")},
}, {
"invalid snapshot.atespace",
&ateapipb.GetActorSnapshotRequest{Snapshot: &ateapipb.ObjectRef{Atespace: "NS1", Name: "snap1"}},
field.ErrorList{field.Invalid(field.NewPath("snapshot", "atespace"), "NS1", "")},
}, {
"missing snapshot.name",
&ateapipb.GetActorSnapshotRequest{Snapshot: &ateapipb.ObjectRef{Atespace: "ns1"}},
field.ErrorList{field.Required(field.NewPath("snapshot", "name"), "")},
}, {
"invalid snapshot.name",
&ateapipb.GetActorSnapshotRequest{Snapshot: &ateapipb.ObjectRef{Atespace: "ns1", Name: "SNAP1"}},
field.ErrorList{field.Invalid(field.NewPath("snapshot", "name"), "SNAP1", "")},
}}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
assertValidateErr(t, validateGetActorSnapshotRequest(tt.req), tt.want)
})
}
}

func TestValidateGetActorSnapshotTagRequest(t *testing.T) {
tests := []struct {
name string
req *ateapipb.GetActorSnapshotTagRequest
want field.ErrorList
}{{
"valid",
&ateapipb.GetActorSnapshotTagRequest{Tag: &ateapipb.ObjectRef{Atespace: "ns1", Name: "tag1"}},
nil,
}, {
"missing tag",
&ateapipb.GetActorSnapshotTagRequest{},
field.ErrorList{field.Required(field.NewPath("tag"), "")},
}, {
"missing tag.atespace",
&ateapipb.GetActorSnapshotTagRequest{Tag: &ateapipb.ObjectRef{Name: "tag1"}},
field.ErrorList{field.Required(field.NewPath("tag", "atespace"), "")},
}, {
"invalid tag.atespace",
&ateapipb.GetActorSnapshotTagRequest{Tag: &ateapipb.ObjectRef{Atespace: "NS1", Name: "tag1"}},
field.ErrorList{field.Invalid(field.NewPath("tag", "atespace"), "NS1", "")},
}, {
"missing tag.name",
&ateapipb.GetActorSnapshotTagRequest{Tag: &ateapipb.ObjectRef{Atespace: "ns1"}},
field.ErrorList{field.Required(field.NewPath("tag", "name"), "")},
}, {
"invalid tag.name",
&ateapipb.GetActorSnapshotTagRequest{Tag: &ateapipb.ObjectRef{Atespace: "ns1", Name: "TAG1"}},
field.ErrorList{field.Invalid(field.NewPath("tag", "name"), "TAG1", "")},
}}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
assertValidateErr(t, validateGetActorSnapshotTagRequest(tt.req), tt.want)
})
}
}

func TestValidateListActorSnapshotsRequest(t *testing.T) {
tests := []struct {
name string
req *ateapipb.ListActorSnapshotsRequest
want field.ErrorList
}{{
"valid, atespace scoped",
&ateapipb.ListActorSnapshotsRequest{Atespace: "ns1"},
nil,
}, {
// Empty atespace means "all atespaces".
"valid, empty atespace means all atespaces",
&ateapipb.ListActorSnapshotsRequest{},
nil,
}, {
"invalid atespace",
&ateapipb.ListActorSnapshotsRequest{Atespace: "NS1"},
field.ErrorList{field.Invalid(field.NewPath("atespace"), "NS1", "")},
}, {
"valid, positive page_size",
&ateapipb.ListActorSnapshotsRequest{Atespace: "ns1", PageSize: 10},
nil,
}, {
"negative page_size",
&ateapipb.ListActorSnapshotsRequest{Atespace: "ns1", PageSize: -1},
field.ErrorList{field.Invalid(field.NewPath("page_size"), int32(-1), "")},
}}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
assertValidateErr(t, validateListActorSnapshotsRequest(tt.req), tt.want)
})
}
}

func TestValidateCreateActorSnapshotTagRequest(t *testing.T) {
scopes := []string{
ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_ATESPACE.String(),
ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED.String(),
}
validTag := func(mutate func(*ateapipb.ActorSnapshotTag)) *ateapipb.CreateActorSnapshotTagRequest {
tag := &ateapipb.ActorSnapshotTag{
Metadata: &ateapipb.ResourceMetadata{Atespace: "ns1", Name: "tag1"},
Snapshot: &ateapipb.ObjectRef{Atespace: "ns1", Name: "snap1"},
Scope: ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_ATESPACE,
}
if mutate != nil {
mutate(tag)
}
return &ateapipb.CreateActorSnapshotTagRequest{ActorSnapshotTag: tag}
}

tests := []struct {
name string
req *ateapipb.CreateActorSnapshotTagRequest
want field.ErrorList
}{{
"valid",
validTag(nil),
nil,
}, {
"missing actor_snapshot_tag",
&ateapipb.CreateActorSnapshotTagRequest{},
field.ErrorList{field.Required(field.NewPath("actor_snapshot_tag"), "")},
}, {
"missing metadata.atespace",
validTag(func(tag *ateapipb.ActorSnapshotTag) { tag.Metadata.Atespace = "" }),
field.ErrorList{field.Required(field.NewPath("actor_snapshot_tag", "metadata", "atespace"), "")},
}, {
"invalid metadata.atespace",
validTag(func(tag *ateapipb.ActorSnapshotTag) { tag.Metadata.Atespace = "NS1" }),
field.ErrorList{field.Invalid(field.NewPath("actor_snapshot_tag", "metadata", "atespace"), "NS1", "")},
}, {
"missing metadata.name",
validTag(func(tag *ateapipb.ActorSnapshotTag) { tag.Metadata.Name = "" }),
field.ErrorList{field.Required(field.NewPath("actor_snapshot_tag", "metadata", "name"), "")},
}, {
"invalid metadata.name",
validTag(func(tag *ateapipb.ActorSnapshotTag) { tag.Metadata.Name = "TAG1" }),
field.ErrorList{field.Invalid(field.NewPath("actor_snapshot_tag", "metadata", "name"), "TAG1", "")},
}, {
"missing snapshot",
validTag(func(tag *ateapipb.ActorSnapshotTag) { tag.Snapshot = nil }),
field.ErrorList{field.Required(field.NewPath("actor_snapshot_tag", "snapshot"), "")},
}, {
"missing snapshot.atespace",
validTag(func(tag *ateapipb.ActorSnapshotTag) { tag.Snapshot.Atespace = "" }),
field.ErrorList{field.Required(field.NewPath("actor_snapshot_tag", "snapshot", "atespace"), "")},
}, {
"invalid snapshot.atespace",
validTag(func(tag *ateapipb.ActorSnapshotTag) { tag.Snapshot.Atespace = "NS1" }),
field.ErrorList{field.Invalid(field.NewPath("actor_snapshot_tag", "snapshot", "atespace"), "NS1", "")},
}, {
"missing snapshot.name",
validTag(func(tag *ateapipb.ActorSnapshotTag) { tag.Snapshot.Name = "" }),
field.ErrorList{field.Required(field.NewPath("actor_snapshot_tag", "snapshot", "name"), "")},
}, {
"invalid snapshot.name",
validTag(func(tag *ateapipb.ActorSnapshotTag) { tag.Snapshot.Name = "SNAP1" }),
field.ErrorList{field.Invalid(field.NewPath("actor_snapshot_tag", "snapshot", "name"), "SNAP1", "")},
}, {
"atespace mismatch",
validTag(func(tag *ateapipb.ActorSnapshotTag) { tag.Metadata.Atespace = "other" }),
field.ErrorList{field.Invalid(field.NewPath("actor_snapshot_tag", "metadata", "atespace"), "other", "")},
}, {
"unset scope",
validTag(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",
validTag(func(tag *ateapipb.ActorSnapshotTag) { tag.Scope = ateapipb.ActorSnapshotTagScope(7) }),
field.ErrorList{field.NotSupported(field.NewPath("actor_snapshot_tag", "scope"), "7", scopes)},
}, {
"published scope",
validTag(func(tag *ateapipb.ActorSnapshotTag) {
tag.Scope = ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED
}),
nil,
}}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
assertValidateErr(t, validateCreateActorSnapshotTagRequest(tt.req), tt.want)
})
}
}

func TestValidateDeleteActorSnapshotTagRequest(t *testing.T) {
tests := []struct {
name string
req *ateapipb.DeleteActorSnapshotTagRequest
want field.ErrorList
}{{
"valid",
&ateapipb.DeleteActorSnapshotTagRequest{Tag: &ateapipb.ObjectRef{Atespace: "ns1", Name: "tag1"}},
nil,
}, {
"missing tag",
&ateapipb.DeleteActorSnapshotTagRequest{},
field.ErrorList{field.Required(field.NewPath("tag"), "")},
}, {
"missing tag.atespace",
&ateapipb.DeleteActorSnapshotTagRequest{Tag: &ateapipb.ObjectRef{Name: "tag1"}},
field.ErrorList{field.Required(field.NewPath("tag", "atespace"), "")},
}, {
"invalid tag.atespace",
&ateapipb.DeleteActorSnapshotTagRequest{Tag: &ateapipb.ObjectRef{Atespace: "NS1", Name: "tag1"}},
field.ErrorList{field.Invalid(field.NewPath("tag", "atespace"), "NS1", "")},
}, {
"missing tag.name",
&ateapipb.DeleteActorSnapshotTagRequest{Tag: &ateapipb.ObjectRef{Atespace: "ns1"}},
field.ErrorList{field.Required(field.NewPath("tag", "name"), "")},
}, {
"invalid tag.name",
&ateapipb.DeleteActorSnapshotTagRequest{Tag: &ateapipb.ObjectRef{Atespace: "ns1", Name: "TAG1"}},
field.ErrorList{field.Invalid(field.NewPath("tag", "name"), "TAG1", "")},
}}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
assertValidateErr(t, validateDeleteActorSnapshotTagRequest(tt.req), tt.want)
})
}
}

func TestValidateUpdateActorSnapshotTagRequest(t *testing.T) {
mutableFields := []string{"scope"}
scopes := []string{
Expand Down Expand Up @@ -352,6 +590,27 @@ func TestCreateActorSnapshotTag_RejectsUnsetScope(t *testing.T) {
}
}

// TestCreateActorSnapshotTag_RejectsAtespaceMismatch checks that a tag cannot
// be created in a different Atespace than the snapshot it points at.
func TestCreateActorSnapshotTag_RejectsAtespaceMismatch(t *testing.T) {
ctx := context.Background()
svc, stored := serviceWithActorSnapshotTag(t, &ateapipb.ActorSnapshotTag{
Metadata: &ateapipb.ResourceMetadata{Atespace: testAtespace, Name: "tag1"},
Scope: ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_ATESPACE,
})

_, err := svc.CreateActorSnapshotTag(ctx, &ateapipb.CreateActorSnapshotTagRequest{
ActorSnapshotTag: &ateapipb.ActorSnapshotTag{
Metadata: &ateapipb.ResourceMetadata{Atespace: "other", Name: "tag2"},
Snapshot: stored.GetSnapshot(),
Scope: ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_ATESPACE,
},
})
if code := status.Code(err); code != codes.InvalidArgument {
t.Errorf("CreateActorSnapshotTag error = %v (code %v), want code InvalidArgument", err, code)
}
}

// serviceWithActorSnapshotTag seeds an ActorSnapshot and a tag pointing at it
// in a miniredis-backed store, and returns a Service over it.
func serviceWithActorSnapshotTag(t *testing.T, tag *ateapipb.ActorSnapshotTag) (*Service, *ateapipb.ActorSnapshotTag) {
Expand Down
4 changes: 2 additions & 2 deletions cmd/ateapi/internal/controlapi/functional_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2135,8 +2135,8 @@ func TestSuspendActor(t *testing.T) {
Snapshot: snapshotRef,
Scope: ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_ATESPACE,
},
}); status.Code(err) != codes.FailedPrecondition {
t.Fatalf("cross-atespace CreateActorSnapshotTag status = %v, want FailedPrecondition", status.Code(err))
}); status.Code(err) != codes.InvalidArgument {
t.Fatalf("cross-atespace CreateActorSnapshotTag status = %v, want InvalidArgument", status.Code(err))
}
if _, err := tc.client.CreateActor(context.Background(), &ateapipb.CreateActorRequest{
Actor: &ateapipb.Actor{
Expand Down