From d1fe86b7bec57100d0044780802a5614f546b100 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Thu, 11 Jan 2024 16:32:37 +0100 Subject: [PATCH] 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 | 92 ++++++++------------ pipeline/backend/kubernetes/pod_test.go | 94 ++++++++++++++++----- pipeline/backend/kubernetes/service.go | 32 +++---- pipeline/backend/kubernetes/service_test.go | 21 +++-- pipeline/backend/kubernetes/utils.go | 7 +- pipeline/backend/kubernetes/utils_test.go | 56 ++++++++++++ pipeline/backend/kubernetes/volume_test.go | 7 +- 7 files changed, 201 insertions(+), 108 deletions(-) create mode 100644 pipeline/backend/kubernetes/utils_test.go diff --git a/pipeline/backend/kubernetes/pod.go b/pipeline/backend/kubernetes/pod.go index 6a8ee4d47ee..1abb20fa69c 100644 --- a/pipeline/backend/kubernetes/pod.go +++ b/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 - - meta := podMeta(name, namespace, labels, annotations) +func mkPod(step *types.Step, config *config, podName, goos string) (*v1.Pod, error) { + meta := podMeta(step, config, podName) - 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), - } - - 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) + 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.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 } diff --git a/pipeline/backend/kubernetes/pod_test.go b/pipeline/backend/kubernetes/pod_test.go index fe33cb63075..b964652c287 100644 --- a/pipeline/backend/kubernetes/pod_test.go +++ b/pipeline/backend/kubernetes/pod_test.go @@ -24,18 +24,35 @@ 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{UUID: "01he8bebctabr3kgk0qj36d2me\\0a"}) + assert.ErrorIs(t, err, ErrDNSPatternInvalid) - _, err = podName(&types.Step{Name: "wp-01he8bebctabr3kgk0qj36d2me-0-services-0.woodpecker-runtime.svc.cluster.local"}) + _, 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) { expected := ` { @@ -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) diff --git a/pipeline/backend/kubernetes/service.go b/pipeline/backend/kubernetes/service.go index 6930f743eac..33aa8f7a6c9 100644 --- a/pipeline/backend/kubernetes/service.go +++ b/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{}) } diff --git a/pipeline/backend/kubernetes/service_test.go b/pipeline/backend/kubernetes/service_test.go index f4b479f60fa..1a43f53d835 100644 --- a/pipeline/backend/kubernetes/service_test.go +++ b/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)) diff --git a/pipeline/backend/kubernetes/utils.go b/pipeline/backend/kubernetes/utils.go index c5802eab6d8..63d6bb1e6b5 100644 --- a/pipeline/backend/kubernetes/utils.go +++ b/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 diff --git a/pipeline/backend/kubernetes/utils_test.go b/pipeline/backend/kubernetes/utils_test.go new file mode 100644 index 00000000000..0cf1af04eb2 --- /dev/null +++ b/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) +} diff --git a/pipeline/backend/kubernetes/volume_test.go b/pipeline/backend/kubernetes/volume_test.go index 90e180d7811..8f906bde5bb 100644 --- a/pipeline/backend/kubernetes/volume_test.go +++ b/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) }