diff --git a/docs/09-Configuration reference/settings.catalog.json b/docs/09-Configuration reference/settings.catalog.json index 24bcba1f..77894ea2 100644 --- a/docs/09-Configuration reference/settings.catalog.json +++ b/docs/09-Configuration reference/settings.catalog.json @@ -365,7 +365,7 @@ "key": "jobs.\u003cowner-kind\u003e.containers.\u003ccontainer-name\u003e.env-vars", "valueType": "map[string]string", "sources": [ - "internal/resources/jobs/job.go:122" + "internal/resources/jobs/job.go:124" ] }, { @@ -383,14 +383,14 @@ } ], "sources": [ - "internal/resources/jobs/job.go:66" + "internal/resources/jobs/job.go:68" ] }, { "key": "jobs.\u003cowner-kind\u003e.init-containers.\u003ccontainer-name\u003e.env-vars", "valueType": "map[string]string", "sources": [ - "internal/resources/jobs/job.go:132" + "internal/resources/jobs/job.go:134" ] }, { @@ -408,14 +408,14 @@ } ], "sources": [ - "internal/resources/jobs/job.go:77" + "internal/resources/jobs/job.go:79" ] }, { "key": "jobs.\u003cowner-kind\u003e.spec.template.annotations", "valueType": "map[string]string", "sources": [ - "internal/resources/jobs/job.go:150" + "internal/resources/jobs/job.go:152" ] }, { diff --git a/internal/resources/jobs/job.go b/internal/resources/jobs/job.go index 095136f3..95f14ff8 100644 --- a/internal/resources/jobs/job.go +++ b/internal/resources/jobs/job.go @@ -1,6 +1,8 @@ package jobs import ( + "crypto/sha256" + "encoding/hex" "fmt" "maps" "strings" @@ -165,6 +167,24 @@ func withSettingAnnotations(ctx core.Context, owner v1beta1.Dependent) HandleJob }) } +// maxNameLength is the longest Job name the apiserver accepts: the Job +// controller copies the name into the job-name label, capped at 63 bytes. +const maxNameLength = 63 + +// Name returns the name of the Job that Handle creates for owner. +// Names that fit are left unchanged, so existing Jobs keep their name. +// Longer names are truncated and suffixed with a hash of the full name. +func Name(owner client.Object, jobName string) string { + name := fmt.Sprintf("%s-%s", owner.GetUID(), jobName) + if len(name) <= maxNameLength { + return name + } + + hash := sha256.Sum256([]byte(name)) + suffix := hex.EncodeToString(hash[:4]) + return name[:maxNameLength-len(suffix)-1] + "-" + suffix +} + func Handle(ctx core.Context, owner v1beta1.Dependent, jobName string, container v1.Container, options ...HandleJobOption) error { configuration := &handleJobConfiguration{} if options == nil { @@ -183,7 +203,7 @@ func Handle(ctx core.Context, owner v1beta1.Dependent, jobName string, container option(configuration) } - jobName = fmt.Sprintf("%s-%s", owner.GetUID(), jobName) + jobName = Name(owner, jobName) job := &batchv1.Job{} err := ctx.GetClient().Get(ctx, types.NamespacedName{ Namespace: owner.GetStack(), diff --git a/internal/resources/jobs/name_test.go b/internal/resources/jobs/name_test.go new file mode 100644 index 00000000..5fd2cf3d --- /dev/null +++ b/internal/resources/jobs/name_test.go @@ -0,0 +1,88 @@ +package jobs + +import ( + "strings" + "testing" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/validation" + + "github.com/formancehq/operator/v3/api/formance.com/v1beta1" +) + +func TestName(t *testing.T) { + t.Parallel() + + owner := &v1beta1.Broker{ + ObjectMeta: metav1.ObjectMeta{ + UID: types.UID("120b264e-5be3-49a8-bd4a-71e2ea097ae3"), + }, + } + + tests := []struct { + name string + jobName string + want string + }{ + { + name: "short name is unchanged", + jobName: "create-stream", + want: "120b264e-5be3-49a8-bd4a-71e2ea097ae3-create-stream", + }, + { + name: "63-byte name is unchanged", + jobName: "create-topic-reconciliatio", + want: "120b264e-5be3-49a8-bd4a-71e2ea097ae3-create-topic-reconciliatio", + }, + { + name: "64-byte name is shortened", + jobName: "create-topic-reconciliation", + }, + { + name: "much longer name is shortened", + jobName: "create-topic-transactionplane-with-a-very-long-suffix", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + got := Name(owner, tt.jobName) + if tt.want != "" && got != tt.want { + t.Fatalf("Name() = %q, want %q", got, tt.want) + } + if len(got) > maxNameLength { + t.Fatalf("Name() = %q is %d bytes, want at most %d", got, len(got), maxNameLength) + } + if errs := validation.IsValidLabelValue(got); len(errs) > 0 { + t.Fatalf("Name() = %q is not a valid label value: %v", got, errs) + } + if errs := validation.IsDNS1123Subdomain(got); len(errs) > 0 { + t.Fatalf("Name() = %q is not a valid object name: %v", got, errs) + } + if !strings.HasPrefix(got, string(owner.GetUID())+"-") { + t.Fatalf("Name() = %q does not start with the owner UID", got) + } + if again := Name(owner, tt.jobName); again != got { + t.Fatalf("Name() is not stable: %q then %q", got, again) + } + }) + } +} + +func TestNameDistinguishesLongNamesWithSamePrefix(t *testing.T) { + t.Parallel() + + owner := &v1beta1.Broker{ + ObjectMeta: metav1.ObjectMeta{ + UID: types.UID("120b264e-5be3-49a8-bd4a-71e2ea097ae3"), + }, + } + + first := Name(owner, "create-topic-reconciliation-a") + second := Name(owner, "create-topic-reconciliation-b") + if first == second { + t.Fatalf("Name() returned %q for two different job names", first) + } +} diff --git a/internal/tests/jobs_controller_test.go b/internal/tests/jobs_controller_test.go index 3d30c03d..20b82765 100644 --- a/internal/tests/jobs_controller_test.go +++ b/internal/tests/jobs_controller_test.go @@ -146,14 +146,14 @@ var _ = Describe("Job", func() { }) It("Should have annotations set", func() { j := &batchv1.Job{} - Expect(LoadResource(stack.Name, fmt.Sprintf("%s-%s", module.GetUID(), job.Name), j)).To(Succeed()) + Expect(LoadResource(stack.Name, jobs.Name(module, job.Name), j)).To(Succeed()) Expect(j.Spec.Template.Annotations).ToNot(BeNil()) Expect(j.Spec.Template.Annotations["first"]).To(Equal("second")) }) It("Should have security context configured with run-as settings", func() { j := &batchv1.Job{} - Expect(LoadResource(stack.Name, fmt.Sprintf("%s-%s", module.GetUID(), job.Name), j)).To(Succeed()) + Expect(LoadResource(stack.Name, jobs.Name(module, job.Name), j)).To(Succeed()) for _, container := range j.Spec.Template.Spec.Containers { Expect(container.SecurityContext).ToNot(BeNil()) @@ -171,6 +171,21 @@ var _ = Describe("Job", func() { Expect(*container.SecurityContext.RunAsGroup).To(Equal(int64(runAS.group))) } }) + Context("With a name too long for the job-name label", func() { + BeforeEach(func() { + // 36-byte owner UID + "-" + 27 bytes = 64 bytes, one over the label limit. + job.Name = "create-topic-reconciliation" + }) + It("Should create the job under a shortened name", func() { + name := jobs.Name(module, job.Name) + Expect(len(module.GetUID()) + 1 + len(job.Name)).To(Equal(64)) + Expect(len(name)).To(BeNumerically("<=", 63)) + + j := &batchv1.Job{} + Expect(LoadResource(stack.Name, name, j)).To(Succeed()) + Expect(j).To(BeControlledBy(module)) + }) + }) AfterEach(func() { for _, setting := range settings { Expect(Delete(&setting)).To(Succeed())