Backport of fix(controller): v2 pod controller errors for acl deletio…

…n into release/1.3.x (#3177) * backport of commit 39449f1 * backport of commit 8f23c90 --------- Co-authored-by: DanStough <[email protected]>
hashicorp · Nov 7, 2023 · 2b71660 · 2b71660
1 parent 6fcc8b1
commit 2b71660
Show file tree

Hide file tree

Showing 8 changed files with 70 additions and 37 deletions.
diff --git a/.changelog/3172.txt b/.changelog/3172.txt
@@ -0,0 +1,7 @@
+```release-note:bug
+control-plane: remove extraneous error log in v2 pod controller when attempting to delete ACL tokens.
+```
+```
+release-note:bug
+init container: fix a bug that didn't clear ACL tokens for init container when tproxy is enabled.
+```
diff --git a/control-plane/config-entries/controllersv2/meshconfig_controller_test.go b/control-plane/config-entries/controllersv2/meshconfig_controller_test.go
@@ -70,13 +70,14 @@ func TestMeshConfigController_createsMeshConfig(t *testing.T) {
 									IdentityName: "source-identity",
 								},
 							},
-							DestinationRules: []*pbauth.DestinationRule{
-								{
-									PathExact: "/hello",
-									Methods:   []string{"GET", "POST"},
-									PortNames: []string{"web", "admin"},
-								},
-							},
+							// TODO: enable this when L7 traffic permissions are supported
+							//DestinationRules: []*pbauth.DestinationRule{
+							//	{
+							//		PathExact: "/hello",
+							//		Methods:   []string{"GET", "POST"},
+							//		PortNames: []string{"web", "admin"},
+							//	},
+							//},
 						},
 					},
 				},
@@ -101,13 +102,13 @@ func TestMeshConfigController_createsMeshConfig(t *testing.T) {
 								Peer:      constants.DefaultConsulPeer,
 							},
 						},
-						DestinationRules: []*pbauth.DestinationRule{
-							{
-								PathExact: "/hello",
-								Methods:   []string{"GET", "POST"},
-								PortNames: []string{"web", "admin"},
-							},
-						},
+						//DestinationRules: []*pbauth.DestinationRule{
+						//	{
+						//		PathExact: "/hello",
+						//		Methods:   []string{"GET", "POST"},
+						//		PortNames: []string{"web", "admin"},
+						//	},
+						//},
 					},
 				},
 			},
@@ -216,13 +217,14 @@ func TestMeshConfigController_updatesMeshConfig(t *testing.T) {
 									IdentityName: "source-identity",
 								},
 							},
-							DestinationRules: []*pbauth.DestinationRule{
-								{
-									PathExact: "/hello",
-									Methods:   []string{"GET", "POST"},
-									PortNames: []string{"web", "admin"},
-								},
-							},
+							// TODO: enable this when L7 traffic permissions are supported
+							//DestinationRules: []*pbauth.DestinationRule{
+							//	{
+							//		PathExact: "/hello",
+							//		Methods:   []string{"GET", "POST"},
+							//		PortNames: []string{"web", "admin"},
+							//	},
+							//},
 						},
 					},
 				},
@@ -241,13 +243,13 @@ func TestMeshConfigController_updatesMeshConfig(t *testing.T) {
 								Peer:      constants.DefaultConsulPeer,
 							},
 						},
-						DestinationRules: []*pbauth.DestinationRule{
-							{
-								PathExact: "/hello",
-								Methods:   []string{"GET", "POST"},
-								PortNames: []string{"web", "admin"},
-							},
-						},
+						//DestinationRules: []*pbauth.DestinationRule{
+						//	{
+						//		PathExact: "/hello",
+						//		Methods:   []string{"GET", "POST"},
+						//		PortNames: []string{"web", "admin"},
+						//	},
+						//},
 					},
 				},
 			},
@@ -373,13 +375,14 @@ func TestMeshConfigController_deletesMeshConfig(t *testing.T) {
 									IdentityName: "source-identity",
 								},
 							},
-							DestinationRules: []*pbauth.DestinationRule{
-								{
-									PathExact: "/hello",
-									Methods:   []string{"GET", "POST"},
-									PortNames: []string{"web", "admin"},
-								},
-							},
+							// TODO: enable this when L7 traffic permissions are supported
+							//DestinationRules: []*pbauth.DestinationRule{
+							//	{
+							//		PathExact: "/hello",
+							//		Methods:   []string{"GET", "POST"},
+							//		PortNames: []string{"web", "admin"},
+							//	},
+							//},
 						},
 					},
 				},

