feat: rollback mechanism virtualservice patch in model endpoint (#655)

# Description
When creating/patching/deleting VirtualService for model endpoint
related action, if there's any error happened after the action is
successfully run, there's a possibility of mismatch state between the
resource state in Kubernetes vs what is being recorded in database (as
this will not be updated).

# Modifications
Changes: 
- Add `GetVirtualService` function to get the current state of
VirtualService
- Add `cleanVirtualServiceFields` function to remove not-needed field
when creating or patching resource, e.g. UUID or generation number, if
this isn't set to empty/default, the Patch/Create will not succeed
- Flow, if there's any error occur after the create/patching/delete
happened, rollback the changes in Kubernetes to previous state
  - Create -> remove the newly created VirtualService
  - Patch -> re-patch the VirtualService to previous state
- Delete -> recreate the VirtualService if previously there's an
existing one

# Tests
<!-- Besides the existing / updated automated tests, what specific
scenarios should be tested? Consider the backward compatibility of the
changes, whether corner cases are covered, etc. Please describe the
tests and check the ones that have been completed. Eg:
- [x] Deploying new and existing standard models
- [ ] Deploying PyFunc models
-->

# Checklist
- [x] Added PR label
- [x] Added unit test, integration, and/or e2e tests
- [x] Tested locally
- [ ] Updated documentation
- [ ] Update Swagger spec if the PR introduce API changes
- [ ] Regenerated Golang and Python client if the PR introduces API
changes

# Release Notes
```release-note
NONE
```

Author: bthari

api/istio/istio.go

@@ -41,6 +41,7 @@ type Config struct {
 
 // Client interface.
 type Client interface {
+	GetVirtualService(ctx context.Context, namespace, name string) (*istiov1beta1.VirtualService, error)
 	CreateVirtualService(ctx context.Context, namespace string, vs *istiov1beta1.VirtualService) (*istiov1beta1.VirtualService, error)
 	PatchVirtualService(ctx context.Context, namespace string, vs *istiov1beta1.VirtualService) (*istiov1beta1.VirtualService, error)
 	DeleteVirtualService(ctx context.Context, namespace, name string) error
@@ -71,6 +72,10 @@ func newClient(networking networkingv1beta1.NetworkingV1beta1Interface) (*client
 	}, nil
 }
 
+func (c *client) GetVirtualService(ctx context.Context, namespace, name string) (*istiov1beta1.VirtualService, error) {
+	return c.networking.VirtualServices(namespace).Get(ctx, name, metav1.GetOptions{})
+}
+
 func (c *client) CreateVirtualService(ctx context.Context, namespace string, vs *istiov1beta1.VirtualService) (*istiov1beta1.VirtualService, error) {
 	return c.networking.VirtualServices(namespace).Create(ctx, vs, metav1.CreateOptions{})
 }

api/istio/istio_test.go

@@ -19,6 +19,7 @@ import (
 	"reflect"
 	"testing"
 
+	"github.com/pkg/errors"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
 	istionetv1beta1 "istio.io/api/networking/v1beta1"
@@ -76,6 +77,83 @@ var (
 	}
 )
 
+func Test_client_GetVirtualService(t *testing.T) {
+	clientSet := istiofake.Clientset{}
+	type fields struct {
+		networking istiocliv1beta1.NetworkingV1beta1Interface
+	}
+	type args struct {
+		ctx       context.Context
+		namespace string
+		name      string
+	}
+
+	tests := []struct {
+		name     string
+		fields   fields
+		mockFunc func(m istiocliv1beta1.NetworkingV1beta1Interface)
+		args     args
+		want     *istiov1beta1.VirtualService
+		wantErr  bool
+	}{
+		{
+			name: "success",
+			fields: fields{
+				networking: clientSet.NetworkingV1beta1(),
+			},
+			mockFunc: func(mockNetworking istiocliv1beta1.NetworkingV1beta1Interface) {
+				mockVirtualService := mockNetworking.VirtualServices("default").(*istiocliv1beta1fake.FakeVirtualServices)
+				mockVirtualService.Fake.PrependReactor("get", "virtualservices", func(action ktesting.Action) (handled bool, ret runtime.Object, err error) {
+					return true, validVirtualService, nil
+				})
+			},
+			args: args{
+				ctx:       context.Background(),
+				namespace: "default",
+				name:      "valid",
+			},
+			want:    validVirtualService,
+			wantErr: false,
+		},
+		{
+			name: "not found",
+			fields: fields{
+				networking: clientSet.NetworkingV1beta1(),
+			},
+			mockFunc: func(mockNetworking istiocliv1beta1.NetworkingV1beta1Interface) {
+				mockVirtualService := mockNetworking.VirtualServices("default").(*istiocliv1beta1fake.FakeVirtualServices)
+				mockVirtualService.Fake.PrependReactor("get", "virtualservices", func(action ktesting.Action) (handled bool, ret runtime.Object, err error) {
+					return true, nil, errors.New("virtualservice not found")
+				})
+			},
+			args: args{
+				ctx:       context.Background(),
+				namespace: "default",
+				name:      "not-exist",
+			},
+			want:    nil,
+			wantErr: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			c, _ := newClient(tt.fields.networking)
+
+			tt.mockFunc(c.networking)
+
+			got, err := c.GetVirtualService(tt.args.ctx, tt.args.namespace, tt.args.name)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("client.GetVirtualService() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+			if !reflect.DeepEqual(got, tt.want) {
+				t.Errorf("client.GetVirtualService() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}
+
 func Test_client_CreateVirtualService(t *testing.T) {
 	clientSet := istiofake.Clientset{}
 	type fields struct {

api/istio/mocks/client.go

@@ -124,6 +124,16 @@ func (s *modelEndpointsService) DeployEndpoint(ctx context.Context, model *model
 		return nil, fmt.Errorf("unable to find istio client for environment: %s", endpoint.EnvironmentName)
 	}
 
+	// if there's error during VirtualService creation, we will rollback the changes, to make sure
+	// there's no hanging state of vs and to align with the database state
+	defer func() {
+		if err != nil {
+			if err := istioClient.DeleteVirtualService(context.WithoutCancel(ctx), model.Project.Name, model.Name); err != nil {
+				log.Errorf("failed to rollback create VirtualService: %v", err)
+			}
+		}
+	}()
+
 	// Deploy Istio's VirtualService
 	vs, err = istioClient.CreateVirtualService(ctx, model.Project.Name, vs)
 	if err != nil {
@@ -159,19 +169,36 @@ func (s *modelEndpointsService) UpdateEndpoint(ctx context.Context, model *model
 		return nil, errors.Wrapf(err, "failed to assign version endpoint to model endpoint")
 	}
 
+	istioClient, ok := s.istioClients[newEndpoint.EnvironmentName]
+	if !ok {
+		log.Errorf("unable to find istio client for environment: %s", newEndpoint.EnvironmentName)
+		return nil, fmt.Errorf("unable to find istio client for environment: %s", newEndpoint.EnvironmentName)
+	}
+
+	// Get current VirtualService
+	currentVs, err := istioClient.GetVirtualService(ctx, model.Project.Name, model.Name)
+	if client.IgnoreNotFound(err) != nil {
+		log.Errorf("failed to get VirtualService: %v", err)
+		return nil, errors.Wrapf(err, "failed to get VirtualService resource on cluster")
+	}
+
+	// rollback to previous virtualService state if there's error during update
+	defer func() {
+		if err != nil && currentVs != nil {
+			s.cleanVirtualServiceFields(currentVs)
+			if _, err := istioClient.PatchVirtualService(context.WithoutCancel(ctx), model.Project.Name, currentVs); err != nil {
+				log.Errorf("failed to rollback update VirtualService: %v", err)
+			}
+		}
+	}()
+
 	// Patch Istio's VirtualService
 	vs, err := s.createVirtualService(model, newEndpoint)
 	if err != nil {
 		log.Errorf("failed to create VirtualService specification: %v", err)
 		return nil, errors.Wrapf(err, "failed to create VirtualService specification")
 	}
 
-	istioClient, ok := s.istioClients[newEndpoint.EnvironmentName]
-	if !ok {
-		log.Errorf("unable to find istio client for environment: %s", newEndpoint.EnvironmentName)
-		return nil, fmt.Errorf("unable to find istio client for environment: %s", newEndpoint.EnvironmentName)
-	}
-
 	// Update Istio's VirtualService
 	vs, err = istioClient.PatchVirtualService(ctx, model.Project.Name, vs)
 	if err != nil {
@@ -208,8 +235,24 @@ func (s *modelEndpointsService) UndeployEndpoint(ctx context.Context, model *mod
 		return nil, fmt.Errorf("unable to find istio client for environment: %s", endpoint.EnvironmentName)
 	}
 
+	currentVs, err := istioClient.GetVirtualService(ctx, model.Project.Name, model.Name)
+	if client.IgnoreNotFound(err) != nil {
+		log.Errorf("failed to get VirtualService: %v", err)
+		return endpoint, nil
+	}
+
+	// rollback to previous virtualService state if there's error during delete
+	defer func() {
+		if err != nil && currentVs != nil {
+			s.cleanVirtualServiceFields(currentVs)
+			if _, err := istioClient.CreateVirtualService(context.WithoutCancel(ctx), model.Project.Name, currentVs); err != nil {
+				log.Errorf("failed to rollback delete VirtualService: %v", err)
+			}
+		}
+	}()
+
 	// Delete Istio's VirtualService
-	err := istioClient.DeleteVirtualService(ctx, model.Project.Name, model.Name)
+	err = istioClient.DeleteVirtualService(ctx, model.Project.Name, model.Name)
 	if client.IgnoreNotFound(err) != nil {
 		log.Errorf("failed to delete VirtualService: %v", err)
 		return nil, errors.Wrapf(err, "failed to delete VirtualService resource on cluster")
@@ -342,6 +385,23 @@ func (c *modelEndpointsService) assignVersionEndpoint(ctx context.Context, endpo
 	return endpoint, nil
 }
 
+// cleanVirtualServiceFields reset fields that should not be sent in update request
+// otherwise, the request will be rejected by the API server
+func (c *modelEndpointsService) cleanVirtualServiceFields(vs *v1beta1.VirtualService) {
+	if vs == nil {
+		return
+	}
+
+	vs.SetResourceVersion("")
+	vs.SetUID("")
+	vs.SetSelfLink("")
+	vs.SetCreationTimestamp(metav1.Time{})
+	vs.SetGeneration(0)
+	vs.SetManagedFields(nil)
+	vs.SetOwnerReferences(nil)
+	vs.SetFinalizers(nil)
+}
+
 func createHttpRoutes(versionEndpointPath string, httpRouteDestinations []*istiov1beta1.HTTPRouteDestination, value protocol.Protocol) []*istiov1beta1.HTTPRoute {
 	switch value {
 	case protocol.UpiV1: