Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/inject volume mounts #3

Merged
merged 8 commits into from
Feb 5, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 23 additions & 1 deletion examples/kubernetes/configmap-sidecar-test.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,34 @@ metadata:
data:
test1: |
name: test1
environment:
iwilltry42 marked this conversation as resolved.
Show resolved Hide resolved
env:
- name: HELLO
value: world
- name: TEST
value: test_that
volumeMounts:
- name: test-vol
mountPath: /tmp/test
volumes:
- name: test-vol
configMap:
name: test-config
containers:
- name: sidecar-nginx
image: nginx:1.12.2
imagePullPolicy: IfNotPresent
ports:
- containerPort: 80
env:
- name: ENV_IN_SIDECAR
value: test-in-sidecar
---
# configmap to test sharing a volume between sidecar and existing container
apiVersion: v1
kind: ConfigMap
metadata:
name: test-config
namespace: default
data:
test.txt: |
this is some test message shared between containers
11 changes: 6 additions & 5 deletions internal/pkg/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,11 @@ 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"`
Name string `json:"name"`
Containers []corev1.Container `json:"containers"`
Volumes []corev1.Volume `json:"volumes"`
Environment []corev1.EnvVar `json:"env"`
VolumeMounts []corev1.VolumeMount `json:"volumeMounts"`
iwilltry42 marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only comment on the PR would have been the use of corev1.VolumeMount As singular vs VolumeMounts In most other places. There is some mix between singular and plular throughout that is not clear to me why it needs to be different.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, corev1.VolumeMount refers to the Kubernetes specification of a single volumeMount in a container object. Now that we want to have more than one volumeMount in our config, I named it volumeMounts which is referring to a list of volumeMounts

}

// Config is a struct indicating how a given injection should be configured
Expand All @@ -44,7 +45,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", c.Name, len(c.Containers), len(c.Volumes), len(c.Environment))
return fmt.Sprintf("%s: %d containers, %d volumes, %d environment vars, %d volume mounts", c.Name, len(c.Containers), len(c.Volumes), len(c.Environment), len(c.VolumeMounts))
}

// ReplaceInjectionConfigs will take a list of new InjectionConfigs, and replace the current configuration with them.
Expand Down
46 changes: 45 additions & 1 deletion internal/pkg/config/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,11 @@ var (
cfg1 = sidecars + "/sidecar-test.yaml"
complicatedConfig = sidecars + "/complex-sidecar.yaml"
env1 = sidecars + "/env1.yaml"
volumeMounts = sidecars + "/volume-mounts.yaml"
)

func TestLoadConfig(t *testing.T) {
expectedNumInjectionsConfig := 3
expectedNumInjectionsConfig := 4
c, err := LoadConfigDirectory(sidecars)
if err != nil {
t.Fatal(err)
Expand All @@ -39,6 +40,7 @@ func TestLoadEnvironmentInjectionConfig(t *testing.T) {
expectedEnvVarCount := 3
expectedContainerCount := 0
expectedVolumeCount := 0
nExpectedVolumeMounts := 0
if c.Name != expectedName {
t.Errorf("expected %s Name loaded from %s but got %s", expectedName, cfg, c.Name)
t.Fail()
Expand All @@ -55,6 +57,9 @@ func TestLoadEnvironmentInjectionConfig(t *testing.T) {
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))
}
}

func TestLoadInjectionConfig1(t *testing.T) {
Expand All @@ -76,6 +81,9 @@ func TestLoadInjectionConfig1(t *testing.T) {
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))
}
}

// load a more complicated test config with LOTS of configuration
Expand All @@ -89,6 +97,36 @@ func TestLoadComplexConfig(t *testing.T) {
nExpectedContainers := 4
nExpectedVolumes := 1
nExpectedEnvironmentVars := 0
nExpectedVolumeMounts := 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))
}
}

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

