Backport of fix(control-plane): acl tokens deleted while pods in graceful shutdown into release/1.1.x

.changelog/3736.txt

@@ -0,0 +1,4 @@
+```release-note:bug
+control-plane: fix an issue where ACL token cleanup did not respect a pod's GracefulShutdownPeriodSeconds and
+tokens were invalidated immediately on pod entering Terminating state.
+```

acceptance/framework/consul/helm_cluster.go

@@ -624,6 +624,10 @@ func defaultValues() map[string]string {
 		// (false positive).
 		"dns.enabled": "false",
 
+		// Adjust the default value from 30s to 1s since we have several tests that verify tokens are cleaned up,
+		// and many of them are using the default retryer (7s max).
+		"connectInject.sidecarProxy.lifecycle.defaultShutdownGracePeriodSeconds": "1",
+
 		// Enable trace logs for servers and clients.
 		"server.extraConfig": `"{\"log_level\": \"TRACE\"}"`,
 		"client.extraConfig": `"{\"log_level\": \"TRACE\"}"`,

acceptance/framework/consul/helm_cluster_test.go

@@ -30,7 +30,8 @@ func TestNewHelmCluster(t *testing.T) {
 				"global.image":    "test-config-image",
 				"global.logLevel": "debug",
 				"server.replicas": "1",
-				"connectInject.transparentProxy.defaultEnabled": "false",
+				"connectInject.transparentProxy.defaultEnabled":                          "false",
+				"connectInject.sidecarProxy.lifecycle.defaultShutdownGracePeriodSeconds": "1",
 				"dns.enabled":        "false",
 				"server.extraConfig": `"{\"log_level\": \"TRACE\"}"`,
 				"client.extraConfig": `"{\"log_level\": \"TRACE\"}"`,
@@ -43,7 +44,8 @@ func TestNewHelmCluster(t *testing.T) {
 				"global.logLevel":        "debug",
 				"server.bootstrapExpect": "3",
 				"server.replicas":        "3",
-				"connectInject.transparentProxy.defaultEnabled": "true",
+				"connectInject.transparentProxy.defaultEnabled":                          "true",
+				"connectInject.sidecarProxy.lifecycle.defaultShutdownGracePeriodSeconds": "3",
 				"dns.enabled":        "true",
 				"server.extraConfig": `"{\"foo\": \"bar\"}"`,
 				"client.extraConfig": `"{\"foo\": \"bar\"}"`,
@@ -54,7 +56,8 @@ func TestNewHelmCluster(t *testing.T) {
 				"global.logLevel":        "debug",
 				"server.bootstrapExpect": "3",
 				"server.replicas":        "3",
-				"connectInject.transparentProxy.defaultEnabled": "true",
+				"connectInject.transparentProxy.defaultEnabled":                          "true",
+				"connectInject.sidecarProxy.lifecycle.defaultShutdownGracePeriodSeconds": "3",
 				"dns.enabled":        "true",
 				"server.extraConfig": `"{\"foo\": \"bar\"}"`,
 				"client.extraConfig": `"{\"foo\": \"bar\"}"`,

acceptance/tests/connect/connect_proxy_lifecycle_test.go

@@ -12,13 +12,14 @@ import (
 	"time"
 
 	"github.com/gruntwork-io/terratest/modules/k8s"
+	"github.com/hashicorp/consul/sdk/testutil/retry"
+	"github.com/stretchr/testify/require"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
 	"github.com/hashicorp/consul-k8s/acceptance/framework/connhelper"
 	"github.com/hashicorp/consul-k8s/acceptance/framework/consul"
 	"github.com/hashicorp/consul-k8s/acceptance/framework/helpers"
 	"github.com/hashicorp/consul-k8s/acceptance/framework/logger"
-	"github.com/hashicorp/consul/sdk/testutil/retry"
-	"github.com/stretchr/testify/require"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
 type LifecycleShutdownConfig struct {
@@ -33,7 +34,6 @@ const (
 
 // Test the endpoints controller cleans up force-killed pods.
 func TestConnectInject_ProxyLifecycleShutdown(t *testing.T) {
-	t.Skipf("skiping this test, will be re-added in a future commit")
 	cfg := suite.Config()
 	cfg.SkipWhenOpenshiftAndCNI(t)
 
@@ -139,7 +139,8 @@ func TestConnectInject_ProxyLifecycleShutdown(t *testing.T) {
 			require.Len(t, pods.Items, 1)
 			clientPodName := pods.Items[0].Name
 
-			var terminationGracePeriod int64 = 60
+			// We should terminate the pods shortly after envoy gracefully shuts down in our 15s test cases.
+			var terminationGracePeriod int64 = 16
 			logger.Logf(t, "killing the %q pod with %dseconds termination grace period", clientPodName, terminationGracePeriod)
 			err = ctx.KubernetesClient(t).CoreV1().Pods(ns).Delete(context.Background(), clientPodName, metav1.DeleteOptions{GracePeriodSeconds: &terminationGracePeriod})
 			require.NoError(t, err)
@@ -154,23 +155,29 @@ func TestConnectInject_ProxyLifecycleShutdown(t *testing.T) {
 			}
 
 			if gracePeriodSeconds > 0 {
-				// Ensure outbound requests are still successful during grace
-				// period.
-				retry.RunWith(&retry.Timer{Timeout: time.Duration(gracePeriodSeconds) * time.Second, Wait: 2 * time.Second}, t, func(r *retry.R) {
-					output, err := k8s.RunKubectlAndGetOutputE(t, ctx.KubectlOptions(t), args...)
-					require.NoError(r, err)
-					require.Condition(r, func() bool {
-						exists := false
-						if strings.Contains(output, "curl: (7) Failed to connect") {
-							exists = true
+				// Ensure outbound requests are still successful during grace period.
+				gracePeriodTimer := time.NewTimer(time.Duration(gracePeriodSeconds) * time.Second)
+			gracePeriodLoop:
+				for {
+					select {
+					case <-gracePeriodTimer.C:
+						break gracePeriodLoop
+					default:
+						output, err := k8s.RunKubectlAndGetOutputE(t, ctx.KubectlOptions(t), args...)
+						require.NoError(t, err)
+						require.True(t, !strings.Contains(output, "curl: (7) Failed to connect"))
+
+						// If listener draining is enabled, ensure inbound
+						// requests are rejected during grace period.
+						if !drainListenersEnabled {
+							connHelper.TestConnectionSuccess(t)
 						}
-						return !exists
-					})
-				})
+						// TODO: check that the connection is unsuccessful when drainListenersEnabled is true
+						// dans note: I found it isn't sufficient to use the existing TestConnectionFailureWithoutIntention
 
-				// If listener draining is enabled, ensure inbound
-				// requests are rejected during grace period.
-				// connHelper.TestConnectionSuccess(t)
+						time.Sleep(2 * time.Second)
+					}
+				}
 			} else {
 				// Ensure outbound requests fail because proxy has terminated
 				retry.RunWith(&retry.Timer{Timeout: time.Duration(terminationGracePeriod) * time.Second, Wait: 2 * time.Second}, t, func(r *retry.R) {
@@ -187,7 +194,10 @@ func TestConnectInject_ProxyLifecycleShutdown(t *testing.T) {
 			}
 
 			logger.Log(t, "ensuring pod is deregistered after termination")
-			retry.Run(t, func(r *retry.R) {
+			// We wait an arbitrarily long time here. With the deployment rollout creating additional endpoints reconciles,
+			// This can cause the re-queued reconcile used to come back and clean up the service registration to be re-re-queued at
+			// 2-3X the intended grace period.
+			retry.RunWith(&retry.Timer{Timeout: time.Duration(60) * time.Second, Wait: 2 * time.Second}, t, func(r *retry.R) {
 				for _, name := range []string{
 					"static-client",
 					"static-client-sidecar-proxy",