fix(kuma-cp): specifying IPv6 Envoy Admin address breaks readiness/li…

…veness probes (backport of #7909) (#7926) * fix(kuma-cp): specifying IPv6 Envoy Admin address breaks readiness/liveness probes (#7909) Put a specified admin address to proxy metadata and use it when generating an admin cluster. * test(unit): fix flaky "should return error when admin address is not allowed" test (#7933) fix(kuma-cp): use SortedKeys function in error msg * make SortedKeys generic * chore(kuma-cp): improve error message for invalid admin addresses (#7934) * update golden files Signed-off-by: Ilya Lobkov <[email protected]> Signed-off-by: Mike Beaumont <[email protected]> Co-authored-by: Ilya Lobkov <[email protected]> Co-authored-by: Mike Beaumont <[email protected]>
kumahq · Oct 5, 2023 · 4aef4e5 · 4aef4e5
1 parent d173e7b
commit 4aef4e5
Show file tree

Hide file tree

Showing 21 changed files with 531 additions and 13 deletions.
diff --git a/pkg/core/xds/metadata.go b/pkg/core/xds/metadata.go
@@ -20,6 +20,7 @@ var metadataLog = core.Log.WithName("xds-server").WithName("metadata-tracker")
 const (
 	// Supported Envoy node metadata fields.
 	fieldDataplaneAdminPort         = "dataplane.admin.port"
+	fieldDataplaneAdminAddress      = "dataplane.admin.address"
 	fieldDataplaneDNSPort           = "dataplane.dns.port"
 	fieldDataplaneDNSEmptyPort      = "dataplane.dns.empty.port"
 	fieldDataplaneDataplaneResource = "dataplane.resource"
@@ -47,6 +48,7 @@ const (
 type DataplaneMetadata struct {
 	Resource        model.Resource
 	AdminPort       uint32
+	AdminAddress    string
 	DNSPort         uint32
 	EmptyDNSPort    uint32
 	DynamicMetadata map[string]string
@@ -105,6 +107,13 @@ func (m *DataplaneMetadata) GetAdminPort() uint32 {
 	return m.AdminPort
 }
 
+func (m *DataplaneMetadata) GetAdminAddress() string {
+	if m == nil {
+		return ""
+	}
+	return m.AdminAddress
+}
+
 func (m *DataplaneMetadata) GetDNSPort() uint32 {
 	if m == nil {
 		return 0
@@ -145,6 +154,7 @@ func DataplaneMetadataFromXdsMetadata(xdsMetadata *structpb.Struct) *DataplaneMe
 		metadata.ProxyType = mesh_proto.ProxyType(field.GetStringValue())
 	}
 	metadata.AdminPort = uint32Metadata(xdsMetadata, fieldDataplaneAdminPort)
+	metadata.AdminAddress = xdsMetadata.Fields[fieldDataplaneAdminAddress].GetStringValue()
 	metadata.DNSPort = uint32Metadata(xdsMetadata, fieldDataplaneDNSPort)
 	metadata.EmptyDNSPort = uint32Metadata(xdsMetadata, fieldDataplaneDNSEmptyPort)
 	if value := xdsMetadata.Fields[fieldDataplaneDataplaneResource]; value != nil {

diff --git a/pkg/util/maps/sorted_keys.go b/pkg/util/maps/sorted_keys.go
@@ -1,12 +1,13 @@
 package maps
 
-import "sort"
+import (
+	"golang.org/x/exp/constraints"
+	"golang.org/x/exp/maps"
+	"golang.org/x/exp/slices"
+)
 
-func SortedKeys(m map[string]string) []string {
-	var keys []string
-	for key := range m {
-		keys = append(keys, key)
-	}
-	sort.Strings(keys)
+func SortedKeys[M ~map[K]V, K constraints.Ordered, V any](m M) []K {
+	keys := maps.Keys(m)
+	slices.Sort(keys)
 	return keys
 }
diff --git a/pkg/xds/bootstrap/template_v3.go b/pkg/xds/bootstrap/template_v3.go
@@ -293,6 +293,7 @@ func genConfig(parameters configParameters, proxyConfig xds.Proxy, useTokenPath
 	}
 	if parameters.AdminPort != 0 {
 		res.Node.Metadata.Fields["dataplane.admin.port"] = util_proto.MustNewValueForStruct(strconv.Itoa(int(parameters.AdminPort)))
+		res.Node.Metadata.Fields["dataplane.admin.address"] = util_proto.MustNewValueForStruct(parameters.AdminAddress)
 		res.Admin = &envoy_bootstrap_v3.Admin{
 			Address: &envoy_core_v3.Address{
 				Address: &envoy_core_v3.Address_SocketAddress{

diff --git a/pkg/xds/bootstrap/testdata/bootstrap.overridden.golden.yaml b/pkg/xds/bootstrap/testdata/bootstrap.overridden.golden.yaml
@@ -46,6 +46,7 @@ node:
   cluster: backend
   id: default.dp-1.default
   metadata:
+    dataplane.admin.address: 127.0.0.1
     dataplane.admin.port: "1234"
     dataplane.proxyType: dataplane
     features: []

diff --git a/pkg/xds/bootstrap/testdata/generator.custom-config-minimal-request.golden.yaml b/pkg/xds/bootstrap/testdata/generator.custom-config-minimal-request.golden.yaml
@@ -40,6 +40,7 @@ node:
   cluster: backend
   id: mesh.name.namespace
   metadata:
+    dataplane.admin.address: 192.168.0.1
     dataplane.admin.port: "9902"
     dataplane.proxyType: dataplane
     features: []

diff --git a/pkg/xds/bootstrap/testdata/generator.custom-config.golden.yaml b/pkg/xds/bootstrap/testdata/generator.custom-config.golden.yaml
@@ -46,6 +46,7 @@ node:
   cluster: backend
   id: mesh.name.namespace
   metadata:
+    dataplane.admin.address: 192.168.0.1
     dataplane.admin.port: "1234"
     dataplane.proxyType: dataplane
     dataplane.resource: |2-

diff --git a/pkg/xds/bootstrap/testdata/generator.default-config-token-path.golden.yaml b/pkg/xds/bootstrap/testdata/generator.default-config-token-path.golden.yaml
@@ -66,6 +66,7 @@ node:
   cluster: backend
   id: mesh.name.namespace
   metadata:
+    dataplane.admin.address: 127.0.0.1
     dataplane.admin.port: "1234"
     dataplane.dns.empty.port: "53002"
     dataplane.dns.port: "53001"

diff --git a/pkg/xds/bootstrap/testdata/generator.default-config.golden.yaml b/pkg/xds/bootstrap/testdata/generator.default-config.golden.yaml
@@ -46,6 +46,7 @@ node:
   cluster: backend
   id: mesh.name.namespace
   metadata:
+    dataplane.admin.address: 127.0.0.1
     dataplane.admin.port: "1234"
     dataplane.dns.empty.port: "53002"
     dataplane.dns.port: "53001"

diff --git a/pkg/xds/bootstrap/testdata/generator.default-config.kubernetes.golden.yaml b/pkg/xds/bootstrap/testdata/generator.default-config.kubernetes.golden.yaml
@@ -36,6 +36,7 @@ node:
   cluster: backend
   id: mesh.name.namespace
   metadata:
+    dataplane.admin.address: 127.0.0.1
     dataplane.admin.port: "1234"
     dataplane.proxyType: dataplane
     features: []

diff --git a/pkg/xds/bootstrap/testdata/generator.default-config.kubernetes.ipv6.golden.yaml b/pkg/xds/bootstrap/testdata/generator.default-config.kubernetes.ipv6.golden.yaml
@@ -36,6 +36,7 @@ node:
   cluster: backend
   id: mesh.name.namespace
   metadata:
+    dataplane.admin.address: 127.0.0.1
     dataplane.admin.port: "1234"
     dataplane.proxyType: dataplane
     features: []

diff --git a/pkg/xds/bootstrap/testdata/generator.metrics-config.kubernetes.golden.yaml b/pkg/xds/bootstrap/testdata/generator.metrics-config.kubernetes.golden.yaml
@@ -36,6 +36,7 @@ node:
   cluster: backend
   id: mesh.name.namespace
   metadata:
+    dataplane.admin.address: 127.0.0.1
     dataplane.admin.port: "1234"
     dataplane.proxyType: dataplane
     features: []

diff --git a/pkg/xds/generator/admin_proxy_generator.go b/pkg/xds/generator/admin_proxy_generator.go
@@ -2,8 +2,14 @@ package generator
 
 import (
 	"context"
+	"fmt"
+	"strings"
+
+	"github.com/asaskevich/govalidator"
+	"github.com/pkg/errors"
 
 	core_xds "github.com/kumahq/kuma/pkg/core/xds"
+	util_maps "github.com/kumahq/kuma/pkg/util/maps"
 	xds_context "github.com/kumahq/kuma/pkg/xds/context"
 	envoy_common "github.com/kumahq/kuma/pkg/xds/envoy"
 	envoy_clusters "github.com/kumahq/kuma/pkg/xds/envoy/clusters"
@@ -33,6 +39,14 @@ var staticTlsEndpointPaths = []*envoy_common.StaticEndpointPath{
 type AdminProxyGenerator struct {
 }
 
+var adminAddressAllowedValues = map[string]struct{}{
+	"127.0.0.1": {},
+	"0.0.0.0":   {},
+	"::1":       {},
+	"::":        {},
+	"":          {},
+}
+
 func (g AdminProxyGenerator) Generate(ctx context.Context, xdsCtx xds_context.Context, proxy *core_xds.Proxy) (*core_xds.ResourceSet, error) {
 	if proxy.Metadata.GetAdminPort() == 0 {
 		// It's not possible to export Admin endpoints if Envoy Admin API has not been enabled on that dataplane.
@@ -42,14 +56,30 @@ func (g AdminProxyGenerator) Generate(ctx context.Context, xdsCtx xds_context.Co
 	adminPort := proxy.Metadata.GetAdminPort()
 	// We assume that Admin API must be available on a loopback interface (while users
 	// can override the default value `127.0.0.1` in the Bootstrap Server section of `kuma-cp` config,
-	// the only reasonable alternative is `0.0.0.0`).
+	// the only reasonable alternatives are `::1`, `0.0.0.0` or `::`).
 	// In contrast to `AdminPort`, we shouldn't trust `AdminAddress` from the Envoy node metadata
 	// since it would allow a malicious user to manipulate that value and use Prometheus endpoint
 	// as a gateway to another host.
-	adminAddress := "127.0.0.1"
 	envoyAdminClusterName := envoy_names.GetEnvoyAdminClusterName()
+	adminAddress := proxy.Metadata.GetAdminAddress()
+	if _, ok := adminAddressAllowedValues[adminAddress]; !ok {
+		var allowedAddresses []string
+		for _, address := range util_maps.SortedKeys(adminAddressAllowedValues) {
+			allowedAddresses = append(allowedAddresses, fmt.Sprintf(`"%s"`, address))
+		}
+		return nil, errors.Errorf("envoy admin cluster is not allowed to have addresses other than %s", strings.Join(allowedAddresses, ", "))
+	}
+	switch adminAddress {
+	case "", "0.0.0.0":
+		adminAddress = "127.0.0.1"
+	case "::":
+		adminAddress = "::1"
+	}
 	cluster, err := envoy_clusters.NewClusterBuilder(proxy.APIVersion).
-		Configure(envoy_clusters.ProvidedEndpointCluster(envoyAdminClusterName, false, core_xds.Endpoint{Target: adminAddress, Port: adminPort})).
+		Configure(envoy_clusters.ProvidedEndpointCluster(
+			envoyAdminClusterName,
+			govalidator.IsIPv6(adminAddress),
+			core_xds.Endpoint{Target: adminAddress, Port: adminPort})).
 		Configure(envoy_clusters.DefaultTimeout()).
 		Build()
 	if err != nil {
@@ -63,7 +93,7 @@ func (g AdminProxyGenerator) Generate(ctx context.Context, xdsCtx xds_context.Co
 	}
 
 	// We bind admin to 127.0.0.1 by default, creating another listener with same address and port will result in error.
-	if g.getAddress(proxy) != "127.0.0.1" {
+	if g.getAddress(proxy) != adminAddress {
 		filterChains := []envoy_listeners.ListenerBuilderOpt{
 			envoy_listeners.FilterChain(envoy_listeners.NewFilterChainBuilder(proxy.APIVersion).
 				Configure(envoy_listeners.StaticEndpoints(envoy_names.GetAdminListenerName(), staticEndpointPaths)),

diff --git a/pkg/xds/generator/admin_proxy_generator_test.go b/pkg/xds/generator/admin_proxy_generator_test.go
@@ -25,6 +25,7 @@ var _ = Describe("AdminProxyGenerator", func() {
 	type testCase struct {
 		dataplaneFile string
 		expected      string
+		adminAddress  string
 	}
 
 	DescribeTable("should generate envoy config",
@@ -49,7 +50,8 @@ var _ = Describe("AdminProxyGenerator", func() {
 
 			proxy := &xds.Proxy{
 				Metadata: &xds.DataplaneMetadata{
-					AdminPort: 9901,
+					AdminPort:    9901,
+					AdminAddress: given.adminAddress,
 				},
 				EnvoyAdminMTLSCerts: xds.ServerSideMTLSCerts{
 					CaPEM: []byte("caPEM"),
@@ -76,9 +78,63 @@ var _ = Describe("AdminProxyGenerator", func() {
 			// and output matches golden files
 			Expect(actual).To(MatchGoldenYAML(filepath.Join("testdata", "admin", given.expected)))
 		},
-		Entry("should generate admin resources", testCase{
+		Entry("should generate admin resources, empty admin address", testCase{
 			dataplaneFile: "01.dataplane.input.yaml",
 			expected:      "01.envoy-config.golden.yaml",
+			adminAddress:  "",
+		}),
+		Entry("should generate admin resources, IPv4 loopback", testCase{
+			dataplaneFile: "02.dataplane.input.yaml",
+			expected:      "02.envoy-config.golden.yaml",
+			adminAddress:  "127.0.0.1",
+		}),
+		Entry("should generate admin resources, IPv6 loopback", testCase{
+			dataplaneFile: "03.dataplane.input.yaml",
+			expected:      "03.envoy-config.golden.yaml",
+			adminAddress:  "::1",
+		}),
+		Entry("should generate admin resources, unspecified IPv4", testCase{
+			dataplaneFile: "04.dataplane.input.yaml",
+			expected:      "04.envoy-config.golden.yaml",
+			adminAddress:  "0.0.0.0",
+		}),
+		Entry("should generate admin resources, unspecified IPv6", testCase{
+			dataplaneFile: "05.dataplane.input.yaml",
+			expected:      "05.envoy-config.golden.yaml",
+			adminAddress:  "::",
 		}),
 	)
+
+	It("should return error when admin address is not allowed", func() {
+		ctx := xds_context.Context{
+			Mesh: xds_context.MeshContext{
+				Resource: &core_mesh.MeshResource{
+					Meta: &test_model.ResourceMeta{
+						Name: "default",
+					},
+				},
+			},
+		}
+
+		proxy := &xds.Proxy{
+			Metadata: &xds.DataplaneMetadata{
+				AdminPort:    9901,
+				AdminAddress: "192.168.0.1", // it's not allowed to use such address
+			},
+			EnvoyAdminMTLSCerts: xds.ServerSideMTLSCerts{
+				CaPEM: []byte("caPEM"),
+				ServerPair: tls.KeyPair{
+					CertPEM: []byte("certPEM"),
+					KeyPEM:  []byte("keyPEM"),
+				},
+			},
+			Dataplane:  core_mesh.NewDataplaneResource(),
+			APIVersion: envoy_common.APIV3,
+		}
+
+		// when
+		_, err := generator.Generate(context.Background(), ctx, proxy)
+		Expect(err).To(HaveOccurred())
+		Expect(err.Error()).To(Equal(`envoy admin cluster is not allowed to have addresses other than "", "0.0.0.0", "127.0.0.1", "::", "::1"`))
+	})
 })
diff --git a/pkg/xds/generator/testdata/admin/02.dataplane.input.yaml b/pkg/xds/generator/testdata/admin/02.dataplane.input.yaml
@@ -0,0 +1,9 @@
+type: Dataplane
+name: web-1
+mesh: default
+networking:
+  address: 192.168.0.1
+  inbound:
+    - port: 1234
+      tags:
+        kuma.io/service: web
diff --git a/pkg/xds/generator/testdata/admin/02.envoy-config.golden.yaml b/pkg/xds/generator/testdata/admin/02.envoy-config.golden.yaml
@@ -0,0 +1,94 @@
+resources:
+- name: kuma:envoy:admin
+  resource:
+    '@type': type.googleapis.com/envoy.config.cluster.v3.Cluster
+    altStatName: kuma_envoy_admin
+    connectTimeout: 10s
+    loadAssignment:
+      clusterName: kuma:envoy:admin
+      endpoints:
+      - lbEndpoints:
+        - endpoint:
+            address:
+              socketAddress:
+                address: 127.0.0.1
+                portValue: 9901
+    name: kuma:envoy:admin
+    type: STATIC
+- name: kuma:envoy:admin
+  resource:
+    '@type': type.googleapis.com/envoy.config.listener.v3.Listener
+    address:
+      socketAddress:
+        address: 192.168.0.1
+        portValue: 9901
+    enableReusePort: false
+    filterChains:
+    - filters:
+      - name: envoy.filters.network.http_connection_manager
+        typedConfig:
+          '@type': type.googleapis.com/envoy.extensions.filters.network.http_connection_manager.v3.HttpConnectionManager
+          httpFilters:
+          - name: envoy.filters.http.router
+            typedConfig:
+              '@type': type.googleapis.com/envoy.extensions.filters.http.router.v3.Router
+          routeConfig:
+            validateClusters: false
+            virtualHosts:
+            - domains:
+              - '*'
+              name: kuma:envoy:admin
+              routes:
+              - match:
+                  prefix: /ready
+                route:
+                  cluster: kuma:envoy:admin
+                  prefixRewrite: /ready
+          statPrefix: kuma_envoy_admin
+    - filterChainMatch:
+        transportProtocol: tls
+      filters:
+      - name: envoy.filters.network.http_connection_manager
+        typedConfig:
+          '@type': type.googleapis.com/envoy.extensions.filters.network.http_connection_manager.v3.HttpConnectionManager
+          httpFilters:
+          - name: envoy.filters.http.router
+            typedConfig:
+              '@type': type.googleapis.com/envoy.extensions.filters.http.router.v3.Router
+          routeConfig:
+            validateClusters: false
+            virtualHosts:
+            - domains:
+              - '*'
+              name: kuma:envoy:admin
+              routes:
+              - match:
+                  prefix: /
+                route:
+                  cluster: kuma:envoy:admin
+                  prefixRewrite: /
+          statPrefix: kuma_envoy_admin
+      transportSocket:
+        name: envoy.transport_sockets.tls
+        typedConfig:
+          '@type': type.googleapis.com/envoy.extensions.transport_sockets.tls.v3.DownstreamTlsContext
+          commonTlsContext:
+            tlsCertificates:
+            - certificateChain:
+                inlineBytes: Y2VydFBFTQ==
+              privateKey:
+                inlineBytes: a2V5UEVN
+            validationContext:
+              matchTypedSubjectAltNames:
+              - matcher:
+                  exact: kuma-cp
+                sanType: DNS
+              trustedCa:
+                inlineBytes: Y2FQRU0=
+          requireClientCertificate: true
+    listenerFilters:
+    - name: envoy.filters.listener.tls_inspector
+      typedConfig:
+        '@type': type.googleapis.com/envoy.extensions.filters.listener.tls_inspector.v3.TlsInspector
+    name: kuma:envoy:admin
+    trafficDirection: INBOUND