From 80c050ed7f17e42a958d492a20c3ff721e446deb Mon Sep 17 00:00:00 2001 From: Thorsten Klein Date: Tue, 5 Mar 2019 14:47:21 +0100 Subject: [PATCH 1/7] Simplify test code and add stuff for initContainers --- internal/pkg/config/config.go | 15 +- internal/pkg/config/config_test.go | 340 ++++++++++++----------------- 2 files changed, 142 insertions(+), 213 deletions(-) diff --git a/internal/pkg/config/config.go b/internal/pkg/config/config.go index 9a1986f..0778397 100644 --- a/internal/pkg/config/config.go +++ b/internal/pkg/config/config.go @@ -30,12 +30,13 @@ var ( // InjectionConfig is a specific instance of a injected config, for a given annotation type InjectionConfig struct { - Name string `json:"name"` - 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"` + Name string `json:"name"` + 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"` + InitContainers []corev1.Container `json:"initContainers"` } // Config is a struct indicating how a given injection should be configured @@ -47,7 +48,7 @@ type Config struct { // String returns a string representation of the config func (c *InjectionConfig) String() string { - return fmt.Sprintf("%s: %d containers, %d volumes, %d environment vars, %d volume mounts, %d host aliases", c.Name, len(c.Containers), len(c.Volumes), len(c.Environment), len(c.VolumeMounts), len(c.HostAliases)) + return fmt.Sprintf("%s: %d containers, %d init containers, %d volumes, %d environment vars, %d volume mounts, %d host aliases", c.Name, len(c.Containers), len(c.InitContainers), len(c.Volumes), len(c.Environment), len(c.VolumeMounts), len(c.HostAliases)) } // ReplaceInjectionConfigs will take a list of new InjectionConfigs, and replace the current configuration with them. diff --git a/internal/pkg/config/config_test.go b/internal/pkg/config/config_test.go index e9f50ac..42929e2 100644 --- a/internal/pkg/config/config_test.go +++ b/internal/pkg/config/config_test.go @@ -6,18 +6,127 @@ import ( _ "github.com/tumblr/k8s-sidecar-injector/internal/pkg/testing" ) +// config struct for testing: where to find the file and what we expect to find in it +type configExpectation struct { + name string + path string + expectedEnvVarCount int + expectedContainerCount int + expectedVolumeCount int + expectedVolumeMountCount int + expectedHostAliasCount int + expectedInitContainerCount int +} + var ( - sidecars = "test/fixtures/sidecars" - cfg1 = sidecars + "/sidecar-test.yaml" - complicatedConfig = sidecars + "/complex-sidecar.yaml" - env1 = sidecars + "/env1.yaml" - volumeMounts = sidecars + "/volume-mounts.yaml" - hostAliases = sidecars + "/host-aliases.yaml" + // location of the fixture sidecar files + fixtureSidecarsDir = "test/fixtures/sidecars" + + // test files and expectations + testConfigs = map[string]configExpectation{ + "sidecar-test": configExpectation{ + name: "sidecar-test", + path: fixtureSidecarsDir + "/sidecar-test.yaml", + expectedEnvVarCount: 2, + expectedContainerCount: 2, + expectedVolumeCount: 1, + expectedVolumeMountCount: 0, + expectedHostAliasCount: 0, + expectedInitContainerCount: 0, + }, + "complex-sidecar": configExpectation{ + name: "complex-sidecar", + path: fixtureSidecarsDir + "/complex-sidecar.yaml", + expectedEnvVarCount: 0, + expectedContainerCount: 4, + expectedVolumeCount: 1, + expectedVolumeMountCount: 0, + expectedHostAliasCount: 0, + expectedInitContainerCount: 0, + }, + "env1": configExpectation{ + name: "env1", + path: fixtureSidecarsDir + "/env1.yaml", + expectedEnvVarCount: 3, + expectedContainerCount: 0, + expectedVolumeCount: 0, + expectedVolumeMountCount: 0, + expectedHostAliasCount: 0, + expectedInitContainerCount: 0, + }, + "volume-mounts": configExpectation{ + name: "volume-mounts", + path: fixtureSidecarsDir + "/volume-mounts.yaml", + expectedEnvVarCount: 2, + expectedContainerCount: 3, + expectedVolumeCount: 2, + expectedVolumeMountCount: 1, + expectedHostAliasCount: 0, + expectedInitContainerCount: 0, + }, + "host-aliases": configExpectation{ + name: "host-aliases", + path: fixtureSidecarsDir + "/host-aliases.yaml", + expectedEnvVarCount: 2, + expectedContainerCount: 1, + expectedVolumeCount: 0, + expectedVolumeMountCount: 0, + expectedHostAliasCount: 6, + expectedInitContainerCount: 0, + }, + "init-containers": configExpectation{ + name: "init-containers", + path: fixtureSidecarsDir + "/init-containers.yaml", + expectedEnvVarCount: 0, + expectedContainerCount: 2, + expectedVolumeCount: 0, + expectedVolumeMountCount: 0, + expectedHostAliasCount: 0, + expectedInitContainerCount: 1, + }, + } ) +// TestConfigs: load configs from filepath and check if we load what we expected +func TestConfigs(t *testing.T) { + for _, testConfig := range testConfigs { + c, err := LoadInjectionConfigFromFilePath(testConfig.path) + if err != nil { + t.Error(err) + t.Fail() + } + if c.Name != testConfig.name { + t.Errorf("expected %s Name loaded from %s but got %s", testConfig.name, testConfig.path, c.Name) + t.Fail() + } + if len(c.Environment) != testConfig.expectedEnvVarCount { + t.Errorf("expected %d EnvVars loaded from %s but got %d", testConfig.expectedEnvVarCount, testConfig.path, len(c.Environment)) + t.Fail() + } + if len(c.Containers) != testConfig.expectedContainerCount { + t.Errorf("expected %d Containers loaded from %s but got %d", testConfig.expectedContainerCount, testConfig.path, len(c.Containers)) + t.Fail() + } + if len(c.Volumes) != testConfig.expectedVolumeCount { + t.Errorf("expected %d Volumes loaded from %s but got %d", testConfig.expectedVolumeCount, testConfig.path, len(c.Volumes)) + t.Fail() + } + if len(c.VolumeMounts) != testConfig.expectedVolumeMountCount { + t.Fatalf("expected %d VolumeMounts loaded from %s but got %d", testConfig.expectedVolumeMountCount, testConfig.path, len(c.VolumeMounts)) + } + if len(c.HostAliases) != testConfig.expectedHostAliasCount { + t.Fatalf("expected %d HostAliases loaded from %s but got %d", testConfig.expectedHostAliasCount, testConfig.path, len(c.HostAliases)) + } + if len(c.InitContainers) != testConfig.expectedInitContainerCount { + t.Fatalf("expected %d InitContainers loaded from %s but got %d", testConfig.expectedInitContainerCount, testConfig.path, len(c.InitContainers)) + } + } +} + +// TestLoadConfig: Check if we get all the configs func TestLoadConfig(t *testing.T) { - expectedNumInjectionsConfig := 5 - c, err := LoadConfigDirectory(sidecars) + expectedNumInjectionsConfig := len(testConfigs) + c, err := LoadConfigDirectory(fixtureSidecarsDir) if err != nil { t.Fatal(err) } @@ -29,216 +138,35 @@ func TestLoadConfig(t *testing.T) { } } -// load a config that uses only environment variable injections -func TestLoadEnvironmentInjectionConfig(t *testing.T) { - cfg := env1 - c, err := LoadInjectionConfigFromFilePath(cfg) - if err != nil { - t.Error(err) - t.Fail() - } - expectedName := "env1" - expectedEnvVarCount := 3 - expectedContainerCount := 0 - expectedVolumeCount := 0 - nExpectedVolumeMounts := 0 - nExpectedHostAliases := 0 - if c.Name != expectedName { - t.Errorf("expected %s Name loaded from %s but got %s", expectedName, cfg, c.Name) - t.Fail() - } - if len(c.Environment) != expectedEnvVarCount { - t.Errorf("expected %d EnvVars loaded from %s but got %d", expectedEnvVarCount, cfg, len(c.Environment)) - t.Fail() - } - if len(c.Containers) != expectedContainerCount { - t.Errorf("expected %d Containers loaded from %s but got %d", expectedContainerCount, cfg, len(c.Containers)) - t.Fail() - } - if len(c.Volumes) != expectedVolumeCount { - t.Errorf("expected %d Volumes loaded from %s but got %d", expectedVolumeCount, cfg, len(c.Volumes)) - t.Fail() - } - if len(c.VolumeMounts) != nExpectedVolumeMounts { - t.Fatalf("expected %d VolumeMounts loaded from %s but got %d", nExpectedVolumeMounts, cfg, len(c.VolumeMounts)) - } - if len(c.HostAliases) != nExpectedHostAliases { - t.Fatalf("expected %d HostAliases loaded from %s but got %d", nExpectedHostAliases, cfg, len(c.HostAliases)) - } -} - -func TestLoadInjectionConfig1(t *testing.T) { - cfg := cfg1 - c, err := LoadInjectionConfigFromFilePath(cfg) - if err != nil { - t.Fatal(err) - } - if c.Name != "sidecar-test" { - t.Fatalf("expected %s Name loaded from %s but got %s", "sidecar-test", cfg, c.Name) - } - expectedEnvVars := 2 - if len(c.Environment) != expectedEnvVars { - t.Fatalf("expected %d EnvVars loaded from %s but got %d", expectedEnvVars, cfg, len(c.Environment)) - } - if len(c.Containers) != 2 { - t.Fatalf("expected %d Containers loaded from %s but got %d", 2, cfg, len(c.Containers)) - } - if len(c.Volumes) != 1 { - t.Fatalf("expected %d Volumes loaded from %s but got %d", 1, cfg, len(c.Volumes)) - } - if len(c.VolumeMounts) != 0 { - t.Fatalf("expected %d VolumeMounts loaded from %s but got %d", 0, cfg, len(c.VolumeMounts)) - } - if len(c.HostAliases) != 0 { - t.Fatalf("expected %d HostAliases loaded from %s but got %d", 0, cfg, len(c.HostAliases)) - } -} - -// load a more complicated test config with LOTS of configuration -func TestLoadComplexConfig(t *testing.T) { - cfg := complicatedConfig - c, err := LoadInjectionConfigFromFilePath(cfg) - if err != nil { - t.Fatal(err) - } - expectedName := "complex-sidecar" - nExpectedContainers := 4 - nExpectedVolumes := 1 - nExpectedEnvironmentVars := 0 - nExpectedVolumeMounts := 0 - nExpectedHostAliases := 0 - - if c.Name != expectedName { - t.Fatalf("expected %s Name loaded from %s but got %s", expectedName, cfg, c.Name) - } - if len(c.Environment) != nExpectedEnvironmentVars { - t.Fatalf("expected %d EnvVars loaded from %s but got %d", nExpectedEnvironmentVars, cfg, len(c.Environment)) - } - if len(c.Containers) != nExpectedContainers { - t.Fatalf("expected %d Containers loaded from %s but got %d", nExpectedContainers, cfg, len(c.Containers)) - } - if len(c.Volumes) != nExpectedVolumes { - t.Fatalf("expected %d Volumes loaded from %s but got %d", nExpectedVolumes, cfg, len(c.Volumes)) - } - if len(c.VolumeMounts) != nExpectedVolumeMounts { - t.Fatalf("expected %d VolumeMounts loaded from %s but got %d", nExpectedVolumeMounts, cfg, len(c.VolumeMounts)) - } - if len(c.HostAliases) != nExpectedHostAliases { - t.Fatalf("expected %d HostAliases loaded from %s but got %d", nExpectedHostAliases, cfg, len(c.HostAliases)) - } -} - -func TestLoadVolumeMountsConfig(t *testing.T) { - cfg := volumeMounts - c, err := LoadInjectionConfigFromFilePath(cfg) - if err != nil { - t.Fatal(err) - } - expectedName := "volume-mounts" - nExpectedContainers := 3 - nExpectedVolumes := 2 - nExpectedEnvironmentVars := 2 - nExpectedVolumeMounts := 1 - nExpectedHostAliases := 0 - - if c.Name != expectedName { - t.Fatalf("expected %s Name loaded from %s but got %s", expectedName, cfg, c.Name) - } - if len(c.Environment) != nExpectedEnvironmentVars { - t.Fatalf("expected %d EnvVars loaded from %s but got %d", nExpectedEnvironmentVars, cfg, len(c.Environment)) - } - if len(c.Containers) != nExpectedContainers { - t.Fatalf("expected %d Containers loaded from %s but got %d", nExpectedContainers, cfg, len(c.Containers)) - } - if len(c.Volumes) != nExpectedVolumes { - t.Fatalf("expected %d Volumes loaded from %s but got %d", nExpectedVolumes, cfg, len(c.Volumes)) - } - if len(c.VolumeMounts) != nExpectedVolumeMounts { - t.Fatalf("expected %d VolumeMounts loaded from %s but got %d", nExpectedVolumeMounts, cfg, len(c.VolumeMounts)) - } - if len(c.HostAliases) != nExpectedHostAliases { - t.Fatalf("expected %d HostAliases loaded from %s but got %d", nExpectedHostAliases, cfg, len(c.HostAliases)) - } -} - -func TestLoadHostAliasesConfig(t *testing.T) { - cfg := hostAliases - c, err := LoadInjectionConfigFromFilePath(cfg) - if err != nil { - t.Fatal(err) - } - expectedName := "host-aliases" - nExpectedContainers := 1 - nExpectedVolumes := 0 - nExpectedEnvironmentVars := 2 - nExpectedVolumeMounts := 0 - nExpectedHostAliases := 6 - - if c.Name != expectedName { - t.Fatalf("expected %s Name loaded from %s but got %s", expectedName, cfg, c.Name) - } - if len(c.Environment) != nExpectedEnvironmentVars { - t.Fatalf("expected %d EnvVars loaded from %s but got %d", nExpectedEnvironmentVars, cfg, len(c.Environment)) - } - if len(c.Containers) != nExpectedContainers { - t.Fatalf("expected %d Containers loaded from %s but got %d", nExpectedContainers, cfg, len(c.Containers)) - } - if len(c.Volumes) != nExpectedVolumes { - t.Fatalf("expected %d Volumes loaded from %s but got %d", nExpectedVolumes, cfg, len(c.Volumes)) - } - if len(c.VolumeMounts) != nExpectedVolumeMounts { - t.Fatalf("expected %d VolumeMounts loaded from %s but got %d", nExpectedVolumeMounts, cfg, len(c.VolumeMounts)) - } - if len(c.HostAliases) != nExpectedHostAliases { - t.Fatalf("expected %d HostAliases loaded from %s but got %d", nExpectedHostAliases, cfg, len(c.HostAliases)) - } -} - -func TestHasInjectionConfig(t *testing.T) { - c, err := LoadConfigDirectory(sidecars) - if err != nil { - t.Fatal(err) - } - - for _, k := range []string{"sidecar-test", "complex-sidecar"} { - if !c.HasInjectionConfig(k) { - t.Fatalf("%s should have included %s but did not", cfg1, k) - } - } - - // make sure it says when things arent present too - for _, k := range []string{"missing-1", "yolo420blazeit"} { - if c.HasInjectionConfig(k) { - t.Fatalf("%s should NOT have included %s but did", cfg1, k) - } - } - -} - +// TestFetInjectionConfig: Check if we can properly load a config by name and see if we read the correct values from it func TestGetInjectionConfig(t *testing.T) { - c, err := LoadConfigDirectory(sidecars) + cfg := testConfigs["sidecar-test"] + c, err := LoadConfigDirectory(fixtureSidecarsDir) if err != nil { t.Fatal(err) } - i, err := c.GetInjectionConfig("sidecar-test") + i, err := c.GetInjectionConfig(cfg.name) if err != nil { t.Fatal(err) } - if len(i.Environment) != 2 { - t.Fatalf("expected 2 envvars, but got %d", len(i.Environment)) + if len(i.Environment) != cfg.expectedEnvVarCount { + t.Fatalf("expected %d envvars, but got %d", cfg.expectedEnvVarCount, len(i.Environment)) + } + if len(i.Containers) != cfg.expectedContainerCount { + t.Fatalf("expected %d container, but got %d", cfg.expectedContainerCount, len(i.Containers)) } - if len(i.Containers) != 2 { - t.Fatalf("expected 2 container, but got %d", len(i.Containers)) + if len(i.Volumes) != cfg.expectedVolumeCount { + t.Fatalf("expected %d volume, but got %d", cfg.expectedVolumeCount, len(i.Volumes)) } - if len(i.Volumes) != 1 { - t.Fatalf("expected 1 volume, but got %d", len(i.Volumes)) + if len(i.VolumeMounts) != cfg.expectedVolumeMountCount { + t.Fatalf("expected %d VolumeMounts, but got %d", cfg.expectedVolumeMountCount, len(i.VolumeMounts)) } - if len(i.VolumeMounts) != 0 { - t.Fatalf("expected %d VolumeMounts, but got %d", 0, len(i.VolumeMounts)) + if len(i.HostAliases) != cfg.expectedHostAliasCount { + t.Fatalf("expected %d HostAliases, but got %d", cfg.expectedHostAliasCount, len(i.HostAliases)) } - if len(i.HostAliases) != 0 { - t.Fatalf("expected %d HostAliases, but got %d", 0, len(i.HostAliases)) + if len(i.InitContainers) != cfg.expectedInitContainerCount { + t.Fatalf("expected %d InitContainers, but got %d", cfg.expectedInitContainerCount, len(i.InitContainers)) } } From 5859e33dfb80df1b87bf55f80c757c712091451e Mon Sep 17 00:00:00 2001 From: Thorsten Klein Date: Thu, 7 Mar 2019 11:47:07 +0100 Subject: [PATCH 2/7] add docs for initContainers --- docs/sidecar-configuration-format.md | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/docs/sidecar-configuration-format.md b/docs/sidecar-configuration-format.md index 2e8ff3a..0a7c5f4 100644 --- a/docs/sidecar-configuration-format.md +++ b/docs/sidecar-configuration-format.md @@ -59,6 +59,12 @@ env: volumeMounts: - name: some-config mountPath: /etc/some-config + +# initContainers will be added, no replacement of existing initContainers with the same names will be done +initContainers: + - name: some-initcontainer + image: init:1.12.2 + imagePullPolicy: IfNotPresent ``` ## Configuring new sidecars @@ -67,6 +73,6 @@ In order for the injector to know about a sidecar configuration, you need to eit 1. Create a new InjectionConfiguration `yaml` 1. Specify your `name:`. This is what you will request with `injector.tumblr.com/request=$name` - 2. Fill in the `containers`, `volumes`, `volumeMounts`, `hostAliases` and `env` fields with your configuration you want injected + 2. Fill in the `containers`, `volumes`, `volumeMounts`, `hostAliases`, `initContainers` and `env` fields with your configuration you want injected 2. Either bake your yaml into your Docker image you run (in `--config-directory=conf/`), or configure it as a ConfigMap in your k8s cluster. See [/docs/configmaps.md](/docs/configmaps.md) for information on how to configure a ConfigMap. 3. Deploy a pod with annotation `injector.tumblr.com/request=$name`! From 632592596f9ea6a2f41cb06326e4b44ade23c190 Mon Sep 17 00:00:00 2001 From: Thorsten Klein Date: Thu, 7 Mar 2019 14:11:33 +0100 Subject: [PATCH 3/7] add functionality and test --- README.md | 2 +- docs/sidecar-configuration-format.md | 1 + internal/pkg/config/watcher/loader_test.go | 120 ++++++++++++--------- pkg/server/webhook.go | 25 ++++- pkg/server/webhook_test.go | 4 +- 5 files changed, 100 insertions(+), 52 deletions(-) diff --git a/README.md b/README.md index 3bd1957..afb5589 100644 --- a/README.md +++ b/README.md @@ -9,7 +9,7 @@ Uses MutatingAdmissionWebhook in Kubernetes to inject sidecars into new deployme At Tumblr, we run some containers that have complicated sidecar setups. A kubernetes pod may run 5+ other containers, with some associated volumes and environment variables. It became clear quickly that keeping these sidecars in line would become an operational hassle; making sure every service uses the correct version of each dependency, updating global environment variable sets as configurations in our DCs change, etc. -To help solve this, we wrote the `k8s-sidecar-injector`. It is a small service that runs in each Kubernetes cluster, and listens to the Kubernetes API via webhooks. For each pod creation, the injector gets a (mutating admission) webhook, asking whether or not to allow the pod launch, and if allowed, what changes we would like to make to it. For pods that have special annotations on them (`injector.tumblr.com/request=some-sidecar-name`), we rewrite the pod configuration to include the containers, volumes, and environment variables defined in the sidecar `some-sidecar-name`'s configuration. +To help solve this, we wrote the `k8s-sidecar-injector`. It is a small service that runs in each Kubernetes cluster, and listens to the Kubernetes API via webhooks. For each pod creation, the injector gets a (mutating admission) webhook, asking whether or not to allow the pod launch, and if allowed, what changes we would like to make to it. For pods that have special annotations on them (`injector.tumblr.com/request=some-sidecar-name`), we rewrite the pod configuration to include the containers, volumes, volume mounts, host aliases, init-containers and environment variables defined in the sidecar `some-sidecar-name`'s configuration. This enabled us to keep sane, centralized configuration for oft-used, but infrequently cared about configuration for our sidecars. diff --git a/docs/sidecar-configuration-format.md b/docs/sidecar-configuration-format.md index 0a7c5f4..3dcda9b 100644 --- a/docs/sidecar-configuration-format.md +++ b/docs/sidecar-configuration-format.md @@ -61,6 +61,7 @@ volumeMounts: mountPath: /etc/some-config # initContainers will be added, no replacement of existing initContainers with the same names will be done +# this works exactly the same way like adding normal containers does: if you have a conflicting name, the server will return an error initContainers: - name: some-initcontainer image: init:1.12.2 diff --git a/internal/pkg/config/watcher/loader_test.go b/internal/pkg/config/watcher/loader_test.go index ddde200..7c955fd 100644 --- a/internal/pkg/config/watcher/loader_test.go +++ b/internal/pkg/config/watcher/loader_test.go @@ -11,16 +11,17 @@ import ( "github.com/tumblr/k8s-sidecar-injector/internal/pkg/config" _ "github.com/tumblr/k8s-sidecar-injector/internal/pkg/testing" "gopkg.in/yaml.v2" - "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" ) type injectionConfigExpectation struct { - name string - volumeCount int - envCount int - containerCount int - volumeMountCount int - hostAliasCount int + name string + volumeCount int + envCount int + containerCount int + volumeMountCount int + hostAliasCount int + initContainerCount int } var ( @@ -28,70 +29,88 @@ var ( ExpectedInjectionConfigFixtures = map[string][]injectionConfigExpectation{ "configmap-env1": []injectionConfigExpectation{ injectionConfigExpectation{ - name: "env1", - volumeCount: 0, - envCount: 3, - containerCount: 0, - volumeMountCount: 0, - hostAliasCount: 0, + name: "env1", + volumeCount: 0, + envCount: 3, + containerCount: 0, + volumeMountCount: 0, + hostAliasCount: 0, + initContainerCount: 0, }, }, "configmap-sidecar-test": []injectionConfigExpectation{ injectionConfigExpectation{ - name: "sidecar-test", - volumeCount: 1, - envCount: 2, - containerCount: 2, - volumeMountCount: 0, - hostAliasCount: 0, + name: "sidecar-test", + volumeCount: 1, + envCount: 2, + containerCount: 2, + volumeMountCount: 0, + hostAliasCount: 0, + initContainerCount: 0, }, }, "configmap-complex-sidecar": []injectionConfigExpectation{ injectionConfigExpectation{ - name: "complex-sidecar", - volumeCount: 1, - envCount: 0, - containerCount: 4, - volumeMountCount: 0, - hostAliasCount: 0, + name: "complex-sidecar", + volumeCount: 1, + envCount: 0, + containerCount: 4, + volumeMountCount: 0, + hostAliasCount: 0, + initContainerCount: 0, }, }, "configmap-multiple1": []injectionConfigExpectation{ injectionConfigExpectation{ - name: "env1", - volumeCount: 0, - envCount: 3, - containerCount: 0, - volumeMountCount: 0, - hostAliasCount: 0, + name: "env1", + volumeCount: 0, + envCount: 3, + containerCount: 0, + volumeMountCount: 0, + hostAliasCount: 0, + initContainerCount: 0, }, injectionConfigExpectation{ - name: "sidecar-test", - volumeCount: 1, - envCount: 2, - containerCount: 2, - volumeMountCount: 0, - hostAliasCount: 0, + name: "sidecar-test", + volumeCount: 1, + envCount: 2, + containerCount: 2, + volumeMountCount: 0, + hostAliasCount: 0, + initContainerCount: 0, }, }, "configmap-volume-mounts": []injectionConfigExpectation{ injectionConfigExpectation{ - name: "volume-mounts", - volumeCount: 2, - envCount: 2, - containerCount: 3, - volumeMountCount: 1, - hostAliasCount: 0, + name: "volume-mounts", + volumeCount: 2, + envCount: 2, + containerCount: 3, + volumeMountCount: 1, + hostAliasCount: 0, + initContainerCount: 0, }, }, "configmap-host-aliases": []injectionConfigExpectation{ injectionConfigExpectation{ - name: "host-aliases", - volumeCount: 0, - envCount: 2, - containerCount: 1, - volumeMountCount: 0, - hostAliasCount: 6, + name: "host-aliases", + volumeCount: 0, + envCount: 2, + containerCount: 1, + volumeMountCount: 0, + hostAliasCount: 6, + initContainerCount: 0, + }, + }, + "configmap-init-containers": []injectionConfigExpectation{ + injectionConfigExpectation{ + name: "init-containers", + volumeCount: 0, + envCount: 0, + containerCount: 2, + volumeMountCount: 0, + hostAliasCount: 0, + initContainerCount: 1, }, }, } @@ -166,6 +185,9 @@ func TestLoadFromConfigMap(t *testing.T) { if len(ic.HostAliases) != expectedICF.hostAliasCount { t.Fatalf("expected %d host aliases in %s, but found %d", expectedICF.hostAliasCount, expectedICF.name, len(ic.HostAliases)) } + if len(ic.InitContainers) != expectedICF.initContainerCount { + t.Fatalf("expected %d init containers in %s, but found %d", expectedICF.initContainerCount, expectedICF.name, len(ic.InitContainers)) + } for _, actualIC := range ics { if ic.Name == actualIC.Name { if ic.String() != actualIC.String() { diff --git a/pkg/server/webhook.go b/pkg/server/webhook.go index f9fdabf..68858fe 100644 --- a/pkg/server/webhook.go +++ b/pkg/server/webhook.go @@ -220,6 +220,27 @@ func addContainers(target, added []corev1.Container, basePath string) (patch []p return patch } +func addInitContainers(target, added []corev1.Container, basePath string) (patch []patchOperation) { + first := len(target) == 0 + var value interface{} + for _, add := range added { + value = add + path := basePath + if first { + first = false + value = []corev1.Container{add} + } else { + path = path + "/-" + } + patch = append(patch, patchOperation{ + Op: "add", + Path: path, + Value: value, + }) + } + return patch +} + func addVolumes(target, added []corev1.Volume, basePath string) (patch []patchOperation) { first := len(target) == 0 var value interface{} @@ -379,9 +400,11 @@ func createPatch(pod *corev1.Pod, inj *config.InjectionConfig, annotations map[s // now, patch all existing containers with the env vars and volume mounts patch = append(patch, setEnvironment(pod.Spec.Containers, inj.Environment)...) patch = append(patch, addVolumeMounts(pod.Spec.Containers, inj.VolumeMounts)...) - // now, add volumes and set annotations + // now, add initContainers, hostAliases and volumes + patch = append(patch, addContainers(pod.Spec.InitContainers, inj.InitContainers, "/spec/initContainers")...) patch = append(patch, addHostAliases(pod.Spec.HostAliases, inj.HostAliases, "/spec/hostAliases")...) patch = append(patch, addVolumes(pod.Spec.Volumes, inj.Volumes, "/spec/volumes")...) + // last but not least, set annotations patch = append(patch, updateAnnotations(pod.Annotations, annotations)...) return json.Marshal(patch) } diff --git a/pkg/server/webhook_test.go b/pkg/server/webhook_test.go index c863d33..4331c3d 100644 --- a/pkg/server/webhook_test.go +++ b/pkg/server/webhook_test.go @@ -22,6 +22,7 @@ var ( obj4 = "test/fixtures/k8s/object4.yaml" obj5 = "test/fixtures/k8s/object5.yaml" obj6 = "test/fixtures/k8s/object6.yaml" + obj7 = "test/fixtures/k8s/object7.yaml" ignoredNamespace = "test/fixtures/k8s/ignored-namespace-pod.yaml" badSidecar = "test/fixtures/k8s/bad-sidecar.yaml" @@ -35,7 +36,7 @@ type expectedSidecarConfiguration struct { } func TestLoadConfig(t *testing.T) { - expectedNumInjectionConfigs := 5 + expectedNumInjectionConfigs := 6 c, err := config.LoadConfigDirectory(sidecars) if err != nil { t.Error(err) @@ -67,6 +68,7 @@ func TestLoadConfig(t *testing.T) { {configuration: obj4, expectedSidecar: "", expectedError: ErrSkipAlreadyInjected}, // this one is already injected, so it should not get injected again {configuration: obj5, expectedSidecar: "volume-mounts"}, {configuration: obj6, expectedSidecar: "host-aliases"}, + {configuration: obj7, expectedSidecar: "init-containers"}, {configuration: ignoredNamespace, expectedSidecar: "", expectedError: ErrSkipIgnoredNamespace}, {configuration: badSidecar, expectedSidecar: "this-doesnt-exist", expectedError: ErrRequestedSidecarNotFound}, } From 40d78e37caf189a2b25fa560111da28f549f8caa Mon Sep 17 00:00:00 2001 From: Thorsten Klein Date: Thu, 7 Mar 2019 14:19:54 +0100 Subject: [PATCH 4/7] add test files --- .../k8s/configmap-init-containers.yaml | 29 +++++++++++++++++++ test/fixtures/k8s/object7.yaml | 4 +++ test/fixtures/sidecars/init-containers.yaml | 21 ++++++++++++++ 3 files changed, 54 insertions(+) create mode 100644 test/fixtures/k8s/configmap-init-containers.yaml create mode 100644 test/fixtures/k8s/object7.yaml create mode 100644 test/fixtures/sidecars/init-containers.yaml diff --git a/test/fixtures/k8s/configmap-init-containers.yaml b/test/fixtures/k8s/configmap-init-containers.yaml new file mode 100644 index 0000000..345a145 --- /dev/null +++ b/test/fixtures/k8s/configmap-init-containers.yaml @@ -0,0 +1,29 @@ +--- +apiVersion: v1 +kind: ConfigMap +metadata: + name: test-init-containers + namespace: default +data: + test-tumblr1: | + name: init-containers + initContainers: + - name: init-container-1 + image: foo:bar1 + imagePullPolicy: Always + command: + - "bash" + - "-c" + - > + echo "sleep 20" && + sleep 20 + 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 diff --git a/test/fixtures/k8s/object7.yaml b/test/fixtures/k8s/object7.yaml new file mode 100644 index 0000000..a3130df --- /dev/null +++ b/test/fixtures/k8s/object7.yaml @@ -0,0 +1,4 @@ +name: object6 +namespace: unittest +annotations: + "injector.unittest.com/request": "init-containers" diff --git a/test/fixtures/sidecars/init-containers.yaml b/test/fixtures/sidecars/init-containers.yaml new file mode 100644 index 0000000..0222e58 --- /dev/null +++ b/test/fixtures/sidecars/init-containers.yaml @@ -0,0 +1,21 @@ +name: init-containers +initContainers: + - name: init-container-1 + image: foo:bar1 + imagePullPolicy: Always + command: + - "bash" + - "-c" + - > + echo "sleep 20" && + sleep 20 +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 \ No newline at end of file From 838639a9a407ac18ed5a4a0057afb721d26d1d4b Mon Sep 17 00:00:00 2001 From: Thorsten Klein Date: Thu, 7 Mar 2019 20:01:43 +0100 Subject: [PATCH 5/7] missing newline --- test/fixtures/sidecars/init-containers.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/fixtures/sidecars/init-containers.yaml b/test/fixtures/sidecars/init-containers.yaml index 0222e58..4e3bff0 100644 --- a/test/fixtures/sidecars/init-containers.yaml +++ b/test/fixtures/sidecars/init-containers.yaml @@ -18,4 +18,4 @@ containers: - name: sidecar-existing-vm image: foo:69 ports: - - containerPort: 420 \ No newline at end of file + - containerPort: 420 From bd34992ce9c727327a292a5c4dbe0a541c78e5ef Mon Sep 17 00:00:00 2001 From: Thorsten Klein Date: Fri, 8 Mar 2019 07:46:07 +0100 Subject: [PATCH 6/7] add annotation escaping from PR #9 --- pkg/server/webhook.go | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/pkg/server/webhook.go b/pkg/server/webhook.go index 68858fe..362bcad 100644 --- a/pkg/server/webhook.go +++ b/pkg/server/webhook.go @@ -367,19 +367,19 @@ func mergeVolumeMounts(volumeMounts []corev1.VolumeMount, containers []corev1.Co func updateAnnotations(target map[string]string, added map[string]string) (patch []patchOperation) { for key, value := range added { + keyEscaped := strings.Replace(key, "/", "~1", -1) + if target == nil || target[key] == "" { target = map[string]string{} patch = append(patch, patchOperation{ - Op: "add", - Path: "/metadata/annotations", - Value: map[string]string{ - key: value, - }, + Op: "add", + Path: "/metadata/annotations/" + keyEscaped, + Value: value, }) } else { patch = append(patch, patchOperation{ Op: "replace", - Path: "/metadata/annotations/" + key, + Path: "/metadata/annotations/" + keyEscaped, Value: value, }) } @@ -395,15 +395,19 @@ 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) + // next, patch containers with our injected containers patch = append(patch, addContainers(pod.Spec.Containers, mutatedInjectedContainers, "/spec/containers")...) + // now, patch all existing containers with the env vars and volume mounts patch = append(patch, setEnvironment(pod.Spec.Containers, inj.Environment)...) patch = append(patch, addVolumeMounts(pod.Spec.Containers, inj.VolumeMounts)...) + // now, add initContainers, hostAliases and volumes patch = append(patch, addContainers(pod.Spec.InitContainers, inj.InitContainers, "/spec/initContainers")...) patch = append(patch, addHostAliases(pod.Spec.HostAliases, inj.HostAliases, "/spec/hostAliases")...) patch = append(patch, addVolumes(pod.Spec.Volumes, inj.Volumes, "/spec/volumes")...) + // last but not least, set annotations patch = append(patch, updateAnnotations(pod.Annotations, annotations)...) return json.Marshal(patch) @@ -449,7 +453,8 @@ func (whsvr *WebhookServer) mutate(ar *v1beta1.AdmissionReview) *v1beta1.Admissi // Workaround: https://github.com/kubernetes/kubernetes/issues/57982 applyDefaultsWorkaround(injectionConfig.Containers, injectionConfig.Volumes) - annotations := map[string]string{config.InjectionStatusAnnotation: StatusInjected} + annotations := map[string]string{} + annotations[config.InjectionStatusAnnotation] = StatusInjected patchBytes, err := createPatch(&pod, injectionConfig, annotations) if err != nil { injectionCounter.With(prometheus.Labels{"status": "error", "reason": "patching_error", "requested": injectionKey}).Inc() From 947330e834c347e829469830fc948fdc1bd868eb Mon Sep 17 00:00:00 2001 From: Thorsten Klein Date: Mon, 11 Mar 2019 07:29:31 +0100 Subject: [PATCH 7/7] address @komapa 's comments' --- docs/sidecar-configuration-format.md | 3 ++- test/fixtures/k8s/object7.yaml | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/docs/sidecar-configuration-format.md b/docs/sidecar-configuration-format.md index 3dcda9b..252183e 100644 --- a/docs/sidecar-configuration-format.md +++ b/docs/sidecar-configuration-format.md @@ -61,7 +61,8 @@ volumeMounts: mountPath: /etc/some-config # initContainers will be added, no replacement of existing initContainers with the same names will be done -# this works exactly the same way like adding normal containers does: if you have a conflicting name, the server will return an error +# this works exactly the same way like adding normal containers does: if you have a conflicting name, +# the server will return an error initContainers: - name: some-initcontainer image: init:1.12.2 diff --git a/test/fixtures/k8s/object7.yaml b/test/fixtures/k8s/object7.yaml index a3130df..ca741bf 100644 --- a/test/fixtures/k8s/object7.yaml +++ b/test/fixtures/k8s/object7.yaml @@ -1,4 +1,4 @@ -name: object6 +name: object7 namespace: unittest annotations: "injector.unittest.com/request": "init-containers"