diff --git a/api/builds/v1alpha1/imagebuild_types.go b/api/builds/v1alpha1/imagebuild_types.go index d583bbf..1ab2d46 100644 --- a/api/builds/v1alpha1/imagebuild_types.go +++ b/api/builds/v1alpha1/imagebuild_types.go @@ -186,10 +186,10 @@ 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] @@ -197,28 +197,26 @@ func buildInputsHash(buildSpec *corev1alpha1.StackResourceBuildSpec) string { 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 { diff --git a/api/builds/v1alpha1/imagebuild_types_test.go b/api/builds/v1alpha1/imagebuild_types_test.go index 55d232e..8299be2 100644 --- a/api/builds/v1alpha1/imagebuild_types_test.go +++ b/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) } } diff --git a/internal/controller/imagebuild/imagebuild_controller.go b/internal/controller/imagebuild/imagebuild_controller.go index 4034776..631836c 100644 --- a/internal/controller/imagebuild/imagebuild_controller.go +++ b/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(), diff --git a/internal/controller/imagebuild/imagebuild_test.go b/internal/controller/imagebuild/imagebuild_test.go index 47e3367..2e57ffb 100644 --- a/internal/controller/imagebuild/imagebuild_test.go +++ b/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()) }) })