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)