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

feat: allow proxies to be injected using a native sidecar #1442

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
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
Original file line number Diff line number Diff line change
Expand Up @@ -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` |
Copy link
Member

Choose a reason for hiding this comment

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

Add a note about the minimum required kubernetes version this annotation can be used.

Copy link
Member

Choose a reason for hiding this comment

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

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).

| `azure.workload.identity/proxy-sidecar-port` | Represents the port of the proxy sidecar. | `8000` |


## Service Account

### Annotations
Expand Down
2 changes: 2 additions & 0 deletions pkg/webhook/consts.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ const (
SkipContainersAnnotation = "azure.workload.identity/skip-containers"
// InjectProxySidecarAnnotation represents the annotation to be used to inject proxy sidecar into the pod
InjectProxySidecarAnnotation = "azure.workload.identity/inject-proxy-sidecar"
// UseNativeSidecarAnnotation represents the annotation to be used to inject the proxy as a native sidecar into the pod
UseNativeSidecarAnnotation = "azure.workload.identity/use-native-sidecar"
// ProxySidecarPortAnnotation represents the annotation to be used to specify the port for proxy sidecar
ProxySidecarPortAnnotation = "azure.workload.identity/proxy-sidecar-port"

Expand Down
26 changes: 23 additions & 3 deletions pkg/webhook/webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -154,8 +154,14 @@ func (m *podMutator) Handle(ctx context.Context, req admission.Request) (respons
return admission.Errored(http.StatusBadRequest, err)
}

useNativeSidecar := useNativeProxySidecar(pod)

pod.Spec.InitContainers = m.injectProxyInitContainer(pod.Spec.InitContainers, proxyPort)
pod.Spec.Containers = m.injectProxySidecarContainer(pod.Spec.Containers, proxyPort)
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)
}
Comment on lines +160 to +164
Copy link
Member

Choose a reason for hiding this comment

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

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)

}

// get service account token expiration
Expand Down Expand Up @@ -228,13 +234,18 @@ func (m *podMutator) injectProxyInitContainer(containers []corev1.Container, pro
return containers
}

func (m *podMutator) injectProxySidecarContainer(containers []corev1.Container, proxyPort int32) []corev1.Container {
func (m *podMutator) injectProxySidecarContainer(containers []corev1.Container, proxyPort int32, restartPolicy *corev1.ContainerRestartPolicy) []corev1.Container {
imageRepository := strings.Join([]string{ProxyImageRegistry, ProxySidecarImageName}, "/")
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 {
Copy link
Member

Choose a reason for hiding this comment

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

this check was removed as part of #1443

return containers
}
}
}
logLevel := currentLogLevel() // run the proxy at the same log level as the webhook

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

containers = append([]corev1.Container{{
Name: ProxySidecarContainerName,
Image: m.proxyImage,
Expand Down Expand Up @@ -267,6 +278,7 @@ func (m *podMutator) injectProxySidecarContainer(containers []corev1.Container,
ReadOnlyRootFilesystem: ptr.To(true),
RunAsNonRoot: ptr.To(true),
},
RestartPolicy: restartPolicy,
}}, containers...)

return containers
Expand All @@ -280,6 +292,14 @@ func shouldInjectProxySidecar(pod *corev1.Pod) bool {
return ok
}

func useNativeProxySidecar(pod *corev1.Pod) bool {
if len(pod.Annotations) == 0 {
return false
}
_, ok := pod.Annotations[UseNativeSidecarAnnotation]
return ok
}

// getSkipContainers gets the list of containers to skip based on the annotation
func getSkipContainers(pod *corev1.Pod) map[string]struct{} {
skipContainers := pod.Annotations[SkipContainersAnnotation]
Expand Down
41 changes: 40 additions & 1 deletion pkg/webhook/webhook_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

Update this table test to include restart policy and ensure the return result contains the desired restart policy

if !reflect.DeepEqual(containers, test.expectedContainers) {
t.Errorf("expected: %v, got: %v", test.expectedContainers, containers)
}
Expand Down Expand Up @@ -1198,6 +1198,45 @@ func TestShouldInjectProxySidecar(t *testing.T) {
}
}

func TestShouldUseNativeSidecar(t *testing.T) {
tests := []struct {
name string
pod *corev1.Pod
expected bool
}{
{
name: "pod not annotated",
pod: &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Name: "pod",
},
},
expected: false,
},
{
name: "pod is annotated with azure.workload.identity/use-native-sidecar=true",
pod: &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Name: "pod",
Annotations: map[string]string{
UseNativeSidecarAnnotation: "true",
},
},
},
expected: true,
},
}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
actual := useNativeProxySidecar(test.pod)
if actual != test.expected {
t.Fatalf("expected: %v, got: %v", test.expected, actual)
}
})
}
}

func TestGetProxyPort(t *testing.T) {
type args struct {
pod *corev1.Pod
Expand Down