Merge branch 'main' into initial_ldap

internal/controller/authenticator/authenticator.go

@@ -5,7 +5,9 @@
 package authenticator
 
 import (
+	"crypto/x509"
 	"encoding/base64"
+	"fmt"
 
 	auth1alpha1 "go.pinniped.dev/generated/latest/apis/concierge/authentication/v1alpha1"
 )
@@ -22,8 +24,18 @@ type Closer interface {
 // nil CA bundle will be returned. If the provided spec contains a CA bundle that is not properly
 // encoded, an error will be returned.
 func CABundle(spec *auth1alpha1.TLSSpec) ([]byte, error) {
-	if spec == nil {
+	if spec == nil || len(spec.CertificateAuthorityData) == 0 {
 		return nil, nil
 	}
-	return base64.StdEncoding.DecodeString(spec.CertificateAuthorityData)
+
+	pem, err := base64.StdEncoding.DecodeString(spec.CertificateAuthorityData)
+	if err != nil {
+		return nil, err
+	}
+
+	if ok := x509.NewCertPool().AppendCertsFromPEM(pem); !ok {
+		return nil, fmt.Errorf("certificateAuthorityData is not valid PEM")
+	}
+
+	return pem, nil
 }

internal/controller/authenticator/authncache/cache.go

@@ -16,6 +16,7 @@ import (
 	loginapi "go.pinniped.dev/generated/latest/apis/concierge/login"
 	"go.pinniped.dev/internal/constable"
 	"go.pinniped.dev/internal/plog"
+	"go.pinniped.dev/internal/valuelesscontext"
 )
 
 // ErrNoSuchAuthenticator is returned by Cache.AuthenticateTokenCredentialRequest() when the requested authenticator is not configured.
@@ -101,7 +102,7 @@ func (c *Cache) AuthenticateTokenCredentialRequest(ctx context.Context, req *log
 
 	// The incoming context could have an audience. Since we do not want to handle audiences right now, do not pass it
 	// through directly to the authentication webhook.
-	ctx = valuelessContext{ctx}
+	ctx = valuelesscontext.New(ctx)
 
 	// Call the selected authenticator.
 	resp, authenticated, err := val.AuthenticateToken(ctx, req.Spec.Token)
@@ -119,7 +120,3 @@ func (c *Cache) AuthenticateTokenCredentialRequest(ctx context.Context, req *log
 	}
 	return respUser, nil
 }
-
-type valuelessContext struct{ context.Context }
-
-func (valuelessContext) Value(interface{}) interface{} { return nil }

internal/controller/authenticator/webhookcachefiller/webhookcachefiller_test.go

@@ -135,6 +135,15 @@ func TestNewWebhookAuthenticator(t *testing.T) {
 		require.EqualError(t, err, "invalid TLS configuration: illegal base64 data at input byte 7")
 	})
 
+	t.Run("invalid pem data", func(t *testing.T) {
+		res, err := newWebhookAuthenticator(&auth1alpha1.WebhookAuthenticatorSpec{
+			Endpoint: "https://example.com",
+			TLS:      &auth1alpha1.TLSSpec{CertificateAuthorityData: base64.StdEncoding.EncodeToString([]byte("bad data"))},
+		}, ioutil.TempFile, clientcmd.WriteToFile)
+		require.Nil(t, res)
+		require.EqualError(t, err, "invalid TLS configuration: certificateAuthorityData is not valid PEM")
+	})
+
 	t.Run("valid config with no TLS spec", func(t *testing.T) {
 		res, err := newWebhookAuthenticator(&auth1alpha1.WebhookAuthenticatorSpec{
 			Endpoint: "https://example.com",

internal/controller/kubecertagent/kubecertagent.go

@@ -64,6 +64,9 @@ type AgentConfig struct {
 	// NamePrefix will be prefixed to all agent pod names.
 	NamePrefix string
 
+	// ServiceAccountName is the service account under which to run the agent pods.
+	ServiceAccountName string
+
 	// ContainerImagePullSecrets is a list of names of Kubernetes Secret objects that will be used as
 	// ImagePullSecrets on the kube-cert-agent pods.
 	ContainerImagePullSecrets []string
@@ -472,6 +475,7 @@ func (c *agentController) newAgentDeployment(controllerManagerPod *corev1.Pod) *
 					RestartPolicy:                corev1.RestartPolicyAlways,
 					NodeSelector:                 controllerManagerPod.Spec.NodeSelector,
 					AutomountServiceAccountToken: pointer.BoolPtr(false),
+					ServiceAccountName:           c.cfg.ServiceAccountName,
 					NodeName:                     controllerManagerPod.Spec.NodeName,
 					Tolerations:                  controllerManagerPod.Spec.Tolerations,
 					// We need to run the agent pod as root since the file permissions

internal/controller/kubecertagent/kubecertagent_test.go

@@ -123,6 +123,7 @@ func TestAgentController(t *testing.T) {
 					}},
 					RestartPolicy:                 corev1.RestartPolicyAlways,
 					TerminationGracePeriodSeconds: pointer.Int64Ptr(0),
+					ServiceAccountName:            "test-service-account-name",
 					AutomountServiceAccountToken:  pointer.BoolPtr(false),
 					SecurityContext: &corev1.PodSecurityContext{
 						RunAsUser:  pointer.Int64Ptr(0),
@@ -672,6 +673,7 @@ func TestAgentController(t *testing.T) {
 				AgentConfig{
 					Namespace:                 "concierge",
 					ContainerImage:            "pinniped-server-image",
+					ServiceAccountName:        "test-service-account-name",
 					NamePrefix:                "pinniped-concierge-kube-cert-agent-",
 					ContainerImagePullSecrets: []string{"pinniped-image-pull-secret"},
 					CredentialIssuerName:      "pinniped-concierge-config",

internal/controller/supervisorconfig/upstreamwatcher/oidc_upstream_watcher.go

@@ -13,6 +13,7 @@ import (
 	"net/http"
 	"net/url"
 	"sort"
+	"strings"
 	"time"
 
 	"github.com/coreos/go-oidc/v3/oidc"
@@ -269,11 +270,17 @@ func (c *oidcWatcherController) validateIssuer(ctx context.Context, upstream *v1
 
 		discoveredProvider, err = oidc.NewProvider(oidc.ClientContext(ctx, httpClient), upstream.Spec.Issuer)
 		if err != nil {
+			const klogLevelTrace = 6
+			c.log.V(klogLevelTrace).WithValues(
+				"namespace", upstream.Namespace,
+				"name", upstream.Name,
+				"issuer", upstream.Spec.Issuer,
+			).Error(err, "failed to perform OIDC discovery")
 			return &v1alpha1.Condition{
 				Type:    typeOIDCDiscoverySucceeded,
 				Status:  v1alpha1.ConditionFalse,
 				Reason:  reasonUnreachable,
-				Message: fmt.Sprintf("failed to perform OIDC discovery against %q", upstream.Spec.Issuer),
+				Message: fmt.Sprintf("failed to perform OIDC discovery against %q:\n%s", upstream.Spec.Issuer, truncateNonOIDCErr(err)),
 			}
 		}
 
@@ -428,3 +435,14 @@ func (*oidcWatcherController) computeScopes(additionalScopes []string) []string
 	sort.Strings(scopes)
 	return scopes
 }
+
+func truncateNonOIDCErr(err error) string {
+	const max = 100
+	msg := err.Error()
+
+	if len(msg) <= max || strings.HasPrefix(msg, "oidc:") {
+		return msg
+	}
+
+	return msg[:max] + fmt.Sprintf(" [truncated %d chars]", len(msg)-max)
+}

internal/controller/supervisorconfig/upstreamwatcher/oidc_upstream_watcher_test.go

@@ -370,7 +370,7 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) {
 			inputUpstreams: []runtime.Object{&v1alpha1.OIDCIdentityProvider{
 				ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName},
 				Spec: v1alpha1.OIDCIdentityProviderSpec{
-					Issuer:              "invalid-url",
+					Issuer:              "invalid-url-that-is-really-really-long",
 					Client:              v1alpha1.OIDCClient{SecretName: testSecretName},
 					AuthorizationConfig: v1alpha1.OIDCAuthorizationConfig{AdditionalScopes: testAdditionalScopes},
 				},
@@ -382,9 +382,10 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) {
 			}},
 			wantErr: controllerlib.ErrSyntheticRequeue.Error(),
 			wantLogs: []string{
+				`oidc-upstream-observer "msg"="failed to perform OIDC discovery" "error"="Get \"invalid-url-that-is-really-really-long/.well-known/openid-configuration\": unsupported protocol scheme \"\"" "issuer"="invalid-url-that-is-really-really-long" "name"="test-name" "namespace"="test-namespace"`,
 				`oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`,
-				`oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="failed to perform OIDC discovery against \"invalid-url\"" "reason"="Unreachable" "status"="False" "type"="OIDCDiscoverySucceeded"`,
-				`oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="failed to perform OIDC discovery against \"invalid-url\"" "name"="test-name" "namespace"="test-namespace" "reason"="Unreachable" "type"="OIDCDiscoverySucceeded"`,
+				`oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="failed to perform OIDC discovery against \"invalid-url-that-is-really-really-long\":\nGet \"invalid-url-that-is-really-really-long/.well-known/openid-configuration\": unsupported protocol  [truncated 9 chars]" "reason"="Unreachable" "status"="False" "type"="OIDCDiscoverySucceeded"`,
+				`oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="failed to perform OIDC discovery against \"invalid-url-that-is-really-really-long\":\nGet \"invalid-url-that-is-really-really-long/.well-known/openid-configuration\": unsupported protocol  [truncated 9 chars]" "name"="test-name" "namespace"="test-namespace" "reason"="Unreachable" "type"="OIDCDiscoverySucceeded"`,
 			},
 			wantResultingCache: []provider.UpstreamOIDCIdentityProviderI{},
 			wantResultingUpstreams: []v1alpha1.OIDCIdentityProvider{{
@@ -404,7 +405,8 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) {
 							Status:             "False",
 							LastTransitionTime: now,
 							Reason:             "Unreachable",
-							Message:            `failed to perform OIDC discovery against "invalid-url"`,
+							Message: `failed to perform OIDC discovery against "invalid-url-that-is-really-really-long":
+Get "invalid-url-that-is-really-really-long/.well-known/openid-configuration": unsupported protocol  [truncated 9 chars]`,
 						},
 					},
 				},
@@ -600,6 +602,151 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) {
 				},
 			}},
 		},
+		{
+			name: "existing valid upstream with trailing slash",
+			inputUpstreams: []runtime.Object{&v1alpha1.OIDCIdentityProvider{
+				ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234},
+				Spec: v1alpha1.OIDCIdentityProviderSpec{
+					Issuer:              testIssuerURL + "/ends-with-slash/",
+					TLS:                 &v1alpha1.TLSSpec{CertificateAuthorityData: testIssuerCABase64},
+					Client:              v1alpha1.OIDCClient{SecretName: testSecretName},
+					AuthorizationConfig: v1alpha1.OIDCAuthorizationConfig{AdditionalScopes: testAdditionalScopes},
+					Claims:              v1alpha1.OIDCClaims{Groups: testGroupsClaim, Username: testUsernameClaim},
+				},
+				Status: v1alpha1.OIDCIdentityProviderStatus{
+					Phase: "Ready",
+					Conditions: []v1alpha1.Condition{
+						{Type: "ClientCredentialsValid", Status: "True", LastTransitionTime: earlier, Reason: "Success", Message: "loaded client credentials"},
+						{Type: "OIDCDiscoverySucceeded", Status: "True", LastTransitionTime: earlier, Reason: "Success", Message: "discovered issuer configuration"},
+					},
+				},
+			}},
+			inputSecrets: []runtime.Object{&corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testSecretName},
+				Type:       "secrets.pinniped.dev/oidc-client",
+				Data:       testValidSecretData,
+			}},
+			wantLogs: []string{
+				`oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`,
+				`oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="discovered issuer configuration" "reason"="Success" "status"="True" "type"="OIDCDiscoverySucceeded"`,
+			},
+			wantResultingCache: []provider.UpstreamOIDCIdentityProviderI{
+				&oidctestutil.TestUpstreamOIDCIdentityProvider{
+					Name:             testName,
+					ClientID:         testClientID,
+					AuthorizationURL: *testIssuerAuthorizeURL,
+					Scopes:           testExpectedScopes,
+					UsernameClaim:    testUsernameClaim,
+					GroupsClaim:      testGroupsClaim,
+				},
+			},
+			wantResultingUpstreams: []v1alpha1.OIDCIdentityProvider{{
+				ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234},
+				Status: v1alpha1.OIDCIdentityProviderStatus{
+					Phase: "Ready",
+					Conditions: []v1alpha1.Condition{
+						{Type: "ClientCredentialsValid", Status: "True", LastTransitionTime: earlier, Reason: "Success", Message: "loaded client credentials", ObservedGeneration: 1234},
+						{Type: "OIDCDiscoverySucceeded", Status: "True", LastTransitionTime: earlier, Reason: "Success", Message: "discovered issuer configuration", ObservedGeneration: 1234},
+					},
+				},
+			}},
+		},
+		{
+			name: "issuer is invalid URL, missing trailing slash",
+			inputUpstreams: []runtime.Object{&v1alpha1.OIDCIdentityProvider{
+				ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName},
+				Spec: v1alpha1.OIDCIdentityProviderSpec{
+					Issuer:              testIssuerURL + "/ends-with-slash",
+					TLS:                 &v1alpha1.TLSSpec{CertificateAuthorityData: testIssuerCABase64},
+					Client:              v1alpha1.OIDCClient{SecretName: testSecretName},
+					AuthorizationConfig: v1alpha1.OIDCAuthorizationConfig{AdditionalScopes: testAdditionalScopes},
+				},
+			}},
+			inputSecrets: []runtime.Object{&corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testSecretName},
+				Type:       "secrets.pinniped.dev/oidc-client",
+				Data:       testValidSecretData,
+			}},
+			wantErr: controllerlib.ErrSyntheticRequeue.Error(),
+			wantLogs: []string{
+				`oidc-upstream-observer "msg"="failed to perform OIDC discovery" "error"="oidc: issuer did not match the issuer returned by provider, expected \"` + testIssuerURL + `/ends-with-slash\" got \"` + testIssuerURL + `/ends-with-slash/\"" "issuer"="` + testIssuerURL + `/ends-with-slash" "name"="test-name" "namespace"="test-namespace"`,
+				`oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`,
+				`oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="failed to perform OIDC discovery against \"` + testIssuerURL + `/ends-with-slash\":\noidc: issuer did not match the issuer returned by provider, expected \"` + testIssuerURL + `/ends-with-slash\" got \"` + testIssuerURL + `/ends-with-slash/\"" "reason"="Unreachable" "status"="False" "type"="OIDCDiscoverySucceeded"`,
+				`oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="failed to perform OIDC discovery against \"` + testIssuerURL + `/ends-with-slash\":\noidc: issuer did not match the issuer returned by provider, expected \"` + testIssuerURL + `/ends-with-slash\" got \"` + testIssuerURL + `/ends-with-slash/\"" "name"="test-name" "namespace"="test-namespace" "reason"="Unreachable" "type"="OIDCDiscoverySucceeded"`,
+			},
+			wantResultingCache: []provider.UpstreamOIDCIdentityProviderI{},
+			wantResultingUpstreams: []v1alpha1.OIDCIdentityProvider{{
+				ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName},
+				Status: v1alpha1.OIDCIdentityProviderStatus{
+					Phase: "Error",
+					Conditions: []v1alpha1.Condition{
+						{
+							Type:               "ClientCredentialsValid",
+							Status:             "True",
+							LastTransitionTime: now,
+							Reason:             "Success",
+							Message:            "loaded client credentials",
+						},
+						{
+							Type:               "OIDCDiscoverySucceeded",
+							Status:             "False",
+							LastTransitionTime: now,
+							Reason:             "Unreachable",
+							Message: `failed to perform OIDC discovery against "` + testIssuerURL + `/ends-with-slash":
+oidc: issuer did not match the issuer returned by provider, expected "` + testIssuerURL + `/ends-with-slash" got "` + testIssuerURL + `/ends-with-slash/"`,
+						},
+					},
+				},
+			}},
+		},
+		{
+			name: "issuer is invalid URL, extra trailing slash",
+			inputUpstreams: []runtime.Object{&v1alpha1.OIDCIdentityProvider{
+				ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName},
+				Spec: v1alpha1.OIDCIdentityProviderSpec{
+					Issuer:              testIssuerURL + "/",
+					TLS:                 &v1alpha1.TLSSpec{CertificateAuthorityData: testIssuerCABase64},
+					Client:              v1alpha1.OIDCClient{SecretName: testSecretName},
+					AuthorizationConfig: v1alpha1.OIDCAuthorizationConfig{AdditionalScopes: testAdditionalScopes},
+				},
+			}},
+			inputSecrets: []runtime.Object{&corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testSecretName},
+				Type:       "secrets.pinniped.dev/oidc-client",
+				Data:       testValidSecretData,
+			}},
+			wantErr: controllerlib.ErrSyntheticRequeue.Error(),
+			wantLogs: []string{
+				`oidc-upstream-observer "msg"="failed to perform OIDC discovery" "error"="oidc: issuer did not match the issuer returned by provider, expected \"` + testIssuerURL + `/\" got \"` + testIssuerURL + `\"" "issuer"="` + testIssuerURL + `/" "name"="test-name" "namespace"="test-namespace"`,
+				`oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`,
+				`oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="failed to perform OIDC discovery against \"` + testIssuerURL + `/\":\noidc: issuer did not match the issuer returned by provider, expected \"` + testIssuerURL + `/\" got \"` + testIssuerURL + `\"" "reason"="Unreachable" "status"="False" "type"="OIDCDiscoverySucceeded"`,
+				`oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="failed to perform OIDC discovery against \"` + testIssuerURL + `/\":\noidc: issuer did not match the issuer returned by provider, expected \"` + testIssuerURL + `/\" got \"` + testIssuerURL + `\"" "name"="test-name" "namespace"="test-namespace" "reason"="Unreachable" "type"="OIDCDiscoverySucceeded"`,
+			},
+			wantResultingCache: []provider.UpstreamOIDCIdentityProviderI{},
+			wantResultingUpstreams: []v1alpha1.OIDCIdentityProvider{{
+				ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName},
+				Status: v1alpha1.OIDCIdentityProviderStatus{
+					Phase: "Error",
+					Conditions: []v1alpha1.Condition{
+						{
+							Type:               "ClientCredentialsValid",
+							Status:             "True",
+							LastTransitionTime: now,
+							Reason:             "Success",
+							Message:            "loaded client credentials",
+						},
+						{
+							Type:               "OIDCDiscoverySucceeded",
+							Status:             "False",
+							LastTransitionTime: now,
+							Reason:             "Unreachable",
+							Message: `failed to perform OIDC discovery against "` + testIssuerURL + `/":
+oidc: issuer did not match the issuer returned by provider, expected "` + testIssuerURL + `/" got "` + testIssuerURL + `"`,
+						},
+					},
+				},
+			}},
+		},
 	}
 	for _, tt := range tests {
 		tt := tt
@@ -728,5 +875,25 @@ func newTestIssuer(t *testing.T) (string, string) {
 		})
 	})
 
