agentless: Multi-cluster tproxy #1632

ishustava · 2022-10-18T23:59:06Z

Changes proposed in this PR:

Redirect traffic to the consul-dataplane's DNS proxy when DNS redirection is enabled. This means we no longer need to configure recursors for the clients and servers
To fall back to kube dns when consul cannot resolve names, we define our own DNSConfig on the pod that uses Consul first, but falls back to the config from /etc/resolv.conf in the cluster.

How I've tested this PR:
acceptance tests

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

ishustava · 2022-10-21T22:57:08Z

control-plane/connect-inject/peering_acceptor_controller_test.go

@@ -1194,7 +1194,7 @@ func TestAcceptorUpdateStatusError(t *testing.T) {
 			reconcileErr: errors.New("this is an error"),
 			expStatus: v1alpha1.PeeringAcceptorStatus{
 				Conditions: v1alpha1.Conditions{
-					{
+					v1alpha1.Condition{


These are from running linter locally

This is so weird. I remember getting linter failures when they initially existed and had to delete them. 🤔

Yeah I'm confused as well. Maybe it's a go 1.19 thing?

ishustava · 2022-10-22T02:13:11Z

acceptance/tests/consul-dns/consul_dns_test.go

@@ -59,16 +57,17 @@ func TestConsulDNS(t *testing.T) {
 				serverIPs = append(serverIPs, serverPod.Status.PodIP)
 			}

+			dnsPodName := fmt.Sprintf("%s-dns-pod", releaseName)


This test fails sometimes because a pod with this name already exists, so making the names unique per test.

ishustava · 2022-10-22T02:14:04Z

control-plane/connect-inject/peering_acceptor_controller_test.go

@@ -620,9 +620,9 @@ func TestReconcile_CreateUpdatePeeringAcceptor(t *testing.T) {
 			decodedTokenData, err := base64.StdEncoding.DecodeString(string(createdSecret.Data["data"]))
 			require.NoError(t, err)

-			require.Contains(t, string(decodedTokenData), "\"CA\":null")
+			require.Contains(t, string(decodedTokenData), "\"CA\":")


These changes are needed to support consul 1.14 peering changes so that we can use the latest binary in tests.

Ahh thanks for bumping that!!

This reverts commit 05ac0f9.

This reverts commit a88c944.

This reverts commit 12011f0.

thisisnotashwin

Have a few aesthetic comments but this is excellent!! Your comments made reviewing and understanding what was happening super easy! Great job!

thisisnotashwin · 2022-10-25T15:16:11Z

charts/consul/templates/_helpers.tpl

-{{/*
-Sets up a list of recusor flags for Consul agents by iterating over the IPs of every nameserver
-in /etc/resolv.conf and concatenating them into a string of arguments that can be passed directly
-to the consul agent command.
-*/}}
-{{- define "consul.recursors" -}}
-              recursor_flags=""
-              for ip in $(cat /etc/resolv.conf | grep nameserver | cut -d' ' -f2)
-              do
-                 recursor_flags="$recursor_flags -recursor=$ip"
-              done
-{{- end -}}
-


YAYYYYY!!!!

thisisnotashwin · 2022-10-25T15:24:42Z

control-plane/connect-inject/peering_acceptor_controller_test.go

@@ -1194,7 +1194,7 @@ func TestAcceptorUpdateStatusError(t *testing.T) {
 			reconcileErr: errors.New("this is an error"),
 			expStatus: v1alpha1.PeeringAcceptorStatus{
 				Conditions: v1alpha1.Conditions{
-					{
+					v1alpha1.Condition{


This is so weird. I remember getting linter failures when they initially existed and had to delete them. 🤔

thisisnotashwin · 2022-10-25T15:28:23Z

control-plane/connect-inject/dns.go

+
+func (w *MeshWebhook) configureDNS(pod *corev1.Pod, k8sNS string) error {
+	// First, we need to determine the nameservers configured in this cluster from /etc/resolv.conf.
+	etcResolvConf := "/etc/resolv.conf"


We should make this a constant. WDYT?

thisisnotashwin · 2022-10-25T15:34:40Z

control-plane/connect-inject/dns.go

+	// configured in our /etc/resolv.conf. It's important to add Consul DNS as the first nameserver because
+	// if we put kube DNS first, it will return NXDOMAIN response and a DNS client will not fall back to other nameservers.
+	if pod.Spec.DNSConfig == nil {
+		consulDPAddress := "127.0.0.1"


potentially make this a constant as well

thisisnotashwin · 2022-10-25T15:42:02Z

control-plane/connect-inject/dns.go

+		// Replace release namespace in the searches with the pod namespace.
+		// This is so that the searches we generate will be for the pod's namespace
+		// instead of the namespace of the connect-injector. E.g. instead of
+		// consul.svc.cluster.local it should be <pod ns>.svc.cluster.local.


Really appreciate the comments 🙏

ndhanushkodi · 2022-10-25T18:26:53Z

control-plane/connect-inject/peering_dialer_controller_test.go

-				c.Datacenter = "acceptor-dc"
+				c.Connect["ca_config"] = map[string]interface{}{
+					"cluster_id": "00000000-2222-3333-4444-555555555555",
+				}


ndhanushkodi · 2022-10-25T18:27:23Z

acceptance/tests/peering/peering_connect_test.go

-	if cfg.EnableTransparentProxy {
-		t.Skipf("skipping because no t-proxy support")
-	}
-


omg so this works???

yup! it works! 🎉

* Redirect traffic to the consul-dataplane's DNS proxy when DNS redirection is enabled. This means we no longer need to configure recursors for the clients and servers * To fall back to kube dns when consul cannot resolve names, we define our own DNSConfig on the pod that uses Consul first, but falls back to the config from /etc/resolv.conf in the cluster.

ishustava force-pushed the ishustava/tproxy-single-cluster branch from dc24cc4 to f092e46 Compare October 19, 2022 22:59

ishustava force-pushed the ishustava/tproxy-multi-cluster branch 6 times, most recently from acbe0ef to f986784 Compare October 21, 2022 01:42

ishustava changed the title ~~Multi-cluster tproxy~~ agentless: Multi-cluster tproxy Oct 21, 2022

ishustava force-pushed the ishustava/tproxy-multi-cluster branch 12 times, most recently from f990912 to 34ca662 Compare October 22, 2022 00:43

ishustava commented Oct 24, 2022

View reviewed changes

ishustava force-pushed the ishustava/tproxy-single-cluster branch from 117ebca to 15b09df Compare October 24, 2022 19:15

ishustava force-pushed the ishustava/tproxy-multi-cluster branch from 51475be to 57eee52 Compare October 24, 2022 19:33

Base automatically changed from ishustava/tproxy-single-cluster to main October 25, 2022 04:13

ishustava added 6 commits October 24, 2022 22:16

add more logging and switch consul image

ef3811e

disable ent for now because dev image is oss

ffcf657

create cni kind clusters

510b30e

make sure we wait to uninstall resources

a49ed22

Multi-cluster tproxy with consul-dataplane's DNS proxy

151d4ae

fix peering tests to work with consul 1.14 binary

d41540d

ishustava added 4 commits October 24, 2022 22:17

Revert "fix peering tests to work with consul 1.14 binary"

a6aefd9

This reverts commit 05ac0f9.

Revert to consul 1.13 in unit tests

3bf27a2

Revert "Revert "fix peering tests to work with consul 1.14 binary""

e4d3533

This reverts commit a88c944.

Revert "Revert to consul 1.13 in unit tests"

edd934f

This reverts commit 12011f0.

ishustava force-pushed the ishustava/tproxy-multi-cluster branch 3 times, most recently from 8e8b5ba to 3cbacec Compare October 25, 2022 04:22

run unit tests with latest consul binary

9437b2c

ishustava force-pushed the ishustava/tproxy-multi-cluster branch from 3cbacec to 9437b2c Compare October 25, 2022 04:30

ishustava marked this pull request as ready for review October 25, 2022 05:23

ishustava requested review from thisisnotashwin and curtbushko October 25, 2022 05:23

thisisnotashwin approved these changes Oct 25, 2022

View reviewed changes

address Ashwin's comments

a488a2c

ishustava force-pushed the ishustava/tproxy-multi-cluster branch from 267355b to a488a2c Compare October 25, 2022 16:08

ndhanushkodi reviewed Oct 25, 2022

View reviewed changes

Add changelog

0745e1e

ishustava force-pushed the ishustava/tproxy-multi-cluster branch from 1f83c02 to 6cff666 Compare October 25, 2022 20:59

use docker images to download consul binary for unit tests

486f1c4

ishustava force-pushed the ishustava/tproxy-multi-cluster branch from 6cff666 to 486f1c4 Compare October 25, 2022 21:03

ishustava merged commit 933cc5e into main Oct 25, 2022

ishustava deleted the ishustava/tproxy-multi-cluster branch October 25, 2022 21:44

curtbushko approved these changes Oct 26, 2022

View reviewed changes

natitomattis mentioned this pull request Jun 20, 2023

INFRA 11282 team-telnyx/consul-k8s#7

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

agentless: Multi-cluster tproxy #1632

agentless: Multi-cluster tproxy #1632

ishustava commented Oct 18, 2022 •

edited

Loading

ishustava Oct 21, 2022

thisisnotashwin Oct 25, 2022

ishustava Oct 25, 2022 •

edited

Loading

ishustava Oct 22, 2022

ishustava Oct 22, 2022

ndhanushkodi Oct 25, 2022

thisisnotashwin left a comment

thisisnotashwin Oct 25, 2022

thisisnotashwin Oct 25, 2022

thisisnotashwin Oct 25, 2022

thisisnotashwin Oct 25, 2022

thisisnotashwin Oct 25, 2022

ndhanushkodi Oct 25, 2022

ndhanushkodi Oct 25, 2022

ishustava Oct 25, 2022

agentless: Multi-cluster tproxy #1632

agentless: Multi-cluster tproxy #1632

Conversation

ishustava commented Oct 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ishustava Oct 25, 2022 • edited Loading

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

thisisnotashwin 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

Choose a reason for hiding this comment

ishustava commented Oct 18, 2022 •

edited

Loading

ishustava Oct 25, 2022 •

edited

Loading