Skip to content

Commit 560ffca

Browse files
committed
fix: pod name logging in admission webhook
- Add potentialPodName() function based on Istio's proven pattern - Handle generateName case with descriptive suffix - Convert all logging to clean structured fields - Remove ugly string concatenation in favor of separate pod/namespace fields - Add comprehensive unit tests for pod naming scenarios - Ensure consistent logging throughout webhook lifecycle This fixes empty pod names in logs during admission control phase when pods only have generateName (common with Deployments/ReplicaSets). Example log output: Before: {"pod": "", "namespace": "flux-system"} After: {"pod": "controller-***** (actual name not yet known)", "namespace": "flux-system"}
1 parent bd257cf commit 560ffca

File tree

2 files changed

+182
-11
lines changed

2 files changed

+182
-11
lines changed

internal/webhook/v1/pod_webhook.go

Lines changed: 25 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -25,20 +25,21 @@ import (
2525
"context"
2626
"encoding/json"
2727
"fmt"
28+
"os"
29+
"strings"
30+
2831
"github.com/prometheus/client_golang/prometheus"
2932
corev1 "k8s.io/api/core/v1"
3033
apierrors "k8s.io/apimachinery/pkg/api/errors"
3134
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
3235
"k8s.io/apimachinery/pkg/runtime"
3336
"k8s.io/apimachinery/pkg/types"
3437
"k8s.io/client-go/tools/record"
35-
"os"
3638
ctrl "sigs.k8s.io/controller-runtime"
3739
"sigs.k8s.io/controller-runtime/pkg/client"
3840
logf "sigs.k8s.io/controller-runtime/pkg/log"
3941
"sigs.k8s.io/controller-runtime/pkg/metrics"
4042
"sigs.k8s.io/controller-runtime/pkg/webhook"
41-
"strings"
4243
)
4344