if c.Name != expectedName {
t.Fatalf("expected %s Name loaded from %s but got %s", expectedName, cfg, c.Name)
}
Expand All @@ -101,6 +139,9 @@ func TestLoadComplexConfig(t *testing.T) {
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))
}
}

func TestHasInjectionConfig(t *testing.T) {
Expand Down Expand Up @@ -144,4 +185,7 @@ func TestGetInjectionConfig(t *testing.T) {
if len(i.Volumes) != 1 {
t.Fatalf("expected 1 volume, but got %d", len(i.Volumes))
}
if len(i.VolumeMounts) != 0 {
t.Fatalf("expected %d VolumeMounts, but got %d", 0, len(i.VolumeMounts))
}
}
66 changes: 42 additions & 24 deletions internal/pkg/config/watcher/loader_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,51 +15,66 @@ import (
)

type injectionConfigExpectation struct {
name string
volumeCount int
envCount int
containerCount int
name string
volumeCount int
envCount int
containerCount int
volumeMountCount int
}

var (
// maps a k8s ConfigMap fixture in test/fixtures/k8s/ => InjectionConfigExpectation
ExpectedInjectionConfigFixtures = map[string][]injectionConfigExpectation{
"configmap-env1": []injectionConfigExpectation{
injectionConfigExpectation{
name: "env1",
volumeCount: 0,
envCount: 3,
containerCount: 0,
name: "env1",
volumeCount: 0,
envCount: 3,
containerCount: 0,
volumeMountCount: 0,
},
},
"configmap-sidecar-test": []injectionConfigExpectation{
injectionConfigExpectation{
name: "sidecar-test",
volumeCount: 1,
envCount: 2,
containerCount: 2,
name: "sidecar-test",
volumeCount: 1,
envCount: 2,
containerCount: 2,
volumeMountCount: 0,
},
},
"configmap-complex-sidecar": []injectionConfigExpectation{
injectionConfigExpectation{
name: "complex-sidecar",
volumeCount: 1,
envCount: 0,
containerCount: 4,
name: "complex-sidecar",
volumeCount: 1,
envCount: 0,
containerCount: 4,
volumeMountCount: 0,
},
},
"configmap-multiple1": []injectionConfigExpectation{
injectionConfigExpectation{
name: "env1",
volumeCount: 0,
envCount: 3,
containerCount: 0,
name: "env1",
volumeCount: 0,
envCount: 3,
containerCount: 0,
volumeMountCount: 0,
},
injectionConfigExpectation{
name: "sidecar-test",
volumeCount: 1,
envCount: 2,
containerCount: 2,
name: "sidecar-test",
volumeCount: 1,
envCount: 2,
containerCount: 2,
volumeMountCount: 0,
},
},
"configmap-volume-mounts": []injectionConfigExpectation{
injectionConfigExpectation{
name: "volume-mounts",
volumeCount: 2,
envCount: 2,
containerCount: 3,
volumeMountCount: 1,
},
},
}
Expand Down Expand Up @@ -128,6 +143,9 @@ func TestLoadFromConfigMap(t *testing.T) {
if len(ic.Volumes) != expectedICF.volumeCount {
t.Fatalf("expected %d volumes in %s, but found %d", expectedICF.volumeCount, expectedICF.name, len(ic.Volumes))
}
if len(ic.VolumeMounts) != expectedICF.volumeMountCount {
t.Fatalf("expected %d volume mounts in %s, but found %d", expectedICF.volumeMountCount, expectedICF.name, len(ic.VolumeMounts))
}
for _, actualIC := range ics {
if ic.Name == actualIC.Name {
if ic.String() != actualIC.String() {
Expand Down
64 changes: 61 additions & 3 deletions pkg/server/webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,41 @@ func addVolumes(target, added []corev1.Volume, basePath string) (patch []patchOp
return patch
}

func addVolumeMounts(target []corev1.Container, addedVolumeMounts []corev1.VolumeMount) (patch []patchOperation) {
var value interface{}
for containerIndex, container := range target {
// for each container in the spec, determine if we want to patch with any volume mounts
first := len(container.VolumeMounts) == 0
for _, add := range addedVolumeMounts {
path := fmt.Sprintf("/spec/containers/%d/volumeMounts", containerIndex)
hasKey := false
// make sure we dont override any existing volume mounts; we only add, dont replace
for _, origVolumeMount := range container.VolumeMounts {
if origVolumeMount.Name == add.Name {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you want to return an error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would I return an error here, as we're consciously checking for matching names to not override an existing volumeMount? I could write a log message saying that the volume mount exists already and that it's not being replaced, but probably that won't be beneficial or what do you think?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It depends on what semantics you want. An add of an already existing mount could be interpreted as something out of wack. In the context of mergeing behavior this make more sense to me.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this logic is sound to me

hasKey = true
break
}
}
if !hasKey {
// make a patch
value = add
if first {
first = false
value = []corev1.VolumeMount{add}
} else {
path = path + "/-"
}
patch = append(patch, patchOperation{
Op: "add",
Path: path,
Value: value,
})
}
}
}
return patch
}

// for containers, add any env vars that are not already defined in the Env list.
// this does _not_ return patches; this is intended to be used only on containers defined
// in the injection config, so the resources do not exist yet in the k8s api (thus no patch needed)
Expand All @@ -252,6 +287,28 @@ func mergeEnvVars(envs []corev1.EnvVar, containers []corev1.Container) []corev1.
return mutatedContainers
}

func mergeVolumeMounts(volumeMounts []corev1.VolumeMount, containers []corev1.Container) []corev1.Container {
mutatedContainers := []corev1.Container{}
for _, c := range containers {
for _, newVolumeMount := range volumeMounts {
// check each container for each volume mount by name.
// if the container has a matching name, dont override!
skip := false
for _, origVolumeMount := range c.VolumeDevices {
if origVolumeMount.Name == newVolumeMount.Name {
skip = true
break
}
}
if !skip {
c.VolumeMounts = append(c.VolumeMounts, newVolumeMount)
}
}
mutatedContainers = append(mutatedContainers, c)
}
return mutatedContainers
}

func updateAnnotations(target map[string]string, added map[string]string) (patch []patchOperation) {
for key, value := range added {
if target == nil || target[key] == "" {
Expand All @@ -278,17 +335,18 @@ func updateAnnotations(target map[string]string, added map[string]string) (patch
func createPatch(pod *corev1.Pod, inj *config.InjectionConfig, annotations map[string]string) ([]byte, error) {
var patch []patchOperation

// first, make sure any injected containers in our config get the EnvVars injected
// first, make sure any injected containers in our config get the EnvVars and VolumeMounts injected
// 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
// 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
patch = append(patch, addVolumes(pod.Spec.Volumes, inj.Volumes, "/spec/volumes")...)
patch = append(patch, updateAnnotations(pod.Annotations, annotations)...)

return json.Marshal(patch)
}

Expand Down
4 changes: 3 additions & 1 deletion pkg/server/webhook_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,11 @@ var (
env1 = "test/fixtures/k8s/env1.yaml"
obj3Missing = "test/fixtures/k8s/object3-missing.yaml"
obj4 = "test/fixtures/k8s/object4.yaml"
obj5 = "test/fixtures/k8s/object5.yaml"
)

func TestLoadConfig(t *testing.T) {
expectedNumInjectionConfigs := 3
expectedNumInjectionConfigs := 4
c, err := config.LoadConfigDirectory(sidecars)
if err != nil {
t.Error(err)
Expand Down Expand Up @@ -51,6 +52,7 @@ func TestLoadConfig(t *testing.T) {
env1: "env1",
obj3Missing: "", // this one is missing any annotations :)
obj4: "", // this one is already injected, so it should not get injected again
obj5: "volume-mounts",
}
for f, k := range objects {
data, err := ioutil.ReadFile(f)
Expand Down
Loading