fix: use correct flag when translating namespaces (#2353)

* fix: use correct flag when translating namespaces * Use non-normalized namespace when deregistering services * Guard against namespace queries when namespaces not enabled in cache
hashicorp · Jun 13, 2023 · 345f62c · 345f62c
1 parent fc40d5e
commit 345f62c
Show file tree

Hide file tree

Showing 4 changed files with 81 additions and 8 deletions.
diff --git a/control-plane/api-gateway/cache/gateway.go b/control-plane/api-gateway/cache/gateway.go
@@ -18,7 +18,7 @@ import (
 )
 
 type GatewayCache struct {
-	config    *consul.Config
+	config    Config
 	serverMgr consul.ServerConnectionManager
 	logger    logr.Logger
 
@@ -35,7 +35,7 @@ type GatewayCache struct {
 
 func NewGatewayCache(ctx context.Context, config Config) *GatewayCache {
 	return &GatewayCache{
-		config:             config.ConsulClientConfig,
+		config:             config,
 		serverMgr:          config.ConsulServerConnMgr,
 		logger:             config.Logger,
 		events:             make(chan event.GenericEvent),
@@ -53,13 +53,13 @@ func (r *GatewayCache) ServicesFor(ref api.ResourceReference) []api.CatalogServi
 }
 
 func (r *GatewayCache) FetchServicesFor(ctx context.Context, ref api.ResourceReference) ([]api.CatalogService, error) {
-	client, err := consul.NewClientFromConnMgr(r.config, r.serverMgr)
+	client, err := consul.NewClientFromConnMgr(r.config.ConsulClientConfig, r.serverMgr)
 	if err != nil {
 		return nil, err
 	}
 
 	opts := &api.QueryOptions{}
-	if ref.Namespace != "" {
+	if r.config.NamespacesEnabled && ref.Namespace != "" {
 		opts.Namespace = ref.Namespace
 	}
 
@@ -95,7 +95,7 @@ func (r *GatewayCache) RemoveSubscription(ref api.ResourceReference) {
 
 func (r *GatewayCache) subscribeToGateway(ctx context.Context, ref api.ResourceReference, resource types.NamespacedName) {
 	opts := &api.QueryOptions{}
-	if ref.Namespace != "" {
+	if r.config.NamespacesEnabled && ref.Namespace != "" {
 		opts.Namespace = ref.Namespace
 	}
 
@@ -117,7 +117,7 @@ func (r *GatewayCache) subscribeToGateway(ctx context.Context, ref api.ResourceR
 		retryBackoff := backoff.WithMaxRetries(backoff.NewExponentialBackOff(), 10)
 
 		if err := backoff.Retry(func() error {
-			client, err := consul.NewClientFromConnMgr(r.config, r.serverMgr)
+			client, err := consul.NewClientFromConnMgr(r.config.ConsulClientConfig, r.serverMgr)
 			if err != nil {
 				return err
 			}

diff --git a/control-plane/api-gateway/common/translation.go b/control-plane/api-gateway/common/translation.go
@@ -50,7 +50,7 @@ func (t ResourceTranslator) NormalizedResourceReference(kind, namespace string,
 }
 
 func (t ResourceTranslator) Namespace(namespace string) string {
-	return namespaces.ConsulNamespace(namespace, t.EnableK8sMirroring, t.ConsulDestNamespace, t.EnableK8sMirroring, t.MirroringPrefix)
+	return namespaces.ConsulNamespace(namespace, t.EnableConsulNamespaces, t.ConsulDestNamespace, t.EnableK8sMirroring, t.MirroringPrefix)
 }
 
 // ToAPIGateway translates a kuberenetes API gateway into a Consul APIGateway Config Entry.

diff --git a/control-plane/api-gateway/common/translation_test.go b/control-plane/api-gateway/common/translation_test.go
@@ -9,11 +9,13 @@ import (
 	"crypto/x509"
 	"crypto/x509/pkix"
 	"encoding/pem"
+	"fmt"
 	"math/big"
 	"testing"
 	"time"
 
 	"github.com/google/go-cmp/cmp"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -42,6 +44,77 @@ func (v fakeReferenceValidator) TCPRouteCanReferenceBackend(tcpRoute gwv1alpha2.
 	return true
 }
 
+func TestTranslator_Namespace(t *testing.T) {
+	testCases := []struct {
+		EnableConsulNamespaces bool
+		ConsulDestNamespace    string
+		EnableK8sMirroring     bool
+		MirroringPrefix        string
+		Input, ExpectedOutput  string
+	}{
+		{
+			EnableConsulNamespaces: false,
+			ConsulDestNamespace:    "default",
+			EnableK8sMirroring:     false,
+			MirroringPrefix:        "",
+			Input:                  "namespace-1",
+			ExpectedOutput:         "",
+		},
+		{
+			EnableConsulNamespaces: false,
+			ConsulDestNamespace:    "default",
+			EnableK8sMirroring:     true,
+			MirroringPrefix:        "",
+			Input:                  "namespace-1",
+			ExpectedOutput:         "",
+		},
+		{
+			EnableConsulNamespaces: false,
+			ConsulDestNamespace:    "default",
+			EnableK8sMirroring:     true,
+			MirroringPrefix:        "pre-",
+			Input:                  "namespace-1",
+			ExpectedOutput:         "",
+		},
+		{
+			EnableConsulNamespaces: true,
+			ConsulDestNamespace:    "default",
+			EnableK8sMirroring:     false,
+			MirroringPrefix:        "",
+			Input:                  "namespace-1",
+			ExpectedOutput:         "default",
+		},
+		{
+			EnableConsulNamespaces: true,
+			ConsulDestNamespace:    "default",
+			EnableK8sMirroring:     true,
+			MirroringPrefix:        "",
+			Input:                  "namespace-1",
+			ExpectedOutput:         "namespace-1",
+		},
+		{
+			EnableConsulNamespaces: true,
+			ConsulDestNamespace:    "default",
+			EnableK8sMirroring:     true,
+			MirroringPrefix:        "pre-",
+			Input:                  "namespace-1",
+			ExpectedOutput:         "pre-namespace-1",
+		},
+	}
+
+	for i, tc := range testCases {
+		t.Run(fmt.Sprintf("%s_%d", t.Name(), i), func(t *testing.T) {
+			translator := ResourceTranslator{
+				EnableConsulNamespaces: tc.EnableConsulNamespaces,
+				ConsulDestNamespace:    tc.ConsulDestNamespace,
+				EnableK8sMirroring:     tc.EnableK8sMirroring,
+				MirroringPrefix:        tc.MirroringPrefix,
+			}
+			assert.Equal(t, tc.ExpectedOutput, translator.Namespace(tc.Input))
+		})
+	}
+}
+
 func TestTranslator_ToAPIGateway(t *testing.T) {
 	t.Parallel()
 	k8sObjectName := "my-k8s-gw"

diff --git a/control-plane/api-gateway/controllers/gateway_controller.go b/control-plane/api-gateway/controllers/gateway_controller.go
@@ -212,7 +212,7 @@ func (r *GatewayController) Reconcile(ctx context.Context, req ctrl.Request) (ct
 		r.gatewayCache.RemoveSubscription(nonNormalizedConsulKey)
 		// make sure we have deregister all services even if they haven't
 		// hit cache yet
-		if err := r.deregisterAllServices(ctx, consulKey); err != nil {
+		if err := r.deregisterAllServices(ctx, nonNormalizedConsulKey); err != nil {
 			log.Error(err, "error deregistering services")
 			return ctrl.Result{}, err
 		}