apicerts: return error on missing pre-requirements

This change updates the apicerts controllers to return an error when
they cannot successfully complete their Sync func.  i.e. `return nil`
is reserved for cases where the controller has fully completed its
job with no errors.  This makes it clear when a controller has
wedged - i.e. it is waiting on some other controller or process to
perform some action before it can complete.  The controller lib's
queue will exponentially back off and thus there is no need to be
concerned with returning an error indefinitely or infinite log spam.
Even when the kubelet throws away container logs, it will be clear
what controllers are wedged based on the last hour or so of logs.

Signed-off-by: Monis Khan <mok@vmware.com>

internal/controller/apicerts/apiservice_updater.go

@@ -60,7 +60,9 @@ func (c *apiServiceUpdaterController) Sync(ctx controllerlib.Context) error {
 	if notFound {
 		// The secret does not exist yet, so nothing to do.
 		klog.Info("apiServiceUpdaterController Sync found that the secret does not exist yet or was deleted")
-		return nil
+		//nolint: goerr113
+		return fmt.Errorf("apiServiceUpdaterController missing pre-requirements, secret %s/%s does not exist: %w",
+			c.namespace, c.certsSecretResourceName, controllerlib.ErrSyntheticRequeue)
 	}
 
 	// Update the APIService to give it the new CA bundle.

internal/controller/apicerts/apiservice_updater_test.go

@@ -173,7 +173,8 @@ func TestAPIServiceUpdaterControllerSync(t *testing.T) {
 			it("does not need to make any API calls with its API client", func() {
 				startInformersAndController()
 				err := controllerlib.TestSync(t, subject, *syncContext)
-				r.NoError(err)
+				r.EqualError(err, "apiServiceUpdaterController missing pre-requirements, secret some-namespace/some-resource-name does not exist: synthetic requeue request")
+				r.True(errors.Is(err, controllerlib.ErrSyntheticRequeue))
 				r.Empty(aggregatorAPIClient.Actions())
 			})
 		})

internal/controller/apicerts/certs_expirer.go

@@ -71,16 +71,17 @@ func (c *certsExpirerController) Sync(ctx controllerlib.Context) error {
 	}
 	if notFound {
 		klog.Info("certsExpirerController Sync found that the secret does not exist yet or was deleted")
-		return nil
+		//nolint: goerr113
+		return fmt.Errorf("certsExpirerController missing pre-requirements, secret %s/%s does not exist: %w",
+			c.namespace, c.certsSecretResourceName, controllerlib.ErrSyntheticRequeue)
 	}
 
 	notBefore, notAfter, err := getCertBounds(secret)
 	if err != nil {
-		// If we can't read the cert, then really all we can do is log something,
+		// If we can't read the cert, then we are wedged and need to complain loudly.
-		// since if we returned an error then the controller lib would just call us
+		// The controller lib code will retry indefinitely, but will back off exponentially.
-		// again and again, which would probably yield the same results.
+		//nolint: goerr113
-		klog.Warningf("certsExpirerController Sync found that the secret is malformed: %s", err.Error())
+		return fmt.Errorf("certsExpirerController Sync found that the secret is malformed: %w", err)
-		return nil
 	}
 
 	certAge := time.Since(notBefore)

internal/controller/apicerts/certs_expirer_test.go

@@ -127,11 +127,13 @@ func TestExpirerControllerSync(t *testing.T) {
 		{
 			name:       "secret does not exist",
 			wantDelete: false,
+			wantError:  "certsExpirerController missing pre-requirements, secret some-namespace/some-resource-name does not exist: synthetic requeue request",
 		},
 		{
 			name:           "secret missing key",
 			fillSecretData: func(t *testing.T, m map[string][]byte) {},
 			wantDelete:     false,
+			wantError:      "certsExpirerController Sync found that the secret is malformed: failed to find certificate",
 		},
 		{
 			name:        "lifetime below threshold",
@@ -209,6 +211,7 @@ func TestExpirerControllerSync(t *testing.T) {
 				require.NoError(t, err)
 			},
 			wantDelete: false,
+			wantError:  "certsExpirerController Sync found that the secret is malformed: failed to decode certificate PEM",
 		},
 	}
 	for _, test := range tests {

internal/controller/apicerts/certs_observer.go

@@ -58,7 +58,9 @@ func (c *certsObserverController) Sync(_ controllerlib.Context) error {
 		klog.Info("certsObserverController Sync found that the secret does not exist yet or was deleted")
 		// The secret does not exist yet or was deleted.
 		c.dynamicCertProvider.Set(nil, nil)
-		return nil
+		//nolint: goerr113
+		return fmt.Errorf("certsObserverController missing pre-requirements, secret %s/%s does not exist: %w",
+			c.namespace, c.certsSecretResourceName, controllerlib.ErrSyntheticRequeue)
 	}
 
 	// Mutate the in-memory cert provider to update with the latest cert values.

internal/controller/apicerts/certs_observer_test.go

@@ -5,6 +5,7 @@ package apicerts
 
 import (
 	"context"
+	"errors"
 	"testing"
 	"time"
 
@@ -167,7 +168,8 @@ func TestObserverControllerSync(t *testing.T) {
 			it("sets the dynamicCertProvider's cert and key to nil", func() {
 				startInformersAndController()
 				err := controllerlib.TestSync(t, subject, *syncContext)
-				r.NoError(err)
+				r.EqualError(err, "certsObserverController missing pre-requirements, secret some-namespace/some-resource-name does not exist: synthetic requeue request")
+				r.True(errors.Is(err, controllerlib.ErrSyntheticRequeue))
 
 				actualCertChain, actualKey := dynamicCertProvider.CurrentCertKeyContent()
 				r.Nil(actualCertChain)