From 39449f1e9123d2b1afb5b1d2c4b3f507103f1ec2 Mon Sep 17 00:00:00 2001 From: DanStough Date: Fri, 3 Nov 2023 17:14:31 -0400 Subject: [PATCH 1/2] fix(controller): v2 pod controller errors for acl deletion --- .changelog/3172.txt | 7 +++++++ .../connect-inject/controllers/pod/pod_controller.go | 9 ++++++++- .../controllers/pod/pod_controller_test.go | 8 ++++++++ .../connect-inject/webhookv2/consul_dataplane_sidecar.go | 6 ++++++ .../webhookv2/consul_dataplane_sidecar_test.go | 2 +- control-plane/subcommand/connect-init/command.go | 1 + control-plane/subcommand/mesh-init/command.go | 1 + 7 files changed, 32 insertions(+), 2 deletions(-) create mode 100644 .changelog/3172.txt diff --git a/.changelog/3172.txt b/.changelog/3172.txt new file mode 100644 index 0000000000..9e255f4278 --- /dev/null +++ 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. +``` \ No newline at end of file diff --git a/control-plane/connect-inject/controllers/pod/pod_controller.go b/control-plane/connect-inject/controllers/pod/pod_controller.go index 85b17bfbbd..c4b867f8c3 100644 --- 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 index 30372e00b5..413f9ac49a 100644 --- 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 index ed0983a1c8..f8a8406d23 100644 --- 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 index fecef1bb5d..81b3520511 100644 --- 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 index 6bb66d0948..2d245797dc 100644 --- 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 index 6c21210e9b..2a6b6ccebd 100644 --- 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) From 8f23c90180241fa25bf0b77e8d1afb4b3abef565 Mon Sep 17 00:00:00 2001 From: DanStough Date: Tue, 7 Nov 2023 11:17:37 -0500 Subject: [PATCH 2/2] test: fix tests for unsupported L7 TPs --- .../meshconfig_controller_test.go | 73 ++++++++++--------- 1 file changed, 38 insertions(+), 35 deletions(-) diff --git a/control-plane/config-entries/controllersv2/meshconfig_controller_test.go b/control-plane/config-entries/controllersv2/meshconfig_controller_test.go index e95e722ea6..c7da7b3de3 100644 --- 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"}, + // }, + //}, }, }, },