From afb039aae3be01f2a48708ee737328d0aeffc7d6 Mon Sep 17 00:00:00 2001 From: kpharasi Date: Wed, 14 Aug 2024 14:28:39 -0700 Subject: [PATCH 1/5] remove implementation of deleteWorkloadData Signed-off-by: kpharasi --- admiral/pkg/clusters/serviceentry.go | 38 ---------------------------- 1 file changed, 38 deletions(-) diff --git a/admiral/pkg/clusters/serviceentry.go b/admiral/pkg/clusters/serviceentry.go index ba583a5b1..8813dbb0d 100644 --- a/admiral/pkg/clusters/serviceentry.go +++ b/admiral/pkg/clusters/serviceentry.go @@ -1602,44 +1602,6 @@ func getDNSPrefixFromServiceEntry(seDR *SeDrTuple) string { } func deleteWorkloadData(clusterName, env string, serviceEntry *v1alpha3.ServiceEntry, rr *RemoteRegistry, ctxLogger *logrus.Entry) error { - start := time.Now() - - if serviceEntry == nil { - return fmt.Errorf("provided service entry is nil") - } - - if reflect.DeepEqual(serviceEntry.Spec, networking.ServiceEntry{}) { - return fmt.Errorf("serviceentry %s has a nil spec", serviceEntry.ObjectMeta.Name) - } - - if serviceEntry.Spec.Hosts == nil { - return fmt.Errorf("hosts are not defined in serviceentry: %s", serviceEntry.ObjectMeta.Name) - } - - if len(serviceEntry.Spec.Hosts) == 0 { - return fmt.Errorf("0 hosts found in serviceentry: %s", serviceEntry.ObjectMeta.Name) - } - - if rr.AdmiralDatabaseClient == nil { - return fmt.Errorf("dynamodb client for workload data table is not initialized") - } - - workloadDataToDelete := WorkloadData{ - AssetAlias: serviceEntry.Annotations[common.GetWorkloadIdentifier()], - Endpoint: serviceEntry.Spec.Hosts[0], - } - - err := rr.AdmiralDatabaseClient.Delete(workloadDataToDelete, ctxLogger) - if err != nil { - return err - } - - _, ok := rr.AdmiralCache.DynamoDbEndpointUpdateCache.Load(workloadDataToDelete.Endpoint) - if ok { - rr.AdmiralCache.DynamoDbEndpointUpdateCache.Delete(workloadDataToDelete.Endpoint) - } - - util.LogElapsedTimeSince("DeleteEndpointRecord", serviceEntry.Spec.Hosts[0], env, clusterName, start) return nil } From bd25d5d7e2aa7b9d646564390fd5ea4c7bc1a99e Mon Sep 17 00:00:00 2001 From: kpharasi Date: Wed, 14 Aug 2024 14:29:12 -0700 Subject: [PATCH 2/5] remove implementation of storeWorkloadData Signed-off-by: kpharasi --- admiral/pkg/clusters/serviceentry.go | 31 ---------------------------- 1 file changed, 31 deletions(-) diff --git a/admiral/pkg/clusters/serviceentry.go b/admiral/pkg/clusters/serviceentry.go index 8813dbb0d..ae79ff1e6 100644 --- a/admiral/pkg/clusters/serviceentry.go +++ b/admiral/pkg/clusters/serviceentry.go @@ -1683,37 +1683,6 @@ func pushWorkloadDataToDynamodbTable(workloadDataToUpdate WorkloadData, endpoint func storeWorkloadData(clusterName string, serviceEntry *v1alpha3.ServiceEntry, globalTrafficPolicy *v1.GlobalTrafficPolicy, additionalEndpoints []string, rr *RemoteRegistry, ctxLogger *logrus.Entry, dr networking.DestinationRule, isSuccess bool) error { - - start := time.Now() - - if serviceEntry == nil { - return fmt.Errorf("provided service entry is nil") - } - - if reflect.DeepEqual(serviceEntry.Spec, networking.ServiceEntry{}) { - return fmt.Errorf("serviceentry %s has a nil spec", serviceEntry.ObjectMeta.Name) - } - - if serviceEntry.Spec.Hosts == nil { - return fmt.Errorf("hosts are not defined in serviceentry: %s", serviceEntry.ObjectMeta.Name) - } - - if len(serviceEntry.Spec.Hosts) == 0 { - return fmt.Errorf("0 hosts found in serviceentry: %s", serviceEntry.ObjectMeta.Name) - } - - if rr.AdmiralDatabaseClient == nil { - return fmt.Errorf("dynamodb client for workload data table is not initialized") - } - - //get workload data based on service entry, globaltrafficpolicy and additional endpoints - workloadData := getWorkloadData(ctxLogger, serviceEntry, globalTrafficPolicy, additionalEndpoints, dr, clusterName, isSuccess) - - err := pushWorkloadDataToDynamodbTable(workloadData, serviceEntry.Spec.Hosts[0], clusterName, rr, ctxLogger) - util.LogElapsedTimeSinceTask(ctxLogger, "UpdateEndpointRecord", serviceEntry.Spec.Hosts[0], "", clusterName, "", start) - if err != nil { - return err - } return nil } From a61f15b3e9d88cf67e8e07e68634102c436d5ebf Mon Sep 17 00:00:00 2001 From: kpharasi Date: Wed, 14 Aug 2024 14:30:26 -0700 Subject: [PATCH 3/5] remove test for storeWorkloadData Signed-off-by: kpharasi --- admiral/pkg/clusters/serviceentry_test.go | 132 ---------------------- 1 file changed, 132 deletions(-) diff --git a/admiral/pkg/clusters/serviceentry_test.go b/admiral/pkg/clusters/serviceentry_test.go index 17d1894a2..b20103b55 100644 --- a/admiral/pkg/clusters/serviceentry_test.go +++ b/admiral/pkg/clusters/serviceentry_test.go @@ -6319,138 +6319,6 @@ func TestDeleteEndpointDataFromDynamoDB(t *testing.T) { } } -func TestUpdateEndpointDataFromDynamoDB(t *testing.T) { - common.ResetSync() - common.InitializeConfig(admiralParamsForServiceEntryTests()) - - var se = &v1alpha3.ServiceEntry{ - //nolint - Spec: istioNetworkingV1Alpha3.ServiceEntry{ - Hosts: []string{"dev.custom.global"}, - Endpoints: []*istioNetworkingV1Alpha3.WorkloadEntry{ - { - Address: "override.svc.cluster.local", - Ports: map[string]uint32{"http": 80}, - Network: "mesh1", - Locality: "us-west", - Weight: 100, - }, - }, - }, - } - - se.Annotations = map[string]string{resourceCreatedByAnnotationLabel: resourceCreatedByAnnotationValue, common.GetWorkloadIdentifier(): "custom"} - se.Labels = map[string]string{"env": "dev"} - - var seWithNilSpec = &v1alpha3.ServiceEntry{ - ObjectMeta: metav1.ObjectMeta{ - Name: "seWithNilSpec", - }, - } - - var seWithNilSpecHosts = &v1alpha3.ServiceEntry{ - ObjectMeta: metav1.ObjectMeta{ - Name: "seWithNilSpecHosts", - }, - Spec: istioNetworkingV1Alpha3.ServiceEntry{ - Addresses: []string{}, - }, - } - - var seWithEmptySpecHosts = &v1alpha3.ServiceEntry{ - ObjectMeta: metav1.ObjectMeta{ - Name: "seWithEmptySpecHosts", - }, - Spec: istioNetworkingV1Alpha3.ServiceEntry{ - Hosts: []string{}, - }, - } - - rr1, _ := InitAdmiral(context.Background(), admiralParamsForServiceEntryTests()) - rr1.AdmiralDatabaseClient = nil - - rr2, _ := InitAdmiral(context.Background(), admiralParamsForServiceEntryTests()) - rr2.AdmiralDatabaseClient = &mockDatabaseClientWithError{} - - testCases := []struct { - name string - serviceEntry *v1alpha3.ServiceEntry - rr *RemoteRegistry - expectedErr bool - expectedErrMsg string - }{ - { - name: "Given nil serviceentry object and nil remote registry " + - "When storeWorkloadData is called, " + - "Then it should return an error", - serviceEntry: nil, - rr: nil, - expectedErr: true, - expectedErrMsg: "provided service entry is nil", - }, - { - name: "Given serviceentry object with nil spec" + - "When storeWorkloadData is called, " + - "Then it should return an error", - serviceEntry: seWithNilSpec, - rr: nil, - expectedErr: true, - expectedErrMsg: "serviceentry seWithNilSpec has a nil spec", - }, - { - name: "Given serviceentry object with nil spec hosts and nil remote registry " + - "When storeWorkloadData is called, " + - "Then it should return an error", - serviceEntry: seWithNilSpecHosts, - rr: nil, - expectedErr: true, - expectedErrMsg: "hosts are not defined in serviceentry: seWithNilSpecHosts", - }, - { - name: "Given nil serviceentry object with empty spec hosts and nil remote registry " + - "When storeWorkloadData is called, " + - "Then it should return an error", - serviceEntry: seWithEmptySpecHosts, - rr: nil, - expectedErr: true, - expectedErrMsg: "0 hosts found in serviceentry: seWithEmptySpecHosts", - }, - { - name: "Given serviceentry object and remote registry with nil admiral database client" + - "When storeWorkloadData is called, " + - "Then it should return an error", - serviceEntry: se, - rr: rr1, - expectedErr: true, - expectedErrMsg: "dynamodb client for workload data table is not initialized", - }, - { - name: "Given serviceentry object and remote registry with admiral database client with errors" + - "When storeWorkloadData is called, " + - "Then it should return an error", - serviceEntry: se, - rr: rr2, - expectedErr: true, - expectedErrMsg: "failed to update workloadData", - }, - } - - var ctxLogger = logrus.WithFields(logrus.Fields{ - "type": "storeWorkloadData", - }) - - for _, c := range testCases { - t.Run(c.name, func(t *testing.T) { - err := storeWorkloadData("testSourceCluster", c.serviceEntry, nil, []string{}, c.rr, ctxLogger, istioNetworkingV1Alpha3.DestinationRule{}, true) - if err == nil && c.expectedErr { - assert.Fail(t, "expected error to be returned") - } else if c.expectedErrMsg != "" && c.expectedErrMsg != err.Error() { - assert.Failf(t, "actual and expected error do not match. actual - %s, expected %s", err.Error(), c.expectedErrMsg) - } - }) - } -} - func TestDeployRolloutMigration(t *testing.T) { setupForServiceEntryTests() var ( From 6696804894a52511a6b9bfce8bbe810327f8edbb Mon Sep 17 00:00:00 2001 From: kpharasi Date: Wed, 14 Aug 2024 14:30:51 -0700 Subject: [PATCH 4/5] remove test for deleteWorkloadData Signed-off-by: kpharasi --- admiral/pkg/clusters/serviceentry_test.go | 127 ---------------------- 1 file changed, 127 deletions(-) diff --git a/admiral/pkg/clusters/serviceentry_test.go b/admiral/pkg/clusters/serviceentry_test.go index b20103b55..09ae05575 100644 --- a/admiral/pkg/clusters/serviceentry_test.go +++ b/admiral/pkg/clusters/serviceentry_test.go @@ -6192,133 +6192,6 @@ func (mockDatabaseClientWithError) Get(env string, identity string) (interface{} return nil, fmt.Errorf("failed to get workloadData items") } -func TestDeleteEndpointDataFromDynamoDB(t *testing.T) { - common.ResetSync() - common.InitializeConfig(admiralParamsForServiceEntryTests()) - - var se = &v1alpha3.ServiceEntry{ - //nolint - Spec: istioNetworkingV1Alpha3.ServiceEntry{ - Hosts: []string{"dev.custom.global"}, - Endpoints: []*istioNetworkingV1Alpha3.WorkloadEntry{ - { - Address: "override.svc.cluster.local", - Ports: map[string]uint32{"http": 80}, - Network: "mesh1", - Locality: "us-west", - Weight: 100, - }, - }, - }, - } - - var seWithNilSpec = &v1alpha3.ServiceEntry{ - ObjectMeta: metav1.ObjectMeta{ - Name: "seWithNilSpec", - }, - } - - var seWithNilSpecHosts = &v1alpha3.ServiceEntry{ - ObjectMeta: metav1.ObjectMeta{ - Name: "seWithNilSpecHosts", - }, - Spec: istioNetworkingV1Alpha3.ServiceEntry{ - Addresses: []string{}, - }, - } - - var seWithEmptySpecHosts = &v1alpha3.ServiceEntry{ - ObjectMeta: metav1.ObjectMeta{ - Name: "seWithEmptySpecHosts", - }, - Spec: istioNetworkingV1Alpha3.ServiceEntry{ - Hosts: []string{}, - }, - } - - se.Annotations = map[string]string{resourceCreatedByAnnotationLabel: resourceCreatedByAnnotationValue, common.GetWorkloadIdentifier(): "custom"} - se.Labels = map[string]string{"env": "dev"} - - rr1, _ := InitAdmiral(context.Background(), admiralParamsForServiceEntryTests()) - rr1.AdmiralDatabaseClient = nil - - rr2, _ := InitAdmiral(context.Background(), admiralParamsForServiceEntryTests()) - rr2.AdmiralDatabaseClient = &mockDatabaseClientWithError{} - - testCases := []struct { - name string - serviceEntry *v1alpha3.ServiceEntry - rr *RemoteRegistry - expectedErr bool - expectedErrMsg string - }{ - { - name: "Given nil serviceentry object and nil remote registry " + - "When deleteWorkloadData is called, " + - "Then it should return an error", - serviceEntry: nil, - rr: nil, - expectedErr: true, - expectedErrMsg: "provided service entry is nil", - }, - { - name: "Given serviceentry object with nil spec" + - "When deleteWorkloadData is called, " + - "Then it should return an error", - serviceEntry: seWithNilSpec, - rr: nil, - expectedErr: true, - expectedErrMsg: "serviceentry seWithNilSpec has a nil spec", - }, - { - name: "Given serviceentry object with nil spec hosts and nil remote registry " + - "When deleteWorkloadData is called, " + - "Then it should return an error", - serviceEntry: seWithNilSpecHosts, - rr: nil, - expectedErr: true, - expectedErrMsg: "hosts are not defined in serviceentry: seWithNilSpecHosts", - }, - { - name: "Given nil serviceentry object with empty spec hosts and nil remote registry " + - "When deleteWorkloadData is called, " + - "Then it should return an error", - serviceEntry: seWithEmptySpecHosts, - rr: nil, - expectedErr: true, - expectedErrMsg: "0 hosts found in serviceentry: seWithEmptySpecHosts", - }, - { - name: "Given serviceentry object and remote registry with nil admiral database client" + - "When deleteWorkloadData is called, " + - "Then it should return an error", - serviceEntry: se, - rr: rr1, - expectedErr: true, - expectedErrMsg: "dynamodb client for workload data table is not initialized", - }, - { - name: "Given serviceentry object and remote registry with admiral database client with errors" + - "When deleteWorkloadData is called, " + - "Then it should return an error", - serviceEntry: se, - rr: rr2, - expectedErr: true, - expectedErrMsg: "failed to delete workloadData", - }, - } - for _, c := range testCases { - t.Run(c.name, func(t *testing.T) { - err := deleteWorkloadData("testSourceCluster", "testEnv", c.serviceEntry, c.rr, nil) - if err == nil && c.expectedErr { - assert.Fail(t, "expected error to be returned") - } else if c.expectedErrMsg != "" && c.expectedErrMsg != err.Error() { - assert.Failf(t, "actual and expected error do not match. actual - %s, expected %s", err.Error(), c.expectedErrMsg) - } - }) - } -} - func TestDeployRolloutMigration(t *testing.T) { setupForServiceEntryTests() var ( From 7867c4f22aef8019f9539169fcf6d1a2f6ae75dc Mon Sep 17 00:00:00 2001 From: kpharasi Date: Wed, 14 Aug 2024 14:49:57 -0700 Subject: [PATCH 5/5] add presteps to TestDeployRolloutMigration Signed-off-by: kpharasi --- admiral/pkg/clusters/serviceentry_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/admiral/pkg/clusters/serviceentry_test.go b/admiral/pkg/clusters/serviceentry_test.go index 09ae05575..9a78e1ea8 100644 --- a/admiral/pkg/clusters/serviceentry_test.go +++ b/admiral/pkg/clusters/serviceentry_test.go @@ -6193,7 +6193,8 @@ func (mockDatabaseClientWithError) Get(env string, identity string) (interface{} } func TestDeployRolloutMigration(t *testing.T) { - setupForServiceEntryTests() + common.ResetSync() + common.InitializeConfig(admiralParamsForServiceEntryTests()) var ( env = "test" stop = make(chan struct{})