Add secret watchers on Peering Acceptor/Dialer Controllers #1284

thisisnotashwin · 2022-06-15T17:30:39Z

Changes proposed in this PR:

Add watches to both the Peering Acceptor and Dialer controller. These watch secrets with a specific label and reconcile the associated acceptor/dialer when the secret that matches their status/spec secret is updated. Additionally, when the acceptor creates the token secret, it now adds that label to the created secret.

How I've tested this PR:

Unit tests.
Created a peering acceptor and dialer in different clusters. After the secret was created and copied over, deleted the contents of the secret in the secret on the acceptor side. This generated a new token. Copied this secret over again and observed the reconcile happen on the dialer end as well.

How I expect reviewers to test this PR:

👀
attempt to perform the above as well.

Checklist:

Tests added
CHANGELOG entry added

HashiCorp engineers only, community PRs should not add a changelog entry.
Entries should use present tense (e.g. Add support for...)

control-plane/connect-inject/peering_acceptor_controller.go

control-plane/connect-inject/peering_acceptor_controller_test.go

control-plane/connect-inject/peering_dialer_controller.go

control-plane/connect-inject/peering_dialer_controller_test.go

kschoche

Looks great!
I left a few questions/comments but nothing blocking.

ndhanushkodi

This looks amazing!! I was wondering what your thoughts are on a duplicate check, but it doesn't need to be in scope of this PR.

control-plane/connect-inject/peering_acceptor_controller.go

control-plane/connect-inject/peering_dialer_controller.go

ndhanushkodi · 2022-06-23T16:51:06Z

control-plane/connect-inject/peering_acceptor_controller.go

+		if acceptor.SecretRef().Backend == "kubernetes" {
+			if acceptor.SecretRef().Name == object.GetName() && acceptor.Namespace == object.GetNamespace() {
+				return []ctrl.Request{{NamespacedName: types.NamespacedName{Namespace: acceptor.Namespace, Name: acceptor.Name}}}
+			}


We currently don't do any validation on if there were multiple acceptors that had the same secret name/namespace, and in theory someone could get into that case, so maybe the logic here can stay the same, but maybe we'd want to add a duplicate check?

It may not be in scope for this PR--

I'm imagining a label on the secret that describes the namespaced name of the acceptor that generated it, and then in the reconcile loop, erroring out if there's an existing secret with the same name and namespace in the acceptor spec, that references a different acceptor resource than the one being reconciled.

hmm.. that is a good point. I think it makes sense to move that into a validation? ill add that as a follow-up task on Asana. This can potentially be true for dialers as well, where multiple dialers have the same spec secret.

- When a Kubernetes secret that has a label indicating it is a peering token secret, the controllers watch those secrets and updated to those secrets re-enqueues the resource that is associated with that peering token secret. The status is used to determine the Peering Acceptor that is re-enqueued while the spec is used to determing the Peering Dialer that gets re-enqueued. This is because the acceptor is responsible for creating the secret and hence metaphorically owns the secret described in it's status. OTOH the dialer should respond to changes in the secret described in it's spec. This is only supported for secrets with a Kubernetes backend.

thisisnotashwin force-pushed the peering-watches branch from ff2dd5e to a31fc72 Compare June 15, 2022 17:32

thisisnotashwin force-pushed the peering branch 2 times, most recently from 4d703ad to 430e52f Compare June 16, 2022 15:27

Base automatically changed from peering to main June 16, 2022 15:40

thisisnotashwin force-pushed the peering-watches branch 2 times, most recently from 5f11f72 to 99e74a6 Compare June 20, 2022 18:57

thisisnotashwin changed the title ~~Peering watches~~ Add secret watchers on Peering Acceptor/Dialer Controllers Jun 21, 2022

thisisnotashwin marked this pull request as ready for review June 21, 2022 14:55

thisisnotashwin requested review from ndhanushkodi, a team and kschoche and removed request for a team June 21, 2022 14:55