From 349d3dad83e6018fcbccebdcd9c71dfc3e540db6 Mon Sep 17 00:00:00 2001 From: Matt Moyer Date: Thu, 27 May 2021 11:13:10 -0500 Subject: [PATCH] Make temporary errors return Pending in impersonatorconfig. Signed-off-by: Matt Moyer --- .../impersonatorconfig/impersonator_config.go | 14 ++++- .../impersonator_config_test.go | 62 +++++++++++-------- .../concierge_impersonation_proxy_test.go | 4 ++ 3 files changed, 52 insertions(+), 28 deletions(-) diff --git a/internal/controller/impersonatorconfig/impersonator_config.go b/internal/controller/impersonatorconfig/impersonator_config.go index 789c4caf..6dfc243c 100644 --- a/internal/controller/impersonatorconfig/impersonator_config.go +++ b/internal/controller/impersonatorconfig/impersonator_config.go @@ -168,7 +168,7 @@ func (c *impersonatorConfigController) Sync(syncCtx controllerlib.Context) error strategy = &v1alpha1.CredentialIssuerStrategy{ Type: v1alpha1.ImpersonationProxyStrategyType, Status: v1alpha1.ErrorStrategyStatus, - Reason: v1alpha1.ErrorDuringSetupStrategyReason, + Reason: strategyReasonForError(err), Message: err.Error(), LastUpdateTime: metav1.NewTime(c.clock.Now()), } @@ -189,6 +189,18 @@ func (c *impersonatorConfigController) Sync(syncCtx controllerlib.Context) error return err } +// strategyReasonForError returns the proper v1alpha1.StrategyReason for a sync error. Some errors are occasionally +// expected because there are multiple pods running, in these cases we should report a Pending reason and we'll +// recover on a following sync. +func strategyReasonForError(err error) v1alpha1.StrategyReason { + switch { + case k8serrors.IsConflict(err), k8serrors.IsAlreadyExists(err): + return v1alpha1.PendingStrategyReason + default: + return v1alpha1.ErrorDuringSetupStrategyReason + } +} + type certNameInfo struct { // ready will be true when the certificate name information is known. // ready will be false when it is pending because we are waiting for a load balancer to get assigned an ip/hostname. diff --git a/internal/controller/impersonatorconfig/impersonator_config_test.go b/internal/controller/impersonatorconfig/impersonator_config_test.go index 2a763603..91db0a76 100644 --- a/internal/controller/impersonatorconfig/impersonator_config_test.go +++ b/internal/controller/impersonatorconfig/impersonator_config_test.go @@ -25,6 +25,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" + k8serrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" @@ -856,17 +857,21 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { return s } - var newPendingStrategy = func() v1alpha1.CredentialIssuerStrategy { + var newPendingStrategy = func(msg string) v1alpha1.CredentialIssuerStrategy { return v1alpha1.CredentialIssuerStrategy{ Type: v1alpha1.ImpersonationProxyStrategyType, Status: v1alpha1.ErrorStrategyStatus, Reason: v1alpha1.PendingStrategyReason, - Message: "waiting for load balancer Service to be assigned IP or hostname", + Message: msg, LastUpdateTime: metav1.NewTime(frozenNow), Frontend: nil, } } + var newPendingStrategyWaitingForLB = func() v1alpha1.CredentialIssuerStrategy { + return newPendingStrategy("waiting for load balancer Service to be assigned IP or hostname") + } + var newErrorStrategy = func(msg string) v1alpha1.CredentialIssuerStrategy { return v1alpha1.CredentialIssuerStrategy{ Type: v1alpha1.ImpersonationProxyStrategyType, @@ -1204,7 +1209,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { requireNodesListed(kubeAPIClient.Actions()[0]) requireLoadBalancerWasCreated(kubeAPIClient.Actions()[1]) requireCASecretWasCreated(kubeAPIClient.Actions()[2]) - requireCredentialIssuer(newPendingStrategy()) + requireCredentialIssuer(newPendingStrategyWaitingForLB()) requireSigningCertProviderIsEmpty() }) }) @@ -1223,7 +1228,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { r.Len(kubeAPIClient.Actions(), 2) requireNodesListed(kubeAPIClient.Actions()[0]) requireCASecretWasCreated(kubeAPIClient.Actions()[1]) - requireCredentialIssuer(newPendingStrategy()) + requireCredentialIssuer(newPendingStrategyWaitingForLB()) requireSigningCertProviderIsEmpty() }) }) @@ -1242,7 +1247,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { r.Len(kubeAPIClient.Actions(), 2) requireNodesListed(kubeAPIClient.Actions()[0]) requireCASecretWasCreated(kubeAPIClient.Actions()[1]) - requireCredentialIssuer(newPendingStrategy()) + requireCredentialIssuer(newPendingStrategyWaitingForLB()) requireSigningCertProviderIsEmpty() }) }) @@ -1492,7 +1497,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { requireLoadBalancerWasCreated(kubeAPIClient.Actions()[1]) requireCASecretWasCreated(kubeAPIClient.Actions()[2]) requireTLSServerIsRunningWithoutCerts() - requireCredentialIssuer(newPendingStrategy()) + requireCredentialIssuer(newPendingStrategyWaitingForLB()) requireSigningCertProviderIsEmpty() }) @@ -1527,7 +1532,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { requireNodesListed(kubeAPIClient.Actions()[0]) requireCASecretWasCreated(kubeAPIClient.Actions()[1]) requireTLSServerIsRunningWithoutCerts() - requireCredentialIssuer(newPendingStrategy()) + requireCredentialIssuer(newPendingStrategyWaitingForLB()) requireSigningCertProviderIsEmpty() }) @@ -1665,7 +1670,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { require.Equal(t, lbService.Annotations, annotations) requireCASecretWasCreated(kubeAPIClient.Actions()[2]) requireTLSServerIsRunningWithoutCerts() - requireCredentialIssuer(newPendingStrategy()) + requireCredentialIssuer(newPendingStrategyWaitingForLB()) requireSigningCertProviderIsEmpty() }) }) @@ -1760,7 +1765,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { requireCASecretWasCreated(kubeAPIClient.Actions()[2]) // Check that the server is running without certs. requireTLSServerIsRunningWithoutCerts() - requireCredentialIssuer(newPendingStrategy()) + requireCredentialIssuer(newPendingStrategyWaitingForLB()) requireSigningCertProviderIsEmpty() }) }) @@ -2140,7 +2145,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { requireNodesListed(kubeAPIClient.Actions()[0]) requireLoadBalancerWasCreated(kubeAPIClient.Actions()[1]) requireCASecretWasCreated(kubeAPIClient.Actions()[2]) - requireCredentialIssuer(newPendingStrategy()) + requireCredentialIssuer(newPendingStrategyWaitingForLB()) requireSigningCertProviderIsEmpty() // Simulate the informer cache's background update from its watch. @@ -2175,7 +2180,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { requireTLSServerIsRunningWithoutCerts() r.Len(kubeAPIClient.Actions(), 5) requireLoadBalancerWasCreated(kubeAPIClient.Actions()[4]) - requireCredentialIssuer(newPendingStrategy()) + requireCredentialIssuer(newPendingStrategyWaitingForLB()) requireSigningCertProviderIsEmpty() }) }) @@ -2206,7 +2211,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { requireNodesListed(kubeAPIClient.Actions()[0]) requireClusterIPWasCreated(kubeAPIClient.Actions()[1]) requireCASecretWasCreated(kubeAPIClient.Actions()[2]) - requireCredentialIssuer(newPendingStrategy()) + requireCredentialIssuer(newPendingStrategyWaitingForLB()) requireSigningCertProviderIsEmpty() // Simulate the informer cache's background update from its watch. @@ -2244,7 +2249,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { requireTLSServerIsRunningWithoutCerts() r.Len(kubeAPIClient.Actions(), 5) requireClusterIPWasCreated(kubeAPIClient.Actions()[4]) - requireCredentialIssuer(newPendingStrategy()) + requireCredentialIssuer(newPendingStrategyWaitingForLB()) requireSigningCertProviderIsEmpty() }) }) @@ -2297,7 +2302,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { requireLoadBalancerWasCreated(kubeAPIClient.Actions()[3]) requireTLSSecretWasDeleted(kubeAPIClient.Actions()[4]) // the Secret was deleted because it contained a cert with the wrong IP requireTLSServerIsRunningWithoutCerts() - requireCredentialIssuer(newPendingStrategy()) + requireCredentialIssuer(newPendingStrategyWaitingForLB()) requireSigningCertProviderIsEmpty() // Simulate the informer cache's background update from its watch. @@ -2309,7 +2314,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { r.NoError(runControllerSync()) r.Len(kubeAPIClient.Actions(), 5) // no new actions while it is waiting for the load balancer's ingress requireTLSServerIsRunningWithoutCerts() - requireCredentialIssuer(newPendingStrategy()) + requireCredentialIssuer(newPendingStrategyWaitingForLB()) requireSigningCertProviderIsEmpty() // Update the ingress of the LB in the informer's client and run Sync again. @@ -2554,7 +2559,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { requireLoadBalancerWasCreated(kubeAPIClient.Actions()[1]) requireCASecretWasCreated(kubeAPIClient.Actions()[2]) requireTLSServerIsRunningWithoutCerts() - requireCredentialIssuer(newPendingStrategy()) + requireCredentialIssuer(newPendingStrategyWaitingForLB()) requireSigningCertProviderIsEmpty() // Simulate the informer cache's background update from its watch. @@ -2564,7 +2569,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { r.NoError(runControllerSync()) r.Equal(1, impersonatorFuncWasCalled) // wasn't started a second time requireTLSServerIsRunningWithoutCerts() // still running - requireCredentialIssuer(newPendingStrategy()) + requireCredentialIssuer(newPendingStrategyWaitingForLB()) requireSigningCertProviderIsEmpty() r.Len(kubeAPIClient.Actions(), 3) // no new API calls }) @@ -2577,7 +2582,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { requireLoadBalancerWasCreated(kubeAPIClient.Actions()[1]) ca := requireCASecretWasCreated(kubeAPIClient.Actions()[2]) requireTLSServerIsRunningWithoutCerts() - requireCredentialIssuer(newPendingStrategy()) + requireCredentialIssuer(newPendingStrategyWaitingForLB()) requireSigningCertProviderIsEmpty() // Simulate the informer cache's background update from its watch. @@ -2614,7 +2619,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { requireLoadBalancerWasCreated(kubeAPIClient.Actions()[1]) ca := requireCASecretWasCreated(kubeAPIClient.Actions()[2]) requireTLSServerIsRunningWithoutCerts() - requireCredentialIssuer(newPendingStrategy()) + requireCredentialIssuer(newPendingStrategyWaitingForLB()) requireSigningCertProviderIsEmpty() // Simulate the informer cache's background update from its watch. @@ -2682,7 +2687,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { requireLoadBalancerWasCreated(kubeAPIClient.Actions()[1]) requireCASecretWasCreated(kubeAPIClient.Actions()[2]) credentialIssuer := getCredentialIssuer() - r.Equal([]v1alpha1.CredentialIssuerStrategy{preExistingStrategy, newPendingStrategy()}, credentialIssuer.Status.Strategies) + r.Equal([]v1alpha1.CredentialIssuerStrategy{preExistingStrategy, newPendingStrategyWaitingForLB()}, credentialIssuer.Status.Strategies) }) }) @@ -2735,7 +2740,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { requireNodesListed(kubeAPIClient.Actions()[0]) requireLoadBalancerWasCreated(kubeAPIClient.Actions()[1]) requireCASecretWasCreated(kubeAPIClient.Actions()[2]) - requireCredentialIssuer(newPendingStrategy()) + requireCredentialIssuer(newPendingStrategyWaitingForLB()) requireSigningCertProviderIsEmpty() // Simulate the informer cache's background update from its watch. @@ -2763,7 +2768,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { // Now everything should be working correctly. r.NoError(runControllerSync()) requireTLSServerIsRunningWithoutCerts() - requireCredentialIssuer(newPendingStrategy()) + requireCredentialIssuer(newPendingStrategyWaitingForLB()) requireSigningCertProviderIsEmpty() }) }) @@ -2791,7 +2796,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { requireNodesListed(kubeAPIClient.Actions()[0]) requireLoadBalancerWasCreated(kubeAPIClient.Actions()[1]) requireCASecretWasCreated(kubeAPIClient.Actions()[2]) - requireCredentialIssuer(newPendingStrategy()) + requireCredentialIssuer(newPendingStrategyWaitingForLB()) requireSigningCertProviderIsEmpty() requireTLSServerIsRunningWithoutCerts() @@ -2823,7 +2828,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { // Now everything should be working correctly. r.NoError(runControllerSync()) requireTLSServerIsRunningWithoutCerts() - requireCredentialIssuer(newPendingStrategy()) + requireCredentialIssuer(newPendingStrategyWaitingForLB()) requireSigningCertProviderIsEmpty() }) }) @@ -2947,7 +2952,10 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { it.Before(func() { addNodeWithRoleToTracker("worker", kubeAPIClient) kubeAPIClient.PrependReactor("create", "services", func(action coretesting.Action) (handled bool, ret runtime.Object, err error) { - return true, nil, fmt.Errorf("error on create") + return true, nil, k8serrors.NewAlreadyExists( + action.GetResource().GroupResource(), + action.(coretesting.CreateAction).GetObject().(*corev1.Service).Name, + ) }) addCredentialIssuerToTrackers(v1alpha1.CredentialIssuer{ ObjectMeta: metav1.ObjectMeta{Name: credentialIssuerResourceName}, @@ -2961,8 +2969,8 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { it("returns an error", func() { startInformersAndController() - r.EqualError(runControllerSync(), "error on create") - requireCredentialIssuer(newErrorStrategy("error on create")) + r.EqualError(runControllerSync(), `services "some-service-resource-name" already exists`) + requireCredentialIssuer(newPendingStrategy(`services "some-service-resource-name" already exists`)) requireSigningCertProviderIsEmpty() requireTLSServerIsRunningWithoutCerts() }) diff --git a/test/integration/concierge_impersonation_proxy_test.go b/test/integration/concierge_impersonation_proxy_test.go index 192db0a4..8fff71a7 100644 --- a/test/integration/concierge_impersonation_proxy_test.go +++ b/test/integration/concierge_impersonation_proxy_test.go @@ -1413,6 +1413,10 @@ func performImpersonatorDiscovery(ctx context.Context, t *testing.T, env *librar } else if strategy.Type == conciergev1alpha.ImpersonationProxyStrategyType { t.Logf("Waiting for successful impersonation proxy strategy on %s: found status %s with reason %s and message: %s", credentialIssuerName(env), strategy.Status, strategy.Reason, strategy.Message) + if strategy.Reason == conciergev1alpha.ErrorDuringSetupStrategyReason { + // The server encountered an unexpected error while starting the impersonator, so fail the test fast. + return false, fmt.Errorf("found impersonation strategy in %s state with message: %s", strategy.Reason, strategy.Message) + } } } t.Log("Did not find any impersonation proxy strategy on CredentialIssuer")