endpoints: don't filter service endpoints in permissive mode

Currently, permissive mode bypasses EDS and uses Envoy's OriginalDestination cluster for service discovery. As a part of implementing support for TrafficSplit in permissive mode, permissive mode will also leverage EDS for endpoint discovery. The existing API leveraged by EDS to discover the endpoints of a service are not usable for permissive mode, so this change adds support for the same. This change also renames the function for clarity and removes an implicit comment. Required by openservicemesh#4052 and openservicemesh#2527. Signed-off-by: Shashank Ram <shashr2204@gmail.com>
shashankram · Sep 1, 2021 · 898e8ff · 898e8ff
1 parent 2d7f62b
commit 898e8ff
Show file tree

Hide file tree

Showing 5 changed files with 73 additions and 22 deletions.
diff --git a/pkg/catalog/endpoint.go b/pkg/catalog/endpoint.go
@@ -38,15 +38,24 @@ func (mc *MeshCatalog) GetResolvableServiceEndpoints(svc service.MeshService) ([
 	return endpoints, nil
 }
 
-// ListEndpointsForServiceIdentity returns a list of endpoints that belongs to an upstream service accounts
-// from the given downstream identity's perspective
-// Note: ServiceIdentity must be in the format "name.namespace" [https://github.com/openservicemesh/osm/issues/3188]
-func (mc *MeshCatalog) ListEndpointsForServiceIdentity(downstreamIdentity identity.ServiceIdentity, upstreamSvc service.MeshService) ([]endpoint.Endpoint, error) {
+// ListAllowedUpstreamEndpointsForService returns the list of endpoints over which the downstream client identity
+// is allowed access the upstream service
+func (mc *MeshCatalog) ListAllowedUpstreamEndpointsForService(downstreamIdentity identity.ServiceIdentity, upstreamSvc service.MeshService) ([]endpoint.Endpoint, error) {
 	outboundEndpoints, err := mc.listEndpointsForService(upstreamSvc)
 	if err != nil {
 		log.Error().Err(err).Msgf("Error looking up endpoints for upstream service %s", upstreamSvc)
 		return nil, err
 	}
+
+	if mc.configurator.IsPermissiveTrafficPolicyMode() {
+		return outboundEndpoints, nil
+	}
+
+	// In SMI mode, the endpoints for an upstream service must be filtered based on the service account
+	// associated with the endpoint. Only endpoints associated with authorized service accounts as referenced
+	// in SMI TrafficTarget resources should be returned.
+	//
+	// The following code filters the upstream service's endpoints for this purpose.
 	outboundEndpointsSet := make(map[string][]endpoint.Endpoint)
 	for _, ep := range outboundEndpoints {
 		ipStr := ep.IP.String()
@@ -60,7 +69,7 @@ func (mc *MeshCatalog) ListEndpointsForServiceIdentity(downstreamIdentity identi
 	}
 
 	// allowedEndpoints comprises of only those endpoints from outboundEndpoints that matches the endpoints from listEndpointsForServiceIdentity
-	// i.e. only those interseting endpoints are taken into cosideration
+	// i.e. only those intersecting endpoints are taken into cosideration
 	var allowedEndpoints []endpoint.Endpoint
 	for _, destSvcIdentity := range destSvcIdentities {
 		for _, ep := range mc.listEndpointsForServiceIdentity(destSvcIdentity) {

diff --git a/pkg/catalog/endpoint_test.go b/pkg/catalog/endpoint_test.go
@@ -54,7 +54,7 @@ var _ = Describe("Test catalog functions", func() {
 
 })
 
-func TestListEndpointsForServiceIdentity(t *testing.T) {
+func TestListAllowedUpstreamEndpointsForService(t *testing.T) {
 	assert := tassert.New(t)
 
 	testCases := []struct {
@@ -65,6 +65,7 @@ func TestListEndpointsForServiceIdentity(t *testing.T) {
 		services                 []service.MeshService
 		outboundServices         map[identity.ServiceIdentity][]service.MeshService
 		outboundServiceEndpoints map[service.MeshService][]endpoint.Endpoint
+		permissiveMode           bool
 		expectedEndpoints        []endpoint.Endpoint
 	}{
 		{
@@ -81,6 +82,7 @@ func TestListEndpointsForServiceIdentity(t *testing.T) {
 			outboundServiceEndpoints: map[service.MeshService][]endpoint.Endpoint{
 				tests.BookstoreV1Service: {tests.Endpoint},
 			},
+			permissiveMode:    false,
 			expectedEndpoints: []endpoint.Endpoint{tests.Endpoint},
 		},
 		{
@@ -102,6 +104,7 @@ func TestListEndpointsForServiceIdentity(t *testing.T) {
 					Port: endpoint.Port(tests.ServicePort),
 				}},
 			},
+			permissiveMode:    false,
 			expectedEndpoints: []endpoint.Endpoint{},
 		},
 		{
@@ -124,11 +127,40 @@ func TestListEndpointsForServiceIdentity(t *testing.T) {
 					Port: endpoint.Port(tests.ServicePort),
 				}},
 			},
+			permissiveMode: false,
 			expectedEndpoints: []endpoint.Endpoint{{
 				IP:   net.ParseIP("9.9.9.9"),
 				Port: endpoint.Port(tests.ServicePort),
 			}},
 		},
+		{
+			name:          `Permissive mode should return all endpoints for a service without filtering them`,
+			proxyIdentity: tests.BookbuyerServiceIdentity,
+			upstreamSvc:   tests.BookstoreV2Service,
+			outboundServiceEndpoints: map[service.MeshService][]endpoint.Endpoint{
+				tests.BookstoreV2Service: {
+					{
+						IP:   net.ParseIP("1.1.1.1"),
+						Port: 80,
+					},
+					{
+						IP:   net.ParseIP("2.2.2.2"),
+						Port: 80,
+					},
+				},
+			},
+			permissiveMode: true,
+			expectedEndpoints: []endpoint.Endpoint{
+				{
+					IP:   net.ParseIP("1.1.1.1"),
+					Port: 80,
+				},
+				{
+					IP:   net.ParseIP("2.2.2.2"),
+					Port: 80,
+				},
+			},
+		},
 	}
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
@@ -147,9 +179,22 @@ func TestListEndpointsForServiceIdentity(t *testing.T) {
 				meshSpec:           mockMeshSpec,
 				endpointsProviders: []endpoint.Provider{mockEndpointProvider},
 				serviceProviders:   []service.Provider{mockServiceProvider},
+				configurator:       mockConfigurator,
+			}
+
+			mockConfigurator.EXPECT().IsPermissiveTrafficPolicyMode().Return(tc.permissiveMode).AnyTimes()
+
+			for svc, endpoints := range tc.outboundServiceEndpoints {
+				mockEndpointProvider.EXPECT().ListEndpointsForService(svc).Return(endpoints).AnyTimes()
+			}
+
+			if tc.permissiveMode {
+				actual, err := mc.ListAllowedUpstreamEndpointsForService(tc.proxyIdentity, tc.upstreamSvc)
+				assert.Nil(err)
+				assert.ElementsMatch(actual, tc.expectedEndpoints)
+				return
 			}
 
-			mockConfigurator.EXPECT().IsPermissiveTrafficPolicyMode().Return(false).AnyTimes()
 			mockMeshSpec.EXPECT().ListTrafficTargets().Return(tc.trafficTargets).AnyTimes()
 
 			mockEndpointProvider.EXPECT().GetID().Return("fake").AnyTimes()
@@ -162,10 +207,6 @@ func TestListEndpointsForServiceIdentity(t *testing.T) {
 				mockServiceProvider.EXPECT().GetServicesForServiceIdentity(sa).Return(services, nil).AnyTimes()
 			}
 
-			for svc, endpoints := range tc.outboundServiceEndpoints {
-				mockEndpointProvider.EXPECT().ListEndpointsForService(svc).Return(endpoints).AnyTimes()
-			}
-
 			var pods []*v1.Pod
 			for serviceIdentity, services := range tc.outboundServices {
 				// TODO(draychev): use ServiceIdentity in the rest of the tests [https://github.com/openservicemesh/osm/issues/2218]
@@ -197,7 +238,7 @@ func TestListEndpointsForServiceIdentity(t *testing.T) {
 				}
 			}
 
-			actual, err := mc.ListEndpointsForServiceIdentity(tc.proxyIdentity, tc.upstreamSvc)
+			actual, err := mc.ListAllowedUpstreamEndpointsForService(tc.proxyIdentity, tc.upstreamSvc)
 			assert.Nil(err)
 			assert.ElementsMatch(actual, tc.expectedEndpoints)
 		})

diff --git a/pkg/catalog/mock_catalog_generated.go b/pkg/catalog/mock_catalog_generated.go
diff --git a/pkg/catalog/types.go b/pkg/catalog/types.go
@@ -64,8 +64,9 @@ type MeshCataloger interface {
 	// ListServiceIdentitiesForService lists the service identities associated with the given service
 	ListServiceIdentitiesForService(service.MeshService) ([]identity.ServiceIdentity, error)
 
-	// ListEndpointsForServiceIdentity returns the list of endpoints backing a service and its allowed service identities
-	ListEndpointsForServiceIdentity(identity.ServiceIdentity, service.MeshService) ([]endpoint.Endpoint, error)
+	// ListAllowedUpstreamEndpointsForService returns the list of endpoints over which the downstream client identity
+	// is allowed access the upstream service
+	ListAllowedUpstreamEndpointsForService(identity.ServiceIdentity, service.MeshService) ([]endpoint.Endpoint, error)
 
 	// GetResolvableServiceEndpoints returns the resolvable set of endpoint over which a service is accessible using its FQDN.
 	// These are the endpoint destinations we'd expect client applications sends the traffic towards to, when attempting to

diff --git a/pkg/envoy/eds/response.go b/pkg/envoy/eds/response.go
@@ -51,7 +51,7 @@ func fulfillEDSRequest(meshCatalog catalog.MeshCataloger, proxy *envoy.Proxy, re
 			log.Error().Err(err).Msgf("Error retrieving MeshService from Cluster %s", cluster)
 			continue
 		}
-		endpoints, err := meshCatalog.ListEndpointsForServiceIdentity(proxyIdentity, meshSvc)
+		endpoints, err := meshCatalog.ListAllowedUpstreamEndpointsForService(proxyIdentity, meshSvc)
 		if err != nil {
 			log.Error().Err(err).Msgf("Failed listing allowed endpoints for service %s, for proxy identity %s", meshSvc, proxyIdentity)
 			continue
@@ -103,7 +103,7 @@ func getEndpointsForProxy(meshCatalog catalog.MeshCataloger, proxyIdentity ident
 	allowedServicesEndpoints := make(map[service.MeshService][]endpoint.Endpoint)
 
 	for _, dstSvc := range meshCatalog.ListOutboundServicesForIdentity(proxyIdentity) {
-		endpoints, err := meshCatalog.ListEndpointsForServiceIdentity(proxyIdentity, dstSvc)
+		endpoints, err := meshCatalog.ListAllowedUpstreamEndpointsForService(proxyIdentity, dstSvc)
 		if err != nil {
 			log.Error().Err(err).Msgf("Failed listing allowed endpoints for service %s for proxy identity %s", dstSvc, proxyIdentity)
 			continue