MESH-6858: Improve VS deletion logic (#957) (#432)

* MESH-6858- improve VirtualService deletion handling in
virtualservice_handler

---------

### Checklist
🚨 Please review this repository's [contribution
guidelines](./CONTRIBUTING.md).

- [ ] I've read and agree to the project's contribution guidelines.
- [ ] I'm requesting to **pull a topic/feature/bugfix branch**.
- [ ] I checked that my code additions will pass code linting checks and
unit tests.
- [ ] I updated unit and integration tests (if applicable).
- [ ] I'm ready to notify the team of this contribution.

### Description
What does this change do and why?

[Link to related ISSUE]

Thank you!

Co-authored-by: Punakshi Chaand <[email protected]>
Co-authored-by: pchaand <[email protected]>

Author: 3 people

admiral/pkg/clusters/serviceentry.go

@@ -2320,7 +2320,7 @@ func deleteAdditionalEndpoints(ctxLogger *logrus.Entry, ctx context.Context, rc
 			return nil
 		}
 
-		err = deleteVirtualService(ctx, vsToDelete, namespace, rc)
+		err = deleteVirtualService(ctx, vsToDelete.Name, namespace, rc)
 		if err != nil {
 			ctxLogger.Errorf(LogErrFormat, "Delete", "VirtualService", vsToDelete.Name, rc.ClusterID, err)
 			return err

admiral/pkg/clusters/virtualservice_handler.go

@@ -2,6 +2,7 @@ package clusters
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"regexp"
 	"strings"
@@ -21,6 +22,16 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
+type IsVSAlreadyDeletedErr struct {
+	msg string
+}
+
+var vsAlreadyDeletedMsg = "either VirtualService was already deleted, or it never existed"
+
+func (e *IsVSAlreadyDeletedErr) Error() string {
+	return vsAlreadyDeletedMsg
+}
+
 // NewVirtualServiceHandler returns a new instance of VirtualServiceHandler after verifying
 // the required properties are set correctly
 func NewVirtualServiceHandler(remoteRegistry *RemoteRegistry, clusterID string) (*VirtualServiceHandler, error) {
@@ -445,9 +456,10 @@ func syncVirtualServiceToDependentCluster(
 		// Best effort delete for existing virtual service with old name
 		_ = rc.VirtualServiceController.IstioClient.NetworkingV1alpha3().VirtualServices(syncNamespace).Delete(ctx, virtualService.Name, metav1.DeleteOptions{})
 
-		err := rc.VirtualServiceController.IstioClient.NetworkingV1alpha3().VirtualServices(syncNamespace).Delete(ctx, vSName, metav1.DeleteOptions{})
+		err := deleteVirtualService(ctx, vSName, syncNamespace, rc)
 		if err != nil {
-			if k8sErrors.IsNotFound(err) {
+			var vsAlreadyDeletedErr *IsVSAlreadyDeletedErr
+			if errors.As(err, &vsAlreadyDeletedErr) {
 				ctxLogger.Infof(LogFormat, "Delete", "VirtualService", vSName, cluster, "Either VirtualService was already deleted, or it never existed")
 				return nil
 			}
@@ -572,9 +584,10 @@ func syncVirtualServiceToRemoteCluster(
 		// Best effort delete for existing virtual service with old name
 		_ = rc.VirtualServiceController.IstioClient.NetworkingV1alpha3().VirtualServices(syncNamespace).Delete(ctx, virtualService.Name, metav1.DeleteOptions{})
 
-		err := rc.VirtualServiceController.IstioClient.NetworkingV1alpha3().VirtualServices(syncNamespace).Delete(ctx, vSName, metav1.DeleteOptions{})
+		err := deleteVirtualService(ctx, vSName, syncNamespace, rc)
 		if err != nil {
-			if k8sErrors.IsNotFound(err) {
+			var vsAlreadyDeletedErr *IsVSAlreadyDeletedErr
+			if errors.As(err, &vsAlreadyDeletedErr) {
 				ctxLogger.Infof(LogFormat, "Delete", common.VirtualServiceResourceType, vSName, cluster, "Either VirtualService was already deleted, or it never existed")
 				return nil
 			}
@@ -814,16 +827,19 @@ func createVirtualServiceSkeleton(vs networkingV1Alpha3.VirtualService, name str
 	return &v1alpha3.VirtualService{Spec: vs, ObjectMeta: metaV1.ObjectMeta{Name: name, Namespace: namespace}}
 }
 
-func deleteVirtualService(ctx context.Context, exist *v1alpha3.VirtualService, namespace string, rc *RemoteController) error {
-	if exist == nil {
-		return fmt.Errorf("the VirtualService passed was nil")
+func deleteVirtualService(ctx context.Context, vsName string, namespace string, rc *RemoteController) error {
+	err := rc.VirtualServiceController.IstioClient.NetworkingV1alpha3().VirtualServices(namespace).Delete(ctx, vsName, metaV1.DeleteOptions{})
+	if err == nil {
+		return nil
 	}
-	err := rc.VirtualServiceController.IstioClient.NetworkingV1alpha3().VirtualServices(namespace).Delete(ctx, exist.Name, metaV1.DeleteOptions{})
-	if err != nil {
+	if k8sErrors.IsNotFound(err) {
+		err = rc.VirtualServiceController.IstioClient.NetworkingV1alpha3().VirtualServices(namespace).Delete(ctx, strings.ToLower(vsName), metaV1.DeleteOptions{})
+		if err == nil {
+			return nil
+		}
 		if k8sErrors.IsNotFound(err) {
-			return fmt.Errorf("either VirtualService was already deleted, or it never existed")
+			return &IsVSAlreadyDeletedErr{vsAlreadyDeletedMsg}
 		}
-		return err
 	}
-	return nil
+	return err
 }

admiral/pkg/clusters/virtualservice_handler_test.go

@@ -1488,49 +1488,64 @@ func TestDeleteVirtualService(t *testing.T) {
 			Hosts: []string{"stage.test00.foo", "stage.test00.bar"},
 		},
 	}
+	barVS := &apiNetworkingV1Alpha3.VirtualService{
+		ObjectMeta: metaV1.ObjectMeta{
+			Name: "stage.test00.bar-vs",
+		},
+		Spec: networkingV1Alpha3.VirtualService{
+			Hosts: []string{"stage.test00.foo", "stage.test00.bar"},
+		},
+	}
 
 	validIstioClient := istioFake.NewSimpleClientset()
 	validIstioClient.NetworkingV1alpha3().VirtualServices(namespace).Create(ctx, fooVS, metaV1.CreateOptions{})
+	validIstioClient.NetworkingV1alpha3().VirtualServices(namespace).Create(ctx, barVS, metaV1.CreateOptions{})
 
 	testcases := []struct {
 		name                  string
-		virtualService        *apiNetworkingV1Alpha3.VirtualService
+		virtualServiceName    string
 		rc                    *RemoteController
 		expectedError         error
 		expectedDeletedVSName string
 	}{
 		{
-			name:           "Given virtualservice to delete, when nil VS is passed, the func should return an error",
-			virtualService: nil,
-			expectedError:  fmt.Errorf("the VirtualService passed was nil"),
+			name:               "Given virtualservice to delete, when VS passed does not exists, the func should return an error",
+			virtualServiceName: "vs-does-not-exists",
+			expectedError:      fmt.Errorf("either VirtualService was already deleted, or it never existed"),
+			rc: &RemoteController{
+				VirtualServiceController: &istio.VirtualServiceController{
+					IstioClient: validIstioClient,
+				},
+			},
 		},
 		{
-			name:           "Given virtualservice to delete, when VS passed does not exists, the func should return an error",
-			virtualService: &apiNetworkingV1Alpha3.VirtualService{ObjectMeta: metaV1.ObjectMeta{Name: "vs-does-not-exists"}},
-			expectedError:  fmt.Errorf("either VirtualService was already deleted, or it never existed"),
+			name:               "Given virtualservice to delete, when VS exists, the func should delete the VS and not return any error",
+			virtualServiceName: "stage.test00.foo-vs",
+			expectedError:      nil,
 			rc: &RemoteController{
 				VirtualServiceController: &istio.VirtualServiceController{
 					IstioClient: validIstioClient,
 				},
 			},
+			expectedDeletedVSName: "stage.test00.foo-vs",
 		},
 		{
-			name:           "Given virtualservice to delete, when VS exists, the func should delete the VS and not return any error",
-			virtualService: fooVS,
-			expectedError:  nil,
+			name:               "Given virtualservice to delete, when VS exists with capital in name, the func should delete the VS and not return any error",
+			virtualServiceName: "stage.test00.Bar-vs",
+			expectedError:      nil,
 			rc: &RemoteController{
 				VirtualServiceController: &istio.VirtualServiceController{
 					IstioClient: validIstioClient,
 				},
 			},
-			expectedDeletedVSName: "stage.test00.foo-vs",
+			expectedDeletedVSName: "stage.test00.bar-vs",
 		},
 	}
 
 	for _, tc := range testcases {
 		t.Run(tc.name, func(t *testing.T) {
 
-			err := deleteVirtualService(ctx, tc.virtualService, namespace, tc.rc)
+			err := deleteVirtualService(ctx, tc.virtualServiceName, namespace, tc.rc)
 
 			if err != nil && tc.expectedError != nil {
 				if !strings.Contains(err.Error(), tc.expectedError.Error()) {