Feature/inject volume mounts

[iwilltry42]

First off: Thank you for open-sourcing this amazing project 😃

What and why?

Feature to add volume mounts to all existing and injected containers by adding top level volumeMounts scope. This is useful when an injected sidecar container is supposed to share files with the existing containers. My use case for this was a Kerberos sidecar container that provides Kerberos tickets to the other containers in the pod.

Testing Steps

Tested within microk8s and minikube on Ubuntu 18.04 by adding the aforementioned fields to the example in examples/kubernetes and then checking if all the containers share the same configmap mounted.
Unfortunately there are no unittests yet, as the current setup (load InjectionConfig and expected IC via struct from JSON annotations) can't appropriately be applied to this case (because we need to check that each container has this one volumeMount afterwards and not count the global number).
If we can clarify a procedure, I'd like to add the tests later 👍

UPDATE (2019-01-28): I added unit tests, which require a bit more complexity, because volumeMounts are top-level (global scope) objects in the injection-config, but a second-level (containers scope) objects in the resulting config (after injection), which didn't really work with the current 1:1 mapping.

EDIT: seems like I maybe misunderstood how your unittests work 🤔 Anyways, I'll just wait for your feedback 😅

[yahoocla]

Thank you for submitting this pull request, however I do not see a valid CLA on file for you. Before we can merge this request please visit https://yahoocla.herokuapp.com/ and agree to the terms. Thanks! 😄

[byxorna]

Thank you for your PR! I am glad you find this utility useful.

I would love to hear a bit more about the usecase that necessitated this PR. Did you need a volumeMount in both an injected container, as well as a container not in your injection config? Internally, we have just been defining VolumeMounts on each container as part of the containers: config. This is because each sidecar container may have different mount requirements (i.e. mountPath, readOnly, etc etc).

Thanks!

[iwilltry42]

@byxorna

Did you need a volumeMount in both an injected container, as well as a container not in your injection config? Internally, we have just been defining VolumeMounts on each container as part of the containers: config. This is because each sidecar container may have different mount requirements (i.e. mountPath, readOnly, etc etc).

Yes exactly. Our use case looks like this: We have several services accessing e.g. a Hadoop Cluster with Kerberos authentication enabled. Up to now, the applications themselves took care of creating/renewing their Kerberos tickets. We now started using a sidecar container to create/renew the tickets. That sidecar container needs to share a volume with the main container(s), to share the created/renewed ticket with them. So yes, we could define the VolumeMounts in the container section of the injection config for the sidecars, but we still need to inject them into the existing containers to make the Kerberos ticket available to them (without them having to have any clue about how the ticket got there).

[byxorna]

@yahoocla rescan?

[iwilltry42]

Is this PR good to merge then @byxorna ? 👍

[byxorna]

@iwilltry42 yep; just a few things before we land this.

1. Have you filled out the CLA? I am not sure if the bot will comment back on this ticket once the CLA is filled out.
2. I was looking at our config, and I realize I provided you bad guidance with the naming of the volume-mounts key; we chose env: so it would align with k8s patterns, so I think your instinct was right on the money the first time. Mind using volumeMounts: instead of volume-mounts:? Sorry for the misinformation!

[iwilltry42]

@byxorna yep, signed it when the Bot first asked for it after the initial commits.

No worries, I'll revert the commit with the name change then 👍

[iwilltry42]

Done 👍

[byxorna]

@defect @komapa @roymarantz @gtorre wanna give this a pass before merging?

[roymarantz]

do you want to return an error?

[iwilltry42]

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?

[roymarantz]

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.

[byxorna]

this logic is sound to me

[byxorna]

@iwilltry42 thanks for the PR! Awesome new functionality :)

[komapa]

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.

[iwilltry42]

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