[NET-9500] Cleanup orphaned inline-certs and acl role/policy #4067

jm96441n · 2024-06-03T21:27:43Z

Changes proposed in this PR

Add a goroutine on start up that loops and tries to clean up inline certs that are no longer referenced by any gateway and the original shared acl role/policy that is now specific to each gateway. Once all those are cleaned up the goroutine will exit and stop polling

How I've tested this PR

Run/Write tests

Manual Testing:

clone the following gist: https://gist.github.com/jm96441n/b92a80c2251c7a54d1feada320c5eac6
run the start.sh script in the gist (this will install consul 1.18-ent so ensure you have an ent license setup)
port forward port 8501 to the consul server instance
once the cluster is up run the following in another terminal:

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

run consul acl role list and you will see the shared "managed-gateway-acl-role"
run consul acl policy list and you will see the shared "api-gateway-token-policy"
run consul acl binding-rule list and you will see the shared binding-rules referencing both managed-gateway-acl-role
run consul config list -kind inline-certificate and you will see an inline certificate listed
in the consul_values.yaml uncomment lines 3 and 4 and comment lines 4 and 5 (also make sure you build consul-k8s from this branch and have an up to date build of main of consul)
in the same terminal run export CONSUL_K8S_CHARTS_LOCATION="$HOME/hashi/consul-k8s/charts/consul" and replace the value being set with the location of the helm charts in your local copy of consul-k8s
run the following helm upgrade --install consul $CONSUL_K8S_CHARTS_LOCATION -f ./consul_values.yaml -n consul --create-namespace --wait to install the new version of consul-k8s that you built
force a re-reconciliation of your gateways (this should happen by default due to the change in dataplane version, if it does not you can delete and recreate the gateways)
run consul acl role list and you will not see the shared "managed-gateway-acl-role"
run consul acl policy list and you will not see the shared "api-gateway-token-policy"
. run consul acl binding-rule list and you will not see the shared binding-rules referencing managed-gateway-acl-role
run consul config list -kind inline-certificate and you will see no inline certificates listed

How I expect reviewers to test this PR

read the code
run the tests
do the above steps

Checklist

Tests added
CHANGELOG entry added

certs

…ld-acl-policy

jm96441n · 2024-06-05T15:32:47Z

control-plane/api-gateway/binding/cleanup.go

+
+	oldBindingRules := make(map[string]*api.ACLBindingRule)
+
+	// here we need to find binding rules with the old name that have a matching selector to the new gateway specific binding rule


so another direction we could've gone down here is to fetch all the gateways and then for each gateway match it to a binding rule and if we match each gateway to a new binding rule then we can safely delete all the binding rules, definitely open to push back to change this to utilize that heuristic for deleting binding rules rather than what we have here (and we already fetch all the gateways for the inline cert cleanup so it's not like we don't need that data anyways)

NiniOak

Code looks fine but the test steps don't seem correct. Happy to re-review it with you before approval

…ld-acl-policy

jm96441n · 2024-06-12T14:48:29Z

Code looks fine but the test steps don't seem correct. Happy to re-review it with you before approval

repro steps should be fixed now! sorry about that!

control-plane/api-gateway/binding/cleanup.go

nathancoleman · 2024-06-12T19:57:24Z

control-plane/api-gateway/binding/cleanup.go

+		client, err := consul.NewClientFromConnMgr(c.ConsulConfig, c.ServerMgr)
+		if err != nil {
+			c.Logger.Error(err, "failed to create Consul client")
+			continue
+		}


Do we need to recreate the client every 60s?

we do this for the cache code as well, everytime the cache syncs we re-create the client so I wanted to mirror that functionality

control-plane/api-gateway/binding/cleanup.go

nathancoleman · 2024-06-12T20:01:01Z

control-plane/api-gateway/binding/cleanup.go

