feat: remove pod/service account labeled check in webhook (#720)

With the objectselector set in mwh, only pods that have the label `azure.workload.identity/use: "true"` will be sent to the webhook for mutation. We no longer need the check for pod/service account labeled. This also changes the default behavior when service account is not found; the mutation is done with the defaults and client id env var will be empty. Signed-off-by: Anish Ramasekar <[email protected]>
Azure · Jan 26, 2023 · 8c37dc1 · 8c37dc1
1 parent 305d10b
commit 8c37dc1
Show file tree

Hide file tree

Showing 8 changed files with 14 additions and 180 deletions.
diff --git a/pkg/cmd/podidentity/detect.go b/pkg/cmd/podidentity/detect.go
@@ -247,13 +247,6 @@ func (dc *detectCmd) createServiceAccountFile(name, ownerName, clientID string)
 		}
 	}
 
-	saLabels := make(map[string]string)
-	if sa.GetLabels() != nil {
-		saLabels = sa.GetLabels()
-	}
-	saLabels[webhook.UseWorkloadIdentityLabel] = "true"
-	sa.SetLabels(saLabels)
-
 	// set the annotations for the service account
 	saAnnotations := make(map[string]string)
 	if sa.GetAnnotations() != nil {

diff --git a/pkg/cmd/podidentity/detect_test.go b/pkg/cmd/podidentity/detect_test.go
@@ -317,9 +317,6 @@ func TestCreateServiceAccountFile(t *testing.T) {
 				t.Errorf("createServiceAccountFile() error = %v, want nil", err)
 			}
 
-			if !strings.Contains(string(gotServiceAccountFile), webhook.UseWorkloadIdentityLabel) {
-				t.Errorf("createServiceAccountFile() file %s does not contain %s, want it to contain it", saFile, webhook.UseWorkloadIdentityLabel)
-			}
 			for _, annotation := range tt.wantedAnnotations {
 				if !strings.Contains(string(gotServiceAccountFile), annotation) {
 					t.Errorf("createServiceAccountFile() file %s does not contain annotation %s, want it to contain it", saFile, annotation)

diff --git a/pkg/cmd/serviceaccount/phases/create/serviceaccount_test.go b/pkg/cmd/serviceaccount/phases/create/serviceaccount_test.go
@@ -113,9 +113,6 @@ func TestServiceAccountRun(t *testing.T) {
 	if err = kubeClient.Get(context.TODO(), types.NamespacedName{Name: "service-account-name", Namespace: "service-account-namespace"}, sa); err != nil {
 		t.Errorf("expected service account to be created")
 	}
-	if sa.Labels[webhook.UseWorkloadIdentityLabel] != "true" {
-		t.Errorf("expected service account to have workload identity label but got: %s", sa.Labels[webhook.UseWorkloadIdentityLabel])
-	}
 	if sa.Annotations[webhook.ClientIDAnnotation] != "aad-application-client-id" {
 		t.Errorf("expected service account to have client id annotation but got: %s", sa.Annotations[webhook.ClientIDAnnotation])
 	}

diff --git a/pkg/kuberneteshelper/serviceaccount.go b/pkg/kuberneteshelper/serviceaccount.go
@@ -20,9 +20,6 @@ func CreateOrUpdateServiceAccount(ctx context.Context, kubeClient client.Client,
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      name,
 			Namespace: namespace,
-			Labels: map[string]string{
-				webhook.UseWorkloadIdentityLabel: "true",
-			},
 			Annotations: map[string]string{
 				webhook.ClientIDAnnotation: clientID,
 				webhook.TenantIDAnnotation: tenantID,

diff --git a/pkg/kuberneteshelper/serviceaccount_test.go b/pkg/kuberneteshelper/serviceaccount_test.go
@@ -42,9 +42,6 @@ func TestCreateOrUpdateServiceAccount(t *testing.T) {
 	if sa.Annotations[webhook.ServiceAccountTokenExpiryAnnotation] != "3601" {
 		t.Errorf("CreateServiceAccount() token expiry annotation = %v, want %v", sa.Annotations[webhook.ServiceAccountTokenExpiryAnnotation], "3601")
 	}
-	if sa.Labels[webhook.UseWorkloadIdentityLabel] != "true" {
-		t.Errorf("CreateServiceAccount() useWorkloadIdentity label = %v, want %v", sa.Labels[webhook.UseWorkloadIdentityLabel], "true")
-	}
 }
 
 func TestCreateOrUpdateServiceAccountDefaultTokenExpiration(t *testing.T) {
@@ -69,9 +66,6 @@ func TestCreateOrUpdateServiceAccountDefaultTokenExpiration(t *testing.T) {
 	if _, ok := sa.Annotations[webhook.ServiceAccountTokenExpiryAnnotation]; ok {
 		t.Errorf("CreateServiceAccount() token expiry annotation should not be set")
 	}
-	if sa.Labels[webhook.UseWorkloadIdentityLabel] != "true" {
-		t.Errorf("CreateServiceAccount() useWorkloadIdentity label = %v, want %v", sa.Labels[webhook.UseWorkloadIdentityLabel], "true")
-	}
 }
 
 func TestDeleteServiceAccount(t *testing.T) {

diff --git a/pkg/webhook/webhook.go b/pkg/webhook/webhook.go
@@ -30,13 +30,6 @@ var (
 	// ProxyImageVersion is the image version of the proxy init and sidecar.
 	// This is injected via LDFLAGS in the Makefile during the build.
 	ProxyImageVersion string
-
-	PodLabelMissingWarning = fmt.Sprintf(`pod missing label '%s: "true"'. This label will be required in a future release for the webhook to mutate pod. Please add the label to the pod.`, UseWorkloadIdentityLabel)
-)
-
-const (
-	// warningAnnotationKey is the annotation key used to store warning messages in audit annotations
-	warningAnnotationKey = "mutation.azure-workload-identity.io/warning"
 )
 
 // +kubebuilder:webhook:path=/mutate-v1-pod,mutating=true,failurePolicy=fail,groups="",resources=pods,verbs=create,versions=v1,name=mutation.azure-workload-identity.io,sideEffects=None,admissionReviewVersions=v1;v1beta1,matchPolicy=Equivalent
@@ -89,11 +82,14 @@ func NewPodMutator(client client.Client, reader client.Reader, arcCluster bool,
 
 // PodMutator adds projected service account volume for incoming pods if service account is annotated
 func (m *podMutator) Handle(ctx context.Context, req admission.Request) (response admission.Response) {
-	warnings := []string{}
-	auditAnnotations := make(map[string]string)
-	pod := &corev1.Pod{}
 	timeStart := time.Now()
+	defer func() {
+		if m.reporter != nil {
+			m.reporter.ReportRequest(ctx, req.Namespace, time.Since(timeStart))
+		}
+	}()
 
+	pod := &corev1.Pod{}
 	err := m.decoder.Decode(req, pod)
 	if err != nil {
 		return admission.Errored(http.StatusBadRequest, err)
@@ -132,34 +128,6 @@ func (m *podMutator) Handle(ctx context.Context, req admission.Request) (respons
 		}
 	}
 
-	// mutate pod if the service account or pod is annotated
-	// service account annotated with "azure.workload-identity/use" in annotation or label is current behavior
-	// pod labeled with "azure.workload-identity/use" is added for backward compatibility in v0.15.0 release
-	// This check will eventually be removed in a future release (tentatively v1.0.0-alpha) and all pods will be
-	// mutated. (ref: https://github.com/Azure/azure-workload-identity/issues/601)
-	serviceAccountAnnotated := isServiceAccountAnnotated(serviceAccount)
-	podLabeled := isPodLabeled(pod)
-	shouldMutatePod := serviceAccountAnnotated || podLabeled
-	if !shouldMutatePod {
-		logger.Info("pod not labeled or service account not annotated, skipping mutation")
-		return admission.Allowed("pod not labeled or service account not annotated, skipping mutation")
-	}
-	if !podLabeled {
-		warnings = append(warnings, PodLabelMissingWarning)
-	}
-
-	// only log metrics for the pods that are actually mutated
-	defer func() {
-		if m.reporter != nil {
-			m.reporter.ReportRequest(ctx, req.Namespace, time.Since(timeStart))
-		}
-		response.Warnings = warnings
-		if len(warnings) > 0 {
-			auditAnnotations[warningAnnotationKey] = strings.Join(warnings, ";")
-			response.AuditAnnotations = auditAnnotations
-		}
-	}()
-
 	if shouldInjectProxySidecar(pod) {
 		proxyPort, err := getProxyPort(pod)
 		if err != nil {
@@ -301,22 +269,6 @@ func (m *podMutator) injectProxySidecarContainer(containers []corev1.Container,
 	return containers
 }
 
-// isServiceAccountAnnotated checks if the service account has been annotated
-// to use with workload identity
-// "azure.workload.identity/use" needs to be available either in annotations or labels
-func isServiceAccountAnnotated(sa *corev1.ServiceAccount) bool {
-	_, isLabeled := sa.Labels[UseWorkloadIdentityLabel]
-	_, isAnnotated := sa.Annotations[UseWorkloadIdentityLabel]
-	return isLabeled || isAnnotated
-}
-
-// isPodLabeled checks if the pod has been labelled to use with workload identity
-// "azure.workload.identity/use" needs to be available either in labels
-func isPodLabeled(pod *corev1.Pod) bool {
-	_, isLabeled := pod.Labels[UseWorkloadIdentityLabel]
-	return isLabeled
-}
-
 func shouldInjectProxySidecar(pod *corev1.Pod) bool {
 	if len(pod.Annotations) == 0 {
 		return false

diff --git a/pkg/webhook/webhook_test.go b/pkg/webhook/webhook_test.go
@@ -7,7 +7,6 @@ import (
 	"path/filepath"
 	"reflect"
 	"strconv"
-	"strings"
 	"testing"
 
 	admissionv1 "k8s.io/api/admission/v1"
@@ -60,56 +59,6 @@ func newPodRaw(name, namespace, serviceAccountName string, labels map[string]str
 	return raw
 }
 
-func TestIsServiceAccountAnnotated(t *testing.T) {
-	tests := []struct {
-		name     string
-		sa       *corev1.ServiceAccount
-		expected bool
-	}{
-		{
-			name: "service account not labeled",
-			sa: &corev1.ServiceAccount{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:      "sa",
-					Namespace: "default",
-				},
-			},
-			expected: false,
-		},
-		{
-			name: "service account is labeled with azure.workload.identity/use=true",
-			sa: &corev1.ServiceAccount{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:      "sa",
-					Namespace: "default",
-					Labels:    map[string]string{UseWorkloadIdentityLabel: "true"},
-				},
-			},
-			expected: true,
-		},
-		{
-			name: "service account is annotated with azure.workload.identity/use=true",
-			sa: &corev1.ServiceAccount{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:        "sa",
-					Namespace:   "default",
-					Annotations: map[string]string{UseWorkloadIdentityLabel: "true"},
-				},
-			},
-			expected: true,
-		},
-	}
-
-	for _, test := range tests {
-		t.Run(test.name, func(t *testing.T) {
-			actual := isServiceAccountAnnotated(test.sa)
-			if actual != test.expected {
-				t.Fatalf("expected: %v, got: %v", test.expected, actual)
-			}
-		})
-	}
-}
-
 func TestGetServiceAccountTokenExpiration(t *testing.T) {
 	tests := []struct {
 		name               string
@@ -766,7 +715,6 @@ func TestHandle(t *testing.T) {
 			ObjectMeta: metav1.ObjectMeta{
 				Name:      name,
 				Namespace: "ns1",
-				Labels:    map[string]string{UseWorkloadIdentityLabel: "true"},
 				Annotations: map[string]string{
 					ClientIDAnnotation:                  "clientID",
 					ServiceAccountTokenExpiryAnnotation: "4800",
@@ -783,42 +731,36 @@ func TestHandle(t *testing.T) {
 		podLabels          map[string]string
 		clientObjects      []client.Object
 		readerObjects      []client.Object
-		expectedWarnings   []string
 	}{
 		{
 			name:               "service account in cache",
 			serviceAccountName: "sa",
 			clientObjects:      serviceAccounts,
 			readerObjects:      nil,
-			expectedWarnings:   []string{PodLabelMissingWarning},
 		},
 		{
 			name:               "service account not in cache",
 			serviceAccountName: "sa",
 			clientObjects:      nil,
 			readerObjects:      serviceAccounts,
-			expectedWarnings:   []string{PodLabelMissingWarning},
 		},
 		{
-			name:             "default service account in cache",
-			clientObjects:    serviceAccounts,
-			readerObjects:    nil,
-			expectedWarnings: []string{PodLabelMissingWarning},
+			name:          "default service account in cache",
+			clientObjects: serviceAccounts,
+			readerObjects: nil,
 		},
 		{
-			name:             "default service account not in cache",
-			clientObjects:    nil,
-			readerObjects:    serviceAccounts,
-			expectedWarnings: []string{PodLabelMissingWarning},
+			name:          "default service account not in cache",
+			clientObjects: nil,
+			readerObjects: serviceAccounts,
 		},
 		{
 			name: "pod has the required label, no warnings",
 			podLabels: map[string]string{
 				UseWorkloadIdentityLabel: "true",
 			},
-			clientObjects:    serviceAccounts,
-			readerObjects:    nil,
-			expectedWarnings: nil,
+			clientObjects: serviceAccounts,
+			readerObjects: nil,
 		},
 	}
 
@@ -848,20 +790,6 @@ func TestHandle(t *testing.T) {
 			if !resp.Allowed {
 				t.Fatalf("expected to be allowed")
 			}
-			if len(resp.Warnings) != len(test.expectedWarnings) {
-				t.Fatalf("expected %d warnings, got %d", len(test.expectedWarnings), len(resp.Warnings))
-			}
-			for i := range resp.Warnings {
-				actual := resp.Warnings[i]
-				actualAuditAnnotations := resp.AuditAnnotations[warningAnnotationKey]
-				expected := test.expectedWarnings[i]
-				if actual != expected {
-					t.Fatalf("expected warning %d to be %s, got %s", i, expected, actual)
-				}
-				if !strings.Contains(actualAuditAnnotations, expected) {
-					t.Fatalf("expected audit annotation to contain %s, got %s", expected, actualAuditAnnotations)
-				}
-			}
 		})
 	}
 }

diff --git a/test/e2e/webhook.go b/test/e2e/webhook.go
@@ -3,12 +3,9 @@
 package e2e
 
 import (
-	"bytes"
 	"context"
 	"fmt"
-	"io"
 	"strings"
-	"sync"
 
 	"github.com/Azure/azure-workload-identity/pkg/webhook"
 
@@ -19,27 +16,6 @@ import (
 	"k8s.io/utils/pointer"
 )
 
-var _ io.Writer = &syncBuffer{}
-
-type syncBuffer struct {
-	lock sync.Mutex
-	buf  bytes.Buffer
-}
-
-func (s *syncBuffer) Write(p []byte) (int, error) {
-	s.lock.Lock()
-	defer s.lock.Unlock()
-
-	return s.buf.Write(p)
-}
-
-func (s *syncBuffer) String() string {
-	s.lock.Lock()
-	defer s.lock.Unlock()
-
-	return strings.TrimSuffix(s.buf.String(), "\n")
-}
-
 var _ = ginkgo.Describe("Webhook", func() {
 	f := framework.NewDefaultFramework("webhook")