Merge branch 'main' into oidc_password_grant

internal/controller/apicerts/certs_expirer_test.go

@@ -297,12 +297,7 @@ func TestExpirerControllerSync(t *testing.T) {
 
 			if test.wantDelete {
 				require.Len(t, *opts, 1)
-				require.Equal(t, metav1.DeleteOptions{
-					Preconditions: &metav1.Preconditions{
-						UID:             &testUID,
-						ResourceVersion: &testRV,
-					},
-				}, (*opts)[0])
+				require.Equal(t, testutil.NewPreconditions(testUID, testRV), (*opts)[0])
 			} else {
 				require.Len(t, *opts, 0)
 			}

internal/controller/impersonatorconfig/impersonator_config_test.go

@@ -29,14 +29,12 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
-	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/clock"
 	"k8s.io/apimachinery/pkg/util/intstr"
 	kubeinformers "k8s.io/client-go/informers"
 	"k8s.io/client-go/kubernetes"
 	kubernetesfake "k8s.io/client-go/kubernetes/fake"
 	coretesting "k8s.io/client-go/testing"
-	"k8s.io/utils/pointer"
 
 	"go.pinniped.dev/generated/latest/apis/concierge/config/v1alpha1"
 	pinnipedfake "go.pinniped.dev/generated/latest/client/concierge/clientset/versioned/fake"
@@ -1032,13 +1030,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 			// validate that we set delete preconditions correctly
 			r.NotEmpty(*deleteOptions)
 			for _, opt := range *deleteOptions {
-				uid := types.UID("uid-1234")
-				r.Equal(metav1.DeleteOptions{
-					Preconditions: &metav1.Preconditions{
-						UID:             &uid,
-						ResourceVersion: pointer.String("rv-5678"),
-					},
-				}, opt)
+				r.Equal(testutil.NewPreconditions("uid-1234", "rv-5678"), opt)
 			}
 		}
 

internal/controller/kubecertagent/legacypodcleaner.go