+
+// cleanupACLRoleAndPolicy deletes the old shared gateway ACL role and policy if they exist.
+func (c Cleaner) cleanupACLRoleAndPolicy(client *api.Client) (bool, error) {
+	existingRules, _, err := client.ACL().BindingRuleList(c.AuthMethod, &api.QueryOptions{})


Does this list for all namespaces?

Discussed on Slack that the gateway approach described below is probably more scalable in all use cases and would query directly whatever namespace the gateway resided in.

we also always write the binding-rules into the default namespace and partition https://github.com/hashicorp/consul-k8s/blob/main/control-plane/api-gateway/cache/consul.go#L557

control-plane/api-gateway/binding/cleanup.go

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

control-plane/api-gateway/binding/cleanup_test.go

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

* add cleanup goroutine to handle cleaning up old acl roles and inline certs * added first unit test * added more tests * fixing binding rule deleting * linting * added tests and cleaned up * Update control-plane/api-gateway/binding/cleanup.go Co-authored-by: Nathan Coleman <nathan.coleman@hashicorp.com> * Update control-plane/api-gateway/binding/cleanup.go Co-authored-by: Nathan Coleman <nathan.coleman@hashicorp.com> * Update control-plane/api-gateway/binding/cleanup.go Co-authored-by: Nathan Coleman <nathan.coleman@hashicorp.com> * Update control-plane/api-gateway/binding/cleanup_test.go Co-authored-by: Nathan Coleman <nathan.coleman@hashicorp.com> * Increase sleep time --------- Co-authored-by: Nathan Coleman <nathan.coleman@hashicorp.com>

…4125) * add cleanup goroutine to handle cleaning up old acl roles and inline certs * added first unit test * added more tests * fixing binding rule deleting * linting * added tests and cleaned up * Update control-plane/api-gateway/binding/cleanup.go * Update control-plane/api-gateway/binding/cleanup.go * Update control-plane/api-gateway/binding/cleanup.go * Update control-plane/api-gateway/binding/cleanup_test.go * Increase sleep time --------- Co-authored-by: Nathan Coleman <nathan.coleman@hashicorp.com>

…4124) * add cleanup goroutine to handle cleaning up old acl roles and inline certs * added first unit test * added more tests * fixing binding rule deleting * linting * added tests and cleaned up * Update control-plane/api-gateway/binding/cleanup.go * Update control-plane/api-gateway/binding/cleanup.go * Update control-plane/api-gateway/binding/cleanup.go * Update control-plane/api-gateway/binding/cleanup_test.go * Increase sleep time --------- Co-authored-by: Nathan Coleman <nathan.coleman@hashicorp.com>

* add cleanup goroutine to handle cleaning up old acl roles and inline certs * added first unit test * added more tests * fixing binding rule deleting * linting * added tests and cleaned up * Update control-plane/api-gateway/binding/cleanup.go Co-authored-by: Nathan Coleman <nathan.coleman@hashicorp.com> * Update control-plane/api-gateway/binding/cleanup.go Co-authored-by: Nathan Coleman <nathan.coleman@hashicorp.com> * Update control-plane/api-gateway/binding/cleanup.go Co-authored-by: Nathan Coleman <nathan.coleman@hashicorp.com> * Update control-plane/api-gateway/binding/cleanup_test.go Co-authored-by: Nathan Coleman <nathan.coleman@hashicorp.com> * Increase sleep time --------- Co-authored-by: Nathan Coleman <nathan.coleman@hashicorp.com>

…4131)

…4132)

…4133)

* add cleanup goroutine to handle cleaning up old acl roles and inline certs * added first unit test * added more tests * fixing binding rule deleting * linting * added tests and cleaned up * Update control-plane/api-gateway/binding/cleanup.go Co-authored-by: Nathan Coleman <nathan.coleman@hashicorp.com> * Update control-plane/api-gateway/binding/cleanup.go Co-authored-by: Nathan Coleman <nathan.coleman@hashicorp.com> * Update control-plane/api-gateway/binding/cleanup.go Co-authored-by: Nathan Coleman <nathan.coleman@hashicorp.com> * Update control-plane/api-gateway/binding/cleanup_test.go Co-authored-by: Nathan Coleman <nathan.coleman@hashicorp.com> * Increase sleep time --------- Co-authored-by: Nathan Coleman <nathan.coleman@hashicorp.com>

jm96441n added 3 commits June 3, 2024 16:29

add cleanup goroutine to handle cleaning up old acl roles and inline

74ee754

certs

added first unit test

eed31ad

added more tests

1eeeec4

jm96441n changed the title ~~add cleanup goroutine to handle cleaning up old acl roles and inline~~ [NET-9500] Cleanup orphaned inline-certs and acl role/policy Jun 4, 2024

jm96441n added 2 commits June 4, 2024 16:09

fixing binding rule deleting

120d2c8

Merge branch 'main' into NET-9500-cleanup-existing-inline-certs-and-o…

743d568

…ld-acl-policy

