Skip to content
This repository was archived by the owner on Jul 11, 2023. It is now read-only.

endpoints: don't filter service endpoints in permissive mode #4066

Merged
merged 1 commit into from
Sep 1, 2021
Merged

endpoints: don't filter service endpoints in permissive mode #4066

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 14 additions & 5 deletions pkg/catalog/endpoint.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this issue #3190, we discussed about dropping the word allowed from method names. Basically we don't return endpoints that are not allowed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For permissive mode, everything is allowed. For SMI, a subset of endpoints is allowed. Hope that makes sense.

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()
Expand All @@ -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) {
Expand Down
55 changes: 48 additions & 7 deletions pkg/catalog/endpoint_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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
}{
{
Expand All @@ -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},
},
{
Expand All @@ -102,6 +104,7 @@ func TestListEndpointsForServiceIdentity(t *testing.T) {
Port: endpoint.Port(tests.ServicePort),
}},
},
permissiveMode: false,
expectedEndpoints: []endpoint.Endpoint{},
},
{
Expand All @@ -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) {
Expand All @@ -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()
Expand All @@ -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]
Expand Down Expand Up @@ -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)
})
Expand Down
12 changes: 6 additions & 6 deletions pkg/catalog/mock_catalog_generated.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 3 additions & 2 deletions pkg/catalog/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions pkg/envoy/eds/response.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down