[NET-10567] Fix namespace normalization on external registration/ACL Setup for Terminating Gateways #4224

jm96441n · 2024-07-31T20:37:03Z

Changes proposed in this PR

Fix namespace normalization on external registration (when namespaces are enabled the empty namespace is converted to "default" for acl rules)
Fix how acls are setup for terminating gateways -> moved from the registrations controller/cache to the terminating gateway controller to avoid race conditions with persisting the terminating gateway config entry (TODO: clean up extra policies when a terminating gateway is removed/services are removed and are not referenced by any other terminating gateways)
Fixed bug in cache where values weren't being persisted with the correct key
updated terminating gateway tests so they would catch this in the future

How I've tested this PR

wrote tests
ran in kind cluster

How I expect reviewers to test this PR

clone this branch and build the container using make docker-dev
clone down the following gist: https://gist.github.com/jm96441n/1be432dd2b6b5ade7bc1b058539f6095
modify the CONSUL_K8S_CHART_LOCATION variable in the start.sh file to point to the helm charts in your local version of consul-
run the start.sh file (this requires you to have kind and yq on your machine, and you'll need to run chmod +x ./start.sh)
after that completes run the following to set up your consul CLI to talk to consul:

export CONSUL_HTTP_ADDR="127.0.0.1:8501"
export CONSUL_HTTP_TOKEN=$(kubectl get --namespace consul secrets/consul-consul-bootstrap-acl-token --template={{.data.token}} | base64 -d)
export CONSUL_HTTP_SSL=true
export CONSUL_HTTP_SSL_VERIFY=false

enable a port forward to the consul server on port 8501
run consul acl role list and you will see both terminating gateways with the term gateway policy and the zoidberg and nibbler policies
run consul acl policy read -name zoidberg-write-policy to see the policy include the namespace
shell into the bender service and run curl localhost:1234 to see the request to zoidberg go through, run curl localhost:5678 to see the request to nibbler go through
now we'll test that acl policies are removed correctly
edit the termgw.yaml file to no longer reference the zoidberg service and apply the file
run consul acl role list and see the first terminating gateway no longer references the zoidberg policy
run consul acl policy list and you'll see the zoidberg policy still exists
now edit the termgw2.yaml file to no longer reference the zoidberg service and apply that file
now run consul acl role list and see that none of the terminating gateways reference that policy
now run consul acl policy list and see that the zoidberg policy no longer exists because no gateway is referencing it

Checklist

Tests added
CHANGELOG entry added

jm96441n · 2024-07-31T20:37:42Z

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) {


modified this argument to make it more clear what options it should be for (in multi-namespace scenarios)

jm96441n · 2024-07-31T20:38:16Z

acceptance/framework/helpers/helpers.go

+// WaitForInput starts a http server on a random port (which is output in the logs) and waits until you
+// issue a request to that endpoint to continue the tests. This is useful for debugging tests that require
+// inspecting the current state of a running cluster and you don't need to use long sleeps
+func WaitForInput(t *testing.T) {


this was super useful for debugging the state of the cluster during tests and not needing to put a random sleep time in the test

moved this into it's own PR #4225

jm96441n · 2024-08-01T20:30:54Z

control-plane/catalog/registration/cache.go

@@ -231,9 +227,9 @@ func (c *RegistrationCache) updateTermGWACLRole(log logr.Logger, registration *v
 	var data bytes.Buffer
 	if err := gatewayTpl.Execute(&data, templateArgs{
 		EnablePartitions: c.partitionsEnabled,
-		Partition:        registration.Spec.Service.Partition,
+		Partition:        defaultIfEmpty(registration.Spec.Service.Partition),


these bits here fix the bug, the rest is just updating tests

jm96441n · 2024-08-12T20:29:33Z

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),
+


this test exercises the terminating gateway path and those don't function correctly without this in your helm chart

jm96441n · 2024-08-12T20:34:51Z

control-plane/catalog/registration/cache.go

 	"sigs.k8s.io/controller-runtime/pkg/client"
 )

 const NotInServiceMeshFilter = "ServiceMeta[\"managed-by\"] != \"consul-k8s-endpoints-controller\""

-func init() {


all this moved to the term gateway controller

jm96441n · 2024-08-12T20:36:10Z

control-plane/catalog/registration/cache.go


-				if err := c.k8sClient.Get(c.ctx, types.NamespacedName{Name: svc, Namespace: namespace}, registration); err != nil {


the service name here doesn't necessarily map directly to the registration name

jm96441n · 2024-08-12T20:36:47Z

control-plane/catalog/registration/registrations_controller.go

@@ -44,12 +44,12 @@ type RegistrationsController struct {
 	Log    logr.Logger
 }

-// +kubebuilder:rbac:groups=consul.hashicorp.com,resources=servicerouters,verbs=get;list;watch;create;update;patch;delete
-// +kubebuilder:rbac:groups=consul.hashicorp.com,resources=servicerouters/status,verbs=get;update;patch


found this copy/pasta mistake

jm96441n · 2024-08-12T20:36:53Z

control-plane/catalog/registration/registrations_controller.go


 func (r *RegistrationsController) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
 	log := r.Log.V(1).WithValues("registration", req.NamespacedName)
-	log.Info("Reconciling Registaration")


fixing typo

jm96441n · 2024-08-12T20:40:53Z

control-plane/controllers/configentries/configentry_controller_test.go

-		t.Run(c.kubeKind, func(t *testing.T) {
-			req := require.New(t)
-			ctx := context.Background()
+		for _, secure := range []bool{true, false} {


updated these tests to run in secure and non secure mode to ensure config entries are created when acls are enabled

left emtpy in acl policy if not specified in the CRD which results in an invalid acl policy

external service config entry registration from tests

jm96441n · 2024-08-16T16:11:17Z

acceptance/framework/helpers/helpers.go

 		}

 		t.Log("input received, continuing test")
+		go func() {


this was causing the caller to hang if we didn't do it async

jm96441n · 2024-08-16T16:12:43Z

control-plane/catalog/registration/registrations_controller.go

@@ -60,18 +60,6 @@ func (r *RegistrationsController) Reconcile(ctx context.Context, req ctrl.Reques
 		return ctrl.Result{}, client.IgnoreNotFound(err)
 	}

-	cachedRegistration, ok := r.Cache.get(registration.Spec.Service.Name)


moved this down so we handle deletion correctly

jm96441n · 2024-08-16T16:13:08Z

control-plane/catalog/registration/registrations_controller.go

@@ -143,48 +143,9 @@ func (r *RegistrationsController) handleRegistration(ctx context.Context, log lo
 		return result
 	}

-	if r.Cache.aclsEnabled() {


all the acl updates are done on the terminating gateway now to remove this controller racing with that one

fixtures

...ating-gateway-namespaces/all-non-default/external-service-registration/external-service.yaml

control-plane/subcommand/inject-connect/v1controllers.go

control-plane/controllers/configentries/terminatinggateway_controller.go

nathancoleman

Some thoughts after walking through the PR with @jm96441n on a call a few minutes ago

control-plane/controllers/configentries/terminatinggateway_controller.go

.changelog/4224.txt

acceptance/go.mod

acceptance/tests/config-entries/config_entries_namespaces_test.go

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

needed and updating variable names to be more consistent

control-plane/controllers/configentries/terminatinggateway_controller.go

…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>

…tration/ACL Setup for Terminating Gateways into release/1.5.x (#4259) [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 * 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: John Maguire <john.maguire@hashicorp.com> Co-authored-by: Nathan Coleman <nathan.coleman@hashicorp.com>

natemollica-nm · 2024-08-22T21:21:33Z

@jm96441n -- Awesome update to the TGW workflow!

Can you ensure that this gets updated though to account for enabling Admin Partitions -- Testing this I had to manually (tproxy destinations method) update the ACL policy for one of my external services from:

namespace "default" {
  service "example" {
    policy = "write"
  }
}

to

partition "default" {
  namespace "default" {
    node_prefix "" {
      policy = "read"
    }
    service "example" {
      policy    = "write"
      intention = "read"
    }
  }
}

Additionally, I'm positive the node:read and intention:read permissions are required for these policies (which is definitely an inconsistency in our docs):

Create a Terminating Gateway ACL Token: Requirements
Envoy Traffic Filter with Terminating Gateway | Blake's Comment
Document - Transparent Proxy + Terminating Gateway

jm96441n added pr/no-changelog PR does not need a corresponding .changelog entry backport/1.5.x labels Jul 31, 2024

jm96441n commented Jul 31, 2024

View reviewed changes

jm96441n commented Aug 1, 2024

View reviewed changes

jm96441n force-pushed the NET-10567-fix-namespace-normalization-registration branch from d05eb97 to bd7508f Compare August 5, 2024 14:42

jm96441n removed the pr/no-changelog PR does not need a corresponding .changelog entry label Aug 5, 2024

jm96441n requested review from a team, NiniOak and xwa153 and removed request for a team August 5, 2024 15:07

nathancoleman self-requested a review August 5, 2024 19:11

jm96441n commented Aug 12, 2024

View reviewed changes

jm96441n changed the title ~~[NET-10567] Fix namespace normalization on external registration~~ [NET-10567] Fix namespace normalization on external registration/ACL Setup for Terminating Gateways Aug 12, 2024

jm96441n commented Aug 12, 2024

View reviewed changes

jm96441n added 11 commits August 15, 2024 11:39

fix bug in external service registration ACL creation where namespace is

16b5d81

left emtpy in acl policy if not specified in the CRD which results in an invalid acl policy

Remove check for timestamp

b07ae41

update tests!

1dc22bf

update to use helper function

0ec89d1

all non default working

d56ad44

test cases all working

3d30c81

move wait for it to separate PR

937e39e

use replace for consul-k8s control-plane

7da17dc

update single namespace test

253002c

updated namespaces and destinations test

3947e69

remove usage of creating terminating gateway config entry creation and

73bbbe7

external service config entry registration from tests

handle removing policies when no other gateways reference them

197501d

jm96441n commented Aug 16, 2024

View reviewed changes

fix terminating gateway configuration for peering connect test

5617b2f

jm96441n requested review from a team and removed request for a team August 16, 2024 19:57

remove unnecessary nodeMeta on fixture, remove unused yaml files from

7fe78ce

fixtures

missylbytes reviewed Aug 19, 2024

View reviewed changes

...ating-gateway-namespaces/all-non-default/external-service-registration/external-service.yaml Show resolved Hide resolved

missylbytes reviewed Aug 19, 2024

View reviewed changes

control-plane/subcommand/inject-connect/v1controllers.go Show resolved Hide resolved

jm96441n commented Aug 20, 2024

View reviewed changes

control-plane/controllers/configentries/terminatinggateway_controller.go Outdated Show resolved Hide resolved

jm96441n commented Aug 20, 2024

View reviewed changes

control-plane/controllers/configentries/terminatinggateway_controller.go Outdated Show resolved Hide resolved

jm96441n commented Aug 20, 2024

View reviewed changes

control-plane/controllers/configentries/terminatinggateway_controller.go Outdated Show resolved Hide resolved

missylbytes approved these changes Aug 20, 2024

View reviewed changes

fix wildcard service names

a8bfe59

nathancoleman reviewed Aug 20, 2024

View reviewed changes

jm96441n and others added 4 commits August 20, 2024 14:04

use more specific matchers to avoid potential substring collisions

4953f78

Update .changelog/4224.txt

e77adb4

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

cleaning up from PR review: moving template execution to where it's

ad73acc

needed and updating variable names to be more consistent

add comment

e4cdf2c

nathancoleman reviewed Aug 20, 2024

View reviewed changes

control-plane/controllers/configentries/terminatinggateway_controller.go Outdated Show resolved Hide resolved

fix typo

a2ad236

nathancoleman approved these changes Aug 20, 2024

View reviewed changes

jm96441n enabled auto-merge (squash) August 20, 2024 20:04

jm96441n merged commit b138a4a into main Aug 20, 2024
50 checks passed

jm96441n deleted the NET-10567-fix-namespace-normalization-registration branch August 20, 2024 23:44

hc-github-team-consul-core mentioned this pull request Aug 20, 2024

Backport of [NET-10567] Fix namespace normalization on external registration/ACL Setup for Terminating Gateways into release/1.5.x #4259

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NET-10567] Fix namespace normalization on external registration/ACL Setup for Terminating Gateways #4224

[NET-10567] Fix namespace normalization on external registration/ACL Setup for Terminating Gateways #4224

jm96441n commented Jul 31, 2024 •

edited

Loading

jm96441n Jul 31, 2024

jm96441n Jul 31, 2024

jm96441n Aug 1, 2024

jm96441n Aug 1, 2024

jm96441n Aug 12, 2024

jm96441n Aug 12, 2024

jm96441n Aug 12, 2024

jm96441n Aug 12, 2024

jm96441n Aug 12, 2024

jm96441n Aug 12, 2024

jm96441n Aug 16, 2024 •

edited

Loading

jm96441n Aug 16, 2024

jm96441n Aug 16, 2024

nathancoleman left a comment

natemollica-nm commented Aug 22, 2024 •

edited

Loading

		@@ -90,6 +90,10 @@ func TestControllerNamespaces(t *testing.T) {

		"global.acls.manageSystemACLs": strconv.FormatBool(c.secure),
		"global.tls.enabled": strconv.FormatBool(c.secure),


		if err := c.k8sClient.Get(c.ctx, types.NamespacedName{Name: svc, Namespace: namespace}, registration); err != nil {

[NET-10567] Fix namespace normalization on external registration/ACL Setup for Terminating Gateways #4224

[NET-10567] Fix namespace normalization on external registration/ACL Setup for Terminating Gateways #4224

Conversation

jm96441n commented Jul 31, 2024 • edited Loading

Changes proposed in this PR

How I've tested this PR

How I expect reviewers to test this PR

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jm96441n Aug 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nathancoleman left a comment

Choose a reason for hiding this comment

natemollica-nm commented Aug 22, 2024 • edited Loading

jm96441n commented Jul 31, 2024 •

edited

Loading

jm96441n Aug 16, 2024 •

edited

Loading

natemollica-nm commented Aug 22, 2024 •

edited

Loading