feat: Support injection into user-defined init containers #55
Conversation
| for i := range initContainerPatch { | ||
| initContainerPatch[i].Path = strings.Replace(initContainerPatch[i].Path, "/spec/containers", "/spec/initContainers", 1) |
There was a problem hiding this comment.
This is a good catch! The mutateContainer function always uses the /spec/containers path, even if the container being mutated is an init container.
Instead of first using the incorrect path, and then replacing it here with strings.Replace, it would be simpler and less error prone to update mutateContainer to use the correct path in the first place.
To do this, we could make mutateContainer take a new containerType parameter, which could be an enum differentiating between Init Containers and Containers:
type ContainerType string
const (
InitContainer ContainerType = "initContainer"
Container ContainerType = "container"
)
// ...
didMutate, initContainerPatch, err := s.mutateContainer(ctx, &c, i, InitContainer)
Then, the mutateContainer function could check the type of container and use the correct corresponding path.
There was a problem hiding this comment.
Do you think I could just use a bool initContainer instead of an enum since there are only two container types?
There was a problem hiding this comment.
@NominalTrajectory that's also possible. I have a slight preference for using an enum because I believe it makes the code more readable.
I think it's easier to understand what this line of code means:
s.mutateContainer(ctx, &c, i, InitContainer)
// and
s.mutateContainer(ctx, &c, i, Container)compared to this code
s.mutateContainer(ctx, &c, i, true)
// and
s.mutateContainer(ctx, &c, i, false)| if value, exists := pod.Annotations[injectorInitFirstAnnotation]; exists && strings.ToLower(value) == "true" { | ||
| patch = append(patch, prependContainers(pod.Spec.InitContainers, containers, "/spec/initContainers")...) | ||
| } else { | ||
| patch = append(patch, addContainers(pod.Spec.InitContainers, containers, "/spec/initContainers")...) | ||
| } |
There was a problem hiding this comment.
I don't think prependContainers actually prepends the op-cli container.
Looking at the PR you linked from Hashicorp Vault, the correct approach to prepending seems to be the following:
- Remove all init containers
- Add our init container (
op-cli) - Add back all other init containers
Here's the source code from Hashicorp vault: https://github.com/hashicorp/vault-k8s/pull/91/files#diff-7854e12d807c917e5f3a303d9f3182cf3d95a2fbc1e2f30ec281ffabf67c95ddR329
Do you think following HashiCorp's approach makes sense here? Let me know if I missed anything!
There was a problem hiding this comment.
After conducting some manual testing, I see that the reordering works because the init container is named 0 instead of -, but I still think Hashicorp's approach is more robust, and we should have the same approach.
There was a problem hiding this comment.
Thanks for the feedback @Marton6, I agree. I will update the PR during the weekend.
This PR addresses the issue #49 and introduces support for injecting secrets into user-defined init containers using the 1Password Kubernetes Secrets Injector.
Background
The 1Password Kubernetes Secrets Injector does not currently support injecting secrets into user-defined init containers, as its init container is appended to the pod specification and runs after all user-defined init containers. This causes the 1Password CLI binary and command modifications to be unavailable during the execution of user-defined init containers.
Key Changes
operator.1password.io/injector-init-first: "true"(defaults to false)true, the injector prepends its init container as the first init container.operator.1password.io/injectannotation are also mutated to enable secret injection.webhook.goto handle the new annotation logic.