From c24a4bfaea6546b41947774434c000b0a8701f8d Mon Sep 17 00:00:00 2001 From: Josh Deprez Date: Thu, 19 Dec 2024 10:07:34 +1100 Subject: [PATCH] Allow patching some container commands/args --- internal/controller/scheduler/scheduler.go | 82 ++++++- .../controller/scheduler/scheduler_test.go | 226 ++++++++++++++---- .../fixtures/podspecpatch-agent.yaml | 17 ++ internal/integration/integration_test.go | 27 ++- 4 files changed, 285 insertions(+), 67 deletions(-) create mode 100644 internal/integration/fixtures/podspecpatch-agent.yaml diff --git a/internal/controller/scheduler/scheduler.go b/internal/controller/scheduler/scheduler.go index 2666b928..544dc13e 100644 --- a/internal/controller/scheduler/scheduler.go +++ b/internal/controller/scheduler/scheduler.go @@ -43,6 +43,11 @@ const ( var errK8sPluginProhibited = errors.New("the kubernetes plugin is prohibited by this controller, but was configured on this job") +var ( + commandContainerCommand = []string{"/workspace/tini-static"} + commandContainerArgs = []string{"--", "/workspace/buildkite-agent", "bootstrap"} +) + type Config struct { Namespace string Image string @@ -399,8 +404,8 @@ func (w *worker) Build(podSpec *corev1.PodSpec, skipCheckout bool, inputs buildI // Substitute the container's entrypoint for tini-static + buildkite-agent // Note that tini-static (not plain tini) is needed because we don't // know what libraries are available in the image. - c.Command = []string{"/workspace/tini-static"} - c.Args = []string{"--", "/workspace/buildkite-agent", "bootstrap"} + c.Command = commandContainerCommand + c.Args = commandContainerArgs if c.ImagePullPolicy == "" { c.ImagePullPolicy = w.cfg.DefaultImagePullPolicy @@ -446,8 +451,8 @@ func (w *worker) Build(podSpec *corev1.PodSpec, skipCheckout bool, inputs buildI c := corev1.Container{ Name: "container-0", Image: w.cfg.Image, - Command: []string{"/workspace/tini-static"}, - Args: []string{"--", "/workspace/buildkite-agent", "bootstrap"}, + Command: commandContainerCommand, + Args: commandContainerArgs, WorkingDir: "/workspace", VolumeMounts: volumeMounts, ImagePullPolicy: w.cfg.DefaultImagePullPolicy, @@ -563,7 +568,7 @@ func (w *worker) Build(podSpec *corev1.PodSpec, skipCheckout bool, inputs buildI // Allow podSpec to be overridden by the controller config and the k8s plugin. // Patch from the controller config is applied first. if w.cfg.PodSpecPatch != nil { - patched, err := PatchPodSpec(podSpec, w.cfg.PodSpecPatch) + patched, err := PatchPodSpec(podSpec, w.cfg.PodSpecPatch, w.cfg.DefaultCommandParams, inputs.k8sPlugin) if err != nil { return nil, fmt.Errorf("failed to apply podSpec patch from agent: %w", err) } @@ -573,7 +578,7 @@ func (w *worker) Build(podSpec *corev1.PodSpec, skipCheckout bool, inputs buildI // If present, patch from the k8s plugin is applied second. if inputs.k8sPlugin != nil && inputs.k8sPlugin.PodSpecPatch != nil { - patched, err := PatchPodSpec(podSpec, inputs.k8sPlugin.PodSpecPatch) + patched, err := PatchPodSpec(podSpec, inputs.k8sPlugin.PodSpecPatch, w.cfg.DefaultCommandParams, inputs.k8sPlugin) if err != nil { return nil, fmt.Errorf("failed to apply podSpec patch from k8s plugin: %w", err) } @@ -747,14 +752,65 @@ func (w *worker) Build(podSpec *corev1.PodSpec, skipCheckout bool, inputs buildI return kjob, nil } -var ErrNoCommandModification = errors.New("modifying container commands or args via podSpecPatch is not supported. Specify the command in the job's command field instead") +var ErrNoCommandModification = errors.New("modifying container commands or args via podSpecPatch is not supported") + +func PatchPodSpec(original *corev1.PodSpec, patch *corev1.PodSpec, cmdParams *config.CommandParams, k8sPlugin *KubernetesPlugin) (*corev1.PodSpec, error) { + + // Index command containers by name - these should be unique within each podSpec. + originalContainers := make(map[string]*corev1.Container) + for i := range original.Containers { + c := &original.Containers[i] + originalContainers[c.Name] = c + } + + // We do special stuff™️ with container commands to make them run as + // buildkite agent things under the hood, so don't let users mess with them + // via podSpecPatch. + for i := range patch.Containers { + c := &patch.Containers[i] + if len(c.Command) == 0 && len(c.Args) == 0 { + // No modification (strategic merge won't set these to empty). + continue + } + oc := originalContainers[c.Name] + if oc != nil && slices.Equal(c.Command, oc.Command) && slices.Equal(c.Args, oc.Args) { + // No modification (original and patch are equal). + continue + } + + // Some modification is occuring. + // What we prevent vs what we fix up depends on the type of container. + + // Agent, checkout: prevent command modification entirely. + switch c.Name { + case AgentContainerName: + return nil, fmt.Errorf("for the agent container, %w", ErrNoCommandModification) + + case CheckoutContainerName: + return nil, fmt.Errorf("for the checkout container, %w; instead consider configuring a checkout hook or skipping the checkout container entirely", ErrNoCommandModification) + + default: + // Is it patching a command container or a sidecar? + if oc == nil || !slices.Equal(oc.Command, commandContainerCommand) || !slices.Equal(oc.Args, commandContainerArgs) { + // It's either a new container, which for now we'll treat as + // a sidecar, or a patch on an existing sidecar. + // Since these are unmonitored by buildkite-agent within the pod + // the command modification is fine. + continue + } -func PatchPodSpec(original *corev1.PodSpec, patch *corev1.PodSpec) (*corev1.PodSpec, error) { - // We do special stuff™️ with container commands to make them run as buildkite agent things under the hood, so don't - // let users mess with them via podSpecPatch. - for _, c := range patch.Containers { - if len(c.Command) != 0 || len(c.Args) != 0 { - return nil, ErrNoCommandModification + // It's a command container. Remove the modification and apply the + // same transformation from container command to BUILDKITE_COMMAND. + command := cmdParams.Command(c.Command, c.Args) + if k8sPlugin != nil && k8sPlugin.CommandParams != nil && k8sPlugin.CommandParams.Interposer != "" { + command = k8sPlugin.CommandParams.Command(c.Command, c.Args) + } + c.Command = nil + c.Args = nil + c.Env = append(c.Env, corev1.EnvVar{ + Name: "BUILDKITE_COMMAND", + Value: command, + }) } } diff --git a/internal/controller/scheduler/scheduler_test.go b/internal/controller/scheduler/scheduler_test.go index 57ad8127..1bbeda45 100644 --- a/internal/controller/scheduler/scheduler_test.go +++ b/internal/controller/scheduler/scheduler_test.go @@ -1,12 +1,12 @@ -package scheduler_test +package scheduler import ( "encoding/json" + "errors" "fmt" "testing" "github.com/buildkite/agent-stack-k8s/v2/api" - "github.com/buildkite/agent-stack-k8s/v2/internal/controller/scheduler" "github.com/google/go-cmp/cmp" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -19,14 +19,13 @@ func TestPatchPodSpec(t *testing.T) { t.Parallel() cases := []struct { - name string - podspec *corev1.PodSpec - patch *corev1.PodSpec - assertion func(t *testing.T, result *corev1.PodSpec) - err error + name string + podspec *corev1.PodSpec + patch *corev1.PodSpec + want *corev1.PodSpec }{ { - name: "patching a container", + name: "patching in a new unmanaged container", podspec: &corev1.PodSpec{ Containers: []corev1.Container{ { @@ -45,19 +44,57 @@ func TestPatchPodSpec(t *testing.T) { }, }, }, - assertion: func(t *testing.T, result *corev1.PodSpec) { - assert.Equal(t, "debian:latest", result.Containers[0].Image) + want: &corev1.PodSpec{ + Containers: []corev1.Container{{ + Name: "my-cool-container", + Image: "debian:latest", + }, { + Image: "alpine:latest", + Command: []string{ + "echo hello world", + }, + }}, }, }, { - name: "patching container commands should fail", + name: "patching a sidecar container", podspec: &corev1.PodSpec{ Containers: []corev1.Container{ { - Image: "alpine:latest", - Command: []string{ - "echo hello world", - }, + Name: "sidecar-0", + Image: "alpine:latest", + Command: []string{"echo hello world"}, + }, + }, + }, + patch: &corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "sidecar-0", + Command: []string{"echo goodbye world"}, + }, + }, + }, + want: &corev1.PodSpec{ + Containers: []corev1.Container{{ + Name: "sidecar-0", + Image: "alpine:latest", + Command: []string{"echo goodbye world"}, + }}, + }, + }, + { + name: "patching command container commands and args should work", + podspec: &corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "my-cool-container", + Image: "alpine:latest", + Command: commandContainerCommand, + Args: commandContainerArgs, + Env: []corev1.EnvVar{{ + Name: "BUILDKITE_COMMAND", Value: "echo hello world", + }}, }, }, }, @@ -65,44 +102,135 @@ func TestPatchPodSpec(t *testing.T) { Containers: []corev1.Container{ { Name: "my-cool-container", - Command: []string{"this", "shouldn't", "work"}, + Command: []string{"this should"}, + Args: []string{"work", "as", "expected"}, + }, + }, + }, + want: &corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "my-cool-container", + Image: "alpine:latest", + Command: commandContainerCommand, + Args: commandContainerArgs, + Env: []corev1.EnvVar{{ + Name: "BUILDKITE_COMMAND", Value: "this should work as expected", + }}, }, }, }, - err: scheduler.ErrNoCommandModification, }, + } + + for _, test := range cases { + t.Run(test.name, func(t *testing.T) { + got, err := PatchPodSpec(test.podspec, test.patch, nil, nil) + if err != nil { + t.Fatalf("PodSpecPatch error = %v", err) + } + if diff := cmp.Diff(got, test.want); diff != "" { + t.Errorf("PodSpecPatch result diff (-got +want):\n%s", diff) + } + }) + } +} + +func TestPatchPodSpec_ErrNoCommandModification(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + podspec *corev1.PodSpec + patch *corev1.PodSpec + }{ { - name: "patching container args should fail", + name: "patching agent command should fail", podspec: &corev1.PodSpec{ Containers: []corev1.Container{ { - Image: "alpine:latest", - Command: []string{ - "echo hello world", - }, + Name: AgentContainerName, + Image: "alpine:latest", + Command: []string{"echo hello world"}, + }, + }, + }, + patch: &corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: AgentContainerName, + Command: []string{"this shouldn't work"}, + }, + }, + }, + }, + { + name: "patching agent args should fail", + podspec: &corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: AgentContainerName, + Image: "alpine:latest", + Command: []string{"echo hello world"}, + }, + }, + }, + patch: &corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: AgentContainerName, + Args: []string{"this", "shouldn't", "work"}, + }, + }, + }, + }, + { + name: "patching checkout command should fail", + podspec: &corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: CheckoutContainerName, + Image: "alpine:latest", + Command: []string{"echo hello world"}, + }, + }, + }, + patch: &corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: CheckoutContainerName, + Command: []string{"this shouldn't work"}, + }, + }, + }, + }, + { + name: "patching checkout args should fail", + podspec: &corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: CheckoutContainerName, + Image: "alpine:latest", + Command: []string{"echo hello world"}, }, }, }, patch: &corev1.PodSpec{ Containers: []corev1.Container{ { - Name: "my-cool-container", - Args: []string{"this", "also", "shouldn't", "work"}, + Name: CheckoutContainerName, + Args: []string{"this", "shouldn't", "work"}, }, }, }, - err: scheduler.ErrNoCommandModification, }, } - for _, c := range cases { - t.Run(c.name, func(t *testing.T) { - result, err := scheduler.PatchPodSpec(c.podspec, c.patch) - if c.err != nil { - require.Error(t, err) - require.ErrorIs(t, c.err, err) - } else { - c.assertion(t, result) + for _, test := range cases { + t.Run(test.name, func(t *testing.T) { + _, err := PatchPodSpec(test.podspec, test.patch, nil, nil) + if !errors.Is(err, ErrNoCommandModification) { + t.Errorf("PodSpecPatch error = %v, want ErrNoCommandModification (%v)", err, ErrNoCommandModification) } }) } @@ -110,7 +238,7 @@ func TestPatchPodSpec(t *testing.T) { func TestJobPluginConversion(t *testing.T) { t.Parallel() - pluginConfig := scheduler.KubernetesPlugin{ + pluginConfig := KubernetesPlugin{ PodSpec: &corev1.PodSpec{ Containers: []corev1.Container{ { @@ -153,10 +281,10 @@ func TestJobPluginConversion(t *testing.T) { Env: []string{fmt.Sprintf("BUILDKITE_PLUGINS=%s", string(pluginsJSON))}, AgentQueryRules: []string{"queue=kubernetes"}, } - worker := scheduler.New( + worker := New( zaptest.NewLogger(t), nil, - scheduler.Config{ + Config{ AgentTokenSecretName: "token-secret", Image: "buildkite/agent:latest", }, @@ -216,7 +344,7 @@ func TestTagEnv(t *testing.T) { t.Parallel() logger := zaptest.NewLogger(t) - pluginConfig := scheduler.KubernetesPlugin{ + pluginConfig := KubernetesPlugin{ PodSpec: &corev1.PodSpec{ Containers: []corev1.Container{ { @@ -238,10 +366,10 @@ func TestTagEnv(t *testing.T) { Env: []string{fmt.Sprintf("BUILDKITE_PLUGINS=%s", string(pluginsJSON))}, AgentQueryRules: []string{"queue=kubernetes"}, } - worker := scheduler.New( + worker := New( logger, nil, - scheduler.Config{ + Config{ AgentTokenSecretName: "token-secret", Image: "buildkite/agent:latest", }, @@ -277,7 +405,7 @@ func TestJobWithNoKubernetesPlugin(t *testing.T) { Command: "echo hello world", AgentQueryRules: []string{}, } - worker := scheduler.New(zaptest.NewLogger(t), nil, scheduler.Config{ + worker := New(zaptest.NewLogger(t), nil, Config{ Image: "buildkite/agent:latest", }) inputs, err := worker.ParseJob(job) @@ -313,10 +441,10 @@ func TestBuild(t *testing.T) { AgentQueryRules: []string{"queue=kubernetes"}, } - worker := scheduler.New( + worker := New( zaptest.NewLogger(t), nil, - scheduler.Config{ + Config{ Namespace: "buildkite", Image: "buildkite/agent:latest", AgentTokenSecretName: "bkcq_1234567890", @@ -381,10 +509,10 @@ func TestBuildSkipCheckout(t *testing.T) { AgentQueryRules: []string{"queue=kubernetes"}, } - worker := scheduler.New( + worker := New( zaptest.NewLogger(t), nil, - scheduler.Config{ + Config{ Namespace: "buildkite", Image: "buildkite/agent:latest", AgentTokenSecretName: "bkcq_1234567890", @@ -426,10 +554,10 @@ func TestBuildCheckoutEmptyConfigEnv(t *testing.T) { AgentQueryRules: []string{"queue=kubernetes"}, } - worker := scheduler.New( + worker := New( zaptest.NewLogger(t), nil, - scheduler.Config{ + Config{ Namespace: "buildkite", Image: "buildkite/agent:latest", AgentTokenSecretName: "bkcq_1234567890", @@ -465,7 +593,7 @@ func TestFailureJobs(t *testing.T) { Env: []string{fmt.Sprintf("BUILDKITE_PLUGINS=%s", pluginsJSON)}, AgentQueryRules: []string{"queue=kubernetes"}, } - wrapper := scheduler.New(zaptest.NewLogger(t), nil, scheduler.Config{}) + wrapper := New(zaptest.NewLogger(t), nil, Config{}) _, err = wrapper.ParseJob(job) require.Error(t, err) } @@ -474,7 +602,7 @@ func TestProhibitKubernetesPlugin(t *testing.T) { t.Parallel() pluginsJSON, err := json.Marshal([]map[string]any{ { - "github.com/buildkite-plugins/kubernetes-buildkite-plugin": scheduler.KubernetesPlugin{}, + "github.com/buildkite-plugins/kubernetes-buildkite-plugin": KubernetesPlugin{}, }, }) require.NoError(t, err) @@ -484,7 +612,7 @@ func TestProhibitKubernetesPlugin(t *testing.T) { Env: []string{fmt.Sprintf("BUILDKITE_PLUGINS=%s", pluginsJSON)}, AgentQueryRules: []string{"queue=kubernetes"}, } - worker := scheduler.New(zaptest.NewLogger(t), nil, scheduler.Config{ + worker := New(zaptest.NewLogger(t), nil, Config{ ProhibitK8sPlugin: true, }) _, err = worker.ParseJob(job) diff --git a/internal/integration/fixtures/podspecpatch-agent.yaml b/internal/integration/fixtures/podspecpatch-agent.yaml new file mode 100644 index 00000000..ff669dbf --- /dev/null +++ b/internal/integration/fixtures/podspecpatch-agent.yaml @@ -0,0 +1,17 @@ +steps: + - label: ":x:" + agents: + queue: "{{.queue}}" + plugins: + - kubernetes: + podSpec: + containers: + - image: alpine:latest + name: nope + command: + - echo "la di da di da" + podSpecPatch: + containers: + - name: agent + command: + - echo "am I out of touch? no, it's buildkite-agent that is wrong!" diff --git a/internal/integration/integration_test.go b/internal/integration/integration_test.go index f76bde94..27dffa95 100644 --- a/internal/integration/integration_test.go +++ b/internal/integration/integration_test.go @@ -2,7 +2,6 @@ package integration_test import ( "context" - "fmt" "strconv" "strings" "testing" @@ -38,7 +37,7 @@ func TestWalkingSkeleton(t *testing.T) { ) } -func TestPodSpecPatchInStep(t *testing.T) { +func TestPodSpecPatch_InStep(t *testing.T) { tc := testcase{ T: t, Fixture: "podspecpatch-step.yaml", @@ -54,7 +53,7 @@ func TestPodSpecPatchInStep(t *testing.T) { tc.AssertLogsContain(build, "value of MOUNTAIN is cotopaxi") } -func TestPodSpecPatchInStepFailsWhenPatchingContainerCommands(t *testing.T) { +func TestPodSpecPatch_AllowsPatchingCommandContainerCommands(t *testing.T) { tc := testcase{ T: t, Fixture: "podspecpatch-command-step.yaml", @@ -68,11 +67,29 @@ func TestPodSpecPatchInStepFailsWhenPatchingContainerCommands(t *testing.T) { tc.StartController(ctx, cfg) build := tc.TriggerBuild(ctx, pipelineID) + tc.AssertSuccess(ctx, build) + tc.AssertLogsContain(build, "i love quito") +} + +func TestPodSpecPatch_RejectsPatchingAgentContainerCommand(t *testing.T) { + tc := testcase{ + T: t, + Fixture: "podspecpatch-agent.yaml", + Repo: repoHTTP, + GraphQL: api.NewClient(cfg.BuildkiteToken, cfg.GraphQLEndpoint), + }.Init() + + ctx := context.Background() + pipelineID := tc.PrepareQueueAndPipelineWithCleanup(ctx) + + tc.StartController(ctx, cfg) + build := tc.TriggerBuild(ctx, pipelineID) + tc.AssertFail(ctx, build) - tc.AssertLogsContain(build, fmt.Sprintf("%v", scheduler.ErrNoCommandModification)) + tc.AssertLogsContain(build, scheduler.ErrNoCommandModification.Error()) } -func TestPodSpecPatchInController(t *testing.T) { +func TestPodSpecPatch_InController(t *testing.T) { tc := testcase{ T: t, Fixture: "mountain.yaml",