4445
// nolint:unused
@@ -102,14 +103,27 @@ type PodCustomDefaulter struct {
102103

103104
var _ webhook.CustomDefaulter = &PodCustomDefaulter{}
104105

106+
// potentialPodName returns the pod name if available, otherwise a descriptive string
107+
// This mirrors Istio's approach to handling generateName in admission webhooks
108+
func potentialPodName(metadata metav1.ObjectMeta) string {
109+
if metadata.Name != "" {
110+
return metadata.Name
111+
}
112+
if metadata.GenerateName != "" {
113+
return metadata.GenerateName + "***** (actual name not yet known)"
114+
}
115+
return ""
116+
}
117+
105118
// Default implements webhook.CustomDefaulter so a webhook will be registered for the Kind Pod.
106119
func (d *PodCustomDefaulter) Default(ctx context.Context, obj runtime.Object) error {
107120
pod, ok := obj.(*corev1.Pod)
108121
if !ok {
109122
return fmt.Errorf("expected an Pod object but got %T", obj)
110123
}
111124

112-
podlog.Info("Defaulting for Pod", "name", pod.GetName())
125+
podName := potentialPodName(pod.ObjectMeta)
126+
podlog.Info("Processing pod for WIF injection", "pod", podName, "namespace", pod.Namespace)
113127

114128
// Skip if pod already has WIF volumes/env vars
115129
if hasWorkloadIdentityConfig(pod) {
@@ -160,7 +174,7 @@ func (d *PodCustomDefaulter) Default(ctx context.Context, obj runtime.Object) er
160174
// Inject WIF configuration
161175
injectWorkloadIdentityConfig(d, pod, wifConfig)
162176

163-
podlog.Info("Injected WIF configuration", "pod", pod.GetName(), "serviceAccount", saName)
177+
podlog.Info("Injected WIF configuration", "pod", podName, "namespace", pod.Namespace, "serviceAccount", saName)
164178
return nil
165179
}
166180

@@ -365,7 +379,7 @@ func injectWorkloadIdentityConfig(d *PodCustomDefaulter, pod *corev1.Pod, config
365379
injectionOperations.WithLabelValues("volume", "injected", "success").Inc()
366380
} else {
367381
podlog.Info("Skipped WIF injection due to existing volume",
368-
"pod", pod.GetName(),
382+
"pod", potentialPodName(pod.ObjectMeta),
369383
"namespace", pod.Namespace,
370384
"component", "volume",
371385
"volume", "token",
@@ -394,7 +408,7 @@ func injectWorkloadIdentityConfig(d *PodCustomDefaulter, pod *corev1.Pod, config
394408
injectionOperations.WithLabelValues("volume", "injected", "success").Inc()
395409
} else {
396410
podlog.Info("Skipped WIF injection due to existing volume",
397-
"pod", pod.GetName(),
411+
"pod", potentialPodName(pod.ObjectMeta),
398412
"namespace", pod.Namespace,
399413
"component", "volume",
400414
"volume", credentialsVolumeName,
@@ -424,7 +438,7 @@ func injectWorkloadIdentityConfig(d *PodCustomDefaulter, pod *corev1.Pod, config
424438
} else {
425439
if volumeMountExists(container, "token") {
426440
podlog.Info("Skipped WIF injection due to existing volume mount",
427-
"pod", pod.GetName(),
441+
"pod", potentialPodName(pod.ObjectMeta),
428442
"namespace", pod.Namespace,
429443
"container", container.Name,
430444
"component", "mount",
@@ -434,7 +448,7 @@ func injectWorkloadIdentityConfig(d *PodCustomDefaulter, pod *corev1.Pod, config
434448
}
435449
if mountPathExists(container, tokenMountPath) {
436450
podlog.Info("Skipped WIF injection due to mount path conflict",
437-
"pod", pod.GetName(),
451+
"pod", potentialPodName(pod.ObjectMeta),
438452
"namespace", pod.Namespace,
439453
"container", container.Name,
440454
"component", "mount",
@@ -457,7 +471,7 @@ func injectWorkloadIdentityConfig(d *PodCustomDefaulter, pod *corev1.Pod, config
457471
} else {
458472
if volumeMountExists(container, credentialsVolumeName) {
459473
podlog.Info("Skipped WIF injection due to existing volume mount",
460-
"pod", pod.GetName(),
474+
"pod", potentialPodName(pod.ObjectMeta),
461475
"namespace", pod.Namespace,
462476
"container", container.Name,
463477
"component", "mount",
@@ -467,7 +481,7 @@ func injectWorkloadIdentityConfig(d *PodCustomDefaulter, pod *corev1.Pod, config
467481
}
468482
if mountPathExists(container, credentialsMountPath) {
469483
podlog.Info("Skipped WIF injection due to mount path conflict",
470-
"pod", pod.GetName(),
484+
"pod", potentialPodName(pod.ObjectMeta),
471485
"namespace", pod.Namespace,
472486
"container", container.Name,
473487
"component", "mount",
@@ -486,7 +500,7 @@ func injectWorkloadIdentityConfig(d *PodCustomDefaulter, pod *corev1.Pod, config
486500
injectionOperations.WithLabelValues("env", "injected", "success").Inc()
487501
} else {
488502
podlog.Info("Skipped WIF injection due to existing environment variable",
489-
"pod", pod.GetName(),
503+
"pod", potentialPodName(pod.ObjectMeta),
490504
"namespace", pod.Namespace,
491505
"container", container.Name,
492506
"component", "env",

internal/webhook/v1/pod_webhook_test.go

Lines changed: 157 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -363,6 +363,163 @@ func TestInjectWorkloadIdentityConfig(t *testing.T) {
363363

364364
}
365365

366+
func TestPotentialPodName(t *testing.T) {
367+
tests := []struct {
368+
name string
369+
metadata metav1.ObjectMeta
370+
expected string
371+
}{
372+
{
373+
name: "pod with explicit name",
374+
metadata: metav1.ObjectMeta{
375+
Name: "my-pod",
376+
Namespace: "default",
377+
},
378+
expected: "my-pod",
379+
},
380+
{
381+
name: "pod with generateName (typical Deployment pattern)",
382+
metadata: metav1.ObjectMeta{
383+
GenerateName: "web-deployment-abc123-",
384+
Namespace: "default",
385+
},
386+
expected: "web-deployment-abc123-***** (actual name not yet known)",
387+
},
388+
{
389+
name: "pod with both name and generateName (name takes precedence)",
390+
metadata: metav1.ObjectMeta{
391+
Name: "explicit-name",
392+
GenerateName: "should-be-ignored-",
393+
Namespace: "default",
394+
},
395+
expected: "explicit-name",
396+
},
397+
{
398+
name: "pod with neither name nor generateName",
399+
metadata: metav1.ObjectMeta{
400+
Namespace: "default",
401+
},
402+
expected: "",
403+
},
404+
}
405+
406+
for _, tt := range tests {
407+
t.Run(tt.name, func(t *testing.T) {
408+
actual := potentialPodName(tt.metadata)
409+
assert.Equal(t, tt.expected, actual)
410+
})
411+
}
412+
}
413+
414+
func TestPodWebhookIntegration(t *testing.T) {
415+
// Set required environment variables for testing
416+
os.Setenv("PROJECT_NUMBER", "123456789")
417+
os.Setenv("POOL_ID", "test-pool")
418+
os.Setenv("PROVIDER_ID", "test-provider")
419+
defer func() {
420+
os.Unsetenv("PROJECT_NUMBER")
421+
os.Unsetenv("POOL_ID")
422+
os.Unsetenv("PROVIDER_ID")
423+
}()
424+
425+
testSA := &corev1.ServiceAccount{
426+
ObjectMeta: metav1.ObjectMeta{
427+
Name: "default",
428+
Namespace: "default",
429+
},
430+
}
431+
432+
runtimeScheme := runtime.NewScheme()
433+
require.NoError(t, scheme.AddToScheme(runtimeScheme))
434+
435+
fakeClient := fake.NewClientBuilder().
436+
WithScheme(runtimeScheme).
437+
WithObjects(testSA).
438+
Build()
439+
440+
webhook := &PodCustomDefaulter{
441+
Client: fakeClient,
442+
Cache: fakeClient,
443+
}
444+
445+
ctx := context.Background()
446+
447+
tests := []struct {
448+
name string
449+
pod *corev1.Pod
450+
}{
451+
{
452+
name: "pod with explicit name",
453+
pod: &corev1.Pod{
454+
ObjectMeta: metav1.ObjectMeta{
455+
Name: "explicit-pod-name",
456+
Namespace: "default",
457+
},
458+
Spec: corev1.PodSpec{
459+
ServiceAccountName: "default",
460+
Containers: []corev1.Container{
461+
{Name: "app", Image: "nginx"},
462+
},
463+
},
464+
},
465+
},
466+
{
467+
name: "pod with generateName (Deployment pattern)",
468+
pod: &corev1.Pod{
469+
ObjectMeta: metav1.ObjectMeta{
470+
GenerateName: "web-deployment-abc123-",
471+
Namespace: "default",
472+
},
473+
Spec: corev1.PodSpec{
474+
ServiceAccountName: "default",
475+
Containers: []corev1.Container{
476+
{Name: "app", Image: "nginx"},
477+
},
478+
},
479+
},
480+
},
481+
{
482+
name: "pod with neither name nor generateName",
483+
pod: &corev1.Pod{
484+
ObjectMeta: metav1.ObjectMeta{
485+
Namespace: "default",
486+
},
487+
Spec: corev1.PodSpec{
488+
ServiceAccountName: "default",
489+
Containers: []corev1.Container{
490+
{Name: "app", Image: "nginx"},
491+
},
492+
},
493+
},
494+
},
495+
}
496+
497+
for _, tt := range tests {
498+
t.Run(tt.name, func(t *testing.T) {
499+
// Test that webhook processes without error
500+
err := webhook.Default(ctx, tt.pod)
501+
require.NoError(t, err)
502+
503+
// Verify WIF injection occurred
504+
assert.True(t, hasWorkloadIdentityConfig(tt.pod), "WIF configuration should be injected")
505+
506+
// Verify volumes were added
507+
assert.Len(t, tt.pod.Spec.Volumes, 2, "Should have token and credentials volumes")
508+
509+
// Verify container modifications
510+
require.Len(t, tt.pod.Spec.Containers, 1, "Should still have one container")
511+
container := tt.pod.Spec.Containers[0]
512+
513+
// Check volume mounts
514+
assert.Len(t, container.VolumeMounts, 2, "Should have WIF volume mounts")
515+
516+
// Check environment variable
517+
assert.Len(t, container.Env, 1, "Should have GOOGLE_APPLICATION_CREDENTIALS env var")
518+
assert.Equal(t, "GOOGLE_APPLICATION_CREDENTIALS", container.Env[0].Name)
519+
})
520+
}
521+
}
522+
366523
// BenchmarkWebhookDefault measures webhook performance
367524
func BenchmarkWebhookDefault(b *testing.B) {
368525
// Set required environment variables for testing

0 commit comments

Comments
 (0)