Cluster Peering #1273

ndhanushkodi · 2022-06-13T21:25:24Z

Changes proposed in this PR:

Best reviewed by commit, plan is to rebase and merge (not squash) this branch into main
Adds PeeringAcceptor controller run as part of the connect-inject deployment
Adds PeeringDialer controller run as part of the connect-inject deployment
Adds PeeringAcceptor and Peering Dialer CRs
Adds Peer field to Exported Services CR and Intentions CR
Allows ExportedServices CR to be used in OSS (previously it was used only for cross-partition in ENT)
Adds Helm value global.peering.enabled which will deploy the controllers and CRs for peering, if connectInject.enabled is also true.

How I've tested this PR:
unit tests and manual testing using steps here

How I expect reviewers to test this PR:
👀

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

ndhanushkodi · 2022-06-13T21:31:09Z

.github/workflows/test.yml

@@ -142,14 +142,13 @@ jobs:
      - run:  mkdir -p ${{env.TEST_RESULTS}}
      - run:  echo "$HOME/bin" >> $GITHUB_PATH



The changes in this file will need to be reverted before merging-- but we need the Consul release to happen first. Then we need to bump the CONSUL_VERSION variable to be the latest Consul release

So we won't be able to merge this until after Consul's release?

ishustava · 2022-06-13T23:37:01Z

.github/workflows/test.yml

@@ -142,14 +142,13 @@ jobs:
      - run:  mkdir -p ${{env.TEST_RESULTS}}
      - run:  echo "$HOME/bin" >> $GITHUB_PATH



So we won't be able to merge this until after Consul's release?

charts/consul/templates/connect-inject-clusterrole.yaml

control-plane/api/v1alpha1/exportedservices_types.go

ishustava · 2022-06-13T23:50:50Z

control-plane/api/v1alpha1/exportedservices_types.go