jm96441n added backport/1.2.x This release branch is no longer active. backport/1.3.x backport/1.4.x backport/1.5.x pr/no-changelog PR does not need a corresponding .changelog entry labels Jun 4, 2024

jm96441n added 2 commits June 4, 2024 16:42

linting

49339cd

added tests and cleaned up

8a932cb

jm96441n marked this pull request as ready for review June 5, 2024 15:30

jm96441n commented Jun 5, 2024

View reviewed changes

jm96441n requested review from a team, NiniOak and wangxinyi7 and removed request for a team June 5, 2024 15:35

NiniOak reviewed Jun 7, 2024

View reviewed changes

Merge branch 'main' into NET-9500-cleanup-existing-inline-certs-and-o…

26a77f1

…ld-acl-policy

nathancoleman reviewed Jun 12, 2024

View reviewed changes

control-plane/api-gateway/binding/cleanup.go Outdated Show resolved Hide resolved

nathancoleman reviewed Jun 12, 2024

View reviewed changes

control-plane/api-gateway/binding/cleanup.go Outdated Show resolved Hide resolved

Update control-plane/api-gateway/binding/cleanup.go

e564c7a

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

nathancoleman reviewed Jun 12, 2024

View reviewed changes

control-plane/api-gateway/binding/cleanup_test.go Outdated Show resolved Hide resolved

jm96441n and others added 3 commits June 12, 2024 16:15

Update control-plane/api-gateway/binding/cleanup.go

474afbe

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

Update control-plane/api-gateway/binding/cleanup.go

386c11d

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

Update control-plane/api-gateway/binding/cleanup_test.go

e97a47c

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

Increase sleep time

00125a5

jm96441n enabled auto-merge (squash) June 12, 2024 20:44

Log confirmation when cleaning up resources

6c2a380

nathancoleman approved these changes Jun 12, 2024

View reviewed changes

jm96441n merged commit b08043e into main Jun 12, 2024
25 of 50 checks passed

jm96441n deleted the NET-9500-cleanup-existing-inline-certs-and-old-acl-policy branch June 12, 2024 22:46

This was referenced Jun 12, 2024

[NET-9500] Cleanup orphaned inline-certs and acl role/policy (#4067) #4124

Merged

[NET-9500] Cleanup orphaned inline-certs and acl role/policy (#4067) #4125

Merged

jm96441n added a commit that referenced this pull request Jun 13, 2024

[NET-9500] Cleanup orphaned inline-certs and acl role/policy (#4067) (#…

2b60feb

…4131)

jm96441n added a commit that referenced this pull request Jun 13, 2024

[NET-9500] Cleanup orphaned inline-certs and acl role/policy (#4067) (#…

e78dd07

…4132)

jm96441n added a commit that referenced this pull request Jun 13, 2024

[NET-9500] Cleanup orphaned inline-certs and acl role/policy (#4067) (#…

286edfd

…4133)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NET-9500] Cleanup orphaned inline-certs and acl role/policy #4067

[NET-9500] Cleanup orphaned inline-certs and acl role/policy #4067

jm96441n commented Jun 3, 2024 •

edited

Loading

jm96441n Jun 5, 2024

NiniOak left a comment

jm96441n commented Jun 12, 2024

nathancoleman Jun 12, 2024

jm96441n Jun 12, 2024

nathancoleman Jun 12, 2024

nathancoleman Jun 12, 2024

jm96441n Jun 12, 2024


		oldBindingRules := make(map[string]*api.ACLBindingRule)

		// here we need to find binding rules with the old name that have a matching selector to the new gateway specific binding rule

[NET-9500] Cleanup orphaned inline-certs and acl role/policy #4067

[NET-9500] Cleanup orphaned inline-certs and acl role/policy #4067

Conversation

jm96441n commented Jun 3, 2024 • edited Loading

Changes proposed in this PR

How I've tested this PR

Manual Testing:

How I expect reviewers to test this PR

Checklist

jm96441n Jun 5, 2024

Choose a reason for hiding this comment

NiniOak left a comment

Choose a reason for hiding this comment

jm96441n commented Jun 12, 2024

nathancoleman Jun 12, 2024

Choose a reason for hiding this comment

jm96441n Jun 12, 2024

Choose a reason for hiding this comment

nathancoleman Jun 12, 2024

Choose a reason for hiding this comment

nathancoleman Jun 12, 2024

Choose a reason for hiding this comment

jm96441n Jun 12, 2024

Choose a reason for hiding this comment

jm96441n commented Jun 3, 2024 •

edited

Loading