Use UUID as podName and cleanup arguments for Kubernetes backend (#3135)

to much args are just horrible to maintain. And we already have it nice
structured stored as step.

pipeline/backend/kubernetes/pod.go

@@ -32,27 +32,18 @@ import (
 
 const (
 	StepLabel = "step"
+	podPrefix = "wp-"
 )
 
-func mkPod(namespace, name, image, workDir, goos, serviceAccountName string,
-	pool, privileged bool,
-	commands, vols, pullSecretNames []string,
-	labels, annotations, env, nodeSelector map[string]string,
-	extraHosts []types.HostAlias, tolerations []types.Toleration, resources types.Resources,
-	securityContext *types.SecurityContext, securityContextConfig SecurityContextConfig,
-) (*v1.Pod, error) {
-	var err error
+func mkPod(step *types.Step, config *config, podName, goos string) (*v1.Pod, error) {
+	meta := podMeta(step, config, podName)
 
-	meta := podMeta(name, namespace, labels, annotations)
-
-	spec, err := podSpec(serviceAccountName, vols, pullSecretNames, env, nodeSelector, extraHosts, tolerations,
-		securityContext, securityContextConfig)
+	spec, err := podSpec(step, config)
 	if err != nil {
 		return nil, err
 	}
 
-	container, err := podContainer(name, image, workDir, goos, pool, privileged, commands, vols, env,
-		resources, securityContext)
+	container, err := podContainer(step, podName, goos)
 	if err != nil {
 		return nil, err
 	}
@@ -74,41 +65,37 @@ func stepToPodName(step *types.Step) (name string, err error) {
 }
 
 func podName(step *types.Step) (string, error) {
-	return dnsName(step.Name)
+	return dnsName(podPrefix + step.UUID)
 }
 
-func podMeta(name, namespace string, labels, annotations map[string]string) metav1.ObjectMeta {
+func podMeta(step *types.Step, config *config, podName string) metav1.ObjectMeta {
 	meta := metav1.ObjectMeta{
-		Name:        name,
-		Namespace:   namespace,
-		Annotations: annotations,
+		Name:        podName,
+		Namespace:   config.Namespace,
+		Annotations: config.PodAnnotations,
 	}
 
-	if labels == nil {
-		labels = make(map[string]string, 1)
-	}
-	labels[StepLabel] = name
+	labels := make(map[string]string, len(config.PodLabels)+1)
+	// copy to not alter the engine config
+	maps.Copy(labels, config.PodLabels)
+	labels[StepLabel] = step.Name
 	meta.Labels = labels
 
 	return meta
 }
 
-func podSpec(serviceAccountName string, vols, pullSecretNames []string, env, backendNodeSelector map[string]string,
-	extraHosts []types.HostAlias, backendTolerations []types.Toleration,
-	securityContext *types.SecurityContext, securityContextConfig SecurityContextConfig,
-) (v1.PodSpec, error) {
+func podSpec(step *types.Step, config *config) (v1.PodSpec, error) {
 	var err error
 	spec := v1.PodSpec{
 		RestartPolicy:      v1.RestartPolicyNever,
-		ServiceAccountName: serviceAccountName,
-		ImagePullSecrets:   imagePullSecretsReferences(pullSecretNames),
+		ServiceAccountName: step.BackendOptions.Kubernetes.ServiceAccountName,
+		ImagePullSecrets:   imagePullSecretsReferences(config.ImagePullSecretNames),
+		HostAliases:        hostAliases(step.ExtraHosts),
+		NodeSelector:       nodeSelector(step.BackendOptions.Kubernetes.NodeSelector, step.Environment["CI_SYSTEM_PLATFORM"]),
+		Tolerations:        tolerations(step.BackendOptions.Kubernetes.Tolerations),
+		SecurityContext:    podSecurityContext(step.BackendOptions.Kubernetes.SecurityContext, config.SecurityContext),
 	}
-
-	spec.HostAliases = hostAliases(extraHosts)
-	spec.NodeSelector = nodeSelector(backendNodeSelector, env["CI_SYSTEM_PLATFORM"])
-	spec.Tolerations = tolerations(backendTolerations)
-	spec.SecurityContext = podSecurityContext(securityContext, securityContextConfig)
-	spec.Volumes, err = volumes(vols)
+	spec.Volumes, err = volumes(step.Volumes)
 	if err != nil {
 		return spec, err
 	}
@@ -116,36 +103,34 @@ func podSpec(serviceAccountName string, vols, pullSecretNames []string, env, bac
 	return spec, nil
 }
 
-func podContainer(name, image, workDir, goos string, pull, privileged bool, commands, volumes []string, env map[string]string, resources types.Resources,
-	securityContext *types.SecurityContext,
-) (v1.Container, error) {
+func podContainer(step *types.Step, podName, goos string) (v1.Container, error) {
 	var err error
 	container := v1.Container{
-		Name:       name,
-		Image:      image,
-		WorkingDir: workDir,
+		Name:       podName,
+		Image:      step.Image,
+		WorkingDir: step.WorkingDir,
 	}
 
-	if pull {
+	if step.Pull {
 		container.ImagePullPolicy = v1.PullAlways
 	}
 
-	if len(commands) != 0 {
-		scriptEnv, command, args := common.GenerateContainerConf(commands, goos)
+	if len(step.Commands) != 0 {
+		scriptEnv, command, args := common.GenerateContainerConf(step.Commands, goos)
 		container.Command = command
 		container.Args = args
-		maps.Copy(env, scriptEnv)
+		maps.Copy(step.Environment, scriptEnv)
 	}
 
-	container.Env = mapToEnvVars(env)
-	container.SecurityContext = containerSecurityContext(securityContext, privileged)
+	container.Env = mapToEnvVars(step.Environment)
+	container.SecurityContext = containerSecurityContext(step.BackendOptions.Kubernetes.SecurityContext, step.Privileged)
 
-	container.Resources, err = resourceRequirements(resources)
+	container.Resources, err = resourceRequirements(step.BackendOptions.Kubernetes.Resources)
 	if err != nil {
 		return container, err
 	}
 
-	container.VolumeMounts, err = volumeMounts(volumes)
+	container.VolumeMounts, err = volumeMounts(step.Volumes)
 	if err != nil {
 		return container, err
 	}
@@ -378,12 +363,7 @@ func startPod(ctx context.Context, engine *kube, step *types.Step) (*v1.Pod, err
 	if err != nil {
 		return nil, err
 	}
-
-	pod, err := mkPod(engine.config.Namespace, podName, step.Image, step.WorkingDir, engine.goos, step.BackendOptions.Kubernetes.ServiceAccountName,
-		step.Pull, step.Privileged,
-		step.Commands, step.Volumes, engine.config.ImagePullSecretNames,
-		engine.config.PodLabels, engine.config.PodAnnotations, step.Environment, step.BackendOptions.Kubernetes.NodeSelector,
-		step.ExtraHosts, step.BackendOptions.Kubernetes.Tolerations, step.BackendOptions.Kubernetes.Resources, step.BackendOptions.Kubernetes.SecurityContext, engine.config.SecurityContext)
+	pod, err := mkPod(step, engine.config, podName, engine.goos)
 	if err != nil {
 		return nil, err
 	}

pipeline/backend/kubernetes/pod_test.go

@@ -24,16 +24,33 @@ import (
 )
 
 func TestPodName(t *testing.T) {
-	name, err := podName(&types.Step{Name: "wp_01he8bebctabr3kgk0qj36d2me_0"})
+	name, err := podName(&types.Step{UUID: "01he8bebctabr3kgk0qj36d2me-0"})
 	assert.NoError(t, err)
 	assert.Equal(t, "wp-01he8bebctabr3kgk0qj36d2me-0", name)
 
-	name, err = podName(&types.Step{Name: "wp\\01he8bebctabr3kgk0qj36d2me-0"})
-	assert.NoError(t, err)
-	assert.Equal(t, "wp\\01he8bebctabr3kgk0qj36d2me-0", name)
-
-	_, err = podName(&types.Step{Name: "wp-01he8bebctabr3kgk0qj36d2me-0-services-0.woodpecker-runtime.svc.cluster.local"})
+	_, err = podName(&types.Step{UUID: "01he8bebctabr3kgk0qj36d2me\\0a"})
 	assert.ErrorIs(t, err, ErrDNSPatternInvalid)
+
+	_, err = podName(&types.Step{UUID: "01he8bebctabr3kgk0qj36d2me-0-services-0..woodpecker-runtime.svc.cluster.local"})
+	assert.ErrorIs(t, err, ErrDNSPatternInvalid)
+}
+
+func TestStepToPodName(t *testing.T) {
+	name, err := stepToPodName(&types.Step{UUID: "01he8bebctabr3kg", Name: "clone", Type: types.StepTypeClone})
+	assert.NoError(t, err)
+	assert.EqualValues(t, "wp-01he8bebctabr3kg", name)
+	name, err = stepToPodName(&types.Step{UUID: "01he8bebctabr3kg", Name: "clone", Type: types.StepTypeCache})
+	assert.NoError(t, err)
+	assert.EqualValues(t, "wp-01he8bebctabr3kg", name)
+	name, err = stepToPodName(&types.Step{UUID: "01he8bebctabr3kg", Name: "clone", Type: types.StepTypePlugin})
+	assert.NoError(t, err)
+	assert.EqualValues(t, "wp-01he8bebctabr3kg", name)
+	name, err = stepToPodName(&types.Step{UUID: "01he8bebctabr3kg", Name: "clone", Type: types.StepTypeCommands})
+	assert.NoError(t, err)
+	assert.EqualValues(t, "wp-01he8bebctabr3kg", name)
+	name, err = stepToPodName(&types.Step{UUID: "01he8bebctabr3kg", Name: "clone", Type: types.StepTypeService})
+	assert.NoError(t, err)
+	assert.EqualValues(t, "clone", name)
 }
 
 func TestTinyPod(t *testing.T) {
@@ -44,7 +61,7 @@ func TestTinyPod(t *testing.T) {
 			"namespace": "woodpecker",
 			"creationTimestamp": null,
 			"labels": {
-				"step": "wp-01he8bebctabr3kgk0qj36d2me-0"
+				"step": "build-via-gradle"
 			}
 		},
 		"spec": {
@@ -101,13 +118,18 @@ func TestTinyPod(t *testing.T) {
 		"status": {}
 	}`
 
-	pod, err := mkPod("woodpecker", "wp-01he8bebctabr3kgk0qj36d2me-0", "gradle:8.4.0-jdk21", "/woodpecker/src", "linux/amd64", "",
-		false, false,
-		[]string{"gradle build"}, []string{"workspace:/woodpecker/src"}, nil,
-		nil, nil, map[string]string{"CI": "woodpecker"}, nil,
-		nil, nil,
-		types.Resources{Requests: nil, Limits: nil}, nil, SecurityContextConfig{},
-	)
+	pod, err := mkPod(&types.Step{
+		Name:        "build-via-gradle",
+		Image:       "gradle:8.4.0-jdk21",
+		WorkingDir:  "/woodpecker/src",
+		Pull:        false,
+		Privileged:  false,
+		Commands:    []string{"gradle build"},
+		Volumes:     []string{"workspace:/woodpecker/src"},
+		Environment: map[string]string{"CI": "woodpecker"},
+	}, &config{
+		Namespace: "woodpecker",
+	}, "wp-01he8bebctabr3kgk0qj36d2me-0", "linux/amd64")
 	assert.NoError(t, err)
 
 	json, err := json.Marshal(pod)
@@ -126,7 +148,7 @@ func TestFullPod(t *testing.T) {
 			"creationTimestamp": null,
 			"labels": {
 				"app": "test",
-				"step": "wp-01he8bebctabr3kgk0qj36d2me-0"
+				"step": "go-test"
 			},
 			"annotations": {
 				"apparmor.security": "runtime/default"
@@ -242,15 +264,41 @@ func TestFullPod(t *testing.T) {
 		{Name: "cloudflare", IP: "1.1.1.1"},
 		{Name: "cf.v6", IP: "2606:4700:4700::64"},
 	}
-	pod, err := mkPod("woodpecker", "wp-01he8bebctabr3kgk0qj36d2me-0", "meltwater/drone-cache", "/woodpecker/src", "linux/amd64", "wp-svc-acc",
-		true, true,
-		[]string{"go get", "go test"}, []string{"woodpecker-cache:/woodpecker/src/cache"}, []string{"regcred", "another-pull-secret"},
-		map[string]string{"app": "test"}, map[string]string{"apparmor.security": "runtime/default"}, map[string]string{"CGO": "0"}, map[string]string{"storage": "ssd"},
-		hostAliases, []types.Toleration{{Key: "net-port", Value: "100Mbit", Effect: types.TaintEffectNoSchedule}},
-		types.Resources{Requests: map[string]string{"memory": "128Mi", "cpu": "1000m"}, Limits: map[string]string{"memory": "256Mi", "cpu": "2"}},
-		&types.SecurityContext{Privileged: newBool(true), RunAsNonRoot: newBool(true), RunAsUser: newInt64(101), RunAsGroup: newInt64(101), FSGroup: newInt64(101)},
-		SecurityContextConfig{RunAsNonRoot: false},
-	)
+	pod, err := mkPod(&types.Step{
+		Name:        "go-test",
+		Image:       "meltwater/drone-cache",
+		WorkingDir:  "/woodpecker/src",
+		Pull:        true,
+		Privileged:  true,
+		Commands:    []string{"go get", "go test"},
+		Volumes:     []string{"woodpecker-cache:/woodpecker/src/cache"},
+		Environment: map[string]string{"CGO": "0"},
+		ExtraHosts:  hostAliases,
+		BackendOptions: types.BackendOptions{
+			Kubernetes: types.KubernetesBackendOptions{
+				NodeSelector:       map[string]string{"storage": "ssd"},
+				ServiceAccountName: "wp-svc-acc",
+				Tolerations:        []types.Toleration{{Key: "net-port", Value: "100Mbit", Effect: types.TaintEffectNoSchedule}},
+				Resources: types.Resources{
+					Requests: map[string]string{"memory": "128Mi", "cpu": "1000m"},
+					Limits:   map[string]string{"memory": "256Mi", "cpu": "2"},
+				},
+				SecurityContext: &types.SecurityContext{
+					Privileged:   newBool(true),
+					RunAsNonRoot: newBool(true),
+					RunAsUser:    newInt64(101),
+					RunAsGroup:   newInt64(101),
+					FSGroup:      newInt64(101),
+				},
+			},
+		},
+	}, &config{
+		Namespace:            "woodpecker",
+		ImagePullSecretNames: []string{"regcred", "another-pull-secret"},
+		PodLabels:            map[string]string{"app": "test"},
+		PodAnnotations:       map[string]string{"apparmor.security": "runtime/default"},
+		SecurityContext:      SecurityContextConfig{RunAsNonRoot: false},
+	}, "wp-01he8bebctabr3kgk0qj36d2me-0", "linux/amd64")
 	assert.NoError(t, err)
 
 	json, err := json.Marshal(pod)

pipeline/backend/kubernetes/service.go

@@ -26,11 +26,22 @@ import (
 	"k8s.io/apimachinery/pkg/util/intstr"
 )
 
-func mkService(namespace, name string, ports []uint16, selector map[string]string) *v1.Service {
-	log.Trace().Str("name", name).Interface("selector", selector).Interface("ports", ports).Msg("Creating service")
+const (
+	ServiceLabel = "service"
+)
+
+func mkService(step *types.Step, namespace string) (*v1.Service, error) {
+	name, err := serviceName(step)
+	if err != nil {
+		return nil, err
+	}
+
+	selector := map[string]string{
+		ServiceLabel: name,
+	}
 
 	var svcPorts []v1.ServicePort
-	for _, port := range ports {
+	for _, port := range step.Ports {
 		svcPorts = append(svcPorts, v1.ServicePort{
 			Name:       fmt.Sprintf("port-%d", port),
 			Port:       int32(port),
@@ -48,7 +59,7 @@ func mkService(namespace, name string, ports []uint16, selector map[string]strin
 			Selector: selector,
 			Ports:    svcPorts,
 		},
-	}
+	}, nil
 }
 
 func serviceName(step *types.Step) (string, error) {
@@ -56,21 +67,12 @@ func serviceName(step *types.Step) (string, error) {
 }
 
 func startService(ctx context.Context, engine *kube, step *types.Step) (*v1.Service, error) {
-	name, err := serviceName(step)
-	if err != nil {
-		return nil, err
-	}
-	podName, err := podName(step)
+	svc, err := mkService(step, engine.config.Namespace)
 	if err != nil {
 		return nil, err
 	}
 
-	selector := map[string]string{
-		StepLabel: podName,
-	}
-
-	svc := mkService(engine.config.Namespace, name, step.Ports, selector)
-
+	log.Trace().Str("name", svc.Name).Interface("selector", svc.Spec.Selector).Interface("ports", svc.Spec.Ports).Msg("creating service")
 	return engine.client.CoreV1().Services(engine.config.Namespace).Create(ctx, svc, metav1.CreateOptions{})
 }
 

pipeline/backend/kubernetes/service_test.go

@@ -23,16 +23,17 @@ import (
 )
 
 func TestServiceName(t *testing.T) {
-	name, err := serviceName(&types.Step{Name: "wp_01he8bebctabr3kgk0qj36d2me_0_services_0"})
+	name, err := serviceName(&types.Step{Name: "database"})
 	assert.NoError(t, err)
-	assert.Equal(t, "wp-01he8bebctabr3kgk0qj36d2me-0-services-0", name)
+	assert.Equal(t, "database", name)
 
-	name, err = serviceName(&types.Step{Name: "wp-01he8bebctabr3kgk0qj36d2me-0\\services-0"})
+	name, err = serviceName(&types.Step{Name: "wp-01he8bebctabr3kgk0qj36d2me-0-services-0.woodpecker-runtime.svc.cluster.local"})
 	assert.NoError(t, err)
-	assert.Equal(t, "wp-01he8bebctabr3kgk0qj36d2me-0\\services-0", name)
+	assert.Equal(t, "wp-01he8bebctabr3kgk0qj36d2me-0-services-0.woodpecker-runtime.svc.cluster.local", name)
 
-	_, err = serviceName(&types.Step{Name: "wp-01he8bebctabr3kgk0qj36d2me-0-services-0.woodpecker-runtime.svc.cluster.local"})
-	assert.ErrorIs(t, err, ErrDNSPatternInvalid)
+	name, err = serviceName(&types.Step{Name: "awesome_service"})
+	assert.NoError(t, err)
+	assert.Equal(t, "awesome-service", name)
 }
 
 func TestService(t *testing.T) {
@@ -62,7 +63,7 @@ func TestService(t *testing.T) {
 	      }
 	    ],
 	    "selector": {
-	      "step": "baz"
+	      "service": "bar"
 	    },
 	    "type": "ClusterIP"
 	  },
@@ -71,7 +72,11 @@ func TestService(t *testing.T) {
 	  }
 	}`
 
-	s := mkService("foo", "bar", []uint16{1, 2, 3}, map[string]string{"step": "baz"})
+	s, err := mkService(&types.Step{
+		Name:  "bar",
+		Ports: []uint16{1, 2, 3},
+	}, "foo")
+	assert.NoError(t, err)
 	j, err := json.Marshal(s)
 	assert.NoError(t, err)
 	assert.JSONEq(t, expected, string(j))

pipeline/backend/kubernetes/utils.go

@@ -27,12 +27,15 @@ import (
 )
 
 var (
-	dnsPattern           = regexp.MustCompile(`^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$`)
+	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
+	)
 	ErrDNSPatternInvalid = errors.New("name is not a valid kubernetes DNS name")
 )
 
 func dnsName(i string) (string, error) {
-	res := strings.ReplaceAll(i, "_", "-")
+	res := strings.ToLower(strings.ReplaceAll(i, "_", "-"))
 
 	if found := dnsPattern.FindStringIndex(res); found == nil {
 		return "", ErrDNSPatternInvalid

pipeline/backend/kubernetes/utils_test.go

@@ -0,0 +1,56 @@
+// Copyright 2024 Woodpecker Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package kubernetes
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+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)
+
+	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)
+}

pipeline/backend/kubernetes/volume_test.go

@@ -26,9 +26,8 @@ func TestPvcName(t *testing.T) {
 	assert.NoError(t, err)
 	assert.Equal(t, "woodpecker-cache", name)
 
-	name, err = volumeName("woodpecker\\cache")
-	assert.NoError(t, err)
-	assert.Equal(t, "woodpecker\\cache", name)
+	_, err = volumeName("woodpecker\\cache")
+	assert.ErrorIs(t, err, ErrDNSPatternInvalid)
 
 	_, err = volumeName("-woodpecker.cache:/woodpecker/src/cache")
 	assert.ErrorIs(t, err, ErrDNSPatternInvalid)
@@ -99,6 +98,6 @@ func TestPersistentVolumeClaim(t *testing.T) {
 	assert.NoError(t, err)
 	assert.JSONEq(t, expectedRwo, string(j))
 
-	_, err = mkPersistentVolumeClaim("someNamespace", "some0INVALID3name", "local-storage", "1Gi", false)
+	_, err = mkPersistentVolumeClaim("someNamespace", "some0..INVALID3name", "local-storage", "1Gi", false)
 	assert.Error(t, err)
 }