@@ -40,12 +40,29 @@ func NewLegacyPodCleanerController(
 		controllerlib.Config{
 			Name: "legacy-pod-cleaner-controller",
 			Syncer: controllerlib.SyncFunc(func(ctx controllerlib.Context) error {
-				if err := client.Kubernetes.CoreV1().Pods(ctx.Key.Namespace).Delete(ctx.Context, ctx.Key.Name, metav1.DeleteOptions{}); err != nil {
+				podClient := client.Kubernetes.CoreV1().Pods(ctx.Key.Namespace)
+
+				// avoid blind writes to the API
+				agentPod, err := podClient.Get(ctx.Context, ctx.Key.Name, metav1.GetOptions{})
+				if err != nil {
+					if k8serrors.IsNotFound(err) {
+						return nil
+					}
+					return fmt.Errorf("could not get legacy agent pod: %w", err)
+				}
+
+				if err := podClient.Delete(ctx.Context, ctx.Key.Name, metav1.DeleteOptions{
+					Preconditions: &metav1.Preconditions{
+						UID:             &agentPod.UID,
+						ResourceVersion: &agentPod.ResourceVersion,
+					},
+				}); err != nil {
 					if k8serrors.IsNotFound(err) {
 						return nil
 					}
 					return fmt.Errorf("could not delete legacy agent pod: %w", err)
 				}
+
 				log.Info("deleted legacy kube-cert-agent pod", "pod", klog.KRef(ctx.Key.Namespace, ctx.Key.Name))
 				return nil
 			}),

internal/controller/kubecertagent/legacypodcleaner_test.go

@@ -20,6 +20,7 @@ import (
 
 	"go.pinniped.dev/internal/controllerlib"
 	"go.pinniped.dev/internal/kubeclient"
+	"go.pinniped.dev/internal/testutil"
 	"go.pinniped.dev/internal/testutil/testlogger"
 )
 
@@ -28,9 +29,11 @@ func TestLegacyPodCleanerController(t *testing.T) {
 
 	legacyAgentPodWithoutExtraLabel := &corev1.Pod{
 		ObjectMeta: metav1.ObjectMeta{
-			Namespace: "concierge",
-			Name:      "pinniped-concierge-kube-cert-agent-without-extra-label",
-			Labels:    map[string]string{"kube-cert-agent.pinniped.dev": "true"},
+			Namespace:       "concierge",
+			Name:            "pinniped-concierge-kube-cert-agent-without-extra-label",
+			Labels:          map[string]string{"kube-cert-agent.pinniped.dev": "true"},
+			UID:             "1",
+			ResourceVersion: "2",
 		},
 		Spec:   corev1.PodSpec{},
 		Status: corev1.PodStatus{Phase: corev1.PodRunning},
@@ -40,10 +43,14 @@ func TestLegacyPodCleanerController(t *testing.T) {
 	legacyAgentPodWithExtraLabel.Name = "pinniped-concierge-kube-cert-agent-with-extra-label"
 	legacyAgentPodWithExtraLabel.Labels["extralabel"] = "labelvalue"
 	legacyAgentPodWithExtraLabel.Labels["anotherextralabel"] = "labelvalue"
+	legacyAgentPodWithExtraLabel.UID = "3"
+	legacyAgentPodWithExtraLabel.ResourceVersion = "4"
 
 	nonLegacyAgentPod := legacyAgentPodWithExtraLabel.DeepCopy()
 	nonLegacyAgentPod.Name = "pinniped-concierge-kube-cert-agent-not-legacy"
 	nonLegacyAgentPod.Labels["kube-cert-agent.pinniped.dev"] = "v2"
+	nonLegacyAgentPod.UID = "5"
+	nonLegacyAgentPod.ResourceVersion = "6"
 
 	tests := []struct {
 		name               string
@@ -52,10 +59,12 @@ func TestLegacyPodCleanerController(t *testing.T) {
 		wantDistinctErrors []string
 		wantDistinctLogs   []string
 		wantActions        []coretesting.Action
+		wantDeleteOptions  []metav1.DeleteOptions
 	}{
 		{
-			name:        "no pods",
-			wantActions: []coretesting.Action{},
+			name:              "no pods",
+			wantActions:       []coretesting.Action{},
+			wantDeleteOptions: []metav1.DeleteOptions{},
 		},
 		{
 			name: "mix of pods",
@@ -69,8 +78,12 @@ func TestLegacyPodCleanerController(t *testing.T) {
 				`legacy-pod-cleaner-controller "level"=0 "msg"="deleted legacy kube-cert-agent pod"  "pod"={"name":"pinniped-concierge-kube-cert-agent-with-extra-label","namespace":"concierge"}`,
 			},
 			wantActions: []coretesting.Action{ // the first delete triggers the informer again, but the second invocation triggers a Not Found
+				coretesting.NewGetAction(corev1.Resource("pods").WithVersion("v1"), "concierge", legacyAgentPodWithExtraLabel.Name),
 				coretesting.NewDeleteAction(corev1.Resource("pods").WithVersion("v1"), "concierge", legacyAgentPodWithExtraLabel.Name),
-				coretesting.NewDeleteAction(corev1.Resource("pods").WithVersion("v1"), "concierge", legacyAgentPodWithExtraLabel.Name),
+				coretesting.NewGetAction(corev1.Resource("pods").WithVersion("v1"), "concierge", legacyAgentPodWithExtraLabel.Name),
+			},
+			wantDeleteOptions: []metav1.DeleteOptions{
+				testutil.NewPreconditions("3", "4"),
 			},
 		},
 		{
@@ -89,9 +102,15 @@ func TestLegacyPodCleanerController(t *testing.T) {
 				"could not delete legacy agent pod: some delete error",
 			},
 			wantActions: []coretesting.Action{
+				coretesting.NewGetAction(corev1.Resource("pods").WithVersion("v1"), "concierge", legacyAgentPodWithExtraLabel.Name),
 				coretesting.NewDeleteAction(corev1.Resource("pods").WithVersion("v1"), "concierge", legacyAgentPodWithExtraLabel.Name),
+				coretesting.NewGetAction(corev1.Resource("pods").WithVersion("v1"), "concierge", legacyAgentPodWithExtraLabel.Name),
 				coretesting.NewDeleteAction(corev1.Resource("pods").WithVersion("v1"), "concierge", legacyAgentPodWithExtraLabel.Name),
 			},
+			wantDeleteOptions: []metav1.DeleteOptions{
+				testutil.NewPreconditions("3", "4"),
+				testutil.NewPreconditions("3", "4"),
+			},
 		},
 		{
 			name: "fail to delete because of not found error",
@@ -107,8 +126,30 @@ func TestLegacyPodCleanerController(t *testing.T) {
 			},
 			wantDistinctErrors: []string{""},
 			wantActions: []coretesting.Action{
+				coretesting.NewGetAction(corev1.Resource("pods").WithVersion("v1"), "concierge", legacyAgentPodWithExtraLabel.Name),
 				coretesting.NewDeleteAction(corev1.Resource("pods").WithVersion("v1"), "concierge", legacyAgentPodWithExtraLabel.Name),
 			},
+			wantDeleteOptions: []metav1.DeleteOptions{
+				testutil.NewPreconditions("3", "4"),
+			},
+		},
+		{
+			name: "fail to delete because of not found error on get",
+			kubeObjects: []runtime.Object{
+				legacyAgentPodWithoutExtraLabel, // should not be delete (missing extra label)
+				legacyAgentPodWithExtraLabel,    // should be deleted
+				nonLegacyAgentPod,               // should not be deleted (missing legacy agent label)
+			},
+			addKubeReactions: func(clientset *kubefake.Clientset) {
+				clientset.PrependReactor("get", "*", func(action coretesting.Action) (handled bool, ret runtime.Object, err error) {
+					return true, nil, k8serrors.NewNotFound(action.GetResource().GroupResource(), "")
+				})
+			},
+			wantDistinctErrors: []string{""},
+			wantActions: []coretesting.Action{
+				coretesting.NewGetAction(corev1.Resource("pods").WithVersion("v1"), "concierge", legacyAgentPodWithExtraLabel.Name),
+			},
+			wantDeleteOptions: []metav1.DeleteOptions{},
 		},
 	}
 	for _, tt := range tests {
@@ -120,6 +161,10 @@ func TestLegacyPodCleanerController(t *testing.T) {
 			if tt.addKubeReactions != nil {
 				tt.addKubeReactions(kubeClientset)
 			}
+
+			opts := &[]metav1.DeleteOptions{}
+			trackDeleteClient := testutil.NewDeleteOptionsRecorder(kubeClientset, opts)
+
 			kubeInformers := informers.NewSharedInformerFactory(kubeClientset, 0)
 			log := testlogger.New(t)
 			controller := NewLegacyPodCleanerController(
@@ -127,7 +172,7 @@ func TestLegacyPodCleanerController(t *testing.T) {
 					Namespace: "concierge",
 					Labels:    map[string]string{"extralabel": "labelvalue"},
 				},
-				&kubeclient.Client{Kubernetes: kubeClientset},
+				&kubeclient.Client{Kubernetes: trackDeleteClient},
 				kubeInformers.Core().V1().Pods(),
 				log,
 				controllerlib.WithMaxRetries(1),
@@ -140,6 +185,7 @@ func TestLegacyPodCleanerController(t *testing.T) {
 			assert.Equal(t, tt.wantDistinctErrors, deduplicate(errorMessages), "unexpected errors")
 			assert.Equal(t, tt.wantDistinctLogs, deduplicate(log.Lines()), "unexpected logs")
 			assert.Equal(t, tt.wantActions, kubeClientset.Actions()[2:], "unexpected actions")
+			assert.Equal(t, tt.wantDeleteOptions, *opts, "unexpected delete options")
 		})
 	}
 }

internal/controller/supervisorstorage/garbage_collector.go

@@ -102,7 +102,12 @@ func (c *garbageCollectorController) Sync(ctx controllerlib.Context) error {
 		}
 
 		if garbageCollectAfterTime.Before(frozenClock.Now()) {
-			err = c.kubeClient.CoreV1().Secrets(secret.Namespace).Delete(ctx.Context, secret.Name, metav1.DeleteOptions{})
+			err = c.kubeClient.CoreV1().Secrets(secret.Namespace).Delete(ctx.Context, secret.Name, metav1.DeleteOptions{
+				Preconditions: &metav1.Preconditions{
+					UID:             &secret.UID,
+					ResourceVersion: &secret.ResourceVersion,
+				},
+			})
 			if err != nil {
 				plog.WarningErr("failed to garbage collect resource", err, logKV(secret))
 				continue

internal/controller/supervisorstorage/garbage_collector_test.go

@@ -18,6 +18,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/util/clock"
 	kubeinformers "k8s.io/client-go/informers"
+	"k8s.io/client-go/kubernetes"
 	kubernetesfake "k8s.io/client-go/kubernetes/fake"
 	kubetesting "k8s.io/client-go/testing"
 
@@ -116,6 +117,8 @@ func TestGarbageCollectorControllerSync(t *testing.T) {
 			subject                 controllerlib.Controller
 			kubeInformerClient      *kubernetesfake.Clientset
 			kubeClient              *kubernetesfake.Clientset
+			deleteOptions           *[]metav1.DeleteOptions
+			deleteOptionsRecorder   kubernetes.Interface
 			kubeInformers           kubeinformers.SharedInformerFactory
 			cancelContext           context.Context
 			cancelContextCancelFunc context.CancelFunc
@@ -130,7 +133,7 @@ func TestGarbageCollectorControllerSync(t *testing.T) {
 			// Set this at the last second to allow for injection of server override.
 			subject = GarbageCollectorController(
 				fakeClock,
-				kubeClient,
+				deleteOptionsRecorder,
 				kubeInformers.Core().V1().Secrets(),
 				controllerlib.WithInformer,
 			)
@@ -158,6 +161,8 @@ func TestGarbageCollectorControllerSync(t *testing.T) {
 
 			kubeInformerClient = kubernetesfake.NewSimpleClientset()
 			kubeClient = kubernetesfake.NewSimpleClientset()
+			deleteOptions = &[]metav1.DeleteOptions{}
+			deleteOptionsRecorder = testutil.NewDeleteOptionsRecorder(kubeClient, deleteOptions)
 			kubeInformers = kubeinformers.NewSharedInformerFactory(kubeInformerClient, 0)
 			frozenNow = time.Now().UTC()
 			fakeClock = clock.NewFakeClock(frozenNow)
@@ -193,8 +198,10 @@ func TestGarbageCollectorControllerSync(t *testing.T) {
 			it.Before(func() {
 				firstExpiredSecret := &corev1.Secret{
 					ObjectMeta: metav1.ObjectMeta{
-						Name:      "first expired secret",
-						Namespace: installedInNamespace,
+						Name:            "first expired secret",
+						Namespace:       installedInNamespace,
+						UID:             "uid-123",
+						ResourceVersion: "rv-456",
 						Annotations: map[string]string{
 							"storage.pinniped.dev/garbage-collect-after": frozenNow.Add(-time.Second).Format(time.RFC3339),
 						},
@@ -204,8 +211,10 @@ func TestGarbageCollectorControllerSync(t *testing.T) {
 				r.NoError(kubeClient.Tracker().Add(firstExpiredSecret))
 				secondExpiredSecret := &corev1.Secret{
 					ObjectMeta: metav1.ObjectMeta{
-						Name:      "second expired secret",
-						Namespace: installedInNamespace,
+						Name:            "second expired secret",
+						Namespace:       installedInNamespace,
+						UID:             "uid-789",
+						ResourceVersion: "rv-555",
 						Annotations: map[string]string{
 							"storage.pinniped.dev/garbage-collect-after": frozenNow.Add(-2 * time.Second).Format(time.RFC3339),
 						},
@@ -237,6 +246,13 @@ func TestGarbageCollectorControllerSync(t *testing.T) {
 					},
 					kubeClient.Actions(),
 				)
+				r.ElementsMatch(
+					[]metav1.DeleteOptions{
+						testutil.NewPreconditions("uid-123", "rv-456"),
+						testutil.NewPreconditions("uid-789", "rv-555"),
+					},
+					*deleteOptions,
+				)
 				list, err := kubeClient.CoreV1().Secrets(installedInNamespace).List(context.Background(), metav1.ListOptions{})
 				r.NoError(err)
 				r.Len(list.Items, 2)