Skip to content

Commit

Permalink
fix(controller): v2 pod controller errors for acl deletion (#3172)
Browse files Browse the repository at this point in the history 
* fix(controller): v2 pod controller errors for acl deletion

* test: fix tests for unsupported L7 TPs
  • Loading branch information
@DanStough
DanStough authored Nov 7, 2023
1 parent e50c8e2 commit cd79533
Show file tree
Hide file tree
Showing 8 changed files with 70 additions and 37 deletions.
7 changes: 7 additions & 0 deletions .changelog/3172.txt
View file
Original file line number Diff line number Diff line change
@@ -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.
```
73 changes: 38 additions & 35 deletions control-plane/config-entries/controllersv2/meshconfig_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"},
// },
//},
},
},
},
Expand All @@ -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"},
// },
//},
},
},
},
Expand Down Expand Up @@ -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"},
// },
//},
},
},
},
Expand All @@ -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"},
// },
//},
},
},
},
Expand Down Expand Up @@ -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"},
// },
//},
},
},
},
Expand Down
9 changes: 8 additions & 1 deletion control-plane/connect-inject/controllers/pod/pod_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package pod
import (
"context"
"encoding/json"
"errors"
"fmt"
"regexp"
"strconv"
Expand Down Expand Up @@ -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)
}
Expand All @@ -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]

Expand Down
8 changes: 8 additions & 0 deletions control-plane/connect-inject/controllers/pod/pod_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down
6 changes: 6 additions & 0 deletions control-plane/connect-inject/webhookv2/consul_dataplane_sidecar.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{
{
Expand Down
2 changes: 1 addition & 1 deletion control-plane/connect-inject/webhookv2/consul_dataplane_sidecar_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down
1 change: 1 addition & 0 deletions control-plane/subcommand/connect-init/command.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
1 change: 1 addition & 0 deletions control-plane/subcommand/mesh-init/command.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down

0 comments on commit cd79533

Please sign in to comment.