Skip to content

Commit

Permalink
Stop creation of service entires with dummy endpoints (#255)
Browse files Browse the repository at this point in the history 
* Stop creation of service entries with dummy endpoints

Signed-off-by: vjoshi3 <[email protected]>

Signed-off-by: vjoshi3 <[email protected]>
Co-authored-by: vjoshi3 <[email protected]>
  • Loading branch information
@vrushalijoshi
vrushalijoshi and vjoshi3 authored Sep 2, 2022
1 parent f933203 commit b371d75
Show file tree
Hide file tree
Showing 4 changed files with 214 additions and 8 deletions.
36 changes: 30 additions & 6 deletions admiral/pkg/clusters/handler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -521,6 +521,23 @@ func addUpdateVirtualService(ctx context.Context, obj *v1alpha3.VirtualService,
}
}

func validateAndProcessServiceEntryEndpoints(obj *v1alpha3.ServiceEntry) bool {
var areEndpointsValid = true

temp := make([]*v1alpha32.WorkloadEntry, 0)
for _, endpoint := range obj.Spec.Endpoints {
if endpoint.Address == "dummy.admiral.global" {
areEndpointsValid = false
} else {
temp = append(temp, endpoint)
}
}
obj.Spec.Endpoints = temp
log.Infof("type=ServiceEntry, name=%s, endpointsValid=%v, numberOfValidEndpoints=%d", obj.Name, areEndpointsValid, len(obj.Spec.Endpoints))

return areEndpointsValid
}

func addUpdateServiceEntry(ctx context.Context, obj *v1alpha3.ServiceEntry, exist *v1alpha3.ServiceEntry, namespace string, rc *RemoteController) {
var (
err error
Expand All @@ -532,13 +549,21 @@ func addUpdateServiceEntry(ctx context.Context, obj *v1alpha3.ServiceEntry, exis
obj.Annotations = map[string]string{}
}
obj.Annotations["app.kubernetes.io/created-by"] = "admiral"

areEndpointsValid := validateAndProcessServiceEntryEndpoints(obj)

if exist == nil || exist.Spec.Hosts == nil {
obj.Namespace = namespace
obj.ResourceVersion = ""
_, err = rc.ServiceEntryController.IstioClient.NetworkingV1alpha3().ServiceEntries(namespace).Create(ctx, obj, v12.CreateOptions{})
op = "Add"
log.Infof(LogFormat+" SE=%s", op, "ServiceEntry", obj.Name, rc.ClusterID, "New SE", obj.Spec.String())
} else {
//se will be created if endpoints are valid, in case they are not valid se will be created with just valid endpoints
if len(obj.Spec.Endpoints) > 0 {
obj.Namespace = namespace
obj.ResourceVersion = ""
_, err = rc.ServiceEntryController.IstioClient.NetworkingV1alpha3().ServiceEntries(namespace).Create(ctx, obj, v12.CreateOptions{})
log.Infof(LogFormat+" SE=%s", op, "ServiceEntry", obj.Name, rc.ClusterID, "New SE", obj.Spec.String())
} else {
log.Errorf(LogFormat+" SE=%s", op, "ServiceEntry", obj.Name, rc.ClusterID, "Creation of SE skipped as endpoints are not valid", obj.Spec.String())
}
} else if areEndpointsValid { //update will happen only when all the endpoints are valid
exist.Labels = obj.Labels
exist.Annotations = obj.Annotations
op = "Update"
Expand All @@ -554,7 +579,6 @@ func addUpdateServiceEntry(ctx context.Context, obj *v1alpha3.ServiceEntry, exis
exist.Spec = obj.Spec
_, err = rc.ServiceEntryController.IstioClient.NetworkingV1alpha3().ServiceEntries(namespace).Update(ctx, exist, v12.UpdateOptions{})
}

}

if err != nil {
Expand Down
155 changes: 155 additions & 0 deletions admiral/pkg/clusters/handler_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1617,6 +1617,26 @@ func TestAddUpdateServiceEntry(t *testing.T) {
},
}

invalidEndpoint := v1alpha3.ServiceEntry{
Hosts: []string{"e2e.test-service.mesh"},
Addresses: []string{"240.10.1.1"},
Ports: []*v1alpha3.Port{{Number: uint32(common.DefaultServiceEntryPort),
Name: "http", Protocol: "http"}},
Location: v1alpha3.ServiceEntry_MESH_INTERNAL,
Resolution: v1alpha3.ServiceEntry_DNS,
SubjectAltNames: []string{"spiffe://prefix/my-first-service"},
Endpoints: []*v1alpha3.WorkloadEntry{
{Address: "dummy.admiral.global", Ports: map[string]uint32{"http": 0}, Locality: "us-west-2"},
{Address: "test.admiral.global", Ports: map[string]uint32{"http": 0}, Locality: "us-east-2"},
},
}

invalidEndpointSe := &v1alpha32.ServiceEntry{
ObjectMeta: metaV1.ObjectMeta{Name: "se3", Namespace: "namespace"},
//nolint
Spec: invalidEndpoint,
}

newSeOneEndpoint := &v1alpha32.ServiceEntry{
ObjectMeta: metaV1.ObjectMeta{Name: "se1", Namespace: "namespace"},
//nolint
Expand Down Expand Up @@ -1673,6 +1693,13 @@ func TestAddUpdateServiceEntry(t *testing.T) {
oldSe: oldSeTwoEndpoints,
skipDestructive: false,
},
{
name: "Should create an SE with one endpoint",
rc: rcNotInWarmupPhase,
newSe: invalidEndpointSe,
oldSe: nil,
skipDestructive: false,
},
}

//Run the test for every provided case
Expand All @@ -1693,3 +1720,131 @@ func TestAddUpdateServiceEntry(t *testing.T) {
})
}
}

