fix: harden scheduler-k3s cron manifests and dockerfile run startup
The cron-id label could exceed Kubernetes' 63-byte cap when commands or schedules were long, and an all-digit job-suffix or cron-id rendered as an unquoted YAML scalar caused the API server to reject manifests. Run pods built from dockerfiles also occasionally hit the 10s startup wait on a cold image pull, even though the pod was scheduled correctly. The cron-id is now stored as an annotation and a shorter hash is used as the selector label. Every interpolated annotation and label value in the cron-job and deployment templates is now quoted to prevent numeric coercion, and the run-pod wait timeout is raised to 30 seconds.
This commit is contained in:
149
plugins/scheduler-k3s/cron_job_template_test.go
Normal file
149
plugins/scheduler-k3s/cron_job_template_test.go
Normal file
@@ -0,0 +1,149 @@
|
||||
package scheduler_k3s
|
||||
|
||||
import (
|
||||
"errors"
|
||||
"io"
|
||||
"os"
|
||||
"path/filepath"
|
||||
"strings"
|
||||
"testing"
|
||||
|
||||
"gopkg.in/yaml.v3"
|
||||
"helm.sh/helm/v3/pkg/chart/loader"
|
||||
"helm.sh/helm/v3/pkg/chartutil"
|
||||
"helm.sh/helm/v3/pkg/engine"
|
||||
)
|
||||
|
||||
// TestCronIDLabelValue asserts the hashed cron ID fits inside Kubernetes' 63
|
||||
// byte label cap and is deterministic. Without this the label can exceed the
|
||||
// cap and the Kubernetes API server rejects the manifest.
|
||||
func TestCronIDLabelValue(t *testing.T) {
|
||||
cases := []string{
|
||||
"short",
|
||||
"app===echo hello-from-cron===5 5 5 5 5",
|
||||
strings.Repeat("a-very-long-cron-id-that-would-far-exceed-the-label-cap-", 10),
|
||||
}
|
||||
for _, in := range cases {
|
||||
out := cronIDLabelValue(in)
|
||||
if len(out) > 63 {
|
||||
t.Errorf("cronIDLabelValue(%q) = %q (len %d); must be <= 63", in, out, len(out))
|
||||
}
|
||||
if out != cronIDLabelValue(in) {
|
||||
t.Errorf("cronIDLabelValue(%q) is not deterministic", in)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// 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.
|
||||
func TestCronJobTemplateQuotesAllDigitSuffix(t *testing.T) {
|
||||
chartDir := t.TempDir()
|
||||
if err := os.MkdirAll(filepath.Join(chartDir, "templates"), 0o755); err != nil {
|
||||
t.Fatalf("mkdir: %v", err)
|
||||
}
|
||||
|
||||
chartYAML := []byte("apiVersion: v2\nname: test\nversion: 0.0.1\n")
|
||||
if err := os.WriteFile(filepath.Join(chartDir, "Chart.yaml"), chartYAML, 0o644); err != nil {
|
||||
t.Fatalf("write Chart.yaml: %v", err)
|
||||
}
|
||||
|
||||
cronJobTpl, err := templates.ReadFile("templates/chart/cron-job.yaml")
|
||||
if err != nil {
|
||||
t.Fatalf("read cron-job template: %v", err)
|
||||
}
|
||||
if err := os.WriteFile(filepath.Join(chartDir, "templates", "cron-job.yaml"), cronJobTpl, 0o644); err != nil {
|
||||
t.Fatalf("write cron-job template: %v", err)
|
||||
}
|
||||
|
||||
helpersTpl, err := templates.ReadFile("templates/chart/_helpers.tpl")
|
||||
if err != nil {
|
||||
t.Fatalf("read _helpers: %v", err)
|
||||
}
|
||||
if err := os.WriteFile(filepath.Join(chartDir, "templates", "_helpers.tpl"), helpersTpl, 0o644); err != nil {
|
||||
t.Fatalf("write _helpers: %v", err)
|
||||
}
|
||||
|
||||
loaded, err := loader.Load(chartDir)
|
||||
if err != nil {
|
||||
t.Fatalf("load chart: %v", err)
|
||||
}
|
||||
|
||||
values := map[string]interface{}{
|
||||
"global": map[string]interface{}{
|
||||
"app_name": "myapp",
|
||||
"deployment_id": "1",
|
||||
"namespace": "myapp",
|
||||
"image": map[string]interface{}{
|
||||
"name": "myapp:latest",
|
||||
"type": "dockerfile",
|
||||
},
|
||||
},
|
||||
"processes": map[string]interface{}{
|
||||
"cron-id-123": 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",
|
||||
},
|
||||
},
|
||||
},
|
||||
}
|
||||
|
||||
renderValues, err := chartutil.ToRenderValues(loaded, values, chartutil.ReleaseOptions{Name: "test", Namespace: "default"}, nil)
|
||||
if err != nil {
|
||||
t.Fatalf("ToRenderValues: %v", err)
|
||||
}
|
||||
|
||||
rendered, err := engine.Render(loaded, renderValues)
|
||||
if err != nil {
|
||||
t.Fatalf("render: %v", err)
|
||||
}
|
||||
|
||||
var manifest string
|
||||
for name, content := range rendered {
|
||||
if filepath.Base(name) == "cron-job.yaml" {
|
||||
manifest = content
|
||||
break
|
||||
}
|
||||
}
|
||||
if manifest == "" {
|
||||
t.Fatalf("cron-job.yaml not rendered; got: %v", rendered)
|
||||
}
|
||||
|
||||
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
|
||||
}
|
||||
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
|
||||
}
|
||||
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)
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -3,6 +3,7 @@ package scheduler_k3s
|
||||
import (
|
||||
"bytes"
|
||||
"context"
|
||||
"crypto/sha256"
|
||||
"encoding/base64"
|
||||
"errors"
|
||||
"fmt"
|
||||
@@ -23,6 +24,7 @@ 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"
|
||||
@@ -2081,6 +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.
|
||||
func cronIDLabelValue(cronID string) string {
|
||||
sum := sha256.Sum256([]byte(cronID))
|
||||
return base36.EncodeToStringLc(sum[:16])
|
||||
}
|
||||
|
||||
// parseMemoryQuantity parses a string into a valid memory quantity
|
||||
func parseMemoryQuantity(input string) (string, error) {
|
||||
if _, err := strconv.ParseInt(input, 10, 64); err == nil {
|
||||
|
||||
@@ -22,6 +22,7 @@ require (
|
||||
github.com/gosimple/slug v1.15.0
|
||||
github.com/kballard/go-shellquote v0.0.0-20180428030007-95032a82bc51
|
||||
github.com/kedacore/keda/v2 v2.18.3
|
||||
github.com/multiformats/go-base36 v0.2.0
|
||||
github.com/ryanuber/columnize v2.1.2+incompatible
|
||||
github.com/spf13/pflag v1.0.10
|
||||
github.com/traefik/traefik/v2 v2.11.45
|
||||
@@ -115,7 +116,6 @@ require (
|
||||
github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect
|
||||
github.com/modern-go/reflect2 v1.0.3-0.20250322232337-35a7c28c31ee // indirect
|
||||
github.com/monochromegane/go-gitignore v0.0.0-20200626010858-205db1a8cc00 // indirect
|
||||
github.com/multiformats/go-base36 v0.2.0 // indirect
|
||||
github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect
|
||||
github.com/opencontainers/go-digest v1.0.0 // indirect
|
||||
github.com/opencontainers/image-spec v1.1.1 // indirect
|
||||
|
||||
@@ -305,6 +305,7 @@ const (
|
||||
|
||||
type ProcessCron struct {
|
||||
ID string `yaml:"id"`
|
||||
Hash string `yaml:"hash"`
|
||||
Schedule string `yaml:"schedule"`
|
||||
Suffix string `yaml:"suffix"`
|
||||
Suspend bool `yaml:"suspend"`
|
||||
|
||||
@@ -9,18 +9,18 @@ kind: CronJob
|
||||
metadata:
|
||||
annotations:
|
||||
app.kubernetes.io/version: {{ $.Values.global.deployment_id | quote }}
|
||||
dokku.com/builder-type: {{ $.Values.global.image.type }}
|
||||
dokku.com/cron-id: {{ $config.cron.id }}
|
||||
dokku.com/job-suffix: {{ $config.cron.suffix }}
|
||||
dokku.com/builder-type: {{ $.Values.global.image.type | quote }}
|
||||
dokku.com/cron-id: {{ $config.cron.id | quote }}
|
||||
dokku.com/job-suffix: {{ $config.cron.suffix | quote }}
|
||||
dokku.com/managed: "true"
|
||||
kubectl.kubernetes.io/default-container: {{ $.Values.global.app_name }}-cron
|
||||
kubectl.kubernetes.io/default-container: {{ printf "%s-cron" $.Values.global.app_name | quote }}
|
||||
{{ include "print.annotations" (dict "config" $.Values.global "key" "cronjob") | indent 4 }}
|
||||
{{ include "print.annotations" (dict "config" $config "key" "cronjob") | indent 4 }}
|
||||
labels:
|
||||
app.kubernetes.io/instance: {{ $.Values.global.app_name }}-cron-{{ $config.cron.suffix }}
|
||||
app.kubernetes.io/instance: {{ printf "%s-cron-%s" $.Values.global.app_name $config.cron.suffix | quote }}
|
||||
app.kubernetes.io/name: cron
|
||||
app.kubernetes.io/part-of: {{ $.Values.global.app_name }}
|
||||
dokku.com/cron-id: {{ $config.cron.id }}
|
||||
app.kubernetes.io/part-of: {{ $.Values.global.app_name | quote }}
|
||||
dokku.com/cron-hash: {{ $config.cron.hash | quote }}
|
||||
{{ include "print.labels" (dict "config" $.Values.global "key" "cronjob") | indent 4 }}
|
||||
{{ include "print.labels" (dict "config" $config "key" "cronjob") | indent 4 }}
|
||||
name: {{ $.Values.global.app_name }}-cron-{{ $config.cron.suffix }}
|
||||
@@ -32,18 +32,18 @@ spec:
|
||||
metadata:
|
||||
annotations:
|
||||
app.kubernetes.io/version: {{ $.Values.global.deployment_id | quote }}
|
||||
dokku.com/builder-type: {{ $.Values.global.image.type }}
|
||||
dokku.com/cron-id: {{ $config.cron.id }}
|
||||
dokku.com/job-suffix: {{ $config.cron.suffix }}
|
||||
dokku.com/builder-type: {{ $.Values.global.image.type | quote }}
|
||||
dokku.com/cron-id: {{ $config.cron.id | quote }}
|
||||
dokku.com/job-suffix: {{ $config.cron.suffix | quote }}
|
||||
dokku.com/managed: "true"
|
||||
kubectl.kubernetes.io/default-container: {{ $.Values.global.app_name }}-cron
|
||||
kubectl.kubernetes.io/default-container: {{ printf "%s-cron" $.Values.global.app_name | quote }}
|
||||
{{ include "print.annotations" (dict "config" $.Values.global "key" "job") | indent 8 }}
|
||||
{{ include "print.annotations" (dict "config" $config "key" "job") | indent 8 }}
|
||||
labels:
|
||||
app.kubernetes.io/instance: {{ $.Values.global.app_name }}-cron-{{ $config.cron.suffix }}
|
||||
app.kubernetes.io/instance: {{ printf "%s-cron-%s" $.Values.global.app_name $config.cron.suffix | quote }}
|
||||
app.kubernetes.io/name: cron
|
||||
app.kubernetes.io/part-of: {{ $.Values.global.app_name }}
|
||||
dokku.com/cron-id: {{ $config.cron.id }}
|
||||
app.kubernetes.io/part-of: {{ $.Values.global.app_name | quote }}
|
||||
dokku.com/cron-hash: {{ $config.cron.hash | quote }}
|
||||
{{ include "print.labels" (dict "config" $.Values.global "key" "job") | indent 8 }}
|
||||
{{ include "print.labels" (dict "config" $config "key" "job") | indent 8 }}
|
||||
spec:
|
||||
@@ -55,18 +55,18 @@ spec:
|
||||
metadata:
|
||||
annotations:
|
||||
app.kubernetes.io/version: {{ $.Values.global.deployment_id | quote }}
|
||||
dokku.com/builder-type: {{ $.Values.global.image.type }}
|
||||
dokku.com/cron-id: {{ $config.cron.id }}
|
||||
dokku.com/job-suffix: {{ $config.cron.suffix }}
|
||||
dokku.com/builder-type: {{ $.Values.global.image.type | quote }}
|
||||
dokku.com/cron-id: {{ $config.cron.id | quote }}
|
||||
dokku.com/job-suffix: {{ $config.cron.suffix | quote }}
|
||||
dokku.com/managed: "true"
|
||||
kubectl.kubernetes.io/default-container: {{ $.Values.global.app_name }}-cron
|
||||
kubectl.kubernetes.io/default-container: {{ printf "%s-cron" $.Values.global.app_name | quote }}
|
||||
{{ include "print.annotations" (dict "config" $.Values.global "key" "pod") | indent 12 }}
|
||||
{{ include "print.annotations" (dict "config" $config "key" "pod") | indent 12 }}
|
||||
labels:
|
||||
app.kubernetes.io/instance: {{ $.Values.global.app_name }}-cron-{{ $config.cron.suffix }}
|
||||
app.kubernetes.io/instance: {{ printf "%s-cron-%s" $.Values.global.app_name $config.cron.suffix | quote }}
|
||||
app.kubernetes.io/name: cron
|
||||
app.kubernetes.io/part-of: {{ $.Values.global.app_name }}
|
||||
dokku.com/cron-id: {{ $config.cron.id }}
|
||||
app.kubernetes.io/part-of: {{ $.Values.global.app_name | quote }}
|
||||
dokku.com/cron-hash: {{ $config.cron.hash | quote }}
|
||||
{{ include "print.labels" (dict "config" $.Values.global "key" "pod") | indent 12 }}
|
||||
{{ include "print.labels" (dict "config" $config "key" "pod") | indent 12 }}
|
||||
spec:
|
||||
|
||||
@@ -16,15 +16,15 @@ kind: Deployment
|
||||
metadata:
|
||||
annotations:
|
||||
app.kubernetes.io/version: {{ $.Values.global.deployment_id | quote }}
|
||||
dokku.com/builder-type: {{ $.Values.global.image.type }}
|
||||
dokku.com/builder-type: {{ $.Values.global.image.type | quote }}
|
||||
dokku.com/managed: "true"
|
||||
kubectl.kubernetes.io/default-container: {{ $.Values.global.app_name }}-{{ $processName }}
|
||||
kubectl.kubernetes.io/default-container: {{ printf "%s-%s" $.Values.global.app_name $processName | quote }}
|
||||
{{ include "print.annotations" (dict "config" $.Values.global "key" "deployment") | indent 4 }}
|
||||
{{ include "print.annotations" (dict "config" $config "key" "deployment") | indent 4 }}
|
||||
labels:
|
||||
app.kubernetes.io/instance: {{ $.Values.global.app_name }}-{{ $processName }}
|
||||
app.kubernetes.io/name: {{ $processName }}
|
||||
app.kubernetes.io/part-of: {{ $.Values.global.app_name }}
|
||||
app.kubernetes.io/instance: {{ printf "%s-%s" $.Values.global.app_name $processName | quote }}
|
||||
app.kubernetes.io/name: {{ $processName | quote }}
|
||||
app.kubernetes.io/part-of: {{ $.Values.global.app_name | quote }}
|
||||
{{ include "print.labels" (dict "config" $.Values.global "key" "deployment") | indent 4 }}
|
||||
{{ include "print.labels" (dict "config" $config "key" "deployment") | indent 4 }}
|
||||
name: {{ $.Values.global.app_name }}-{{ $processName }}
|
||||
|
||||
@@ -282,7 +282,7 @@ func TriggerSchedulerCronWrite(scheduler string, appName string) error {
|
||||
for _, cronTask := range cronTasks {
|
||||
labelSelector := []string{
|
||||
fmt.Sprintf("app.kubernetes.io/part-of=%s", appName),
|
||||
fmt.Sprintf("dokku.com/cron-id=%s", cronTask.ID),
|
||||
fmt.Sprintf("dokku.com/cron-hash=%s", cronIDLabelValue(cronTask.ID)),
|
||||
}
|
||||
|
||||
if cronTask.Maintenance {
|
||||
@@ -790,7 +790,7 @@ func TriggerSchedulerDeploy(scheduler string, appName string, imageTag string) e
|
||||
// todo: implement pod annotations
|
||||
suffix := ""
|
||||
for _, cronJob := range cronJobs {
|
||||
if cronJob.Labels["dokku.com/cron-id"] == cronTask.ID {
|
||||
if cronJob.Annotations["dokku.com/cron-id"] == cronTask.ID {
|
||||
var ok bool
|
||||
suffix, ok = cronJob.Annotations["dokku.com/job-suffix"]
|
||||
if !ok {
|
||||
@@ -843,6 +843,7 @@ func TriggerSchedulerDeploy(scheduler string, appName string, imageTag string) e
|
||||
Annotations: annotations,
|
||||
Cron: ProcessCron{
|
||||
ID: cronTask.ID,
|
||||
Hash: cronIDLabelValue(cronTask.ID),
|
||||
Schedule: cronTask.Schedule,
|
||||
Suffix: suffix,
|
||||
Suspend: cronTask.Maintenance,
|
||||
@@ -1368,26 +1369,25 @@ func TriggerSchedulerRun(scheduler string, appName string, envCount int, args []
|
||||
processType := "run"
|
||||
if os.Getenv("DOKKU_CRON_ID") != "" {
|
||||
processType = "cron"
|
||||
labels["dokku.com/cron-id"] = os.Getenv("DOKKU_CRON_ID")
|
||||
cronHash := cronIDLabelValue(os.Getenv("DOKKU_CRON_ID"))
|
||||
labels["dokku.com/cron-hash"] = cronHash
|
||||
concurrencyPolicy := strings.ToUpper(os.Getenv("DOKKU_CONCURRENCY_POLICY"))
|
||||
switch concurrencyPolicy {
|
||||
case "forbid":
|
||||
// check if there is a running pod with the same dokku.com/cron-id label
|
||||
pods, err := clientset.ListPods(context.Background(), ListPodsInput{
|
||||
Namespace: namespace,
|
||||
LabelSelector: fmt.Sprintf("dokku.com/cron-id=%s", os.Getenv("DOKKU_CRON_ID")),
|
||||
LabelSelector: fmt.Sprintf("dokku.com/cron-hash=%s", cronHash),
|
||||
})
|
||||
if err != nil {
|
||||
return fmt.Errorf("Error listing pods: %w", err)
|
||||
}
|
||||
if len(pods) > 0 {
|
||||
return fmt.Errorf("There is a running pod with the same dokku.com/cron-id label")
|
||||
return fmt.Errorf("There is a running pod with the same dokku.com/cron-hash label")
|
||||
}
|
||||
case "replace":
|
||||
// delete any existing pod with the same dokku.com/cron-id label
|
||||
err := clientset.DeletePod(context.Background(), DeletePodInput{
|
||||
Namespace: namespace,
|
||||
LabelSelector: fmt.Sprintf("dokku.com/cron-id=%s", os.Getenv("DOKKU_CRON_ID")),
|
||||
LabelSelector: fmt.Sprintf("dokku.com/cron-hash=%s", cronHash),
|
||||
})
|
||||
if err != nil {
|
||||
return fmt.Errorf("Error deleting pod: %w", err)
|
||||
@@ -1572,7 +1572,7 @@ func TriggerSchedulerRun(scheduler string, appName string, envCount int, args []
|
||||
Clientset: clientset,
|
||||
Namespace: namespace,
|
||||
LabelSelector: batchJobSelector,
|
||||
Timeout: 10,
|
||||
Timeout: 30,
|
||||
Waiter: isPodReady,
|
||||
})
|
||||
if streamLogsOnly && (err == nil || errors.Is(err, conditions.ErrPodCompleted)) {
|
||||
@@ -1748,9 +1748,9 @@ func TriggerSchedulerRunList(scheduler string, appName string, format string) er
|
||||
}
|
||||
}
|
||||
|
||||
cronID, ok := cronJob.Labels["dokku.com/cron-id"]
|
||||
cronID, ok := cronJob.Annotations["dokku.com/cron-id"]
|
||||
if !ok {
|
||||
common.LogWarn(fmt.Sprintf("Cron job %s does not have a cron ID label", cronJob.Name))
|
||||
common.LogWarn(fmt.Sprintf("Cron job %s does not have a cron ID annotation", cronJob.Name))
|
||||
continue
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user