From bf2cfe1886c4378eb755346ce548f00bb71bac42 Mon Sep 17 00:00:00 2001 From: Jose Diaz-Gonzalez Date: Sun, 24 May 2026 02:50:09 -0400 Subject: [PATCH] fix: hash scheduler-k3s cron-id label to fit Kubernetes' 63-byte cap The `dokku.com/cron-id` label could exceed Kubernetes' 63-byte cap because the cron ID is `base36(appName === command === schedule)`, which expands roughly 1.5x per byte. The label is now keyed `dokku.com/cron-hash` and holds the `sha1` hex digest of the cron-id, a fixed 40-character value that always fits the cap. The same hex digest is mirrored into the `dokku.com/cron-hash` annotation, and the original base36 cron-id stays in the `dokku.com/cron-id` annotation that `cron:list` reads when surfacing user-facing IDs. Per-task lookups stay server-side via label selectors, so `cron:set --maintenance` and the forbid/replace concurrency checks on `dokku run --cron-id` keep working without any in-memory filtering. --- .../scheduler-k3s/cron_job_template_test.go | 182 ++++++++++++++---- plugins/scheduler-k3s/functions.go | 16 +- plugins/scheduler-k3s/template.go | 2 + .../templates/chart/cron-job.yaml | 3 + plugins/scheduler-k3s/triggers.go | 17 +- tests/apps/python/app-cnb-cron.json | 2 +- tests/unit/builder-pack.bats | 2 +- tests/unit/scheduler-k3s-cnb.bats | 22 ++- tests/unit/scheduler-k3s-dockerfile.bats | 20 ++ tests/unit/scheduler-k3s-herokuish.bats | 20 ++ 10 files changed, 235 insertions(+), 51 deletions(-) diff --git a/plugins/scheduler-k3s/cron_job_template_test.go b/plugins/scheduler-k3s/cron_job_template_test.go index 4770f7cb8..b814528a2 100644 --- a/plugins/scheduler-k3s/cron_job_template_test.go +++ b/plugins/scheduler-k3s/cron_job_template_test.go @@ -15,7 +15,8 @@ import ( ) // TestCronIDLabelValue asserts the hashed cron ID fits inside Kubernetes' 63 -// byte label cap and is deterministic. Without this the label can exceed the +// byte label cap, is a fixed 40-character sha1 hex digest, and is +// deterministic. Without this the dokku.com/cron-hash label can exceed the // cap and the Kubernetes API server rejects the manifest. func TestCronIDLabelValue(t *testing.T) { cases := []string{ @@ -25,6 +26,9 @@ func TestCronIDLabelValue(t *testing.T) { } for _, in := range cases { out := cronIDLabelValue(in) + if len(out) != 40 { + t.Errorf("cronIDLabelValue(%q) = %q (len %d); want 40 hex chars", in, out, len(out)) + } if len(out) > 63 { t.Errorf("cronIDLabelValue(%q) = %q (len %d); must be <= 63", in, out, len(out)) } @@ -35,11 +39,123 @@ func TestCronIDLabelValue(t *testing.T) { } // TestCronJobTemplateQuotesAllDigitSuffix asserts that the rendered cron-job -// manifest produces string-typed annotation values even when the suffix and -// cron-id consist entirely of digits. Without `| quote` in the template, YAML -// would coerce these to numbers and the manifest would be rejected by the -// Kubernetes API server. +// manifest produces string-typed annotation and label values even when the +// suffix and cron-id consist entirely of digits. Without `| quote` in the +// template, YAML would coerce these to numbers and the manifest would be +// rejected by the Kubernetes API server. func TestCronJobTemplateQuotesAllDigitSuffix(t *testing.T) { + manifest := renderCronJobTemplate(t, map[string]interface{}{ + "id": "1234567890", + "hash": "0123456789abcdef0123456789abcdef01234567", + "schedule": "5 5 5 5 5", + "suffix": "1234567890", + "suspend": false, + "concurrency_policy": "Allow", + }) + + forEachManifestMetadata(t, manifest, func(metadata map[string]interface{}) { + annotations, _ := metadata["annotations"].(map[string]interface{}) + for key, value := range annotations { + if _, isString := value.(string); !isString { + t.Errorf("annotation %q has non-string value %v (type %T); helm template must apply | quote", key, value, value) + } + } + labels, _ := metadata["labels"].(map[string]interface{}) + for key, value := range labels { + if _, isString := value.(string); !isString { + t.Errorf("label %q has non-string value %v (type %T); helm template must apply | quote", key, value, value) + } + } + }) +} + +// TestCronJobTemplateEmitsHashLabel asserts that the rendered manifest +// emits the sha1 hash as the dokku.com/cron-hash label and matching +// annotation, while keeping the original base36 cron-id in the +// dokku.com/cron-id annotation so cron:list can still surface it to users. +func TestCronJobTemplateEmitsHashLabel(t *testing.T) { + originalID := "app===echo hello-from-cron===5 5 5 5 5" + hashedID := cronIDLabelValue(originalID) + + manifest := renderCronJobTemplate(t, map[string]interface{}{ + "id": originalID, + "hash": hashedID, + "schedule": "5 5 5 5 5", + "suffix": "abcde", + "suspend": false, + "concurrency_policy": "Allow", + }) + + sawHashLabel := false + sawIDAnnotation := false + forEachManifestMetadata(t, manifest, func(metadata map[string]interface{}) { + if labels, ok := metadata["labels"].(map[string]interface{}); ok { + if value, ok := labels["dokku.com/cron-hash"]; ok { + sawHashLabel = true + if value != hashedID { + t.Errorf("dokku.com/cron-hash label = %q, want %q", value, hashedID) + } + } + } + annotations, _ := metadata["annotations"].(map[string]interface{}) + if value, ok := annotations["dokku.com/cron-hash"]; ok { + if value != hashedID { + t.Errorf("dokku.com/cron-hash annotation = %q, want %q (must mirror the label)", value, hashedID) + } + } + if value, ok := annotations["dokku.com/cron-id"]; ok { + sawIDAnnotation = true + if value != originalID { + t.Errorf("dokku.com/cron-id annotation = %q, want %q", value, originalID) + } + } + }) + if !sawHashLabel { + t.Errorf("rendered manifest did not include a dokku.com/cron-hash label") + } + if !sawIDAnnotation { + t.Errorf("rendered manifest did not include a dokku.com/cron-id annotation") + } +} + +// TestCronJobTemplateRendersLongCronID is the direct regression test for +// dokku/dokku#8594: a cron-id well over the 63-byte Kubernetes label cap +// must render cleanly and every resulting label value must stay under the +// cap. +func TestCronJobTemplateRendersLongCronID(t *testing.T) { + longCronID := strings.Repeat("a-very-long-cron-id-that-would-far-exceed-the-label-cap-", 10) + manifest := renderCronJobTemplate(t, map[string]interface{}{ + "id": longCronID, + "hash": cronIDLabelValue(longCronID), + "schedule": "5 5 5 5 5", + "suffix": "abcde", + "suspend": false, + "concurrency_policy": "Allow", + }) + + forEachManifestMetadata(t, manifest, func(metadata map[string]interface{}) { + labels, _ := metadata["labels"].(map[string]interface{}) + for key, value := range labels { + str, ok := value.(string) + if !ok { + t.Errorf("label %q has non-string value %v (type %T)", key, value, value) + continue + } + if len(str) > 63 { + t.Errorf("label %q value %q exceeds Kubernetes' 63-byte cap (len %d)", key, str, len(str)) + } + } + }) +} + +// renderCronJobTemplate renders templates/chart/cron-job.yaml with the +// supplied cron-process values and returns the YAML manifest as a string. +// The chart is materialised in a tempdir alongside the shared _helpers.tpl +// so the helm engine resolves named templates the same way it does in +// production. +func renderCronJobTemplate(t *testing.T, cronValues map[string]interface{}) string { + t.Helper() + chartDir := t.TempDir() if err := os.MkdirAll(filepath.Join(chartDir, "templates"), 0o755); err != nil { t.Fatalf("mkdir: %v", err) @@ -82,16 +198,9 @@ func TestCronJobTemplateQuotesAllDigitSuffix(t *testing.T) { }, }, "processes": map[string]interface{}{ - "cron-id-123": map[string]interface{}{ + "cron-process": map[string]interface{}{ "args": []interface{}{"echo", "hello"}, - "cron": map[string]interface{}{ - "id": "1234567890", - "hash": "abc123def", - "schedule": "5 5 5 5 5", - "suffix": "1234567890", - "suspend": false, - "concurrency_policy": "Allow", - }, + "cron": cronValues, }, }, } @@ -106,44 +215,49 @@ func TestCronJobTemplateQuotesAllDigitSuffix(t *testing.T) { t.Fatalf("render: %v", err) } - var manifest string for name, content := range rendered { if filepath.Base(name) == "cron-job.yaml" { - manifest = content - break + return content } } - if manifest == "" { - t.Fatalf("cron-job.yaml not rendered; got: %v", rendered) - } + t.Fatalf("cron-job.yaml not rendered; got: %v", rendered) + return "" +} + +// forEachManifestMetadata invokes fn on every metadata block in a multi-doc +// YAML manifest, including the nested jobTemplate and pod template metadata +// blocks inside a CronJob spec. +func forEachManifestMetadata(t *testing.T, manifest string, fn func(metadata map[string]interface{})) { + t.Helper() decoder := yaml.NewDecoder(strings.NewReader(manifest)) for { var doc map[string]interface{} if err := decoder.Decode(&doc); err != nil { if errors.Is(err, io.EOF) { - break + return } t.Fatalf("yaml decode failed (would also fail in Kubernetes API): %v\nrendered:\n%s", err, manifest) } if doc == nil { continue } - metadata, ok := doc["metadata"].(map[string]interface{}) - if !ok { - continue + walkMetadata(doc, fn) + } +} + +func walkMetadata(node interface{}, fn func(metadata map[string]interface{})) { + switch typed := node.(type) { + case map[string]interface{}: + if metadata, ok := typed["metadata"].(map[string]interface{}); ok { + fn(metadata) } - annotations, _ := metadata["annotations"].(map[string]interface{}) - for key, value := range annotations { - if _, isString := value.(string); !isString { - t.Errorf("annotation %q has non-string value %v (type %T); helm template must apply | quote", key, value, value) - } + for _, value := range typed { + walkMetadata(value, fn) } - labels, _ := metadata["labels"].(map[string]interface{}) - for key, value := range labels { - if _, isString := value.(string); !isString { - t.Errorf("label %q has non-string value %v (type %T); helm template must apply | quote", key, value, value) - } + case []interface{}: + for _, value := range typed { + walkMetadata(value, fn) } } } diff --git a/plugins/scheduler-k3s/functions.go b/plugins/scheduler-k3s/functions.go index a183efe7b..3bb27b193 100644 --- a/plugins/scheduler-k3s/functions.go +++ b/plugins/scheduler-k3s/functions.go @@ -3,8 +3,9 @@ package scheduler_k3s import ( "bytes" "context" - "crypto/sha256" + "crypto/sha1" "encoding/base64" + "encoding/hex" "errors" "fmt" "net" @@ -24,7 +25,6 @@ import ( nginxvhosts "github.com/dokku/dokku/plugins/nginx-vhosts" resty "github.com/go-resty/resty/v2" kedav1alpha1 "github.com/kedacore/keda/v2/apis/keda/v1alpha1" - "github.com/multiformats/go-base36" "golang.org/x/sync/errgroup" "gopkg.in/yaml.v3" "helm.sh/helm/v3/pkg/strvals" @@ -2083,12 +2083,14 @@ func kubernetesNodeToNode(node v1.Node) Node { } } -// cronIDLabelValue returns a Kubernetes-label-safe hash of a cron ID. -// The raw cron ID can exceed the 63-byte label cap, so we keep it as an -// annotation and use this short hash for selectors. +// cronIDLabelValue returns the sha1 hex digest of the supplied cron ID, +// used as the dokku.com/cron-hash label/annotation value on cron resources. +// The raw cron ID exceeds Kubernetes' 63-byte label cap for non-trivial +// commands, so we hash it to a fixed 40-character string and keep the +// original in the dokku.com/cron-id annotation for display. func cronIDLabelValue(cronID string) string { - sum := sha256.Sum256([]byte(cronID)) - return base36.EncodeToStringLc(sum[:16]) + sum := sha1.Sum([]byte(cronID)) + return hex.EncodeToString(sum[:]) } // parseMemoryQuantity parses a string into a valid memory quantity diff --git a/plugins/scheduler-k3s/template.go b/plugins/scheduler-k3s/template.go index 554bde5f4..e3a2dcbe8 100644 --- a/plugins/scheduler-k3s/template.go +++ b/plugins/scheduler-k3s/template.go @@ -356,6 +356,7 @@ type ClusterIssuer struct { } type Job struct { + Annotations map[string]string AppName string Command []string DeploymentID int64 @@ -429,6 +430,7 @@ func templateKubernetesJob(input Job) (batchv1.Job, error) { "dokku.com/builder-type": input.ImageSourceType, "dokku.com/managed": "true", } + maps.Copy(annotations, input.Annotations) maps.Copy(labels, input.Labels) secretName := GetConfigSecretName(input.AppName) diff --git a/plugins/scheduler-k3s/templates/chart/cron-job.yaml b/plugins/scheduler-k3s/templates/chart/cron-job.yaml index 3d4ee9b79..371ddb34c 100644 --- a/plugins/scheduler-k3s/templates/chart/cron-job.yaml +++ b/plugins/scheduler-k3s/templates/chart/cron-job.yaml @@ -10,6 +10,7 @@ metadata: annotations: app.kubernetes.io/version: {{ $.Values.global.deployment_id | quote }} dokku.com/builder-type: {{ $.Values.global.image.type | quote }} + dokku.com/cron-hash: {{ $config.cron.hash | quote }} dokku.com/cron-id: {{ $config.cron.id | quote }} dokku.com/job-suffix: {{ $config.cron.suffix | quote }} dokku.com/managed: "true" @@ -33,6 +34,7 @@ spec: annotations: app.kubernetes.io/version: {{ $.Values.global.deployment_id | quote }} dokku.com/builder-type: {{ $.Values.global.image.type | quote }} + dokku.com/cron-hash: {{ $config.cron.hash | quote }} dokku.com/cron-id: {{ $config.cron.id | quote }} dokku.com/job-suffix: {{ $config.cron.suffix | quote }} dokku.com/managed: "true" @@ -56,6 +58,7 @@ spec: annotations: app.kubernetes.io/version: {{ $.Values.global.deployment_id | quote }} dokku.com/builder-type: {{ $.Values.global.image.type | quote }} + dokku.com/cron-hash: {{ $config.cron.hash | quote }} dokku.com/cron-id: {{ $config.cron.id | quote }} dokku.com/job-suffix: {{ $config.cron.suffix | quote }} dokku.com/managed: "true" diff --git a/plugins/scheduler-k3s/triggers.go b/plugins/scheduler-k3s/triggers.go index cf21f33f2..3201afeff 100644 --- a/plugins/scheduler-k3s/triggers.go +++ b/plugins/scheduler-k3s/triggers.go @@ -795,10 +795,8 @@ func TriggerSchedulerDeploy(scheduler string, appName string, imageTag string) e suffix := "" for _, cronJob := range cronJobs { if cronJob.Annotations["dokku.com/cron-id"] == cronTask.ID { - var ok bool - suffix, ok = cronJob.Annotations["dokku.com/job-suffix"] - if !ok { - suffix = "" + if value, ok := cronJob.Annotations["dokku.com/job-suffix"]; ok { + suffix = value } } } @@ -1355,6 +1353,7 @@ func TriggerSchedulerRun(scheduler string, appName string, envCount int, args [] labels := map[string]string{ "app.kubernetes.io/part-of": appName, } + annotations := map[string]string{} if os.Getenv("DOKKU_TRACE") == "1" { extraEnv["TRACE"] = "true" @@ -1373,8 +1372,11 @@ func TriggerSchedulerRun(scheduler string, appName string, envCount int, args [] processType := "run" if os.Getenv("DOKKU_CRON_ID") != "" { processType = "cron" - cronHash := cronIDLabelValue(os.Getenv("DOKKU_CRON_ID")) + cronID := os.Getenv("DOKKU_CRON_ID") + cronHash := cronIDLabelValue(cronID) labels["dokku.com/cron-hash"] = cronHash + annotations["dokku.com/cron-hash"] = cronHash + annotations["dokku.com/cron-id"] = cronID concurrencyPolicy := strings.ToUpper(os.Getenv("DOKKU_CONCURRENCY_POLICY")) switch concurrencyPolicy { case "forbid": @@ -1498,6 +1500,8 @@ func TriggerSchedulerRun(scheduler string, appName string, envCount int, args [] workingDir := common.GetWorkingDir(appName, image) job, err := templateKubernetesJob(Job{ + ActiveDeadlineSeconds: activeDeadlineSeconds, + Annotations: annotations, AppName: appName, Command: command, DeploymentID: deploymentID, @@ -1513,7 +1517,6 @@ func TriggerSchedulerRun(scheduler string, appName string, envCount int, args [] RemoveContainer: rmContainer, SecurityContext: securityContext, WorkingDir: workingDir, - ActiveDeadlineSeconds: activeDeadlineSeconds, }) if err != nil { return fmt.Errorf("Error templating job: %w", err) @@ -1754,7 +1757,7 @@ func TriggerSchedulerRunList(scheduler string, appName string, format string) er cronID, ok := cronJob.Annotations["dokku.com/cron-id"] if !ok { - common.LogWarn(fmt.Sprintf("Cron job %s does not have a cron ID annotation", cronJob.Name)) + common.LogWarn(fmt.Sprintf("Cron job %s does not have a dokku.com/cron-id annotation", cronJob.Name)) continue } diff --git a/tests/apps/python/app-cnb-cron.json b/tests/apps/python/app-cnb-cron.json index 67943099b..54d9aa7a1 100644 --- a/tests/apps/python/app-cnb-cron.json +++ b/tests/apps/python/app-cnb-cron.json @@ -1,7 +1,7 @@ { "cron": [ { - "command": "python3 task.py", + "command": "python3 task.py some cron task", "schedule": "5 5 5 5 5" } ], diff --git a/tests/unit/builder-pack.bats b/tests/unit/builder-pack.bats index 2e8d5876c..403989785 100644 --- a/tests/unit/builder-pack.bats +++ b/tests/unit/builder-pack.bats @@ -265,7 +265,7 @@ teardown() { echo "output: $output" echo "status: $status" assert_success - assert_output "['task.py']" + assert_output "['task.py', 'some', 'cron', 'task']" } @test "(builder-pack) cron:run with Procfile reference" { diff --git a/tests/unit/scheduler-k3s-cnb.bats b/tests/unit/scheduler-k3s-cnb.bats index 4d54065f6..3c2bd8db5 100644 --- a/tests/unit/scheduler-k3s-cnb.bats +++ b/tests/unit/scheduler-k3s-cnb.bats @@ -103,11 +103,31 @@ teardown() { assert_success assert_output_exists + cron_hash="$(printf '%s' "$cron_id" | sha1sum | awk '{print $1}')" + + run /bin/bash -c "kubectl get cronjob -o json | jq -r '.items[0].metadata.annotations.\"dokku.com/cron-id\"'" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "$cron_id" + + run /bin/bash -c "kubectl get cronjob -o json | jq -r '.items[0].metadata.labels.\"dokku.com/cron-hash\"'" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "$cron_hash" + + run /bin/bash -c "kubectl get cronjob -o json | jq -r '.items[0].metadata.annotations.\"dokku.com/cron-hash\"'" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "$cron_hash" + run /bin/bash -c "dokku --quiet cron:run $TEST_APP $cron_id" echo "output: $output" echo "status: $status" assert_success - assert_output_contains "['task.py']" + assert_output_contains "['task.py', 'some', 'cron', 'task']" } @test "(scheduler-k3s) cnb dokku run uses launcher entrypoint" { diff --git a/tests/unit/scheduler-k3s-dockerfile.bats b/tests/unit/scheduler-k3s-dockerfile.bats index 6be1e49b0..83636ce6f 100644 --- a/tests/unit/scheduler-k3s-dockerfile.bats +++ b/tests/unit/scheduler-k3s-dockerfile.bats @@ -94,6 +94,26 @@ teardown() { assert_success assert_output_exists + cron_hash="$(printf '%s' "$cron_id" | sha1sum | awk '{print $1}')" + + run /bin/bash -c "kubectl get cronjob -o json | jq -r '.items[0].metadata.annotations.\"dokku.com/cron-id\"'" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "$cron_id" + + run /bin/bash -c "kubectl get cronjob -o json | jq -r '.items[0].metadata.labels.\"dokku.com/cron-hash\"'" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "$cron_hash" + + run /bin/bash -c "kubectl get cronjob -o json | jq -r '.items[0].metadata.annotations.\"dokku.com/cron-hash\"'" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "$cron_hash" + run /bin/bash -c "dokku --quiet cron:run $TEST_APP $cron_id" echo "output: $output" echo "status: $status" diff --git a/tests/unit/scheduler-k3s-herokuish.bats b/tests/unit/scheduler-k3s-herokuish.bats index 3f3786b41..ad8e4c238 100644 --- a/tests/unit/scheduler-k3s-herokuish.bats +++ b/tests/unit/scheduler-k3s-herokuish.bats @@ -99,6 +99,26 @@ teardown() { assert_success assert_output_exists + cron_hash="$(printf '%s' "$cron_id" | sha1sum | awk '{print $1}')" + + run /bin/bash -c "kubectl get cronjob -o json | jq -r '.items[0].metadata.annotations.\"dokku.com/cron-id\"'" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "$cron_id" + + run /bin/bash -c "kubectl get cronjob -o json | jq -r '.items[0].metadata.labels.\"dokku.com/cron-hash\"'" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "$cron_hash" + + run /bin/bash -c "kubectl get cronjob -o json | jq -r '.items[0].metadata.annotations.\"dokku.com/cron-hash\"'" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "$cron_hash" + run /bin/bash -c "dokku --quiet cron:run $TEST_APP $cron_id" echo "output: $output" echo "status: $status"