From 46a50125976b55b43052d160fa8017b414915e55 Mon Sep 17 00:00:00 2001 From: Matt Van Horn Date: Fri, 25 Sep 2026 06:13:21 +0000 Subject: [PATCH] feat(server): reject resource names that are not RFC 1123 labels ax apply accepted any metadata.name, but Substrate requires actor, ActorTemplate and atespace names to be lowercase RFC 1123 labels. A name such as Task-With-Caps was therefore stored, and only failed seconds later with an asynchronous ActorCreationFailed condition that the user had to dig out of ax describe or the controller logs. Validate metadata.name (and metadata.atespace when set) against [a-z0-9]([-a-z0-9]*[a-z0-9])? with a 63 character limit for Task, Workspace and Model alike, and return InvalidArgument at apply time. Fixes #370 Co-authored-by: Matt Van Horn --- docs/manifests.md | 2 + internal/server/server.go | 6 +++ internal/server/server_test.go | 42 +++++++++++++++++ pkg/apis/v1alpha1/types.go | 57 ++++++++++++++++++++++- pkg/apis/v1alpha1/types_test.go | 81 ++++++++++++++++++++++++++++++++- 5 files changed, 186 insertions(+), 2 deletions(-) diff --git a/docs/manifests.md b/docs/manifests.md index c3bdcb12..744248fa 100644 --- a/docs/manifests.md +++ b/docs/manifests.md @@ -2,6 +2,8 @@ All four kinds can live in one multi-document YAML file. See [`examples/task.yaml`](../examples/task.yaml) for a complete, working set. +`metadata.name` and `metadata.atespace` become Substrate resource names, so they must be lowercase RFC 1123 labels: at most 63 lowercase alphanumeric characters or `-`, starting and ending with an alphanumeric character. `ax apply` rejects anything else up front rather than letting the task fail later with `ActorCreationFailed`. + ## Task ```yaml diff --git a/internal/server/server.go b/internal/server/server.go index ed69f099..82fbabd6 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -274,6 +274,9 @@ func (s *Server) UpdateWorkspace(ctx context.Context, req *v1alpha1.UpdateWorksp if req == nil || req.Workspace == nil { return nil, status.Error(codes.InvalidArgument, "workspace required") } + if err := v1alpha1.ValidateWorkspace(req.Workspace); err != nil { + return nil, status.Error(codes.InvalidArgument, err.Error()) + } req.Workspace.Metadata = defaultMetadata(req.Workspace.Metadata, func(atespace, name string) *v1alpha1.ObjectMeta { existing, err := s.store.GetWorkspace(ctx, atespace, name) if err != nil { @@ -337,6 +340,9 @@ func (s *Server) UpdateModel(ctx context.Context, req *v1alpha1.UpdateModelReque if req == nil || req.Model == nil { return nil, status.Error(codes.InvalidArgument, "model required") } + if err := v1alpha1.ValidateModel(req.Model); err != nil { + return nil, status.Error(codes.InvalidArgument, err.Error()) + } req.Model.Metadata = defaultMetadata(req.Model.Metadata, func(atespace, name string) *v1alpha1.ObjectMeta { existing, err := s.store.GetModel(ctx, atespace, name) if err != nil { diff --git a/internal/server/server_test.go b/internal/server/server_test.go index 6a2e35d9..ef144c9f 100644 --- a/internal/server/server_test.go +++ b/internal/server/server_test.go @@ -252,6 +252,48 @@ func TestServerGRPC(t *testing.T) { } } +// Names and atespaces become Substrate resource names, which must be RFC 1123 +// labels. The server rejects them up front instead of letting the controller +// fail asynchronously with ActorCreationFailed. +func TestUpdate_RejectsInvalidNames(t *testing.T) { + srv := server.NewServer(memory.NewStore()) + ctx := context.Background() + + for _, meta := range []*v1alpha1.ObjectMeta{ + {Name: "Task-With-Caps"}, + {Name: "under_score"}, + {Name: ""}, + {Name: "ok", Atespace: "Not-Lowercase"}, + } { + if _, err := srv.UpdateTask(ctx, &v1alpha1.UpdateTaskRequest{Task: &v1alpha1.Task{Metadata: meta}}); status.Code(err) != codes.InvalidArgument { + t.Errorf("UpdateTask(%v): got %v, want InvalidArgument", meta, err) + } + if _, err := srv.UpdateWorkspace(ctx, &v1alpha1.UpdateWorkspaceRequest{Workspace: &v1alpha1.Workspace{Metadata: meta}}); status.Code(err) != codes.InvalidArgument { + t.Errorf("UpdateWorkspace(%v): got %v, want InvalidArgument", meta, err) + } + if _, err := srv.UpdateModel(ctx, &v1alpha1.UpdateModelRequest{Model: &v1alpha1.Model{Metadata: meta}}); status.Code(err) != codes.InvalidArgument { + t.Errorf("UpdateModel(%v): got %v, want InvalidArgument", meta, err) + } + } + + // Nothing invalid was persisted. + if resp, err := srv.ListTasks(ctx, &v1alpha1.ListTasksRequest{}); err != nil || len(resp.GetTasks()) != 0 { + t.Errorf("ListTasks after rejected applies = %v, %v; want empty", resp.GetTasks(), err) + } + + // Valid names still go through, with and without an explicit atespace. + good := &v1alpha1.ObjectMeta{Name: "task-with-caps", Atespace: "team-a"} + if _, err := srv.UpdateTask(ctx, &v1alpha1.UpdateTaskRequest{Task: &v1alpha1.Task{Metadata: good}}); err != nil { + t.Errorf("UpdateTask(%v): %v", good, err) + } + if _, err := srv.UpdateWorkspace(ctx, &v1alpha1.UpdateWorkspaceRequest{Workspace: &v1alpha1.Workspace{Metadata: &v1alpha1.ObjectMeta{Name: "ws-1"}}}); err != nil { + t.Errorf("UpdateWorkspace: %v", err) + } + if _, err := srv.UpdateModel(ctx, &v1alpha1.UpdateModelRequest{Model: &v1alpha1.Model{Metadata: &v1alpha1.ObjectMeta{Name: "gemini"}}}); err != nil { + t.Errorf("UpdateModel: %v", err) + } +} + func TestUpdateTask_ValidatesWorkspaceBindings(t *testing.T) { srv := server.NewServer(memory.NewStore()) ctx := context.Background() diff --git a/pkg/apis/v1alpha1/types.go b/pkg/apis/v1alpha1/types.go index bb1b02c7..c3195d15 100644 --- a/pkg/apis/v1alpha1/types.go +++ b/pkg/apis/v1alpha1/types.go @@ -17,7 +17,9 @@ package v1alpha1 import ( "bytes" "encoding/json" + "errors" "fmt" + "regexp" "strconv" "strings" "time" @@ -241,9 +243,62 @@ func (s *TaskSpec) WorkspacePaths() []string { return paths } -// ValidateTask reports the first problem with a task's spec that would make it +// Resource names. +// +// A resource's name and atespace become Substrate resource names (the task's +// actor and ActorTemplate, and the atespace itself), which must be lowercase +// RFC 1123 labels. Checking them at apply time turns an asynchronous +// ActorCreationFailed condition into an immediate error. + +// MaxNameLength is the longest name or atespace a resource may have. +const MaxNameLength = 63 + +var nameRegexp = regexp.MustCompile(`^[a-z0-9]([-a-z0-9]*[a-z0-9])?$`) + +// ValidateName reports whether s can be used as a resource name or atespace: a +// lowercase RFC 1123 label of at most MaxNameLength characters. +func ValidateName(s string) error { + if s == "" { + return errors.New("must not be empty") + } + if len(s) > MaxNameLength || !nameRegexp.MatchString(s) { + return fmt.Errorf("must be a lowercase RFC 1123 label: at most %d lowercase alphanumeric characters or '-', starting and ending with an alphanumeric character", MaxNameLength) + } + return nil +} + +// ValidateObjectMeta checks a resource's name and, when set, its atespace. An +// empty atespace is fine here; the API server defaults it. +func ValidateObjectMeta(meta *ObjectMeta) error { + if err := ValidateName(meta.GetName()); err != nil { + return fmt.Errorf("metadata.name: invalid value %q: %w", meta.GetName(), err) + } + if atespace := meta.GetAtespace(); atespace != "" { + if err := ValidateName(atespace); err != nil { + return fmt.Errorf("metadata.atespace: invalid value %q: %w", atespace, err) + } + } + return nil +} + +// ValidateWorkspace reports the first problem with a workspace that would make +// it unusable. It is called by the API server before saving. +func ValidateWorkspace(w *Workspace) error { + return ValidateObjectMeta(w.GetMetadata()) +} + +// ValidateModel reports the first problem with a model that would make it +// unusable. It is called by the API server before saving. +func ValidateModel(m *Model) error { + return ValidateObjectMeta(m.GetMetadata()) +} + +// ValidateTask reports the first problem with a task that would make it // impossible to run correctly. It is called by the API server before saving. func ValidateTask(t *Task) error { + if err := ValidateObjectMeta(t.GetMetadata()); err != nil { + return err + } spec := t.GetSpec() if spec == nil { return nil diff --git a/pkg/apis/v1alpha1/types_test.go b/pkg/apis/v1alpha1/types_test.go index 6520db24..99ab8bd0 100644 --- a/pkg/apis/v1alpha1/types_test.go +++ b/pkg/apis/v1alpha1/types_test.go @@ -368,6 +368,85 @@ func TestTaskSpec_WorkspaceRefs(t *testing.T) { } } +func TestValidateName(t *testing.T) { + valid := []string{ + "a", + "0", + "task123", + "my-task", + "a-b-c", + "123-abc", + strings.Repeat("a", 63), + } + for _, name := range valid { + if err := v1alpha1.ValidateName(name); err != nil { + t.Errorf("ValidateName(%q) = %v, want nil", name, err) + } + } + + invalid := []string{ + "", + "Task-With-Caps", + "my_task", + "my.task", + "-leading-dash", + "trailing-dash-", + "with space", + "tab\tchar", + "ünïcödé", + "slash/name", + strings.Repeat("a", 64), + } + for _, name := range invalid { + if err := v1alpha1.ValidateName(name); err == nil { + t.Errorf("ValidateName(%q) = nil, want error", name) + } + } +} + +func TestValidateObjectMeta(t *testing.T) { + tests := []struct { + name string + meta *v1alpha1.ObjectMeta + wantErr string + }{ + {name: "nil metadata", wantErr: "metadata.name"}, + {name: "missing name", meta: &v1alpha1.ObjectMeta{Atespace: "default"}, wantErr: `metadata.name: invalid value ""`}, + {name: "name only", meta: &v1alpha1.ObjectMeta{Name: "task123"}}, + {name: "name and atespace", meta: &v1alpha1.ObjectMeta{Name: "task123", Atespace: "team-a"}}, + {name: "uppercase name", meta: &v1alpha1.ObjectMeta{Name: "Task-With-Caps"}, wantErr: `metadata.name: invalid value "Task-With-Caps"`}, + {name: "underscore in name", meta: &v1alpha1.ObjectMeta{Name: "my_task"}, wantErr: "metadata.name"}, + {name: "name too long", meta: &v1alpha1.ObjectMeta{Name: strings.Repeat("x", 64)}, wantErr: "metadata.name"}, + {name: "uppercase atespace", meta: &v1alpha1.ObjectMeta{Name: "task123", Atespace: "Default"}, wantErr: `metadata.atespace: invalid value "Default"`}, + {name: "dotted atespace", meta: &v1alpha1.ObjectMeta{Name: "task123", Atespace: "team.a"}, wantErr: "metadata.atespace"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := v1alpha1.ValidateObjectMeta(tt.meta) + if tt.wantErr == "" { + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + return + } + if err == nil || !strings.Contains(err.Error(), tt.wantErr) { + t.Fatalf("error = %v, want containing %q", err, tt.wantErr) + } + }) + } + + // Every kind is validated the same way. + if err := v1alpha1.ValidateTask(&v1alpha1.Task{Metadata: &v1alpha1.ObjectMeta{Name: "Bad"}}); err == nil { + t.Error("ValidateTask accepted an invalid name") + } + if err := v1alpha1.ValidateWorkspace(&v1alpha1.Workspace{Metadata: &v1alpha1.ObjectMeta{Name: "Bad"}}); err == nil { + t.Error("ValidateWorkspace accepted an invalid name") + } + if err := v1alpha1.ValidateModel(&v1alpha1.Model{Metadata: &v1alpha1.ObjectMeta{Name: "Bad"}}); err == nil { + t.Error("ValidateModel accepted an invalid name") + } +} + func TestValidateTask(t *testing.T) { tests := []struct { name string @@ -412,7 +491,7 @@ func TestValidateTask(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - err := v1alpha1.ValidateTask(&v1alpha1.Task{Spec: tt.spec}) + err := v1alpha1.ValidateTask(&v1alpha1.Task{Metadata: &v1alpha1.ObjectMeta{Name: "task"}, Spec: tt.spec}) if tt.wantErr == "" { if err != nil { t.Fatalf("unexpected error: %v", err)