fixes(controller): istio dropping fields not defined in type (#2268)

* fixes #2266

Signed-off-by: zachaller <[email protected]>

* typo

Signed-off-by: zachaller <[email protected]>

* test after calling set mirror

Signed-off-by: zachaller <[email protected]>

* Keep port on user defined service by adding to internal types

Signed-off-by: zachaller <[email protected]>

* github trigger re-run

Signed-off-by: zachaller <[email protected]>

* Drop port on virtual service to be consistent with added routes

Drop the port on the virtual service with the statment that rollouts only supports
one port services.

Signed-off-by: zachaller <[email protected]>

* cleanup test to just test extra fields and add a port check on destination as well

Signed-off-by: zachaller <[email protected]>

* keep port for both mirroring and header routes

Signed-off-by: zachaller <[email protected]>

* github trigger re-run

Signed-off-by: zachaller <[email protected]>

* github trigger re-run

Signed-off-by: zachaller <[email protected]>

* add function comment

Signed-off-by: zachaller <[email protected]>

Signed-off-by: zachaller <[email protected]>

rollout/trafficrouting/istio/istio.go

@@ -652,7 +652,7 @@ func (r *Reconciler) getVirtualService(namespace string, vsvcName string, client
 	return vsvc, err
 }
 
-func (r *Reconciler) reconcileVirtualServiceHeaderRoutes(obj *unstructured.Unstructured, headerRouting *v1alpha1.SetHeaderRoute) error {
+func (r *Reconciler) reconcileVirtualServiceHeaderRoutes(virtualService v1alpha1.IstioVirtualService, obj *unstructured.Unstructured, headerRouting *v1alpha1.SetHeaderRoute) error {
 	// HTTP Routes
 	httpRoutesI, err := GetHttpRoutesI(obj)
 	if err != nil {
@@ -687,7 +687,7 @@ func (r *Reconciler) reconcileVirtualServiceHeaderRoutes(obj *unstructured.Unstr
 		return fmt.Errorf("[reconcileVirtualServiceHeaderRoutes] failed to remove http route from virtual service: %w", err)
 	}
 
-	httpRoutesI = append(httpRoutesI, createHeaderRoute(headerRouting, canarySvc, canarySubset))
+	httpRoutesI = append(httpRoutesI, createHeaderRoute(virtualService, obj, headerRouting, canarySvc, canarySubset))
 
 	err = unstructured.SetNestedSlice(obj.Object, httpRoutesI, "spec", Http)
 	if err != nil {
@@ -712,7 +712,7 @@ func (r *Reconciler) SetHeaderRoute(headerRouting *v1alpha1.SetHeaderRoute) erro
 			return fmt.Errorf("[SetHeaderRoute] failed to get istio virtual service: %w", err)
 		}
 
-		err = r.reconcileVirtualServiceHeaderRoutes(vsvc, headerRouting)
+		err = r.reconcileVirtualServiceHeaderRoutes(virtualService, vsvc, headerRouting)
 		if err != nil {
 			return fmt.Errorf("[SetHeaderRoute] failed to reconcile header routes: %w", err)
 		}
@@ -765,12 +765,19 @@ func (r *Reconciler) getDestinationRule(dRuleSpec *v1alpha1.IstioDestinationRule
 	return origBytes, dRule, dRuleNew, nil
 }
 
-func createHeaderRoute(headerRouting *v1alpha1.SetHeaderRoute, host string, subset string) map[string]interface{} {
+func createHeaderRoute(virtualService v1alpha1.IstioVirtualService, unVsvc *unstructured.Unstructured, headerRouting *v1alpha1.SetHeaderRoute, host string, subset string) map[string]interface{} {
 	var routeMatches []interface{}
 	for _, hrm := range headerRouting.Match {
 		routeMatches = append(routeMatches, createHeaderRouteMatch(hrm))
 	}
-	canaryDestination := routeDestination(host, subset, 100)
+
+	port, err := getVirtualServiceCanaryPort(unVsvc, virtualService)
+	if err != nil {
+		port = Port{Number: 0}
+	}
+
+	canaryDestination := routeDestination(host, port.Number, subset, 100)
+
 	return map[string]interface{}{
 		"name":  headerRouting.Name,
 		"match": routeMatches,
@@ -795,10 +802,13 @@ func setMapValueIfNotEmpty(m map[string]interface{}, key string, value string) {
 	}
 }
 
-func routeDestination(host, subset string, weight int64) map[string]interface{} {
+func routeDestination(host string, port uint32, subset string, weight int64) map[string]interface{} {
 	dest := map[string]interface{}{
 		"host": host,
 	}
+	if port > 0 {
+		dest["port"] = map[string]interface{}{"number": int64(port)}
+	}
 	if subset != "" {
 		dest["subset"] = subset
 	}
@@ -1141,14 +1151,20 @@ func createMirrorRoute(virtualService v1alpha1.IstioVirtualService, httpRoutes [
 		})
 	}
 
+	mirrorDestinations := VirtualServiceDestination{
+		Host:   canarySvc,
+		Subset: subset,
+	}
+	if len(route) >= 0 && route[0].Destination.Port != nil {
+		// We try to pull the port from any of the routes destinations that are supposed to be updated via SetWeight
+		mirrorDestinations.Port = &Port{Number: route[0].Destination.Port.Number}
+	}
+
 	mirrorRoute := map[string]interface{}{
-		"name":  mirrorRouting.Name,
-		"match": istioMatch,
-		"route": route,
-		"mirror": VirtualServiceDestination{
-			Host:   canarySvc,
-			Subset: subset,
-		},
+		"name":             mirrorRouting.Name,
+		"match":            istioMatch,
+		"route":            route,
+		"mirror":           mirrorDestinations,
 		"mirrorPercentage": map[string]interface{}{"value": float64(percent)},
 	}
 
@@ -1232,7 +1248,7 @@ func (r *Reconciler) orderRoutes(istioVirtualService *unstructured.Unstructured)
 		return fmt.Errorf("[orderRoutes] could not split routes between managed and non managed: %w", err)
 	}
 
-	finalRoutes, err := getOrderedVirtualServiceRoutes(managedRoutes, httpRoutesWithinManagedRoutes, httpRoutesNotWithinManagedRoutes)
+	finalRoutes, err := getOrderedVirtualServiceRoutes(httpRouteI, managedRoutes, httpRoutesWithinManagedRoutes, httpRoutesNotWithinManagedRoutes)
 	if err != nil {
 		return fmt.Errorf("[orderRoutes] could not get ordered virtual service routes: %w", err)
 	}
@@ -1279,7 +1295,7 @@ func splitManagedRoutesAndNonManagedRoutes(managedRoutes []v1alpha1.MangedRoutes
 // getOrderedVirtualServiceRoutes This returns an []interface{} of istio virtual routes where the routes are ordered based
 // on the rollouts managedRoutes field. We take the routes from the rollouts managedRoutes field order them and place them on top
 // of routes that are manually defined within the virtual service (aka. routes that users have defined manually)
-func getOrderedVirtualServiceRoutes(managedRoutes []v1alpha1.MangedRoutes, httpRoutesWithinManagedRoutes []VirtualServiceHTTPRoute, httpRoutesNotWithinManagedRoutes []VirtualServiceHTTPRoute) ([]interface{}, error) {
+func getOrderedVirtualServiceRoutes(httpRouteI []interface{}, managedRoutes []v1alpha1.MangedRoutes, httpRoutesWithinManagedRoutes []VirtualServiceHTTPRoute, httpRoutesNotWithinManagedRoutes []VirtualServiceHTTPRoute) ([]interface{}, error) {
 	var orderedManagedRoutes []VirtualServiceHTTPRoute
 	for _, route := range managedRoutes {
 		for _, managedRoute := range httpRoutesWithinManagedRoutes {
@@ -1289,18 +1305,45 @@ func getOrderedVirtualServiceRoutes(managedRoutes []v1alpha1.MangedRoutes, httpR
 		}
 	}
 
-	allIstioRoutes := append(orderedManagedRoutes, httpRoutesNotWithinManagedRoutes...)
+	orderedVirtualServiceHTTPRoutes := append(orderedManagedRoutes, httpRoutesNotWithinManagedRoutes...)
 
-	jsonAllIstioRoutes, err := json.Marshal(allIstioRoutes)
+	var orderedInterfaceVSVCHTTPRoutes []interface{}
+	for _, routeTyped := range orderedVirtualServiceHTTPRoutes {
+		for _, route := range httpRouteI {
+			r := route.(map[string]interface{})
+
+			// No need to check if exist because the empty string returned on cast failure is good for this check
+			name, _ := r["name"].(string)
+			if name == routeTyped.Name {
+				orderedInterfaceVSVCHTTPRoutes = append(orderedInterfaceVSVCHTTPRoutes, route)
+			}
+		}
+	}
+
+	return orderedInterfaceVSVCHTTPRoutes, nil
+}
+
+// getVirtualServiceCanaryPort This function returns the port that the canary service is running on. It does this by looking at the
+// istio Virtual Service and finding any port from a destination that is suppose to be update via SetWeight.
+func getVirtualServiceCanaryPort(unVsvc *unstructured.Unstructured, virtualService v1alpha1.IstioVirtualService) (Port, error) {
+	httpRoutes, _, err := getVirtualServiceHttpRoutes(unVsvc)
 	if err != nil {
-		return nil, fmt.Errorf("[getOrderedVirtualServiceRoutes] failed to marsharl istio routes: %w", err)
+		return Port{}, fmt.Errorf("[getVirtualServiceCanaryPort] failed to get virtual service http routes: %w", err)
 	}
-	var orderedRoutes []interface{}
-	if err := json.Unmarshal(jsonAllIstioRoutes, &orderedRoutes); err != nil {
-		return nil, fmt.Errorf("[getOrderedVirtualServiceRoutes] failed to unmarsharl istio routes: %w", err)
+
+	route, err := getVirtualServiceSetWeightRoute(virtualService.Routes, httpRoutes)
+	if err != nil {
+		return Port{}, fmt.Errorf("[getVirtualServiceCanaryPort] failed to get virtual service set weight route: %w", err)
+	}
+
+	var port uint32 = 0
+	if len(route) > 0 && route[0].Destination.Port != nil {
+		port = route[0].Destination.Port.Number
 	}
 
-	return orderedRoutes, nil
+	return Port{
+		Number: port,
+	}, nil
 }
 
 // RemoveManagedRoutes this removes all the routes in all the istio virtual services rollouts is managing by getting two slices

rollout/trafficrouting/istio/istio_test.go

@@ -603,13 +603,88 @@ spec:
 	assert.Equal(t, httpRoutes[0].Route[0].Destination.Subset, "canary-subset")
 }
 
+func TestHttpReconcileHeaderRouteWithExtra(t *testing.T) {
+	ro := rolloutWithHttpRoutes("stable", "canary", "vsvc", []string{"primary"})
+	obj := unstructuredutil.StrToUnstructuredUnsafe(regularVsvcWithExtra)
+	client := testutil.NewFakeDynamicClient(obj)
+	vsvcLister, druleLister := getIstioListers(client)
+	r := NewReconciler(ro, client, record.NewFakeEventRecorder(), vsvcLister, druleLister)
+	client.ClearActions()
+
+	const headerName = "test-header-route"
+	r.rollout.Spec.Strategy.Canary.TrafficRouting.ManagedRoutes = append(r.rollout.Spec.Strategy.Canary.TrafficRouting.ManagedRoutes, []v1alpha1.MangedRoutes{{
+		Name: headerName,
+	},
+	}...)
+
+	// Test for both the HTTP VS & Mixed VS
+	hr := &v1alpha1.SetHeaderRoute{
+		Name: headerName,
+		Match: []v1alpha1.HeaderRoutingMatch{
+			{
+				HeaderName:  "agent",
+				HeaderValue: &v1alpha1.StringMatch{Exact: "firefox"},
+			},
+		},
+	}
+
+	err := r.SetHeaderRoute(hr)
+	assert.Nil(t, err)
+
+	iVirtualService, err := client.Resource(istioutil.GetIstioVirtualServiceGVR()).Namespace(r.rollout.Namespace).Get(context.TODO(), ro.Spec.Strategy.Canary.TrafficRouting.Istio.VirtualService.Name, metav1.GetOptions{})
+	assert.NoError(t, err)
+
+	// HTTP Routes
+	httpRoutes := extractHttpRoutes(t, iVirtualService)
+
+	// Assertions
+	assert.Equal(t, httpRoutes[0].Name, headerName)
+	checkDestination(t, httpRoutes[0].Route, "canary", 100)
+	assert.Equal(t, len(httpRoutes[0].Route), 1)
+	assert.Equal(t, httpRoutes[1].Name, "primary")
+	checkDestination(t, httpRoutes[1].Route, "stable", 100)
+	assert.Equal(t, httpRoutes[2].Name, "secondary")
+
+	iVirtualService, err = client.Resource(istioutil.GetIstioVirtualServiceGVR()).Namespace(r.rollout.Namespace).Get(context.TODO(), ro.Spec.Strategy.Canary.TrafficRouting.Istio.VirtualService.Name, metav1.GetOptions{})
+	assert.NoError(t, err)
+	// HTTP Routes
+	httpRoutes = extractHttpRoutes(t, iVirtualService)
+	// Assertions
+	assert.Equal(t, httpRoutes[0].Name, headerName)
+	assert.Equal(t, httpRoutes[1].Name, "primary")
+	assert.Equal(t, httpRoutes[2].Name, "secondary")
+
+	routes, found, err := unstructured.NestedSlice(iVirtualService.Object, "spec", "http")
+	assert.NoError(t, err)
+	assert.True(t, found)
+
+	r0 := routes[0].(map[string]interface{})
+	route, found := r0["route"].([]interface{})
+	assert.True(t, found)
+
+	port1 := route[0].(map[string]interface{})["destination"].(map[string]interface{})["port"].(map[string]interface{})["number"]
+	assert.True(t, port1 == int64(8443))
+
+	r1 := routes[1].(map[string]interface{})
+	_, found = r1["retries"]
+	assert.True(t, found)
+
+	r2 := routes[2].(map[string]interface{})
+	_, found = r2["retries"]
+	assert.True(t, found)
+	_, found = r2["corsPolicy"]
+	assert.True(t, found)
+
+}
+
 func TestReconcileUpdateHeader(t *testing.T) {
 	ro := rolloutWithHttpRoutes("stable", "canary", "vsvc", []string{"primary"})
 	ro.Spec.Strategy.Canary.TrafficRouting.ManagedRoutes = append(ro.Spec.Strategy.Canary.TrafficRouting.ManagedRoutes, v1alpha1.MangedRoutes{
 		Name: "test-mirror-1",
 	})
 	AssertReconcileUpdateHeader(t, regularVsvc, ro)
 }
+
 func AssertReconcileUpdateHeader(t *testing.T, vsvc string, ro *v1alpha1.Rollout) *dynamicfake.FakeDynamicClient {
 	obj := unstructuredutil.StrToUnstructuredUnsafe(vsvc)
 	client := testutil.NewFakeDynamicClient(obj)
@@ -1642,6 +1717,60 @@ spec:
         host: canary
       weight: 0`
 
+const regularVsvcWithExtra = `apiVersion: networking.istio.io/v1alpha3
+kind: VirtualService
+metadata:
+  name: vsvc
+  namespace: default
+spec:
+  gateways:
+  - istio-rollout-gateway
+  hosts:
+  - istio-rollout.dev.argoproj.io
+  http:
+  - name: primary
+    retries:
+      attempts: 3
+      perTryTimeout: 10s
+      retryOn: 'gateway-error,connect-failure,refused-stream'
+    route:
+    - destination:
+        host: 'stable'
+        port:
+          number: 8443
+      weight: 100
+    - destination:
+        host: canary
+        port:
+          number: 8443
+      weight: 0
+  - name: secondary
+    retries:
+      attempts: 3
+      perTryTimeout: 10s
+      retryOn: 'gateway-error,connect-failure,refused-stream'
+    corsPolicy:
+      allowOrigins:
+        - exact: https://example.com
+      allowMethods:
+        - POST
+        - GET
+      allowCredentials: false
+      allowHeaders:
+        - X-Foo-Bar
+      maxAge: "24h"
+    route:
+    - destination:
+        host: 'stable'
+        port:
+          number: 8443
+      weight: 100
+    - destination:
+        host: canary
+        port:
+          number: 8443
+      weight: 0`
+
 func TestMultipleVirtualServiceConfigured(t *testing.T) {
 	multipleVirtualService := []v1alpha1.IstioVirtualService{{Name: "vsvc1", Routes: []string{"primary", "secondary"}}, {Name: "vsvc2", Routes: []string{"blue-green"}}}
 	ro := multiVsRollout("stable", "canary", multipleVirtualService)
@@ -1874,6 +2003,58 @@ func TestHttpReconcileMirrorRoute(t *testing.T) {
 
 }
 
+func TestHttpReconcileMirrorRouteWithExtraFields(t *testing.T) {
+	ro := rolloutWithHttpRoutes("stable", "canary", "vsvc", []string{"primary"})
+	obj := unstructuredutil.StrToUnstructuredUnsafe(regularVsvcWithExtra)
+	client := testutil.NewFakeDynamicClient(obj)
+	vsvcLister, druleLister := getIstioListers(client)
+	r := NewReconciler(ro, client, record.NewFakeEventRecorder(), vsvcLister, druleLister)
+	client.ClearActions()
+
+	// Test for both the HTTP VS & Mixed VS
+	setMirror1 := &v1alpha1.SetMirrorRoute{
+		Name: "test-mirror-1",
+		Match: []v1alpha1.RouteMatch{{
+			Method: &v1alpha1.StringMatch{
+				Exact: "GET",
+			},
+		}},
+	}
+	r.rollout.Spec.Strategy.Canary.TrafficRouting.ManagedRoutes = append(r.rollout.Spec.Strategy.Canary.TrafficRouting.ManagedRoutes, []v1alpha1.MangedRoutes{{
+		Name: "test-mirror-1",
+	},
+	}...)
+
+	err := r.SetMirrorRoute(setMirror1)
+	assert.Nil(t, err)
+	iVirtualService, err := client.Resource(istioutil.GetIstioVirtualServiceGVR()).Namespace(r.rollout.Namespace).Get(context.TODO(), ro.Spec.Strategy.Canary.TrafficRouting.Istio.VirtualService.Name, metav1.GetOptions{})
+	assert.NoError(t, err)
+
+	routes, found, err := unstructured.NestedSlice(iVirtualService.Object, "spec", "http")
+	assert.NoError(t, err)
+	assert.True(t, found)
+
+	r0 := routes[0].(map[string]interface{})
+	mirrorRoute, found := r0["route"].([]interface{})
+	assert.True(t, found)
+
+	port1 := mirrorRoute[0].(map[string]interface{})["destination"].(map[string]interface{})["port"].(map[string]interface{})["number"]
+	port2 := mirrorRoute[1].(map[string]interface{})["destination"].(map[string]interface{})["port"].(map[string]interface{})["number"]
+	assert.True(t, port1 == float64(8443))
+	assert.True(t, port2 == float64(8443))
+
+	r1 := routes[1].(map[string]interface{})
+	_, found = r1["retries"]
+	assert.True(t, found)
+
+	r2 := routes[2].(map[string]interface{})
+	_, found = r2["retries"]
+	assert.True(t, found)
+	_, found = r2["corsPolicy"]
+	assert.True(t, found)
+
+}
+
 func TestHttpReconcileMirrorRouteOrder(t *testing.T) {
 	ro := rolloutWithHttpRoutes("stable", "canary", "vsvc", []string{"primary", "secondary"})
 	obj := unstructuredutil.StrToUnstructuredUnsafe(regularVsvc)

rollout/trafficrouting/istio/istio_types.go

@@ -70,6 +70,11 @@ type VirtualServiceRouteDestination struct {
 type VirtualServiceDestination struct {
 	Host   string `json:"host,omitempty"`
 	Subset string `json:"subset,omitempty"`
+	Port   *Port  `json:"port,omitempty"`
+}
+
+type Port struct {
+	Number uint32 `json:"number,omitempty"`
 }
 
 // DestinationRule is an Istio DestinationRule containing only the fields which we care about