feat: allow proxies to be injected using a native sidecar #1442
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@microsoft-github-policy-service agree |
|
@bench I think that this repo is dead tbh. I'm going to get on to our account managers and see if they can talk to whoever is in charge of it |
aramase
requested changes
Feb 26, 2025
| @@ -21,9 +21,9 @@ All annotations are optional. If the annotation is not specified, the default va | |||
| | `azure.workload.identity/service-account-token-expiration` | **(Takes precedence if the service account is also annotated)** Represents the `expirationSeconds` field for the projected service account token. It is an optional field that the user might want to configure this to prevent any downtime caused by errors during service account token refresh. Kubernetes service account token expiry will not be correlated with AAD tokens. AAD tokens will expire in 24 hours after they are issued. | `3600` (acceptable range: `3600 - 86400`) | | |||
| | `azure.workload.identity/skip-containers` | Represents a semi-colon-separated list of containers (e.g. `container1;container2`) to skip adding projected service account token volume. By default, the projected service account token volume will be added to all containers. | | | |||
| | `azure.workload.identity/inject-proxy-sidecar` | Injects a proxy init container and proxy sidecar into the pod. The proxy sidecar is used to intercept token requests to IMDS and acquire an AAD token on behalf of the user with federated identity credential. | `true` | | |||
| | `azure.workload.identity/use-native-sidecar` | If injected then the proxy sidecar should be injected as a native sidecar init container instead of a container. Native sidecars are terminated when if pod exits, for instance when running as a cron-job. | `false` | | |||
There was a problem hiding this comment.
Add a note about the minimum required kubernetes version this annotation can be used.
There was a problem hiding this comment.
This is the wrong approach IMO. I think we should instead detect at runtime (once at webhook start up time) if the cluster supports native sidecars, and if it does, we should simply always use them for the proxy sidecar. If for some reason we cannot detect this at runtime, we can make it a CLI flag based config to the webhook (possibly enabled by default since in a few days Kube v1.30 will be the oldest supported upstream version).
Comment on lines
+160
to
+164
| if useNativeSidecar { | ||
| pod.Spec.InitContainers = m.injectProxySidecarContainer(pod.Spec.InitContainers, proxyPort, ptr.To(corev1.ContainerRestartPolicyAlways)) | ||
| } else { | ||
| pod.Spec.Containers = m.injectProxySidecarContainer(pod.Spec.Containers, proxyPort, nil) | ||
| } |
There was a problem hiding this comment.
Suggested change
| if useNativeSidecar { | |
| pod.Spec.InitContainers = m.injectProxySidecarContainer(pod.Spec.InitContainers, proxyPort, ptr.To(corev1.ContainerRestartPolicyAlways)) | |
| } else { | |
| pod.Spec.Containers = m.injectProxySidecarContainer(pod.Spec.Containers, proxyPort, nil) | |
| } | |
| var restartPolicy *corev1.ContainerRestartPolicy | |
| if useNativeSidecar { | |
| restartPolicy = ptr.To(corev1.ContainerRestartPolicyAlways) | |
| } | |
| pod.Spec.Containers = m.injectProxySidecarContainer(pod.Spec.Containers, proxyPort, restartPolicy) |
| for _, container := range containers { | ||
| if container.Name == ProxySidecarContainerName { | ||
| return containers | ||
| // proxy-init starts with proxy, so we append ":" to imageRepository when checking | ||
| if strings.HasPrefix(container.Image, fmt.Sprintf("%s:", imageRepository)) || container.Name == ProxySidecarContainerName { |
There was a problem hiding this comment.
this check was removed as part of #1443
| } | ||
| } | ||
| logLevel := currentLogLevel() // run the proxy at the same log level as the webhook | ||
|
|
| @@ -1151,7 +1151,7 @@ func TestInjectProxySidecarContainer(t *testing.T) { | |||
| m := &podMutator{proxyImage: imageURL} | |||
| for _, test := range tests { | |||
| t.Run(test.name, func(t *testing.T) { | |||
| containers := m.injectProxySidecarContainer(test.containers, proxyPort) | |||
| containers := m.injectProxySidecarContainer(test.containers, proxyPort, nil) | |||
There was a problem hiding this comment.
Update this table test to include restart policy and ensure the return result contains the desired restart policy
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reason for Change:
Fixes Issue 773 by using native sidecars so that pods exit cleanly when running as cronjobs, currently azwi sidecar prevents the pod from terminating.
It also addresses an annoyance where the first container in the pod is the proxy, so any
kubectlcommands target that container, previously I was working around this by adding the default-container annotation.I've kept the current behaviour as the default because this change requires at least k8s 1.28, adding the annotation
azure.workload.identity/use-native-sidecarenables native sidecars. But it seems to work well so at some point in the future you could just make this the default behaviour.No update to deployment.yaml
No change to Helm chart
Requirements
Issue Fixed:
Fixes #733
Please answer the following questions with yes/no:
Does this change contain code from or inspired by another project? If so, did you notify the maintainers and provide attribution?
Notes for Reviewers:
Tested on v1.30.0-eks-fff26e3