From 0385a5bb589577c64e35a902aa77ba5968149c96 Mon Sep 17 00:00:00 2001
From: Chris Piraino <cpiraino@hashicorp.com>
Date: Thu, 23 Jul 2020 13:12:08 -0500
Subject: [PATCH] Fix envoy bootstrap logic to not append multiple self_admin
 clusters (#8371)

Previously, the envoy bootstrap config would blindly copy the self_admin
cluster into the list of static clusters when configuring either
ReadyBindAddr, PrometheusBindAddr, or StatsBindAddr.

Since ingress gateways always configure the ReadyBindAddr property,
users ran into this case much more often than previously.
---
 command/connect/envoy/bootstrap_config.go     | 43 ++++++++++++++++---
 .../connect/envoy/bootstrap_config_test.go    | 25 +++++++++++
 2 files changed, 63 insertions(+), 5 deletions(-)

diff --git a/command/connect/envoy/bootstrap_config.go b/command/connect/envoy/bootstrap_config.go
index 87406fd896..f2d696e216 100644
--- a/command/connect/envoy/bootstrap_config.go
+++ b/command/connect/envoy/bootstrap_config.go
@@ -11,6 +11,10 @@ import (
 	"text/template"
 )
 
+const (
+	selfAdminName = "self_admin"
+)
+
 // BootstrapConfig is the set of keys we care about in a Connect.Proxy.Config
 // map. Note that this only includes config keys that affects Envoy bootstrap
 // generation. For Envoy config keys that affect runtime xDS behavior see
@@ -70,7 +74,7 @@ type BootstrapConfig struct {
 	// liveness of an Envoy instance when no other listeners are garaunteed to be
 	// configured, as is the case with ingress gateways.
 	//
-	// Not that we do not allow this to be configured via the service
+	// Note that we do not allow this to be configured via the service
 	// definition config map currently.
 	ReadyBindAddr string `mapstructure:"-"`
 
@@ -405,7 +409,7 @@ func (c *BootstrapConfig) generateListenerConfig(args *BootstrapTplArgs, bindAdd
 	}
 
 	clusterJSON := `{
-		"name": "self_admin",
+		"name": "` + selfAdminName + `",
 		"connect_timeout": "5s",
 		"type": "STATIC",
 		"http_protocol_options": {},
@@ -476,10 +480,18 @@ func (c *BootstrapConfig) generateListenerConfig(args *BootstrapTplArgs, bindAdd
 		]
 	}`
 
-	if args.StaticClustersJSON != "" {
-		clusterJSON = ",\n" + clusterJSON
+	// Make sure we do not append the same cluster multiple times, as that will
+	// cause envoy startup to fail.
+	selfAdminClusterExists, err := containsSelfAdminCluster(args.StaticClustersJSON)
+	if err != nil {
+		return err
+	}
+
+	if args.StaticClustersJSON == "" {
+		args.StaticClustersJSON = clusterJSON
+	} else if !selfAdminClusterExists {
+		args.StaticClustersJSON += ",\n" + clusterJSON
 	}
-	args.StaticClustersJSON += clusterJSON
 
 	if args.StaticListenersJSON != "" {
 		listenerJSON = ",\n" + listenerJSON
@@ -487,3 +499,24 @@ func (c *BootstrapConfig) generateListenerConfig(args *BootstrapTplArgs, bindAdd
 	args.StaticListenersJSON += listenerJSON
 	return nil
 }
+
+func containsSelfAdminCluster(clustersJSON string) (bool, error) {
+	clusterNames := []struct {
+		Name string
+	}{}
+
+	// StaticClustersJSON is defined as a comma-separated list of clusters, so we
+	// need to wrap it in JSON array brackets
+	err := json.Unmarshal([]byte("["+clustersJSON+"]"), &clusterNames)
+	if err != nil {
+		return false, fmt.Errorf("failed to parse static clusters: %s", err)
+	}
+
+	for _, cluster := range clusterNames {
+		if cluster.Name == selfAdminName {
+			return true, nil
+		}
+	}
+
+	return false, nil
+}
diff --git a/command/connect/envoy/bootstrap_config_test.go b/command/connect/envoy/bootstrap_config_test.go
index 38e70f1048..d6d33d13f3 100644
--- a/command/connect/envoy/bootstrap_config_test.go
+++ b/command/connect/envoy/bootstrap_config_test.go
@@ -598,6 +598,31 @@ func TestBootstrapConfig_ConfigureArgs(t *testing.T) {
 			},
 			wantErr: false,
 		},
+		{
+			name: "ready-bind-addr-and-prometheus-and-stats",
+			input: BootstrapConfig{
+				ReadyBindAddr:      "0.0.0.0:4444",
+				PrometheusBindAddr: "0.0.0.0:9000",
+				StatsBindAddr:      "0.0.0.0:9000",
+			},
+			baseArgs: BootstrapTplArgs{
+				AdminBindAddress: "127.0.0.1",
+				AdminBindPort:    "19000",
+			},
+			wantArgs: BootstrapTplArgs{
+				AdminBindAddress: "127.0.0.1",
+				AdminBindPort:    "19000",
+				// Should add a static cluster for the self-proxy to admin
+				StaticClustersJSON: expectedSelfAdminCluster,
+				// Should add a static http listener too
+				StaticListenersJSON: strings.Join(
+					[]string{expectedPromListener, expectedStatsListener, expectedReadyListener},
+					", ",
+				),
+				StatsConfigJSON: defaultStatsConfigJSON,
+			},
+			wantErr: false,
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {