Skip to content

Commit

Permalink
Avoid creation of service entry with dummy endpoints
Browse files Browse the repository at this point in the history
  • Loading branch information
vjoshi3 committed Aug 30, 2022
1 parent 05fc3db commit 5c054e2
Show file tree
Hide file tree
Showing 4 changed files with 157 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 @@ -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
Expand All @@ -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"
Expand All @@ -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 {
Expand Down
99 changes: 99 additions & 0 deletions admiral/pkg/clusters/handler_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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))
}
})
}
}
10 changes: 9 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,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
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 5c054e2

Please sign in to comment.