From c372d1f97022a2102c00059ad1c708bbab87edfe Mon Sep 17 00:00:00 2001 From: Sneha Chhabria Date: Tue, 16 Feb 2021 11:51:24 -0800 Subject: [PATCH] feat/fix(endpoints) : Ensure endpoints for a serivce honors traffic policy This PR ensures that the endpoints built for a service in EDS honor SMI traffic target policies. Only those destinations endpoints are programmed on the envoy, if its destination pod has a service account specified as a destination in any of of the applicable traffic targets. Resolves issue #1658 Signed-off-by: Sneha Chhabria --- pkg/catalog/endpoint.go | 14 ++ pkg/catalog/fake.go | 2 +- pkg/catalog/helpers_test.go | 2 +- pkg/catalog/ingress_test.go | 10 +- pkg/catalog/mock_catalog.go | 14 ++ pkg/catalog/routes_test.go | 6 +- pkg/catalog/types.go | 3 + pkg/debugger/policy_test.go | 2 +- pkg/endpoint/mock_provider.go | 14 ++ pkg/endpoint/providers/kube/client.go | 27 +++ pkg/endpoint/providers/kube/client_test.go | 85 ++++++++++ pkg/endpoint/providers/kube/fake.go | 18 +- pkg/endpoint/types.go | 3 + pkg/envoy/eds/response.go | 57 +++++-- pkg/envoy/eds/response_test.go | 187 +++++++++++++++++++++ pkg/smi/fake.go | 1 + pkg/tests/fixtures.go | 15 +- 17 files changed, 435 insertions(+), 25 deletions(-) diff --git a/pkg/catalog/endpoint.go b/pkg/catalog/endpoint.go index 7a4e7a7287..4780ff6610 100644 --- a/pkg/catalog/endpoint.go +++ b/pkg/catalog/endpoint.go @@ -36,3 +36,17 @@ func (mc *MeshCatalog) GetResolvableServiceEndpoints(svc service.MeshService) ([ } return endpoints, nil } + +// ListEndpointsForPodFromServiceAccount retrieves the list of IP addresses for the given service account +func (mc *MeshCatalog) ListEndpointsForPodFromServiceAccount(sa service.K8sServiceAccount) []endpoint.Endpoint { + var endpoints []endpoint.Endpoint + for _, provider := range mc.endpointsProviders { + ep := provider.ListEndpointsForPodFromServiceAccount(sa) + if len(ep) == 0 { + log.Trace().Msgf("[%s] No endpoints found for service account=%s", provider.GetID(), sa) + continue + } + endpoints = append(endpoints, ep...) + } + return endpoints +} diff --git a/pkg/catalog/fake.go b/pkg/catalog/fake.go index 73a73b2ab2..2456904103 100644 --- a/pkg/catalog/fake.go +++ b/pkg/catalog/fake.go @@ -104,7 +104,7 @@ func NewFakeMeshCatalog(kubeClient kubernetes.Interface) *MeshCatalog { mockKubeController.EXPECT().IsMonitoredNamespace(tests.BookbuyerService.Namespace).Return(true).AnyTimes() mockKubeController.EXPECT().IsMonitoredNamespace(tests.BookwarehouseService.Namespace).Return(true).AnyTimes() mockKubeController.EXPECT().ListServiceAccountsForService(tests.BookstoreV1Service).Return([]service.K8sServiceAccount{tests.BookstoreServiceAccount}, nil).AnyTimes() - mockKubeController.EXPECT().ListServiceAccountsForService(tests.BookstoreV2Service).Return([]service.K8sServiceAccount{tests.BookstoreServiceAccount}, nil).AnyTimes() + mockKubeController.EXPECT().ListServiceAccountsForService(tests.BookstoreV2Service).Return([]service.K8sServiceAccount{tests.BookstoreV2ServiceAccount}, nil).AnyTimes() mockKubeController.EXPECT().ListServiceAccountsForService(tests.BookbuyerService).Return([]service.K8sServiceAccount{tests.BookbuyerServiceAccount}, nil).AnyTimes() return NewMeshCatalog(mockKubeController, kubeClient, meshSpec, certManager, diff --git a/pkg/catalog/helpers_test.go b/pkg/catalog/helpers_test.go index ae8d6d0061..474a468c05 100644 --- a/pkg/catalog/helpers_test.go +++ b/pkg/catalog/helpers_test.go @@ -51,7 +51,7 @@ func newFakeMeshCatalogForRoutes(t *testing.T, testParams testParams) *MeshCatal } // Create a bookstoreV2 pod - bookstoreV2Pod := tests.NewPodFixture(tests.BookstoreV2Service.Namespace, tests.BookstoreV2Service.Name, tests.BookstoreServiceAccountName, tests.PodLabels) + bookstoreV2Pod := tests.NewPodFixture(tests.BookstoreV2Service.Namespace, tests.BookstoreV2Service.Name, tests.BookstoreV2ServiceAccountName, tests.PodLabels) if _, err := kubeClient.CoreV1().Pods(tests.BookstoreV2Service.Namespace).Create(context.TODO(), &bookstoreV2Pod, metav1.CreateOptions{}); err != nil { t.Fatalf("Error creating new pod: %s", err.Error()) } diff --git a/pkg/catalog/ingress_test.go b/pkg/catalog/ingress_test.go index 8ba47ef043..a1bc523ee4 100644 --- a/pkg/catalog/ingress_test.go +++ b/pkg/catalog/ingress_test.go @@ -70,8 +70,14 @@ func newFakeMeshCatalog() *MeshCatalog { certManager := tresor.NewFakeCertManager(cfg) - // Create a pod - pod := tests.NewPodFixture(tests.Namespace, "pod-name", tests.BookstoreServiceAccountName, tests.PodLabels) + // Create a Bookstore-v1 pod + pod := tests.NewPodFixture(tests.Namespace, tests.BookstoreV1Service.Name, tests.BookstoreServiceAccountName, tests.PodLabels) + if _, err := kubeClient.CoreV1().Pods(tests.Namespace).Create(context.TODO(), &pod, metav1.CreateOptions{}); err != nil { + GinkgoT().Fatalf("Error creating new fake Mesh Catalog: %s", err.Error()) + } + + // Create a Bookstore-v2 pod + pod = tests.NewPodFixture(tests.Namespace, tests.BookstoreV2Service.Name, tests.BookstoreV2ServiceAccountName, tests.PodLabels) if _, err := kubeClient.CoreV1().Pods(tests.Namespace).Create(context.TODO(), &pod, metav1.CreateOptions{}); err != nil { GinkgoT().Fatalf("Error creating new fake Mesh Catalog: %s", err.Error()) } diff --git a/pkg/catalog/mock_catalog.go b/pkg/catalog/mock_catalog.go index 8c52e734ef..c18b599584 100644 --- a/pkg/catalog/mock_catalog.go +++ b/pkg/catalog/mock_catalog.go @@ -301,6 +301,20 @@ func (m *MockMeshCataloger) ListInboundTrafficTargetsWithRoutes(arg0 service.K8s return ret0, ret1 } +// ListEndpointsForPodFromServiceAccount mocks base method +func (m *MockMeshCataloger) ListEndpointsForPodFromServiceAccount(arg0 service.K8sServiceAccount) []endpoint.Endpoint { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ListEndpointsForPodFromServiceAccount", arg0) + ret0, _ := ret[0].([]endpoint.Endpoint) + return ret0 +} + +// ListEndpointsForService indicates an expected call of ListEndpointsForService +func (mr *MockMeshCatalogerMockRecorder) ListEndpointsForPodFromServiceAccount(arg0 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ListEndpointsForPodFromServiceAccount", reflect.TypeOf((*MockMeshCataloger)(nil).ListEndpointsForPodFromServiceAccount), arg0) +} + // ListInboundTrafficTargetsWithRoutes indicates an expected call of ListInboundTrafficTargetsWithRoutes func (mr *MockMeshCatalogerMockRecorder) ListInboundTrafficTargetsWithRoutes(arg0 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() diff --git a/pkg/catalog/routes_test.go b/pkg/catalog/routes_test.go index c97a374846..d0240264bc 100644 --- a/pkg/catalog/routes_test.go +++ b/pkg/catalog/routes_test.go @@ -558,6 +558,10 @@ func TestListTrafficPolicies(t *testing.T) { input: tests.BookstoreV1Service, output: []trafficpolicy.TrafficTarget{tests.BookstoreV1TrafficPolicy}, }, + { + input: tests.BookstoreV2Service, + output: []trafficpolicy.TrafficTarget{tests.BookstoreV2TrafficPolicy}, + }, { input: tests.BookbuyerService, output: []trafficpolicy.TrafficTarget{tests.BookstoreV1TrafficPolicy, tests.BookstoreV2TrafficPolicy, tests.BookstoreApexTrafficPolicy}, @@ -592,7 +596,7 @@ func TestGetTrafficPoliciesForService(t *testing.T) { HTTPRouteMatches: tests.BookstoreV1TrafficPolicy.HTTPRouteMatches, }, { - Name: utils.GetTrafficTargetName(tests.TrafficTargetName, tests.BookbuyerService, tests.BookstoreV2Service), + Name: utils.GetTrafficTargetName(tests.BookstoreV2TrafficTargetName, tests.BookbuyerService, tests.BookstoreV2Service), Destination: tests.BookstoreV2Service, Source: tests.BookbuyerService, HTTPRouteMatches: tests.BookstoreV2TrafficPolicy.HTTPRouteMatches, diff --git a/pkg/catalog/types.go b/pkg/catalog/types.go index 1c74600372..9b38b07c90 100644 --- a/pkg/catalog/types.go +++ b/pkg/catalog/types.go @@ -89,6 +89,9 @@ type MeshCataloger interface { // ListEndpointsForService returns the list of individual instance endpoint backing a service ListEndpointsForService(service.MeshService) ([]endpoint.Endpoint, error) + // ListEndpointsForPodFromServiceAccount retrieves the list of IP addresses for the given service account + ListEndpointsForPodFromServiceAccount(sa service.K8sServiceAccount) []endpoint.Endpoint + // 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 // reach a specific service. diff --git a/pkg/debugger/policy_test.go b/pkg/debugger/policy_test.go index ce6fa75d10..9363dee6c8 100644 --- a/pkg/debugger/policy_test.go +++ b/pkg/debugger/policy_test.go @@ -53,6 +53,6 @@ func TestGetSMIPolicies(t *testing.T) { responseRecorder := httptest.NewRecorder() smiPoliciesHandler.ServeHTTP(responseRecorder, nil) actualResponseBody := responseRecorder.Body.String() - expectedResponseBody := `{"traffic_splits":[{"metadata":{"name":"bar","namespace":"foo","creationTimestamp":null},"spec":{}}],"weighted_services":[{"service_name:omitempty":{"Namespace":"default","Name":"bookstore-v1"},"weight:omitempty":90,"root_service:omitempty":"bookstore-apex"},{"service_name:omitempty":{"Namespace":"default","Name":"bookstore-v2"},"weight:omitempty":10,"root_service:omitempty":"bookstore-apex"}],"service_accounts":[{"Namespace":"default","Name":"bookbuyer"}],"route_groups":[{"kind":"HTTPRouteGroup","apiVersion":"specs.smi-spec.io/v1alpha4","metadata":{"name":"bookstore-service-routes","namespace":"default","creationTimestamp":null},"spec":{"matches":[{"name":"buy-books","methods":["GET"],"pathRegex":"/buy","headers":[{"user-agent":"test-UA"}]},{"name":"sell-books","methods":["GET"],"pathRegex":"/sell","headers":[{"user-agent":"test-UA"}]},{"name":"allow-everything-on-header","headers":[{"user-agent":"test-UA"}]}]}}],"traffic_targets":[{"kind":"TrafficTarget","apiVersion":"access.smi-spec.io/v1alpha3","metadata":{"name":"bookbuyer-access-bookstore","namespace":"default","creationTimestamp":null},"spec":{"destination":{"kind":"Name","name":"bookstore","namespace":"default"},"sources":[{"kind":"Name","name":"bookbuyer","namespace":"default"}],"rules":[{"kind":"HTTPRouteGroup","name":"bookstore-service-routes","matches":["buy-books","sell-books"]}]}}]}` + expectedResponseBody := `{"traffic_splits":[{"metadata":{"name":"bar","namespace":"foo","creationTimestamp":null},"spec":{}}],"weighted_services":[{"service_name:omitempty":{"Namespace":"default","Name":"bookstore-v1"},"weight:omitempty":90,"root_service:omitempty":"bookstore-apex"},{"service_name:omitempty":{"Namespace":"default","Name":"bookstore-v2"},"weight:omitempty":10,"root_service:omitempty":"bookstore-apex"}],"service_accounts":[{"Namespace":"default","Name":"bookbuyer"}],"route_groups":[{"kind":"HTTPRouteGroup","apiVersion":"specs.smi-spec.io/v1alpha4","metadata":{"name":"bookstore-service-routes","namespace":"default","creationTimestamp":null},"spec":{"matches":[{"name":"buy-books","methods":["GET"],"pathRegex":"/buy","headers":[{"user-agent":"test-UA"}]},{"name":"sell-books","methods":["GET"],"pathRegex":"/sell","headers":[{"user-agent":"test-UA"}]},{"name":"allow-everything-on-header","headers":[{"user-agent":"test-UA"}]}]}}],"traffic_targets":[{"kind":"TrafficTarget","apiVersion":"access.smi-spec.io/v1alpha3","metadata":{"name":"bookbuyer-access-bookstore","namespace":"default","creationTimestamp":null},"spec":{"destination":{"kind":"ServiceAccount","name":"bookstore","namespace":"default"},"sources":[{"kind":"ServiceAccount","name":"bookbuyer","namespace":"default"}],"rules":[{"kind":"HTTPRouteGroup","name":"bookstore-service-routes","matches":["buy-books","sell-books"]}]}}]}` assert.Equal(actualResponseBody, expectedResponseBody, "Actual value did not match expectations:\n%s", actualResponseBody) } diff --git a/pkg/endpoint/mock_provider.go b/pkg/endpoint/mock_provider.go index 5a5ecb72e7..f7560ed26d 100644 --- a/pkg/endpoint/mock_provider.go +++ b/pkg/endpoint/mock_provider.go @@ -49,6 +49,20 @@ func (mr *MockProviderMockRecorder) ListEndpointsForService(arg0 interface{}) *g return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ListEndpointsForService", reflect.TypeOf((*MockProvider)(nil).ListEndpointsForService), arg0) } +// ListEndpointsForPodFromServiceAccount mocks base method +func (m *MockProvider) ListEndpointsForPodFromServiceAccount(arg0 service.K8sServiceAccount) []Endpoint { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ListEndpointsForPodFromServiceAccount", arg0) + ret0, _ := ret[0].([]Endpoint) + return ret0 +} + +// ListEndpointsForPodFromServiceAccount indicates an expected call of ListEndpointsForPodFromServiceAccount +func (mr *MockProviderMockRecorder) ListEndpointsForPodFromServiceAccount(arg0 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ListEndpointsForPodFromServiceAccount", reflect.TypeOf((*MockProvider)(nil).ListEndpointsForPodFromServiceAccount), arg0) +} + // GetServicesForServiceAccount mocks base method func (m *MockProvider) GetServicesForServiceAccount(arg0 service.K8sServiceAccount) ([]service.MeshService, error) { m.ctrl.T.Helper() diff --git a/pkg/endpoint/providers/kube/client.go b/pkg/endpoint/providers/kube/client.go index cf79ea3c8e..f03fefb0f5 100644 --- a/pkg/endpoint/providers/kube/client.go +++ b/pkg/endpoint/providers/kube/client.go @@ -67,6 +67,33 @@ func (c Client) ListEndpointsForService(svc service.MeshService) []endpoint.Endp return endpoints } +// ListEndpointsForPodFromServiceAccount retrieves the list of IP addresses for the given service account +func (c Client) ListEndpointsForPodFromServiceAccount(sa service.K8sServiceAccount) []endpoint.Endpoint { + log.Trace().Msgf("[%s] Getting Endpoints for service account %s on Kubernetes", c.providerIdent, sa) + var endpoints []endpoint.Endpoint + + podList := c.kubeController.ListPods() + for _, pod := range podList { + if pod.Namespace != sa.Namespace { + continue + } + if pod.Spec.ServiceAccountName != sa.Name { + continue + } + + for _, podIP := range pod.Status.PodIPs { + ip := net.ParseIP(podIP.IP) + if ip == nil { + log.Error().Msgf("[%s] Error parsing IP address %s", c.providerIdent, podIP.IP) + break + } + ept := endpoint.Endpoint{IP: ip} + endpoints = append(endpoints, ept) + } + } + return endpoints +} + // GetServicesForServiceAccount retrieves a list of services for the given service account. func (c Client) GetServicesForServiceAccount(svcAccount service.K8sServiceAccount) ([]service.MeshService, error) { services := mapset.NewSet() diff --git a/pkg/endpoint/providers/kube/client_test.go b/pkg/endpoint/providers/kube/client_test.go index 63cf3d7195..397aa6a1ac 100644 --- a/pkg/endpoint/providers/kube/client_test.go +++ b/pkg/endpoint/providers/kube/client_test.go @@ -4,16 +4,20 @@ import ( "context" "fmt" "net" + "testing" "time" + "github.com/google/uuid" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" "github.com/golang/mock/gomock" + tassert "github.com/stretchr/testify/assert" corev1 "k8s.io/api/core/v1" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes/fake" + testclient "k8s.io/client-go/kubernetes/fake" "github.com/openservicemesh/osm/pkg/announcements" "github.com/openservicemesh/osm/pkg/configurator" @@ -552,3 +556,84 @@ var _ = Describe("Test Kube Client Provider (/w kubecontroller)", func() { <-podsAndServiceChannel }) }) + +func TestListEndpointsForPodFromServiceAccount(t *testing.T) { + assert := tassert.New(t) + + testCases := []struct { + name string + serviceAccount service.K8sServiceAccount + outboundServiceAccountEndpoints map[service.K8sServiceAccount][]endpoint.Endpoint + expectedEndpoints []endpoint.Endpoint + }{ + { + name: "get endpoints for pod with only one ip", + serviceAccount: tests.BookstoreServiceAccount, + outboundServiceAccountEndpoints: map[service.K8sServiceAccount][]endpoint.Endpoint{ + tests.BookstoreServiceAccount: {{ + IP: net.ParseIP(tests.ServiceIP), + }}, + }, + expectedEndpoints: []endpoint.Endpoint{{ + IP: net.ParseIP(tests.ServiceIP), + }}, + }, + { + name: "get endpoints for pod with multiple ips", + serviceAccount: tests.BookstoreServiceAccount, + outboundServiceAccountEndpoints: map[service.K8sServiceAccount][]endpoint.Endpoint{ + tests.BookstoreServiceAccount: { + endpoint.Endpoint{ + IP: net.ParseIP(tests.ServiceIP), + }, + endpoint.Endpoint{ + IP: net.ParseIP("9.9.9.9"), + }, + }, + }, + expectedEndpoints: []endpoint.Endpoint{{ + IP: net.ParseIP(tests.ServiceIP), + }, + { + IP: net.ParseIP("9.9.9.9"), + }}, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + mockCtrl := gomock.NewController(t) + kubeClient := testclient.NewSimpleClientset() + defer mockCtrl.Finish() + + mockKubeController := k8s.NewMockController(mockCtrl) + mockConfigurator := configurator.NewMockConfigurator(mockCtrl) + providerID := "provider" + + provider, err := NewProvider(kubeClient, mockKubeController, providerID, mockConfigurator) + assert.Nil(err) + + var pods []*v1.Pod + for sa, endpoints := range tc.outboundServiceAccountEndpoints { + podlabels := map[string]string{ + tests.SelectorKey: tests.SelectorValue, + constants.EnvoyUniqueIDLabelName: uuid.New().String(), + } + pod := tests.NewPodFixture(sa.Namespace, sa.Name, sa.Name, podlabels) + var podIps []v1.PodIP + for _, ep := range endpoints { + podIps = append(podIps, v1.PodIP{IP: ep.IP.String()}) + } + pod.Status.PodIPs = podIps + _, err := kubeClient.CoreV1().Pods(sa.Namespace).Create(context.TODO(), &pod, metav1.CreateOptions{}) + assert.Nil(err) + pods = append(pods, &pod) + } + mockKubeController.EXPECT().ListPods().Return(pods).AnyTimes() + + actual := provider.ListEndpointsForPodFromServiceAccount(tc.serviceAccount) + assert.NotNil(actual) + assert.ElementsMatch(actual, tc.expectedEndpoints) + }) + } +} diff --git a/pkg/endpoint/providers/kube/fake.go b/pkg/endpoint/providers/kube/fake.go index 15160f0898..ffa3a17ba5 100644 --- a/pkg/endpoint/providers/kube/fake.go +++ b/pkg/endpoint/providers/kube/fake.go @@ -24,12 +24,18 @@ func NewFakeProvider() endpoint.Provider { tests.BookstoreV2ServiceAccount: {tests.BookstoreV2Service}, tests.BookbuyerServiceAccount: {tests.BookbuyerService}, }, + svcAccountEndpoints: map[service.K8sServiceAccount][]endpoint.Endpoint{ + tests.BookstoreServiceAccount: {tests.Endpoint, tests.Endpoint}, + tests.BookstoreV2ServiceAccount: {tests.Endpoint}, + tests.BookbuyerServiceAccount: {tests.Endpoint}, + }, } } type fakeClient struct { - endpoints map[string][]endpoint.Endpoint - services map[service.K8sServiceAccount][]service.MeshService + endpoints map[string][]endpoint.Endpoint + services map[service.K8sServiceAccount][]service.MeshService + svcAccountEndpoints map[service.K8sServiceAccount][]endpoint.Endpoint } // Retrieve the IP addresses comprising the given service. @@ -40,6 +46,14 @@ func (f fakeClient) ListEndpointsForService(svc service.MeshService) []endpoint. panic(fmt.Sprintf("You are asking for MeshService=%s but the fake Kubernetes client has not been initialized with this. What we have is: %+v", svc.String(), f.endpoints)) } +// Retrieve the IP addresses comprising the given service account. +func (f fakeClient) ListEndpointsForPodFromServiceAccount(sa service.K8sServiceAccount) []endpoint.Endpoint { + if ep, ok := f.svcAccountEndpoints[sa]; ok { + return ep + } + panic(fmt.Sprintf("You are asking for K8sServiceAccount=%s but the fake Kubernetes client has not been initialized with this. What we have is: %+v", sa.String(), f.svcAccountEndpoints)) +} + func (f fakeClient) GetServicesForServiceAccount(svcAccount service.K8sServiceAccount) ([]service.MeshService, error) { services, ok := f.services[svcAccount] if !ok { diff --git a/pkg/endpoint/types.go b/pkg/endpoint/types.go index f894bcb553..389b80af7d 100644 --- a/pkg/endpoint/types.go +++ b/pkg/endpoint/types.go @@ -12,6 +12,9 @@ type Provider interface { // Retrieve the IP addresses comprising the given service. ListEndpointsForService(service.MeshService) []Endpoint + // ListEndpointsForPodFromServiceAccount retrieves the list of IP addresses for the given service account + ListEndpointsForPodFromServiceAccount(sa service.K8sServiceAccount) []Endpoint + // Retrieve the namespaced services for a given service account GetServicesForServiceAccount(service.K8sServiceAccount) ([]service.MeshService, error) diff --git a/pkg/envoy/eds/response.go b/pkg/envoy/eds/response.go index 6383e6bd8d..6e0e002aef 100644 --- a/pkg/envoy/eds/response.go +++ b/pkg/envoy/eds/response.go @@ -23,20 +23,14 @@ func NewResponse(meshCatalog catalog.MeshCataloger, proxy *envoy.Proxy, _ *xds_d return nil, err } - outboundServicesEndpoints := make(map[service.MeshService][]endpoint.Endpoint) - for _, dstSvc := range meshCatalog.ListAllowedOutboundServicesForIdentity(proxyIdentity) { - endpoints, err := meshCatalog.ListEndpointsForService(dstSvc) - if err != nil { - log.Error().Err(err).Msgf("Failed listing endpoints for service %s", dstSvc) - continue - } - outboundServicesEndpoints[dstSvc] = endpoints + allowedEndpoints, err := getEndpointsForProxy(meshCatalog, proxyIdentity) + if err != nil { + log.Error().Err(err).Msgf("Error looking up endpoints for proxy with SerialNumber=%s on Pod with UID=%s", proxy.GetCertificateSerialNumber(), proxy.GetPodUID()) + return nil, err } - log.Trace().Msgf("Outbound service endpoints for proxy with SerialNumber=%s on Pod with UID=%s: %v", proxy.GetCertificateSerialNumber(), proxy.GetPodUID(), outboundServicesEndpoints) - var protos []*any.Any - for svc, endpoints := range outboundServicesEndpoints { + for svc, endpoints := range allowedEndpoints { loadAssignment := cla.NewClusterLoadAssignment(svc, endpoints) proto, err := ptypes.MarshalAny(loadAssignment) if err != nil { @@ -52,3 +46,44 @@ func NewResponse(meshCatalog catalog.MeshCataloger, proxy *envoy.Proxy, _ *xds_d } return resp, nil } + +// getEndpointsForProxy returns only those service endpoints that belong to the allowed outbound service accounts for the proxy +func getEndpointsForProxy(meshCatalog catalog.MeshCataloger, proxyIdentity service.K8sServiceAccount) (map[service.MeshService][]endpoint.Endpoint, error) { + outboundServicesEndpoints := make(map[service.MeshService][]endpoint.Endpoint) + + // outboundServicesEndpoints is a map of all endpoints that belong to a given service + // some of these endpoints may belong to pods who's service accounts aren't in the list of allowed outbound service accounts + for _, dstSvc := range meshCatalog.ListAllowedOutboundServicesForIdentity(proxyIdentity) { + endpoints, err := meshCatalog.ListEndpointsForService(dstSvc) + if err != nil { + log.Error().Err(err).Msgf("Failed listing endpoints for service %s for proxy identity %s", dstSvc, proxyIdentity) + continue + } + outboundServicesEndpoints[dstSvc] = endpoints + } + log.Trace().Msgf("Outbound service endpoints for proxy with identity %s: %v", proxyIdentity, outboundServicesEndpoints) + + destSvcAccounts, err := meshCatalog.ListAllowedOutboundServiceAccounts(proxyIdentity) + if err != nil { + log.Error().Err(err).Msgf("Error looking outbound service accounts for proxy with identity %s", proxyIdentity) + return nil, err + } + + // allowedServicesEndpoints is a map of only those service endpoints that belong to the allowed outbound service accounts + // it is formed by taking only those endpoints from outboundServicesEndpoints that match the pods having the allowed service account + allowedServicesEndpoints := make(map[service.MeshService][]endpoint.Endpoint) + for _, destSvcAccount := range destSvcAccounts { + podEndpoints := meshCatalog.ListEndpointsForPodFromServiceAccount(destSvcAccount) + for destSvc, endpoints := range outboundServicesEndpoints { + for _, ep := range endpoints { + for _, podIP := range podEndpoints { + if ep.IP.Equal(podIP.IP) { + allowedServicesEndpoints[destSvc] = append(allowedServicesEndpoints[destSvc], ep) + } + } + } + } + } + log.Trace().Msgf("Allowed outbound service endpoints for proxy with identity %s: %v", proxyIdentity, allowedServicesEndpoints) + return allowedServicesEndpoints, nil +} diff --git a/pkg/envoy/eds/response_test.go b/pkg/envoy/eds/response_test.go index cdbf97eb34..c7de6340ea 100644 --- a/pkg/envoy/eds/response_test.go +++ b/pkg/envoy/eds/response_test.go @@ -1,13 +1,19 @@ package eds import ( + "context" "fmt" + "net" "testing" xds_endpoint "github.com/envoyproxy/go-control-plane/envoy/config/endpoint/v3" "github.com/golang/mock/gomock" "github.com/golang/protobuf/ptypes" + "github.com/google/uuid" + access "github.com/servicemeshinterface/smi-sdk-go/pkg/apis/access/v1alpha3" tassert "github.com/stretchr/testify/assert" + v1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes" testclient "k8s.io/client-go/kubernetes/fake" @@ -15,7 +21,11 @@ import ( "github.com/openservicemesh/osm/pkg/certificate" "github.com/openservicemesh/osm/pkg/configurator" "github.com/openservicemesh/osm/pkg/constants" + "github.com/openservicemesh/osm/pkg/endpoint" "github.com/openservicemesh/osm/pkg/envoy" + k8s "github.com/openservicemesh/osm/pkg/kubernetes" + "github.com/openservicemesh/osm/pkg/service" + "github.com/openservicemesh/osm/pkg/smi" "github.com/openservicemesh/osm/pkg/tests" ) @@ -79,3 +89,180 @@ func TestEndpointConfiguration(t *testing.T) { assert.Nil(err) assert.Len(loadAssignment.Endpoints, 1) } + +func TestGetEndpointsFromProxy(t *testing.T) { + assert := tassert.New(t) + + testCases := []struct { + name string + proxyIdentity service.K8sServiceAccount + trafficTargets []*access.TrafficTarget + allowedServiceAccounts []service.K8sServiceAccount + services []service.MeshService + outboundServices map[service.K8sServiceAccount][]service.MeshService + outboundServiceEndpoints map[service.MeshService][]endpoint.Endpoint + outboundServiceAccountEndpoints map[service.K8sServiceAccount][]endpoint.Endpoint + expectedEndpoints map[service.MeshService][]endpoint.Endpoint + }{ + { + name: `Traffic target defined for bookstore ServiceAccount. + This service account has only bookstore-v1 service on it. + Hence endpoints for bookstore-v1 should be in the expected list`, + proxyIdentity: tests.BookbuyerServiceAccount, + trafficTargets: []*access.TrafficTarget{&tests.TrafficTarget}, + allowedServiceAccounts: []service.K8sServiceAccount{tests.BookstoreServiceAccount}, + services: []service.MeshService{tests.BookstoreV1Service}, + outboundServices: map[service.K8sServiceAccount][]service.MeshService{ + tests.BookstoreServiceAccount: {tests.BookstoreV1Service}, + }, + outboundServiceEndpoints: map[service.MeshService][]endpoint.Endpoint{ + tests.BookstoreV1Service: {tests.Endpoint}, + }, + outboundServiceAccountEndpoints: map[service.K8sServiceAccount][]endpoint.Endpoint{ + tests.BookstoreServiceAccount: {tests.Endpoint}, + }, + expectedEndpoints: map[service.MeshService][]endpoint.Endpoint{ + tests.BookstoreV1Service: {tests.Endpoint}, + }, + }, + { + name: `Traffic target defined for bookstore ServiceAccount. + This service account has bookstore-v1 bookstore-v2 services, + but bookstore-v2 pod has service account bookstore-v2. + Hence endpoints for bookstore-v2 shouldn't be in the expected list`, + proxyIdentity: tests.BookbuyerServiceAccount, + trafficTargets: []*access.TrafficTarget{&tests.TrafficTarget}, + allowedServiceAccounts: []service.K8sServiceAccount{tests.BookstoreServiceAccount}, + services: []service.MeshService{tests.BookstoreV1Service, tests.BookstoreV2Service}, + outboundServices: map[service.K8sServiceAccount][]service.MeshService{ + tests.BookstoreServiceAccount: {tests.BookstoreV1Service, tests.BookstoreV2Service}, + }, + outboundServiceEndpoints: map[service.MeshService][]endpoint.Endpoint{ + tests.BookstoreV1Service: {tests.Endpoint}, + tests.BookstoreV2Service: {endpoint.Endpoint{ + IP: net.ParseIP("9.9.9.9"), + Port: endpoint.Port(tests.ServicePort), + }}, + }, + outboundServiceAccountEndpoints: map[service.K8sServiceAccount][]endpoint.Endpoint{ + tests.BookstoreServiceAccount: {tests.Endpoint}, + tests.BookstoreV2ServiceAccount: {endpoint.Endpoint{ + IP: net.ParseIP("9.9.9.9"), + Port: endpoint.Port(tests.ServicePort), + }}, + }, + expectedEndpoints: map[service.MeshService][]endpoint.Endpoint{ + tests.BookstoreV1Service: {tests.Endpoint}, + }, + }, + { + name: `Traffic target defined for bookstore and bookstore-v2 ServiceAccount. + Hence endpoints for bookstore-v1 and bookstore-v2 should be in the expected list`, + proxyIdentity: tests.BookbuyerServiceAccount, + trafficTargets: []*access.TrafficTarget{&tests.TrafficTarget, &tests.BookstoreV2TrafficTarget}, + allowedServiceAccounts: []service.K8sServiceAccount{tests.BookstoreServiceAccount, tests.BookstoreV2ServiceAccount}, + services: []service.MeshService{tests.BookstoreV1Service, tests.BookstoreV2Service}, + outboundServices: map[service.K8sServiceAccount][]service.MeshService{ + tests.BookstoreServiceAccount: {tests.BookstoreV1Service}, + tests.BookstoreV2ServiceAccount: {tests.BookstoreV2Service}, + }, + outboundServiceEndpoints: map[service.MeshService][]endpoint.Endpoint{ + tests.BookstoreV1Service: {tests.Endpoint}, + tests.BookstoreV2Service: {endpoint.Endpoint{ + IP: net.ParseIP("9.9.9.9"), + Port: endpoint.Port(tests.ServicePort), + }}, + }, + outboundServiceAccountEndpoints: map[service.K8sServiceAccount][]endpoint.Endpoint{ + tests.BookstoreServiceAccount: {tests.Endpoint}, + tests.BookstoreV2ServiceAccount: {endpoint.Endpoint{ + IP: net.ParseIP("9.9.9.9"), + Port: endpoint.Port(tests.ServicePort), + }}, + }, + expectedEndpoints: map[service.MeshService][]endpoint.Endpoint{ + tests.BookstoreV1Service: {tests.Endpoint}, + tests.BookstoreV2Service: {endpoint.Endpoint{ + IP: net.ParseIP("9.9.9.9"), + Port: endpoint.Port(tests.ServicePort), + }}, + }, + }, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + mockCtrl := gomock.NewController(t) + kubeClient := testclient.NewSimpleClientset() + defer mockCtrl.Finish() + + mockCatalog := catalog.NewMockMeshCataloger(mockCtrl) + mockConfigurator := configurator.NewMockConfigurator(mockCtrl) + mockKubeController := k8s.NewMockController(mockCtrl) + meshSpec := smi.NewMockMeshSpec(mockCtrl) + mockEndpointProvider := endpoint.NewMockProvider(mockCtrl) + + mockConfigurator.EXPECT().IsPermissiveTrafficPolicyMode().Return(false).AnyTimes() + meshSpec.EXPECT().ListTrafficTargets().Return(tc.trafficTargets).AnyTimes() + + proxy, err := getProxy(kubeClient) + assert.Empty(err) + assert.NotNil(mockCatalog) + assert.NotNil(proxy) + + mockEndpointProvider.EXPECT().GetID().Return("fake").AnyTimes() + + for sa, services := range tc.outboundServices { + for _, svc := range services { + k8sService := tests.NewServiceFixture(svc.Name, svc.Namespace, map[string]string{}) + mockKubeController.EXPECT().GetService(svc).Return(k8sService).AnyTimes() + } + mockEndpointProvider.EXPECT().GetServicesForServiceAccount(sa).Return(services, nil).AnyTimes() + } + + mockCatalog.EXPECT().ListAllowedOutboundServicesForIdentity(tc.proxyIdentity).Return(tc.services).AnyTimes() + + for svc, endpoints := range tc.outboundServiceEndpoints { + mockEndpointProvider.EXPECT().ListEndpointsForService(svc).Return(endpoints).AnyTimes() + mockCatalog.EXPECT().ListEndpointsForService(svc).Return(endpoints, nil).AnyTimes() + } + + mockCatalog.EXPECT().ListAllowedOutboundServiceAccounts(tc.proxyIdentity).Return(tc.allowedServiceAccounts, nil).AnyTimes() + + var pods []*v1.Pod + for sa, services := range tc.outboundServices { + for _, svc := range services { + podlabels := map[string]string{ + tests.SelectorKey: tests.SelectorValue, + constants.EnvoyUniqueIDLabelName: uuid.New().String(), + } + pod := tests.NewPodFixture(tests.Namespace, svc.Name, sa.Name, podlabels) + podEndpoints := tc.outboundServiceEndpoints[svc] + var podIps []v1.PodIP + for _, ep := range podEndpoints { + podIps = append(podIps, v1.PodIP{IP: ep.IP.String()}) + } + pod.Status.PodIPs = podIps + _, err = kubeClient.CoreV1().Pods(tests.Namespace).Create(context.TODO(), &pod, metav1.CreateOptions{}) + assert.Nil(err) + pods = append(pods, &pod) + } + } + mockKubeController.EXPECT().ListPods().Return(pods).AnyTimes() + + for sa, endpoints := range tc.outboundServiceAccountEndpoints { + mockCatalog.EXPECT().ListEndpointsForPodFromServiceAccount(sa).Return(endpoints).AnyTimes() + } + + actual, err := getEndpointsForProxy(mockCatalog, tc.proxyIdentity) + assert.Nil(err) + assert.NotNil(actual) + + assert.Len(actual, len(tc.expectedEndpoints)) + for svc, endpoints := range tc.expectedEndpoints { + _, ok := actual[svc] + assert.True(ok) + assert.ElementsMatch(actual[svc], endpoints) + } + }) + } +} diff --git a/pkg/smi/fake.go b/pkg/smi/fake.go index 742fa73c7b..ed16ed1a7a 100644 --- a/pkg/smi/fake.go +++ b/pkg/smi/fake.go @@ -31,6 +31,7 @@ func NewFakeMeshSpecClient() MeshSpec { weightedServices: []service.WeightedService{tests.BookstoreV1WeightedService, tests.BookstoreV2WeightedService}, serviceAccounts: []service.K8sServiceAccount{ tests.BookstoreServiceAccount, + tests.BookstoreV2ServiceAccount, tests.BookbuyerServiceAccount, }, diff --git a/pkg/tests/fixtures.go b/pkg/tests/fixtures.go index a0abd3ff19..e7f3ea3391 100644 --- a/pkg/tests/fixtures.go +++ b/pkg/tests/fixtures.go @@ -57,6 +57,9 @@ const ( // TrafficTargetName is the name of the traffic target SMI object. TrafficTargetName = "bookbuyer-access-bookstore" + // BookstoreV2TrafficTargetName is the name of the traffic target SMI object. + BookstoreV2TrafficTargetName = "bookbuyer-access-bookstore-v2" + // BuyBooksMatchName is the name of the match object. BuyBooksMatchName = "buy-books" @@ -234,7 +237,7 @@ var ( // BookstoreV2TrafficPolicy is a traffic policy SMI object. BookstoreV2TrafficPolicy = trafficpolicy.TrafficTarget{ - Name: fmt.Sprintf("%s:default/bookbuyer->default/bookstore-v2", TrafficTargetName), + Name: fmt.Sprintf("%s:default/bookbuyer->default/bookstore-v2", BookstoreV2TrafficTargetName), Destination: BookstoreV2Service, Source: BookbuyerService, HTTPRouteMatches: []trafficpolicy.HTTPRouteMatch{ @@ -310,12 +313,12 @@ var ( }, Spec: access.TrafficTargetSpec{ Destination: access.IdentityBindingSubject{ - Kind: "Name", + Kind: "ServiceAccount", Name: BookstoreServiceAccountName, Namespace: "default", }, Sources: []access.IdentityBindingSubject{{ - Kind: "Name", + Kind: "ServiceAccount", Name: BookbuyerServiceAccountName, Namespace: "default", }}, @@ -334,17 +337,17 @@ var ( Kind: "TrafficTarget", }, ObjectMeta: v1.ObjectMeta{ - Name: TrafficTargetName, + Name: BookstoreV2TrafficTargetName, Namespace: "default", }, Spec: access.TrafficTargetSpec{ Destination: access.IdentityBindingSubject{ - Kind: "Name", + Kind: "ServiceAccount", Name: BookstoreV2ServiceAccountName, Namespace: "default", }, Sources: []access.IdentityBindingSubject{{ - Kind: "Name", + Kind: "ServiceAccount", Name: BookbuyerServiceAccountName, Namespace: "default", }},