Flexible backend group bug fix: if clientPolicy is defined under backends, it does not get converted to sdk VN spec (#651)

* Added primary changes to fix the clientPolicy missing in VN spec

* Bug fix: clientPolicy now can be added under backends, which previously does not get converted to AppMesh SDK spec

* Bug fix: clientPolicy now can be added under backends, which previously does not get converted to AppMesh SDK spec

Co-authored-by: Mazhar Islam <[email protected]>

Author: rakeb

apis/appmesh/v1beta2/backendgroup_types.go

@@ -48,9 +48,9 @@ type BackendGroupReference struct {
 	Name string `json:"name"`
 }
 
-//+kubebuilder:object:root=true
+// +kubebuilder:object:root=true
 // +kubebuilder:resource:categories=all
-//+kubebuilder:subresource:status
+// +kubebuilder:subresource:status
 // +kubebuilder:pruning:PreserveUnknownFields
 // BackendGroup is the Schema for the backendgroups API
 type BackendGroup struct {

pkg/equality/ignore_left_hand_unset.go

@@ -8,8 +8,8 @@ import (
 
 // IgnoreLeftHandUnset is an option that ignores struct fields that are unset on the left hand side of a comparison.
 // Note:
-//	 1. for map and slices, only nil value is considered to be unset, non-nil but empty is not considered as unset.
-//   2. for struct pointers, nil value is considered to be unset
+//  1. for map and slices, only nil value is considered to be unset, non-nil but empty is not considered as unset.
+//  2. for struct pointers, nil value is considered to be unset
 func IgnoreLeftHandUnset(typ interface{}, fields ...string) cmp.Option {
 	t := reflect.TypeOf(typ)
 	fieldsSet := sets.NewString(fields...)

pkg/inject/sidecar_builder.go

@@ -15,7 +15,7 @@ import (
 const envoyTracingConfigVolumeName = "envoy-tracing-config"
 
 // Envoy template variables used by envoys in pod and the envoy in VirtualGateway
-//as we use the same envoy image
+// as we use the same envoy image
 type EnvoyTemplateVariables struct {
 	AWSRegion                string
 	MeshName                 string

pkg/virtualnode/resource_manager.go

@@ -3,7 +3,6 @@ package virtualnode
 import (
 	"context"
 	"fmt"
-
 	appmesh "github.com/aws/aws-app-mesh-controller-for-k8s/apis/appmesh/v1beta2"
 	"github.com/aws/aws-app-mesh-controller-for-k8s/pkg/aws/services"
 	"github.com/aws/aws-app-mesh-controller-for-k8s/pkg/conversions"
@@ -343,8 +342,17 @@ func BuildSDKVirtualNodeSpec(vn *appmesh.VirtualNode, vsByKey map[types.Namespac
 	})
 	sdkVNSpec := &appmeshsdk.VirtualNodeSpec{}
 	tempSpec := vn.Spec.DeepCopy()
-	tempSpec.Backends = []appmesh.Backend{}
-	for _, vs := range vsByKey {
+	backendMap := make(map[types.NamespacedName]bool)
+
+	for _, backend := range tempSpec.Backends {
+		vsKey := references.ObjectKeyForVirtualServiceReference(vn, *backend.VirtualService.VirtualServiceRef)
+		backendMap[vsKey] = true
+	}
+
+	for vsKey, vs := range vsByKey {
+		if _, ok := backendMap[vsKey]; ok {
+			continue
+		}
 		tempSpec.Backends = append(tempSpec.Backends, appmesh.Backend{
 			VirtualService: appmesh.VirtualServiceBackend{
 				VirtualServiceRef: &appmesh.VirtualServiceReference{

pkg/virtualnode/resource_manager_test.go

@@ -593,3 +593,64 @@ func Test_defaultResourceManager_findVirtualServiceDependencies(t *testing.T) {
 		})
 	}
 }
+
+/*
+This is a bit tricky unit testing. First we created a vn having VirtualServiceRef and ClientPolicy. The VirtualServiceRef key is (ns-1/vs-1).
+Later we created two entries of vsByKey having keys (ns-2/vs-2) with a VirtualRouterServiceProvider and (ns-1/vs-1) with an empty body.
+The reason behind that, the BuildSDKVirtualNodeSpec function will not modify the vn spec under key (ns-1/vs-1) as it is part of the
+Backends. However, VirtualRouterServiceProvider will get wiped out because it is under key (ns-2/vs-2) and will be treated as flexible backend.
+*/
+func Test_BuildSDKVirtualNodeSpec(t *testing.T) {
+	vn := &appmesh.VirtualNode{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "vn-1",
+		},
+		Spec: appmesh.VirtualNodeSpec{
+			AWSName: aws.String("app1"),
+			Backends: []appmesh.Backend{
+				{
+					VirtualService: appmesh.VirtualServiceBackend{
+						VirtualServiceRef: &appmesh.VirtualServiceReference{
+							Namespace: aws.String("ns-1"),
+							Name:      "vs-1",
+						},
+						ClientPolicy: &appmesh.ClientPolicy{
+							TLS: &appmesh.ClientPolicyTLS{
+								Enforce: aws.Bool(true),
+							},
+						},
+					}}},
+		},
+	}
+
+	vsByKey := map[types.NamespacedName]*appmesh.VirtualService{types.NamespacedName{
+		Namespace: "ns-2", Name: "vs-2"}: &appmesh.VirtualService{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: "ns-2",
+			Name:      "vs-2",
+		},
+		Spec: appmesh.VirtualServiceSpec{
+			AWSName: aws.String("app2"),
+			Provider: &appmesh.VirtualServiceProvider{
+				VirtualRouter: &appmesh.VirtualRouterServiceProvider{
+					VirtualRouterRef: &appmesh.VirtualRouterReference{
+						Namespace: aws.String("ns-2"),
+						Name:      "vr-2",
+					},
+				},
+			},
+		}}}
+
+	vsByKey[types.NamespacedName{Namespace: "ns-1", Name: "vs-1"}] = &appmesh.VirtualService{}
+
+	ctrl := gomock.NewController(t)
+	defer ctrl.Finish()
+
+	sdkVnSpec, err := BuildSDKVirtualNodeSpec(vn, vsByKey)
+	if err != nil {
+		assert.Fail(t, "Could not convert to sdkVn spec", err)
+	} else {
+		assert.NotNil(t, sdkVnSpec.Backends[0].VirtualService.ClientPolicy)
+		assert.Nil(t, sdkVnSpec.Backends[1].VirtualService.ClientPolicy)
+	}
+}

test/e2e/fishapp/dynamic_stack.go

@@ -65,13 +65,16 @@ var (
 
 // A dynamic generated stack designed to test app mesh integration :D
 // Suppose given configuration below:
-//		5 VirtualServicesCount
-//		10 VirtualNodesCount
-//		2 RoutesCountPerVirtualRouter
-//		2 TargetsCountPerRoute
-//		4 BackendsCountPerVirtualNode
+//
+//	5 VirtualServicesCount
+//	10 VirtualNodesCount
+//	2 RoutesCountPerVirtualRouter
+//	2 TargetsCountPerRoute
+//	4 BackendsCountPerVirtualNode
+//
 // We will generate virtual service configuration & virtual node configuration follows:
 // =======virtual services =========
+//
 //	vs1 -> /path1 -> vn1(50)
 //				  -> vn2(50)
 //		-> /path2 -> vn3(50)
@@ -92,11 +95,13 @@ var (
 //				  -> vn8(50)
 //		-> /path2 -> vn9(50)
 //				  -> vn10(50)
+//
 // =======virtual nodes =========
-//  vn1 -> vs1,vs2,vs3,vs4
-//  vn2 -> vs5,vs1,vs2,vs3
-//  vn3 -> vs4,vs5,vs1,vs2
-//  ...
+//
+//	vn1 -> vs1,vs2,vs3,vs4
+//	vn2 -> vs5,vs1,vs2,vs3
+//	vn3 -> vs4,vs5,vs1,vs2
+//	...
 //
 // then we validate each virtual node can access each virtual service at every path, and calculates the target distribution
 type DynamicStack struct {

test/e2e/fishapp/load/dynamic_stack_load_test.go

@@ -69,13 +69,16 @@ var (
 
 // A dynamic generated stack designed to test app mesh integration :D
 // Suppose given configuration below:
-//		5 VirtualServicesCount
-//		5 VirtualNodesCount
-//		2 RoutesCountPerVirtualRouter
-//		2 TargetsCountPerRoute
-//		4 BackendsCountPerVirtualNode
+//
+//	5 VirtualServicesCount
+//	5 VirtualNodesCount
+//	2 RoutesCountPerVirtualRouter
+//	2 TargetsCountPerRoute
+//	4 BackendsCountPerVirtualNode
+//
 // We will generate virtual service configuration & virtual node configuration follows:
 // =======virtual services =========
+//
 //	vs1 -> /path1 -> vn1(50)
 //				  -> vn2(50)
 //		-> /path2 -> vn3(50)
@@ -96,11 +99,13 @@ var (
 //				  -> vn8(50)
 //		-> /path2 -> vn9(50)
 //				  -> vn10(50)
+//
 // =======virtual nodes =========
-//  vn1 -> vs1,vs2,vs3,vs4
-//  vn2 -> vs5,vs1,vs2,vs3
-//  vn3 -> vs4,vs5,vs1,vs2
-//  ...
+//
+//	vn1 -> vs1,vs2,vs3,vs4
+//	vn2 -> vs5,vs1,vs2,vs3
+//	vn3 -> vs4,vs5,vs1,vs2
+//	...
 //
 // then we validate each virtual node can access each virtual service at every path, and calculates the target distribution
 type DynamicStack struct {

test/integration/timeout/timeout_stack.go

@@ -107,7 +107,7 @@ func (s *TimeoutStack) CleanupTimeoutStack(ctx context.Context, f *framework.Fra
 	Expect(len(deletionErrors)).To(BeZero())
 }
 
-//Check Timeout behavior with and with timeout configured
+// Check Timeout behavior with and with timeout configured
 func (s *TimeoutStack) CheckTimeoutBehavior(ctx context.Context, f *framework.Framework) {
 	By(fmt.Sprintf("verify route timesout if it takes more than default 15s w/o listener timeout configured"), func() {
 		err := s.checkExpectedRouteBehavior(ctx, f, s.FrontEndDP, "defaultroute", false)

test/integration/tls/tls_stack.go

@@ -66,8 +66,8 @@ type TLSStack struct {
 	BackEndVR *appmesh.VirtualRouter
 }
 
-//TLS Validation is enabled on the Frontend and Listener TLS is configured on the backend. Frontend looks
-//for certs signed by CA1 and backend certs are signed by CA1 as well.
+// TLS Validation is enabled on the Frontend and Listener TLS is configured on the backend. Frontend looks
+// for certs signed by CA1 and backend certs are signed by CA1 as well.
 func (s *TLSStack) DeployTLSStack(ctx context.Context, f *framework.Framework) {
 	s.createTLSStackMeshAndNamespace(ctx, f)
 	mb := &manifest.ManifestBuilder{
@@ -80,7 +80,7 @@ func (s *TLSStack) DeployTLSStack(ctx context.Context, f *framework.Framework) {
 	s.assignBackendVSToFrontEndVN(ctx, f)
 }
 
-//Frontend has TLS Validation enabled while TLS is disabled on the backend
+// Frontend has TLS Validation enabled while TLS is disabled on the backend
 func (s *TLSStack) DeployPartialTLSStack(ctx context.Context, f *framework.Framework) {
 	s.createTLSStackMeshAndNamespace(ctx, f)
 	time.Sleep(30 * time.Second)
@@ -94,8 +94,8 @@ func (s *TLSStack) DeployPartialTLSStack(ctx context.Context, f *framework.Frame
 	s.assignBackendVSToFrontEndVN(ctx, f)
 }
 
-//TLS Validation is enabled on the Frontend and Listener TLS is configured on the backend. Frontend looks
-//for certs signed by CA2 while backend certs are signed by CA1.
+// TLS Validation is enabled on the Frontend and Listener TLS is configured on the backend. Frontend looks
+// for certs signed by CA2 while backend certs are signed by CA1.
 func (s *TLSStack) DeployTLSValidationStack(ctx context.Context, f *framework.Framework) {
 	s.createTLSStackMeshAndNamespace(ctx, f)
 	time.Sleep(30 * time.Second)