func TestValidateServiceEntryEndpoints(t *testing.T) {

twoValidEndpoints := []*v1alpha3.WorkloadEntry{
{Address: "valid1.admiral.global", Ports: map[string]uint32{"http": 0}, Locality: "us-west-2"},
{Address: "valid2.admiral.global", Ports: map[string]uint32{"http": 0}, Locality: "us-east-2"},
}

oneValidEndpoints := []*v1alpha3.WorkloadEntry{
{Address: "valid1.admiral.global", Ports: map[string]uint32{"http": 0}, Locality: "us-west-2"},
}

dummyEndpoints := []*v1alpha3.WorkloadEntry{
{Address: "dummy.admiral.global", Ports: map[string]uint32{"http": 0}, Locality: "us-west-2"},
}

validAndInvalidEndpoints := []*v1alpha3.WorkloadEntry{
{Address: "dummy.admiral.global", Ports: map[string]uint32{"http": 0}, Locality: "us-west-2"},
{Address: "valid2.admiral.global", Ports: map[string]uint32{"http": 0}, Locality: "us-east-2"},
}

twoValidEndpointsSe := &v1alpha32.ServiceEntry{
ObjectMeta: metaV1.ObjectMeta{Name: "twoValidEndpointsSe", Namespace: "namespace"},
Spec: v1alpha3.ServiceEntry{
Hosts: []string{"e2e.my-first-service.mesh"},
Addresses: []string{"240.10.1.1"},
Ports: []*v1alpha3.Port{{Number: uint32(common.DefaultServiceEntryPort),
Name: "http", Protocol: "http"}},
Location: v1alpha3.ServiceEntry_MESH_INTERNAL,
Resolution: v1alpha3.ServiceEntry_DNS,
SubjectAltNames: []string{"spiffe://prefix/my-first-service"},
Endpoints: twoValidEndpoints,
},
}

oneValidEndpointsSe := &v1alpha32.ServiceEntry{
ObjectMeta: metaV1.ObjectMeta{Name: "twoValidEndpointsSe", Namespace: "namespace"},
Spec: v1alpha3.ServiceEntry{
Hosts: []string{"e2e.my-first-service.mesh"},
Addresses: []string{"240.10.1.1"},
Ports: []*v1alpha3.Port{{Number: uint32(common.DefaultServiceEntryPort),
Name: "http", Protocol: "http"}},
Location: v1alpha3.ServiceEntry_MESH_INTERNAL,
Resolution: v1alpha3.ServiceEntry_DNS,
SubjectAltNames: []string{"spiffe://prefix/my-first-service"},
Endpoints: oneValidEndpoints,
},
}

dummyEndpointsSe := &v1alpha32.ServiceEntry{
ObjectMeta: metaV1.ObjectMeta{Name: "twoValidEndpointsSe", Namespace: "namespace"},
Spec: v1alpha3.ServiceEntry{
Hosts: []string{"e2e.my-first-service.mesh"},
Addresses: []string{"240.10.1.1"},
Ports: []*v1alpha3.Port{{Number: uint32(common.DefaultServiceEntryPort),
Name: "http", Protocol: "http"}},
Location: v1alpha3.ServiceEntry_MESH_INTERNAL,
Resolution: v1alpha3.ServiceEntry_DNS,
SubjectAltNames: []string{"spiffe://prefix/my-first-service"},
Endpoints: dummyEndpoints,
},
}

validAndInvalidEndpointsSe := &v1alpha32.ServiceEntry{
ObjectMeta: metaV1.ObjectMeta{Name: "twoValidEndpointsSe", Namespace: "namespace"},
Spec: v1alpha3.ServiceEntry{
Hosts: []string{"e2e.my-first-service.mesh"},
Addresses: []string{"240.10.1.1"},
Ports: []*v1alpha3.Port{{Number: uint32(common.DefaultServiceEntryPort),
Name: "http", Protocol: "http"}},
Location: v1alpha3.ServiceEntry_MESH_INTERNAL,
Resolution: v1alpha3.ServiceEntry_DNS,
SubjectAltNames: []string{"spiffe://prefix/my-first-service"},
Endpoints: validAndInvalidEndpoints,
},
}

//Struct of test case info. Name is required.
testCases := []struct {
name string
serviceEntry *v1alpha32.ServiceEntry
expectedAreEndpointsValid bool
expectedValidEndpoints []*v1alpha3.WorkloadEntry
}{
{
name: "Validate SE with dummy endpoint",
serviceEntry: dummyEndpointsSe,
expectedAreEndpointsValid: false,
expectedValidEndpoints: []*v1alpha3.WorkloadEntry{},
},
{
name: "Validate SE with valid endpoint",
serviceEntry: oneValidEndpointsSe,
expectedAreEndpointsValid: true,
expectedValidEndpoints: []*v1alpha3.WorkloadEntry{{Address: "valid1.admiral.global", Ports: map[string]uint32{"http": 0}, Locality: "us-west-2"}},
},
{
name: "Validate endpoint with multiple valid endpoints",
serviceEntry: twoValidEndpointsSe,
expectedAreEndpointsValid: true,
expectedValidEndpoints: []*v1alpha3.WorkloadEntry{
{Address: "valid1.admiral.global", Ports: map[string]uint32{"http": 0}, Locality: "us-west-2"},
{Address: "valid2.admiral.global", Ports: map[string]uint32{"http": 0}, Locality: "us-east-2"}},
},
{
name: "Validate endpoint with mix of valid and dummy endpoints",
serviceEntry: validAndInvalidEndpointsSe,
expectedAreEndpointsValid: false,
expectedValidEndpoints: []*v1alpha3.WorkloadEntry{
{Address: "valid2.admiral.global", Ports: map[string]uint32{"http": 0}, Locality: "us-east-2"}},
},
}

//Run the test for every provided case
for _, c := range testCases {
t.Run(c.name, func(t *testing.T) {
areValidEndpoints := validateAndProcessServiceEntryEndpoints(c.serviceEntry)

if areValidEndpoints != c.expectedAreEndpointsValid {
t.Errorf("Failed. Got %v, expected %v", areValidEndpoints, c.expectedAreEndpointsValid)
}

if len(c.serviceEntry.Spec.Endpoints) != len(c.expectedValidEndpoints) {
t.Errorf("Failed. Got %v, expected %v", len(c.serviceEntry.Spec.Endpoints), len(c.expectedValidEndpoints))
}
})
}
}
11 changes: 10 additions & 1 deletion admiral/pkg/clusters/serviceentry.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -462,7 +462,16 @@ func AddServiceEntriesWithDr(ctx context.Context, rr *RemoteRegistry, sourceClus
oldDestinationRule = nil
}