diff --git a/control-plane/connect-inject/controllers/pod/pod_controller.go b/control-plane/connect-inject/controllers/pod/pod_controller.go
@@ -6,6 +6,7 @@ package pod
 import (
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"regexp"
 	"strconv"
@@ -267,6 +268,10 @@ func (r *Controller) deleteACLTokensForPod(apiClient *api.Client, pod types.Name
 	// See discussion above about optimizing the token list query.
 	for _, token := range tokens {
 		tokenMeta, err := getTokenMetaFromDescription(token.Description)
+		// It is possible this is from another component, so continue searching
+		if errors.Is(err, NoMetadataErr) {
+			continue
+		}
 		if err != nil {
 			return fmt.Errorf("failed to parse token metadata: %s", err)
 		}
@@ -284,13 +289,15 @@ func (r *Controller) deleteACLTokensForPod(apiClient *api.Client, pod types.Name
 	return nil
 }
 
+var NoMetadataErr = fmt.Errorf("failed to extract token metadata from description")
+
 // getTokenMetaFromDescription parses JSON metadata from token's description.
 func getTokenMetaFromDescription(description string) (map[string]string, error) {
 	re := regexp.MustCompile(`.*({.+})`)
 
 	matches := re.FindStringSubmatch(description)
 	if len(matches) != 2 {
-		return nil, fmt.Errorf("failed to extract token metadata from description: %s", description)
+		return nil, NoMetadataErr
 	}
 	tokenMetaJSON := matches[1]
 

diff --git a/control-plane/connect-inject/controllers/pod/pod_controller_test.go b/control-plane/connect-inject/controllers/pod/pod_controller_test.go
@@ -1777,6 +1777,14 @@ func TestReconcileDeletePod(t *testing.T) {
 				},
 			}, nil)
 			require.NoError(t, err)
+
+			// We create another junk token here just to make sure it doesn't interfere with cleaning up the
+			// previous "real" token that has metadata.
+			_, _, err = testClient.APIClient.ACL().Login(&api.ACLLoginParams{
+				AuthMethod:  test.AuthMethod,
+				BearerToken: test.ServiceAccountJWTToken,
+			}, nil)
+			require.NoError(t, err)
 		}
 
 		namespacedName := types.NamespacedName{

diff --git a/control-plane/connect-inject/webhookv2/consul_dataplane_sidecar.go b/control-plane/connect-inject/webhookv2/consul_dataplane_sidecar.go
@@ -112,6 +112,12 @@ func (w *MeshWebhook) consulDataplaneSidecar(namespace corev1.Namespace, pod cor
 				Name:  "DP_CREDENTIAL_LOGIN_META",
 				Value: "pod=$(POD_NAMESPACE)/$(POD_NAME)",
 			},
+			// This entry exists to support newer versions of consul dataplane, where environment variable entries
+			// utilize this numbered notation to indicate individual KV pairs in a map.
+			{
+				Name:  "DP_CREDENTIAL_LOGIN_META1",
+				Value: "pod=$(POD_NAMESPACE)/$(POD_NAME)",
+			},
 		},
 		VolumeMounts: []corev1.VolumeMount{
 			{

diff --git a/control-plane/connect-inject/webhookv2/consul_dataplane_sidecar_test.go b/control-plane/connect-inject/webhookv2/consul_dataplane_sidecar_test.go
@@ -219,7 +219,7 @@ func TestHandlerConsulDataplaneSidecar(t *testing.T) {
 			}
 			require.Equal(t, expectedProbe, container.ReadinessProbe)
 			require.Nil(t, container.StartupProbe)
-			require.Len(t, container.Env, 6)
+			require.Len(t, container.Env, 7)
 			require.Equal(t, container.Env[0].Name, "TMPDIR")
 			require.Equal(t, container.Env[0].Value, "/consul/mesh-inject")
 			require.Equal(t, container.Env[2].Name, "POD_NAME")

diff --git a/control-plane/subcommand/connect-init/command.go b/control-plane/subcommand/connect-init/command.go
@@ -206,6 +206,7 @@ func (c *Command) Run(args []string) int {
 	}
 
 	if c.flagRedirectTrafficConfig != "" {
+		c.watcher.Stop() // Explicitly stop the watcher so that ACLs are cleaned up before we apply re-direction.
 		err = c.applyTrafficRedirectionRules(proxyService)
 		if err != nil {
 			c.logger.Error("error applying traffic redirection rules", "err", err)

diff --git a/control-plane/subcommand/mesh-init/command.go b/control-plane/subcommand/mesh-init/command.go
@@ -183,6 +183,7 @@ func (c *Command) Run(args []string) int {
 	}
 
 	if c.flagRedirectTrafficConfig != "" {
+		c.watcher.Stop()                                        // Explicitly stop the watcher so that ACLs are cleaned up before we apply re-direction.
 		err := c.applyTrafficRedirectionRules(&bootstrapConfig) // BootstrapConfig is always populated non-nil from the RPC
 		if err != nil {
 			c.logger.Error("error applying traffic redirection rules", "err", err)