From b1ab75ecdc985c7b90fa0422b8801d443cf12698 Mon Sep 17 00:00:00 2001 From: Rob Best Date: Tue, 8 Sep 2020 12:36:36 +0100 Subject: [PATCH 1/3] Add prependedContainers --- docs/sidecar-configuration-format.md | 22 ++++++ internal/pkg/config/config.go | 41 ++++++++---- internal/pkg/config/config_test.go | 50 +++++++++----- internal/pkg/config/watcher/loader_test.go | 15 +++++ internal/pkg/testing/config.go | 21 +++--- pkg/server/webhook.go | 28 ++++++-- pkg/server/webhook_test.go | 3 + .../patch/prepended-containers.json | 67 +++++++++++++++++++ .../request/prepended-containers.yaml | 9 +++ .../k8s/configmap-prepended-containers.yaml | 29 ++++++++ test/fixtures/k8s/object8.yaml | 4 ++ test/fixtures/sidecars/inheritance-1.yaml | 4 ++ .../fixtures/sidecars/inheritance-deep-2.yaml | 4 ++ .../sidecars/prepended-containers.yaml | 21 ++++++ 14 files changed, 274 insertions(+), 44 deletions(-) create mode 100644 test/fixtures/k8s/admissioncontrol/patch/prepended-containers.json create mode 100644 test/fixtures/k8s/admissioncontrol/request/prepended-containers.yaml create mode 100644 test/fixtures/k8s/configmap-prepended-containers.yaml create mode 100644 test/fixtures/k8s/object8.yaml create mode 100644 test/fixtures/sidecars/prepended-containers.yaml diff --git a/docs/sidecar-configuration-format.md b/docs/sidecar-configuration-format.md index 17a1e00..74ba502 100644 --- a/docs/sidecar-configuration-format.md +++ b/docs/sidecar-configuration-format.md @@ -97,6 +97,28 @@ initContainers: - name: some-initcontainer image: init:1.12.2 imagePullPolicy: IfNotPresent + +# prependedContainers will be prepended to the top of the list of normal +# containers. This primarily allows exploitation of this workaround for ensuring +# sidecars finish starting before the other containers in the pod are launched: +# https://medium.com/@marko.luksa/delaying-application-start-until-sidecar-is-ready-2ec2d21a7b74 +prependedContainers: +- name: prepended-nginx-container + image: nginx:1.12.2 + imagePullPolicy: IfNotPresent + ports: + - containerPort: 80 + volumeMounts: + - name: nginx-conf + mountPath: /etc/nginx + lifecycle: + postStart: + exec: + command: + - /bin/sh + - -c + - | + while ! nc -w 1 127.0.0.1 80; do sleep 1; done ``` ## Configuring new sidecars diff --git a/internal/pkg/config/config.go b/internal/pkg/config/config.go index dc2d2ef..1820c36 100644 --- a/internal/pkg/config/config.go +++ b/internal/pkg/config/config.go @@ -32,17 +32,18 @@ var ( // InjectionConfig is a specific instance of a injected config, for a given annotation type InjectionConfig struct { - Name string `json:"name"` - Inherits string `json:"inherits"` - Containers []corev1.Container `json:"containers"` - Volumes []corev1.Volume `json:"volumes"` - Environment []corev1.EnvVar `json:"env"` - VolumeMounts []corev1.VolumeMount `json:"volumeMounts"` - HostAliases []corev1.HostAlias `json:"hostAliases"` - HostNetwork bool `json:"hostNetwork"` - HostPID bool `json:"hostPID"` - InitContainers []corev1.Container `json:"initContainers"` - ServiceAccountName string `json:"serviceAccountName"` + Name string `json:"name"` + Inherits string `json:"inherits"` + Containers []corev1.Container `json:"containers"` + Volumes []corev1.Volume `json:"volumes"` + Environment []corev1.EnvVar `json:"env"` + VolumeMounts []corev1.VolumeMount `json:"volumeMounts"` + HostAliases []corev1.HostAlias `json:"hostAliases"` + HostNetwork bool `json:"hostNetwork"` + HostPID bool `json:"hostPID"` + InitContainers []corev1.Container `json:"initContainers"` + ServiceAccountName string `json:"serviceAccountName"` + PrependedContainers []corev1.Container `json:"prependedContainers"` version string } @@ -68,7 +69,7 @@ func (c *InjectionConfig) String() string { return fmt.Sprintf("%s%s: %d containers, %d init containers, %d volumes, %d environment vars, %d volume mounts, %d host aliases%s", c.FullName(), inheritsString, - len(c.Containers), + len(c.Containers)+len(c.PrependedContainers), len(c.InitContainers), len(c.Volumes), len(c.Environment), @@ -272,6 +273,22 @@ func (c *InjectionConfig) Merge(child *InjectionConfig) error { } } + // merge prepended containers + for _, cv := range child.PrependedContainers { + contains := false + + for bi, bv := range c.PrependedContainers { + if bv.Name == cv.Name { + contains = true + c.PrependedContainers[bi] = cv + } + } + + if !contains { + c.PrependedContainers = append(c.PrependedContainers, cv) + } + } + // merge serviceAccount settings to the left if child.ServiceAccountName != "" { c.ServiceAccountName = child.ServiceAccountName diff --git a/internal/pkg/config/config_test.go b/internal/pkg/config/config_test.go index c097c5c..2cacc83 100644 --- a/internal/pkg/config/config_test.go +++ b/internal/pkg/config/config_test.go @@ -112,27 +112,29 @@ var ( }, // test simple inheritance "simple inheritance from complex-sidecar": testhelper.ConfigExpectation{ - Name: "inheritance-complex", - Version: "v1", - Path: fixtureSidecarsDir + "/inheritance-1.yaml", - EnvCount: 2, - ContainerCount: 5, - VolumeCount: 2, - VolumeMountCount: 0, - HostAliasCount: 1, - InitContainerCount: 1, + Name: "inheritance-complex", + Version: "v1", + Path: fixtureSidecarsDir + "/inheritance-1.yaml", + EnvCount: 2, + ContainerCount: 5, + VolumeCount: 2, + VolumeMountCount: 0, + HostAliasCount: 1, + InitContainerCount: 1, + PrependedContainerCount: 1, }, // test deep inheritance "deep inheritance from inheritance-complex": testhelper.ConfigExpectation{ - Name: "inheritance-deep", - Version: "v2", - Path: fixtureSidecarsDir + "/inheritance-deep-2.yaml", - EnvCount: 3, - ContainerCount: 6, - VolumeCount: 3, - VolumeMountCount: 0, - HostAliasCount: 3, - InitContainerCount: 2, + Name: "inheritance-deep", + Version: "v2", + Path: fixtureSidecarsDir + "/inheritance-deep-2.yaml", + EnvCount: 3, + ContainerCount: 6, + VolumeCount: 3, + VolumeMountCount: 0, + HostAliasCount: 3, + InitContainerCount: 2, + PrependedContainerCount: 2, }, "service-account": testhelper.ConfigExpectation{ Name: "service-account", @@ -194,6 +196,18 @@ var ( HostNetwork: true, HostPID: true, }, + "prepended-containers": testhelper.ConfigExpectation{ + Name: "prepended-containers", + Version: "latest", + Path: fixtureSidecarsDir + "/prepended-containers.yaml", + EnvCount: 0, + ContainerCount: 2, + VolumeCount: 0, + VolumeMountCount: 0, + HostAliasCount: 0, + InitContainerCount: 0, + PrependedContainerCount: 2, + }, } ) diff --git a/internal/pkg/config/watcher/loader_test.go b/internal/pkg/config/watcher/loader_test.go index edce1a5..47e900f 100644 --- a/internal/pkg/config/watcher/loader_test.go +++ b/internal/pkg/config/watcher/loader_test.go @@ -133,6 +133,21 @@ var ( InitContainerCount: 1, }, }, + + "configmap-prepended-containers": []testhelper.ConfigExpectation{ + testhelper.ConfigExpectation{ + Name: "prepended-containers", + Version: "latest", + Path: fixtureSidecarsDir + "/prepended-containers.yaml", + VolumeCount: 0, + EnvCount: 0, + ContainerCount: 2, + VolumeMountCount: 0, + HostAliasCount: 0, + InitContainerCount: 0, + PrependedContainerCount: 2, + }, + }, } ) diff --git a/internal/pkg/testing/config.go b/internal/pkg/testing/config.go index 5655b0e..8d6fee5 100644 --- a/internal/pkg/testing/config.go +++ b/internal/pkg/testing/config.go @@ -12,16 +12,17 @@ type ConfigExpectation struct { // version is the parsed version string, or "latest" if omitted Version string // Path is the path to the YAML to load the sidecar yaml from - Path string - EnvCount int - ContainerCount int - VolumeCount int - VolumeMountCount int - HostAliasCount int - HostNetwork bool - HostPID bool - InitContainerCount int - ServiceAccount string + Path string + EnvCount int + ContainerCount int + VolumeCount int + VolumeMountCount int + HostAliasCount int + HostNetwork bool + HostPID bool + InitContainerCount int + ServiceAccount string + PrependedContainerCount int // LoadError is an error, if any, that is expected during load LoadError error diff --git a/pkg/server/webhook.go b/pkg/server/webhook.go index 2142ae7..8b0809e 100644 --- a/pkg/server/webhook.go +++ b/pkg/server/webhook.go @@ -210,10 +210,27 @@ func setEnvironment(target []corev1.Container, addedEnv []corev1.EnvVar, basePat return patch } -func addContainers(target, added []corev1.Container, basePath string) (patch []patchOperation) { +func addContainers(target, prepended, appended []corev1.Container, basePath string) (patch []patchOperation) { first := len(target) == 0 var value interface{} - for _, add := range added { + for i := len(prepended) - 1; i >= 0; i-- { + add := prepended[i] + value = add + path := basePath + if first { + first = false + value = []corev1.Container{add} + } else { + path = path + "/0" + } + patch = append(patch, patchOperation{ + Op: "add", + Path: path, + Value: value, + }) + } + + for _, add := range appended { value = add path := basePath if first { @@ -464,7 +481,7 @@ func createPatch(pod *corev1.Pod, inj *config.InjectionConfig, annotations map[s // this mutates inj.InitContainers with our environment vars mutatedInjectedInitContainers := mergeEnvVars(inj.Environment, inj.InitContainers) mutatedInjectedInitContainers = mergeVolumeMounts(inj.VolumeMounts, mutatedInjectedInitContainers) - patch = append(patch, addContainers(pod.Spec.InitContainers, mutatedInjectedInitContainers, "/spec/initContainers")...) + patch = append(patch, addContainers(pod.Spec.InitContainers, []corev1.Container{}, mutatedInjectedInitContainers, "/spec/initContainers")...) } { // container injections @@ -475,7 +492,10 @@ func createPatch(pod *corev1.Pod, inj *config.InjectionConfig, annotations map[s // this mutates inj.Containers with our environment vars mutatedInjectedContainers := mergeEnvVars(inj.Environment, inj.Containers) mutatedInjectedContainers = mergeVolumeMounts(inj.VolumeMounts, mutatedInjectedContainers) - patch = append(patch, addContainers(pod.Spec.Containers, mutatedInjectedContainers, "/spec/containers")...) + mutatedInjectedPrependedContainers := mergeEnvVars(inj.Environment, inj.PrependedContainers) + mutatedInjectedPrependedContainers = mergeVolumeMounts(inj.VolumeMounts, mutatedInjectedPrependedContainers) + // then, add containers to the patch + patch = append(patch, addContainers(pod.Spec.Containers, mutatedInjectedPrependedContainers, mutatedInjectedContainers, "/spec/containers")...) } { // pod level mutations diff --git a/pkg/server/webhook_test.go b/pkg/server/webhook_test.go index 4f0d58c..e3be74e 100644 --- a/pkg/server/webhook_test.go +++ b/pkg/server/webhook_test.go @@ -33,6 +33,7 @@ var ( obj7 = "test/fixtures/k8s/object7.yaml" obj7v2 = "test/fixtures/k8s/object7-v2.yaml" obj7v3 = "test/fixtures/k8s/object7-badrequestformat.yaml" + obj8 = "test/fixtures/k8s/object8.yaml" ignoredNamespace = "test/fixtures/k8s/ignored-namespace-pod.yaml" badSidecar = "test/fixtures/k8s/bad-sidecar.yaml" @@ -51,6 +52,7 @@ var ( {configuration: obj7, expectedSidecar: "init-containers:latest"}, {configuration: obj7v2, expectedSidecar: "init-containers:v2"}, {configuration: obj7v3, expectedSidecar: "", expectedError: ErrRequestedSidecarNotFound}, + {configuration: obj8, expectedSidecar: "prepended-containers:latest"}, {configuration: ignoredNamespace, expectedSidecar: "", expectedError: ErrSkipIgnoredNamespace}, {configuration: badSidecar, expectedSidecar: "", expectedError: ErrRequestedSidecarNotFound}, } @@ -60,6 +62,7 @@ var ( {name: "missing-sidecar-config", allowed: true, patchExpected: false}, {name: "sidecar-test-1", allowed: true, patchExpected: true}, {name: "env-override", allowed: true, patchExpected: true}, + {name: "prepended-containers", allowed: true, patchExpected: true}, {name: "service-account", allowed: true, patchExpected: true}, {name: "service-account-already-set", allowed: true, patchExpected: true}, {name: "service-account-set-default", allowed: true, patchExpected: true}, diff --git a/test/fixtures/k8s/admissioncontrol/patch/prepended-containers.json b/test/fixtures/k8s/admissioncontrol/patch/prepended-containers.json new file mode 100644 index 0000000..8243409 --- /dev/null +++ b/test/fixtures/k8s/admissioncontrol/patch/prepended-containers.json @@ -0,0 +1,67 @@ +[ + { + "op": "add", + "path": "/spec/containers", + "value": [ + { + "image": "alpine:latest", + "name": "prepended-container-2", + "ports": [ + { + "containerPort": 1234 + } + ], + "resources": {} + } + ] + }, + { + "op": "add", + "path": "/spec/containers/0", + "value": { + "image": "foobar:2020", + "imagePullPolicy": "IfNotPresent", + "name": "prepended-container-1", + "ports": [ + { + "containerPort": 666 + } + ], + "resources": {} + } + }, + { + "op": "add", + "path": "/spec/containers/-", + "value": { + "image": "nginx:1.12.2", + "imagePullPolicy": "IfNotPresent", + "name": "sidecar-add-vm", + "ports": [ + { + "containerPort": 80 + } + ], + "resources": {} + } + }, + { + "op": "add", + "path": "/spec/containers/-", + "value": { + "image": "foo:69", + "name": "sidecar-existing-vm", + "ports": [ + { + "containerPort": 420 + } + ], + "resources": {} + } + }, + { + "op": "add", + "path": "/metadata/annotations/injector.unittest.com~1status", + "value": "injected" + } +] diff --git a/test/fixtures/k8s/admissioncontrol/request/prepended-containers.yaml b/test/fixtures/k8s/admissioncontrol/request/prepended-containers.yaml new file mode 100644 index 0000000..bd2386a --- /dev/null +++ b/test/fixtures/k8s/admissioncontrol/request/prepended-containers.yaml @@ -0,0 +1,9 @@ +--- +# this is an AdmissionRequest object +# https://godoc.org/k8s.io/api/admission/v1beta1#AdmissionRequest +object: + metadata: + annotations: + injector.unittest.com/request: "prepended-containers" + spec: + containers: [] diff --git a/test/fixtures/k8s/configmap-prepended-containers.yaml b/test/fixtures/k8s/configmap-prepended-containers.yaml new file mode 100644 index 0000000..63c5306 --- /dev/null +++ b/test/fixtures/k8s/configmap-prepended-containers.yaml @@ -0,0 +1,29 @@ +--- +apiVersion: v1 +kind: ConfigMap +metadata: + name: test-prepended-containers + namespace: default +data: + test-tumblr1: | + name: prepended-containers + containers: + - name: sidecar-add-vm + image: nginx:1.12.2 + imagePullPolicy: IfNotPresent + ports: + - containerPort: 80 + - name: sidecar-existing-vm + image: foo:69 + ports: + - containerPort: 420 + prependedContainers: + - name: prepended-container-1 + image: foobar:2020 + imagePullPolicy: IfNotPresent + ports: + - containerPort: 666 + - name: prepended-container-2 + image: alpine:latest + ports: + - containerPort: 1234 diff --git a/test/fixtures/k8s/object8.yaml b/test/fixtures/k8s/object8.yaml new file mode 100644 index 0000000..4365839 --- /dev/null +++ b/test/fixtures/k8s/object8.yaml @@ -0,0 +1,4 @@ +name: object8 +namespace: unittest +annotations: + "injector.unittest.com/request": "prepended-containers" diff --git a/test/fixtures/sidecars/inheritance-1.yaml b/test/fixtures/sidecars/inheritance-1.yaml index 97beacf..13352cd 100644 --- a/test/fixtures/sidecars/inheritance-1.yaml +++ b/test/fixtures/sidecars/inheritance-1.yaml @@ -40,3 +40,7 @@ containers: initContainers: - name: a-new-image image: some-value + +prependedContainers: + - name: a-new-image + image: some-value diff --git a/test/fixtures/sidecars/inheritance-deep-2.yaml b/test/fixtures/sidecars/inheritance-deep-2.yaml index 87f9640..c39c6d3 100644 --- a/test/fixtures/sidecars/inheritance-deep-2.yaml +++ b/test/fixtures/sidecars/inheritance-deep-2.yaml @@ -43,3 +43,7 @@ containers: initContainers: - name: a-new-image-2 image: some-value + +prependedContainers: + - name: a-new-image-2 + image: some-value diff --git a/test/fixtures/sidecars/prepended-containers.yaml b/test/fixtures/sidecars/prepended-containers.yaml new file mode 100644 index 0000000..76c104a --- /dev/null +++ b/test/fixtures/sidecars/prepended-containers.yaml @@ -0,0 +1,21 @@ +name: prepended-containers +containers: + - name: sidecar-add-vm + image: nginx:1.12.2 + imagePullPolicy: IfNotPresent + ports: + - containerPort: 80 + - name: sidecar-existing-vm + image: foo:69 + ports: + - containerPort: 420 +prependedContainers: + - name: prepended-container-1 + image: foobar:2020 + imagePullPolicy: IfNotPresent + ports: + - containerPort: 666 + - name: prepended-container-2 + image: alpine:latest + ports: + - containerPort: 1234 From 73bc2f142385fe422c305c61ee06357bc98ae805 Mon Sep 17 00:00:00 2001 From: Rob Best Date: Fri, 11 Sep 2020 08:53:35 +0100 Subject: [PATCH 2/3] Replace prependedContainers list with prependContainers bool This bool modifies the behaviour of the container injection (prepend, append). --- docs/sidecar-configuration-format.md | 29 ++------ internal/pkg/config/config.go | 42 ++++-------- internal/pkg/config/config_test.go | 60 ++++++++--------- internal/pkg/config/watcher/loader_test.go | 22 +++--- internal/pkg/testing/config.go | 22 +++--- pkg/server/webhook.go | 27 +++++--- pkg/server/webhook_test.go | 4 +- .../patch/prepend-containers.json | 38 +++++++++++ .../patch/prepended-containers.json | 67 ------------------- ...ontainers.yaml => prepend-containers.yaml} | 2 +- .../k8s/configmap-prepend-containers.yaml | 20 ++++++ .../k8s/configmap-prepended-containers.yaml | 29 -------- test/fixtures/k8s/object8.yaml | 2 +- test/fixtures/sidecars/inheritance-1.yaml | 4 -- .../fixtures/sidecars/inheritance-deep-2.yaml | 4 -- .../fixtures/sidecars/prepend-containers.yaml | 12 ++++ .../sidecars/prepended-containers.yaml | 21 ------ 17 files changed, 162 insertions(+), 243 deletions(-) create mode 100644 test/fixtures/k8s/admissioncontrol/patch/prepend-containers.json delete mode 100644 test/fixtures/k8s/admissioncontrol/patch/prepended-containers.json rename test/fixtures/k8s/admissioncontrol/request/{prepended-containers.yaml => prepend-containers.yaml} (74%) create mode 100644 test/fixtures/k8s/configmap-prepend-containers.yaml delete mode 100644 test/fixtures/k8s/configmap-prepended-containers.yaml create mode 100644 test/fixtures/sidecars/prepend-containers.yaml delete mode 100644 test/fixtures/sidecars/prepended-containers.yaml diff --git a/docs/sidecar-configuration-format.md b/docs/sidecar-configuration-format.md index 74ba502..32faef3 100644 --- a/docs/sidecar-configuration-format.md +++ b/docs/sidecar-configuration-format.md @@ -38,6 +38,13 @@ name: "test:v1.2" # NOTE: this is relative to the current file, and does not allow for absolute pathing! inherits: "some-sidecar.yaml" +# prependContainers modifies the behaviour of the injector so that containers +# are injected at the top of the list of normal containers, rather than the +# bottom. Defaults to `false`. This primarily allows exploitation of this workaround for +# ensuring sidecars finish starting before the other containers in the pod are launched: +# https://medium.com/@marko.luksa/delaying-application-start-until-sidecar-is-ready-2ec2d21a7b74 +prependContainers: false + containers: # we inject a nginx container - name: sidecar-nginx @@ -97,28 +104,6 @@ initContainers: - name: some-initcontainer image: init:1.12.2 imagePullPolicy: IfNotPresent - -# prependedContainers will be prepended to the top of the list of normal -# containers. This primarily allows exploitation of this workaround for ensuring -# sidecars finish starting before the other containers in the pod are launched: -# https://medium.com/@marko.luksa/delaying-application-start-until-sidecar-is-ready-2ec2d21a7b74 -prependedContainers: -- name: prepended-nginx-container - image: nginx:1.12.2 - imagePullPolicy: IfNotPresent - ports: - - containerPort: 80 - volumeMounts: - - name: nginx-conf - mountPath: /etc/nginx - lifecycle: - postStart: - exec: - command: - - /bin/sh - - -c - - | - while ! nc -w 1 127.0.0.1 80; do sleep 1; done ``` ## Configuring new sidecars diff --git a/internal/pkg/config/config.go b/internal/pkg/config/config.go index 1820c36..2fa243d 100644 --- a/internal/pkg/config/config.go +++ b/internal/pkg/config/config.go @@ -32,18 +32,18 @@ var ( // InjectionConfig is a specific instance of a injected config, for a given annotation type InjectionConfig struct { - Name string `json:"name"` - Inherits string `json:"inherits"` - Containers []corev1.Container `json:"containers"` - Volumes []corev1.Volume `json:"volumes"` - Environment []corev1.EnvVar `json:"env"` - VolumeMounts []corev1.VolumeMount `json:"volumeMounts"` - HostAliases []corev1.HostAlias `json:"hostAliases"` - HostNetwork bool `json:"hostNetwork"` - HostPID bool `json:"hostPID"` - InitContainers []corev1.Container `json:"initContainers"` - ServiceAccountName string `json:"serviceAccountName"` - PrependedContainers []corev1.Container `json:"prependedContainers"` + Name string `json:"name"` + Inherits string `json:"inherits"` + Containers []corev1.Container `json:"containers"` + Volumes []corev1.Volume `json:"volumes"` + Environment []corev1.EnvVar `json:"env"` + VolumeMounts []corev1.VolumeMount `json:"volumeMounts"` + HostAliases []corev1.HostAlias `json:"hostAliases"` + HostNetwork bool `json:"hostNetwork"` + HostPID bool `json:"hostPID"` + InitContainers []corev1.Container `json:"initContainers"` + ServiceAccountName string `json:"serviceAccountName"` + PrependContainers bool `json:"prependContainers"` version string } @@ -69,7 +69,7 @@ func (c *InjectionConfig) String() string { return fmt.Sprintf("%s%s: %d containers, %d init containers, %d volumes, %d environment vars, %d volume mounts, %d host aliases%s", c.FullName(), inheritsString, - len(c.Containers)+len(c.PrependedContainers), + len(c.Containers), len(c.InitContainers), len(c.Volumes), len(c.Environment), @@ -273,22 +273,6 @@ func (c *InjectionConfig) Merge(child *InjectionConfig) error { } } - // merge prepended containers - for _, cv := range child.PrependedContainers { - contains := false - - for bi, bv := range c.PrependedContainers { - if bv.Name == cv.Name { - contains = true - c.PrependedContainers[bi] = cv - } - } - - if !contains { - c.PrependedContainers = append(c.PrependedContainers, cv) - } - } - // merge serviceAccount settings to the left if child.ServiceAccountName != "" { c.ServiceAccountName = child.ServiceAccountName diff --git a/internal/pkg/config/config_test.go b/internal/pkg/config/config_test.go index 2cacc83..08f5664 100644 --- a/internal/pkg/config/config_test.go +++ b/internal/pkg/config/config_test.go @@ -112,29 +112,27 @@ var ( }, // test simple inheritance "simple inheritance from complex-sidecar": testhelper.ConfigExpectation{ - Name: "inheritance-complex", - Version: "v1", - Path: fixtureSidecarsDir + "/inheritance-1.yaml", - EnvCount: 2, - ContainerCount: 5, - VolumeCount: 2, - VolumeMountCount: 0, - HostAliasCount: 1, - InitContainerCount: 1, - PrependedContainerCount: 1, + Name: "inheritance-complex", + Version: "v1", + Path: fixtureSidecarsDir + "/inheritance-1.yaml", + EnvCount: 2, + ContainerCount: 5, + VolumeCount: 2, + VolumeMountCount: 0, + HostAliasCount: 1, + InitContainerCount: 1, }, // test deep inheritance "deep inheritance from inheritance-complex": testhelper.ConfigExpectation{ - Name: "inheritance-deep", - Version: "v2", - Path: fixtureSidecarsDir + "/inheritance-deep-2.yaml", - EnvCount: 3, - ContainerCount: 6, - VolumeCount: 3, - VolumeMountCount: 0, - HostAliasCount: 3, - InitContainerCount: 2, - PrependedContainerCount: 2, + Name: "inheritance-deep", + Version: "v2", + Path: fixtureSidecarsDir + "/inheritance-deep-2.yaml", + EnvCount: 3, + ContainerCount: 6, + VolumeCount: 3, + VolumeMountCount: 0, + HostAliasCount: 3, + InitContainerCount: 2, }, "service-account": testhelper.ConfigExpectation{ Name: "service-account", @@ -196,17 +194,17 @@ var ( HostNetwork: true, HostPID: true, }, - "prepended-containers": testhelper.ConfigExpectation{ - Name: "prepended-containers", - Version: "latest", - Path: fixtureSidecarsDir + "/prepended-containers.yaml", - EnvCount: 0, - ContainerCount: 2, - VolumeCount: 0, - VolumeMountCount: 0, - HostAliasCount: 0, - InitContainerCount: 0, - PrependedContainerCount: 2, + "prepend-containers": testhelper.ConfigExpectation{ + Name: "prepend-containers", + Version: "latest", + Path: fixtureSidecarsDir + "/prepend-containers.yaml", + EnvCount: 0, + ContainerCount: 2, + VolumeCount: 0, + VolumeMountCount: 0, + HostAliasCount: 0, + InitContainerCount: 0, + PrependContainers: true, }, } ) diff --git a/internal/pkg/config/watcher/loader_test.go b/internal/pkg/config/watcher/loader_test.go index 47e900f..0d165ac 100644 --- a/internal/pkg/config/watcher/loader_test.go +++ b/internal/pkg/config/watcher/loader_test.go @@ -134,18 +134,18 @@ var ( }, }, - "configmap-prepended-containers": []testhelper.ConfigExpectation{ + "configmap-prepend-containers": []testhelper.ConfigExpectation{ testhelper.ConfigExpectation{ - Name: "prepended-containers", - Version: "latest", - Path: fixtureSidecarsDir + "/prepended-containers.yaml", - VolumeCount: 0, - EnvCount: 0, - ContainerCount: 2, - VolumeMountCount: 0, - HostAliasCount: 0, - InitContainerCount: 0, - PrependedContainerCount: 2, + Name: "prepend-containers", + Version: "latest", + Path: fixtureSidecarsDir + "/prepend-containers.yaml", + VolumeCount: 0, + EnvCount: 0, + ContainerCount: 2, + VolumeMountCount: 0, + HostAliasCount: 0, + InitContainerCount: 0, + PrependContainers: true, }, }, } diff --git a/internal/pkg/testing/config.go b/internal/pkg/testing/config.go index 8d6fee5..fefa78f 100644 --- a/internal/pkg/testing/config.go +++ b/internal/pkg/testing/config.go @@ -12,17 +12,17 @@ type ConfigExpectation struct { // version is the parsed version string, or "latest" if omitted Version string // Path is the path to the YAML to load the sidecar yaml from - Path string - EnvCount int - ContainerCount int - VolumeCount int - VolumeMountCount int - HostAliasCount int - HostNetwork bool - HostPID bool - InitContainerCount int - ServiceAccount string - PrependedContainerCount int + Path string + EnvCount int + ContainerCount int + VolumeCount int + VolumeMountCount int + HostAliasCount int + HostNetwork bool + HostPID bool + InitContainerCount int + ServiceAccount string + PrependContainers bool // LoadError is an error, if any, that is expected during load LoadError error diff --git a/pkg/server/webhook.go b/pkg/server/webhook.go index 8b0809e..134b8fb 100644 --- a/pkg/server/webhook.go +++ b/pkg/server/webhook.go @@ -210,18 +210,17 @@ func setEnvironment(target []corev1.Container, addedEnv []corev1.EnvVar, basePat return patch } -func addContainers(target, prepended, appended []corev1.Container, basePath string) (patch []patchOperation) { +func addContainers(target, added []corev1.Container, basePath string) (patch []patchOperation) { first := len(target) == 0 var value interface{} - for i := len(prepended) - 1; i >= 0; i-- { - add := prepended[i] + for _, add := range added { value = add path := basePath if first { first = false value = []corev1.Container{add} } else { - path = path + "/0" + path = path + "/-" } patch = append(patch, patchOperation{ Op: "add", @@ -229,15 +228,21 @@ func addContainers(target, prepended, appended []corev1.Container, basePath stri Value: value, }) } + return patch +} - for _, add := range appended { +func prependContainers(target, added []corev1.Container, basePath string) (patch []patchOperation) { + first := len(target) == 0 + var value interface{} + for i := len(added) - 1; i >= 0; i-- { + add := added[i] value = add path := basePath if first { first = false value = []corev1.Container{add} } else { - path = path + "/-" + path = path + "/0" } patch = append(patch, patchOperation{ Op: "add", @@ -481,7 +486,7 @@ func createPatch(pod *corev1.Pod, inj *config.InjectionConfig, annotations map[s // this mutates inj.InitContainers with our environment vars mutatedInjectedInitContainers := mergeEnvVars(inj.Environment, inj.InitContainers) mutatedInjectedInitContainers = mergeVolumeMounts(inj.VolumeMounts, mutatedInjectedInitContainers) - patch = append(patch, addContainers(pod.Spec.InitContainers, []corev1.Container{}, mutatedInjectedInitContainers, "/spec/initContainers")...) + patch = append(patch, addContainers(pod.Spec.InitContainers, mutatedInjectedInitContainers, "/spec/initContainers")...) } { // container injections @@ -492,10 +497,12 @@ func createPatch(pod *corev1.Pod, inj *config.InjectionConfig, annotations map[s // this mutates inj.Containers with our environment vars mutatedInjectedContainers := mergeEnvVars(inj.Environment, inj.Containers) mutatedInjectedContainers = mergeVolumeMounts(inj.VolumeMounts, mutatedInjectedContainers) - mutatedInjectedPrependedContainers := mergeEnvVars(inj.Environment, inj.PrependedContainers) - mutatedInjectedPrependedContainers = mergeVolumeMounts(inj.VolumeMounts, mutatedInjectedPrependedContainers) // then, add containers to the patch - patch = append(patch, addContainers(pod.Spec.Containers, mutatedInjectedPrependedContainers, mutatedInjectedContainers, "/spec/containers")...) + if inj.PrependContainers { + patch = append(patch, prependContainers(pod.Spec.Containers, mutatedInjectedContainers, "/spec/containers")...) + } else { + patch = append(patch, addContainers(pod.Spec.Containers, mutatedInjectedContainers, "/spec/containers")...) + } } { // pod level mutations diff --git a/pkg/server/webhook_test.go b/pkg/server/webhook_test.go index e3be74e..e13ed31 100644 --- a/pkg/server/webhook_test.go +++ b/pkg/server/webhook_test.go @@ -52,7 +52,7 @@ var ( {configuration: obj7, expectedSidecar: "init-containers:latest"}, {configuration: obj7v2, expectedSidecar: "init-containers:v2"}, {configuration: obj7v3, expectedSidecar: "", expectedError: ErrRequestedSidecarNotFound}, - {configuration: obj8, expectedSidecar: "prepended-containers:latest"}, + {configuration: obj8, expectedSidecar: "prepend-containers:latest"}, {configuration: ignoredNamespace, expectedSidecar: "", expectedError: ErrSkipIgnoredNamespace}, {configuration: badSidecar, expectedSidecar: "", expectedError: ErrRequestedSidecarNotFound}, } @@ -62,7 +62,7 @@ var ( {name: "missing-sidecar-config", allowed: true, patchExpected: false}, {name: "sidecar-test-1", allowed: true, patchExpected: true}, {name: "env-override", allowed: true, patchExpected: true}, - {name: "prepended-containers", allowed: true, patchExpected: true}, + {name: "prepend-containers", allowed: true, patchExpected: true}, {name: "service-account", allowed: true, patchExpected: true}, {name: "service-account-already-set", allowed: true, patchExpected: true}, {name: "service-account-set-default", allowed: true, patchExpected: true}, diff --git a/test/fixtures/k8s/admissioncontrol/patch/prepend-containers.json b/test/fixtures/k8s/admissioncontrol/patch/prepend-containers.json new file mode 100644 index 0000000..da6bebe --- /dev/null +++ b/test/fixtures/k8s/admissioncontrol/patch/prepend-containers.json @@ -0,0 +1,38 @@ +[ + { + "op": "add", + "path": "/spec/containers", + "value": [ + { + "image": "foo:69", + "name": "sidecar-existing-vm", + "ports": [ + { + "containerPort": 420 + } + ], + "resources": {} + } + ] + }, + { + "op": "add", + "path": "/spec/containers/0", + "value": { + "image": "nginx:1.12.2", + "imagePullPolicy": "IfNotPresent", + "name": "sidecar-add-vm", + "ports": [ + { + "containerPort": 80 + } + ], + "resources": {} + } + }, + { + "op": "add", + "path": "/metadata/annotations/injector.unittest.com~1status", + "value": "injected" + } +] diff --git a/test/fixtures/k8s/admissioncontrol/patch/prepended-containers.json b/test/fixtures/k8s/admissioncontrol/patch/prepended-containers.json deleted file mode 100644 index 8243409..0000000 --- a/test/fixtures/k8s/admissioncontrol/patch/prepended-containers.json +++ /dev/null @@ -1,67 +0,0 @@ -[ - { - "op": "add", - "path": "/spec/containers", - "value": [ - { - "image": "alpine:latest", - "name": "prepended-container-2", - "ports": [ - { - "containerPort": 1234 - } - ], - "resources": {} - } - ] - }, - { - "op": "add", - "path": "/spec/containers/0", - "value": { - "image": "foobar:2020", - "imagePullPolicy": "IfNotPresent", - "name": "prepended-container-1", - "ports": [ - { - "containerPort": 666 - } - ], - "resources": {} - } - }, - { - "op": "add", - "path": "/spec/containers/-", - "value": { - "image": "nginx:1.12.2", - "imagePullPolicy": "IfNotPresent", - "name": "sidecar-add-vm", - "ports": [ - { - "containerPort": 80 - } - ], - "resources": {} - } - }, - { - "op": "add", - "path": "/spec/containers/-", - "value": { - "image": "foo:69", - "name": "sidecar-existing-vm", - "ports": [ - { - "containerPort": 420 - } - ], - "resources": {} - } - }, - { - "op": "add", - "path": "/metadata/annotations/injector.unittest.com~1status", - "value": "injected" - } -] diff --git a/test/fixtures/k8s/admissioncontrol/request/prepended-containers.yaml b/test/fixtures/k8s/admissioncontrol/request/prepend-containers.yaml similarity index 74% rename from test/fixtures/k8s/admissioncontrol/request/prepended-containers.yaml rename to test/fixtures/k8s/admissioncontrol/request/prepend-containers.yaml index bd2386a..db20215 100644 --- a/test/fixtures/k8s/admissioncontrol/request/prepended-containers.yaml +++ b/test/fixtures/k8s/admissioncontrol/request/prepend-containers.yaml @@ -4,6 +4,6 @@ object: metadata: annotations: - injector.unittest.com/request: "prepended-containers" + injector.unittest.com/request: "prepend-containers" spec: containers: [] diff --git a/test/fixtures/k8s/configmap-prepend-containers.yaml b/test/fixtures/k8s/configmap-prepend-containers.yaml new file mode 100644 index 0000000..fbc9a55 --- /dev/null +++ b/test/fixtures/k8s/configmap-prepend-containers.yaml @@ -0,0 +1,20 @@ +--- +apiVersion: v1 +kind: ConfigMap +metadata: + name: test-prepend-containers + namespace: default +data: + test-tumblr1: | + name: prepend-containers + containers: + - name: sidecar-add-vm + image: nginx:1.12.2 + imagePullPolicy: IfNotPresent + ports: + - containerPort: 80 + - name: sidecar-existing-vm + image: foo:69 + ports: + - containerPort: 420 + prependContainers: true diff --git a/test/fixtures/k8s/configmap-prepended-containers.yaml b/test/fixtures/k8s/configmap-prepended-containers.yaml deleted file mode 100644 index 63c5306..0000000 --- a/test/fixtures/k8s/configmap-prepended-containers.yaml +++ /dev/null @@ -1,29 +0,0 @@ ---- -apiVersion: v1 -kind: ConfigMap -metadata: - name: test-prepended-containers - namespace: default -data: - test-tumblr1: | - name: prepended-containers - containers: - - name: sidecar-add-vm - image: nginx:1.12.2 - imagePullPolicy: IfNotPresent - ports: - - containerPort: 80 - - name: sidecar-existing-vm - image: foo:69 - ports: - - containerPort: 420 - prependedContainers: - - name: prepended-container-1 - image: foobar:2020 - imagePullPolicy: IfNotPresent - ports: - - containerPort: 666 - - name: prepended-container-2 - image: alpine:latest - ports: - - containerPort: 1234 diff --git a/test/fixtures/k8s/object8.yaml b/test/fixtures/k8s/object8.yaml index 4365839..84e6219 100644 --- a/test/fixtures/k8s/object8.yaml +++ b/test/fixtures/k8s/object8.yaml @@ -1,4 +1,4 @@ name: object8 namespace: unittest annotations: - "injector.unittest.com/request": "prepended-containers" + "injector.unittest.com/request": "prepend-containers" diff --git a/test/fixtures/sidecars/inheritance-1.yaml b/test/fixtures/sidecars/inheritance-1.yaml index 13352cd..97beacf 100644 --- a/test/fixtures/sidecars/inheritance-1.yaml +++ b/test/fixtures/sidecars/inheritance-1.yaml @@ -40,7 +40,3 @@ containers: initContainers: - name: a-new-image image: some-value - -prependedContainers: - - name: a-new-image - image: some-value diff --git a/test/fixtures/sidecars/inheritance-deep-2.yaml b/test/fixtures/sidecars/inheritance-deep-2.yaml index c39c6d3..87f9640 100644 --- a/test/fixtures/sidecars/inheritance-deep-2.yaml +++ b/test/fixtures/sidecars/inheritance-deep-2.yaml @@ -43,7 +43,3 @@ containers: initContainers: - name: a-new-image-2 image: some-value - -prependedContainers: - - name: a-new-image-2 - image: some-value diff --git a/test/fixtures/sidecars/prepend-containers.yaml b/test/fixtures/sidecars/prepend-containers.yaml new file mode 100644 index 0000000..751811f --- /dev/null +++ b/test/fixtures/sidecars/prepend-containers.yaml @@ -0,0 +1,12 @@ +name: prepend-containers +containers: + - name: sidecar-add-vm + image: nginx:1.12.2 + imagePullPolicy: IfNotPresent + ports: + - containerPort: 80 + - name: sidecar-existing-vm + image: foo:69 + ports: + - containerPort: 420 +prependContainers: true diff --git a/test/fixtures/sidecars/prepended-containers.yaml b/test/fixtures/sidecars/prepended-containers.yaml deleted file mode 100644 index 76c104a..0000000 --- a/test/fixtures/sidecars/prepended-containers.yaml +++ /dev/null @@ -1,21 +0,0 @@ -name: prepended-containers -containers: - - name: sidecar-add-vm - image: nginx:1.12.2 - imagePullPolicy: IfNotPresent - ports: - - containerPort: 80 - - name: sidecar-existing-vm - image: foo:69 - ports: - - containerPort: 420 -prependedContainers: - - name: prepended-container-1 - image: foobar:2020 - imagePullPolicy: IfNotPresent - ports: - - containerPort: 666 - - name: prepended-container-2 - image: alpine:latest - ports: - - containerPort: 1234 From 4e875efa74721e2f8b5590ee9fb66a156784cf1e Mon Sep 17 00:00:00 2001 From: Rob Best Date: Fri, 11 Sep 2020 14:59:19 +0100 Subject: [PATCH 3/3] Rename addContainers to appendContainers --- pkg/server/webhook.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/server/webhook.go b/pkg/server/webhook.go index 134b8fb..f0903a7 100644 --- a/pkg/server/webhook.go +++ b/pkg/server/webhook.go @@ -210,7 +210,7 @@ func setEnvironment(target []corev1.Container, addedEnv []corev1.EnvVar, basePat return patch } -func addContainers(target, added []corev1.Container, basePath string) (patch []patchOperation) { +func appendContainers(target, added []corev1.Container, basePath string) (patch []patchOperation) { first := len(target) == 0 var value interface{} for _, add := range added { @@ -486,7 +486,7 @@ func createPatch(pod *corev1.Pod, inj *config.InjectionConfig, annotations map[s // this mutates inj.InitContainers with our environment vars mutatedInjectedInitContainers := mergeEnvVars(inj.Environment, inj.InitContainers) mutatedInjectedInitContainers = mergeVolumeMounts(inj.VolumeMounts, mutatedInjectedInitContainers) - patch = append(patch, addContainers(pod.Spec.InitContainers, mutatedInjectedInitContainers, "/spec/initContainers")...) + patch = append(patch, appendContainers(pod.Spec.InitContainers, mutatedInjectedInitContainers, "/spec/initContainers")...) } { // container injections @@ -501,7 +501,7 @@ func createPatch(pod *corev1.Pod, inj *config.InjectionConfig, annotations map[s if inj.PrependContainers { patch = append(patch, prependContainers(pod.Spec.Containers, mutatedInjectedContainers, "/spec/containers")...) } else { - patch = append(patch, addContainers(pod.Spec.Containers, mutatedInjectedContainers, "/spec/containers")...) + patch = append(patch, appendContainers(pod.Spec.Containers, mutatedInjectedContainers, "/spec/containers")...) } }