From af4cd1b51554322e20e482665ee5062ef0183f8e Mon Sep 17 00:00:00 2001 From: Matt Moyer Date: Wed, 2 Jun 2021 13:47:54 -0500 Subject: [PATCH 1/2] Tolerate NotFound when deleting services in `impersonatorconfig`. When a CredentialIssuer is switched from one service type to another (or switched to disabled mode), the `impersonatorconfig` controller will delete the previous Service, if any. Normally one Concierge pod will succeed to delete this initially and any other pods will see a NotFound error. Before this change, the NotFound would bubble up and cause the strategy to enter a ErrorDuringSetup status until the next reconcile loop. We now handle this case without reporting an error. Signed-off-by: Matt Moyer --- .../controller/impersonatorconfig/impersonator_config.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/internal/controller/impersonatorconfig/impersonator_config.go b/internal/controller/impersonatorconfig/impersonator_config.go index 29e7dfa7..bb857c67 100644 --- a/internal/controller/impersonatorconfig/impersonator_config.go +++ b/internal/controller/impersonatorconfig/impersonator_config.go @@ -477,7 +477,8 @@ func (c *impersonatorConfigController) ensureLoadBalancerIsStopped(ctx context.C c.infoLog.Info("deleting load balancer for impersonation proxy", "service", klog.KRef(c.namespace, c.generatedLoadBalancerServiceName), ) - return c.k8sClient.CoreV1().Services(c.namespace).Delete(ctx, c.generatedLoadBalancerServiceName, metav1.DeleteOptions{}) + err = c.k8sClient.CoreV1().Services(c.namespace).Delete(ctx, c.generatedLoadBalancerServiceName, metav1.DeleteOptions{}) + return utilerrors.FilterOut(err, k8serrors.IsNotFound) } func (c *impersonatorConfigController) ensureClusterIPServiceIsStarted(ctx context.Context, config *v1alpha1.ImpersonationProxySpec) error { @@ -516,7 +517,8 @@ func (c *impersonatorConfigController) ensureClusterIPServiceIsStopped(ctx conte c.infoLog.Info("deleting cluster ip for impersonation proxy", "service", klog.KRef(c.namespace, c.generatedClusterIPServiceName), ) - return c.k8sClient.CoreV1().Services(c.namespace).Delete(ctx, c.generatedClusterIPServiceName, metav1.DeleteOptions{}) + err = c.k8sClient.CoreV1().Services(c.namespace).Delete(ctx, c.generatedClusterIPServiceName, metav1.DeleteOptions{}) + return utilerrors.FilterOut(err, k8serrors.IsNotFound) } func (c *impersonatorConfigController) createOrUpdateService(ctx context.Context, service *v1.Service) error { From 6903196c181b05eb37f70d12baa5a5e85c7beb4d Mon Sep 17 00:00:00 2001 From: Matt Moyer Date: Wed, 2 Jun 2021 14:00:35 -0500 Subject: [PATCH 2/2] Fix a data race in TestImpersonationProxy. The `require.Eventually()` function runs the body of the check in a separate goroutine, so it's not safe to use other `require` assertions as we did here. Our `library.RequireEventuallyWithoutError()` function does not spawn a goroutine, so it's safer to use here. Signed-off-by: Matt Moyer --- test/integration/concierge_impersonation_proxy_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/integration/concierge_impersonation_proxy_test.go b/test/integration/concierge_impersonation_proxy_test.go index 688ccb80..98b44dcf 100644 --- a/test/integration/concierge_impersonation_proxy_test.go +++ b/test/integration/concierge_impersonation_proxy_test.go @@ -1267,9 +1267,9 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl }) // wait until the credential issuer is updated with the new url - require.Eventually(t, func() bool { + library.RequireEventuallyWithoutError(t, func() (bool, error) { newImpersonationProxyURL, _ := performImpersonatorDiscovery(ctx, t, env, adminConciergeClient) - return newImpersonationProxyURL == "https://"+clusterIPServiceURL + return newImpersonationProxyURL == "https://"+clusterIPServiceURL, nil }, 30*time.Second, 500*time.Millisecond) newImpersonationProxyURL, newImpersonationProxyCACertPEM := performImpersonatorDiscovery(ctx, t, env, adminConciergeClient)