+	// handle the four issuer with trailing slash configs
+
+	// valid case in= out=
+	// handled above at the root of testURL
+
+	// valid case in=/ out=/
+	mux.HandleFunc("/ends-with-slash/.well-known/openid-configuration", func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("content-type", "application/json")
+		_ = json.NewEncoder(w).Encode(&providerJSON{
+			Issuer:  testURL + "/ends-with-slash/",
+			AuthURL: "https://example.com/authorize",
+		})
+	})
+
+	// invalid case in= out=/
+	// can be tested using /ends-with-slash/ endpoint
+
+	// invalid case in=/ out=
+	// can be tested using root endpoint
+
 	return caBundlePEM, testURL
 }

internal/controller/supervisorstorage/garbage_collector.go

@@ -1,4 +1,4 @@
-// Copyright 2020 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package supervisorstorage
@@ -59,7 +59,7 @@ func GarbageCollectorController(
 					return isSecretWithGCAnnotation(oldObj) || isSecretWithGCAnnotation(newObj)
 				},
 				DeleteFunc: func(obj metav1.Object) bool { return false }, // ignore all deletes
-				ParentFunc: nil,
+				ParentFunc: pinnipedcontroller.SingletonQueue(),
 			},
 			controllerlib.InformerOption{},
 		),
