Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Conversation

hc-github-team-consul-core
Copy link
Collaborator

Backport

This PR is auto-generated from #4224 to be assessed for backporting due to the inclusion of the label backport/1.5.x.

🚨

Warning automatic cherry-pick of commits failed. If the first commit failed,
you will see a blank no-op commit below. If at least one commit succeeded, you
will see the cherry-picked commits up to, not including, the commit where
the merge conflict occurred.

The person who merged in the original PR is:
@jm96441n
This person should manually cherry-pick the original PR into a new backport PR,
and close this one when the manual backport PR is merged in.

merge conflict error: POST https://api.github.com/repos/hashicorp/consul-k8s/merges: 409 Merge conflict []

The below text is copied from the body of the original PR.


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


Overview of commits

Copy link

hashicorp-cla-app bot commented Aug 20, 2024

CLA assistant check
All committers have signed the CLA.

Copy link

CLA assistant check

Thank you for your submission! We require that all contributors sign our Contributor License Agreement ("CLA") before we can accept the contribution. Read and sign the agreement

Learn more about why HashiCorp requires a CLA and what the CLA includes


temp seems not to be a GitHub user.
You need a GitHub account to be able to sign the CLA.
If you have already a GitHub account, please add the email address used for this commit to your account.

Have you signed the CLA already but the status is still pending? Recheck it.

…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 <[email protected]>

* 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 <[email protected]>
@jm96441n jm96441n force-pushed the backport/NET-10567-fix-namespace-normalization-registration/virtually-one-guinea branch from c741fb9 to 7165d48 Compare August 21, 2024 13:33
@jm96441n jm96441n marked this pull request as ready for review August 21, 2024 13:33
@jm96441n jm96441n enabled auto-merge (squash) August 21, 2024 13:33
}

mux := http.NewServeMux()
srv := &http.Server{

Check warning

Code scanning / Go Modules Scanner

HTTP server insecure configuration Warning

server has no ReadTimeout
}

mux := http.NewServeMux()
srv := &http.Server{

Check warning

Code scanning / Go Modules Scanner

HTTP server insecure configuration Warning

server has no ReadHeaderTimeout
}

mux := http.NewServeMux()
srv := &http.Server{

Check warning

Code scanning / Go Modules Scanner

HTTP server insecure configuration Warning

server has no WriteTimeout
}

mux := http.NewServeMux()
srv := &http.Server{

Check warning

Code scanning / Go Modules Scanner

HTTP server insecure configuration Warning

server has no IdleTimeout
@jm96441n jm96441n merged commit 557f7c3 into release/1.5.x Aug 21, 2024
50 checks passed
@jm96441n jm96441n deleted the backport/NET-10567-fix-namespace-normalization-registration/virtually-one-guinea branch August 21, 2024 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants