Add config option to disable command modification check

buildkite · Dec 12, 2024 · 46c0cc6 · 46c0cc6
1 parent 4de522d
commit 46c0cc6
Show file tree

Hide file tree

Showing 7 changed files with 133 additions and 56 deletions.
diff --git a/README.md b/README.md
@@ -3,27 +3,38 @@
 [![Build status](https://badge.buildkite.com/d58c90abfe8b48f8d8750dac8e911fc0b6afe026631b4dc97c.svg?branch=main)](https://buildkite.com/buildkite-kubernetes-stack/kubernetes-agent-stack)
 
 ## Table of Contents
-- [Overview](#Overview)
-- [How does it work](#How-does-it-work)
-- [Architecture](#Architecture)
-- [Installation](#Installation)
-  - [Requirements](#Requirements)
-  - [Deploy with Helm](#Deploy-with-Helm)
-  - [Options](#Options)
-- [Sample Buildkite Pipeline](#Sample-Buildkite-Pipelines)
-  - [Cloning repos via SSH](#Cloning-repos-via-SSH)
-  - [Cloning repos via HTTPS](#Cloning-repos-via-HTTPS)
-  - [Pod Spec Patch](#Pod-Spec-Patch)
-  - [Sidecars](#Sidecars)
-  - [Extra volume mounts](#Extra-volume-mounts)
-  - [Skipping checkout](#Skipping-checkout)
-  - [Overriding flags for git clone/fetch](#Overriding-flags-for-git-clonefetch)
-  - [Validating your pipeline](#Validating-your-pipeline)
-- [Securing the stack](#securing-the-stack)
-  - [Prohibiting the kubernetes plugin (v0.13.0 and later)](#prohibiting-the-kubernetes-plugin-v0130-and-later)
-- [How to setup agent hooks](#How-to-setup-agent-hooks)
-- [Debugging](#Debugging)
-- [Open Questions](#Open-Questions)
+-   [Overview](#overview)
+-   [How does it work](#how-does-it-work)
+-   [Architecture](#architecture)
+-   [Installation](#installation)
+    -   [Requirements](#requirements)
+    -   [Deploy with Helm](#deploy-with-helm)
+    -   [Options](#options)
+-   [Sample Buildkite Pipelines](#sample-buildkite-pipelines)
+    -   [PodSpec command and args interpretation](#podspec-command-and-args-interpretation)
+    -   [Cloning repos via SSH](#cloning-repos-via-ssh)
+    -   [Cloning repos via HTTPS](#cloning-repos-via-https)
+    -   [Default job metadata](#default-job-metadata)
+    -   [Pod Spec Patch](#pod-spec-patch)
+    -   [Sidecars](#sidecars)
+    -   [The workspace volume](#the-workspace-volume)
+    -   [Extra volume mounts](#extra-volume-mounts)
+    -   [Skipping checkout (v0.13.0 and later)](#skipping-checkout-v0130-and-later)
+    -   [Overriding flags for git clone and git fetch (v0.13.0 and later)](#overriding-flags-for-git-clone-and-git-fetch-v0130-and-later)
+    -   [Overriding other git settings (v0.16.0 and later)](#overriding-other-git-settings-v0160-and-later)
+    -   [Default envFrom](#default-envfrom)
+-   [Setting agent configuration (v0.16.0 and later)](#setting-agent-configuration-v0160-and-later)
+-   [How to set up pipeline signing (v0.16.0 and later)](#how-to-set-up-pipeline-signing-v0160-and-later)
+-   [How to set up agent hooks and plugins (v0.16.0 and later)](#how-to-set-up-agent-hooks-and-plugins-v0160-and-later)
+-   [How to set up agent hooks (v0.15.0 and earlier)](#how-to-set-up-agent-hooks-v0150-and-earlier)
+    -   [Validating your pipeline](#validating-your-pipeline)
+-   [Securing the stack](#securing-the-stack)
+    -   [Prohibiting the kubernetes plugin (v0.13.0 and later)](#prohibiting-the-kubernetes-plugin-v0130-and-later)
+-   [Debugging](#debugging)
+    -   [Prerequisites](#prerequisites)
+    -   [Inputs to the script](#inputs-to-the-script)
+    -   [Data/logs gathered:](#datalogs-gathered)
+-   [Open questions](#open-questions)
 
 ## Overview
 
@@ -553,6 +564,47 @@ steps:
   command: echo Hello World!
 ```
 
+#### Overriding commands
+
+By default, PodSpecPatch is prevented from modifying a container's `command` or
+`args`. Attempting to do so results in an error.
+
+If this is something you want to do, first consider other potential solutions:
+
+* To override checkout behaviour, consider writing a `checkout` hook, or
+  disabling the checkout container entirely with `checkout: skip: true`.
+* To run additional containers without `buildkite-agent` in them, consider using
+  a sidecar.
+
+We are continually investigating ways to make the stack more flexible while
+ensuring core functionality.
+
+> [!CAUTION]
+> Avoid using PodSpecPatch to override `command` or `args`. Such modifications,
+> if not done with extreme care and detailed knowledge about how agent-stack-k8s
+> constructs podspecs, are very likely to break how the agent within the pod
+> works.
+>
+> If the replacement command for a checkout or command container does not invoke
+> `buildkite-agent bootstrap`:
+>
+>  * the container will not connect to the `agent` container, and the agent will
+>    not finish the job normally because there was not an expected number of
+>    other containers connecting to it
+>  * logs from the container will not be visible in Buildkite
+>  * hooks will not be executed automatically
+>  * plugins will not be checked out or executed automatically
+>
+> and various other functions provided by `buildkite-agent` may not work.
+>
+> If the command for the `agent` container is overridden, and the replacement
+> command does not invoke `buildkite-agent start`, then the job will not be
+> acquired on Buildkite at all.
+
+If you still wish to disable this precaution, and override the raw `command` or
+`args` of your containers using PodSpecPatch, you may do so with the
+`allow-pod-spec-patch-raw-command-modification` config option.
+
 ### Sidecars
 
 Sidecar containers can be added to your job by specifying them under the top-level `sidecars` key. See [this example](internal/integration/fixtures/sidecars.yaml) for a simple job that runs `nginx` as a sidecar, and accesses the nginx server from the main job.

diff --git a/charts/agent-stack-k8s/values.schema.json b/charts/agent-stack-k8s/values.schema.json
@@ -262,6 +262,12 @@
           "title": "Causes the controller to prohibit the kubernetes plugin specified within jobs (pipeline YAML) - enabling this causes jobs with a kubernetes plugin to fail, preventing the pipeline YAML from having any influence over the podSpec",
           "examples": [true]
         },
+        "allow-pod-spec-patch-raw-command-modification": {
+          "type": "boolean",
+          "default": false,
+          "title": "Permits PodSpecPatch to modify the command or args fields of containers. See the warning in the README before enabling this option",
+          "examples": [false]
+        },
         "workspaceVolume": {
           "$ref": "https://kubernetesjsonschema.dev/master/_definitions.json#/definitions/io.k8s.api.core.v1.Volume"
         },

diff --git a/cmd/controller/controller.go b/cmd/controller/controller.go
@@ -120,6 +120,11 @@ func AddConfigFlags(cmd *cobra.Command) {
 		false,
 		"Causes the controller to prohibit the kubernetes plugin specified within jobs (pipeline YAML) - enabling this causes jobs with a kubernetes plugin to fail, preventing the pipeline YAML from having any influence over the podSpec",
 	)
+	cmd.Flags().Bool(
+		"allow-pod-spec-patch-raw-command-modification",
+		false,
+		"Permits PodSpecPatch to modify the command or args fields of containers. See the warning in the README before enabling this option",
+	)
 }
 
 // ReadConfigFromFileArgsAndEnv reads the config from the file, env and args in that order.

diff --git a/internal/controller/config/config.go b/internal/controller/config/config.go
@@ -68,6 +68,12 @@ type Config struct {
 	// from the job (the kubernetes "plugin" in pipeline.yml). If enabled,
 	// jobs with a "kubernetes" plugin will fail.
 	ProhibitKubernetesPlugin bool `json:"prohibit-kubernetes-plugin" validate:"omitempty"`
+
+	// AllowPodSpecPatchRawCmdMod can be used to allow podSpecPatch to change
+	// container commands. Normally this is prevented, because if the
+	// replacement command does not execute buildkite-agent in the right way,
+	// then the pod will malfunction.
+	AllowPodSpecPatchRawCmdMod bool `json:"allow-pod-spec-patch-raw-command-modification" validate:"omitempty"`
 }
 
 type stringSlice []string
@@ -97,6 +103,7 @@ func (c Config) MarshalLogObject(enc zapcore.ObjectEncoder) error {
 	enc.AddUint16("prometheus-port", c.PrometheusPort)
 	enc.AddString("cluster-uuid", c.ClusterUUID)
 	enc.AddBool("prohibit-kubernetes-plugin", c.ProhibitKubernetesPlugin)
+	enc.AddBool("allow-pod-spec-patch-raw-command-modification", c.AllowPodSpecPatchRawCmdMod)
 	if err := enc.AddArray("additional-redacted-vars", c.AdditionalRedactedVars); err != nil {
 		return err
 	}

diff --git a/internal/controller/controller.go b/internal/controller/controller.go
@@ -87,19 +87,20 @@ func Run(
 	// Scheduler does the complicated work of converting a Buildkite job into
 	// a pod to run that job. It talks to the k8s API to create pods.
 	sched := scheduler.New(logger.Named("scheduler"), k8sClient, scheduler.Config{
-		Namespace:              cfg.Namespace,
-		Image:                  cfg.Image,
-		AgentTokenSecretName:   cfg.AgentTokenSecret,
-		JobTTL:                 cfg.JobTTL,
-		AdditionalRedactedVars: cfg.AdditionalRedactedVars,
-		WorkspaceVolume:        cfg.WorkspaceVolume,
-		AgentConfig:            cfg.AgentConfig,
-		DefaultCheckoutParams:  cfg.DefaultCheckoutParams,
-		DefaultCommandParams:   cfg.DefaultCommandParams,
-		DefaultSidecarParams:   cfg.DefaultSidecarParams,
-		DefaultMetadata:        cfg.DefaultMetadata,
-		PodSpecPatch:           cfg.PodSpecPatch,
-		ProhibitK8sPlugin:      cfg.ProhibitKubernetesPlugin,
+		Namespace:                  cfg.Namespace,
+		Image:                      cfg.Image,
+		AgentTokenSecretName:       cfg.AgentTokenSecret,
+		JobTTL:                     cfg.JobTTL,
+		AdditionalRedactedVars:     cfg.AdditionalRedactedVars,
+		WorkspaceVolume:            cfg.WorkspaceVolume,
+		AgentConfig:                cfg.AgentConfig,
+		DefaultCheckoutParams:      cfg.DefaultCheckoutParams,
+		DefaultCommandParams:       cfg.DefaultCommandParams,
+		DefaultSidecarParams:       cfg.DefaultSidecarParams,
+		DefaultMetadata:            cfg.DefaultMetadata,
+		PodSpecPatch:               cfg.PodSpecPatch,
+		ProhibitK8sPlugin:          cfg.ProhibitKubernetesPlugin,
+		AllowPodSpecPatchRawCmdMod: cfg.AllowPodSpecPatchRawCmdMod,
 	})
 
 	informerFactory, err := NewInformerFactory(k8sClient, cfg.Namespace, cfg.Tags)

diff --git a/internal/controller/scheduler/scheduler.go b/internal/controller/scheduler/scheduler.go
@@ -40,19 +40,20 @@ const (
 var errK8sPluginProhibited = errors.New("the kubernetes plugin is prohibited by this controller, but was configured on this job")
 
 type Config struct {
-	Namespace              string
-	Image                  string
-	AgentTokenSecretName   string
-	JobTTL                 time.Duration
-	AdditionalRedactedVars []string
-	WorkspaceVolume        *corev1.Volume
-	AgentConfig            *config.AgentConfig
-	DefaultCheckoutParams  *config.CheckoutParams
-	DefaultCommandParams   *config.CommandParams
-	DefaultSidecarParams   *config.SidecarParams
-	DefaultMetadata        config.Metadata
-	PodSpecPatch           *corev1.PodSpec
-	ProhibitK8sPlugin      bool
+	Namespace                  string
+	Image                      string
+	AgentTokenSecretName       string
+	JobTTL                     time.Duration
+	AdditionalRedactedVars     []string
+	WorkspaceVolume            *corev1.Volume
+	AgentConfig                *config.AgentConfig
+	DefaultCheckoutParams      *config.CheckoutParams
+	DefaultCommandParams       *config.CommandParams
+	DefaultSidecarParams       *config.SidecarParams
+	DefaultMetadata            config.Metadata
+	PodSpecPatch               *corev1.PodSpec
+	ProhibitK8sPlugin          bool
+	AllowPodSpecPatchRawCmdMod bool
 }
 
 func New(logger *zap.Logger, client kubernetes.Interface, cfg Config) *worker {
@@ -662,7 +663,7 @@ func (w *worker) Build(podSpec *corev1.PodSpec, skipCheckout bool, inputs buildI
 
 	// Patch from the agent is applied first
 	if w.cfg.PodSpecPatch != nil {
-		patched, err := PatchPodSpec(podSpec, w.cfg.PodSpecPatch)
+		patched, err := PatchPodSpec(podSpec, w.cfg.PodSpecPatch, w.cfg.AllowPodSpecPatchRawCmdMod)
 		if err != nil {
 			return nil, fmt.Errorf("failed to apply podSpec patch from agent: %w", err)
 		}
@@ -671,7 +672,7 @@ func (w *worker) Build(podSpec *corev1.PodSpec, skipCheckout bool, inputs buildI
 	}
 
 	if inputs.k8sPlugin != nil && inputs.k8sPlugin.PodSpecPatch != nil {
-		patched, err := PatchPodSpec(podSpec, inputs.k8sPlugin.PodSpecPatch)
+		patched, err := PatchPodSpec(podSpec, inputs.k8sPlugin.PodSpecPatch, w.cfg.AllowPodSpecPatchRawCmdMod)
 		if err != nil {
 			return nil, fmt.Errorf("failed to apply podSpec patch from k8s plugin: %w", err)
 		}
@@ -686,12 +687,17 @@ func (w *worker) Build(podSpec *corev1.PodSpec, skipCheckout bool, inputs buildI
 
 var ErrNoCommandModification = errors.New("modifying container commands or args via podSpecPatch is not supported. Specify the command in the job's command field instead")
 
-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
+func PatchPodSpec(original, patch *corev1.PodSpec, allowCmdMod bool) (*corev1.PodSpec, error) {
+	if !allowCmdMod {
+		// 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...
+		// Unless they specifically configure the stack to allow it and have
+		// read the big red warning in the readme.
+		for _, c := range patch.Containers {
+			if len(c.Command) != 0 || len(c.Args) != 0 {
+				return nil, ErrNoCommandModification
+			}
 		}
 	}
 

diff --git a/internal/controller/scheduler/scheduler_test.go b/internal/controller/scheduler/scheduler_test.go
@@ -97,7 +97,7 @@ func TestPatchPodSpec(t *testing.T) {
 
 	for _, c := range cases {
 		t.Run(c.name, func(t *testing.T) {
-			result, err := scheduler.PatchPodSpec(c.podspec, c.patch)
+			result, err := scheduler.PatchPodSpec(c.podspec, c.patch, false)
 			if c.err != nil {
 				require.Error(t, err)
 				require.ErrorIs(t, c.err, err)