Fix mirroring all services when remote selector is empty (linkerd#11344)

* Fix mirroring all services when remote selector is empty The multicluster link supports two selectors: one for normal (endpoint-based) mirrors, and one for remote-discovery where only service imports are created. When the remote-discovery selector is empty (i.e. an empty object '{}'), then all services in a cluster will be mirrored. The created imports also have an underlying Endpoints object created. There are two distinct checks that decide whether a service import should be created: `isExported` and `isRemote`. When a selector is empty, the checks are shortcircuted and return early. The former check (`isExported`) additionally also checks if a service is remote, without checking if the corresponding selector is empty. This allows services to slip through since an empty selector encompasses everything. The change fixes the issue by removing any remote discovery checks from `isExported`. Where necessary, we add another call to `isRemote`. Fixes linkerd#11309 Signed-off-by: Matei David <[email protected]> * Add an integration test for empty selectors Signed-off-by: Matei David <[email protected]> * Differentiate test service ports Signed-off-by: Matei David <[email protected]> * @alpeb's feedback Signed-off-by: Matei David <[email protected]> --------- Signed-off-by: Matei David <[email protected]> Signed-off-by: Adam Shaw <[email protected]>
adamshawvipps · Sep 18, 2023 · bf178da · bf178da
1 parent d614821
commit bf178da
Show file tree

Hide file tree

Showing 3 changed files with 124 additions and 7 deletions.
diff --git a/multicluster/service-mirror/cluster_watcher.go b/multicluster/service-mirror/cluster_watcher.go
@@ -739,7 +739,7 @@ func (rcsw *RemoteClusterServiceWatcher) getMirrorServices() (*corev1.ServiceLis
 }
 
 func (rcsw *RemoteClusterServiceWatcher) handleOnDelete(service *corev1.Service) {
-	if rcsw.isExported(service.Labels) {
+	if rcsw.isExported(service.Labels) || rcsw.isRemoteDiscovery(service.Labels) {
 		rcsw.eventsQueue.Add(&RemoteServiceDeleted{
 			Name:      service.Name,
 			Namespace: service.Namespace,
@@ -1242,12 +1242,7 @@ func (rcsw *RemoteClusterServiceWatcher) isExported(l map[string]string) bool {
 		rcsw.log.Errorf("Invalid selector: %s", err)
 		return false
 	}
-	remoteDiscoverySelector, err := metav1.LabelSelectorAsSelector(&rcsw.link.RemoteDiscoverySelector)
-	if err != nil {
-		rcsw.log.Errorf("Invalid selector: %s", err)
-		return false
-	}
-	return selector.Matches(labels.Set(l)) || remoteDiscoverySelector.Matches(labels.Set(l))
+	return selector.Matches(labels.Set(l))
 }
 
 func (rcsw *RemoteClusterServiceWatcher) isRemoteDiscovery(l map[string]string) bool {

diff --git a/multicluster/service-mirror/cluster_watcher_mirroring_test.go b/multicluster/service-mirror/cluster_watcher_mirroring_test.go
@@ -641,6 +641,64 @@ func TestRemoteServiceUpdatedMirroring(t *testing.T) {
 	}
 }
 
+// TestEmptyRemoteSelectors asserts that empty selectors do not introduce side
+// effects, such as mirroring unexported services. An empty label selector
+// functions as a catch-all (i.e. matches everything), the cluster watcher must
+// uphold an invariant whereby empty selectors do not export everything by
+// default.
+func TestEmptyRemoteSelectors(t *testing.T) {
+	for _, tt := range []mirroringTestCase{
+		{
+			description: "empty remote discovery selector does not result in exports",
+			environment: createEnvWithSelector(defaultSelector, &metav1.LabelSelector{}),
+			expectedEventsInQueue: []interface{}{&RemoteServiceCreated{
+				service: remoteService("service-one", "ns1", "111", map[string]string{
+					consts.DefaultExportedServiceSelector: "true",
+				}, []corev1.ServicePort{
+					{
+						Name:     "default1",
+						Protocol: "TCP",
+						Port:     555,
+					},
+					{
+						Name:     "default2",
+						Protocol: "TCP",
+						Port:     666,
+					},
+				}),
+			},
+			},
+		},
+		{
+			description: "empty default selector does not result in exports",
+			environment: createEnvWithSelector(&metav1.LabelSelector{}, defaultRemoteDiscoverySelector),
+			expectedEventsInQueue: []interface{}{&RemoteServiceCreated{
+				service: remoteService("service-two", "ns1", "111", map[string]string{
+					consts.DefaultExportedServiceSelector: "remote-discovery",
+				}, []corev1.ServicePort{
+					{
+						Name:     "remote1",
+						Protocol: "TCP",
+						Port:     777,
+					},
+					{
+						Name:     "remote2",
+						Protocol: "TCP",
+						Port:     888,
+					},
+				}),
+			}},
+		},
+		{
+			description: "no selector in link does not result in exports",
+			environment: createEnvWithSelector(&metav1.LabelSelector{}, &metav1.LabelSelector{}),
+		},
+	} {
+		tc := tt
+		tc.run(t)
+	}
+}
+
 func TestRemoteEndpointsUpdatedMirroring(t *testing.T) {
 	for _, tt := range []mirroringTestCase{
 		{

diff --git a/multicluster/service-mirror/cluster_watcher_test_util.go b/multicluster/service-mirror/cluster_watcher_test_util.go
@@ -1222,3 +1222,67 @@ func endpointMirrorEndpointsAsYaml(name, namespace, hostname, gatewayIP, gateway
 
 	return string(bytes)
 }
+
+// createEmptySelectorEnv will create a test environment with two services. It
+// accepts a default and a remote discovery selector which it will use for the
+// link creation. This function is used to create environments that differ only
+// in the selector used in the link.
+func createEnvWithSelector(defaultSelector, remoteSelector *metav1.LabelSelector) *testEnvironment {
+	return &testEnvironment{
+		events: []interface{}{
+			&OnAddCalled{
+				svc: remoteService("service-one", "ns1", "111", map[string]string{
+					consts.DefaultExportedServiceSelector: "true",
+				}, []corev1.ServicePort{
+					{
+						Name:     "default1",
+						Protocol: "TCP",
+						Port:     555,
+					},
+					{
+						Name:     "default2",
+						Protocol: "TCP",
+						Port:     666,
+					},
+				}),
+			},
+			&OnAddCalled{
+				svc: remoteService("service-two", "ns1", "111", map[string]string{
+					consts.DefaultExportedServiceSelector: "remote-discovery",
+				}, []corev1.ServicePort{
+					{
+						Name:     "remote1",
+						Protocol: "TCP",
+						Port:     777,
+					},
+					{
+						Name:     "remote2",
+						Protocol: "TCP",
+						Port:     888,
+					},
+				}),
+			},
+		},
+		localResources: []string{
+			namespaceAsYaml("ns1"),
+		},
+		remoteResources: []string{
+			endpointsAsYaml("service-one", "ns1", "192.0.2.127", "gateway-identity", []corev1.EndpointPort{}),
+			endpointsAsYaml("service-two", "ns1", "192.0.3.127", "gateway-identity", []corev1.EndpointPort{}),
+		},
+		link: multicluster.Link{
+			TargetClusterName:   clusterName,
+			TargetClusterDomain: clusterDomain,
+			GatewayIdentity:     "",
+			GatewayAddress:      "",
+			GatewayPort:         0,
+			ProbeSpec: multicluster.ProbeSpec{
+				Path:   "",
+				Port:   0,
+				Period: time.Duration(0) * time.Second,
+			},
+			Selector:                *defaultSelector,
+			RemoteDiscoverySelector: *remoteSelector,
+		},
+	}
+}