From 5c054e20936370adad1e5bb304171db3838e2a5d Mon Sep 17 00:00:00 2001 From: vjoshi3 Date: Tue, 30 Aug 2022 00:32:46 -0700 Subject: [PATCH] Avoid creation of service entry with dummy endpoints --- admiral/pkg/clusters/handler.go | 36 +++++++-- admiral/pkg/clusters/handler_test.go | 99 +++++++++++++++++++++++ admiral/pkg/clusters/serviceentry.go | 10 ++- admiral/pkg/clusters/serviceentry_test.go | 20 ++++- 4 files changed, 157 insertions(+), 8 deletions(-) diff --git a/admiral/pkg/clusters/handler.go b/admiral/pkg/clusters/handler.go index 1d160728d..5d68c9f27 100644 --- a/admiral/pkg/clusters/handler.go +++ b/admiral/pkg/clusters/handler.go @@ -504,6 +504,21 @@ func addUpdateVirtualService(ctx context.Context, obj *v1alpha3.VirtualService, } } +func validateServiceEntryEndpoints(endpoints []*v1alpha32.WorkloadEntry) (bool, []*v1alpha32.WorkloadEntry) { + var areEndpointsValid = true + validEndpoints := make([]*v1alpha32.WorkloadEntry, 0) + + for _, endpoint := range endpoints { + if endpoint.Address == "dummy.admiral.global" { + areEndpointsValid = false + } else { + validEndpoints = append(validEndpoints, endpoint) + } + } + + return areEndpointsValid, validEndpoints +} + func addUpdateServiceEntry(ctx context.Context, obj *v1alpha3.ServiceEntry, exist *v1alpha3.ServiceEntry, namespace string, rc *RemoteController) { var ( err error @@ -515,13 +530,23 @@ func addUpdateServiceEntry(ctx context.Context, obj *v1alpha3.ServiceEntry, exis obj.Annotations = map[string]string{} } obj.Annotations["app.kubernetes.io/created-by"] = "admiral" + + areEndpointsValid, validEndpoints := validateServiceEntryEndpoints(obj.Spec.Endpoints) + log.Infof("type=ServiceEntry, name=%s, endpointsValid=%v, numberOfValidEndpoints=%d", obj.Name, areEndpointsValid, len(validEndpoints)) + 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 areEndpointsValid || (!areEndpointsValid && len(validEndpoints) > 0) { + obj.Spec.Endpoints = validEndpoints + 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" @@ -537,7 +562,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 { diff --git a/admiral/pkg/clusters/handler_test.go b/admiral/pkg/clusters/handler_test.go index 431e957e4..66070ac6d 100644 --- a/admiral/pkg/clusters/handler_test.go +++ b/admiral/pkg/clusters/handler_test.go @@ -1589,6 +1589,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 @@ -1645,6 +1665,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 @@ -1665,3 +1692,75 @@ 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"}, + } + + //Struct of test case info. Name is required. + testCases := []struct { + name string + endpoints []*v1alpha3.WorkloadEntry + expectedAreEndpointsValid bool + expectedValidEndpoints []*v1alpha3.WorkloadEntry + }{ + { + name: "Validate SE with dummy endpoint", + endpoints: dummyEndpoints, + expectedAreEndpointsValid: false, + expectedValidEndpoints: []*v1alpha3.WorkloadEntry{}, + }, + { + name: "Validate SE with valid endpoint", + endpoints: oneValidEndpoints, + 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", + endpoints: twoValidEndpoints, + 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", + endpoints: validAndInvalidEndpoints, + 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, validEndpoints := validateServiceEntryEndpoints(c.endpoints) + + if areValidEndpoints != c.expectedAreEndpointsValid { + t.Errorf("Failed. Got %v, expected %v", areValidEndpoints, c.expectedAreEndpointsValid) + } + + if len(validEndpoints) != len(c.expectedValidEndpoints) { + t.Errorf("Failed. Got %v, expected %v", len(validEndpoints), len(c.expectedValidEndpoints)) + } + }) + } +} diff --git a/admiral/pkg/clusters/serviceentry.go b/admiral/pkg/clusters/serviceentry.go index 6f99b46c8..0786b91b9 100644 --- a/admiral/pkg/clusters/serviceentry.go +++ b/admiral/pkg/clusters/serviceentry.go @@ -462,7 +462,15 @@ func AddServiceEntriesWithDr(ctx context.Context, rr *RemoteRegistry, sourceClus oldDestinationRule = nil } - if len(seDr.ServiceEntry.Endpoints) == 0 { + var areEndpointsValid = true + var validEndpoints = make([]*networking.WorkloadEntry, 0) + if oldServiceEntry != nil { + areEndpointsValid, validEndpoints := validateServiceEntryEndpoints(oldServiceEntry.Spec.Endpoints) + log.Infof("type=ServiceEntry, name=%s, endpointsValid=%v, numberOfValidEndpoints=%d", oldServiceEntry.Name, areEndpointsValid, len(validEndpoints)) + } + + //clean service entry in case no endpoints are configured or if all the endpoints are invalid + if (len(seDr.ServiceEntry.Endpoints) == 0) || (!areEndpointsValid && len(validEndpoints) == 0) { 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 diff --git a/admiral/pkg/clusters/serviceentry_test.go b/admiral/pkg/clusters/serviceentry_test.go index 74456d3b6..4a8add8b9 100644 --- a/admiral/pkg/clusters/serviceentry_test.go +++ b/admiral/pkg/clusters/serviceentry_test.go @@ -79,6 +79,13 @@ 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, @@ -86,10 +93,19 @@ func TestAddServiceEntriesWithDr(t *testing.T) { 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, @@ -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) {