fix(spawner): Normalize Task names to valid RFC 1123 names

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
+}

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))
+	}
+}