From f1f8b2be0037151ec75d64089e324eabd0b1e21f Mon Sep 17 00:00:00 2001 From: Robert Kaussow Date: Thu, 16 Jul 2026 15:21:01 +0200 Subject: [PATCH] refactor: rework k8s utils to handle resource names and labels (#6855) --- pipeline/backend/kubernetes/pod.go | 4 +- pipeline/backend/kubernetes/pod_test.go | 15 +- pipeline/backend/kubernetes/secrets.go | 2 +- pipeline/backend/kubernetes/service.go | 4 +- pipeline/backend/kubernetes/service_test.go | 16 +- pipeline/backend/kubernetes/utils.go | 64 +++- pipeline/backend/kubernetes/utils_test.go | 383 +++++++++++++++++--- pipeline/backend/kubernetes/volume.go | 2 +- pipeline/backend/kubernetes/volume_test.go | 15 +- 9 files changed, 416 insertions(+), 89 deletions(-) diff --git a/pipeline/backend/kubernetes/pod.go b/pipeline/backend/kubernetes/pod.go index 027878c45..936e308b7 100644 --- a/pipeline/backend/kubernetes/pod.go +++ b/pipeline/backend/kubernetes/pod.go @@ -89,7 +89,7 @@ func stepToPodName(step *types.Step) (name string, err error) { } func podName(step *types.Step) (string, error) { - return dnsName(podPrefix + step.UUID) + return toDNSName(podPrefix + step.UUID) } func podMeta(step *types.Step, config *config, options BackendOptions, podName, taskUUID string) (kube_meta_v1.ObjectMeta, error) { @@ -116,7 +116,7 @@ func podLabels(step *types.Step, config *config, options BackendOptions, taskUUI // Only copy user labels if allowed by agent config. // Internal labels are filtered on the server-side. if config.PodLabelsAllowFromStep || strings.HasPrefix(k, pipeline.InternalLabelPrefix) { - labels[k], err = toDNSName(v) + labels[k], err = toLabelValue(v) if err != nil { return labels, err } diff --git a/pipeline/backend/kubernetes/pod_test.go b/pipeline/backend/kubernetes/pod_test.go index 99d38368b..9fbf36782 100644 --- a/pipeline/backend/kubernetes/pod_test.go +++ b/pipeline/backend/kubernetes/pod_test.go @@ -33,11 +33,13 @@ func TestPodName(t *testing.T) { assert.NoError(t, err) assert.Equal(t, "wp-01he8bebctabr3kgk0qj36d2me-0", name) - _, err = podName(&types.Step{UUID: "01he8bebctabr3kgk0qj36d2me\\0a"}) - assert.ErrorIs(t, err, ErrDNSPatternInvalid) + name, err = podName(&types.Step{UUID: "01he8bebctabr3kgk0qj36d2me\\0a"}) + assert.NoError(t, err) + assert.Equal(t, "wp-01he8bebctabr3kgk0qj36d2me-0a", name) - _, err = podName(&types.Step{UUID: "01he8bebctabr3kgk0qj36d2me-0-services-0..woodpecker-runtime.svc.cluster.local"}) - assert.ErrorIs(t, err, ErrDNSPatternInvalid) + name, err = podName(&types.Step{UUID: "01he8bebctabr3kgk0qj36d2me-0-services-0..woodpecker-runtime.svc.cluster.local"}) + assert.NoError(t, err) + assert.Equal(t, "wp-01he8bebctabr3kgk0qj36d2me-0-services-0.woodpecker-runtime.svc.cluster.local", name) } func TestStepToPodName(t *testing.T) { @@ -118,8 +120,9 @@ func TestStepLabel(t *testing.T) { assert.NoError(t, err) assert.EqualValues(t, "build-image", name) - _, err = stepLabel(&types.Step{Name: ".build.image"}) - assert.ErrorIs(t, err, ErrDNSPatternInvalid) + name, err = stepLabel(&types.Step{Name: ".build.image"}) + assert.NoError(t, err) + assert.EqualValues(t, "build.image", name) } func TestPodHostnameSanitized(t *testing.T) { diff --git a/pipeline/backend/kubernetes/secrets.go b/pipeline/backend/kubernetes/secrets.go index 474cfb498..9fb9bc566 100644 --- a/pipeline/backend/kubernetes/secrets.go +++ b/pipeline/backend/kubernetes/secrets.go @@ -260,7 +260,7 @@ func registrySecretLabels(step *types.Step, config *config) (map[string]string, // Only copy user labels if allowed by agent config. // Internal labels are filtered on the server-side. if config.PodLabelsAllowFromStep || strings.HasPrefix(k, pipeline.InternalLabelPrefix) { - labels[k], err = toDNSName(v) + labels[k], err = toLabelValue(v) if err != nil { return labels, err } diff --git a/pipeline/backend/kubernetes/service.go b/pipeline/backend/kubernetes/service.go index 8793d92d0..8220ce212 100644 --- a/pipeline/backend/kubernetes/service.go +++ b/pipeline/backend/kubernetes/service.go @@ -56,7 +56,7 @@ func mkHeadlessService(namespace, taskUUID string) (*kube_core_v1.Service, error } func serviceName(step *types.Step) (string, error) { - return dnsName(ServicePrefix + step.UUID + "-" + step.Name) + return toDNSName(ServicePrefix + step.UUID + "-" + step.Name) } func isService(step *types.Step) bool { @@ -64,7 +64,7 @@ func isService(step *types.Step) bool { } func subdomain(taskUUID string) (string, error) { - return dnsName(HeadlessServicePrefix + taskUUID) + return toDNSName(HeadlessServicePrefix + taskUUID) } func startHeadlessService(ctx context.Context, engine *kube, namespace, taskUUID string) (*kube_core_v1.Service, error) { diff --git a/pipeline/backend/kubernetes/service_test.go b/pipeline/backend/kubernetes/service_test.go index eb854cd77..cf7461745 100644 --- a/pipeline/backend/kubernetes/service_test.go +++ b/pipeline/backend/kubernetes/service_test.go @@ -67,8 +67,9 @@ func TestHeadlessService(t *testing.T) { } func TestInvalidHeadlessService(t *testing.T) { - _, err := mkHeadlessService("foo", "invalid_task_uuid!") - assert.Error(t, err, "expected error due to invalid task UUID") + svc, err := mkHeadlessService("foo", "invalid_task_uuid!") + assert.NoError(t, err, "invalid characters are sanitized") + assert.Equal(t, "wp-hsvc-invalid-task-uuid", svc.Name) } func TestStartHeadlessService(t *testing.T) { @@ -93,14 +94,15 @@ func TestStartHeadlessService(t *testing.T) { assert.Equal(t, svc.Name, createdSvc.Name, "expected created service name to match") }) - t.Run("error on invalid task UUID resulting in invalid domain-name", func(t *testing.T) { + t.Run("sanitizes invalid task UUID", func(t *testing.T) { engine := &kube{ client: fake.NewClientset(), config: &config{Namespace: "test-namespace"}, } - _, err := startHeadlessService(t.Context(), engine, "test-namespace", "invalid_task_uuid!") - assert.Error(t, err, "expected error due to invalid task UUID") + svc, err := startHeadlessService(t.Context(), engine, "test-namespace", "invalid_task_uuid!") + assert.NoError(t, err, "invalid characters are sanitized") + assert.Equal(t, "wp-hsvc-invalid-task-uuid", svc.Name) }) } @@ -138,13 +140,13 @@ func TestStopHeadlessService(t *testing.T) { assert.NoError(t, err, "expected no error when deleting a non-existent service") }) - t.Run("error on invalid task UUID resulting in invalid domain-name", func(t *testing.T) { + t.Run("sanitizes invalid task UUID", func(t *testing.T) { engine := &kube{ client: fake.NewClientset(), config: &config{Namespace: "test-namespace"}, } err := engine.stopHeadlessService(t.Context(), engine, "test-namespace", "invalid_task_uuid!") - assert.Error(t, err, "expected error due to invalid task UUID") + assert.NoError(t, err, "invalid characters are sanitized and missing service is tolerated") }) } diff --git a/pipeline/backend/kubernetes/utils.go b/pipeline/backend/kubernetes/utils.go index e09d0a873..6917e4da4 100644 --- a/pipeline/backend/kubernetes/utils.go +++ b/pipeline/backend/kubernetes/utils.go @@ -15,12 +15,15 @@ package kubernetes import ( + "crypto/sha256" + "encoding/hex" "errors" "os" "regexp" "strings" kube_core_v1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/util/validation" "k8s.io/client-go/kubernetes" "k8s.io/client-go/rest" kube_client_cmd "k8s.io/client-go/tools/clientcmd" @@ -29,14 +32,12 @@ import ( const maxDNSLabelLen = 63 var ( - dnsPattern = regexp.MustCompile( - `^[a-z0-9]` + // must start with - `([-a-z0-9]*[a-z0-9])?` + // inside can als contain - - `(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$`, // allow the same pattern as before with dots in between but only one dot - ) - dnsLabelPattern = regexp.MustCompile(`^[a-z0-9]([-a-z0-9]*[a-z0-9])?$`) - dnsDisallowedCharacters = regexp.MustCompile(`[^-^.a-z0-9]+`) + dnsDisallowedCharacters = regexp.MustCompile(`[^-.a-z0-9]+`) + dotsAndDashes = regexp.MustCompile(`[.-]{2,}`) + labelDisallowedChars = regexp.MustCompile(`[^-_.a-z0-9]+`) + labelSeparators = regexp.MustCompile(`[-_.]{2,}`) ErrDNSPatternInvalid = errors.New("name is not a valid kubernetes DNS name") + ErrLabelInvalid = errors.New("value is not a valid kubernetes label value") ) func getHostnameOrEmpty(name string) string { @@ -52,28 +53,57 @@ func getHostnameOrEmpty(name string) string { clean = strings.Trim(clean, "-") - if dnsLabelPattern.MatchString(clean) { + if len(validation.IsDNS1123Label(clean)) == 0 { return clean } return "" } -func dnsName(i string) (string, error) { - res := strings.ToLower(strings.ReplaceAll(i, "_", "-")) +func toDNSName(in string) (string, error) { + res := strings.ToLower(in) + res = dnsDisallowedCharacters.ReplaceAllString(res, "-") + res = dotsAndDashes.ReplaceAllStringFunc(res, func(s string) string { + if strings.ContainsRune(s, '.') { + return "." + } + return "-" + }) + res = strings.Trim(res, "-.") - if found := dnsPattern.FindStringIndex(res); found == nil { + if len(res) > validation.DNS1123SubdomainMaxLength { + res = truncateWithHash(res, in, validation.DNS1123SubdomainMaxLength, "-.") + } + + if res == "" || len(validation.IsDNS1123Subdomain(res)) > 0 { return "", ErrDNSPatternInvalid } return res, nil } -func toDNSName(in string) (string, error) { - lower := strings.ToLower(in) - withoutUnderscores := strings.ReplaceAll(lower, "_", "-") - withoutSpaces := strings.ReplaceAll(withoutUnderscores, " ", "-") - almostDNS := dnsDisallowedCharacters.ReplaceAllString(withoutSpaces, "") - return dnsName(almostDNS) +func truncateWithHash(s, original string, maxLen int, trimChars string) string { + hash := sha256.Sum256([]byte(original)) + hashStr := hex.EncodeToString(hash[:])[:16] + maxBaseLength := maxLen - 1 - len(hashStr) + truncated := strings.TrimRight(s[:maxBaseLength], trimChars) + return truncated + "-" + hashStr +} + +func toLabelValue(in string) (string, error) { + res := strings.ToLower(in) + res = labelDisallowedChars.ReplaceAllString(res, "-") + res = labelSeparators.ReplaceAllString(res, "-") + res = strings.Trim(res, "-_.") + + if len(res) > validation.LabelValueMaxLength { + res = truncateWithHash(res, in, validation.LabelValueMaxLength, "-_.") + } + + if len(validation.IsValidLabelValue(res)) > 0 { + return "", ErrLabelInvalid + } + + return res, nil } func isImagePullBackOffState(pod *kube_core_v1.Pod) bool { diff --git a/pipeline/backend/kubernetes/utils_test.go b/pipeline/backend/kubernetes/utils_test.go index 2ffc3dda7..b3d640a77 100644 --- a/pipeline/backend/kubernetes/utils_test.go +++ b/pipeline/backend/kubernetes/utils_test.go @@ -15,75 +15,364 @@ package kubernetes import ( + "strings" "testing" "github.com/stretchr/testify/assert" + "k8s.io/apimachinery/pkg/util/validation" ) -func TestDNSName(t *testing.T) { - name, err := dnsName("wp_01he8bebctabr3kgk0qj36d2me_0_services_0") - assert.NoError(t, err) - assert.Equal(t, "wp-01he8bebctabr3kgk0qj36d2me-0-services-0", name) +func TestToDnsName(t *testing.T) { + tests := []struct { + name string + in string + want string + wantErr bool + }{ + { + name: "underscores to dashes", + in: "wp_01he8bebctabr3kgk0qj36d2me_0_services_0", + want: "wp-01he8bebctabr3kgk0qj36d2me-0-services-0", + }, + { + name: "mixed case with dots and dashes", + in: "a.0-AA", + want: "a.0-aa", + }, + { + name: "long valid fqdn unchanged", + in: "wp-01he8bebctabr3kgk0qj36d2me-0-services-0.woodpecker-runtime.svc.cluster.local", + want: "wp-01he8bebctabr3kgk0qj36d2me-0-services-0.woodpecker-runtime.svc.cluster.local", + }, + { + name: "uppercase with underscores", + in: "BUILD_AND_DEPLOY_0", + want: "build-and-deploy-0", + }, + { + name: "spaces to dashes", + in: "build and deploy", + want: "build-and-deploy", + }, + { + name: "special char ampersand", + in: "build & deploy", + want: "build-deploy", + }, + { + name: "backslash to dash", + in: "abc\\def", + want: "abc-def", + }, + { + name: "leading dash trimmed", + in: "-build-and-deploy", + want: "build-and-deploy", + }, + { + name: "trailing dash trimmed", + in: "test-", + want: "test", + }, + { + name: "leading dot trimmed", + in: ".0-a", + want: "0-a", + }, + { + name: "consecutive dots collapsed", + in: "ABC..DEF", + want: "abc.def", + }, + { + name: "dot-dash collapsed", + in: "0.-a", + want: "0.a", + }, + { + name: "trailing dot trimmed", + in: "0-a.", + want: "0-a", + }, + { + name: "consecutive underscores single dash", + in: "a__b", + want: "a-b", + }, + { + name: "mixed dots and dashes", + in: "a.-.-b", + want: "a.b", + }, + { + name: "unicode emoji replaced", + in: "hello🚀world", + want: "hello-world", + }, + { + name: "single valid char", + in: "a", + want: "a", + }, + { + name: "numbers only", + in: "123", + want: "123", + }, + { + name: "mixed leading trailing", + in: "-.-test-.-", + want: "test", + }, + { + name: "all special chars empty", + in: "!!!", + wantErr: true, + }, + { + name: "all dots empty", + in: "...", + wantErr: true, + }, + { + name: "all dashes empty", + in: "---", + wantErr: true, + }, + { + name: "empty string", + in: "", + wantErr: true, + }, + { + name: "truncation needed", + in: strings.Repeat("a", 300), + }, + { + name: "truncation boundary exact 253", + in: strings.Repeat("a", 253), + want: strings.Repeat("a", 253), + }, + { + name: "truncation with trailing dash at cut", + in: strings.Repeat("a", 240) + "----" + strings.Repeat("b", 60), + }, + { + name: "truncation at dot boundary", + in: strings.Repeat("a.", 130), + }, + { + name: "truncation with long label segment", + in: strings.Repeat("a", 70) + "." + strings.Repeat("b", 70) + "." + strings.Repeat("c", 70), + }, + } - name, err = dnsName("a.0-AA") - assert.NoError(t, err) - assert.Equal(t, "a.0-aa", name) - - name, err = dnsName("wp-01he8bebctabr3kgk0qj36d2me-0-services-0.woodpecker-runtime.svc.cluster.local") - assert.NoError(t, err) - assert.Equal(t, "wp-01he8bebctabr3kgk0qj36d2me-0-services-0.woodpecker-runtime.svc.cluster.local", name) - - _, err = dnsName(".0-a") - assert.ErrorIs(t, err, ErrDNSPatternInvalid) - - _, err = dnsName("ABC..DEF") - assert.ErrorIs(t, err, ErrDNSPatternInvalid) - - _, err = dnsName("0.-a") - assert.ErrorIs(t, err, ErrDNSPatternInvalid) - - _, err = dnsName("test-") - assert.ErrorIs(t, err, ErrDNSPatternInvalid) - - _, err = dnsName("-test") - assert.ErrorIs(t, err, ErrDNSPatternInvalid) - - _, err = dnsName("0-a.") - assert.ErrorIs(t, err, ErrDNSPatternInvalid) - - _, err = dnsName("abc\\def") - assert.ErrorIs(t, err, ErrDNSPatternInvalid) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := toDNSName(tt.in) + if tt.wantErr { + assert.ErrorIs(t, err, ErrDNSPatternInvalid) + return + } + assert.NoError(t, err) + if tt.name == "truncation needed" || tt.name == "truncation with trailing dash at cut" || tt.name == "truncation at dot boundary" || tt.name == "truncation with long label segment" { + assert.LessOrEqual(t, len(got), validation.DNS1123SubdomainMaxLength) + assert.Len(t, validation.IsDNS1123Subdomain(got), 0) + return + } + assert.Equal(t, tt.want, got) + }) + } } -func TestToDnsName(t *testing.T) { - name, err := toDNSName("BUILD_AND_DEPLOY_0") - assert.NoError(t, err) - assert.Equal(t, "build-and-deploy-0", name) +func TestToDnsNameTruncationUniqueness(t *testing.T) { + long1 := strings.Repeat("a", 300) + long2 := strings.Repeat("a", 299) + "b" - name, err = toDNSName("build and deploy") + got1, err := toDNSName(long1) assert.NoError(t, err) - assert.Equal(t, "build-and-deploy", name) - name, err = toDNSName("build & deploy") + got2, err := toDNSName(long2) assert.NoError(t, err) - assert.Equal(t, "build--deploy", name) - _, err = toDNSName("-build-and-deploy") - assert.ErrorIs(t, err, ErrDNSPatternInvalid) + assert.NotEqual(t, got1, got2, "different long inputs should produce different truncated names") +} + +func TestToLabelValue(t *testing.T) { + tests := []struct { + name string + in string + want string + wantErr bool + }{ + { + name: "underscores preserved", + in: "wp_01he8bebctabr3kgk0qj36d2me_0_services_0", + want: "wp_01he8bebctabr3kgk0qj36d2me_0_services_0", + }, + { + name: "dots preserved", + in: "a.0-AA", + want: "a.0-aa", + }, + { + name: "uppercase with underscores", + in: "BUILD_AND_DEPLOY_0", + want: "build_and_deploy_0", + }, + { + name: "spaces to dashes", + in: "build and deploy", + want: "build-and-deploy", + }, + { + name: "special char ampersand", + in: "build & deploy", + want: "build-deploy", + }, + { + name: "backslash to dash", + in: "abc\\def", + want: "abc-def", + }, + { + name: "leading dash trimmed", + in: "-build-and-deploy", + want: "build-and-deploy", + }, + { + name: "trailing dash trimmed", + in: "test-", + want: "test", + }, + { + name: "leading dot trimmed", + in: ".0-a", + want: "0-a", + }, + { + name: "trailing dot trimmed", + in: "0-a.", + want: "0-a", + }, + { + name: "consecutive underscores collapsed", + in: "a__b", + want: "a-b", + }, + { + name: "consecutive dots collapsed", + in: "ABC..DEF", + want: "abc-def", + }, + { + name: "mixed separators collapsed", + in: "a._-b", + want: "a-b", + }, + { + name: "unicode emoji replaced", + in: "hello🚀world", + want: "hello-world", + }, + { + name: "single valid char", + in: "a", + want: "a", + }, + { + name: "numbers only", + in: "123", + want: "123", + }, + { + name: "mixed leading trailing", + in: "-.-test-.-", + want: "test", + }, + { + name: "all special chars become empty", + in: "!!!", + want: "", + }, + { + name: "empty string is valid", + in: "", + want: "", + }, + { + name: "truncation needed", + in: strings.Repeat("a", 100), + }, + { + name: "truncation boundary exact 63", + in: strings.Repeat("a", 63), + want: strings.Repeat("a", 63), + }, + { + name: "truncation with trailing dash at cut", + in: strings.Repeat("a", 50) + "----" + strings.Repeat("b", 30), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := toLabelValue(tt.in) + if tt.wantErr { + assert.ErrorIs(t, err, ErrLabelInvalid) + return + } + assert.NoError(t, err) + if tt.name == "truncation needed" || tt.name == "truncation with trailing dash at cut" { + assert.LessOrEqual(t, len(got), validation.LabelValueMaxLength) + assert.Len(t, validation.IsValidLabelValue(got), 0) + return + } + assert.Equal(t, tt.want, got) + }) + } +} + +func TestToLabelValueTruncationUniqueness(t *testing.T) { + long1 := strings.Repeat("a", 100) + long2 := strings.Repeat("a", 99) + "b" + + got1, err := toLabelValue(long1) + assert.NoError(t, err) + + got2, err := toLabelValue(long2) + assert.NoError(t, err) + + assert.NotEqual(t, got1, got2, "different long inputs should produce different truncated labels") } func TestGetHostnameOrEmpty(t *testing.T) { tests := []struct { + name string in string want string }{ - {"Update repos", "update-repos"}, - {"MY_STEP", "my-step"}, - {"Build 🚀", ""}, + { + name: "spaces to dashes", + in: "Update repos", + want: "update-repos", + }, + { + name: "underscores to dashes", + in: "MY_STEP", + want: "my-step", + }, + { + name: "emoji removed", + in: "Build 🚀", + want: "build", + }, } for _, tt := range tests { - got := getHostnameOrEmpty(tt.in) - assert.Equal(t, tt.want, got, "input: %q", tt.in) + t.Run(tt.name, func(t *testing.T) { + got := getHostnameOrEmpty(tt.in) + assert.Equal(t, tt.want, got) + }) } } diff --git a/pipeline/backend/kubernetes/volume.go b/pipeline/backend/kubernetes/volume.go index 2760e51c3..a368a207c 100644 --- a/pipeline/backend/kubernetes/volume.go +++ b/pipeline/backend/kubernetes/volume.go @@ -64,7 +64,7 @@ func mkPersistentVolumeClaim(config *config, name, namespace string) (*kube_core } func volumeName(name string) (string, error) { - return dnsName(strings.Split(name, ":")[0]) + return toDNSName(strings.Split(name, ":")[0]) } func volumeMountPath(name string) string { diff --git a/pipeline/backend/kubernetes/volume_test.go b/pipeline/backend/kubernetes/volume_test.go index 7080d4f53..30997c7c1 100644 --- a/pipeline/backend/kubernetes/volume_test.go +++ b/pipeline/backend/kubernetes/volume_test.go @@ -26,11 +26,13 @@ func TestPvcName(t *testing.T) { assert.NoError(t, err) assert.Equal(t, "woodpecker-cache", name) - _, err = volumeName("woodpecker\\cache") - assert.ErrorIs(t, err, ErrDNSPatternInvalid) + name, err = volumeName("woodpecker\\cache") + assert.NoError(t, err) + assert.Equal(t, "woodpecker-cache", name) - _, err = volumeName("-woodpecker.cache:/woodpecker/src/cache") - assert.ErrorIs(t, err, ErrDNSPatternInvalid) + name, err = volumeName("-woodpecker.cache:/woodpecker/src/cache") + assert.NoError(t, err) + assert.Equal(t, "woodpecker.cache", name) } func TestPvcMount(t *testing.T) { @@ -107,11 +109,12 @@ func TestPersistentVolumeClaim(t *testing.T) { assert.NoError(t, err) assert.JSONEq(t, expectedRwo, string(j)) - _, err = mkPersistentVolumeClaim(&config{ + pvc, err = mkPersistentVolumeClaim(&config{ Namespace: namespace, StorageClass: "local-storage", VolumeSize: "1Gi", StorageRwx: false, }, "some0..INVALID3name", namespace) - assert.Error(t, err) + assert.NoError(t, err) + assert.Equal(t, "some0.invalid3name", pvc.Name) }