diff --git a/cmd/kelos-spawner/main.go b/cmd/kelos-spawner/main.go index 7e1f67ff6..222079771 100644 --- a/cmd/kelos-spawner/main.go +++ b/cmd/kelos-spawner/main.go @@ -2,6 +2,8 @@ package main import ( "context" + "crypto/sha256" + "encoding/hex" "flag" "fmt" "net/http" @@ -16,6 +18,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" utilruntime "k8s.io/apimachinery/pkg/util/runtime" + "k8s.io/apimachinery/pkg/util/validation" clientgoscheme "k8s.io/client-go/kubernetes/scheme" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/cache" @@ -301,7 +304,7 @@ func runCycleWithSourceCore(ctx context.Context, cl client.Client, key types.Nam var newItems []source.WorkItem for _, item := range items { - taskName := fmt.Sprintf("%s-%s", ts.Name, item.ID) + taskName := buildTaskName(ts.Name, item.ID) existing, found := existingTaskMap[taskName] if !found { newItems = append(newItems, item) @@ -367,7 +370,7 @@ func runCycleWithSourceCore(ctx context.Context, cl client.Client, key types.Nam break } - taskName := fmt.Sprintf("%s-%s", ts.Name, item.ID) + taskName := buildTaskName(ts.Name, item.ID) templateVars := source.WorkItemToTemplateVars(item) @@ -807,3 +810,36 @@ func parsePollInterval(s string) time.Duration { } return d } + +// taskNameMaxLength bounds generated Task names to a single RFC 1123 label. +// Although a Kubernetes object name may be up to DNS1123SubdomainMaxLength, the +// name is reused as the kelos.dev/task label value and as the Job name, both of +// which are limited to a 63-character label. +const taskNameMaxLength = validation.DNS1123LabelMaxLength + +// buildTaskName combines a spawner name and work item ID into a valid RFC 1123 +// name (at most taskNameMaxLength) for use as a Task object name. It normalizes +// IDs that are not name-safe on their own, such as Jira issue keys (e.g. +// "PROJECT-1234"), whose uppercase letters would otherwise be rejected by +// Kubernetes. Names that are already valid and within length are returned +// unchanged. Over-length names are truncated with a short hash suffix so that +// distinct names do not collide on a shared prefix. +func buildTaskName(spawnerName, itemID string) string { + combined := spawnerName + "-" + itemID + + if len(combined) <= taskNameMaxLength && len(validation.IsDNS1123Subdomain(combined)) == 0 { + return combined + } + + segments := strings.FieldsFunc(strings.ToLower(combined), func(r rune) bool { + return !('a' <= r && r <= 'z' || '0' <= r && r <= '9') + }) + name := strings.Join(segments, "-") + + if len(name) > taskNameMaxLength { + sum := sha256.Sum256([]byte(name)) + suffix := "-" + hex.EncodeToString(sum[:])[:10] + name = strings.TrimRight(name[:taskNameMaxLength-len(suffix)], "-") + suffix + } + return name +} diff --git a/cmd/kelos-spawner/taskname_test.go b/cmd/kelos-spawner/taskname_test.go new file mode 100644 index 000000000..be4b7b4f2 --- /dev/null +++ b/cmd/kelos-spawner/taskname_test.go @@ -0,0 +1,107 @@ +package main + +import ( + "context" + "strings" + "testing" + + "k8s.io/apimachinery/pkg/util/validation" + "sigs.k8s.io/controller-runtime/pkg/client" + + kelos "github.com/kelos-dev/kelos/api/v1alpha2" + "github.com/kelos-dev/kelos/internal/source" +) + +func TestBuildTaskName(t *testing.T) { + cases := []struct { + name string + spawner string + id string + want string + }{ + {"jira key is lowercased", "my-spawner", "PROJECT-1234", "my-spawner-project-1234"}, + {"github numeric id unchanged", "spawner", "42", "spawner-42"}, + {"cron timestamp unchanged", "spawner", "20060102-1504", "spawner-20060102-1504"}, + {"invalid characters replaced", "spawner", "PROJ_1", "spawner-proj-1"}, + {"consecutive dashes collapsed", "spawner", "PROJ--1", "spawner-proj-1"}, + {"dots treated as separators", "spawner", "a..b", "spawner-a-b"}, + {"trailing separator trimmed", "spawner", "ABC-", "spawner-abc"}, + {"all-invalid id falls back to spawner", "spawner", "@@@", "spawner"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := buildTaskName(tc.spawner, tc.id) + if got != tc.want { + t.Errorf("buildTaskName(%q, %q) = %q, want %q", tc.spawner, tc.id, got, tc.want) + } + // The result must be a name the Kubernetes API server accepts. + if errs := validation.IsDNS1123Subdomain(got); len(errs) != 0 { + t.Errorf("buildTaskName(%q, %q) = %q is not a valid RFC 1123 name: %v", tc.spawner, tc.id, got, errs) + } + }) + } +} + +func TestBuildTaskNameLength(t *testing.T) { + // A valid but over-length name (lowercase alphanumerics) must still be + // capped: this exercises both the length guard on the fast path and the + // truncation branch. + got := buildTaskName("spawner", strings.Repeat("a", 100)) + if len(got) > taskNameMaxLength { + t.Errorf("name length = %d, want <= %d (%q)", len(got), taskNameMaxLength, got) + } + if errs := validation.IsDNS1123Subdomain(got); len(errs) != 0 { + t.Errorf("name %q is not a valid RFC 1123 name: %v", got, errs) + } + + // Distinct over-length inputs that share a truncated prefix must not collide, + // otherwise dedup would silently skip one of them. + a := buildTaskName("spawner", strings.Repeat("a", 80)+"-one") + b := buildTaskName("spawner", strings.Repeat("a", 80)+"-two") + if a == b { + t.Errorf("distinct long inputs collided: both produced %q", a) + } + if len(a) > taskNameMaxLength || len(b) > taskNameMaxLength { + t.Errorf("truncated names exceed %d: %q (%d), %q (%d)", taskNameMaxLength, a, len(a), b, len(b)) + } +} + +func TestRunCycleWithSource_NormalizesJiraKeyAndDedupes(t *testing.T) { + ts := newTaskSpawner("spawner", "default", nil) + cl, key := setupTest(t, ts) + + src := &fakeSource{ + items: []source.WorkItem{ + {ID: "PROJECT-1234", Number: 1234, Title: "Fix the thing"}, + }, + } + + // First cycle: the Task is created with a normalized name. + if err := runCycleWithSource(context.Background(), cl, key, src); err != nil { + t.Fatalf("first cycle: unexpected error: %v", err) + } + + var taskList kelos.TaskList + if err := cl.List(context.Background(), &taskList, client.InNamespace("default")); err != nil { + t.Fatalf("listing tasks: %v", err) + } + if len(taskList.Items) != 1 { + t.Fatalf("after first cycle: expected 1 task, got %d", len(taskList.Items)) + } + if got, want := taskList.Items[0].Name, "spawner-project-1234"; got != want { + t.Errorf("task name = %q, want %q", got, want) + } + + // Second cycle, same item: must not create a duplicate. This proves the + // dedup lookup name and the creation name are derived identically. + if err := runCycleWithSource(context.Background(), cl, key, src); err != nil { + t.Fatalf("second cycle: unexpected error: %v", err) + } + if err := cl.List(context.Background(), &taskList, client.InNamespace("default")); err != nil { + t.Fatalf("listing tasks: %v", err) + } + if len(taskList.Items) != 1 { + t.Fatalf("after second cycle: expected 1 task (no duplicate), got %d", len(taskList.Items)) + } +}