@@ -69,6 +68,8 @@ type ExportedService struct {
 type ServiceConsumer struct {
 	// Partition is the admin partition to export the service to.
 	Partition string `json:"partition,omitempty"`
+	// [Experimental] PeerName is the name of the peer to export the service to.
+	PeerName string `json:"peerName,omitempty"`


nit: should this be Peer to match partition above?

we have matched the name from the consul config entry so far but I actually do prefer this.

Do you think its ok to have a different field name from the consul config entry/have we done this before? Should we consider changing the config entry field in core to also match?

It would be great if we could change this in core as well TBH. I think peer is verbose enough.

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

ishustava · 2022-06-14T16:42:45Z

control-plane/connect-inject/peering_dialer_controller.go

+	if specSecret == nil {
+		err = errors.New("PeeringDialer spec.peer.secret does not exist")
+		r.updateStatusError(ctx, dialer, err)
+		return ctrl.Result{}, err
+	}


should getSecret return an error for this case?

In the future, this step will move into a validation webhook. and trying to get a secret that does not exist is a valid use case for when we want to get the secret from the status.

🤔 In this case though we're getting the secret from the spec not status. And the call above on line 88 gets the secret. Then here we're checking if secret is nil and returning an error, so I'm wondering why not combine these two calls into one and just make getSecret return an error when the secret doesn't exist instead of returning an error here.

Maybe I'm misunderstanding something.

Aah! sorry. I should have been specific. We use the method to get the status secret (L110) too. And if it is the first reconcile, it is ok if the secret doesn't exist, so it would be ok for getSecret to not return an error if the secret does not exist.

control-plane/connect-inject/peering_dialer_controller.go

ishustava · 2022-06-14T16:52:25Z

@ndhanushkodi will changelog be added separately or in this PR?

ishustava

Looks great! A couple of follow-up comments, but won't block approval as everything looks great!

❤️ ❤️ ❤️

ishustava · 2022-06-14T20:26:45Z

control-plane/connect-inject/consul_sidecar.go

@@ -11,13 +11,13 @@ import (
 // the metrics merging server when metrics merging feature is enabled.
 // It always disables service registration because for connect we no longer
 // need to keep services registered as this is handled in the endpoints-controller.
-func (h *Handler) consulSidecar(pod corev1.Pod) (corev1.Container, error) {
-	metricsPorts, err := h.MetricsConfig.mergedMetricsServerConfiguration(pod)
+func (w *MeshWebhook) consulSidecar(pod corev1.Pod) (corev1.Container, error) {


ishustava · 2022-06-14T20:30:32Z

control-plane/api/v1alpha1/serviceintentions_types.go

+		if source.Peer != "" && source.Partition != "" {
+			errs = append(errs, field.Invalid(path.Child("sources").Index(i), source, `Both source.peer and source.partition cannot be set.`))


Oh yes I do mean that! Thank you for reading my mind!

ishustava · 2022-06-14T20:31:24Z

control-plane/connect-inject/endpoints_controller.go

+
+	service := parts[0]
+
+	pieces := strings.Split(service, ".")


yep, sounds good!

ishustava · 2022-06-14T20:32:34Z

control-plane/connect-inject/peering_acceptor_controller.go

+			if err := r.deletePeering(ctx, req.Name); err != nil {
+				return ctrl.Result{}, err
+			}
+			if acceptor.Secret().Backend == "kubernetes" {
+				if err := r.deleteK8sSecret(ctx, acceptor); err != nil {
+					return ctrl.Result{}, err
+				}
+			}
+			controllerutil.RemoveFinalizer(acceptor, FinalizerName)


Oh yeah that is surprising, but I suppose is a good thing for this case since this will just work.

ishustava · 2022-06-14T20:40:41Z

control-plane/connect-inject/peering_dialer_controller.go

+	if specSecret == nil {
+		err = errors.New("PeeringDialer spec.peer.secret does not exist")
+		r.updateStatusError(ctx, dialer, err)
+		return ctrl.Result{}, err
+	}


🤔 In this case though we're getting the secret from the spec not status. And the call above on line 88 gets the secret. Then here we're checking if secret is nil and returning an error, so I'm wondering why not combine these two calls into one and just make getSecret return an error when the secret doesn't exist instead of returning an error here.

Maybe I'm misunderstanding something.

ndhanushkodi · 2022-06-15T01:27:36Z

control-plane/connect-inject/peering_dialer_controller_test.go

+
+	// After reconciliation, Consul should have a peering with its DeletedAt set.
+	peering, _, err := consulClient.Peerings().Read(context.Background(), "dialer-deleted", nil)
+	require.NotNil(t, peering.DeletedAt)


Seeing the tests fail with a nil pointer exception here. I think its because the deletion logic was implemented in Consul, and the peering is sometimes being deleted before we can read it here. So I don't think we can make this assertion anymore. Maybe we can do a retry to check that the peering is nil? Since it's a deferred delete?

I think you need to build a newer Consul image. I ran into the same issue but turns out, I had an older Consul binary.

I added the new binaries for consul/consulent and saw the same issue in the pipeline over multiple runs with the new binaries. But locally with those same binaries I was seeing different results. I think it is the deferred deletion logic that was added since I added a retry that waits 5 seconds and retries asserting that the peering is nil instead of looking at the DeletedAt stamp, and now it doesn't intermittently fail with nil pointer exceptions. I'll try pushing it now and seeing if it passes in the pipeline. Edit: I see it pass the pipeline now!

THE DEFERRED DELETION!!! ok ok. that makes sense. thank you so much Nitya! that was an excellent catch.

thisisnotashwin

Woot woot! lets get this puppy merged today!!

Adds new PeeringAcceptor CRD and controller with unit tests. This controller determines whether to generate a new peering token in Consul by reconciling the PeeringAcceptor resource. It currently supports writing the peering token to a K8s secret. Generating the new CRD and API: After updating control-plane/PROJECT L6 to reference the control plane folder, ran the command: `operator-sdk create api --group consul --version v1alpha1 --kind Peering --controller --namespaced=true --make=false --resource=true`

PeeringDialer CRD and controller, not including unit tests.

- Clean up some of the logic in peering dialler and acceptor - Rename handler -> connectWebhook

update source intentions with peer name and bump api module and change initiate -> establish

- Remove ownerRef from secret created by acceptor.

- when global.peering.enabled is true, and connectInject.enabled is true, enable the peering controllers and the peering CRDs.

- rename exported service CRD consumer.peerName field from peerName --> peer - rename ConnectWebhook --> MeshWebhook - add some missing unit test cases - only add permissions to connect-inject service account for peering CRs when peering is enabled

ndhanushkodi requested a review from ishustava June 13, 2022 21:25

ndhanushkodi force-pushed the peering branch from c9fef42 to bb5f84b Compare June 13, 2022 21:29

ndhanushkodi requested a review from thisisnotashwin June 13, 2022 21:29

ndhanushkodi commented Jun 13, 2022

View reviewed changes

ndhanushkodi force-pushed the peering branch 2 times, most recently from a9637a9 to 3f6b9c0 Compare June 13, 2022 21:42

thisisnotashwin force-pushed the peering branch from 62c7c3e to fc1e4fe Compare June 13, 2022 23:52

ishustava reviewed Jun 14, 2022

View reviewed changes

thisisnotashwin force-pushed the peering branch from fc1e4fe to 318a4e7 Compare June 14, 2022 19:30

thisisnotashwin requested a review from ishustava June 14, 2022 19:31

thisisnotashwin force-pushed the peering branch from 318a4e7 to 485fd73 Compare June 14, 2022 19:34

ishustava approved these changes Jun 14, 2022

View reviewed changes

ndhanushkodi force-pushed the peering branch from 485fd73 to b4e76b2 Compare June 15, 2022 00:52

ndhanushkodi commented Jun 15, 2022

View reviewed changes

ndhanushkodi force-pushed the peering branch from b4e76b2 to 2a6cfb8 Compare June 15, 2022 06:41

thisisnotashwin approved these changes Jun 15, 2022

View reviewed changes

thisisnotashwin force-pushed the peering branch from 2a6cfb8 to 6387c89 Compare June 15, 2022 16:38

ndhanushkodi force-pushed the peering branch 2 times, most recently from 4b2ee85 to 46ba198 Compare June 15, 2022 16:46

thisisnotashwin force-pushed the peering branch from 1981735 to 4d703ad Compare June 16, 2022 15:17

Ashwin Venkatesh and others added 9 commits June 16, 2022 11:25

Add peerName to ExportedServices. (#1239)

2c6eb17

PeeringDialer controller (#1236)

4608c90

PeeringDialer CRD and controller, not including unit tests.

Support new annotated format for upstreams (#1237)

db6f47e

update api module, update exported services validation (#1248)

3e848cf

Backfill unit tests for peering dialler (#1265)

636816e

- Clean up some of the logic in peering dialler and acceptor - Rename handler -> connectWebhook

Add Peer field to Intention CRD source intention (#1263)

0b38248

update source intentions with peer name and bump api module and change initiate -> establish

Add finalizers to peering acceptor and dialer

d2fa0ad

- Remove ownerRef from secret created by acceptor.

add a helm value for cluster peering

e9f53c4

- when global.peering.enabled is true, and connectInject.enabled is true, enable the peering controllers and the peering CRDs.

thisisnotashwin and others added 2 commits June 16, 2022 11:27

Add changelog for cluster peering (#1279)

430e52f

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

thisisnotashwin merged commit 136d312 into main Jun 16, 2022

thisisnotashwin deleted the peering branch June 16, 2022 15:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cluster Peering #1273

Cluster Peering #1273

ndhanushkodi commented Jun 13, 2022

ndhanushkodi Jun 13, 2022

ishustava Jun 13, 2022

thisisnotashwin Jun 14, 2022

ishustava Jun 13, 2022

ishustava Jun 13, 2022

thisisnotashwin Jun 14, 2022

ndhanushkodi Jun 15, 2022

thisisnotashwin Jun 15, 2022

ishustava Jun 14, 2022

thisisnotashwin Jun 14, 2022

ishustava Jun 14, 2022

thisisnotashwin Jun 15, 2022

ishustava commented Jun 14, 2022

ishustava left a comment

ishustava Jun 14, 2022

ishustava Jun 14, 2022

ishustava Jun 14, 2022

ishustava Jun 14, 2022

ishustava Jun 14, 2022

ndhanushkodi Jun 15, 2022

thisisnotashwin Jun 15, 2022

ndhanushkodi Jun 15, 2022 •

edited

Loading

thisisnotashwin Jun 15, 2022

thisisnotashwin left a comment

		@@ -142,14 +142,13 @@ jobs:
		- run: mkdir -p ${{env.TEST_RESULTS}}
		- run: echo "$HOME/bin" >> $GITHUB_PATH

		if source.Peer != "" && source.Partition != "" {
		errs = append(errs, field.Invalid(path.Child("sources").Index(i), source, `Both source.peer and source.partition cannot be set.`))

Cluster Peering #1273

Cluster Peering #1273

Conversation

ndhanushkodi commented Jun 13, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ishustava commented Jun 14, 2022

ishustava left a 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

ndhanushkodi Jun 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thisisnotashwin left a comment

Choose a reason for hiding this comment

ndhanushkodi Jun 15, 2022 •

edited

Loading