fix: controller would drop fields when updating DestinationRules (argoproj#1253)

Signed-off-by: Hui Kang <[email protected]>

Author: huikang

go.mod

@@ -18,6 +18,7 @@ require (
 	github.com/hashicorp/golang-lru v0.5.4 // indirect
 	github.com/juju/ansiterm v0.0.0-20180109212912-720a0952cc2a
 	github.com/lunixbochs/vtclean v1.0.0 // indirect
+	github.com/mitchellh/mapstructure v1.3.3
 	github.com/newrelic/newrelic-client-go v0.49.0
 	github.com/pkg/errors v0.9.1
 	github.com/prometheus/client_golang v1.10.0

rollout/trafficrouting/istio/istio.go

@@ -7,6 +7,7 @@ import (
 	"strings"
 
 	jsonpatch "github.com/evanphx/json-patch/v5"
+	"github.com/mitchellh/mapstructure"
 	log "github.com/sirupsen/logrus"
 	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -224,15 +225,49 @@ func (r *Reconciler) UpdateHash(canaryHash, stableHash string) error {
 	return nil
 }
 
+// destinationRuleReplaceExtraMarshal relace the key of "Extra" with the actual content
+// e.g., "trafficpolicy" and return the bytes of the new object
+func destinationRuleReplaceExtraMarshal(dRule *DestinationRule) []byte {
+	dRuleNew := map[string]interface{}{}
+	dRuleNew["metadata"] = dRule.ObjectMeta.DeepCopy()
+
+	subsets := []map[string]interface{}{}
+	for _, subset := range dRule.Spec.Subsets {
+		newsubset := map[string]interface{}{}
+		newsubset["name"] = subset.Name
+		newsubset["labels"] = subset.Labels
+
+		if subset.Extra == nil {
+			subsets = append(subsets, newsubset)
+			continue
+		}
+
+		extra := map[string]interface{}{}
+		inputbyte, _ := json.Marshal(subset.Extra)
+		json.Unmarshal(inputbyte, &extra)
+
+		subset.Extra = nil
+		for k, v := range extra {
+			newsubset[k] = v
+		}
+		subsets = append(subsets, newsubset)
+	}
+	dRuleNew["spec"] = map[string]interface{}{
+		"subsets": subsets,
+	}
+
+	dRuleNewBytes, _ := json.Marshal(dRuleNew)
+	return dRuleNewBytes
+}
+
 func updateDestinationRule(ctx context.Context, client dynamic.ResourceInterface, orig []byte, dRule, dRuleNew *DestinationRule) (bool, error) {
 	dRuleBytes, err := json.Marshal(dRule)
 	if err != nil {
 		return false, err
 	}
-	dRuleNewBytes, err := json.Marshal(dRuleNew)
-	if err != nil {
-		return false, err
-	}
+	dRuleNewBytes := destinationRuleReplaceExtraMarshal(dRuleNew)
+	log.Debugf("dRuleNewBytes: %s", string(dRuleNewBytes))
+
 	patch, err := jsonpatch.CreateMergePatch(dRuleBytes, dRuleNewBytes)
 	if err != nil {
 		return false, err
@@ -253,7 +288,7 @@ func updateDestinationRule(ctx context.Context, client dynamic.ResourceInterface
 	if err != nil {
 		return false, err
 	}
-	log.Infof("updated destinationrule: %s", string(patch))
+	log.Infof("updating destinationrule: %s", string(patch))
 	return true, nil
 }
 
@@ -275,12 +310,97 @@ func unstructuredToDestinationRules(un *unstructured.Unstructured) ([]byte, *Des
 	return dRuleBytes, dRule1, dRule2, nil
 }
 
+func unMarshalSubsets(dRule *DestinationRule, dRuleBytes []byte) error {
+	var err error
+
+	unstructured := map[string]interface{}{}
+	var extractFieldBytes func([]byte, string) ([]byte, error)
+	extractFieldBytes = func(input []byte, name string) ([]byte, error) {
+		err = json.Unmarshal(input, &unstructured)
+		if err != nil {
+			return nil, err
+		}
+		fieldBytes, err := json.Marshal(unstructured[name])
+		if err != nil {
+			return nil, err
+		}
+		return fieldBytes, nil
+	}
+
+	specBytes, err := extractFieldBytes(dRuleBytes, "spec")
+	if err != nil {
+		return err
+	}
+
+	subsetsBytes, err := extractFieldBytes(specBytes, "subsets")
+	if err != nil {
+		return err
+	}
+
+	subsetsMap := []map[string]interface{}{}
+	err = json.Unmarshal(subsetsBytes, &subsetsMap)
+	if err != nil {
+		return err
+	}
+
+	dRule.Spec.Subsets = []Subset{}
+	for _, si := range subsetsMap {
+		var subset Subset
+
+		jsonInput, _ := json.Marshal(si)
+		extra, err := UnmarshalJson(jsonInput, &subset)
+		if err != nil {
+			return err
+		}
+
+		subset.Extra = extra
+		if len(subset.Extra) == 0 {
+			subset.Extra = nil
+		}
+		dRule.Spec.Subsets = append(dRule.Spec.Subsets, subset)
+	}
+	return nil
+}
+
+func UnmarshalJson(input []byte, result interface{}) (map[string]interface{}, error) {
+	// unmarshal json to a map
+	foomap := make(map[string]interface{})
+	json.Unmarshal(input, &foomap)
+
+	// create a mapstructure decoder
+	var md mapstructure.Metadata
+	decoder, err := mapstructure.NewDecoder(
+		&mapstructure.DecoderConfig{
+			Metadata: &md,
+			Result:   result,
+		})
+	if err != nil {
+		return nil, err
+	}
+
+	// decode the unmarshalled map into the given struct
+	if err := decoder.Decode(foomap); err != nil {
+		return nil, err
+	}
+
+	// copy and return unused fields
+	unused := map[string]interface{}{}
+	for _, k := range md.Unused {
+		unused[k] = foomap[k]
+	}
+	return unused, nil
+}
+
 func jsonBytesToDestinationRule(dRuleBytes []byte) (*DestinationRule, error) {
 	var dRule DestinationRule
 	err := json.Unmarshal(dRuleBytes, &dRule)
 	if err != nil {
 		return nil, err
 	}
+	err = unMarshalSubsets(&dRule, dRuleBytes)
+	if err != nil {
+		return nil, err
+	}
 	return &dRule, nil
 }
 

rollout/trafficrouting/istio/istio_test.go

@@ -2,6 +2,7 @@ package istio
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
 	"testing"
 
@@ -391,6 +392,62 @@ func rolloutWithDestinationRule() *v1alpha1.Rollout {
 	}
 }
 
+// TestUpdateHashWithListers verifies behavior of UpdateHash when using informers/listers
+func TestUpdateHashAdditionalFieldsWithListers(t *testing.T) {
+	ro := rolloutWithDestinationRule()
+	obj := unstructuredutil.StrToUnstructuredUnsafe(`
+apiVersion: networking.istio.io/v1alpha3
+kind: DestinationRule
+metadata:
+  name: istio-destrule
+  namespace: default
+spec:
+  host: ratings.prod.svc.cluster.local
+  trafficPolicy:
+    loadBalancer:
+      simple: LEAST_CONN
+  subsets:
+  - name: stable
+    labels:
+      version: v3
+  - name: canary
+    trafficPolicy:
+      loadBalancer:
+        simple: ROUND_ROBIN
+`)
+	client := testutil.NewFakeDynamicClient(obj)
+	vsvcLister, druleLister := getIstioListers(client)
+	r := NewReconciler(ro, client, record.NewFakeEventRecorder(), vsvcLister, druleLister)
+	client.ClearActions()
+
+	err := r.UpdateHash("abc123", "def456")
+	assert.NoError(t, err)
+	actions := client.Actions()
+	assert.Len(t, actions, 1)
+	assert.Equal(t, "update", actions[0].GetVerb())
+
+	dRuleUn, err := client.Resource(istioutil.GetIstioDestinationRuleGVR()).Namespace(r.rollout.Namespace).Get(context.TODO(), "istio-destrule", metav1.GetOptions{})
+	assert.NoError(t, err)
+
+	dRuleUnBytes, err := json.Marshal(dRuleUn)
+	assert.NoError(t, err)
+	assert.Equal(t, `{"apiVersion":"networking.istio.io/v1alpha3","kind":"DestinationRule","metadata":{"annotations":{"argo-rollouts.argoproj.io/managed-by-rollouts":"rollout"},"name":"istio-destrule","namespace":"default"},"spec":{"host":"ratings.prod.svc.cluster.local","subsets":[{"labels":{"rollouts-pod-template-hash":"def456","version":"v3"},"name":"stable"},{"labels":{"rollouts-pod-template-hash":"abc123"},"name":"canary","trafficPolicy":{"loadBalancer":{"simple":"ROUND_ROBIN"}}}],"trafficPolicy":{"loadBalancer":{"simple":"LEAST_CONN"}}}}`,
+		string(dRuleUnBytes))
+
+	_, dRule, _, err := unstructuredToDestinationRules(dRuleUn)
+	assert.NoError(t, err)
+	assert.Equal(t, dRule.Annotations[v1alpha1.ManagedByRolloutsKey], "rollout")
+	assert.Equal(t, dRule.Spec.Subsets[0].Labels[v1alpha1.DefaultRolloutUniqueLabelKey], "def456")
+	assert.Equal(t, dRule.Spec.Subsets[1].Labels[v1alpha1.DefaultRolloutUniqueLabelKey], "abc123")
+	assert.Nil(t, dRule.Spec.Subsets[0].Extra)
+	assert.NotNil(t, dRule.Spec.Subsets[1].Extra)
+
+	jsonBytes, err := json.Marshal(dRule)
+	assert.NoError(t, err)
+	assert.Equal(t, `{"metadata":{"name":"istio-destrule","namespace":"default","creationTimestamp":null,"annotations":{"argo-rollouts.argoproj.io/managed-by-rollouts":"rollout"}},"spec":{"subsets":[{"name":"stable","labels":{"rollouts-pod-template-hash":"def456","version":"v3"}},{"name":"canary","labels":{"rollouts-pod-template-hash":"abc123"},"Extra":{"trafficPolicy":{"loadBalancer":{"simple":"ROUND_ROBIN"}}}}]}}`,
+		string(jsonBytes))
+}
+
 // TestUpdateHashWithListers verifies behavior of UpdateHash when using informers/listers
 func TestUpdateHashWithListers(t *testing.T) {
 	ro := rolloutWithDestinationRule()

rollout/trafficrouting/istio/istio_types.go

@@ -47,4 +47,6 @@ type DestinationRuleSpec struct {
 type Subset struct {
 	Name   string            `json:"name,omitempty"`
 	Labels map[string]string `json:"labels,omitempty"`
+	// TrafficPolicy *json.RawMessage  `json:"trafficPolicy,omitempty"`
+	Extra map[string]interface{} `json:",omitempty"`
 }