From 2ae7da69ea41d7a47f566f0e0a316323c9a64648 Mon Sep 17 00:00:00 2001 From: Danielle Mustillo Date: Thu, 15 Oct 2026 23:50:58 -0400 Subject: [PATCH 1/2] fix(substrate): limit Substrate views to watched namespaces - fix(substrate): limit Substrate views to watched namespaces - docs(substrate): say empty atespace lists watched atespaces - test(substrate): page tokens must name only watched data - fix(substrate): page only through watched namespaces - fix(substrate): time out GetWorker and read worker pods uncached - refactor(substrate): simplify page tokens and tests Signed-off-by: Danielle Mustillo --- go/api/gen/kagent/api/v1/system.pb.go | 19 +- go/core/internal/a2agateway/http_test.go | 2 +- go/core/internal/grpcserver/server_test.go | 2 +- .../grpcserver/system_feedback_test.go | 6 +- .../service/sandbox/transports_test.go | 2 +- go/core/internal/service/system/service.go | 47 +- .../internal/service/system/service_test.go | 206 +++-- go/core/internal/service/system/substrate.go | 261 +++++- .../service/system/substrate_paging_test.go | 819 ++++++++++++++++++ go/core/internal/substrate/client.go | 7 + go/core/pkg/app/app.go | 2 +- proto/kagent/api/v1/system.proto | 19 +- ui/src/api/domain/substrate.ts | 6 +- ui/src/generated/kagent/api/v1/system_pb.ts | 19 +- ui/src/pages/SubstratePage.tsx | 2 +- 15 files changed, 1280 insertions(+), 139 deletions(-) create mode 100644 go/core/internal/service/system/substrate_paging_test.go diff --git a/go/api/gen/kagent/api/v1/system.pb.go b/go/api/gen/kagent/api/v1/system.pb.go index 5ae0980ae8..7bf269f121 100644 --- a/go/api/gen/kagent/api/v1/system.pb.go +++ b/go/api/gen/kagent/api/v1/system.pb.go @@ -396,7 +396,8 @@ type GetSubstrateSummaryRequest struct { state protoimpl.MessageState `protogen:"open.v1"` // Kubernetes namespace for workers and pools. Empty uses watched namespaces. Namespace string `protobuf:"bytes,1,opt,name=namespace,proto3" json:"namespace,omitempty"` - // ATE atespace for actors and templates. Empty lists all atespaces. + // ATE atespace for actors and templates. Empty lists watched atespaces (all when + // the controller watches all namespaces). Atespace string `protobuf:"bytes,2,opt,name=atespace,proto3" json:"atespace,omitempty"` unknownFields protoimpl.UnknownFields sizeCache protoimpl.SizeCache @@ -614,10 +615,12 @@ func (x *GetSubstrateSummaryResponse) GetActorTemplates() []*ateapipb.ActorTempl return nil } -// One upstream page of actors, in Substrate order. +// One page of actors. A controller that watches some namespaces reads its watched +// atespaces one at a time, in name order, and Substrate order inside each. type ListSubstrateActorsRequest struct { state protoimpl.MessageState `protogen:"open.v1"` - // ATE atespace of the actor itself. Empty lists all atespaces. + // ATE atespace of the actor itself. Empty lists watched atespaces (all when the + // controller watches all namespaces). Atespace string `protobuf:"bytes,6,opt,name=atespace,proto3" json:"atespace,omitempty"` // `limit` is capped at 100 in common.proto and zero means the server's default; // above the cap is refused rather than clamped. @@ -674,7 +677,9 @@ type ListSubstrateActorsResponse struct { state protoimpl.MessageState `protogen:"open.v1"` AteApiError string `protobuf:"bytes,2,opt,name=ate_api_error,json=ateApiError,proto3" json:"ate_api_error,omitempty"` Actors []*ateapipb.Actor `protobuf:"bytes,3,rep,name=actors,proto3" json:"actors,omitempty"` - // Upstream continuation token. A page can be empty even when another page exists. + // Continuation token. A controller that watches all namespaces passes Substrate's + // token through. A controller that watches some issues its own, which names only + // watched data. Only a last page can be empty. Page *PageResponse `protobuf:"bytes,4,opt,name=page,proto3" json:"page,omitempty"` ComputedAt *timestamppb.Timestamp `protobuf:"bytes,5,opt,name=computed_at,json=computedAt,proto3" json:"computed_at,omitempty"` unknownFields protoimpl.UnknownFields @@ -739,8 +744,10 @@ func (x *ListSubstrateActorsResponse) GetComputedAt() *timestamppb.Timestamp { return nil } -// One upstream page of workers, in Substrate order. -// Namespace filtering applies to this page only; it may be empty and still have a next token. +// One page of workers, in worker pod UID order. A controller that watches all +// namespaces filters one Substrate page, which may be empty and still have a next +// token. A controller that watches some reads only the workers of its own worker pods; +// only its last page can be empty. type ListSubstrateWorkersRequest struct { state protoimpl.MessageState `protogen:"open.v1"` Namespace string `protobuf:"bytes,1,opt,name=namespace,proto3" json:"namespace,omitempty"` diff --git a/go/core/internal/a2agateway/http_test.go b/go/core/internal/a2agateway/http_test.go index 9235c9bb12..306e8c4fe4 100644 --- a/go/core/internal/a2agateway/http_test.go +++ b/go/core/internal/a2agateway/http_test.go @@ -449,7 +449,7 @@ func startCoreTestServer(t *testing.T, gateway a2asrv.RequestHandler, shares ses require.NoError(t, err) server, err := grpcserver.New(grpcserver.Config{ Listener: listener, Authenticator: httpTestAuthenticator{}, ShareStore: shares, - SystemService: systemservice.NewService(nil, nil, nil, nil), + SystemService: systemservice.NewService(nil, nil, nil, nil, nil), A2AHandler: gateway, HTTPHandler: NewHTTPHandler(gateway, httpTestAuthenticator{}, shares), }) require.NoError(t, err) diff --git a/go/core/internal/grpcserver/server_test.go b/go/core/internal/grpcserver/server_test.go index 08c97430a0..b554cc4c73 100644 --- a/go/core/internal/grpcserver/server_test.go +++ b/go/core/internal/grpcserver/server_test.go @@ -18,7 +18,7 @@ import ( ) func testSystemService() *systemservice.Service { - return systemservice.NewService(nil, nil, nil, nil) + return systemservice.NewService(nil, nil, nil, nil, nil) } func TestServerServesGRPCAndHTTP(t *testing.T) { diff --git a/go/core/internal/grpcserver/system_feedback_test.go b/go/core/internal/grpcserver/system_feedback_test.go index 612eb3bd44..d75139403c 100644 --- a/go/core/internal/grpcserver/system_feedback_test.go +++ b/go/core/internal/grpcserver/system_feedback_test.go @@ -53,7 +53,7 @@ func TestSystemGeneratedClient(t *testing.T) { server, err := New(Config{ Listener: listener, Authenticator: &authimpl.InsecureAuthenticator{}, - SystemService: systemservice.NewService(kubeClient, nil, &pkgauth.NoopAuthorizer{}, emptySystemATEClient{}), + SystemService: systemservice.NewService(kubeClient, kubeClient, nil, &pkgauth.NoopAuthorizer{}, emptySystemATEClient{}), }) if err != nil { t.Fatalf("New() error = %v", err) @@ -200,3 +200,7 @@ func (emptySystemATEClient) ListActorsPage(context.Context, string, int32, strin func (emptySystemATEClient) ListWorkersPage(context.Context, int32, string) ([]*ateapipb.Worker, string, error) { return nil, "", nil } + +func (emptySystemATEClient) GetWorker(_ context.Context, name string) (*ateapipb.Worker, error) { + return nil, status.Errorf(codes.NotFound, "Worker %s not found", name) +} diff --git a/go/core/internal/service/sandbox/transports_test.go b/go/core/internal/service/sandbox/transports_test.go index eca00727d0..0ce2cbd944 100644 --- a/go/core/internal/service/sandbox/transports_test.go +++ b/go/core/internal/service/sandbox/transports_test.go @@ -38,7 +38,7 @@ func TestSandboxTransports(t *testing.T) { listener := bufconn.Listen(1 << 20) server, err := grpcserver.New(grpcserver.Config{ Listener: listener, Authenticator: testAuth{}, - SystemService: system.NewService(nil, nil, nil, nil), + SystemService: system.NewService(nil, nil, nil, nil, nil), SandboxService: service, }) require.NoError(t, err) diff --git a/go/core/internal/service/system/service.go b/go/core/internal/service/system/service.go index f3dd63e716..ac7dc43b74 100644 --- a/go/core/internal/service/system/service.go +++ b/go/core/internal/service/system/service.go @@ -29,10 +29,16 @@ type ATEClient interface { ListActorTemplates(context.Context, string) ([]*ateapipb.ActorTemplate, error) ListActorsPage(ctx context.Context, atespace string, pageSize int32, pageToken string) ([]*ateapipb.Actor, string, error) ListWorkersPage(ctx context.Context, pageSize int32, pageToken string) ([]*ateapipb.Worker, string, error) + GetWorker(ctx context.Context, name string) (*ateapipb.Worker, error) } type Service struct { - kubeClient client.Client + kubeClient client.Client + // Reads worker pods. The manager passes its uncached API reader, so the manager's + // cache holds no Pods. + podReader client.Reader + // Sorted and unique: the order Substrate pages are read in. A repeat must not read + // an atespace twice. Empty means every namespace. observedNamespaces []string authorizer auth.Authorizer ateClient ATEClient @@ -45,13 +51,15 @@ type Namespace struct { func NewService( kubeClient client.Client, + podReader client.Reader, observedNamespaces []string, authorizer auth.Authorizer, ateClient ATEClient, ) *Service { return &Service{ kubeClient: kubeClient, - observedNamespaces: slices.Clone(observedNamespaces), + podReader: podReader, + observedNamespaces: slices.Compact(slices.Sorted(slices.Values(observedNamespaces))), authorizer: authorizer, ateClient: ateClient, } @@ -172,24 +180,27 @@ func (s *Service) listWorkerPools(ctx context.Context, namespace string) ([]atev return workerPoolList.Items, nil } -// substrateActorTemplates drains upstream pagination for the configuration-sized list. +// substrateActorTemplates lists the templates of the requested atespace, or of each +// watched one. The list is configuration-sized. func (s *Service) substrateActorTemplates(ctx context.Context, atespace string) ([]*ateapipb.ActorTemplate, error) { - templatesFromAPI, err := s.ateClient.ListActorTemplates(ctx, atespace) - if err != nil { - return nil, err - } - templates := make([]*ateapipb.ActorTemplate, 0, len(templatesFromAPI)) - for _, template := range templatesFromAPI { - if template == nil { - continue + templates := []*ateapipb.ActorTemplate{} + for _, atespace := range s.substrateNamespaces(atespace) { + page, err := s.ateClient.ListActorTemplates(ctx, atespace) + if err != nil { + return nil, err + } + for _, template := range page { + // ate-api scoped the list already. This is a second guard. + if template != nil && (atespace == "" || template.GetMetadata().GetAtespace() == atespace) { + // Only expose inventory fields: containers can contain resolved credentials. + templates = append(templates, &ateapipb.ActorTemplate{ + Metadata: template.GetMetadata(), + Status: template.GetStatus(), + SandboxConfig: template.GetSandboxConfig(), + WorkerSelector: template.GetWorkerSelector(), + }) + } } - // Only expose inventory fields: containers can contain resolved credentials. - templates = append(templates, &ateapipb.ActorTemplate{ - Metadata: template.GetMetadata(), - Status: template.GetStatus(), - SandboxConfig: template.GetSandboxConfig(), - WorkerSelector: template.GetWorkerSelector(), - }) } slices.SortStableFunc(templates, func(left, right *ateapipb.ActorTemplate) int { diff --git a/go/core/internal/service/system/service_test.go b/go/core/internal/service/system/service_test.go index c6e19b1dd1..8d341c7482 100644 --- a/go/core/internal/service/system/service_test.go +++ b/go/core/internal/service/system/service_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "slices" "strconv" "strings" "testing" @@ -12,6 +13,8 @@ import ( "github.com/agent-substrate/substrate/pkg/proto/ateapipb" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -48,36 +51,104 @@ type fakeATEClient struct { // was followed rather than that the rows came back. actorReads int workerReads int + // Every call in order, so a test can assert on what reached ate-api. + calls []ateCall + // ignoreAtespace answers an actor or template list with the rows of every + // atespace, as an ate-api that drops the filter would. + ignoreAtespace bool + // failCall fails the call with this 1-based number with failErr. + failCall int + failErr error +} + +// ateCall is one ate-api call: the method, the atespace or worker name, the token +// and the page size. +type ateCall struct { + method, scope, token string + size int32 +} + +// record notes a call and answers with the error it is set up to fail with. +func (client *fakeATEClient) record(method, scope, token string, size int32) error { + client.calls = append(client.calls, ateCall{method: method, scope: scope, token: token, size: size}) + if client.failCall == len(client.calls) { + return client.failErr + } + return client.err } func (client *fakeATEClient) ListActorTemplates(_ context.Context, atespace string) ([]*ateapipb.ActorTemplate, error) { + if err := client.record("ListActorTemplates", atespace, "", 0); err != nil { + return nil, err + } templates := []*ateapipb.ActorTemplate{} for _, template := range client.templates { - if atespace == "" || template.GetMetadata().GetAtespace() == atespace { + if atespace == "" || client.ignoreAtespace || template.GetMetadata().GetAtespace() == atespace { templates = append(templates, template) } } - return templates, client.err + return templates, nil } func (client *fakeATEClient) ListActorsPage(_ context.Context, atespace string, pageSize int32, pageToken string) ([]*ateapipb.Actor, string, error) { client.actorReads++ - if err := client.readError(client.actorReads); err != nil { + if err := client.record("ListActorsPage", atespace, pageToken, pageSize); err != nil { return nil, "", err } - return fakePage(actorsInAtespace(client.actors, atespace), client.pageSize, pageSize, pageToken) + scope := atespace + if client.ignoreAtespace { + scope = "" + } + return actorPage(actorsInAtespace(client.actors, scope), atespace, client.pageSize, pageSize, pageToken) } func (client *fakeATEClient) ListWorkersPage(_ context.Context, pageSize int32, pageToken string) ([]*ateapipb.Worker, string, error) { client.workerReads++ - if err := client.readError(client.workerReads); err != nil { + if err := client.record("ListWorkersPage", "", pageToken, pageSize); err != nil { return nil, "", err } return fakePage(client.workers, client.pageSize, pageSize, pageToken) } -func (client *fakeATEClient) readError(int) error { - return client.err +func (client *fakeATEClient) GetWorker(_ context.Context, name string) (*ateapipb.Worker, error) { + if err := client.record("GetWorker", name, "", 0); err != nil { + return nil, err + } + for _, worker := range client.workers { + if worker.GetMetadata().GetName() == name { + return worker, nil + } + } + // ate-api's own NotFound text, which repeats the name it was sent. + return nil, status.Errorf(codes.NotFound, "Worker %s not found", name) +} + +// actorPage pages actors the way ate-api does: in atespace and name order, after the +// last row of the token, with a token bound to the atespace it was issued for. A token +// sent with another atespace is refused, as ate-api refuses it. +func actorPage(actors []*ateapipb.Actor, atespace string, ceiling int, requested int32, pageToken string) ([]*ateapipb.Actor, string, error) { + key := func(actor *ateapipb.Actor) string { + return actor.GetMetadata().GetAtespace() + "/" + actor.GetMetadata().GetName() + } + rows := slices.SortedStableFunc(slices.Values(actors), func(left, right *ateapipb.Actor) int { + return strings.Compare(key(left), key(right)) + }) + prefix := "upstream:" + atespace + ":" + if pageToken != "" { + last, ok := strings.CutPrefix(pageToken, prefix) + if !ok { + return nil, "", status.Error(codes.InvalidArgument, "invalid page_token") + } + rows = slices.DeleteFunc(rows, func(actor *ateapipb.Actor) bool { return key(actor) <= last }) + } + pageSize := ceiling + if requested > 0 && (ceiling <= 0 || int(requested) < ceiling) { + pageSize = int(requested) + } + if pageSize <= 0 || pageSize >= len(rows) { + return rows, "", nil + } + return rows[:pageSize], prefix + key(rows[pageSize-1]), nil } // fakePage slices rows the way ate-api pages them: an opaque token, empty on the last @@ -110,7 +181,7 @@ func fakePage[T any](rows []T, ceiling int, requested int32, pageToken string) ( } func TestCurrentUser(t *testing.T) { - service := system.NewService(nil, nil, nil, nil) + service := system.NewService(nil, nil, nil, nil, nil) claims := map[string]any{"sub": "user-1", "groups": []any{"admins"}} ctx := pkgAuth.AuthSessionTo(t.Context(), &authimpl.SimpleSession{P: pkgAuth.Principal{ User: pkgAuth.User{ID: "user-1"}, @@ -141,7 +212,7 @@ func TestListNamespaces(t *testing.T) { &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "Zoo"}, Status: corev1.NamespaceStatus{Phase: corev1.NamespaceActive}}, &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "alpha"}, Status: corev1.NamespaceStatus{Phase: corev1.NamespaceTerminating}}, ).Build() - service := system.NewService(kubeClient, nil, nil, nil) + service := system.NewService(kubeClient, kubeClient, nil, nil, nil) result, err := service.ListNamespaces(t.Context()) require.NoError(t, err) @@ -157,7 +228,7 @@ func TestListNamespaces(t *testing.T) { return apierrors.NewForbidden(schema.GroupResource{Resource: "namespaces"}, "", nil) }, }).Build() - service := system.NewService(kubeClient, []string{"team-b", "team-a"}, nil, nil) + service := system.NewService(kubeClient, kubeClient, []string{"team-b", "team-a"}, nil, nil) result, err := service.ListNamespaces(t.Context()) require.NoError(t, err) @@ -186,7 +257,7 @@ func TestListSubstrateActors(t *testing.T) { ctx := pkgAuth.AuthSessionTo(t.Context(), &authimpl.SimpleSession{P: pkgAuth.Principal{User: pkgAuth.User{ID: "user"}}}) newService := func(client system.ATEClient) *system.Service { - return system.NewService(nil, nil, &pkgAuth.NoopAuthorizer{}, client) + return system.NewService(nil, nil, nil, &pkgAuth.NoopAuthorizer{}, client) } t.Run("answers with one page and the token for the next", func(t *testing.T) { @@ -253,7 +324,7 @@ func TestListSubstrateActors(t *testing.T) { }) t.Run("authorizes", func(t *testing.T) { - denied := system.NewService(nil, nil, systemDenyAuthorizer{}, &fakeATEClient{}) + denied := system.NewService(nil, nil, nil, systemDenyAuthorizer{}, &fakeATEClient{}) _, err := denied.ListSubstrateActors(ctx, &apiv1.ListSubstrateActorsRequest{}) assert.True(t, serviceerrors.IsCode(err, serviceerrors.CodePermissionDenied), err) }) @@ -266,8 +337,9 @@ func TestListSubstrateWorkers(t *testing.T) { {WorkerNamespace: "team", WorkerPool: "pool", WorkerPod: "worker-0", Status: &ateapipb.WorkerStatus{Allocated: &ateapipb.WorkerResources{Actors: 2}}}, {WorkerNamespace: "other", WorkerPool: "pool", WorkerPod: "worker-1"}, {WorkerNamespace: "team", WorkerPool: "pool", WorkerPod: "worker-2"}, + {WorkerPool: "pool", WorkerPod: "names-no-namespace"}, }} - service := system.NewService(nil, nil, &pkgAuth.NoopAuthorizer{}, ateClient) + service := system.NewService(nil, nil, nil, &pkgAuth.NoopAuthorizer{}, ateClient) page, err := service.ListSubstrateWorkers(ctx, &apiv1.ListSubstrateWorkersRequest{Namespace: "team", Page: &apiv1.PageRequest{Limit: 2}}) require.NoError(t, err) @@ -321,7 +393,7 @@ func TestGetSubstrateSummary(t *testing.T) { {WorkerNamespace: "other", WorkerPool: "pool", WorkerPod: "worker-9"}, }, } - service := system.NewService(kubeClient, nil, &pkgAuth.NoopAuthorizer{}, ateClient) + service := system.NewService(kubeClient, kubeClient, nil, &pkgAuth.NoopAuthorizer{}, ateClient) result, err := service.GetSubstrateSummary(ctx, "team", "team") require.NoError(t, err) @@ -349,7 +421,7 @@ func TestGetSubstrateSummary(t *testing.T) { }) t.Run("an ate-api failure leaves the Kubernetes halves complete", func(t *testing.T) { - service := system.NewService(kubeClient, nil, &pkgAuth.NoopAuthorizer{}, &fakeATEClient{err: errors.New("ate-api unreachable")}) + service := system.NewService(kubeClient, kubeClient, nil, &pkgAuth.NoopAuthorizer{}, &fakeATEClient{err: errors.New("ate-api unreachable")}) result, err := service.GetSubstrateSummary(ctx, "team", "team") require.NoError(t, err) @@ -380,7 +452,7 @@ func TestGetSubstrateSummaryReadsAreIndependent(t *testing.T) { ateClient := &failingTemplatesATEClient{ fakeATEClient: fakeATEClient{actors: actors, workers: workers}, } - service := system.NewService(kubeClient, nil, &pkgAuth.NoopAuthorizer{}, ateClient) + service := system.NewService(kubeClient, kubeClient, nil, &pkgAuth.NoopAuthorizer{}, ateClient) result, err := service.GetSubstrateSummary(ctx, "team", "team") require.NoError(t, err) @@ -395,7 +467,7 @@ func TestGetSubstrateSummaryReadsAreIndependent(t *testing.T) { t.Run("a failed actor walk still counts busy workers", func(t *testing.T) { ateClient := &failingActorsATEClient{fakeATEClient: fakeATEClient{workers: workers}} - service := system.NewService(kubeClient, nil, &pkgAuth.NoopAuthorizer{}, ateClient) + service := system.NewService(kubeClient, kubeClient, nil, &pkgAuth.NoopAuthorizer{}, ateClient) result, err := service.GetSubstrateSummary(ctx, "team", "team") require.NoError(t, err) assert.Equal(t, "actors unavailable", result.ATEAPIError) @@ -413,7 +485,7 @@ func TestGetSubstrateSummaryReadsAreIndependent(t *testing.T) { }, }, } - service := system.NewService(kubeClient, nil, &pkgAuth.NoopAuthorizer{}, ateClient) + service := system.NewService(kubeClient, kubeClient, nil, &pkgAuth.NoopAuthorizer{}, ateClient) result, err := service.GetSubstrateSummary(ctx, "team", "team") require.NoError(t, err) @@ -438,7 +510,7 @@ func TestGetSubstrateSummaryReadsAreIndependent(t *testing.T) { {WorkerNamespace: "kagent", WorkerPool: "pool", WorkerPod: "worker-0"}, }, } - service := system.NewService(kubeClient, nil, &pkgAuth.NoopAuthorizer{}, ateClient) + service := system.NewService(kubeClient, kubeClient, nil, &pkgAuth.NoopAuthorizer{}, ateClient) result, err := service.GetSubstrateSummary(ctx, "team", "team") require.NoError(t, err) @@ -486,7 +558,7 @@ func TestBusyWorkerWithOutOfScopeActor(t *testing.T) { {WorkerNamespace: "kagent", WorkerPod: "worker-0", Status: &ateapipb.WorkerStatus{Allocated: &ateapipb.WorkerResources{Actors: 1}}}, }, } - service := system.NewService(kubeClient, nil, &pkgAuth.NoopAuthorizer{}, ateClient) + service := system.NewService(kubeClient, kubeClient, nil, &pkgAuth.NoopAuthorizer{}, ateClient) summary, err := service.GetSubstrateSummary(ctx, "kagent", "kagent") require.NoError(t, err) require.Empty(t, summary.ATEAPIError) @@ -529,7 +601,7 @@ func TestSubstrateScopesAreIndependent(t *testing.T) { }, workers: []*ateapipb.Worker{{WorkerNamespace: "workers", WorkerPod: "pod"}}, } - service := system.NewService(kubeClient, []string{"workers"}, &pkgAuth.NoopAuthorizer{}, ateClient) + service := system.NewService(kubeClient, kubeClient, nil, &pkgAuth.NoopAuthorizer{}, ateClient) for _, tc := range []struct { atespace string count int64 @@ -573,36 +645,72 @@ func TestSubstrateScopesAreIndependent(t *testing.T) { assert.Same(t, actor, page.Actors[0]) } -type emptyPageATEClient struct { - fakeATEClient - t *testing.T -} - -func (c *emptyPageATEClient) ListActorsPage(_ context.Context, atespace string, size int32, token string) ([]*ateapipb.Actor, string, error) { - assert.Equal(c.t, "team", atespace) - assert.Equal(c.t, int32(7), size) - assert.Equal(c.t, "upstream:opaque/cursor=", token) - return nil, "upstream:next/cursor=", nil +// A controller that watches namespaces must not read another tenant's namespace or +// atespace, and must not ask ate-api or Kubernetes for it. +func TestSubstrateRefusesUnwatchedNamespaces(t *testing.T) { + ctx := pkgAuth.AuthSessionTo(t.Context(), &authimpl.SimpleSession{P: pkgAuth.Principal{User: pkgAuth.User{ID: "user"}}}) + for _, tc := range []struct { + name string + read func(*system.Service) error + }{ + {"actors in another atespace", func(service *system.Service) error { + _, err := service.ListSubstrateActors(ctx, &apiv1.ListSubstrateActorsRequest{Atespace: "tenant-b"}) + return err + }}, + {"actors in a watched atespace with other case", func(service *system.Service) error { + _, err := service.ListSubstrateActors(ctx, &apiv1.ListSubstrateActorsRequest{Atespace: "Tenant-A"}) + return err + }}, + {"actors in a watched atespace with a trailing space", func(service *system.Service) error { + _, err := service.ListSubstrateActors(ctx, &apiv1.ListSubstrateActorsRequest{Atespace: "tenant-a "}) + return err + }}, + {"a summary of another atespace", func(service *system.Service) error { + _, err := service.GetSubstrateSummary(ctx, "", "tenant-b") + return err + }}, + {"a summary of another namespace", func(service *system.Service) error { + _, err := service.GetSubstrateSummary(ctx, "tenant-b", "") + return err + }}, + {"a summary of a watched namespace and another atespace", func(service *system.Service) error { + _, err := service.GetSubstrateSummary(ctx, "tenant-a", "tenant-b") + return err + }}, + {"workers in another namespace", func(service *system.Service) error { + _, err := service.ListSubstrateWorkers(ctx, &apiv1.ListSubstrateWorkersRequest{Namespace: "tenant-b"}) + return err + }}, + } { + t.Run(tc.name, func(t *testing.T) { + kubeLists := 0 + ateClient := pagingATEClient() + kubeClient := pagingKubeClient(t, interceptor.Funcs{List: func(ctx context.Context, c ctrlclient.WithWatch, list ctrlclient.ObjectList, opts ...ctrlclient.ListOption) error { + kubeLists++ + return c.List(ctx, list, opts...) + }}) + service := system.NewService(kubeClient, kubeClient, []string{"tenant-a"}, &pkgAuth.NoopAuthorizer{}, ateClient) + + err := tc.read(service) + assert.True(t, serviceerrors.IsCode(err, serviceerrors.CodePermissionDenied), "err = %v", err) + assert.Empty(t, ateClient.calls, "a refused read must not reach ate-api") + assert.Zero(t, kubeLists, "a refused read must not list WorkerPools or pods") + }) + } } -func (c *emptyPageATEClient) ListWorkersPage(_ context.Context, size int32, token string) ([]*ateapipb.Worker, string, error) { - assert.Equal(c.t, int32(7), size) - assert.Equal(c.t, "upstream:opaque/cursor=", token) - return []*ateapipb.Worker{{WorkerNamespace: "other"}}, "upstream:next/cursor=", nil +func actorNames(actors []*ateapipb.Actor) []string { + names := []string{} + for _, actor := range actors { + names = append(names, actor.GetMetadata().GetName()) + } + return names } -func TestSubstrateEmptyPagesPreserveUpstreamTokens(t *testing.T) { - ctx := pkgAuth.AuthSessionTo(t.Context(), &authimpl.SimpleSession{P: pkgAuth.Principal{User: pkgAuth.User{ID: "user"}}}) - service := system.NewService(nil, []string{"team"}, &pkgAuth.NoopAuthorizer{}, &emptyPageATEClient{t: t}) - page := &apiv1.PageRequest{Limit: 7, PageToken: "upstream:opaque/cursor="} - actors, err := service.ListSubstrateActors(ctx, &apiv1.ListSubstrateActorsRequest{Atespace: "team", Page: page}) - require.NoError(t, err) - assert.Empty(t, actors.Actors) - assert.Empty(t, actors.ATEAPIError) - assert.Equal(t, "upstream:next/cursor=", actors.NextPageToken) - workers, err := service.ListSubstrateWorkers(ctx, &apiv1.ListSubstrateWorkersRequest{Page: page}) - require.NoError(t, err) - assert.Empty(t, workers.Workers) - assert.Empty(t, workers.ATEAPIError) - assert.Equal(t, "upstream:next/cursor=", workers.NextPageToken) +func templateNames(templates []*ateapipb.ActorTemplate) []string { + names := []string{} + for _, template := range templates { + names = append(names, template.GetMetadata().GetName()) + } + return names } diff --git a/go/core/internal/service/system/substrate.go b/go/core/internal/service/system/substrate.go index 33349bbff2..27abb4619b 100644 --- a/go/core/internal/service/system/substrate.go +++ b/go/core/internal/service/system/substrate.go @@ -2,6 +2,8 @@ package system import ( "context" + "encoding/base64" + "errors" "fmt" "maps" "slices" @@ -10,6 +12,10 @@ import ( atev1alpha1 "github.com/agent-substrate/substrate/pkg/api/v1alpha1" "github.com/agent-substrate/substrate/pkg/proto/ateapipb" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" + corev1 "k8s.io/api/core/v1" + "sigs.k8s.io/controller-runtime/pkg/client" apiv1 "github.com/kagent-dev/kagent/go/api/gen/kagent/api/v1" "github.com/kagent-dev/kagent/go/core/internal/service/serviceerrors" @@ -76,7 +82,7 @@ type SubstrateActorStatusCount struct { // ate-api reports no totals, so every count costs a walk of its pages. The walk holds // one page at a time and keeps only tallies. func (s *Service) GetSubstrateSummary(ctx context.Context, requestedNamespace, atespace string) (SubstrateSummary, error) { - namespaces, err := s.substrateScope(ctx, requestedNamespace) + namespaces, err := s.substrateScope(ctx, requestedNamespace, atespace) if err != nil { return SubstrateSummary{}, err } @@ -99,8 +105,6 @@ func (s *Service) GetSubstrateSummary(ctx context.Context, requestedNamespace, a return strings.Compare(left.Namespace+"/"+left.Name, right.Namespace+"/"+right.Name) }) - allowAll, allowed := substrateScopeFilter(namespaces) - // Three independent reads: none gates the others, so one failure leaves the rest // counted rather than zeroing the whole summary. if templates, err := s.substrateActorTemplates(ctx, atespace); err != nil { @@ -111,9 +115,6 @@ func (s *Service) GetSubstrateSummary(ctx context.Context, requestedNamespace, a statusCounts := map[ateapipb.ActorState]int64{} if err := s.walkActors(ctx, atespace, func(actor *ateapipb.Actor) { - if actor == nil { - return - } result.ActorCount++ statusCounts[actor.GetStatus().GetState()]++ if actor.GetStatus().GetState() == ateapipb.ActorState_ACTOR_STATE_RUNNING { @@ -123,10 +124,7 @@ func (s *Service) GetSubstrateSummary(ctx context.Context, requestedNamespace, a result.recordATEError(ctx, err) } - if err := s.walkWorkers(ctx, func(worker *ateapipb.Worker) { - if worker == nil || !allowedWorkerNamespace(worker.GetWorkerNamespace(), allowAll, allowed) { - return - } + if err := s.walkWorkers(ctx, namespaces, func(worker *ateapipb.Worker) { result.WorkerCount++ if worker.GetStatus().GetAllocated().GetActors() > 0 { result.BusyWorkerCount++ @@ -145,13 +143,36 @@ func (s *Service) GetSubstrateSummary(ctx context.Context, requestedNamespace, a return result, nil } -// ListSubstrateActors returns one upstream page without changing its order or token. +// ListSubstrateActors returns one page of actors. A controller that watches every +// namespace passes ate-api's page and token through. A controller that watches some +// reads each of its atespaces with its own scoped list, so ate-api reads, and no page +// token names, an actor of a namespace it does not watch. func (s *Service) ListSubstrateActors(ctx context.Context, input *apiv1.ListSubstrateActorsRequest) (SubstrateActorPage, error) { - if err := s.authorize(ctx, auth.VerbGet, auth.Resource{Type: "Substrate"}); err != nil { + if _, err := s.substrateScope(ctx, "", input.GetAtespace()); err != nil { return SubstrateActorPage{}, err } result := SubstrateActorPage{ComputedAt: time.Now().UTC()} - actors, next, err := s.ateClient.ListActorsPage(ctx, input.GetAtespace(), substratePageSize(input.GetPage().GetLimit()), input.GetPage().GetPageToken()) + limit, token := substratePageSize(input.GetPage().GetLimit()), input.GetPage().GetPageToken() + var actors []*ateapipb.Actor + var next string + var err error + if len(s.observedNamespaces) == 0 { + actors, next, err = s.ateClient.ListActorsPage(ctx, input.GetAtespace(), limit, token) + } else { + atespaces := s.substrateNamespaces(input.GetAtespace()) + start, inner := 0, "" + if token != "" { + // A token without a watched atespace is refused rather than read as page one. + decoded, decodeErr := base64.RawURLEncoding.DecodeString(token) + atespace, rest, ok := strings.Cut(string(decoded), "/") + start = slices.Index(atespaces, atespace) + if len(token) > maxPageTokenBytes || decodeErr != nil || !ok || atespace == "" || start < 0 { + return SubstrateActorPage{}, errInvalidPageToken + } + inner = rest + } + actors, next, err = s.listScopedActors(ctx, atespaces[start:], inner, limit) + } if err != nil { result.ATEAPIError = err.Error() logging.FromContext(ctx).ErrorContext(ctx, "failed to list ate-api actors", "error", err) @@ -162,23 +183,67 @@ func (s *Service) ListSubstrateActors(ctx context.Context, input *apiv1.ListSubs return result, nil } -// ListSubstrateWorkers filters one upstream page by namespace and preserves its token. -// A page with no workers in scope may still have a next page. +// actorPageToken is the page token of a controller that watches some namespaces: the +// atespace to read next, and ate-api's token inside it. An atespace holds no "/". +func actorPageToken(atespace, inner string) string { + return base64.RawURLEncoding.EncodeToString([]byte(atespace + "/" + inner)) +} + +// The longest page token this controller accepts. Its own tokens are far shorter. +const maxPageTokenBytes = 1024 + +// errInvalidPageToken carries no cause: the token is the caller's, and its text can +// name anything. +var errInvalidPageToken = serviceerrors.NewInvalidArgument("Invalid page token", nil) + +// listScopedActors fills one page from the atespaces in order, each with its own scoped +// list, starting at ate-api's inner token in the first. +func (s *Service) listScopedActors(ctx context.Context, atespaces []string, inner string, limit int32) ([]*ateapipb.Actor, string, error) { + var actors []*ateapipb.Actor + for i, atespace := range atespaces { + rows, next, err := s.ateClient.ListActorsPage(ctx, atespace, limit-int32(len(actors)), inner) + if err != nil { + return nil, "", err + } + for _, actor := range rows { + // ate-api scoped the list already. This is a second guard. + if actor.GetMetadata().GetAtespace() == atespace { + actors = append(actors, actor) + } + } + switch { + case next != "": + return actors, actorPageToken(atespace, next), nil + case int32(len(actors)) >= limit && i+1 < len(atespaces): + return actors, actorPageToken(atespaces[i+1], ""), nil + } + inner = "" + } + return actors, "", nil +} + +// ListSubstrateWorkers returns one page of workers. A controller that watches every +// namespace filters ate-api's page and passes its token through. A controller that +// watches some reads the Worker of each of its own worker pods, so ate-api reads, and +// no page token names, a worker of a namespace it does not watch. func (s *Service) ListSubstrateWorkers(ctx context.Context, input *apiv1.ListSubstrateWorkersRequest) (SubstrateWorkerPage, error) { - namespaces, err := s.substrateScope(ctx, input.GetNamespace()) + namespaces, err := s.substrateScope(ctx, input.GetNamespace(), "") if err != nil { return SubstrateWorkerPage{}, err } + limit, token := substratePageSize(input.GetPage().GetLimit()), input.GetPage().GetPageToken() + if len(s.observedNamespaces) > 0 { + return s.listPodWorkers(ctx, namespaces, limit, token) + } result := SubstrateWorkerPage{ComputedAt: time.Now().UTC()} - workers, next, err := s.ateClient.ListWorkersPage(ctx, substratePageSize(input.GetPage().GetLimit()), input.GetPage().GetPageToken()) + workers, next, err := s.ateClient.ListWorkersPage(ctx, limit, token) if err != nil { result.ATEAPIError = err.Error() logging.FromContext(ctx).ErrorContext(ctx, "failed to list ate-api workers", "error", err) return result, nil } - allowAll, allowed := substrateScopeFilter(namespaces) for _, worker := range workers { - if worker != nil && allowedWorkerNamespace(worker.GetWorkerNamespace(), allowAll, allowed) { + if inNamespace(worker, namespaces[0]) { result.Workers = append(result.Workers, worker) } } @@ -186,17 +251,124 @@ func (s *Service) ListSubstrateWorkers(ctx context.Context, input *apiv1.ListSub return result, nil } -// walkActors visits every actor in the requested atespace, one page at a time. +// listPodWorkers fills one page with the Workers of the worker pods in the namespaces, +// in pod UID order, which is ate-api's worker order. The token is the UID of the last +// worker pod on the previous page: a cursor only, never sent to ate-api. +func (s *Service) listPodWorkers(ctx context.Context, namespaces []string, limit int32, token string) (SubstrateWorkerPage, error) { + if len(token) > maxPageTokenBytes { + return SubstrateWorkerPage{}, errInvalidPageToken + } + result := SubstrateWorkerPage{ComputedAt: time.Now().UTC()} + pods, err := s.workerPods(ctx, namespaces) + if err != nil { + result.ATEAPIError = err.Error() + return result, nil + } + for i := range pods { + if string(pods[i].UID) <= token { + continue + } + worker, err := s.podWorker(ctx, &pods[i]) + if err != nil { + return SubstrateWorkerPage{ATEAPIError: err.Error(), ComputedAt: result.ComputedAt}, nil + } + if worker == nil { + continue + } + result.Workers = append(result.Workers, worker) + if int32(len(result.Workers)) >= limit { + if i+1 < len(pods) { + result.NextPageToken = string(pods[i].UID) + } + break + } + } + return result, nil +} + +// WorkerPoolLabel names the WorkerPool of a Substrate worker pod. ate-api names each +// Worker after its pod's UID. +const WorkerPoolLabel = "ate.dev/worker-pool" + +// workerPods lists the worker pods of the namespaces in UID order. +func (s *Service) workerPods(ctx context.Context, namespaces []string) ([]corev1.Pod, error) { + var pods []corev1.Pod + for _, namespace := range namespaces { + podList := &corev1.PodList{} + // ponytail: one uncached apiserver list per watched namespace per read. Add a + // label-limited cache if UI polling load matters. + if err := s.podReader.List(ctx, podList, client.InNamespace(namespace), client.HasLabels{WorkerPoolLabel}); err != nil { + logging.FromContext(ctx).ErrorContext(ctx, "failed to list worker pods", "error", err, "namespace", namespace) + return nil, errors.New("failed to list worker pods") + } + pods = append(pods, podList.Items...) + } + slices.SortFunc(pods, func(left, right corev1.Pod) int { return strings.Compare(string(left.UID), string(right.UID)) }) + return pods, nil +} + +// podWorker reads the Worker of a worker pod. It is nil when ate-api has none yet, +// or when the Worker is not that pod's. +func (s *Service) podWorker(ctx context.Context, pod *corev1.Pod) (*ateapipb.Worker, error) { + worker, err := s.ateClient.GetWorker(ctx, string(pod.UID)) + if status.Code(err) == codes.NotFound { + // ate-api registers a worker pod only when it is Ready. + return nil, nil + } + if err != nil { + // ate-api error text can name a worker of any namespace. The user gets the code. + logging.FromContext(ctx).ErrorContext(ctx, "failed to get ate-api worker", "error", err, "pod", pod.Name) + return nil, fmt.Errorf("ate-api could not read a worker: %s", status.Code(err)) + } + if worker.GetWorkerNamespace() != pod.Namespace || worker.GetWorkerPodUid() != string(pod.UID) { + return nil, nil + } + return worker, nil +} + +// walkActors visits every actor in the requested atespace, or in each watched one. func (s *Service) walkActors(ctx context.Context, atespace string, visit func(*ateapipb.Actor)) error { - read := func(ctx context.Context, pageSize int32, pageToken string) ([]*ateapipb.Actor, string, error) { - return s.ateClient.ListActorsPage(ctx, atespace, pageSize, pageToken) + for _, atespace := range s.substrateNamespaces(atespace) { + read := func(ctx context.Context, pageSize int32, pageToken string) ([]*ateapipb.Actor, string, error) { + return s.ateClient.ListActorsPage(ctx, atespace, pageSize, pageToken) + } + if err := walkSubstrate(ctx, read, func(actor *ateapipb.Actor) { + if actor != nil && s.watches(actor.GetMetadata().GetAtespace()) { + visit(actor) + } + }); err != nil { + return err + } } - return walkSubstrate(ctx, read, visit) + return nil } -// walkWorkers calls visit for every worker ate-api holds, one page at a time. -func (s *Service) walkWorkers(ctx context.Context, visit func(*ateapipb.Worker)) error { - return walkSubstrate(ctx, s.ateClient.ListWorkersPage, visit) +// walkWorkers visits every worker in the namespaces: from ate-api's pages for a +// controller that watches every namespace, else the Worker of each worker pod. +func (s *Service) walkWorkers(ctx context.Context, namespaces []string, visit func(*ateapipb.Worker)) error { + if len(s.observedNamespaces) == 0 { + return walkSubstrate(ctx, s.ateClient.ListWorkersPage, func(worker *ateapipb.Worker) { + if inNamespace(worker, namespaces[0]) { + visit(worker) + } + }) + } + pods, err := s.workerPods(ctx, namespaces) + if err != nil { + return err + } + // ponytail: one sequential GetWorker per own worker pod (about 1 ms each). Add + // bounded concurrency when a controller runs more than about 100 own workers. + for i := range pods { + worker, err := s.podWorker(ctx, &pods[i]) + if err != nil { + return err + } + if worker != nil { + visit(worker) + } + } + return nil } // walkSubstrate calls visit for every row ate-api holds, holding one page at a time @@ -226,35 +398,30 @@ func walkSubstrate[Row any]( } // substrateScope authorizes the caller and resolves the namespaces a read covers. -// ATE-only actor reads authorize independently of Kubernetes scope. -func (s *Service) substrateScope(ctx context.Context, requestedNamespace string) ([]string, error) { +// A controller that watches namespaces refuses a namespace or atespace it does not watch: +// kagent names each atespace after its namespace, and ate-api serves every tenant. +func (s *Service) substrateScope(ctx context.Context, requestedNamespace, requestedAtespace string) ([]string, error) { if err := s.authorize(ctx, auth.VerbGet, auth.Resource{Type: "Substrate"}); err != nil { return nil, err } + for _, requested := range []string{requestedNamespace, requestedAtespace} { + if requested != "" && !s.watches(requested) { + return nil, serviceerrors.NewPermissionDenied("Not authorized", fmt.Errorf("namespace %q is not watched", requested)) + } + } return s.substrateNamespaces(requestedNamespace), nil } -// substrateScopeFilter turns resolved namespaces into the pair every row filter here -// takes: whether every namespace is in scope, and the set that is when it is not. -func substrateScopeFilter(namespaces []string) (bool, map[string]struct{}) { - allowAll := len(namespaces) == 1 && namespaces[0] == "" - allowed := make(map[string]struct{}, len(namespaces)) - for _, namespace := range namespaces { - if namespace != "" { - allowed[namespace] = struct{}{} - } - } - return allowAll, allowed +// watches reports whether this controller manages the namespace, or the atespace of +// the same name. +func (s *Service) watches(namespace string) bool { + return len(s.observedNamespaces) == 0 || slices.Contains(s.observedNamespaces, namespace) } -// allowedWorkerNamespace filters workers by their Kubernetes namespace. -func allowedWorkerNamespace(namespace string, allowAll bool, allowed map[string]struct{}) bool { - namespace = strings.TrimSpace(namespace) - if allowAll || namespace == "" { - return true - } - _, ok := allowed[namespace] - return ok +// inNamespace reports whether a worker is in the namespace, or in any when it is empty. +// A controller that watches every namespace reads exactly one, maybe empty. +func inNamespace(worker *ateapipb.Worker, namespace string) bool { + return worker != nil && (namespace == "" || worker.GetWorkerNamespace() == namespace) } func substratePageSize(requested int32) int32 { diff --git a/go/core/internal/service/system/substrate_paging_test.go b/go/core/internal/service/system/substrate_paging_test.go new file mode 100644 index 0000000000..7383053bf5 --- /dev/null +++ b/go/core/internal/service/system/substrate_paging_test.go @@ -0,0 +1,819 @@ +package system_test + +import ( + "bytes" + "context" + "encoding/base64" + "errors" + "fmt" + "log/slog" + "slices" + "strconv" + "strings" + "testing" + + atev1alpha1 "github.com/agent-substrate/substrate/pkg/api/v1alpha1" + "github.com/agent-substrate/substrate/pkg/proto/ateapipb" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + ctrlclient "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + + apiv1 "github.com/kagent-dev/kagent/go/api/gen/kagent/api/v1" + authimpl "github.com/kagent-dev/kagent/go/core/internal/httpserver/auth" + "github.com/kagent-dev/kagent/go/core/internal/service/serviceerrors" + "github.com/kagent-dev/kagent/go/core/internal/service/system" + pkgAuth "github.com/kagent-dev/kagent/go/core/pkg/auth" + "github.com/kagent-dev/kagent/go/pkg/logging" +) + +func pagingContext(t *testing.T) context.Context { + return pkgAuth.AuthSessionTo(t.Context(), &authimpl.SimpleSession{P: pkgAuth.Principal{User: pkgAuth.User{ID: "user"}}}) +} + +// pagingATEClient holds the paging tests' data: three tenants whose rows mix in +// ate-api's order. Every tenant-b name holds "tenant-b", so a test can look for it in +// any text. +func pagingATEClient() *fakeATEClient { + client := &fakeATEClient{} + for _, tenant := range []struct { + atespace string + actors int + }{{"tenant-a", 5}, {"tenant-b", 4}, {"tenant-c", 3}} { + for i := 1; i <= tenant.actors; i++ { + client.actors = append(client.actors, namedActor(tenant.atespace, tenant.atespace+"-actor-"+strconv.Itoa(i))) + } + client.templates = append(client.templates, &ateapipb.ActorTemplate{ + Metadata: &ateapipb.ResourceMetadata{Atespace: tenant.atespace, Name: tenant.atespace + "-template"}, + }) + } + for _, pod := range pagingPods() { + if pod.Labels[system.WorkerPoolLabel] != "" { + client.workers = append(client.workers, ateWorker(pod.Namespace, string(pod.UID), busyPods[string(pod.UID)])) + } + } + return client +} + +// The actors placed on each busy worker pod, by pod UID. +var busyPods = map[string]int32{"u1-tenant-a": 2, "u2-tenant-b": 1, "u3-tenant-c": 1} + +// pagingPods are the worker pods of the paging tests. Their UIDs mix the tenants in +// UID order, which is ate-api's worker order. One tenant-a pod is not a worker pod. +func pagingPods() []*corev1.Pod { + return []*corev1.Pod{ + workerPod("tenant-a", "", "u0-tenant-a"), + workerPod("tenant-a", "pool-a", "u1-tenant-a"), + workerPod("tenant-b", "pool-b", "u2-tenant-b"), + workerPod("tenant-c", "pool-c", "u3-tenant-c"), + workerPod("tenant-a", "pool-a", "u4-tenant-a"), + workerPod("tenant-b", "pool-b", "u5-tenant-b"), + workerPod("tenant-a", "pool-a", "u6-tenant-a"), + } +} + +// ownWorkerUIDs are the workers of tenant-a and tenant-c, in UID order. +var ownWorkerUIDs = []string{"u1-tenant-a", "u3-tenant-c", "u4-tenant-a", "u6-tenant-a"} + +// workerPod is a running worker pod of the pool, or a plain pod when pool is empty. +func workerPod(namespace, pool, uid string) *corev1.Pod { + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{Namespace: namespace, Name: "pod-" + uid, UID: types.UID(uid)}, + Status: corev1.PodStatus{ + Phase: corev1.PodRunning, + Conditions: []corev1.PodCondition{{Type: corev1.PodReady, Status: corev1.ConditionTrue}}, + }, + } + if pool != "" { + pod.Labels = map[string]string{system.WorkerPoolLabel: pool} + } + return pod +} + +// ateWorker is the Worker ate-api holds for a worker pod: it is named after the pod UID. +func ateWorker(namespace, uid string, allocatedActors int32) *ateapipb.Worker { + return &ateapipb.Worker{ + Metadata: &ateapipb.ResourceMetadata{Name: uid}, + WorkerNamespace: namespace, + WorkerPod: "pod-" + uid, + WorkerPodUid: uid, + Status: &ateapipb.WorkerStatus{Allocated: &ateapipb.WorkerResources{Actors: allocatedActors}}, + } +} + +// pagingKubeClient holds the paging pods, and a WorkerPool in each tenant. +func pagingKubeClient(t *testing.T, funcs interceptor.Funcs, pods ...*corev1.Pod) ctrlclient.WithWatch { + scheme := runtime.NewScheme() + require.NoError(t, corev1.AddToScheme(scheme)) + require.NoError(t, atev1alpha1.AddToScheme(scheme)) + objects := []ctrlclient.Object{} + for _, pod := range append(pagingPods(), pods...) { + objects = append(objects, pod) + } + for _, tenant := range []string{"tenant-a", "tenant-b", "tenant-c"} { + objects = append(objects, &atev1alpha1.WorkerPool{ObjectMeta: metav1.ObjectMeta{Namespace: tenant, Name: "pool"}}) + } + return fake.NewClientBuilder().WithScheme(scheme).WithObjects(objects...).WithInterceptorFuncs(funcs).Build() +} + +// walkWorkers reads every page of workers and fails the test on a hard or soft error. +func walkWorkers(t *testing.T, ctx context.Context, service *system.Service, namespace string, limit int32) []system.SubstrateWorkerPage { + t.Helper() + pages := []system.SubstrateWorkerPage{} + pageToken := "" + for range 100 { + page, err := service.ListSubstrateWorkers(ctx, &apiv1.ListSubstrateWorkersRequest{Namespace: namespace, Page: &apiv1.PageRequest{Limit: limit, PageToken: pageToken}}) + require.NoError(t, err) + require.Empty(t, page.ATEAPIError) + pages = append(pages, page) + if pageToken = page.NextPageToken; pageToken == "" { + return pages + } + } + t.Fatal("the walk did not end") + return nil +} + +func walkedWorkerUIDs(pages []system.SubstrateWorkerPage) []string { + uids := []string{} + for _, page := range pages { + for _, worker := range page.Workers { + uids = append(uids, worker.GetWorkerPodUid()) + } + } + return uids +} + +// ownActorNames is every actor of the watched atespaces, in the order a walk returns them. +func ownActorNames(client *fakeATEClient, watched []string) []string { + names := []string{} + for _, atespace := range slices.Sorted(slices.Values(watched)) { + for _, actor := range client.actors { + if actor.GetMetadata().GetAtespace() == atespace && !slices.Contains(names, actor.GetMetadata().GetName()) { + names = append(names, actor.GetMetadata().GetName()) + } + } + } + return names +} + +// walkActors reads every page of actors and fails the test on a hard or soft error. +func walkActors(t *testing.T, ctx context.Context, service *system.Service, atespace string, limit int32) []system.SubstrateActorPage { + t.Helper() + pages := []system.SubstrateActorPage{} + pageToken := "" + for range 100 { + page, err := service.ListSubstrateActors(ctx, &apiv1.ListSubstrateActorsRequest{Atespace: atespace, Page: &apiv1.PageRequest{Limit: limit, PageToken: pageToken}}) + require.NoError(t, err) + require.Empty(t, page.ATEAPIError) + pages = append(pages, page) + if pageToken = page.NextPageToken; pageToken == "" { + return pages + } + } + t.Fatal("the walk did not end") + return nil +} + +func walkedActorNames(pages []system.SubstrateActorPage) []string { + names := []string{} + for _, page := range pages { + names = append(names, actorNames(page.Actors)...) + } + return names +} + +func namedActor(atespace, name string) *ateapipb.Actor { + return substrateActor(name, atespace, ateapipb.ActorState_ACTOR_STATE_RUNNING, "", "") +} + +// tokenAtespace reads the atespace a kagent actor page token names. +func tokenAtespace(t *testing.T, token string) string { + raw, err := base64.RawURLEncoding.DecodeString(token) + require.NoError(t, err) + atespace, _, _ := strings.Cut(string(raw), "/") + return atespace +} + +// A page that runs out of rows in one watched atespace goes on in the next, and its +// token names where it stopped. +func TestListSubstrateActorsFanOut(t *testing.T) { + ctx := pagingContext(t) + ateClient := &fakeATEClient{actors: []*ateapipb.Actor{ + namedActor("tenant-a", "a-1"), namedActor("tenant-b", "b-1"), namedActor("tenant-c", "c-1"), + namedActor("tenant-a", "a-2"), namedActor("tenant-c", "c-2"), namedActor("tenant-a", "a-3"), + }} + service := system.NewService(nil, nil, []string{"tenant-a", "tenant-c"}, &pkgAuth.NoopAuthorizer{}, ateClient) + + first, err := service.ListSubstrateActors(ctx, &apiv1.ListSubstrateActorsRequest{Page: &apiv1.PageRequest{Limit: 2}}) + require.NoError(t, err) + assert.Equal(t, []string{"a-1", "a-2"}, actorNames(first.Actors)) + assert.Equal(t, []ateCall{{method: "ListActorsPage", scope: "tenant-a", size: 2}}, ateClient.calls) + + second, err := service.ListSubstrateActors(ctx, &apiv1.ListSubstrateActorsRequest{Page: &apiv1.PageRequest{Limit: 2, PageToken: first.NextPageToken}}) + require.NoError(t, err) + assert.Equal(t, []string{"a-3", "c-1"}, actorNames(second.Actors)) + assert.Equal(t, []ateCall{ + {method: "ListActorsPage", scope: "tenant-a", token: "upstream:tenant-a:tenant-a/a-2", size: 2}, + {method: "ListActorsPage", scope: "tenant-c", size: 1}, + }, ateClient.calls[1:]) + assert.Equal(t, "tenant-c", tokenAtespace(t, second.NextPageToken)) +} + +func token(raw string) string { + return base64.RawURLEncoding.EncodeToString([]byte(raw)) +} + +// A Substrate actor token, as the released controller passed it through. It names a +// tenant-b session. +const substrateActorToken = `{"v":1,"kind":"actor","scope":"","last":["tenant-b","session-1"]}` + +// badActorTokens are tokens a controller that watches tenant-a and tenant-c did not +// issue. None of them may start a walk again at page one. +var badActorTokens = map[string]string{ + "no separator": `tenant-a`, + "empty atespace": `/x`, + "unwatched atespace": `tenant-b/x`, + "worker token": `u1-tenant-a`, + "substrate token, raw base64": substrateActorToken, +} + +// rejectsToken asserts that a read with the token is a hard InvalidArgument that +// reaches neither ate-api nor the log or the error with a tenant-b name. +func rejectsToken(t *testing.T, ateClient *fakeATEClient, read func(context.Context) error) { + t.Helper() + var log bytes.Buffer + ctx := logging.IntoContext(pagingContext(t), slog.New(slog.NewTextHandler(&log, nil))) + err := read(ctx) + require.Error(t, err) + assert.True(t, serviceerrors.IsCode(err, serviceerrors.CodeInvalidArgument), "err = %v", err) + assert.NotContains(t, err.Error(), "tenant-b") + assert.NotContains(t, log.String(), "tenant-b") + assert.Empty(t, ateClient.calls, "a bad token must not reach ate-api") +} + +func listActorsWithToken(service *system.Service, pageToken string) func(context.Context) error { + return func(ctx context.Context) error { + _, err := service.ListSubstrateActors(ctx, &apiv1.ListSubstrateActorsRequest{Page: &apiv1.PageRequest{Limit: 2, PageToken: pageToken}}) + return err + } +} + +func TestListSubstrateActorsStrictToken(t *testing.T) { + tokens := map[string]string{ + "not base64": "not base64!", + "substrate token, std base64": base64.StdEncoding.EncodeToString([]byte(substrateActorToken)), + "longer than 1 KiB, else valid": token("tenant-a/" + strings.Repeat("x", 800)), + } + for name, raw := range badActorTokens { + tokens[name] = token(raw) + } + for name, pageToken := range tokens { + t.Run(name, func(t *testing.T) { + ateClient := &fakeATEClient{} + service := system.NewService(nil, nil, []string{"tenant-a", "tenant-c"}, &pkgAuth.NoopAuthorizer{}, ateClient) + rejectsToken(t, ateClient, listActorsWithToken(service, pageToken)) + }) + } +} + +// ate-api checks the token inside a kagent token: a forged one cannot read another +// atespace, and its refusal names nothing. +func TestListSubstrateActorsForeignInnerToken(t *testing.T) { + ateClient := pagingATEClient() + service := system.NewService(nil, nil, []string{"tenant-a", "tenant-c"}, &pkgAuth.NoopAuthorizer{}, ateClient) + + page, err := service.ListSubstrateActors(pagingContext(t), &apiv1.ListSubstrateActorsRequest{Page: &apiv1.PageRequest{ + PageToken: token("tenant-a/upstream:tenant-b:tenant-b/tenant-b-actor-1"), + }}) + require.NoError(t, err) + assert.Empty(t, page.Actors) + assert.Empty(t, page.NextPageToken) + assert.NotEmpty(t, page.ATEAPIError) + assert.NotContains(t, page.ATEAPIError, "tenant-b") + assert.Equal(t, []ateCall{{method: "ListActorsPage", scope: "tenant-a", token: "upstream:tenant-b:tenant-b/tenant-b-actor-1", size: 50}}, ateClient.calls) +} + +// The token names its atespace, not its place in the list, so a token survives a +// change to the watched list or fails loudly, and never reads a new neighbour. +func TestListSubstrateActorsTokenHoldsAtespaceName(t *testing.T) { + ctx := pagingContext(t) + ateClient := pagingATEClient() + first := system.NewService(nil, nil, []string{"tenant-a", "tenant-c"}, &pkgAuth.NoopAuthorizer{}, ateClient) + // tenant-a has five actors, so a page of six ends one row into tenant-c. + page, err := first.ListSubstrateActors(ctx, &apiv1.ListSubstrateActorsRequest{Page: &apiv1.PageRequest{Limit: 6}}) + require.NoError(t, err) + require.Equal(t, "tenant-c", tokenAtespace(t, page.NextPageToken)) + + t.Run("a wider list resumes in the same atespace", func(t *testing.T) { + ateClient := pagingATEClient() + wider := system.NewService(nil, nil, []string{"tenant-a", "tenant-b", "tenant-c"}, &pkgAuth.NoopAuthorizer{}, ateClient) + next, err := wider.ListSubstrateActors(ctx, &apiv1.ListSubstrateActorsRequest{Page: &apiv1.PageRequest{Limit: 6, PageToken: page.NextPageToken}}) + require.NoError(t, err) + assert.Equal(t, []string{"tenant-c-actor-2", "tenant-c-actor-3"}, actorNames(next.Actors)) + for _, call := range ateClient.calls { + assert.Equal(t, "tenant-c", call.scope) + } + }) + + t.Run("a narrower list refuses it", func(t *testing.T) { + ateClient := pagingATEClient() + narrower := system.NewService(nil, nil, []string{"tenant-a"}, &pkgAuth.NoopAuthorizer{}, ateClient) + rejectsToken(t, ateClient, listActorsWithToken(narrower, page.NextPageToken)) + }) +} + +// A page that fills at the end of one atespace offers the next one, which may turn out +// to be empty. That last page is accepted: knowing in advance costs a read per page. +func TestListSubstrateActorsEmptyLastPage(t *testing.T) { + ctx := pagingContext(t) + ateClient := &fakeATEClient{actors: []*ateapipb.Actor{namedActor("tenant-a", "a-1"), namedActor("tenant-a", "a-2")}} + service := system.NewService(nil, nil, []string{"tenant-a", "tenant-c"}, &pkgAuth.NoopAuthorizer{}, ateClient) + + pages := walkActors(t, ctx, service, "", 2) + require.Len(t, pages, 2) + assert.Equal(t, []string{"a-1", "a-2"}, actorNames(pages[0].Actors)) + assert.Empty(t, pages[1].Actors) + assert.Len(t, ateClient.calls, 2, "one read for each page") +} + +func TestListSubstrateActorsEmptyWatchedAtespace(t *testing.T) { + ctx := pagingContext(t) + ateClient := pagingATEClient() + watched := []string{"tenant-a", "empty", "tenant-c"} + service := system.NewService(nil, nil, watched, &pkgAuth.NoopAuthorizer{}, ateClient) + + assert.Equal(t, ownActorNames(ateClient, watched), walkedActorNames(walkActors(t, ctx, service, "", 2))) + assert.Equal(t, []string{"empty", "tenant-a", "tenant-c"}, calledScopes(ateClient, "ListActorsPage")) +} + +// ate-api scopes each list, and the controller still keeps only that atespace's rows. +func TestListSubstrateActorsKeepsRowFilter(t *testing.T) { + ctx := pagingContext(t) + ateClient := pagingATEClient() + ateClient.ignoreAtespace = true + watched := []string{"tenant-a", "tenant-c"} + service := system.NewService(nil, nil, watched, &pkgAuth.NoopAuthorizer{}, ateClient) + + assert.Equal(t, ownActorNames(ateClient, watched), walkedActorNames(walkActors(t, ctx, service, "", 100))) + assert.Equal(t, watched, calledScopes(ateClient, "ListActorsPage")) +} + +func TestListSubstrateActorsNamedAtespace(t *testing.T) { + ctx := pagingContext(t) + ateClient := pagingATEClient() + service := system.NewService(nil, nil, []string{"tenant-a", "tenant-c"}, &pkgAuth.NoopAuthorizer{}, ateClient) + + pages := walkActors(t, ctx, service, "tenant-c", 1) + assert.Equal(t, ownActorNames(ateClient, []string{"tenant-c"}), walkedActorNames(pages)) + for _, call := range ateClient.calls { + assert.Equal(t, "tenant-c", call.scope) + } + for _, page := range pages[:len(pages)-1] { + assert.Equal(t, "tenant-c", tokenAtespace(t, page.NextPageToken)) + } +} + +// A page whose second read fails returns none of the first read's rows: a short page +// beside a warning would look like the end of the list. +func TestListSubstrateActorsLaterCallFails(t *testing.T) { + ateClient := &fakeATEClient{ + actors: []*ateapipb.Actor{namedActor("tenant-a", "a-1"), namedActor("tenant-c", "c-1")}, + failCall: 2, + failErr: errors.New("ate-api unreachable"), + } + service := system.NewService(nil, nil, []string{"tenant-a", "tenant-c"}, &pkgAuth.NoopAuthorizer{}, ateClient) + + page, err := service.ListSubstrateActors(pagingContext(t), &apiv1.ListSubstrateActorsRequest{Page: &apiv1.PageRequest{Limit: 2}}) + require.NoError(t, err) + assert.Equal(t, "ate-api unreachable", page.ATEAPIError) + assert.Empty(t, page.Actors) + assert.Empty(t, page.NextPageToken) +} + +// A watched list with a repeat must not read an atespace twice, or offer a token that +// leads back to a page already read. +func TestNewServiceDeduplicatesWatched(t *testing.T) { + ctx := pagingContext(t) + ateClient := pagingATEClient() + service := system.NewService(nil, nil, []string{"tenant-c", "tenant-a", "tenant-a"}, &pkgAuth.NoopAuthorizer{}, ateClient) + + assert.Equal(t, ownActorNames(ateClient, []string{"tenant-a", "tenant-c"}), walkedActorNames(walkActors(t, ctx, service, "", 1))) + assert.Equal(t, []string{"tenant-a", "tenant-c"}, calledScopes(ateClient, "ListActorsPage"), "sorted") +} + +// The app passes an empty list for an install that watches every namespace. +func TestNewServiceWatchAllStaysWatchAll(t *testing.T) { + for name, watched := range map[string][]string{"nil": nil, "empty": {}} { + t.Run(name, func(t *testing.T) { + ateClient := pagingATEClient() + service := system.NewService(nil, nil, watched, &pkgAuth.NoopAuthorizer{}, ateClient) + + page, err := service.ListSubstrateActors(pagingContext(t), &apiv1.ListSubstrateActorsRequest{Page: &apiv1.PageRequest{Limit: 100}}) + require.NoError(t, err) + assert.Len(t, page.Actors, len(ateClient.actors)) + assert.Equal(t, []ateCall{{method: "ListActorsPage", size: 100}}, ateClient.calls) + }) + } +} + +// The ate-api reads of a full walk, counted by hand from the paging data: tenant-a +// has five actors and tenant-c three. +func TestListSubstrateActorsCallCounts(t *testing.T) { + ctx := pagingContext(t) + for limit, want := range map[int32]int{1: 8, 2: 5, 3: 4, 5: 2, 50: 2} { + t.Run(strconv.Itoa(int(limit)), func(t *testing.T) { + ateClient := pagingATEClient() + service := system.NewService(nil, nil, []string{"tenant-a", "tenant-c"}, &pkgAuth.NoopAuthorizer{}, ateClient) + + calls, pageToken := 0, "" + for range 100 { + page, err := service.ListSubstrateActors(ctx, &apiv1.ListSubstrateActorsRequest{Page: &apiv1.PageRequest{Limit: limit, PageToken: pageToken}}) + require.NoError(t, err) + pageCalls := ateClient.calls[calls:] + calls += len(pageCalls) + read := map[string]bool{} + for _, call := range pageCalls { + assert.False(t, read[call.scope], "page %v read %s twice", actorNames(page.Actors), call.scope) + read[call.scope] = true + } + if pageToken = page.NextPageToken; pageToken == "" { + break + } + } + assert.Equal(t, want, calls) + }) + } +} + +// Workers are read one by one, by the UID of a worker pod in a watched namespace. +func TestListSubstrateWorkersFromOwnPods(t *testing.T) { + ctx := pagingContext(t) + ateClient := pagingATEClient() + kubeClient := pagingKubeClient(t, interceptor.Funcs{}) + service := system.NewService(kubeClient, kubeClient, []string{"tenant-a", "tenant-c"}, &pkgAuth.NoopAuthorizer{}, ateClient) + + pages := walkWorkers(t, ctx, service, "", 1) + require.Len(t, pages, len(ownWorkerUIDs), "the last worker ends the walk") + assert.Equal(t, ownWorkerUIDs, walkedWorkerUIDs(pages)) + for i, page := range pages[:len(pages)-1] { + assert.Equal(t, ownWorkerUIDs[i], page.NextPageToken) + } + for _, call := range ateClient.calls { + assert.Equal(t, "GetWorker", call.method) + assert.Contains(t, ownWorkerUIDs, call.scope) + } +} + +func newPagingWorkerService(t *testing.T, ateClient *fakeATEClient, funcs interceptor.Funcs, pods ...*corev1.Pod) *system.Service { + kubeClient := pagingKubeClient(t, funcs, pods...) + return system.NewService(kubeClient, kubeClient, []string{"tenant-a", "tenant-c"}, &pkgAuth.NoopAuthorizer{}, ateClient) +} + +// A worker pod that is not Ready has no Worker yet. The page fills from later pods. +func TestListSubstrateWorkersSkipsUnregisteredPod(t *testing.T) { + ateClient := pagingATEClient() + service := newPagingWorkerService(t, ateClient, interceptor.Funcs{}, workerPod("tenant-a", "pool-a", "u2a-tenant-a")) + + pages := walkWorkers(t, pagingContext(t), service, "", 2) + assert.Equal(t, []string{"u1-tenant-a", "u3-tenant-c"}, walkedWorkerUIDs(pages[:1])) + assert.Equal(t, ownWorkerUIDs, walkedWorkerUIDs(pages)) +} + +// A Worker whose pod is gone is not shown, and not read. +func TestListSubstrateWorkersHidesStaleWorker(t *testing.T) { + ateClient := pagingATEClient() + ateClient.workers = append(ateClient.workers, ateWorker("tenant-a", "u9-tenant-a", 0)) + service := newPagingWorkerService(t, ateClient, interceptor.Funcs{}) + + assert.Equal(t, ownWorkerUIDs, walkedWorkerUIDs(walkWorkers(t, pagingContext(t), service, "", 100))) + assert.NotContains(t, ateClient.calls, ateCall{method: "GetWorker", scope: "u9-tenant-a"}) +} + +// A Worker that names another namespace or another pod than the pod it was read for +// is not that pod's Worker. +func TestListSubstrateWorkersChecksWorkerMatchesPod(t *testing.T) { + ateClient := pagingATEClient() + for _, worker := range ateClient.workers { + switch worker.GetWorkerPodUid() { + case "u4-tenant-a": + worker.WorkerNamespace = "tenant-b" + case "u6-tenant-a": + worker.WorkerPodUid = "u5-tenant-b" + } + } + service := newPagingWorkerService(t, ateClient, interceptor.Funcs{}) + + assert.Equal(t, []string{"u1-tenant-a", "u3-tenant-c"}, walkedWorkerUIDs(walkWorkers(t, pagingContext(t), service, "", 100))) +} + +// A worker token holds a UID only as a cursor. It is never sent to ate-api. +func TestListSubstrateWorkersForgedToken(t *testing.T) { + t.Run("a foreign UID only moves the cursor", func(t *testing.T) { + ateClient := pagingATEClient() + service := newPagingWorkerService(t, ateClient, interceptor.Funcs{}) + + page, err := service.ListSubstrateWorkers(pagingContext(t), &apiv1.ListSubstrateWorkersRequest{Page: &apiv1.PageRequest{PageToken: "u2-tenant-b"}}) + require.NoError(t, err) + assert.Equal(t, ownWorkerUIDs[1:], walkedWorkerUIDs([]system.SubstrateWorkerPage{page})) + for _, call := range ateClient.calls { + assert.Contains(t, ownWorkerUIDs, call.scope) + } + }) + + t.Run("longer than 1 KiB", func(t *testing.T) { + ateClient := pagingATEClient() + service := newPagingWorkerService(t, ateClient, interceptor.Funcs{}) + rejectsToken(t, ateClient, func(ctx context.Context) error { + _, err := service.ListSubstrateWorkers(ctx, &apiv1.ListSubstrateWorkersRequest{Page: &apiv1.PageRequest{PageToken: "u2-tenant-b" + strings.Repeat("x", 1024)}}) + return err + }) + }) +} + +func TestListSubstrateWorkersNamedNamespace(t *testing.T) { + ateClient := pagingATEClient() + service := newPagingWorkerService(t, ateClient, interceptor.Funcs{}) + + tenantA := []string{"u1-tenant-a", "u4-tenant-a", "u6-tenant-a"} + assert.Equal(t, tenantA, walkedWorkerUIDs(walkWorkers(t, pagingContext(t), service, "tenant-a", 1))) + assert.Equal(t, tenantA, calledScopes(ateClient, "GetWorker")) + assert.Empty(t, calledScopes(ateClient, "ListWorkersPage")) +} + +// ate-api error text can name a worker of another namespace. The user sees a fixed text. +func TestListSubstrateWorkersFixedErrorText(t *testing.T) { + ateClient := pagingATEClient() + ateClient.failCall = 2 + ateClient.failErr = status.Error(codes.Internal, "worker u2-tenant-b is on a node that failed") + service := newPagingWorkerService(t, ateClient, interceptor.Funcs{}) + + page, err := service.ListSubstrateWorkers(pagingContext(t), &apiv1.ListSubstrateWorkersRequest{}) + require.NoError(t, err) + assert.Equal(t, "ate-api could not read a worker: Internal", page.ATEAPIError) + assert.Empty(t, page.Workers, "no partial page") + assert.Empty(t, page.NextPageToken) +} + +func TestListSubstrateWorkersPodListFails(t *testing.T) { + ateClient := pagingATEClient() + service := newPagingWorkerService(t, ateClient, interceptor.Funcs{ + List: func(context.Context, ctrlclient.WithWatch, ctrlclient.ObjectList, ...ctrlclient.ListOption) error { + return errors.New("etcd is down") + }, + }) + + page, err := service.ListSubstrateWorkers(pagingContext(t), &apiv1.ListSubstrateWorkersRequest{}) + require.NoError(t, err) + assert.NotEmpty(t, page.ATEAPIError) + assert.Empty(t, page.Workers) + assert.Empty(t, page.NextPageToken) + assert.Empty(t, ateClient.calls) +} + +// ownSummary is the summary of tenant-a and tenant-c in the paging data. +func assertOwnSummary(t *testing.T, summary system.SubstrateSummary) { + t.Helper() + assert.Equal(t, int64(8), summary.ActorCount) + assert.Equal(t, int64(8), summary.RunningActorCount) + assert.Equal(t, []string{"tenant-a-template", "tenant-c-template"}, templateNames(summary.ActorTemplates)) + assert.Equal(t, int64(4), summary.WorkerCount) + assert.Equal(t, int64(2), summary.BusyWorkerCount) +} + +// calledScopes lists the atespace or worker of each call to the method. +func calledScopes(client *fakeATEClient, method string) []string { + scopes := []string{} + for _, call := range client.calls { + if call.method == method && !slices.Contains(scopes, call.scope) { + scopes = append(scopes, call.scope) + } + } + return scopes +} + +func TestGetSubstrateSummaryReadsOnlyWatched(t *testing.T) { + ateClient := pagingATEClient() + service := newPagingWorkerService(t, ateClient, interceptor.Funcs{}) + + summary, err := service.GetSubstrateSummary(pagingContext(t), "", "") + require.NoError(t, err) + assert.Empty(t, summary.ATEAPIError) + assertOwnSummary(t, summary) + assert.Equal(t, []string{"tenant-a", "tenant-c"}, calledScopes(ateClient, "ListActorsPage")) + assert.Equal(t, []string{"tenant-a", "tenant-c"}, calledScopes(ateClient, "ListActorTemplates")) + assert.ElementsMatch(t, ownWorkerUIDs, calledScopes(ateClient, "GetWorker")) + assert.Empty(t, calledScopes(ateClient, "ListWorkersPage")) +} + +func TestGetSubstrateSummaryInputs(t *testing.T) { + ateClient := pagingATEClient() + service := newPagingWorkerService(t, ateClient, interceptor.Funcs{}) + + summary, err := service.GetSubstrateSummary(pagingContext(t), "tenant-a", "tenant-c") + require.NoError(t, err) + assert.Equal(t, int64(3), summary.ActorCount) + assert.Equal(t, int64(3), summary.WorkerCount) + assert.Equal(t, []string{"tenant-c"}, calledScopes(ateClient, "ListActorsPage")) + assert.Equal(t, []string{"tenant-c"}, calledScopes(ateClient, "ListActorTemplates")) + assert.ElementsMatch(t, []string{"u1-tenant-a", "u4-tenant-a", "u6-tenant-a"}, calledScopes(ateClient, "GetWorker")) +} + +// An ate-api that drops the atespace filter cannot show another tenant's template, +// nor one template twice. +func TestGetSubstrateSummaryKeepsTemplateFilter(t *testing.T) { + ateClient := pagingATEClient() + ateClient.ignoreAtespace = true + service := newPagingWorkerService(t, ateClient, interceptor.Funcs{}) + + summary, err := service.GetSubstrateSummary(pagingContext(t), "", "") + require.NoError(t, err) + assert.Equal(t, []string{"tenant-a-template", "tenant-c-template"}, templateNames(summary.ActorTemplates)) + + summary, err = service.GetSubstrateSummary(pagingContext(t), "", "tenant-a") + require.NoError(t, err) + assert.Equal(t, []string{"tenant-a-template"}, templateNames(summary.ActorTemplates)) +} + +// One failed scoped read is a warning. The reads that did not fail still count. +func TestGetSubstrateSummaryPartialFailure(t *testing.T) { + ateClient := pagingATEClient() + ateClient.failCall = 2 // the tenant-c template list + ateClient.failErr = errors.New("templates unavailable") + service := newPagingWorkerService(t, ateClient, interceptor.Funcs{}) + + summary, err := service.GetSubstrateSummary(pagingContext(t), "", "") + require.NoError(t, err) + assert.Equal(t, "templates unavailable", summary.ATEAPIError) + assert.Empty(t, summary.ActorTemplates) + assert.Equal(t, int64(8), summary.ActorCount) + assert.Equal(t, int64(4), summary.WorkerCount) + assert.Equal(t, int64(2), summary.BusyWorkerCount) +} + +// A failed Worker read is a warning. Templates and actors still count. +func TestGetSubstrateSummaryWorkerUnavailable(t *testing.T) { + ateClient := pagingATEClient() + ateClient.failCall = 5 // the first GetWorker, after two template and two actor lists + ateClient.failErr = status.Error(codes.Unavailable, "worker u1-tenant-a is unreachable") + service := newPagingWorkerService(t, ateClient, interceptor.Funcs{}) + + summary, err := service.GetSubstrateSummary(pagingContext(t), "", "") + require.NoError(t, err) + assert.Equal(t, "ate-api could not read a worker: Unavailable", summary.ATEAPIError) + assert.Equal(t, []string{"tenant-a-template", "tenant-c-template"}, templateNames(summary.ActorTemplates)) + assert.Equal(t, int64(8), summary.ActorCount) + assert.Equal(t, int64(8), summary.RunningActorCount) + assert.Zero(t, summary.WorkerCount) +} + +// A failed worker pod list is a warning too. WorkerPools come from the cached client +// and still list. +func TestGetSubstrateSummaryPodListFails(t *testing.T) { + ateClient := pagingATEClient() + podReader := pagingKubeClient(t, interceptor.Funcs{ + List: func(context.Context, ctrlclient.WithWatch, ctrlclient.ObjectList, ...ctrlclient.ListOption) error { + return errors.New("etcd is down") + }, + }) + service := system.NewService(pagingKubeClient(t, interceptor.Funcs{}), podReader, []string{"tenant-a", "tenant-c"}, &pkgAuth.NoopAuthorizer{}, ateClient) + + summary, err := service.GetSubstrateSummary(pagingContext(t), "", "") + require.NoError(t, err) + assert.Equal(t, "failed to list worker pods", summary.ATEAPIError) + assert.Len(t, summary.WorkerPools, 2) + assert.Equal(t, []string{"tenant-a-template", "tenant-c-template"}, templateNames(summary.ActorTemplates)) + assert.Equal(t, int64(8), summary.ActorCount) + assert.Zero(t, summary.WorkerCount) + assert.Empty(t, calledScopes(ateClient, "GetWorker")) +} + +// Walks every RPC with each watched list and page size of the paging data. A walk +// reads, and its tokens name, only watched data. It returns each own row once, and no +// page is empty beside a token: that would tell the caller a hidden row exists. +func TestSubstrateWalksStayInWatchedNamespaces(t *testing.T) { + for _, watched := range [][]string{{"tenant-a", "tenant-c"}, {"tenant-a"}} { + for _, limit := range []int32{1, 2, 3, 4, 5, 6, 7, 100} { + t.Run(fmt.Sprintf("%v/%d", watched, limit), func(t *testing.T) { + ctx := pagingContext(t) + ateClient := pagingATEClient() + kubeClient := pagingKubeClient(t, interceptor.Funcs{}) + service := system.NewService(kubeClient, kubeClient, watched, &pkgAuth.NoopAuthorizer{}, ateClient) + ownWorkerIDs := []string{} + for _, uid := range ownWorkerUIDs { + if slices.ContainsFunc(watched, func(namespace string) bool { return strings.HasSuffix(uid, namespace) }) { + ownWorkerIDs = append(ownWorkerIDs, uid) + } + } + actorPages := walkActors(t, ctx, service, "", limit) + workerPages := walkWorkers(t, ctx, service, "", limit) + summary, err := service.GetSubstrateSummary(ctx, "", "") + require.NoError(t, err) + + for _, call := range ateClient.calls { + switch call.method { + case "ListActorsPage", "ListActorTemplates": + assert.Contains(t, watched, call.scope, "%+v", call) + case "GetWorker": + assert.Contains(t, ownWorkerIDs, call.scope, "%+v", call) + default: + t.Errorf("unexpected call %+v", call) + } + } + checkPage := func(rows int, pageToken string) { + // An actor token holds ate-api's inner token as base64 text. + raw, _ := base64.RawURLEncoding.DecodeString(pageToken) + assert.NotContains(t, pageToken+string(raw), "tenant-b") + if rows == 0 { + assert.Empty(t, pageToken) + } + } + for _, page := range actorPages { + checkPage(len(page.Actors), page.NextPageToken) + } + for _, page := range workerPages { + checkPage(len(page.Workers), page.NextPageToken) + } + assert.Equal(t, ownActorNames(ateClient, watched), walkedActorNames(actorPages)) + assert.Equal(t, ownWorkerIDs, walkedWorkerUIDs(workerPages)) + assert.Equal(t, int64(len(ownActorNames(ateClient, watched))), summary.ActorCount) + assert.Equal(t, int64(len(ownWorkerIDs)), summary.WorkerCount) + }) + } + } +} + +// A controller that watches every namespace answers as it did before: ate-api's page, +// and ate-api's token unchanged. +func TestSubstratePassThroughWhenWatchingAll(t *testing.T) { + ctx := pagingContext(t) + ateClient := pagingATEClient() + podLists := 0 + kubeClient := pagingKubeClient(t, interceptor.Funcs{ + List: func(ctx context.Context, c ctrlclient.WithWatch, list ctrlclient.ObjectList, opts ...ctrlclient.ListOption) error { + if _, ok := list.(*corev1.PodList); ok { + podLists++ + } + return c.List(ctx, list, opts...) + }, + }) + service := system.NewService(kubeClient, kubeClient, nil, &pkgAuth.NoopAuthorizer{}, ateClient) + + for _, atespace := range []string{"", "tenant-b"} { + page, err := service.ListSubstrateActors(ctx, &apiv1.ListSubstrateActorsRequest{Atespace: atespace, Page: &apiv1.PageRequest{Limit: 1}}) + require.NoError(t, err) + calls := ateClient.calls + assert.Equal(t, ateCall{method: "ListActorsPage", scope: atespace, size: 1}, calls[len(calls)-1]) + assert.Equal(t, "upstream:"+atespace+":"+page.Actors[0].GetMetadata().GetAtespace()+"/"+page.Actors[0].GetMetadata().GetName(), page.NextPageToken) + } + + workers, err := service.ListSubstrateWorkers(ctx, &apiv1.ListSubstrateWorkersRequest{Page: &apiv1.PageRequest{Limit: 1}}) + require.NoError(t, err) + assert.Equal(t, "upstream:1", workers.NextPageToken) + _, err = service.GetSubstrateSummary(ctx, "", "") + require.NoError(t, err) + assert.Empty(t, calledScopes(ateClient, "GetWorker")) + assert.Zero(t, podLists) +} + +func TestSubstrateRowsChangeBetweenPages(t *testing.T) { + ctx := pagingContext(t) + ateClient := pagingATEClient() + kubeClient := pagingKubeClient(t, interceptor.Funcs{}) + service := system.NewService(kubeClient, kubeClient, []string{"tenant-a", "tenant-c"}, &pkgAuth.NoopAuthorizer{}, ateClient) + + actors, err := service.ListSubstrateActors(ctx, &apiv1.ListSubstrateActorsRequest{Page: &apiv1.PageRequest{Limit: 2}}) + require.NoError(t, err) + require.Equal(t, []string{"tenant-a-actor-1", "tenant-a-actor-2"}, actorNames(actors.Actors)) + ateClient.actors = slices.DeleteFunc(ateClient.actors, func(actor *ateapipb.Actor) bool { return actor.GetMetadata().GetName() == "tenant-a-actor-4" }) + ateClient.actors = append(ateClient.actors, namedActor("tenant-a", "tenant-a-actor-0")) + rest := []string{} + for pageToken := actors.NextPageToken; pageToken != ""; { + page, err := service.ListSubstrateActors(ctx, &apiv1.ListSubstrateActorsRequest{Page: &apiv1.PageRequest{Limit: 2, PageToken: pageToken}}) + require.NoError(t, err) + require.Empty(t, page.ATEAPIError) + rest, pageToken = append(rest, actorNames(page.Actors)...), page.NextPageToken + } + assert.Equal(t, []string{"tenant-a-actor-3", "tenant-a-actor-5", "tenant-c-actor-1", "tenant-c-actor-2", "tenant-c-actor-3"}, rest) + + workers, err := service.ListSubstrateWorkers(ctx, &apiv1.ListSubstrateWorkersRequest{Page: &apiv1.PageRequest{Limit: 1}}) + require.NoError(t, err) + require.Equal(t, ownWorkerUIDs[:1], walkedWorkerUIDs([]system.SubstrateWorkerPage{workers})) + require.NoError(t, kubeClient.Delete(ctx, workerPod("tenant-a", "pool-a", "u4-tenant-a"))) + require.NoError(t, kubeClient.Create(ctx, workerPod("tenant-a", "pool-a", "u0a-tenant-a"))) + rest = []string{} + for pageToken := workers.NextPageToken; pageToken != ""; { + page, err := service.ListSubstrateWorkers(ctx, &apiv1.ListSubstrateWorkersRequest{Page: &apiv1.PageRequest{Limit: 1, PageToken: pageToken}}) + require.NoError(t, err) + require.Empty(t, page.ATEAPIError) + rest, pageToken = append(rest, walkedWorkerUIDs([]system.SubstrateWorkerPage{page})...), page.NextPageToken + } + assert.Equal(t, []string{"u3-tenant-c", "u6-tenant-a"}, rest) +} diff --git a/go/core/internal/substrate/client.go b/go/core/internal/substrate/client.go index d5542c13e2..ab5c856bf9 100644 --- a/go/core/internal/substrate/client.go +++ b/go/core/internal/substrate/client.go @@ -145,6 +145,13 @@ func (c *Client) GetActorTemplate(ctx context.Context, atespace, name string) (* return c.ControlClient.GetActorTemplate(ctx, &ateapipb.GetActorTemplateRequest{ActorTemplate: actorRef(atespace, name)}) } +// GetWorker reads a Worker by name. ate-api names each Worker after its pod's UID. +func (c *Client) GetWorker(ctx context.Context, name string) (*ateapipb.Worker, error) { + ctx, cancel := c.callCtx(ctx) + defer cancel() + return c.ControlClient.GetWorker(ctx, &ateapipb.GetWorkerRequest{Worker: &ateapipb.ObjectRef{Name: name}}) +} + func (c *Client) CreateActorTemplate(ctx context.Context, template *ateapipb.ActorTemplate) (*ateapipb.ActorTemplate, error) { ctx, cancel := c.callCtx(ctx) defer cancel() diff --git a/go/core/pkg/app/app.go b/go/core/pkg/app/app.go index 13bfd5fe13..2faa46259f 100644 --- a/go/core/pkg/app/app.go +++ b/go/core/pkg/app/app.go @@ -316,7 +316,7 @@ func Run(ctx context.Context, input *configv1.ControllerConfig, opts Options) er models := modelservice.NewService(manager.GetClient(), authorizer, resourceNamespace) tools := toolservice.NewService(manager.GetClient(), store, authorizer, resourceNamespace, mcpClient) prompts := prompttemplateservice.NewService(manager.GetClient(), authorizer) - system := systemservice.NewService(manager.GetClient(), watchNamespaces, authorizer, actors) + system := systemservice.NewService(manager.GetClient(), manager.GetAPIReader(), watchNamespaces, authorizer, actors) memory := memoryservice.NewService(store) quiescenceWake := make(chan struct{}, 1) pushWake := make(chan struct{}, 1) diff --git a/proto/kagent/api/v1/system.proto b/proto/kagent/api/v1/system.proto index b8e7cbecdf..d8a1a3ad77 100644 --- a/proto/kagent/api/v1/system.proto +++ b/proto/kagent/api/v1/system.proto @@ -62,7 +62,8 @@ message GetSubstrateSummaryRequest { max_len: 63 pattern: "^([a-z0-9]([-a-z0-9]*[a-z0-9])?)?$" }]; - // ATE atespace for actors and templates. Empty lists all atespaces. + // ATE atespace for actors and templates. Empty lists watched atespaces (all when + // the controller watches all namespaces). string atespace = 2 [(buf.validate.field).string = { max_len: 63 pattern: "^([a-z0-9]([-a-z0-9]*[a-z0-9])?)?$" @@ -94,9 +95,11 @@ message GetSubstrateSummaryResponse { repeated ateapi.ActorTemplate actor_templates = 11; } -// One upstream page of actors, in Substrate order. +// One page of actors. A controller that watches some namespaces reads its watched +// atespaces one at a time, in name order, and Substrate order inside each. message ListSubstrateActorsRequest { - // ATE atespace of the actor itself. Empty lists all atespaces. + // ATE atespace of the actor itself. Empty lists watched atespaces (all when the + // controller watches all namespaces). string atespace = 6 [(buf.validate.field).string = { max_len: 63 pattern: "^([a-z0-9]([-a-z0-9]*[a-z0-9])?)?$" @@ -109,13 +112,17 @@ message ListSubstrateActorsRequest { message ListSubstrateActorsResponse { string ate_api_error = 2; repeated ateapi.Actor actors = 3; - // Upstream continuation token. A page can be empty even when another page exists. + // Continuation token. A controller that watches all namespaces passes Substrate's + // token through. A controller that watches some issues its own, which names only + // watched data. Only a last page can be empty. PageResponse page = 4; google.protobuf.Timestamp computed_at = 5; } -// One upstream page of workers, in Substrate order. -// Namespace filtering applies to this page only; it may be empty and still have a next token. +// One page of workers, in worker pod UID order. A controller that watches all +// namespaces filters one Substrate page, which may be empty and still have a next +// token. A controller that watches some reads only the workers of its own worker pods; +// only its last page can be empty. message ListSubstrateWorkersRequest { string namespace = 1 [(buf.validate.field).string = { max_len: 63 diff --git a/ui/src/api/domain/substrate.ts b/ui/src/api/domain/substrate.ts index 09e3d1d3d1..2afe547b0b 100644 --- a/ui/src/api/domain/substrate.ts +++ b/ui/src/api/domain/substrate.ts @@ -94,7 +94,11 @@ export interface Timed { computedAt?: string; } -/** One upstream page; an empty page may still have a continuation token. */ +/** + * One page. Only a last page is empty, except a workers page from a controller that + * watches all namespaces: it filters one upstream page, so it can be empty and still + * have a continuation token. + */ interface SubstratePage extends Timed { /** Upstream read failure. No rows or continuation token accompany it. */ ateApiError?: string; diff --git a/ui/src/generated/kagent/api/v1/system_pb.ts b/ui/src/generated/kagent/api/v1/system_pb.ts index 504a6cdba8..959c6d5067 100644 --- a/ui/src/generated/kagent/api/v1/system_pb.ts +++ b/ui/src/generated/kagent/api/v1/system_pb.ts @@ -184,7 +184,8 @@ export type GetSubstrateSummaryRequest = Message<"kagent.api.v1.GetSubstrateSumm namespace: string; /** - * ATE atespace for actors and templates. Empty lists all atespaces. + * ATE atespace for actors and templates. Empty lists watched atespaces (all when + * the controller watches all namespaces). * * @generated from field: string atespace = 2; */ @@ -291,13 +292,15 @@ export const GetSubstrateSummaryResponseSchema: GenMessage & { /** - * ATE atespace of the actor itself. Empty lists all atespaces. + * ATE atespace of the actor itself. Empty lists watched atespaces (all when the + * controller watches all namespaces). * * @generated from field: string atespace = 6; */ @@ -334,7 +337,9 @@ export type ListSubstrateActorsResponse = Message<"kagent.api.v1.ListSubstrateAc actors: Actor[]; /** - * Upstream continuation token. A page can be empty even when another page exists. + * Continuation token. A controller that watches all namespaces passes Substrate's + * token through. A controller that watches some issues its own, which names only + * watched data. Only a last page can be empty. * * @generated from field: kagent.api.v1.PageResponse page = 4; */ @@ -354,8 +359,10 @@ export const ListSubstrateActorsResponseSchema: GenMessage Date: Fri, 16 Oct 2026 17:01:56 -0400 Subject: [PATCH 2/2] refactor(substrate): read workers through ate-api only and trim tests - refactor(substrate): read workers through ate-api only - test(substrate): drop pod checks from the pass-through test - refactor(substrate): one worker walk for both scopes - test(substrate): drop duplicate worker tests - refactor(substrate): cap only actor tokens and drop duplicate tests - docs(ui): describe Substrate paging for watched namespaces - test(substrate): trim paging tests to unique branches Signed-off-by: Danielle Mustillo --- go/api/gen/kagent/api/v1/system.pb.go | 8 +- go/core/internal/a2agateway/http_test.go | 2 +- go/core/internal/grpcserver/server_test.go | 2 +- .../grpcserver/system_feedback_test.go | 6 +- .../service/sandbox/transports_test.go | 2 +- go/core/internal/service/system/service.go | 6 - .../internal/service/system/service_test.go | 53 +- go/core/internal/service/system/substrate.go | 147 ++-- .../service/system/substrate_paging_test.go | 654 +++++------------- go/core/internal/substrate/client.go | 7 - go/core/pkg/app/app.go | 2 +- proto/kagent/api/v1/system.proto | 8 +- ui/playwright/DEFERRED.md | 12 +- ui/src/generated/kagent/api/v1/system_pb.ts | 8 +- 14 files changed, 280 insertions(+), 637 deletions(-) diff --git a/go/api/gen/kagent/api/v1/system.pb.go b/go/api/gen/kagent/api/v1/system.pb.go index 7bf269f121..13e953fa5d 100644 --- a/go/api/gen/kagent/api/v1/system.pb.go +++ b/go/api/gen/kagent/api/v1/system.pb.go @@ -744,10 +744,10 @@ func (x *ListSubstrateActorsResponse) GetComputedAt() *timestamppb.Timestamp { return nil } -// One page of workers, in worker pod UID order. A controller that watches all -// namespaces filters one Substrate page, which may be empty and still have a next -// token. A controller that watches some reads only the workers of its own worker pods; -// only its last page can be empty. +// One page of workers, in worker name order. A controller that watches all namespaces +// filters one Substrate page, which may be empty and still have a next token. A +// controller that watches some reads every Substrate worker and returns only those of +// its watched namespaces; only its last page can be empty. type ListSubstrateWorkersRequest struct { state protoimpl.MessageState `protogen:"open.v1"` Namespace string `protobuf:"bytes,1,opt,name=namespace,proto3" json:"namespace,omitempty"` diff --git a/go/core/internal/a2agateway/http_test.go b/go/core/internal/a2agateway/http_test.go index 306e8c4fe4..9235c9bb12 100644 --- a/go/core/internal/a2agateway/http_test.go +++ b/go/core/internal/a2agateway/http_test.go @@ -449,7 +449,7 @@ func startCoreTestServer(t *testing.T, gateway a2asrv.RequestHandler, shares ses require.NoError(t, err) server, err := grpcserver.New(grpcserver.Config{ Listener: listener, Authenticator: httpTestAuthenticator{}, ShareStore: shares, - SystemService: systemservice.NewService(nil, nil, nil, nil, nil), + SystemService: systemservice.NewService(nil, nil, nil, nil), A2AHandler: gateway, HTTPHandler: NewHTTPHandler(gateway, httpTestAuthenticator{}, shares), }) require.NoError(t, err) diff --git a/go/core/internal/grpcserver/server_test.go b/go/core/internal/grpcserver/server_test.go index b554cc4c73..08c97430a0 100644 --- a/go/core/internal/grpcserver/server_test.go +++ b/go/core/internal/grpcserver/server_test.go @@ -18,7 +18,7 @@ import ( ) func testSystemService() *systemservice.Service { - return systemservice.NewService(nil, nil, nil, nil, nil) + return systemservice.NewService(nil, nil, nil, nil) } func TestServerServesGRPCAndHTTP(t *testing.T) { diff --git a/go/core/internal/grpcserver/system_feedback_test.go b/go/core/internal/grpcserver/system_feedback_test.go index d75139403c..612eb3bd44 100644 --- a/go/core/internal/grpcserver/system_feedback_test.go +++ b/go/core/internal/grpcserver/system_feedback_test.go @@ -53,7 +53,7 @@ func TestSystemGeneratedClient(t *testing.T) { server, err := New(Config{ Listener: listener, Authenticator: &authimpl.InsecureAuthenticator{}, - SystemService: systemservice.NewService(kubeClient, kubeClient, nil, &pkgauth.NoopAuthorizer{}, emptySystemATEClient{}), + SystemService: systemservice.NewService(kubeClient, nil, &pkgauth.NoopAuthorizer{}, emptySystemATEClient{}), }) if err != nil { t.Fatalf("New() error = %v", err) @@ -200,7 +200,3 @@ func (emptySystemATEClient) ListActorsPage(context.Context, string, int32, strin func (emptySystemATEClient) ListWorkersPage(context.Context, int32, string) ([]*ateapipb.Worker, string, error) { return nil, "", nil } - -func (emptySystemATEClient) GetWorker(_ context.Context, name string) (*ateapipb.Worker, error) { - return nil, status.Errorf(codes.NotFound, "Worker %s not found", name) -} diff --git a/go/core/internal/service/sandbox/transports_test.go b/go/core/internal/service/sandbox/transports_test.go index 0ce2cbd944..eca00727d0 100644 --- a/go/core/internal/service/sandbox/transports_test.go +++ b/go/core/internal/service/sandbox/transports_test.go @@ -38,7 +38,7 @@ func TestSandboxTransports(t *testing.T) { listener := bufconn.Listen(1 << 20) server, err := grpcserver.New(grpcserver.Config{ Listener: listener, Authenticator: testAuth{}, - SystemService: system.NewService(nil, nil, nil, nil, nil), + SystemService: system.NewService(nil, nil, nil, nil), SandboxService: service, }) require.NoError(t, err) diff --git a/go/core/internal/service/system/service.go b/go/core/internal/service/system/service.go index ac7dc43b74..95bd09903f 100644 --- a/go/core/internal/service/system/service.go +++ b/go/core/internal/service/system/service.go @@ -29,14 +29,10 @@ type ATEClient interface { ListActorTemplates(context.Context, string) ([]*ateapipb.ActorTemplate, error) ListActorsPage(ctx context.Context, atespace string, pageSize int32, pageToken string) ([]*ateapipb.Actor, string, error) ListWorkersPage(ctx context.Context, pageSize int32, pageToken string) ([]*ateapipb.Worker, string, error) - GetWorker(ctx context.Context, name string) (*ateapipb.Worker, error) } type Service struct { kubeClient client.Client - // Reads worker pods. The manager passes its uncached API reader, so the manager's - // cache holds no Pods. - podReader client.Reader // Sorted and unique: the order Substrate pages are read in. A repeat must not read // an atespace twice. Empty means every namespace. observedNamespaces []string @@ -51,14 +47,12 @@ type Namespace struct { func NewService( kubeClient client.Client, - podReader client.Reader, observedNamespaces []string, authorizer auth.Authorizer, ateClient ATEClient, ) *Service { return &Service{ kubeClient: kubeClient, - podReader: podReader, observedNamespaces: slices.Compact(slices.Sorted(slices.Values(observedNamespaces))), authorizer: authorizer, ateClient: ateClient, diff --git a/go/core/internal/service/system/service_test.go b/go/core/internal/service/system/service_test.go index 8d341c7482..b493b825a0 100644 --- a/go/core/internal/service/system/service_test.go +++ b/go/core/internal/service/system/service_test.go @@ -1,6 +1,7 @@ package system_test import ( + "cmp" "context" "errors" "fmt" @@ -47,6 +48,8 @@ type fakeATEClient struct { // for, so a fake that always fills the request would hide a caller that stops // following the token as soon as it has enough rows. pageSize int + // The ceiling of a worker page, when it differs from pageSize. + workerPageSize int // How many page reads each list has taken, so a test can assert that the token // was followed rather than that the rows came back. actorReads int @@ -61,8 +64,7 @@ type fakeATEClient struct { failErr error } -// ateCall is one ate-api call: the method, the atespace or worker name, the token -// and the page size. +// ateCall is one ate-api call: the method, the atespace, the token and the page size. type ateCall struct { method, scope, token string size int32 @@ -107,20 +109,7 @@ func (client *fakeATEClient) ListWorkersPage(_ context.Context, pageSize int32, if err := client.record("ListWorkersPage", "", pageToken, pageSize); err != nil { return nil, "", err } - return fakePage(client.workers, client.pageSize, pageSize, pageToken) -} - -func (client *fakeATEClient) GetWorker(_ context.Context, name string) (*ateapipb.Worker, error) { - if err := client.record("GetWorker", name, "", 0); err != nil { - return nil, err - } - for _, worker := range client.workers { - if worker.GetMetadata().GetName() == name { - return worker, nil - } - } - // ate-api's own NotFound text, which repeats the name it was sent. - return nil, status.Errorf(codes.NotFound, "Worker %s not found", name) + return fakePage(client.workers, cmp.Or(client.workerPageSize, client.pageSize), pageSize, pageToken) } // actorPage pages actors the way ate-api does: in atespace and name order, after the @@ -181,7 +170,7 @@ func fakePage[T any](rows []T, ceiling int, requested int32, pageToken string) ( } func TestCurrentUser(t *testing.T) { - service := system.NewService(nil, nil, nil, nil, nil) + service := system.NewService(nil, nil, nil, nil) claims := map[string]any{"sub": "user-1", "groups": []any{"admins"}} ctx := pkgAuth.AuthSessionTo(t.Context(), &authimpl.SimpleSession{P: pkgAuth.Principal{ User: pkgAuth.User{ID: "user-1"}, @@ -212,7 +201,7 @@ func TestListNamespaces(t *testing.T) { &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "Zoo"}, Status: corev1.NamespaceStatus{Phase: corev1.NamespaceActive}}, &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "alpha"}, Status: corev1.NamespaceStatus{Phase: corev1.NamespaceTerminating}}, ).Build() - service := system.NewService(kubeClient, kubeClient, nil, nil, nil) + service := system.NewService(kubeClient, nil, nil, nil) result, err := service.ListNamespaces(t.Context()) require.NoError(t, err) @@ -228,7 +217,7 @@ func TestListNamespaces(t *testing.T) { return apierrors.NewForbidden(schema.GroupResource{Resource: "namespaces"}, "", nil) }, }).Build() - service := system.NewService(kubeClient, kubeClient, []string{"team-b", "team-a"}, nil, nil) + service := system.NewService(kubeClient, []string{"team-b", "team-a"}, nil, nil) result, err := service.ListNamespaces(t.Context()) require.NoError(t, err) @@ -257,7 +246,7 @@ func TestListSubstrateActors(t *testing.T) { ctx := pkgAuth.AuthSessionTo(t.Context(), &authimpl.SimpleSession{P: pkgAuth.Principal{User: pkgAuth.User{ID: "user"}}}) newService := func(client system.ATEClient) *system.Service { - return system.NewService(nil, nil, nil, &pkgAuth.NoopAuthorizer{}, client) + return system.NewService(nil, nil, &pkgAuth.NoopAuthorizer{}, client) } t.Run("answers with one page and the token for the next", func(t *testing.T) { @@ -324,7 +313,7 @@ func TestListSubstrateActors(t *testing.T) { }) t.Run("authorizes", func(t *testing.T) { - denied := system.NewService(nil, nil, nil, systemDenyAuthorizer{}, &fakeATEClient{}) + denied := system.NewService(nil, nil, systemDenyAuthorizer{}, &fakeATEClient{}) _, err := denied.ListSubstrateActors(ctx, &apiv1.ListSubstrateActorsRequest{}) assert.True(t, serviceerrors.IsCode(err, serviceerrors.CodePermissionDenied), err) }) @@ -339,7 +328,7 @@ func TestListSubstrateWorkers(t *testing.T) { {WorkerNamespace: "team", WorkerPool: "pool", WorkerPod: "worker-2"}, {WorkerPool: "pool", WorkerPod: "names-no-namespace"}, }} - service := system.NewService(nil, nil, nil, &pkgAuth.NoopAuthorizer{}, ateClient) + service := system.NewService(nil, nil, &pkgAuth.NoopAuthorizer{}, ateClient) page, err := service.ListSubstrateWorkers(ctx, &apiv1.ListSubstrateWorkersRequest{Namespace: "team", Page: &apiv1.PageRequest{Limit: 2}}) require.NoError(t, err) @@ -393,7 +382,7 @@ func TestGetSubstrateSummary(t *testing.T) { {WorkerNamespace: "other", WorkerPool: "pool", WorkerPod: "worker-9"}, }, } - service := system.NewService(kubeClient, kubeClient, nil, &pkgAuth.NoopAuthorizer{}, ateClient) + service := system.NewService(kubeClient, nil, &pkgAuth.NoopAuthorizer{}, ateClient) result, err := service.GetSubstrateSummary(ctx, "team", "team") require.NoError(t, err) @@ -421,7 +410,7 @@ func TestGetSubstrateSummary(t *testing.T) { }) t.Run("an ate-api failure leaves the Kubernetes halves complete", func(t *testing.T) { - service := system.NewService(kubeClient, kubeClient, nil, &pkgAuth.NoopAuthorizer{}, &fakeATEClient{err: errors.New("ate-api unreachable")}) + service := system.NewService(kubeClient, nil, &pkgAuth.NoopAuthorizer{}, &fakeATEClient{err: errors.New("ate-api unreachable")}) result, err := service.GetSubstrateSummary(ctx, "team", "team") require.NoError(t, err) @@ -452,7 +441,7 @@ func TestGetSubstrateSummaryReadsAreIndependent(t *testing.T) { ateClient := &failingTemplatesATEClient{ fakeATEClient: fakeATEClient{actors: actors, workers: workers}, } - service := system.NewService(kubeClient, kubeClient, nil, &pkgAuth.NoopAuthorizer{}, ateClient) + service := system.NewService(kubeClient, nil, &pkgAuth.NoopAuthorizer{}, ateClient) result, err := service.GetSubstrateSummary(ctx, "team", "team") require.NoError(t, err) @@ -467,7 +456,7 @@ func TestGetSubstrateSummaryReadsAreIndependent(t *testing.T) { t.Run("a failed actor walk still counts busy workers", func(t *testing.T) { ateClient := &failingActorsATEClient{fakeATEClient: fakeATEClient{workers: workers}} - service := system.NewService(kubeClient, kubeClient, nil, &pkgAuth.NoopAuthorizer{}, ateClient) + service := system.NewService(kubeClient, nil, &pkgAuth.NoopAuthorizer{}, ateClient) result, err := service.GetSubstrateSummary(ctx, "team", "team") require.NoError(t, err) assert.Equal(t, "actors unavailable", result.ATEAPIError) @@ -485,7 +474,7 @@ func TestGetSubstrateSummaryReadsAreIndependent(t *testing.T) { }, }, } - service := system.NewService(kubeClient, kubeClient, nil, &pkgAuth.NoopAuthorizer{}, ateClient) + service := system.NewService(kubeClient, nil, &pkgAuth.NoopAuthorizer{}, ateClient) result, err := service.GetSubstrateSummary(ctx, "team", "team") require.NoError(t, err) @@ -510,7 +499,7 @@ func TestGetSubstrateSummaryReadsAreIndependent(t *testing.T) { {WorkerNamespace: "kagent", WorkerPool: "pool", WorkerPod: "worker-0"}, }, } - service := system.NewService(kubeClient, kubeClient, nil, &pkgAuth.NoopAuthorizer{}, ateClient) + service := system.NewService(kubeClient, nil, &pkgAuth.NoopAuthorizer{}, ateClient) result, err := service.GetSubstrateSummary(ctx, "team", "team") require.NoError(t, err) @@ -558,7 +547,7 @@ func TestBusyWorkerWithOutOfScopeActor(t *testing.T) { {WorkerNamespace: "kagent", WorkerPod: "worker-0", Status: &ateapipb.WorkerStatus{Allocated: &ateapipb.WorkerResources{Actors: 1}}}, }, } - service := system.NewService(kubeClient, kubeClient, nil, &pkgAuth.NoopAuthorizer{}, ateClient) + service := system.NewService(kubeClient, nil, &pkgAuth.NoopAuthorizer{}, ateClient) summary, err := service.GetSubstrateSummary(ctx, "kagent", "kagent") require.NoError(t, err) require.Empty(t, summary.ATEAPIError) @@ -601,7 +590,7 @@ func TestSubstrateScopesAreIndependent(t *testing.T) { }, workers: []*ateapipb.Worker{{WorkerNamespace: "workers", WorkerPod: "pod"}}, } - service := system.NewService(kubeClient, kubeClient, nil, &pkgAuth.NoopAuthorizer{}, ateClient) + service := system.NewService(kubeClient, nil, &pkgAuth.NoopAuthorizer{}, ateClient) for _, tc := range []struct { atespace string count int64 @@ -689,12 +678,12 @@ func TestSubstrateRefusesUnwatchedNamespaces(t *testing.T) { kubeLists++ return c.List(ctx, list, opts...) }}) - service := system.NewService(kubeClient, kubeClient, []string{"tenant-a"}, &pkgAuth.NoopAuthorizer{}, ateClient) + service := system.NewService(kubeClient, []string{"tenant-a"}, &pkgAuth.NoopAuthorizer{}, ateClient) err := tc.read(service) assert.True(t, serviceerrors.IsCode(err, serviceerrors.CodePermissionDenied), "err = %v", err) assert.Empty(t, ateClient.calls, "a refused read must not reach ate-api") - assert.Zero(t, kubeLists, "a refused read must not list WorkerPools or pods") + assert.Zero(t, kubeLists, "a refused read must not list WorkerPools") }) } } diff --git a/go/core/internal/service/system/substrate.go b/go/core/internal/service/system/substrate.go index 27abb4619b..c3059286dc 100644 --- a/go/core/internal/service/system/substrate.go +++ b/go/core/internal/service/system/substrate.go @@ -3,7 +3,6 @@ package system import ( "context" "encoding/base64" - "errors" "fmt" "maps" "slices" @@ -12,10 +11,7 @@ import ( atev1alpha1 "github.com/agent-substrate/substrate/pkg/api/v1alpha1" "github.com/agent-substrate/substrate/pkg/proto/ateapipb" - "google.golang.org/grpc/codes" "google.golang.org/grpc/status" - corev1 "k8s.io/api/core/v1" - "sigs.k8s.io/controller-runtime/pkg/client" apiv1 "github.com/kagent-dev/kagent/go/api/gen/kagent/api/v1" "github.com/kagent-dev/kagent/go/core/internal/service/serviceerrors" @@ -163,10 +159,13 @@ func (s *Service) ListSubstrateActors(ctx context.Context, input *apiv1.ListSubs start, inner := 0, "" if token != "" { // A token without a watched atespace is refused rather than read as page one. + if len(token) > maxPageTokenBytes { + return SubstrateActorPage{}, errInvalidPageToken + } decoded, decodeErr := base64.RawURLEncoding.DecodeString(token) atespace, rest, ok := strings.Cut(string(decoded), "/") start = slices.Index(atespaces, atespace) - if len(token) > maxPageTokenBytes || decodeErr != nil || !ok || atespace == "" || start < 0 { + if decodeErr != nil || !ok || atespace == "" || start < 0 { return SubstrateActorPage{}, errInvalidPageToken } inner = rest @@ -224,8 +223,8 @@ func (s *Service) listScopedActors(ctx context.Context, atespaces []string, inne // ListSubstrateWorkers returns one page of workers. A controller that watches every // namespace filters ate-api's page and passes its token through. A controller that -// watches some reads the Worker of each of its own worker pods, so ate-api reads, and -// no page token names, a worker of a namespace it does not watch. +// watches some reads every ate-api worker and returns only the workers of watched +// namespaces, so no page token names a worker of a namespace it does not watch. func (s *Service) ListSubstrateWorkers(ctx context.Context, input *apiv1.ListSubstrateWorkersRequest) (SubstrateWorkerPage, error) { namespaces, err := s.substrateScope(ctx, input.GetNamespace(), "") if err != nil { @@ -233,7 +232,7 @@ func (s *Service) ListSubstrateWorkers(ctx context.Context, input *apiv1.ListSub } limit, token := substratePageSize(input.GetPage().GetLimit()), input.GetPage().GetPageToken() if len(s.observedNamespaces) > 0 { - return s.listPodWorkers(ctx, namespaces, limit, token) + return s.listOwnWorkers(ctx, namespaces, limit, token) } result := SubstrateWorkerPage{ComputedAt: time.Now().UTC()} workers, next, err := s.ateClient.ListWorkersPage(ctx, limit, token) @@ -243,7 +242,7 @@ func (s *Service) ListSubstrateWorkers(ctx context.Context, input *apiv1.ListSub return result, nil } for _, worker := range workers { - if inNamespace(worker, namespaces[0]) { + if inNamespace(worker, namespaces) { result.Workers = append(result.Workers, worker) } } @@ -251,81 +250,32 @@ func (s *Service) ListSubstrateWorkers(ctx context.Context, input *apiv1.ListSub return result, nil } -// listPodWorkers fills one page with the Workers of the worker pods in the namespaces, -// in pod UID order, which is ate-api's worker order. The token is the UID of the last -// worker pod on the previous page: a cursor only, never sent to ate-api. -func (s *Service) listPodWorkers(ctx context.Context, namespaces []string, limit int32, token string) (SubstrateWorkerPage, error) { - if len(token) > maxPageTokenBytes { - return SubstrateWorkerPage{}, errInvalidPageToken - } +// listOwnWorkers fills one page with the workers of the namespaces, in worker name +// order. The token is the name of the last worker on the previous page: a cursor only, +// never sent to ate-api. +func (s *Service) listOwnWorkers(ctx context.Context, namespaces []string, limit int32, token string) (SubstrateWorkerPage, error) { result := SubstrateWorkerPage{ComputedAt: time.Now().UTC()} - pods, err := s.workerPods(ctx, namespaces) - if err != nil { + var workers []*ateapipb.Worker + if err := s.walkWorkers(ctx, namespaces, func(worker *ateapipb.Worker) { + if worker.GetMetadata().GetName() > token { + workers = append(workers, worker) + } + }); err != nil { result.ATEAPIError = err.Error() return result, nil } - for i := range pods { - if string(pods[i].UID) <= token { - continue - } - worker, err := s.podWorker(ctx, &pods[i]) - if err != nil { - return SubstrateWorkerPage{ATEAPIError: err.Error(), ComputedAt: result.ComputedAt}, nil - } - if worker == nil { - continue - } - result.Workers = append(result.Workers, worker) - if int32(len(result.Workers)) >= limit { - if i+1 < len(pods) { - result.NextPageToken = string(pods[i].UID) - } - break - } + // ate-api sends workers in name order already. The cursor must not rely on it. + slices.SortFunc(workers, func(left, right *ateapipb.Worker) int { + return strings.Compare(left.GetMetadata().GetName(), right.GetMetadata().GetName()) + }) + if int32(len(workers)) > limit { + workers = workers[:limit] + result.NextPageToken = workers[limit-1].GetMetadata().GetName() } + result.Workers = workers return result, nil } -// WorkerPoolLabel names the WorkerPool of a Substrate worker pod. ate-api names each -// Worker after its pod's UID. -const WorkerPoolLabel = "ate.dev/worker-pool" - -// workerPods lists the worker pods of the namespaces in UID order. -func (s *Service) workerPods(ctx context.Context, namespaces []string) ([]corev1.Pod, error) { - var pods []corev1.Pod - for _, namespace := range namespaces { - podList := &corev1.PodList{} - // ponytail: one uncached apiserver list per watched namespace per read. Add a - // label-limited cache if UI polling load matters. - if err := s.podReader.List(ctx, podList, client.InNamespace(namespace), client.HasLabels{WorkerPoolLabel}); err != nil { - logging.FromContext(ctx).ErrorContext(ctx, "failed to list worker pods", "error", err, "namespace", namespace) - return nil, errors.New("failed to list worker pods") - } - pods = append(pods, podList.Items...) - } - slices.SortFunc(pods, func(left, right corev1.Pod) int { return strings.Compare(string(left.UID), string(right.UID)) }) - return pods, nil -} - -// podWorker reads the Worker of a worker pod. It is nil when ate-api has none yet, -// or when the Worker is not that pod's. -func (s *Service) podWorker(ctx context.Context, pod *corev1.Pod) (*ateapipb.Worker, error) { - worker, err := s.ateClient.GetWorker(ctx, string(pod.UID)) - if status.Code(err) == codes.NotFound { - // ate-api registers a worker pod only when it is Ready. - return nil, nil - } - if err != nil { - // ate-api error text can name a worker of any namespace. The user gets the code. - logging.FromContext(ctx).ErrorContext(ctx, "failed to get ate-api worker", "error", err, "pod", pod.Name) - return nil, fmt.Errorf("ate-api could not read a worker: %s", status.Code(err)) - } - if worker.GetWorkerNamespace() != pod.Namespace || worker.GetWorkerPodUid() != string(pod.UID) { - return nil, nil - } - return worker, nil -} - // walkActors visits every actor in the requested atespace, or in each watched one. func (s *Service) walkActors(ctx context.Context, atespace string, visit func(*ateapipb.Actor)) error { for _, atespace := range s.substrateNamespaces(atespace) { @@ -333,7 +283,8 @@ func (s *Service) walkActors(ctx context.Context, atespace string, visit func(*a return s.ateClient.ListActorsPage(ctx, atespace, pageSize, pageToken) } if err := walkSubstrate(ctx, read, func(actor *ateapipb.Actor) { - if actor != nil && s.watches(actor.GetMetadata().GetAtespace()) { + // ate-api scoped the list already. This is a second guard. + if actor != nil && (atespace == "" || actor.GetMetadata().GetAtespace() == atespace) { visit(actor) } }); err != nil { @@ -343,32 +294,22 @@ func (s *Service) walkActors(ctx context.Context, atespace string, visit func(*a return nil } -// walkWorkers visits every worker in the namespaces: from ate-api's pages for a -// controller that watches every namespace, else the Worker of each worker pod. +// walkWorkers visits every worker in the namespaces. A controller that watches some +// reads every ate-api worker and visits only the workers of watched namespaces. func (s *Service) walkWorkers(ctx context.Context, namespaces []string, visit func(*ateapipb.Worker)) error { - if len(s.observedNamespaces) == 0 { - return walkSubstrate(ctx, s.ateClient.ListWorkersPage, func(worker *ateapipb.Worker) { - if inNamespace(worker, namespaces[0]) { - visit(worker) - } - }) - } - pods, err := s.workerPods(ctx, namespaces) - if err != nil { - return err - } - // ponytail: one sequential GetWorker per own worker pod (about 1 ms each). Add - // bounded concurrency when a controller runs more than about 100 own workers. - for i := range pods { - worker, err := s.podWorker(ctx, &pods[i]) - if err != nil { - return err - } - if worker != nil { + // NOTE: this drains every ate-api worker page per request. Replace it with an + // ate-api worker namespace filter when one lands. + err := walkSubstrate(ctx, s.ateClient.ListWorkersPage, func(worker *ateapipb.Worker) { + if inNamespace(worker, namespaces) { visit(worker) } + }) + if err != nil && len(s.observedNamespaces) > 0 { + // ate-api error text can name a worker of any namespace. The user gets the code. + logging.FromContext(ctx).ErrorContext(ctx, "failed to list ate-api workers", "error", err) + return fmt.Errorf("ate-api could not list workers: %s", status.Code(err)) } - return nil + return err } // walkSubstrate calls visit for every row ate-api holds, holding one page at a time @@ -418,10 +359,10 @@ func (s *Service) watches(namespace string) bool { return len(s.observedNamespaces) == 0 || slices.Contains(s.observedNamespaces, namespace) } -// inNamespace reports whether a worker is in the namespace, or in any when it is empty. -// A controller that watches every namespace reads exactly one, maybe empty. -func inNamespace(worker *ateapipb.Worker, namespace string) bool { - return worker != nil && (namespace == "" || worker.GetWorkerNamespace() == namespace) +// inNamespace reports whether a worker is in one of the namespaces, or in any when the +// first is empty. A controller that watches every namespace reads exactly one, maybe empty. +func inNamespace(worker *ateapipb.Worker, namespaces []string) bool { + return worker != nil && (namespaces[0] == "" || slices.Contains(namespaces, worker.GetWorkerNamespace())) } func substratePageSize(requested int32) int32 { diff --git a/go/core/internal/service/system/substrate_paging_test.go b/go/core/internal/service/system/substrate_paging_test.go index 7383053bf5..b6a84f49d9 100644 --- a/go/core/internal/service/system/substrate_paging_test.go +++ b/go/core/internal/service/system/substrate_paging_test.go @@ -1,12 +1,10 @@ package system_test import ( - "bytes" "context" "encoding/base64" "errors" "fmt" - "log/slog" "slices" "strconv" "strings" @@ -21,7 +19,6 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/types" ctrlclient "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/controller-runtime/pkg/client/interceptor" @@ -31,7 +28,6 @@ import ( "github.com/kagent-dev/kagent/go/core/internal/service/serviceerrors" "github.com/kagent-dev/kagent/go/core/internal/service/system" pkgAuth "github.com/kagent-dev/kagent/go/core/pkg/auth" - "github.com/kagent-dev/kagent/go/pkg/logging" ) func pagingContext(t *testing.T) context.Context { @@ -42,7 +38,18 @@ func pagingContext(t *testing.T) context.Context { // ate-api's order. Every tenant-b name holds "tenant-b", so a test can look for it in // any text. func pagingATEClient() *fakeATEClient { - client := &fakeATEClient{} + client := &fakeATEClient{ + workers: []*ateapipb.Worker{ + ateWorker("tenant-a", "u1-tenant-a", 2), + ateWorker("tenant-b", "u2-tenant-b", 1), + ateWorker("tenant-c", "u3-tenant-c", 1), + ateWorker("tenant-a", "u4-tenant-a", 0), + ateWorker("tenant-b", "u5-tenant-b", 0), + ateWorker("tenant-a", "u6-tenant-a", 0), + }, + // Two workers a page, so a drain of the workers takes three reads. + workerPageSize: 2, + } for _, tenant := range []struct { atespace string actors int @@ -54,50 +61,13 @@ func pagingATEClient() *fakeATEClient { Metadata: &ateapipb.ResourceMetadata{Atespace: tenant.atespace, Name: tenant.atespace + "-template"}, }) } - for _, pod := range pagingPods() { - if pod.Labels[system.WorkerPoolLabel] != "" { - client.workers = append(client.workers, ateWorker(pod.Namespace, string(pod.UID), busyPods[string(pod.UID)])) - } - } return client } -// The actors placed on each busy worker pod, by pod UID. -var busyPods = map[string]int32{"u1-tenant-a": 2, "u2-tenant-b": 1, "u3-tenant-c": 1} - -// pagingPods are the worker pods of the paging tests. Their UIDs mix the tenants in -// UID order, which is ate-api's worker order. One tenant-a pod is not a worker pod. -func pagingPods() []*corev1.Pod { - return []*corev1.Pod{ - workerPod("tenant-a", "", "u0-tenant-a"), - workerPod("tenant-a", "pool-a", "u1-tenant-a"), - workerPod("tenant-b", "pool-b", "u2-tenant-b"), - workerPod("tenant-c", "pool-c", "u3-tenant-c"), - workerPod("tenant-a", "pool-a", "u4-tenant-a"), - workerPod("tenant-b", "pool-b", "u5-tenant-b"), - workerPod("tenant-a", "pool-a", "u6-tenant-a"), - } -} - -// ownWorkerUIDs are the workers of tenant-a and tenant-c, in UID order. +// ownWorkerUIDs are the workers of tenant-a and tenant-c, in name order. var ownWorkerUIDs = []string{"u1-tenant-a", "u3-tenant-c", "u4-tenant-a", "u6-tenant-a"} -// workerPod is a running worker pod of the pool, or a plain pod when pool is empty. -func workerPod(namespace, pool, uid string) *corev1.Pod { - pod := &corev1.Pod{ - ObjectMeta: metav1.ObjectMeta{Namespace: namespace, Name: "pod-" + uid, UID: types.UID(uid)}, - Status: corev1.PodStatus{ - Phase: corev1.PodRunning, - Conditions: []corev1.PodCondition{{Type: corev1.PodReady, Status: corev1.ConditionTrue}}, - }, - } - if pool != "" { - pod.Labels = map[string]string{system.WorkerPoolLabel: pool} - } - return pod -} - -// ateWorker is the Worker ate-api holds for a worker pod: it is named after the pod UID. +// ateWorker is a Worker as ate-api holds it: named after its pod's UID. func ateWorker(namespace, uid string, allocatedActors int32) *ateapipb.Worker { return &ateapipb.Worker{ Metadata: &ateapipb.ResourceMetadata{Name: uid}, @@ -108,21 +78,24 @@ func ateWorker(namespace, uid string, allocatedActors int32) *ateapipb.Worker { } } -// pagingKubeClient holds the paging pods, and a WorkerPool in each tenant. -func pagingKubeClient(t *testing.T, funcs interceptor.Funcs, pods ...*corev1.Pod) ctrlclient.WithWatch { +// pagingKubeClient holds a WorkerPool in each tenant. +func pagingKubeClient(t *testing.T, funcs interceptor.Funcs) ctrlclient.WithWatch { scheme := runtime.NewScheme() require.NoError(t, corev1.AddToScheme(scheme)) require.NoError(t, atev1alpha1.AddToScheme(scheme)) objects := []ctrlclient.Object{} - for _, pod := range append(pagingPods(), pods...) { - objects = append(objects, pod) - } for _, tenant := range []string{"tenant-a", "tenant-b", "tenant-c"} { objects = append(objects, &atev1alpha1.WorkerPool{ObjectMeta: metav1.ObjectMeta{Namespace: tenant, Name: "pool"}}) } return fake.NewClientBuilder().WithScheme(scheme).WithObjects(objects...).WithInterceptorFuncs(funcs).Build() } +// ownWorkerService watches tenant-a and tenant-c. It has no Kubernetes client: the +// workers view reads ate-api only. +func ownWorkerService(ateClient system.ATEClient) *system.Service { + return system.NewService(nil, []string{"tenant-a", "tenant-c"}, &pkgAuth.NoopAuthorizer{}, ateClient) +} + // walkWorkers reads every page of workers and fails the test on a hard or soft error. func walkWorkers(t *testing.T, ctx context.Context, service *system.Service, namespace string, limit int32) []system.SubstrateWorkerPage { t.Helper() @@ -141,6 +114,14 @@ func walkWorkers(t *testing.T, ctx context.Context, service *system.Service, nam return nil } +// listWorkers reads one page of workers in every watched namespace. +func listWorkers(t *testing.T, service *system.Service, limit int32, pageToken string) system.SubstrateWorkerPage { + t.Helper() + page, err := service.ListSubstrateWorkers(pagingContext(t), &apiv1.ListSubstrateWorkersRequest{Page: &apiv1.PageRequest{Limit: limit, PageToken: pageToken}}) + require.NoError(t, err) + return page +} + func walkedWorkerUIDs(pages []system.SubstrateWorkerPage) []string { uids := []string{} for _, page := range pages { @@ -210,7 +191,7 @@ func TestListSubstrateActorsFanOut(t *testing.T) { namedActor("tenant-a", "a-1"), namedActor("tenant-b", "b-1"), namedActor("tenant-c", "c-1"), namedActor("tenant-a", "a-2"), namedActor("tenant-c", "c-2"), namedActor("tenant-a", "a-3"), }} - service := system.NewService(nil, nil, []string{"tenant-a", "tenant-c"}, &pkgAuth.NoopAuthorizer{}, ateClient) + service := system.NewService(nil, []string{"tenant-a", "tenant-c"}, &pkgAuth.NoopAuthorizer{}, ateClient) first, err := service.ListSubstrateActors(ctx, &apiv1.ListSubstrateActorsRequest{Page: &apiv1.PageRequest{Limit: 2}}) require.NoError(t, err) @@ -231,151 +212,55 @@ func token(raw string) string { return base64.RawURLEncoding.EncodeToString([]byte(raw)) } -// A Substrate actor token, as the released controller passed it through. It names a -// tenant-b session. -const substrateActorToken = `{"v":1,"kind":"actor","scope":"","last":["tenant-b","session-1"]}` - -// badActorTokens are tokens a controller that watches tenant-a and tenant-c did not -// issue. None of them may start a walk again at page one. -var badActorTokens = map[string]string{ - "no separator": `tenant-a`, - "empty atespace": `/x`, - "unwatched atespace": `tenant-b/x`, - "worker token": `u1-tenant-a`, - "substrate token, raw base64": substrateActorToken, -} - -// rejectsToken asserts that a read with the token is a hard InvalidArgument that -// reaches neither ate-api nor the log or the error with a tenant-b name. -func rejectsToken(t *testing.T, ateClient *fakeATEClient, read func(context.Context) error) { - t.Helper() - var log bytes.Buffer - ctx := logging.IntoContext(pagingContext(t), slog.New(slog.NewTextHandler(&log, nil))) - err := read(ctx) - require.Error(t, err) - assert.True(t, serviceerrors.IsCode(err, serviceerrors.CodeInvalidArgument), "err = %v", err) - assert.NotContains(t, err.Error(), "tenant-b") - assert.NotContains(t, log.String(), "tenant-b") - assert.Empty(t, ateClient.calls, "a bad token must not reach ate-api") -} - -func listActorsWithToken(service *system.Service, pageToken string) func(context.Context) error { - return func(ctx context.Context) error { - _, err := service.ListSubstrateActors(ctx, &apiv1.ListSubstrateActorsRequest{Page: &apiv1.PageRequest{Limit: 2, PageToken: pageToken}}) - return err - } -} - +// A token this controller did not issue is a hard InvalidArgument. It never reaches +// ate-api, never starts a walk again at page one, and its error names no tenant-b row. func TestListSubstrateActorsStrictToken(t *testing.T) { - tokens := map[string]string{ - "not base64": "not base64!", - "substrate token, std base64": base64.StdEncoding.EncodeToString([]byte(substrateActorToken)), + for name, pageToken := range map[string]string{ + "not base64": "not base64!", + "no separator": token("tenant-a"), + "empty atespace": token("/x"), + "unwatched atespace": token("tenant-b/x"), + // A Substrate actor token, as the released controller passed it through. + "substrate token": token(`{"v":1,"kind":"actor","scope":"","last":["tenant-b","session-1"]}`), "longer than 1 KiB, else valid": token("tenant-a/" + strings.Repeat("x", 800)), - } - for name, raw := range badActorTokens { - tokens[name] = token(raw) - } - for name, pageToken := range tokens { + } { t.Run(name, func(t *testing.T) { ateClient := &fakeATEClient{} - service := system.NewService(nil, nil, []string{"tenant-a", "tenant-c"}, &pkgAuth.NoopAuthorizer{}, ateClient) - rejectsToken(t, ateClient, listActorsWithToken(service, pageToken)) + service := system.NewService(nil, []string{"tenant-a", "tenant-c"}, &pkgAuth.NoopAuthorizer{}, ateClient) + _, err := service.ListSubstrateActors(pagingContext(t), &apiv1.ListSubstrateActorsRequest{Page: &apiv1.PageRequest{Limit: 2, PageToken: pageToken}}) + require.Error(t, err) + assert.True(t, serviceerrors.IsCode(err, serviceerrors.CodeInvalidArgument), "err = %v", err) + assert.NotContains(t, err.Error(), "tenant-b") + assert.Empty(t, ateClient.calls, "a bad token must not reach ate-api") }) } } -// ate-api checks the token inside a kagent token: a forged one cannot read another -// atespace, and its refusal names nothing. -func TestListSubstrateActorsForeignInnerToken(t *testing.T) { - ateClient := pagingATEClient() - service := system.NewService(nil, nil, []string{"tenant-a", "tenant-c"}, &pkgAuth.NoopAuthorizer{}, ateClient) - - page, err := service.ListSubstrateActors(pagingContext(t), &apiv1.ListSubstrateActorsRequest{Page: &apiv1.PageRequest{ - PageToken: token("tenant-a/upstream:tenant-b:tenant-b/tenant-b-actor-1"), - }}) - require.NoError(t, err) - assert.Empty(t, page.Actors) - assert.Empty(t, page.NextPageToken) - assert.NotEmpty(t, page.ATEAPIError) - assert.NotContains(t, page.ATEAPIError, "tenant-b") - assert.Equal(t, []ateCall{{method: "ListActorsPage", scope: "tenant-a", token: "upstream:tenant-b:tenant-b/tenant-b-actor-1", size: 50}}, ateClient.calls) -} - -// The token names its atespace, not its place in the list, so a token survives a -// change to the watched list or fails loudly, and never reads a new neighbour. +// The token names its atespace, not its place in the watched list, so it resumes in the +// same atespace after the list grows. func TestListSubstrateActorsTokenHoldsAtespaceName(t *testing.T) { ctx := pagingContext(t) - ateClient := pagingATEClient() - first := system.NewService(nil, nil, []string{"tenant-a", "tenant-c"}, &pkgAuth.NoopAuthorizer{}, ateClient) + first := system.NewService(nil, []string{"tenant-a", "tenant-c"}, &pkgAuth.NoopAuthorizer{}, pagingATEClient()) // tenant-a has five actors, so a page of six ends one row into tenant-c. page, err := first.ListSubstrateActors(ctx, &apiv1.ListSubstrateActorsRequest{Page: &apiv1.PageRequest{Limit: 6}}) require.NoError(t, err) - require.Equal(t, "tenant-c", tokenAtespace(t, page.NextPageToken)) - - t.Run("a wider list resumes in the same atespace", func(t *testing.T) { - ateClient := pagingATEClient() - wider := system.NewService(nil, nil, []string{"tenant-a", "tenant-b", "tenant-c"}, &pkgAuth.NoopAuthorizer{}, ateClient) - next, err := wider.ListSubstrateActors(ctx, &apiv1.ListSubstrateActorsRequest{Page: &apiv1.PageRequest{Limit: 6, PageToken: page.NextPageToken}}) - require.NoError(t, err) - assert.Equal(t, []string{"tenant-c-actor-2", "tenant-c-actor-3"}, actorNames(next.Actors)) - for _, call := range ateClient.calls { - assert.Equal(t, "tenant-c", call.scope) - } - }) - - t.Run("a narrower list refuses it", func(t *testing.T) { - ateClient := pagingATEClient() - narrower := system.NewService(nil, nil, []string{"tenant-a"}, &pkgAuth.NoopAuthorizer{}, ateClient) - rejectsToken(t, ateClient, listActorsWithToken(narrower, page.NextPageToken)) - }) -} - -// A page that fills at the end of one atespace offers the next one, which may turn out -// to be empty. That last page is accepted: knowing in advance costs a read per page. -func TestListSubstrateActorsEmptyLastPage(t *testing.T) { - ctx := pagingContext(t) - ateClient := &fakeATEClient{actors: []*ateapipb.Actor{namedActor("tenant-a", "a-1"), namedActor("tenant-a", "a-2")}} - service := system.NewService(nil, nil, []string{"tenant-a", "tenant-c"}, &pkgAuth.NoopAuthorizer{}, ateClient) - pages := walkActors(t, ctx, service, "", 2) - require.Len(t, pages, 2) - assert.Equal(t, []string{"a-1", "a-2"}, actorNames(pages[0].Actors)) - assert.Empty(t, pages[1].Actors) - assert.Len(t, ateClient.calls, 2, "one read for each page") -} - -func TestListSubstrateActorsEmptyWatchedAtespace(t *testing.T) { - ctx := pagingContext(t) ateClient := pagingATEClient() - watched := []string{"tenant-a", "empty", "tenant-c"} - service := system.NewService(nil, nil, watched, &pkgAuth.NoopAuthorizer{}, ateClient) - - assert.Equal(t, ownActorNames(ateClient, watched), walkedActorNames(walkActors(t, ctx, service, "", 2))) - assert.Equal(t, []string{"empty", "tenant-a", "tenant-c"}, calledScopes(ateClient, "ListActorsPage")) -} - -// ate-api scopes each list, and the controller still keeps only that atespace's rows. -func TestListSubstrateActorsKeepsRowFilter(t *testing.T) { - ctx := pagingContext(t) - ateClient := pagingATEClient() - ateClient.ignoreAtespace = true - watched := []string{"tenant-a", "tenant-c"} - service := system.NewService(nil, nil, watched, &pkgAuth.NoopAuthorizer{}, ateClient) - - assert.Equal(t, ownActorNames(ateClient, watched), walkedActorNames(walkActors(t, ctx, service, "", 100))) - assert.Equal(t, watched, calledScopes(ateClient, "ListActorsPage")) + wider := system.NewService(nil, []string{"tenant-a", "tenant-b", "tenant-c"}, &pkgAuth.NoopAuthorizer{}, ateClient) + next, err := wider.ListSubstrateActors(ctx, &apiv1.ListSubstrateActorsRequest{Page: &apiv1.PageRequest{Limit: 6, PageToken: page.NextPageToken}}) + require.NoError(t, err) + assert.Equal(t, []string{"tenant-c-actor-2", "tenant-c-actor-3"}, actorNames(next.Actors)) + assert.Equal(t, []string{"tenant-c"}, calledScopes(ateClient, "ListActorsPage")) } func TestListSubstrateActorsNamedAtespace(t *testing.T) { ctx := pagingContext(t) ateClient := pagingATEClient() - service := system.NewService(nil, nil, []string{"tenant-a", "tenant-c"}, &pkgAuth.NoopAuthorizer{}, ateClient) + service := system.NewService(nil, []string{"tenant-a", "tenant-c"}, &pkgAuth.NoopAuthorizer{}, ateClient) pages := walkActors(t, ctx, service, "tenant-c", 1) - assert.Equal(t, ownActorNames(ateClient, []string{"tenant-c"}), walkedActorNames(pages)) - for _, call := range ateClient.calls { - assert.Equal(t, "tenant-c", call.scope) - } + assert.Equal(t, []string{"tenant-c-actor-1", "tenant-c-actor-2", "tenant-c-actor-3"}, walkedActorNames(pages)) + assert.Equal(t, []string{"tenant-c"}, calledScopes(ateClient, "ListActorsPage")) for _, page := range pages[:len(pages)-1] { assert.Equal(t, "tenant-c", tokenAtespace(t, page.NextPageToken)) } @@ -389,7 +274,7 @@ func TestListSubstrateActorsLaterCallFails(t *testing.T) { failCall: 2, failErr: errors.New("ate-api unreachable"), } - service := system.NewService(nil, nil, []string{"tenant-a", "tenant-c"}, &pkgAuth.NoopAuthorizer{}, ateClient) + service := system.NewService(nil, []string{"tenant-a", "tenant-c"}, &pkgAuth.NoopAuthorizer{}, ateClient) page, err := service.ListSubstrateActors(pagingContext(t), &apiv1.ListSubstrateActorsRequest{Page: &apiv1.PageRequest{Limit: 2}}) require.NoError(t, err) @@ -398,197 +283,117 @@ func TestListSubstrateActorsLaterCallFails(t *testing.T) { assert.Empty(t, page.NextPageToken) } -// A watched list with a repeat must not read an atespace twice, or offer a token that -// leads back to a page already read. -func TestNewServiceDeduplicatesWatched(t *testing.T) { - ctx := pagingContext(t) - ateClient := pagingATEClient() - service := system.NewService(nil, nil, []string{"tenant-c", "tenant-a", "tenant-a"}, &pkgAuth.NoopAuthorizer{}, ateClient) - - assert.Equal(t, ownActorNames(ateClient, []string{"tenant-a", "tenant-c"}), walkedActorNames(walkActors(t, ctx, service, "", 1))) - assert.Equal(t, []string{"tenant-a", "tenant-c"}, calledScopes(ateClient, "ListActorsPage"), "sorted") -} - -// The app passes an empty list for an install that watches every namespace. -func TestNewServiceWatchAllStaysWatchAll(t *testing.T) { - for name, watched := range map[string][]string{"nil": nil, "empty": {}} { - t.Run(name, func(t *testing.T) { - ateClient := pagingATEClient() - service := system.NewService(nil, nil, watched, &pkgAuth.NoopAuthorizer{}, ateClient) - - page, err := service.ListSubstrateActors(pagingContext(t), &apiv1.ListSubstrateActorsRequest{Page: &apiv1.PageRequest{Limit: 100}}) - require.NoError(t, err) - assert.Len(t, page.Actors, len(ateClient.actors)) - assert.Equal(t, []ateCall{{method: "ListActorsPage", size: 100}}, ateClient.calls) - }) - } -} - // The ate-api reads of a full walk, counted by hand from the paging data: tenant-a // has five actors and tenant-c three. func TestListSubstrateActorsCallCounts(t *testing.T) { - ctx := pagingContext(t) for limit, want := range map[int32]int{1: 8, 2: 5, 3: 4, 5: 2, 50: 2} { t.Run(strconv.Itoa(int(limit)), func(t *testing.T) { ateClient := pagingATEClient() - service := system.NewService(nil, nil, []string{"tenant-a", "tenant-c"}, &pkgAuth.NoopAuthorizer{}, ateClient) - - calls, pageToken := 0, "" - for range 100 { - page, err := service.ListSubstrateActors(ctx, &apiv1.ListSubstrateActorsRequest{Page: &apiv1.PageRequest{Limit: limit, PageToken: pageToken}}) - require.NoError(t, err) - pageCalls := ateClient.calls[calls:] - calls += len(pageCalls) - read := map[string]bool{} - for _, call := range pageCalls { - assert.False(t, read[call.scope], "page %v read %s twice", actorNames(page.Actors), call.scope) - read[call.scope] = true - } - if pageToken = page.NextPageToken; pageToken == "" { - break - } - } - assert.Equal(t, want, calls) + service := system.NewService(nil, []string{"tenant-a", "tenant-c"}, &pkgAuth.NoopAuthorizer{}, ateClient) + walkActors(t, pagingContext(t), service, "", limit) + assert.Len(t, ateClient.calls, want) }) } } -// Workers are read one by one, by the UID of a worker pod in a watched namespace. -func TestListSubstrateWorkersFromOwnPods(t *testing.T) { - ctx := pagingContext(t) - ateClient := pagingATEClient() - kubeClient := pagingKubeClient(t, interceptor.Funcs{}) - service := system.NewService(kubeClient, kubeClient, []string{"tenant-a", "tenant-c"}, &pkgAuth.NoopAuthorizer{}, ateClient) - - pages := walkWorkers(t, ctx, service, "", 1) - require.Len(t, pages, len(ownWorkerUIDs), "the last worker ends the walk") - assert.Equal(t, ownWorkerUIDs, walkedWorkerUIDs(pages)) - for i, page := range pages[:len(pages)-1] { - assert.Equal(t, ownWorkerUIDs[i], page.NextPageToken) - } - for _, call := range ateClient.calls { - assert.Equal(t, "GetWorker", call.method) - assert.Contains(t, ownWorkerUIDs, call.scope) - } -} - -func newPagingWorkerService(t *testing.T, ateClient *fakeATEClient, funcs interceptor.Funcs, pods ...*corev1.Pod) *system.Service { - kubeClient := pagingKubeClient(t, funcs, pods...) - return system.NewService(kubeClient, kubeClient, []string{"tenant-a", "tenant-c"}, &pkgAuth.NoopAuthorizer{}, ateClient) -} - -// A worker pod that is not Ready has no Worker yet. The page fills from later pods. -func TestListSubstrateWorkersSkipsUnregisteredPod(t *testing.T) { - ateClient := pagingATEClient() - service := newPagingWorkerService(t, ateClient, interceptor.Funcs{}, workerPod("tenant-a", "pool-a", "u2a-tenant-a")) - - pages := walkWorkers(t, pagingContext(t), service, "", 2) - assert.Equal(t, []string{"u1-tenant-a", "u3-tenant-c"}, walkedWorkerUIDs(pages[:1])) - assert.Equal(t, ownWorkerUIDs, walkedWorkerUIDs(pages)) -} - -// A Worker whose pod is gone is not shown, and not read. -func TestListSubstrateWorkersHidesStaleWorker(t *testing.T) { +// ate-api error text can name a worker of another namespace. The user sees a fixed text. +func TestListSubstrateWorkersFixedErrorText(t *testing.T) { ateClient := pagingATEClient() - ateClient.workers = append(ateClient.workers, ateWorker("tenant-a", "u9-tenant-a", 0)) - service := newPagingWorkerService(t, ateClient, interceptor.Funcs{}) + ateClient.failCall = 2 // the second page of the drain + ateClient.failErr = status.Error(codes.Internal, "worker u2-tenant-b is on a node that failed") - assert.Equal(t, ownWorkerUIDs, walkedWorkerUIDs(walkWorkers(t, pagingContext(t), service, "", 100))) - assert.NotContains(t, ateClient.calls, ateCall{method: "GetWorker", scope: "u9-tenant-a"}) + page := listWorkers(t, ownWorkerService(ateClient), 0, "") + assert.Equal(t, "ate-api could not list workers: Internal", page.ATEAPIError) + assert.Empty(t, page.Workers, "no partial page") + assert.Empty(t, page.NextPageToken) } -// A Worker that names another namespace or another pod than the pod it was read for -// is not that pod's Worker. -func TestListSubstrateWorkersChecksWorkerMatchesPod(t *testing.T) { - ateClient := pagingATEClient() - for _, worker := range ateClient.workers { - switch worker.GetWorkerPodUid() { - case "u4-tenant-a": - worker.WorkerNamespace = "tenant-b" - case "u6-tenant-a": - worker.WorkerPodUid = "u5-tenant-b" - } +// A named namespace narrows the view. A watched namespace with no worker is one empty +// page with no token. +func TestListSubstrateWorkersNamedNamespace(t *testing.T) { + for namespace, want := range map[string][]string{ + "tenant-a": {"u1-tenant-a", "u4-tenant-a", "u6-tenant-a"}, + "tenant-d": {}, + } { + t.Run(namespace, func(t *testing.T) { + service := system.NewService(nil, []string{"tenant-a", "tenant-c", "tenant-d"}, &pkgAuth.NoopAuthorizer{}, pagingATEClient()) + pages := walkWorkers(t, pagingContext(t), service, namespace, 1) + assert.Equal(t, want, walkedWorkerUIDs(pages)) + assert.Len(t, pages, max(1, len(want))) + }) } - service := newPagingWorkerService(t, ateClient, interceptor.Funcs{}) - - assert.Equal(t, []string{"u1-tenant-a", "u3-tenant-c"}, walkedWorkerUIDs(walkWorkers(t, pagingContext(t), service, "", 100))) } -// A worker token holds a UID only as a cursor. It is never sent to ate-api. -func TestListSubstrateWorkersForgedToken(t *testing.T) { - t.Run("a foreign UID only moves the cursor", func(t *testing.T) { - ateClient := pagingATEClient() - service := newPagingWorkerService(t, ateClient, interceptor.Funcs{}) - - page, err := service.ListSubstrateWorkers(pagingContext(t), &apiv1.ListSubstrateWorkersRequest{Page: &apiv1.PageRequest{PageToken: "u2-tenant-b"}}) - require.NoError(t, err) - assert.Equal(t, ownWorkerUIDs[1:], walkedWorkerUIDs([]system.SubstrateWorkerPage{page})) - for _, call := range ateClient.calls { - assert.Contains(t, ownWorkerUIDs, call.scope) - } - }) - - t.Run("longer than 1 KiB", func(t *testing.T) { - ateClient := pagingATEClient() - service := newPagingWorkerService(t, ateClient, interceptor.Funcs{}) - rejectsToken(t, ateClient, func(ctx context.Context) error { - _, err := service.ListSubstrateWorkers(ctx, &apiv1.ListSubstrateWorkersRequest{Page: &apiv1.PageRequest{PageToken: "u2-tenant-b" + strings.Repeat("x", 1024)}}) - return err - }) - }) +// endlessATEClient answers each worker page with no rows and the next token it is given. +type endlessATEClient struct { + fakeATEClient + next func(read int) string } -func TestListSubstrateWorkersNamedNamespace(t *testing.T) { - ateClient := pagingATEClient() - service := newPagingWorkerService(t, ateClient, interceptor.Funcs{}) - - tenantA := []string{"u1-tenant-a", "u4-tenant-a", "u6-tenant-a"} - assert.Equal(t, tenantA, walkedWorkerUIDs(walkWorkers(t, pagingContext(t), service, "tenant-a", 1))) - assert.Equal(t, tenantA, calledScopes(ateClient, "GetWorker")) - assert.Empty(t, calledScopes(ateClient, "ListWorkersPage")) +func (client *endlessATEClient) ListWorkersPage(context.Context, int32, string) ([]*ateapipb.Worker, string, error) { + client.workerReads++ + return nil, client.next(client.workerReads), nil } -// ate-api error text can name a worker of another namespace. The user sees a fixed text. -func TestListSubstrateWorkersFixedErrorText(t *testing.T) { - ateClient := pagingATEClient() - ateClient.failCall = 2 - ateClient.failErr = status.Error(codes.Internal, "worker u2-tenant-b is on a node that failed") - service := newPagingWorkerService(t, ateClient, interceptor.Funcs{}) - - page, err := service.ListSubstrateWorkers(pagingContext(t), &apiv1.ListSubstrateWorkersRequest{}) - require.NoError(t, err) - assert.Equal(t, "ate-api could not read a worker: Internal", page.ATEAPIError) - assert.Empty(t, page.Workers, "no partial page") - assert.Empty(t, page.NextPageToken) +// ate-api faults that stop a drain: a repeated token, which the drain error quotes, and +// a token that never ends. +func TestListSubstrateWorkersEndlessPages(t *testing.T) { + for name, tc := range map[string]struct { + next func(read int) string + reads int + }{ + "repeated token": {func(int) string { return "upstream:u2-tenant-b" }, 2}, + "endless tokens": {func(read int) string { return "upstream:" + strconv.Itoa(read) }, 10_000}, + } { + t.Run(name, func(t *testing.T) { + ateClient := &endlessATEClient{next: tc.next} + page := listWorkers(t, ownWorkerService(ateClient), 0, "") + assert.Equal(t, "ate-api could not list workers: Unknown", page.ATEAPIError) + assert.Empty(t, page.NextPageToken) + assert.Equal(t, tc.reads, ateClient.workerReads) + }) + } } -func TestListSubstrateWorkersPodListFails(t *testing.T) { - ateClient := pagingATEClient() - service := newPagingWorkerService(t, ateClient, interceptor.Funcs{ - List: func(context.Context, ctrlclient.WithWatch, ctrlclient.ObjectList, ...ctrlclient.ListOption) error { - return errors.New("etcd is down") - }, - }) - - page, err := service.ListSubstrateWorkers(pagingContext(t), &apiv1.ListSubstrateWorkersRequest{}) - require.NoError(t, err) - assert.NotEmpty(t, page.ATEAPIError) - assert.Empty(t, page.Workers) - assert.Empty(t, page.NextPageToken) - assert.Empty(t, ateClient.calls) +// A page holds 50 workers by default, in name order even when ate-api's is not. +func TestListSubstrateWorkersDefaultPageSize(t *testing.T) { + ateClient := &fakeATEClient{workerPageSize: 2} + for i := 59; i >= 0; i-- { + ateClient.workers = append(ateClient.workers, ateWorker("tenant-a", fmt.Sprintf("w%02d-tenant-a", i), 0)) + } + page := listWorkers(t, ownWorkerService(ateClient), 0, "") + require.Len(t, page.Workers, 50) + assert.Equal(t, "w00-tenant-a", page.Workers[0].GetWorkerPodUid()) + assert.Equal(t, "w49-tenant-a", page.NextPageToken) +} + +// Any token is a cursor: an actor token, a foreign worker name or a name past the end +// moves it, and no row, token or error names tenant-b. A worker added or deleted +// between pages moves it the same way. +func TestListSubstrateWorkersTokenIsCursor(t *testing.T) { + for name, tc := range map[string]struct { + pageToken string + want []string + }{ + "actor token": {token("tenant-a/upstream:tenant-b:tenant-b/tenant-b-actor-1"), ownWorkerUIDs}, + "foreign worker": {"u2-tenant-b", []string{"u3-tenant-c", "u4-tenant-a", "u6-tenant-a"}}, + "after every row": {"zzz", []string{}}, + } { + t.Run(name, func(t *testing.T) { + page := listWorkers(t, ownWorkerService(pagingATEClient()), 50, tc.pageToken) + assert.Equal(t, tc.want, walkedWorkerUIDs([]system.SubstrateWorkerPage{page})) + assert.Empty(t, page.NextPageToken) + assert.Empty(t, page.ATEAPIError) + }) + } } -// ownSummary is the summary of tenant-a and tenant-c in the paging data. -func assertOwnSummary(t *testing.T, summary system.SubstrateSummary) { - t.Helper() - assert.Equal(t, int64(8), summary.ActorCount) - assert.Equal(t, int64(8), summary.RunningActorCount) - assert.Equal(t, []string{"tenant-a-template", "tenant-c-template"}, templateNames(summary.ActorTemplates)) - assert.Equal(t, int64(4), summary.WorkerCount) - assert.Equal(t, int64(2), summary.BusyWorkerCount) +// newPagingWorkerService is ownWorkerService with WorkerPools for the summary. +func newPagingWorkerService(t *testing.T, ateClient system.ATEClient) *system.Service { + return system.NewService(pagingKubeClient(t, interceptor.Funcs{}), []string{"tenant-a", "tenant-c"}, &pkgAuth.NoopAuthorizer{}, ateClient) } -// calledScopes lists the atespace or worker of each call to the method. +// calledScopes lists the atespace of each call to the method, once each. func calledScopes(client *fakeATEClient, method string) []string { scopes := []string{} for _, call := range client.calls { @@ -599,23 +404,36 @@ func calledScopes(client *fakeATEClient, method string) []string { return scopes } -func TestGetSubstrateSummaryReadsOnlyWatched(t *testing.T) { - ateClient := pagingATEClient() - service := newPagingWorkerService(t, ateClient, interceptor.Funcs{}) +// The summary and the actor list read only watched atespaces, and keep only their rows +// even from an ate-api that drops the atespace filter. +func TestSubstrateReadsOnlyWatched(t *testing.T) { + for _, ignoreAtespace := range []bool{false, true} { + t.Run("ignoreAtespace="+strconv.FormatBool(ignoreAtespace), func(t *testing.T) { + ctx := pagingContext(t) + ateClient := pagingATEClient() + ateClient.ignoreAtespace = ignoreAtespace + service := newPagingWorkerService(t, ateClient) - summary, err := service.GetSubstrateSummary(pagingContext(t), "", "") - require.NoError(t, err) - assert.Empty(t, summary.ATEAPIError) - assertOwnSummary(t, summary) - assert.Equal(t, []string{"tenant-a", "tenant-c"}, calledScopes(ateClient, "ListActorsPage")) - assert.Equal(t, []string{"tenant-a", "tenant-c"}, calledScopes(ateClient, "ListActorTemplates")) - assert.ElementsMatch(t, ownWorkerUIDs, calledScopes(ateClient, "GetWorker")) - assert.Empty(t, calledScopes(ateClient, "ListWorkersPage")) + summary, err := service.GetSubstrateSummary(ctx, "", "") + require.NoError(t, err) + assert.Empty(t, summary.ATEAPIError) + assert.Equal(t, int64(8), summary.ActorCount) + assert.Equal(t, int64(8), summary.RunningActorCount) + assert.Equal(t, []string{"tenant-a-template", "tenant-c-template"}, templateNames(summary.ActorTemplates)) + assert.Equal(t, int64(4), summary.WorkerCount) + assert.Equal(t, int64(2), summary.BusyWorkerCount) + assert.Equal(t, []string{"tenant-a", "tenant-c"}, calledScopes(ateClient, "ListActorsPage")) + assert.Equal(t, []string{"tenant-a", "tenant-c"}, calledScopes(ateClient, "ListActorTemplates")) + assert.Equal(t, 3, ateClient.workerReads, "one drain") + + assert.Equal(t, ownActorNames(ateClient, []string{"tenant-a", "tenant-c"}), walkedActorNames(walkActors(t, ctx, service, "", 100))) + }) + } } func TestGetSubstrateSummaryInputs(t *testing.T) { ateClient := pagingATEClient() - service := newPagingWorkerService(t, ateClient, interceptor.Funcs{}) + service := newPagingWorkerService(t, ateClient) summary, err := service.GetSubstrateSummary(pagingContext(t), "tenant-a", "tenant-c") require.NoError(t, err) @@ -623,31 +441,20 @@ func TestGetSubstrateSummaryInputs(t *testing.T) { assert.Equal(t, int64(3), summary.WorkerCount) assert.Equal(t, []string{"tenant-c"}, calledScopes(ateClient, "ListActorsPage")) assert.Equal(t, []string{"tenant-c"}, calledScopes(ateClient, "ListActorTemplates")) - assert.ElementsMatch(t, []string{"u1-tenant-a", "u4-tenant-a", "u6-tenant-a"}, calledScopes(ateClient, "GetWorker")) -} - -// An ate-api that drops the atespace filter cannot show another tenant's template, -// nor one template twice. -func TestGetSubstrateSummaryKeepsTemplateFilter(t *testing.T) { - ateClient := pagingATEClient() - ateClient.ignoreAtespace = true - service := newPagingWorkerService(t, ateClient, interceptor.Funcs{}) - - summary, err := service.GetSubstrateSummary(pagingContext(t), "", "") - require.NoError(t, err) - assert.Equal(t, []string{"tenant-a-template", "tenant-c-template"}, templateNames(summary.ActorTemplates)) - summary, err = service.GetSubstrateSummary(pagingContext(t), "", "tenant-a") + summary, err = service.GetSubstrateSummary(pagingContext(t), "tenant-c", "") require.NoError(t, err) - assert.Equal(t, []string{"tenant-a-template"}, templateNames(summary.ActorTemplates)) + assert.Equal(t, int64(1), summary.WorkerCount) + assert.Equal(t, int64(1), summary.BusyWorkerCount) } -// One failed scoped read is a warning. The reads that did not fail still count. +// A failed second template list drops the first one's templates too. The other reads +// still count. func TestGetSubstrateSummaryPartialFailure(t *testing.T) { ateClient := pagingATEClient() ateClient.failCall = 2 // the tenant-c template list ateClient.failErr = errors.New("templates unavailable") - service := newPagingWorkerService(t, ateClient, interceptor.Funcs{}) + service := newPagingWorkerService(t, ateClient) summary, err := service.GetSubstrateSummary(pagingContext(t), "", "") require.NoError(t, err) @@ -655,57 +462,21 @@ func TestGetSubstrateSummaryPartialFailure(t *testing.T) { assert.Empty(t, summary.ActorTemplates) assert.Equal(t, int64(8), summary.ActorCount) assert.Equal(t, int64(4), summary.WorkerCount) - assert.Equal(t, int64(2), summary.BusyWorkerCount) -} - -// A failed Worker read is a warning. Templates and actors still count. -func TestGetSubstrateSummaryWorkerUnavailable(t *testing.T) { - ateClient := pagingATEClient() - ateClient.failCall = 5 // the first GetWorker, after two template and two actor lists - ateClient.failErr = status.Error(codes.Unavailable, "worker u1-tenant-a is unreachable") - service := newPagingWorkerService(t, ateClient, interceptor.Funcs{}) - - summary, err := service.GetSubstrateSummary(pagingContext(t), "", "") - require.NoError(t, err) - assert.Equal(t, "ate-api could not read a worker: Unavailable", summary.ATEAPIError) - assert.Equal(t, []string{"tenant-a-template", "tenant-c-template"}, templateNames(summary.ActorTemplates)) - assert.Equal(t, int64(8), summary.ActorCount) - assert.Equal(t, int64(8), summary.RunningActorCount) - assert.Zero(t, summary.WorkerCount) -} - -// A failed worker pod list is a warning too. WorkerPools come from the cached client -// and still list. -func TestGetSubstrateSummaryPodListFails(t *testing.T) { - ateClient := pagingATEClient() - podReader := pagingKubeClient(t, interceptor.Funcs{ - List: func(context.Context, ctrlclient.WithWatch, ctrlclient.ObjectList, ...ctrlclient.ListOption) error { - return errors.New("etcd is down") - }, - }) - service := system.NewService(pagingKubeClient(t, interceptor.Funcs{}), podReader, []string{"tenant-a", "tenant-c"}, &pkgAuth.NoopAuthorizer{}, ateClient) - - summary, err := service.GetSubstrateSummary(pagingContext(t), "", "") - require.NoError(t, err) - assert.Equal(t, "failed to list worker pods", summary.ATEAPIError) - assert.Len(t, summary.WorkerPools, 2) - assert.Equal(t, []string{"tenant-a-template", "tenant-c-template"}, templateNames(summary.ActorTemplates)) - assert.Equal(t, int64(8), summary.ActorCount) - assert.Zero(t, summary.WorkerCount) - assert.Empty(t, calledScopes(ateClient, "GetWorker")) } // Walks every RPC with each watched list and page size of the paging data. A walk // reads, and its tokens name, only watched data. It returns each own row once, and no -// page is empty beside a token: that would tell the caller a hidden row exists. +// page is empty beside a token: that would tell the caller a hidden row exists. The +// lists cover a repeat, which must not read an atespace twice, and a watched atespace +// with no rows. func TestSubstrateWalksStayInWatchedNamespaces(t *testing.T) { - for _, watched := range [][]string{{"tenant-a", "tenant-c"}, {"tenant-a"}} { + for _, watched := range [][]string{{"tenant-a", "tenant-c"}, {"tenant-a"}, {"tenant-c", "tenant-a", "tenant-a"}, {"tenant-a", "empty", "tenant-c"}} { for _, limit := range []int32{1, 2, 3, 4, 5, 6, 7, 100} { t.Run(fmt.Sprintf("%v/%d", watched, limit), func(t *testing.T) { ctx := pagingContext(t) ateClient := pagingATEClient() kubeClient := pagingKubeClient(t, interceptor.Funcs{}) - service := system.NewService(kubeClient, kubeClient, watched, &pkgAuth.NoopAuthorizer{}, ateClient) + service := system.NewService(kubeClient, watched, &pkgAuth.NoopAuthorizer{}, ateClient) ownWorkerIDs := []string{} for _, uid := range ownWorkerUIDs { if slices.ContainsFunc(watched, func(namespace string) bool { return strings.HasSuffix(uid, namespace) }) { @@ -721,8 +492,9 @@ func TestSubstrateWalksStayInWatchedNamespaces(t *testing.T) { switch call.method { case "ListActorsPage", "ListActorTemplates": assert.Contains(t, watched, call.scope, "%+v", call) - case "GetWorker": - assert.Contains(t, ownWorkerIDs, call.scope, "%+v", call) + case "ListWorkersPage": + // A drain sends only ate-api's own tokens. + assert.Contains(t, []string{"", "upstream:2", "upstream:4"}, call.token, "%+v", call) default: t.Errorf("unexpected call %+v", call) } @@ -750,21 +522,12 @@ func TestSubstrateWalksStayInWatchedNamespaces(t *testing.T) { } } -// A controller that watches every namespace answers as it did before: ate-api's page, -// and ate-api's token unchanged. +// A controller that watches every namespace (the app passes an empty list) answers as +// it did before: ate-api's page, token and error text unchanged. func TestSubstratePassThroughWhenWatchingAll(t *testing.T) { ctx := pagingContext(t) ateClient := pagingATEClient() - podLists := 0 - kubeClient := pagingKubeClient(t, interceptor.Funcs{ - List: func(ctx context.Context, c ctrlclient.WithWatch, list ctrlclient.ObjectList, opts ...ctrlclient.ListOption) error { - if _, ok := list.(*corev1.PodList); ok { - podLists++ - } - return c.List(ctx, list, opts...) - }, - }) - service := system.NewService(kubeClient, kubeClient, nil, &pkgAuth.NoopAuthorizer{}, ateClient) + service := system.NewService(nil, []string{}, &pkgAuth.NoopAuthorizer{}, ateClient) for _, atespace := range []string{"", "tenant-b"} { page, err := service.ListSubstrateActors(ctx, &apiv1.ListSubstrateActorsRequest{Atespace: atespace, Page: &apiv1.PageRequest{Limit: 1}}) @@ -774,46 +537,11 @@ func TestSubstratePassThroughWhenWatchingAll(t *testing.T) { assert.Equal(t, "upstream:"+atespace+":"+page.Actors[0].GetMetadata().GetAtespace()+"/"+page.Actors[0].GetMetadata().GetName(), page.NextPageToken) } - workers, err := service.ListSubstrateWorkers(ctx, &apiv1.ListSubstrateWorkersRequest{Page: &apiv1.PageRequest{Limit: 1}}) - require.NoError(t, err) - assert.Equal(t, "upstream:1", workers.NextPageToken) - _, err = service.GetSubstrateSummary(ctx, "", "") - require.NoError(t, err) - assert.Empty(t, calledScopes(ateClient, "GetWorker")) - assert.Zero(t, podLists) -} + page := listWorkers(t, service, 1, "upstream:2") + assert.Equal(t, []string{"u3-tenant-c"}, walkedWorkerUIDs([]system.SubstrateWorkerPage{page})) + assert.Equal(t, "upstream:3", page.NextPageToken) + assert.Equal(t, ateCall{method: "ListWorkersPage", token: "upstream:2", size: 1}, ateClient.calls[len(ateClient.calls)-1]) -func TestSubstrateRowsChangeBetweenPages(t *testing.T) { - ctx := pagingContext(t) - ateClient := pagingATEClient() - kubeClient := pagingKubeClient(t, interceptor.Funcs{}) - service := system.NewService(kubeClient, kubeClient, []string{"tenant-a", "tenant-c"}, &pkgAuth.NoopAuthorizer{}, ateClient) - - actors, err := service.ListSubstrateActors(ctx, &apiv1.ListSubstrateActorsRequest{Page: &apiv1.PageRequest{Limit: 2}}) - require.NoError(t, err) - require.Equal(t, []string{"tenant-a-actor-1", "tenant-a-actor-2"}, actorNames(actors.Actors)) - ateClient.actors = slices.DeleteFunc(ateClient.actors, func(actor *ateapipb.Actor) bool { return actor.GetMetadata().GetName() == "tenant-a-actor-4" }) - ateClient.actors = append(ateClient.actors, namedActor("tenant-a", "tenant-a-actor-0")) - rest := []string{} - for pageToken := actors.NextPageToken; pageToken != ""; { - page, err := service.ListSubstrateActors(ctx, &apiv1.ListSubstrateActorsRequest{Page: &apiv1.PageRequest{Limit: 2, PageToken: pageToken}}) - require.NoError(t, err) - require.Empty(t, page.ATEAPIError) - rest, pageToken = append(rest, actorNames(page.Actors)...), page.NextPageToken - } - assert.Equal(t, []string{"tenant-a-actor-3", "tenant-a-actor-5", "tenant-c-actor-1", "tenant-c-actor-2", "tenant-c-actor-3"}, rest) - - workers, err := service.ListSubstrateWorkers(ctx, &apiv1.ListSubstrateWorkersRequest{Page: &apiv1.PageRequest{Limit: 1}}) - require.NoError(t, err) - require.Equal(t, ownWorkerUIDs[:1], walkedWorkerUIDs([]system.SubstrateWorkerPage{workers})) - require.NoError(t, kubeClient.Delete(ctx, workerPod("tenant-a", "pool-a", "u4-tenant-a"))) - require.NoError(t, kubeClient.Create(ctx, workerPod("tenant-a", "pool-a", "u0a-tenant-a"))) - rest = []string{} - for pageToken := workers.NextPageToken; pageToken != ""; { - page, err := service.ListSubstrateWorkers(ctx, &apiv1.ListSubstrateWorkersRequest{Page: &apiv1.PageRequest{Limit: 1, PageToken: pageToken}}) - require.NoError(t, err) - require.Empty(t, page.ATEAPIError) - rest, pageToken = append(rest, walkedWorkerUIDs([]system.SubstrateWorkerPage{page})...), page.NextPageToken - } - assert.Equal(t, []string{"u3-tenant-c", "u6-tenant-a"}, rest) + ateClient.err = errors.New("worker u2-tenant-b is down") + assert.Equal(t, "worker u2-tenant-b is down", listWorkers(t, service, 1, "").ATEAPIError) } diff --git a/go/core/internal/substrate/client.go b/go/core/internal/substrate/client.go index ab5c856bf9..d5542c13e2 100644 --- a/go/core/internal/substrate/client.go +++ b/go/core/internal/substrate/client.go @@ -145,13 +145,6 @@ func (c *Client) GetActorTemplate(ctx context.Context, atespace, name string) (* return c.ControlClient.GetActorTemplate(ctx, &ateapipb.GetActorTemplateRequest{ActorTemplate: actorRef(atespace, name)}) } -// GetWorker reads a Worker by name. ate-api names each Worker after its pod's UID. -func (c *Client) GetWorker(ctx context.Context, name string) (*ateapipb.Worker, error) { - ctx, cancel := c.callCtx(ctx) - defer cancel() - return c.ControlClient.GetWorker(ctx, &ateapipb.GetWorkerRequest{Worker: &ateapipb.ObjectRef{Name: name}}) -} - func (c *Client) CreateActorTemplate(ctx context.Context, template *ateapipb.ActorTemplate) (*ateapipb.ActorTemplate, error) { ctx, cancel := c.callCtx(ctx) defer cancel() diff --git a/go/core/pkg/app/app.go b/go/core/pkg/app/app.go index 2faa46259f..13bfd5fe13 100644 --- a/go/core/pkg/app/app.go +++ b/go/core/pkg/app/app.go @@ -316,7 +316,7 @@ func Run(ctx context.Context, input *configv1.ControllerConfig, opts Options) er models := modelservice.NewService(manager.GetClient(), authorizer, resourceNamespace) tools := toolservice.NewService(manager.GetClient(), store, authorizer, resourceNamespace, mcpClient) prompts := prompttemplateservice.NewService(manager.GetClient(), authorizer) - system := systemservice.NewService(manager.GetClient(), manager.GetAPIReader(), watchNamespaces, authorizer, actors) + system := systemservice.NewService(manager.GetClient(), watchNamespaces, authorizer, actors) memory := memoryservice.NewService(store) quiescenceWake := make(chan struct{}, 1) pushWake := make(chan struct{}, 1) diff --git a/proto/kagent/api/v1/system.proto b/proto/kagent/api/v1/system.proto index d8a1a3ad77..0eb8c62bbb 100644 --- a/proto/kagent/api/v1/system.proto +++ b/proto/kagent/api/v1/system.proto @@ -119,10 +119,10 @@ message ListSubstrateActorsResponse { google.protobuf.Timestamp computed_at = 5; } -// One page of workers, in worker pod UID order. A controller that watches all -// namespaces filters one Substrate page, which may be empty and still have a next -// token. A controller that watches some reads only the workers of its own worker pods; -// only its last page can be empty. +// One page of workers, in worker name order. A controller that watches all namespaces +// filters one Substrate page, which may be empty and still have a next token. A +// controller that watches some reads every Substrate worker and returns only those of +// its watched namespaces; only its last page can be empty. message ListSubstrateWorkersRequest { string namespace = 1 [(buf.validate.field).string = { max_len: 63 diff --git a/ui/playwright/DEFERRED.md b/ui/playwright/DEFERRED.md index be664c4f12..530fd90a6f 100644 --- a/ui/playwright/DEFERRED.md +++ b/ui/playwright/DEFERRED.md @@ -219,11 +219,13 @@ when it holds one page of them, and only the proto says which. | `ListSubstrateActors` | `ListSubstrateActorsRequest { atespace, page }` | a page | `string filter` and a sort field enum — **and ate-api has to grow them first** | | `ListSubstrateWorkers` | `ListSubstrateWorkersRequest { namespace, page }` | a page | the same two, on the same condition | -**Substrate actor and worker lists use upstream pagination.** Each list request makes -one ate-api call and preserves its order and continuation token. Actors use upstream -atespace filtering. Worker namespace filtering applies only to the returned page, so an -empty worker page may still have a next token. The UI labels counts as "on this page" -and offers no actor/worker text search or column sorting. +**Substrate actor and worker lists use upstream pagination.** A controller that watches +every namespace makes one ate-api call per list request and passes its order and +continuation token through. Worker namespace filtering then applies only to the returned +page, so an empty worker page may still have a next token. A controller that watches some +namespaces reads actors with one scoped ate-api list per watched atespace and returns its +own token. It reads every ate-api worker page and returns a worker-name cursor. The UI +labels counts as "on this page" and offers no actor/worker text search or column sorting. Two capabilities remain deferred until Substrate supports them: diff --git a/ui/src/generated/kagent/api/v1/system_pb.ts b/ui/src/generated/kagent/api/v1/system_pb.ts index 959c6d5067..492410d558 100644 --- a/ui/src/generated/kagent/api/v1/system_pb.ts +++ b/ui/src/generated/kagent/api/v1/system_pb.ts @@ -359,10 +359,10 @@ export const ListSubstrateActorsResponseSchema: GenMessage