[NET-10567] Fix namespace normalization on external registration/ACL …

…Setup for Terminating Gateways (#4224)

* fix bug in external service registration ACL creation where namespace is
left emtpy in acl policy if not specified in the CRD which results in an
invalid acl policy

* Remove check for timestamp

* update tests!

* update to use helper function

* all non default working

* test cases all working

* move wait for it to separate PR

* use replace for consul-k8s control-plane

* update single namespace test

* updated namespaces and destinations test

* remove usage of creating terminating gateway config entry creation and
external service config entry registration from tests

* fix typo

* update comment

* comment out broken test for the time being

* remove unused import and add period to comment

* add changelog

* fix bug in cache creation for registrations, still debugging issue with
termianting gateways and acl roles

* fix issue with terminating gateway acl role by moving role modification from registrations controller to terminating gateway controller

* appease the linter

* add acl status condition to terminating gateways

* linter

* update config entry terminating gateway tests

* Use more robust method of checking if acls are enabled

* update config entries controller unit tests to run with acls and without

* fix config entries namespaces test setup

* fix unused import

* fix config entries main test

* remove block for deregistering service

* fix comment

* fix acceptance test registration

* handle removing policies when no other gateways reference  them

* fix terminating gateway configuration for peering connect test

* remove unnecessary nodeMeta on fixture, remove unused yaml files from
fixtures

* fix wildcard service names

* use more specific matchers to avoid potential substring collisions

* Update .changelog/4224.txt

Co-authored-by: Nathan Coleman <nathan.coleman@hashicorp.com>

* cleaning up from PR review: moving template execution to where it's
needed and updating variable names to be more consistent

* add comment

* fix typo

---------

Co-authored-by: Nathan Coleman <nathan.coleman@hashicorp.com>

.changelog/4224.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+terminating-gateways: Fix bug where namespace field was not correctly set on ACL policies if using the `Registration` CRD with the service's namespace unset.
+```

acceptance/framework/helpers/helpers.go

@@ -12,6 +12,7 @@ import (
 	"os"
 	"os/exec"
 	"os/signal"
+	"slices"
 	"strings"
 	"syscall"
 	"testing"
@@ -22,10 +23,12 @@ import (
 	terratestLogger "github.com/gruntwork-io/terratest/modules/logger"
 	"github.com/gruntwork-io/terratest/modules/random"
 	"github.com/hashicorp/consul-k8s/acceptance/framework/logger"
+	"github.com/hashicorp/consul-k8s/control-plane/api/v1alpha1"
 	"github.com/hashicorp/consul/api"
 	"github.com/hashicorp/consul/sdk/testutil"
 	"github.com/hashicorp/consul/sdk/testutil/retry"
 	"github.com/stretchr/testify/require"
+	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/kubernetes"
 )
@@ -157,22 +160,23 @@ func MergeMaps(a, b map[string]string) {
 }
 
 type K8sOptions struct {
-	Options            *k8s.KubectlOptions
-	NoCleanupOnFailure bool
-	NoCleanup          bool
-	ConfigPath         string
+	Options             *k8s.KubectlOptions
+	NoCleanupOnFailure  bool
+	NoCleanup           bool
+	KustomizeConfigPath string
 }
 
 type ConsulOptions struct {
-	ConsulClient *api.Client
-	Namespace    string
+	ConsulClient                    *api.Client
+	Namespace                       string
+	ExternalServiceNameRegistration string
 }
 
 func RegisterExternalServiceCRD(t *testing.T, k8sOptions K8sOptions, consulOptions ConsulOptions) {
 	t.Helper()
-	t.Logf("Registering external service %s", k8sOptions.ConfigPath)
+	t.Logf("Registering external service %s", k8sOptions.KustomizeConfigPath)
 
-	if consulOptions.Namespace != "" {
+	if consulOptions.Namespace != "" && consulOptions.Namespace != "default" {
 		logger.Logf(t, "creating the %s namespace in Consul", consulOptions.Namespace)
 		_, _, err := consulOptions.ConsulClient.Namespaces().Create(&api.Namespace{
 			Name: consulOptions.Namespace,
@@ -181,52 +185,34 @@ func RegisterExternalServiceCRD(t *testing.T, k8sOptions K8sOptions, consulOptio
 	}
 
 	// Register the external service
-	k8s.KubectlApply(t, k8sOptions.Options, k8sOptions.ConfigPath)
-
+	k8s.KubectlApplyFromKustomize(t, k8sOptions.Options, k8sOptions.KustomizeConfigPath)
 	Cleanup(t, k8sOptions.NoCleanupOnFailure, k8sOptions.NoCleanup, func() {
-		// Note: this delete command won't wait for pods to be fully terminated.
-		// This shouldn't cause any test pollution because the underlying
-		// objects are deployments, and so when other tests create these
-		// they should have different pod names.
-		k8s.KubectlDelete(t, k8sOptions.Options, k8sOptions.ConfigPath)
+		k8s.KubectlDeleteFromKustomize(t, k8sOptions.Options, k8sOptions.KustomizeConfigPath)
 	})
+
+	CheckExternalServiceConditions(t, consulOptions.ExternalServiceNameRegistration, k8sOptions.Options)
 }
 
-// RegisterExternalService registers an external service to a virtual node in Consul for testing purposes.
-// This function takes a testing.T object, a Consul client, service namespace, service name, address, and port as
-// parameters. It registers the service with Consul, and if a namespace is provided, it also creates the namespace
-// in Consul. It uses the provided testing.T object to log registration details and verify the registration process.
-// If the registration fails, the test calling the function will fail.
-// DEPRECATED: Use RegisterExternalServiceCRD instead.
-func RegisterExternalService(t *testing.T, consulClient *api.Client, namespace, name, address string, port int) {
+func CheckExternalServiceConditions(t *testing.T, registrationName string, opts *k8s.KubectlOptions) {
 	t.Helper()
-	t.Log("RegisterExternalService is DEPRECATED, use RegisterExternalServiceCRD instead")
-
-	service := &api.AgentService{
-		ID:      name,
-		Service: name,
-		Port:    port,
-	}
-
-	if namespace != "" {
-		address = fmt.Sprintf("%s.%s", name, namespace)
-		service.Namespace = namespace
 
-		logger.Logf(t, "creating the %s namespace in Consul", namespace)
-		_, _, err := consulClient.Namespaces().Create(&api.Namespace{
-			Name: namespace,
-		}, nil)
-		require.NoError(t, err)
-	}
+	ogLogger := opts.Logger
+	defer func() {
+		opts.Logger = ogLogger
+	}()
 
-	logger.Log(t, fmt.Sprintf("registering the external service %s", name))
-	_, err := consulClient.Catalog().Register(&api.CatalogRegistration{
-		Node:     "external",
-		Address:  address,
-		NodeMeta: map[string]string{"external-node": "true", "external-probe": "true"},
-		Service:  service,
-	}, nil)
-	require.NoError(t, err)
+	opts.Logger = terratestLogger.Discard
+	retry.RunWith(&retry.Counter{Wait: 2 * time.Second, Count: 15}, t, func(r *retry.R) {
+		var err error
+		out, err := k8s.RunKubectlAndGetOutputE(r, opts, "get", "-o=json", "registrations.consul.hashicorp.com", registrationName)
+		require.NoError(r, err)
+		reg := v1alpha1.Registration{}
+		err = json.Unmarshal([]byte(out), &reg)
+		require.NoError(r, err)
+		require.NotEmpty(r, reg.Status.Conditions, "conditions should not be empty, retrying")
+		// ensure all statuses are true which means that the registration is successful
+		require.True(r, !slices.ContainsFunc(reg.Status.Conditions, func(c v1alpha1.Condition) bool { return c.Status == corev1.ConditionFalse }), "registration failed because of %v", reg.Status.Conditions)
+	})
 }
 
 type Command struct {
@@ -382,26 +368,27 @@ func WaitForInput(t *testing.T) {
 	}
 
 	mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
-		defer func() {
-			err := r.Body.Close()
-			if err != nil {
-				t.Logf("error closing request body: %v", err)
-			}
-		}()
-
 		w.WriteHeader(http.StatusOK)
 
 		_, err := w.Write([]byte("input received\n"))
 		if err != nil {
-			t.Logf("writing body: %v", err)
+			t.Logf("error writing body: %v", err)
+			err = nil
 		}
 
-		err = srv.Shutdown(context.Background())
+		err = r.Body.Close()
 		if err != nil {
-			t.Logf("error closing listener: %v", err)
+			t.Logf("error closing request body: %v", err)
+			err = nil
 		}
 
 		t.Log("input received, continuing test")
+		go func() {
+			err = srv.Shutdown(context.Background())
+			if err != nil {
+				t.Logf("error closing listener: %v", err)
+			}
+		}()
 	})
 
 	t.Logf("Waiting for input on http://localhost:%s", listenerPort)

acceptance/framework/k8s/deploy.go

@@ -165,10 +165,10 @@ func CheckStaticServerConnectionSuccessfulWithMessage(t *testing.T, options *k8s
 
 // CheckStaticServerConnectionSuccessful is just like CheckStaticServerConnection
 // but it always expects a successful connection.
-func CheckStaticServerConnectionSuccessful(t *testing.T, options *k8s.KubectlOptions, sourceApp string, curlArgs ...string) {
+func CheckStaticServerConnectionSuccessful(t *testing.T, sourceAppOpts *k8s.KubectlOptions, sourceApp string, curlArgs ...string) {
 	t.Helper()
 	start := time.Now()
-	CheckStaticServerConnection(t, options, sourceApp, true, nil, "", curlArgs...)
+	CheckStaticServerConnection(t, sourceAppOpts, sourceApp, true, nil, "", curlArgs...)
 	logger.Logf(t, "Took %s to check if static server connection was successful", time.Since(start))
 }
 

acceptance/go.mod

@@ -29,6 +29,12 @@ require (
 	sigs.k8s.io/gateway-api v0.7.1
 )
 
+// replace these so we always use the latest version of the control-plane types
+replace (
+	github.com/hashicorp/consul-k8s/control-plane => ../control-plane
+	github.com/hashicorp/consul-k8s/version => ../version
+)
+
 require (
 	github.com/armon/go-metrics v0.4.1 // indirect
 	github.com/asaskevich/govalidator v0.0.0-20210307081110-f21760c49a8d // indirect
@@ -44,6 +50,7 @@ require (
 	github.com/evanphx/json-patch v5.6.0+incompatible // indirect
 	github.com/evanphx/json-patch/v5 v5.6.0 // indirect
 	github.com/fatih/color v1.16.0 // indirect
+	github.com/fsnotify/fsnotify v1.6.0 // indirect
 	github.com/ghodss/yaml v1.0.0 // indirect
 	github.com/go-errors/errors v1.4.2 // indirect
 	github.com/go-jose/go-jose/v3 v3.0.3 // indirect
@@ -61,14 +68,15 @@ require (
 	github.com/go-ozzo/ozzo-validation v3.6.0+incompatible // indirect
 	github.com/go-sql-driver/mysql v1.5.0 // indirect
 	github.com/gogo/protobuf v1.3.2 // indirect
+	github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect
 	github.com/golang/protobuf v1.5.4 // indirect
 	github.com/google/gnostic-models v0.6.8 // indirect
 	github.com/google/go-cmp v0.5.9 // indirect
 	github.com/google/gofuzz v1.2.0 // indirect
 	github.com/google/pprof v0.0.0-20230602150820-91b7bce49751 // indirect
 	github.com/grpc-ecosystem/grpc-gateway/v2 v2.16.0 // indirect
 	github.com/gruntwork-io/go-commons v0.8.0 // indirect
-	github.com/hashicorp/consul/proto-public v0.6.1 // indirect
+	github.com/hashicorp/consul-k8s/version v0.0.0 // indirect
 	github.com/hashicorp/errwrap v1.1.0 // indirect
 	github.com/hashicorp/go-bexpr v0.1.11 // indirect
 	github.com/hashicorp/go-cleanhttp v0.5.2 // indirect
@@ -137,6 +145,8 @@ require (
 	google.golang.org/grpc v1.58.3 // indirect
 	gopkg.in/inf.v0 v0.9.1 // indirect
 	gopkg.in/yaml.v3 v3.0.1 // indirect
+	k8s.io/apiextensions-apiserver v0.28.3 // indirect
+	k8s.io/component-base v0.28.3 // indirect
 	k8s.io/klog/v2 v2.100.1 // indirect
 	k8s.io/kube-openapi v0.0.0-20230717233707-2695361300d9 // indirect
 	sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd // indirect

acceptance/go.sum

@@ -183,8 +183,6 @@ github.com/gruntwork-io/go-commons v0.8.0 h1:k/yypwrPqSeYHevLlEDmvmgQzcyTwrlZGRa
 github.com/gruntwork-io/go-commons v0.8.0/go.mod h1:gtp0yTtIBExIZp7vyIV9I0XQkVwiQZze678hvDXof78=
 github.com/gruntwork-io/terratest v0.46.7 h1:oqGPBBO87SEsvBYaA0R5xOq+Lm2Xc5dmFVfxEolfZeU=
 github.com/gruntwork-io/terratest v0.46.7/go.mod h1:6gI5MlLeyF+SLwqocA5GBzcTix+XiuxCy1BPwKuT+WM=
-github.com/hashicorp/consul-k8s/control-plane v0.0.0-20240226161840-f3842c41cb2b h1:AdeWjUb+rxrRryC5ZHaL32oOZuxubOzV2q6oJ97UMT0=
-github.com/hashicorp/consul-k8s/control-plane v0.0.0-20240226161840-f3842c41cb2b/go.mod h1:TVaSJM7vYM/mtKGpVc/Lch53lrqLI9XAXJgy/gY8v4A=
 github.com/hashicorp/consul/api v1.29.1 h1:UEwOjYJrd3lG1x5w7HxDRMGiAUPrb3f103EoeKuuEcc=
 github.com/hashicorp/consul/api v1.29.1/go.mod h1:lumfRkY/coLuqMICkI7Fh3ylMG31mQSRZyef2c5YvJI=
 github.com/hashicorp/consul/proto-public v0.6.1 h1:+uzH3olCrksXYWAYHKqK782CtK9scfqH+Unlw3UHhCg=
@@ -452,6 +450,8 @@ go.opentelemetry.io/otel/trace v1.19.0 h1:DFVQmlVbfVeOuBRrwdtaehRrWiL1JoVs9CPIQ1
 go.opentelemetry.io/otel/trace v1.19.0/go.mod h1:mfaSyvGyEJEI0nyV2I4qhNQnbBOUUmYZpYojqMnX2vo=
 go.opentelemetry.io/proto/otlp v1.0.0 h1:T0TX0tmXU8a3CbNXzEKGeU5mIVOdf0oykP+u2lIVU/I=
 go.opentelemetry.io/proto/otlp v1.0.0/go.mod h1:Sy6pihPLfYHkr3NkUbEhGHFhINUSI/v80hjKIs5JXpM=
+go.uber.org/goleak v1.2.1 h1:NBol2c7O1ZokfZ0LEU9K6Whx/KnwvepVetCUhtKja4A=
+go.uber.org/goleak v1.2.1/go.mod h1:qlT2yGI9QafXHhZZLxlSuNsMw3FFLxBr+tBRlmO1xH4=
 go.uber.org/multierr v1.11.0 h1:blXXJkSxSSfBVBlC76pxqeO+LN3aDfLQo+309xJstO0=
 go.uber.org/multierr v1.11.0/go.mod h1:20+QtiLqy0Nd6FdQB9TLXag12DsQkrbs3htMFfDN80Y=
 go.uber.org/zap v1.25.0 h1:4Hvk6GtkucQ790dqmj7l1eEnRdKm3k3ZUrUMS2d5+5c=
@@ -543,6 +543,7 @@ golang.org/x/sys v0.0.0-20220520151302-bc2c85ada10a/go.mod h1:oPkhp1MJrh7nUepCBc
 golang.org/x/sys v0.0.0-20220722155257-8c9f86f7a55f/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.0.0-20220728004956-3c1f35247d10/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.0.0-20220811171246-fbc7d0a398ab/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
+golang.org/x/sys v0.0.0-20220908164124-27713097b956/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.1.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.5.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.6.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
@@ -635,6 +636,8 @@ k8s.io/apimachinery v0.28.9 h1:aXz4Zxsw+Pk4KhBerAtKRxNN1uSMWKfciL/iOdBfXvA=
 k8s.io/apimachinery v0.28.9/go.mod h1:zUG757HaKs6Dc3iGtKjzIpBfqTM4yiRsEe3/E7NX15o=
 k8s.io/client-go v0.28.9 h1:mmMvejwc/KDjMLmDpyaxkWNzlWRCJ6ht7Qsbsnwn39Y=
 k8s.io/client-go v0.28.9/go.mod h1:GFDy3rUNId++WGrr0hRaBrs+y1eZz5JtVZODEalhRMo=
+k8s.io/component-base v0.28.3 h1:rDy68eHKxq/80RiMb2Ld/tbH8uAE75JdCqJyi6lXMzI=
+k8s.io/component-base v0.28.3/go.mod h1:fDJ6vpVNSk6cRo5wmDa6eKIG7UlIQkaFmZN2fYgIUD8=
 k8s.io/klog/v2 v2.100.1 h1:7WCHKK6K8fNhTqfBhISHQ97KrnJNFZMcQvKp7gP/tmg=
 k8s.io/klog/v2 v2.100.1/go.mod h1:y1WjHnz7Dj687irZUWR/WLkLc5N1YHtjLdmgWjndZn0=
 k8s.io/kube-openapi v0.0.0-20230717233707-2695361300d9 h1:LyMgNKD2P8Wn1iAwQU5OhxCKlKJy0sHc+PcDwFB24dQ=

acceptance/tests/config-entries/config_entries_namespaces_test.go

@@ -90,6 +90,10 @@ func TestControllerNamespaces(t *testing.T) {
 
 				"global.acls.manageSystemACLs": strconv.FormatBool(c.secure),
 				"global.tls.enabled":           strconv.FormatBool(c.secure),
+
+				"terminatingGateways.enabled":              "true",
+				"terminatingGateways.gateways[0].name":     "terminating-gateway",
+				"terminatingGateways.gateways[0].replicas": "1",
 			}
 
 			releaseName := helpers.RandomName()

acceptance/tests/config-entries/config_entries_test.go

@@ -59,6 +59,10 @@ func TestController(t *testing.T) {
 				"connectInject.enabled":        "true",
 				"global.tls.enabled":           strconv.FormatBool(c.secure),
 				"global.acls.manageSystemACLs": strconv.FormatBool(c.secure),
+
+				"terminatingGateways.enabled":              "true",
+				"terminatingGateways.gateways[0].name":     "terminating-gateway",
+				"terminatingGateways.gateways[0].replicas": "1",
 			}
 
 			releaseName := helpers.RandomName()
@@ -238,7 +242,6 @@ func TestController(t *testing.T) {
 					require.Equal(r, 100.0, rateLimitIPConfigEntry.Session.WriteRate)
 					require.Equal(r, 100.0, rateLimitIPConfigEntry.Txn.ReadRate)
 					require.Equal(r, 100.0, rateLimitIPConfigEntry.Txn.WriteRate)
-
 				})
 			}
 

acceptance/tests/fixtures/cases/terminating-gateway/external-service.yaml → acceptance/tests/fixtures/bases/external-service-registration/external-service.yaml

@@ -15,4 +15,4 @@ spec:
   service:
     id: static-server
     name: static-server
-    port: 80 
+    port: 80

acceptance/tests/fixtures/bases/external-service-registration/kustomization.yaml

@@ -0,0 +1,5 @@
+# Copyright (c) HashiCorp, Inc.
+# SPDX-License-Identifier: MPL-2.0
+
+resources:
+  - external-service.yaml

acceptance/tests/fixtures/bases/terminating-gateway/kustomization.yaml

@@ -0,0 +1,5 @@
+# Copyright (c) HashiCorp, Inc.
+# SPDX-License-Identifier: MPL-2.0
+
+resources:
+  - terminating-gateway.yaml

acceptance/tests/fixtures/cases/crd-peers/default-terminating-gateway/kustomization.yaml

@@ -0,0 +1,9 @@
+# Copyright (c) HashiCorp, Inc.
+# SPDX-License-Identifier: MPL-2.0
+
+apiVersion: kustomize.config.k8s.io/v1beta1
+kind: Kustomization
+resources:
+- ../../../../fixtures/bases/terminating-gateway
+patches:
+- path: terminating-gateway.yaml

acceptance/tests/fixtures/cases/crd-peers/default-terminating-gateway/terminating-gateway.yaml

@@ -0,0 +1,10 @@
+# Copyright (c) HashiCorp, Inc.
+# SPDX-License-Identifier: MPL-2.0
+
+apiVersion: consul.hashicorp.com/v1alpha1
+kind: TerminatingGateway
+metadata:
+  name: terminating-gateway 
+spec:
+  services:
+    - name: static-server-hostname

acceptance/tests/fixtures/cases/crd-peers/external-service-registration/external-service.yaml

@@ -0,0 +1,18 @@
+# Copyright (c) HashiCorp, Inc.
+# SPDX-License-Identifier: MPL-2.0
+
+apiVersion: consul.hashicorp.com/v1alpha1
+kind: Registration
+metadata:
+  name: static-server-registration
+spec:
+  datacenter: server
+  node: external
+  nodeMeta:
+    external-node: "true"
+    external-probe: "true"
+  address: static-server.external
+  service:
+    id: static-server
+    name: static-server-hostname
+    port: 80

acceptance/tests/fixtures/cases/crd-peers/external-service-registration/kustomization.yaml

@@ -0,0 +1,9 @@
+# Copyright (c) HashiCorp, Inc.
+# SPDX-License-Identifier: MPL-2.0
+
+apiVersion: kustomize.config.k8s.io/v1beta1
+kind: Kustomization
+resources:
+- ../../../bases/external-service-registration
+patches:
+- path: external-service.yaml

acceptance/tests/fixtures/cases/terminating-gateway-destinations/kustomization.yaml

@@ -0,0 +1,9 @@
+# Copyright (c) HashiCorp, Inc.
+# SPDX-License-Identifier: MPL-2.0
+
+apiVersion: kustomize.config.k8s.io/v1beta1
+kind: Kustomization
+resources:
+- ../../../fixtures/bases/terminating-gateway
+patches:
+- path: terminating-gateway.yaml

acceptance/tests/fixtures/cases/terminating-gateway-namespaces/external-service.yaml → acceptance/tests/fixtures/cases/terminating-gateway-namespaces/all-non-default/external-service-registration/external-service.yaml

@@ -5,7 +5,6 @@ apiVersion: consul.hashicorp.com/v1alpha1
 kind: Registration
 metadata:
   name: static-server-registration
-  namespace: ns1
 spec:
   datacenter: dc1
   node: external
@@ -17,4 +16,4 @@ spec:
     id: static-server
     name: static-server
     namespace: ns1
-    port: 80 
+    port: 80

acceptance/tests/fixtures/cases/terminating-gateway-namespaces/all-non-default/external-service-registration/kustomization.yaml

@@ -0,0 +1,9 @@
+# Copyright (c) HashiCorp, Inc.
+# SPDX-License-Identifier: MPL-2.0
+
+apiVersion: kustomize.config.k8s.io/v1beta1
+kind: Kustomization
+resources:
+- ../../../../bases/external-service-registration/
+patches:
+- path: external-service.yaml