if len(seDr.ServiceEntry.Endpoints) == 0 {
var deleteOldServiceEntry = false
if oldServiceEntry != nil {
areEndpointsValid := validateAndProcessServiceEntryEndpoints(oldServiceEntry)
if !areEndpointsValid && len(oldServiceEntry.Spec.Endpoints) == 0 {
deleteOldServiceEntry = true
}
}

//clean service entry in case no endpoints are configured or if all the endpoints are invalid
if (len(seDr.ServiceEntry.Endpoints) == 0) || deleteOldServiceEntry {
deleteServiceEntry(ctx, oldServiceEntry, syncNamespace, rc)
cache.SeClusterCache.Delete(seDr.ServiceEntry.Hosts[0])
// after deleting the service entry, destination rule also need to be deleted if the service entry host no longer exists
Expand Down
20 changes: 19 additions & 1 deletion admiral/pkg/clusters/serviceentry_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,17 +79,33 @@ func TestAddServiceEntriesWithDr(t *testing.T) {
Endpoints: []*istionetworkingv1alpha3.WorkloadEntry{},
}

dummyEndpointSe := istionetworkingv1alpha3.ServiceEntry{
Hosts: []string{"dev.dummy.global"},
Endpoints: []*istionetworkingv1alpha3.WorkloadEntry{
{Address: "dummy.admiral.global", Ports: map[string]uint32{"https": 80}, Labels: map[string]string{}, Network: "mesh1", Locality: "us-west", Weight: 100},
},
}

seConfig := v1alpha3.ServiceEntry{
//nolint
Spec: se,
}
seConfig.Name = "se1"
seConfig.Namespace = "admiral-sync"

dummySeConfig := v1alpha3.ServiceEntry{
//nolint
Spec: dummyEndpointSe,
}
dummySeConfig.Name = "dummySe"
dummySeConfig.Namespace = "admiral-sync"

ctx := context.Background()

fakeIstioClient := istiofake.NewSimpleClientset()
fakeIstioClient.NetworkingV1alpha3().ServiceEntries("admiral-sync").Create(ctx, &seConfig, v12.CreateOptions{})
fakeIstioClient.NetworkingV1alpha3().ServiceEntries("admiral-sync").Create(ctx, &dummySeConfig, v12.CreateOptions{})

rc := &RemoteController{
ServiceEntryController: &istio.ServiceEntryController{
IstioClient: fakeIstioClient,
Expand All @@ -104,10 +120,12 @@ func TestAddServiceEntriesWithDr(t *testing.T) {
},
}
rr := NewRemoteRegistry(nil, common.AdmiralParams{})
rr.PutRemoteController("c1", rc)
rr.PutRemoteController("cl1", rc)
rr.AdmiralCache = &admiralCache
AddServiceEntriesWithDr(ctx, rr, map[string]string{"cl1": "cl1"}, map[string]*istionetworkingv1alpha3.ServiceEntry{"se1": &se})
AddServiceEntriesWithDr(ctx, rr, map[string]string{"cl1": "cl1"}, map[string]*istionetworkingv1alpha3.ServiceEntry{"se1": &emptyEndpointSe})
AddServiceEntriesWithDr(ctx, rr, map[string]string{"cl1": "cl1"}, map[string]*istionetworkingv1alpha3.ServiceEntry{"dummySe": &dummyEndpointSe})

}

func TestCreateSeAndDrSetFromGtp(t *testing.T) {
Expand Down

0 comments on commit b371d75

Please sign in to comment.