fix(imagebuild): derive build Job name from all build inputs

api/builds/v1alpha1/imagebuild_types.go

@@ -186,39 +186,37 @@ func ImageBuildName(resourceName string, buildSpec *corev1alpha1.StackResourceBu
 	return name + "-" + buildInputsHash(buildSpec)
 }
 
-func buildInputsHash(buildSpec *corev1alpha1.StackResourceBuildSpec) string {
-	raw, err := json.Marshal(buildSpec)
+func buildInputsHash(inputs any) string {
+	raw, err := json.Marshal(inputs)
 	if err != nil {
-		raw = []byte(fmt.Sprintf("%#v", buildSpec))
+		raw = []byte(fmt.Sprintf("%#v", inputs))
 	}
 	sum := sha256.Sum256(raw)
 	return hex.EncodeToString(sum[:])[:buildInputsHashLen]
 }
 
 const (
 	buildJobSuffix       = "-build"
-	shortRevisionLen     = 8
 	maxLabelValueLen     = 63
-	maxResourceNameInJob = maxLabelValueLen - shortRevisionLen - len(buildJobSuffix) - 1 // 48
+	maxResourceNameInJob = maxLabelValueLen - buildInputsHashLen - len(buildJobSuffix) - 1 // 48
 )
 
-func BuildJobName(resourceName string, sourceRevision string) string {
-	cleanName := SanitizeDNSLabel(resourceName, "app")
+// BuildJobName derives the build Job name from every spec input that changes the
+// produced image — source revision, Dockerfile path, context path, repository and
+// build args — so a spec change gets its own Job instead of adopting a completed
+// one from an earlier build.
+func (w *ImageBuild) BuildJobName() string {
+	cleanName := SanitizeDNSLabel(w.Spec.ResourceName, "app")
 
 	if len(cleanName) > maxResourceNameInJob {
 		cleanName = cleanName[:maxResourceNameInJob]
 		cleanName = strings.TrimSuffix(cleanName, "-")
 	}
 
-	var cleanRev string
-	if sourceRevision == "" {
-		cleanRev = "rev"
-	} else {
-		hash := sha256.Sum256([]byte(sourceRevision))
-		cleanRev = fmt.Sprintf("%x", hash)[:shortRevisionLen]
-	}
+	inputs := w.Spec
+	inputs.Cancelled = false // cancelling deletes the Job; not a build input
 
-	return fmt.Sprintf("%s-%s%s", cleanName, cleanRev, buildJobSuffix)
+	return fmt.Sprintf("%s-%s%s", cleanName, buildInputsHash(inputs), buildJobSuffix)
 }
 
 func (w *ImageBuild) ShortBuildSrcRevisionFromStatus() string {

api/builds/v1alpha1/imagebuild_types_test.go

@@ -1,125 +1,132 @@
 package v1alpha1
 
 import (
+	"regexp"
 	"strings"
 	"testing"
 
 	corev1alpha1 "stackdome.io/cluster-agent/api/core/v1alpha1"
 )
 
-func TestBuildJobName_BranchCollisionFixed(t *testing.T) {
-	// This is the bug we're fixing - truncation made these identical
-	a := BuildJobName("app", "create-stackfile-42010165d15be77adc3a6ae05563a40e5ca9bb5d")
-	b := BuildJobName("app", "create-stackfile-e3eb341d3a9aa5a89e7c4a0cc4fd7c9e10d7d58d")
-	if a == b {
-		t.Errorf("different commits on same long branch should produce different names, both = %q", a)
+// A Job name must be a DNS-1123 label: the Job controller copies it verbatim into
+// the `batch.kubernetes.io/job-name` label on every pod it creates, and label
+// values top out at 63 characters.
+var dns1123LabelRegex = regexp.MustCompile(`^[a-z0-9]([-a-z0-9]*[a-z0-9])?$`)
+
+func imageBuildFor(resourceName, revision string) *ImageBuild {
+	return &ImageBuild{
+		Spec: ImageBuildSpec{
+			ResourceName: resourceName,
+			SourceRevision: corev1alpha1.SourceRevisionSpec{
+				Volume: &corev1alpha1.VolumeRevision{RevisionString: revision},
+			},
+			BuildContext: BuildContextSpec{
+				DockerfilePath: "Dockerfile",
+				ContextPath:    "/",
+			},
+		},
 	}
 }
 
-func TestBuildJobName(t *testing.T) {
+func assertValidJobName(t *testing.T, name string) {
+	t.Helper()
+	if len(name) > maxLabelValueLen {
+		t.Errorf("BuildJobName() = %q (len %d), exceeds k8s label limit %d", name, len(name), maxLabelValueLen)
+	}
+	if !dns1123LabelRegex.MatchString(name) {
+		t.Errorf("BuildJobName() = %q, not a valid DNS-1123 label", name)
+	}
+}
+
+func TestBuildJobName_HonorsK8sNameLimits(t *testing.T) {
 	tests := []struct {
-		name           string
-		resourceName   string
-		sourceRevision string
-		wantName       string
-		wantMaxLen     int
+		name         string
+		resourceName string
+		revision     string
 	}{
-		{
-			name:           "short name stays as-is",
-			resourceName:   "my-app",
-			sourceRevision: "abc123def456",
-			wantName:       "my-app-e861b2ea-build",
-			wantMaxLen:     63,
-		},
-		{
-			name:           "full 40-char SHA is truncated to 8",
-			resourceName:   "frontend",
-			sourceRevision: "20d73f323a4d95ff5a3847717892e1740a5a81b6",
-			wantName:       "frontend-3bfc284a-build",
-			wantMaxLen:     63,
-		},
-		{
-			name:           "long resource name from the issue",
-			resourceName:   "broken-app-broken-dockerfile",
-			sourceRevision: "20d73f323a4d95ff5a3847717892e1740a5a81b6",
-			wantName:       "broken-app-broken-dockerfile-3bfc284a-build",
-			wantMaxLen:     63,
-		},
-		{
-			name:           "very long resource name gets truncated",
-			resourceName:   "this-is-an-extremely-long-resource-name-that-exceeds-the-limit",
-			sourceRevision: "abc123def456",
-			wantMaxLen:     63,
-		},
-		{
-			name:           "short revision used as-is (hashed)",
-			resourceName:   "app",
-			sourceRevision: "abc",
-			wantName:       "app-ba7816bf-build",
-			wantMaxLen:     63,
-		},
-		{
-			name:           "branch name with slash and uppercase",
-			resourceName:   "app",
-			sourceRevision: "Feature/Cool-Stuff",
-			wantName:       "app-9d30e60e-build",
-			wantMaxLen:     63,
-		},
-		{
-			name:           "branch name with dots and underscores",
-			resourceName:   "app",
-			sourceRevision: "v1.2.3_beta",
-			wantName:       "app-0b506316-build",
-			wantMaxLen:     63,
-		},
-		{
-			name:           "non-ASCII UTF-8 branch name",
-			resourceName:   "app",
-			sourceRevision: "feature/日本語",
-			wantName:       "app-ffc43221-build",
-			wantMaxLen:     63,
-		},
-		{
-			name:           "empty revision falls back to rev",
-			resourceName:   "app",
-			sourceRevision: "",
-			wantName:       "app-rev-build",
-			wantMaxLen:     63,
-		},
+		{"short name", "my-app", "abc123def456"},
+		{"full 40-char SHA", "frontend", "20d73f323a4d95ff5a3847717892e1740a5a81b6"},
+		{"long resource name", "broken-app-broken-dockerfile", "20d73f323a4d95ff5a3847717892e1740a5a81b6"},
+		{"very long resource name gets truncated", strings.Repeat("a", 200), "abc123def456"},
+		{"resource name truncated onto a dash", strings.Repeat("ab", 24) + "-tail", "abc"},
+		{"branch with slash and uppercase", "app", "Feature/Cool-Stuff"},
+		{"branch with dots and underscores", "app", "v1.2.3_beta"},
+		{"non-ASCII UTF-8 branch name", "app", "feature/日本語"},
+		{"empty resource name falls back", "", "abc123def456"},
+		{"empty revision", "app", ""},
+		{"resource name of only separators", "___", "abc"},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got := BuildJobName(tt.resourceName, tt.sourceRevision)
-			if len(got) > tt.wantMaxLen {
-				t.Errorf("BuildJobName() = %q (len %d), exceeds max %d", got, len(got), tt.wantMaxLen)
-			}
-			if tt.wantName != "" && got != tt.wantName {
-				t.Errorf("BuildJobName() = %q, want %q", got, tt.wantName)
-			}
+			assertValidJobName(t, imageBuildFor(tt.resourceName, tt.revision).BuildJobName())
 		})
 	}
 }
 
+func TestBuildJobName_ReadablePrefix(t *testing.T) {
+	got := imageBuildFor("broken-app-broken-dockerfile", "abc123def456").BuildJobName()
+	if !strings.HasPrefix(got, "broken-app-broken-dockerfile-") || !strings.HasSuffix(got, "-build") {
+		t.Errorf("BuildJobName() = %q, want resource-name prefix and -build suffix", got)
+	}
+}
+
 func TestBuildJobName_Deterministic(t *testing.T) {
-	a := BuildJobName("my-app", "abc123def456")
-	b := BuildJobName("my-app", "abc123def456")
+	a := imageBuildFor("my-app", "abc123def456").BuildJobName()
+	b := imageBuildFor("my-app", "abc123def456").BuildJobName()
 	if a != b {
 		t.Errorf("BuildJobName is not deterministic: %q != %q", a, b)
 	}
 }
 
+func TestBuildJobName_CancelledIsNotABuildInput(t *testing.T) {
+	base := imageBuildFor("app", "abc123def456")
+	cancelled := imageBuildFor("app", "abc123def456")
+	cancelled.Spec.Cancelled = true
+	if base.BuildJobName() != cancelled.BuildJobName() {
+		t.Errorf("cancelling should not change the job name: %q != %q", base.BuildJobName(), cancelled.BuildJobName())
+	}
+}
+
 func TestBuildJobName_DifferentInputsProduceDifferentNames(t *testing.T) {
-	a := BuildJobName("app-a", "abc123def456")
-	b := BuildJobName("app-b", "abc123def456")
-	if a == b {
-		t.Errorf("different resource names should produce different job names: both = %q", a)
+	base := imageBuildFor("app", "abc123def456")
+
+	tests := []struct {
+		name   string
+		mutate func(b *ImageBuild)
+	}{
+		{"resource name", func(b *ImageBuild) { b.Spec.ResourceName = "other-app" }},
+		{"source revision", func(b *ImageBuild) {
+			b.Spec.SourceRevision.Volume.RevisionString = "def456abc123"
+		}},
+		// The bug this covers: same commit, different Dockerfile. Before, both
+		// builds mapped to one Job and the second adopted the first's result.
+		{"dockerfile path", func(b *ImageBuild) { b.Spec.BuildContext.DockerfilePath = "worker/Dockerfile" }},
+		{"context path", func(b *ImageBuild) { b.Spec.BuildContext.ContextPath = "hello-stack/worker" }},
+		{"build args", func(b *ImageBuild) {
+			b.Spec.BuildArgs = []corev1alpha1.BuildArg{{Name: "MODE", Value: "debug"}}
+		}},
+		{"repository", func(b *ImageBuild) { b.Spec.Repository.Repository = "other-repo" }},
 	}
 
-	c := BuildJobName("app-a", "abc123def456")
-	d := BuildJobName("app-a", "def456abc123")
-	if c == d {
-		t.Errorf("different revisions should produce different job names: both = %q", c)
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			changed := imageBuildFor("app", "abc123def456")
+			tt.mutate(changed)
+			if got := changed.BuildJobName(); got == base.BuildJobName() {
+				t.Errorf("%s change should produce a different job name, both = %q", tt.name, got)
+			}
+			assertValidJobName(t, changed.BuildJobName())
+		})
+	}
+}
+
+func TestBuildJobName_BranchCollisionFixed(t *testing.T) {
+	// Truncation used to make these identical.
+	a := imageBuildFor("app", "create-stackfile-42010165d15be77adc3a6ae05563a40e5ca9bb5d").BuildJobName()
+	b := imageBuildFor("app", "create-stackfile-e3eb341d3a9aa5a89e7c4a0cc4fd7c9e10d7d58d").BuildJobName()
+	if a == b {
+		t.Errorf("different commits on same long branch should produce different names, both = %q", a)
 	}
 }
 

internal/controller/imagebuild/imagebuild_controller.go

@@ -263,7 +263,7 @@ func (r *ImageBuildReconciler) reconcileImageBuildWithVolumeSource(ctx context.C
 	}
 
 	sourceRevision := buildConfig.Spec.SourceRevision.GetSourceRevisionString()
-	jobName := buildsv1alpha1.BuildJobName(buildConfig.Spec.ResourceName, sourceRevision)
+	jobName := buildConfig.BuildJobName()
 	buildSource := &imagebuilder.Source{
 		Volume: &imagebuilder.VolumeSource{
 			PvcName: volumeRef.Status.PvcName,
@@ -334,7 +334,7 @@ func (r *ImageBuildReconciler) reconcileImageBuildWithGitSource(ctx context.Cont
 	}
 
 	sourceRevision := buildConfig.Spec.SourceRevision.GetSourceRevisionString()
-	jobName := buildsv1alpha1.BuildJobName(buildConfig.Spec.ResourceName, sourceRevision)
+	jobName := buildConfig.BuildJobName()
 	buildSource := &imagebuilder.Source{
 		GitRepo: &imagebuilder.GitRepoBuildSource{
 			Repo:     buildConfig.Spec.BuildContext.ContextSource.Git.DeepCopy(),

internal/controller/imagebuild/imagebuild_test.go

@@ -224,7 +224,7 @@ var _ = Describe("reconcile with a ClusterRegistryRef destination", func() {
 			Expect(result.RequeueAfter).To(BeZero())
 
 			job := &batchv1.Job{}
-			jobName := buildsv1alpha1.BuildJobName(resourceName, commit)
+			jobName := buildConfig.BuildJobName()
 			Expect(r.Client.Get(context.Background(), types.NamespacedName{Name: jobName, Namespace: ns}, job)).To(Succeed())
 		})
 	})