Repository: kumahq/kuma
Pull request #1489 (2021-01-28 17:30:36)

fix(dns) Fix mesh outbound leak in VIPs
Outbounds for all available services in an ingress are added to all dataplanes
This shouldn't be necessary and adds unnecessary endpoints to dataplanes
This patch restricts outbounds to only the ones in the same mesh


lahabana: Also while looking at this I noticed there are not checks to ensure Vips outbound are not added to Ingresses.
It just happens to not be added because Ingresses usually are not setup with Transparent proxying.
Shouldn't we add a check for this too?
nickolaev: > Also while looking at this I noticed there are not checks to ensure Vips outbound are not added to Ingresses.
> It just happens to not be added because Ingresses usually are not setup with Transparent proxying.
> Shouldn't we add a check for this too?

Makes sense, please go ahead with a PR. Thanks!

subnetmarco: @lahabana thank you for the contribution
lahabana: Hmm looks like the failure is a timing issue. Shouldn't these assertions use `Eventually` as services might be starting up?
lahabana: @lobkovilya PTAL I've updated with your latest comments. Thanks!
lahabana: Thanks for the thorough review @lobkovilya 
lobkovilya: @Mergifyio update
mergify[bot]: **Command `update`: success**
> **Branch has been successfully updated**
> 

CLAassistant: [![CLA assistant check](https://cla-assistant.io/pull/badge/not_signed)](https://cla-assistant.io/kumahq/kuma?pullRequest=1489) <br/>Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our [Contributor License Agreement](https://cla-assistant.io/kumahq/kuma?pullRequest=1489) before we can accept your contribution.<br/>**1** out of **2** committers have signed the CLA.<br/><br/>:white_check_mark: lahabana<br/>:x: mergify[bot]<br/><sub>You have signed the CLA already but the status is still pending? Let us [recheck](https://cla-assistant.io/check/kumahq/kuma?pullRequest=1489) it.</sub>

Patch:
diff --git a/pkg/dns/outbound.go b/pkg/dns/outbound.go
index e244e5f33..1da742288 100644
--- a/pkg/dns/outbound.go
+++ b/pkg/dns/outbound.go
@@ -3,6 +3,8 @@ package dns
 import (
 	"sort"
 
+	"github.com/kumahq/kuma/pkg/core/resources/model"
+
 	"github.com/pkg/errors"
 
 	"github.com/kumahq/kuma/pkg/dns/vips"
@@ -14,7 +16,7 @@ import (
 const VIPListenPort = uint32(80)
 
 func VIPOutbounds(
-	name string,
+	resourceKey model.ResourceKey,
 	dataplanes []*core_mesh.DataplaneResource,
 	vips vips.List,
 	externalServices []*core_mesh.ExternalServiceResource,
@@ -22,18 +24,21 @@ func VIPOutbounds(
 	serviceVIPMap := map[string]string{}
 	services := []string{}
 	for _, dataplane := range dataplanes {
-		if dataplane.Meta.GetName() == name {
+		if resourceKey == model.MetaToResourceKey(dataplane.Meta) {
 			continue
 		}
 
 		if dataplane.Spec.IsIngress() {
 			for _, service := range dataplane.Spec.Networking.Ingress.AvailableServices {
-				inService := service.Tags[mesh_proto.ServiceTag]
-				if _, found := serviceVIPMap[inService]; !found {
-					vip, err := ForwardLookup(vips, inService)
-					if err == nil {
-						serviceVIPMap[inService] = vip
-						services = append(services, inService)
+				if service.Mesh == resourceKey.Mesh {
+					// Only add outbounds for services in the same mesh
+					inService := service.Tags[mesh_proto.ServiceTag]
+					if _, found := serviceVIPMap[inService]; !found {
+						vip, err := ForwardLookup(vips, inService)
+						if err == nil {
+							serviceVIPMap[inService] = vip
+							services = append(services, inService)
+						}
 					}
 				}
 			}
diff --git a/pkg/plugins/runtime/k8s/controllers/outbound_converter.go b/pkg/plugins/runtime/k8s/controllers/outbound_converter.go
index 55d096ef8..8dd069d73 100644
--- a/pkg/plugins/runtime/k8s/controllers/outbound_converter.go
+++ b/pkg/plugins/runtime/k8s/controllers/outbound_converter.go
@@ -5,6 +5,8 @@ import (
 	"strconv"
 	"strings"
 
+	"github.com/kumahq/kuma/pkg/core/resources/model"
+
 	core_mesh "github.com/kumahq/kuma/pkg/core/resources/apis/mesh"
 	"github.com/kumahq/kuma/pkg/dns"
 	"github.com/kumahq/kuma/pkg/dns/vips"
@@ -83,7 +85,11 @@ func (p *PodConverter) OutboundInterfacesFor(
 		}
 	}
 
-	outbounds = append(outbounds, dns.VIPOutbounds(pod.Name, dataplanes, vips, externalServicesRes)...)
+	resourceKey := model.ResourceKey{
+		Mesh: MeshFor(pod),
+		Name: pod.Name,
+	}
+	outbounds = append(outbounds, dns.VIPOutbounds(resourceKey, dataplanes, vips, externalServicesRes)...)
 	return outbounds, nil
 }
 
diff --git a/pkg/plugins/runtime/universal/outbound/outbound.go b/pkg/plugins/runtime/universal/outbound/outbound.go
index 76f137641..94fe5e3aa 100644
--- a/pkg/plugins/runtime/universal/outbound/outbound.go
+++ b/pkg/plugins/runtime/universal/outbound/outbound.go
@@ -4,6 +4,8 @@ import (
 	"context"
 	"time"
 
+	"github.com/kumahq/kuma/pkg/core/resources/model"
+
 	"github.com/kumahq/kuma/pkg/core"
 	"github.com/kumahq/kuma/pkg/dns/resolver"
 
@@ -59,6 +61,18 @@ func (v *VIPOutboundsReconciler) Start(stop <-chan struct{}) error {
 }
 
 func (v *VIPOutboundsReconciler) UpdateVIPOutbounds(ctx context.Context) error {
+	// First get all ingresses
+	var ingresses []*mesh.DataplaneResource
+	dpList := &mesh.DataplaneResourceList{}
+	if err := v.rorm.List(ctx, dpList); err != nil {
+		return err
+	}
+	for _, dp := range dpList.Items {
+		if dp.Spec.IsIngress() {
+			ingresses = append(ingresses, dp)
+		}
+	}
+	// Then add outbounds to each Dataplane
 	meshes := &mesh.MeshResourceList{}
 	if err := v.rorm.List(ctx, meshes); err != nil {
 		return err
@@ -73,11 +87,16 @@ func (v *VIPOutboundsReconciler) UpdateVIPOutbounds(ctx context.Context) error {
 			return err
 		}
 		dpsUpdated := 0
+
+		allDps := make([]*mesh.DataplaneResource, len(ingresses)+len(dpList.Items))
+		copy(allDps[:len(ingresses)], ingresses)
+		copy(allDps[len(ingresses):], dpList.Items)
+
 		for _, dp := range dpList.Items {
 			if dp.Spec.Networking.GetTransparentProxying() == nil {
 				continue
 			}
-			newOutbounds := dns.VIPOutbounds(dp.Meta.GetName(), dpList.Items, v.resolver.GetVIPs(), externalServices.Items)
+			newOutbounds := dns.VIPOutbounds(model.MetaToResourceKey(dp.Meta), allDps, v.resolver.GetVIPs(), externalServices.Items)
 
 			if outboundsEqual(newOutbounds, dp.Spec.Networking.Outbound) {
 				continue


Test patch:
diff --git a/pkg/dns/outbound_test.go b/pkg/dns/outbound_test.go
index d1b9c9875..8e1766a5a 100644
--- a/pkg/dns/outbound_test.go
+++ b/pkg/dns/outbound_test.go
@@ -4,6 +4,8 @@ import (
 	"fmt"
 	"strconv"
 
+	"github.com/kumahq/kuma/pkg/core/resources/model"
+
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
 
@@ -69,7 +71,7 @@ var _ = Describe("VIPOutbounds", func() {
 		}
 
 		// when
-		outbounds := dns.VIPOutbounds(dataplane.Meta.GetName(), dataplanes.Items, vipList, externalServices.Items)
+		outbounds := dns.VIPOutbounds(model.MetaToResourceKey(dataplane.Meta), dataplanes.Items, vipList, externalServices.Items)
 		// and
 		Expect(outbounds).To(HaveLen(4))
 		// and
@@ -77,4 +79,73 @@ var _ = Describe("VIPOutbounds", func() {
 		// and
 		Expect(outbounds[3].Port).To(Equal(dns.VIPListenPort))
 	})
+
+	It("shouldn't add outbounds from other meshes", func() {
+		dataplane := &core_mesh.DataplaneResource{
+			Meta: &test_model.ResourceMeta{
+				Name: "dp1",
+				Mesh: "default",
+			},
+			Spec: &mesh_proto.Dataplane{
+				Networking: &mesh_proto.Dataplane_Networking{
+					Address: "192.168.0.1",
+					Inbound: []*mesh_proto.Dataplane_Networking_Inbound{
+						{
+							Port: 8080,
+							Tags: map[string]string{
+								"kuma.io/service": "backend",
+							},
+						},
+					},
+				},
+			},
+		}
+
+		// given
+		vipList := vips.List{
+			"service-a": "240.0.0.1",
+			"service-b": "240.0.0.2",
+		}
+		services := []*mesh_proto.Dataplane_Networking_Ingress_AvailableService{
+			{
+				Mesh: "default",
+				Tags: map[string]string{
+					"kuma.io/service": "service-a",
+				},
+			},
+			{
+				Mesh: "other",
+				Tags: map[string]string{
+					"kuma.io/service": "service-b",
+				},
+			},
+		}
+		externalServices := &core_mesh.ExternalServiceResourceList{}
+		dataplanes := core_mesh.DataplaneResourceList{
+			Items: []*core_mesh.DataplaneResource{
+				{
+					Meta: &test_model.ResourceMeta{
+						Name: "dp-ingress",
+						Mesh: "default",
+					},
+					Spec: &mesh_proto.Dataplane{
+						Networking: &mesh_proto.Dataplane_Networking{
+							Ingress: &mesh_proto.Dataplane_Networking_Ingress{
+								AvailableServices: services,
+							},
+						},
+					},
+				},
+			},
+		}
+
+		// when
+		outbounds := dns.VIPOutbounds(model.MetaToResourceKey(dataplane.Meta), dataplanes.Items, vipList, externalServices.Items)
+		// and
+		Expect(outbounds).To(HaveLen(1))
+		// and
+		Expect(outbounds[0].GetTags()[mesh_proto.ServiceTag]).To(Equal("service-a"))
+		// and
+		Expect(outbounds[0].Port).To(Equal(dns.VIPListenPort))
+	})
 })
diff --git a/pkg/plugins/runtime/universal/outbound/outbound_suite_test.go b/pkg/plugins/runtime/universal/outbound/outbound_suite_test.go
new file mode 100644
index 000000000..6fc91e01d
--- /dev/null
+++ b/pkg/plugins/runtime/universal/outbound/outbound_suite_test.go
@@ -0,0 +1,13 @@
+package outbound_test
+
+import (
+	"testing"
+
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+)
+
+func TestOutbound(t *testing.T) {
+	RegisterFailHandler(Fail)
+	RunSpecs(t, "Universal outbound test")
+}
diff --git a/pkg/plugins/runtime/universal/outbound/outbound_test.go b/pkg/plugins/runtime/universal/outbound/outbound_test.go
index d9d1378c7..a8500daaf 100644
--- a/pkg/plugins/runtime/universal/outbound/outbound_test.go
+++ b/pkg/plugins/runtime/universal/outbound/outbound_test.go
@@ -134,6 +134,124 @@ var _ = Describe("UpdateOutbound", func() {
 			Expect(dp1.Spec.Networking.Outbound).To(HaveLen(0))
 		})
 
+		It("should update dataplane outbounds when new service in the same mesh is added to an ingress", func() {
+			// when
+			err := rm.Create(context.Background(), &mesh.DataplaneResource{
+				Spec: &mesh_proto.Dataplane{
+					Networking: &mesh_proto.Dataplane_Networking{
+						Address: "127.0.0.1",
+						Ingress: &mesh_proto.Dataplane_Networking_Ingress{
+							AvailableServices: []*mesh_proto.Dataplane_Networking_Ingress_AvailableService{
+								{
+									Mesh: "default",
+									Tags: map[string]string{
+										mesh_proto.ServiceTag: "service-2",
+									},
+								},
+							},
+						},
+						Inbound: []*mesh_proto.Dataplane_Networking_Inbound{
+							{
+								Port: 8081,
+								Tags: map[string]string{
+									"kuma.io/service": "ingress",
+								},
+							},
+						},
+					},
+				},
+			}, store.CreateByKey("dp-2", "default"))
+			Expect(err).ToNot(HaveOccurred())
+			err = vipOutboundsReconciler.UpdateVIPOutbounds(context.Background())
+			Expect(err).ToNot(HaveOccurred())
+			// then
+			dp1 := mesh.NewDataplaneResource()
+			err = rm.Get(context.Background(), dp1, store.GetByKey("dp-1", "default"))
+			Expect(err).ToNot(HaveOccurred())
+			Expect(dp1.Spec.Networking.Outbound).To(HaveLen(1))
+			Expect(dp1.Spec.Networking.Outbound[0].Tags["kuma.io/service"]).To(Equal("service-2"))
+			Expect(dp1.Spec.Networking.Outbound[0].Address).To(Equal("240.0.0.2"))
+		})
+
+		It("shouldn't update dataplane outbounds when new service in a different mesh is added to an ingress", func() {
+			// when
+			err := rm.Create(context.Background(), &mesh.DataplaneResource{
+				Spec: &mesh_proto.Dataplane{
+					Networking: &mesh_proto.Dataplane_Networking{
+						Address: "127.0.0.1",
+						Ingress: &mesh_proto.Dataplane_Networking_Ingress{
+							AvailableServices: []*mesh_proto.Dataplane_Networking_Ingress_AvailableService{
+								{
+									Mesh: "other-mesh",
+									Tags: map[string]string{
+										mesh_proto.ServiceTag: "service-2",
+									},
+								},
+							},
+						},
+						Inbound: []*mesh_proto.Dataplane_Networking_Inbound{
+							{
+								Port: 8081,
+								Tags: map[string]string{
+									"kuma.io/service": "ingress",
+								},
+							},
+						},
+					},
+				},
+			}, store.CreateByKey("dp-2", "default"))
+			Expect(err).ToNot(HaveOccurred())
+			err = vipOutboundsReconciler.UpdateVIPOutbounds(context.Background())
+			Expect(err).ToNot(HaveOccurred())
+			// then
+			dp1 := mesh.NewDataplaneResource()
+			err = rm.Get(context.Background(), dp1, store.GetByKey("dp-1", "default"))
+			Expect(err).ToNot(HaveOccurred())
+			Expect(dp1.Spec.Networking.Outbound).To(HaveLen(0))
+		})
+
+		It("should update dataplane outbounds when new service is added to an ingress in a different mesh", func() {
+			// when
+			err := rm.Create(context.Background(), mesh.NewMeshResource(), store.CreateByKey("another-mesh", model.NoMesh))
+			Expect(err).ToNot(HaveOccurred())
+			// and
+			err = rm.Create(context.Background(), &mesh.DataplaneResource{
+				Spec: &mesh_proto.Dataplane{
+					Networking: &mesh_proto.Dataplane_Networking{
+						Address: "127.0.0.1",
+						Ingress: &mesh_proto.Dataplane_Networking_Ingress{
+							AvailableServices: []*mesh_proto.Dataplane_Networking_Ingress_AvailableService{
+								{
+									Mesh: "default",
+									Tags: map[string]string{
+										mesh_proto.ServiceTag: "service-2",
+									},
+								},
+							},
+						},
+						Inbound: []*mesh_proto.Dataplane_Networking_Inbound{
+							{
+								Port: 8081,
+								Tags: map[string]string{
+									"kuma.io/service": "ingress",
+								},
+							},
+						},
+					},
+				},
+			}, store.CreateByKey("dp-2", "another-mesh"))
+			Expect(err).ToNot(HaveOccurred())
+			err = vipOutboundsReconciler.UpdateVIPOutbounds(context.Background())
+			Expect(err).ToNot(HaveOccurred())
+			// then
+			dp1 := mesh.NewDataplaneResource()
+			err = rm.Get(context.Background(), dp1, store.GetByKey("dp-1", "default"))
+			Expect(err).ToNot(HaveOccurred())
+			Expect(dp1.Spec.Networking.Outbound).To(HaveLen(1))
+			Expect(dp1.Spec.Networking.Outbound[0].Tags["kuma.io/service"]).To(Equal("service-2"))
+			Expect(dp1.Spec.Networking.Outbound[0].Address).To(Equal("240.0.0.2"))
+		})
+
 		Context("outbounds already updated", func() {
 			BeforeEach(func() {
 				err := rm.Create(context.Background(), &mesh.DataplaneResource{