@@ -67,16 +67,20 @@ func GarbageCollectorController(
 }
 
 func (c *garbageCollectorController) Sync(ctx controllerlib.Context) error {
+	// make sure we have a consistent, static meaning for the current time during the sync loop
+	frozenClock := clock.NewFakeClock(c.clock.Now())
+
 	// The Sync method is triggered upon any change to any Secret, which would make this
 	// controller too chatty, so it rate limits itself to a more reasonable interval.
 	// Note that even during a period when no secrets are changing, it will still run
 	// at the informer's full-resync interval (as long as there are some secrets).
-	if c.clock.Now().Sub(c.timeOfMostRecentSweep) < minimumRepeatInterval {
+	if since := frozenClock.Since(c.timeOfMostRecentSweep); since < minimumRepeatInterval {
+		ctx.Queue.AddAfter(ctx.Key, minimumRepeatInterval-since)
 		return nil
 	}
 
 	plog.Info("starting storage garbage collection sweep")
-	c.timeOfMostRecentSweep = c.clock.Now()
+	c.timeOfMostRecentSweep = frozenClock.Now()
 
 	listOfSecrets, err := c.secretInformer.Lister().List(labels.Everything())
 	if err != nil {
@@ -97,7 +101,7 @@ func (c *garbageCollectorController) Sync(ctx controllerlib.Context) error {
 			continue
 		}
 
-		if garbageCollectAfterTime.Before(c.clock.Now()) {
+		if garbageCollectAfterTime.Before(frozenClock.Now()) {
 			err = c.kubeClient.CoreV1().Secrets(secret.Namespace).Delete(ctx.Context, secret.Name, metav1.DeleteOptions{})
 			if err != nil {
 				plog.WarningErr("failed to garbage collect resource", err, logKV(secret))

internal/controller/supervisorstorage/garbage_collector_test.go

@@ -66,6 +66,10 @@ func TestGarbageCollectorControllerInformerFilters(t *testing.T) {
 					r.True(subject.Update(secretWithAnnotation, otherSecret))
 					r.True(subject.Update(otherSecret, secretWithAnnotation))
 				})
+
+				it("returns the same singleton key", func() {
+					r.Equal(controllerlib.Key{}, subject.Parent(secretWithAnnotation))
+				})
 			})
 
 			when("any Secret with the required annotation is deleted", func() {
@@ -136,9 +140,10 @@ func TestGarbageCollectorControllerSync(t *testing.T) {
 				Context: cancelContext,
 				Name:    subject.Name(),
 				Key: controllerlib.Key{
-					Namespace: "",
-					Name:      "",
+					Namespace: "foo",
+					Name:      "bar",
 				},
+				Queue: &testQueue{t: t},
 			}
 
 			// Must start informers before calling TestRunSynchronously()
@@ -262,16 +267,23 @@ func TestGarbageCollectorControllerSync(t *testing.T) {
 				// Run sync once with the current time set to frozenTime.
 				r.NoError(controllerlib.TestSync(t, subject, *syncContext))
 				require.Empty(t, kubeClient.Actions())
+				r.False(syncContext.Queue.(*testQueue).called)
 
 				// Run sync again when not enough time has passed since the most recent run, so no delete
 				// operations should happen even though there is a expired secret now.
 				fakeClock.Step(29 * time.Second)
 				r.NoError(controllerlib.TestSync(t, subject, *syncContext))
 				require.Empty(t, kubeClient.Actions())
+				r.True(syncContext.Queue.(*testQueue).called)
+				r.Equal(controllerlib.Key{Namespace: "foo", Name: "bar"}, syncContext.Queue.(*testQueue).key) // assert key is passed through
+				r.Equal(time.Second, syncContext.Queue.(*testQueue).duration)                                 // assert that we get the exact requeue time
+
+				syncContext.Queue = &testQueue{t: t} // reset the queue for the next sync
 
 				// Step to the exact threshold and run Sync again. Now we are past the rate limiting period.
-				fakeClock.Step(1*time.Second + 1*time.Millisecond)
+				fakeClock.Step(time.Second)
 				r.NoError(controllerlib.TestSync(t, subject, *syncContext))
+				r.False(syncContext.Queue.(*testQueue).called)
 
 				// It should have deleted the expired secret.
 				r.ElementsMatch(
@@ -381,3 +393,23 @@ func TestGarbageCollectorControllerSync(t *testing.T) {
 		})
 	}, spec.Parallel(), spec.Report(report.Terminal{}))
 }
+
+type testQueue struct {
+	t *testing.T
+
+	called   bool
+	key      controllerlib.Key
+	duration time.Duration
+
+	controllerlib.Queue // panic if any other methods called
+}
+
+func (q *testQueue) AddAfter(key controllerlib.Key, duration time.Duration) {
+	q.t.Helper()
+
+	require.False(q.t, q.called, "AddAfter should only be called once")
+
+	q.called = true
+	q.key = key
+	q.duration = duration
+}