Merge pull request #991 from shashidharatd/json-patch-override

Replace override paths with JSON pointer style paths

charts/kubefed/templates/crds.yaml

@@ -33,6 +33,8 @@ spec:
                   clusterOverrides:
                     items:
                       properties:
+                        op:
+                          type: string
                         path:
                           type: string
                         value:
@@ -340,6 +342,8 @@ spec:
                   clusterOverrides:
                     items:
                       properties:
+                        op:
+                          type: string
                         path:
                           type: string
                         value:
@@ -601,6 +605,8 @@ spec:
                   clusterOverrides:
                     items:
                       properties:
+                        op:
+                          type: string
                         path:
                           type: string
                         value:
@@ -2908,6 +2914,8 @@ spec:
                   clusterOverrides:
                     items:
                       properties:
+                        op:
+                          type: string
                         path:
                           type: string
                         value:
@@ -3219,6 +3227,8 @@ spec:
                   clusterOverrides:
                     items:
                       properties:
+                        op:
+                          type: string
                         path:
                           type: string
                         value:
@@ -5508,6 +5518,8 @@ spec:
                   clusterOverrides:
                     items:
                       properties:
+                        op:
+                          type: string
                         path:
                           type: string
                         value:
@@ -5767,6 +5779,8 @@ spec:
                   clusterOverrides:
                     items:
                       properties:
+                        op:
+                          type: string
                         path:
                           type: string
                         value:
@@ -8045,6 +8059,8 @@ spec:
                   clusterOverrides:
                     items:
                       properties:
+                        op:
+                          type: string
                         path:
                           type: string
                         value:
@@ -8308,6 +8324,8 @@ spec:
                   clusterOverrides:
                     items:
                       properties:
+                        op:
+                          type: string
                         path:
                           type: string
                         value:
@@ -8588,6 +8606,8 @@ spec:
                   clusterOverrides:
                     items:
                       properties:
+                        op:
+                          type: string
                         path:
                           type: string
                         value:

example/sample1/federatedconfigmap.yaml

@@ -14,6 +14,6 @@ spec:
   overrides:
   - clusterName: cluster2
     clusterOverrides:
-    - path: data
+    - path: /data
       value:
         foo: bar

example/sample1/federateddeployment.yaml

@@ -28,5 +28,13 @@ spec:
   overrides:
   - clusterName: cluster2
     clusterOverrides:
-    - path: spec.replicas
+    - path: "/spec/replicas"
       value: 5
+    - path: "/spec/template/spec/containers/0/image"
+      value: "nginx:1.17.0-alpine"
+    - path: "/metadata/annotations"
+      op: "add"
+      value:
+        foo: bar
+    - path: "/metadata/annotations/foo"
+      op: "remove"

example/sample1/federatedjob.yaml

@@ -29,5 +29,5 @@ spec:
   overrides:
   - clusterName: cluster2
     clusterOverrides:
-    - path: spec.parallelism
+    - path: /spec/parallelism
       value: 2

example/sample1/federatedsecret.yaml

@@ -15,6 +15,6 @@ spec:
   overrides:
   - clusterName: cluster2
     clusterOverrides:
-    - path: data
+    - path: /data
       value:
         A: null

pkg/controller/sync/resource.go

@@ -183,11 +183,8 @@ func (r *federatedResource) ObjectForCluster(clusterName string) (*unstructured.
 		return nil, err
 	}
 	if overrides != nil {
-		for path, value := range overrides {
-			pathEntries := strings.Split(path, ".")
-			if err := unstructured.SetNestedField(obj.Object, value, pathEntries...); err != nil {
-				return nil, err
-			}
+		if err := util.ApplyJsonPatch(obj, overrides); err != nil {
+			return nil, err
 		}
 	}
 
@@ -208,7 +205,7 @@ func (r *federatedResource) RecordEvent(reason, messageFmt string, args ...inter
 	r.eventRecorder.Eventf(r.Object(), corev1.EventTypeNormal, reason, messageFmt, args...)
 }
 
-func (r *federatedResource) overridesForCluster(clusterName string) (util.ClusterOverridesMap, error) {
+func (r *federatedResource) overridesForCluster(clusterName string) (util.ClusterOverrides, error) {
 	r.Lock()
 	defer r.Unlock()
 	if r.overridesMap == nil {

pkg/controller/util/overrides.go

@@ -19,6 +19,7 @@ package util
 import (
 	"encoding/json"
 
+	"github.com/evanphx/json-patch"
 	"github.com/pkg/errors"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -27,8 +28,9 @@ import (
 )
 
 type ClusterOverride struct {
+	Op    string      `json:"op,omitempty"`
 	Path  string      `json:"path"`
-	Value interface{} `json:"value"`
+	Value interface{} `json:"value,omitempty"`
 }
 
 type GenericOverrideItem struct {
@@ -51,29 +53,22 @@ type GenericOverride struct {
 // primary mechanism of association between a federated resource in
 // the host cluster and the target resources in the member clusters.
 var invalidPaths = sets.NewString(
-	"metadata.namespace",
-	"metadata.name",
-	"metadata.generateName",
+	"/metadata/namespace",
+	"/metadata/name",
+	"/metadata/generateName",
 )
 
-// Mapping of qualified path (e.g. spec.replicas) to value
-type ClusterOverridesMap map[string]interface{}
+// Slice of ClusterOverride
+type ClusterOverrides []ClusterOverride
 
 // Mapping of clusterName to overrides for the cluster
-type OverridesMap map[string]ClusterOverridesMap
+type OverridesMap map[string]ClusterOverrides
 
 // ToUnstructuredSlice converts the map of overrides to a slice of
 // interfaces that can be set in an unstructured object.
 func (m OverridesMap) ToUnstructuredSlice() []interface{} {
 	overrides := []interface{}{}
-	for clusterName, clusterOverridesMap := range m {
-		clusterOverrides := []map[string]interface{}{}
-		for path, value := range clusterOverridesMap {
-			clusterOverrides = append(clusterOverrides, map[string]interface{}{
-				PathField:  path,
-				ValueField: value,
-			})
-		}
+	for clusterName, clusterOverrides := range m {
 		overridesItem := map[string]interface{}{
 			ClusterNameField:      clusterName,
 			ClusterOverridesField: clusterOverrides,
@@ -92,37 +87,37 @@ func GetOverrides(rawObj *unstructured.Unstructured) (OverridesMap, error) {
 		return overridesMap, nil
 	}
 
-	override := GenericOverride{}
-	err := UnstructuredToInterface(rawObj, &override)
+	genericFedObject := GenericOverride{}
+	err := UnstructuredToInterface(rawObj, &genericFedObject)
 	if err != nil {
 		return nil, err
 	}
 
-	if override.Spec == nil || override.Spec.Overrides == nil {
+	if genericFedObject.Spec == nil || genericFedObject.Spec.Overrides == nil {
 		// No overrides defined for the federated type
 		return overridesMap, nil
 	}
 
-	for _, overrideItem := range override.Spec.Overrides {
+	for _, overrideItem := range genericFedObject.Spec.Overrides {
 		clusterName := overrideItem.ClusterName
 		if _, ok := overridesMap[clusterName]; ok {
 			return nil, errors.Errorf("cluster %q appears more than once", clusterName)
 		}
-		overridesMap[clusterName] = make(ClusterOverridesMap)
 
 		clusterOverrides := overrideItem.ClusterOverrides
 
+		paths := sets.NewString()
 		for i, clusterOverride := range clusterOverrides {
 			path := clusterOverride.Path
 			if invalidPaths.Has(path) {
 				return nil, errors.Errorf("override[%d] for cluster %q has an invalid path: %s", i, clusterName, path)
 			}
-			if _, ok := overridesMap[clusterName][path]; ok {
+			if paths.Has(path) {
 				return nil, errors.Errorf("path %q appears more than once for cluster %q", path, clusterName)
 			}
-
-			overridesMap[clusterName][path] = clusterOverride.Value
+			paths.Insert(path)
 		}
+		overridesMap[clusterName] = clusterOverrides
 	}
 
 	return overridesMap, nil
@@ -154,3 +149,35 @@ func UnstructuredToInterface(rawObj *unstructured.Unstructured, obj interface{})
 	}
 	return json.Unmarshal(content, obj)
 }
+
+// ApplyJsonPatch applies the override on to the given unstructured object.
+func ApplyJsonPatch(obj *unstructured.Unstructured, overrides ClusterOverrides) error {
+	// TODO: Do the defaulting of "op" field to "replace" in API defaulting
+	for i, overrideItem := range overrides {
+		if overrideItem.Op == "" {
+			overrides[i].Op = "replace"
+		}
+	}
+	jsonPatchBytes, err := json.Marshal(overrides)
+	if err != nil {
+		return err
+	}
+
+	patch, err := jsonpatch.DecodePatch(jsonPatchBytes)
+	if err != nil {
+		return err
+	}
+
+	ObjectJSONBytes, err := obj.MarshalJSON()
+	if err != nil {
+		return err
+	}
+
+	patchedObjectJSONBytes, err := patch.Apply(ObjectJSONBytes)
+	if err != nil {
+		return err
+	}
+
+	err = obj.UnmarshalJSON(patchedObjectJSONBytes)
+	return err
+}

pkg/kubefedctl/enable/validation.go

@@ -107,6 +107,9 @@ func federatedTypeValidationSchema(templateSchema map[string]v1beta1.JSONSchemaP
 									Schema: &v1beta1.JSONSchemaProps{
 										Type: "object",
 										Properties: map[string]v1beta1.JSONSchemaProps{
+											"op": {
+												Type: "string",
+											},
 											"path": {
 												Type: "string",
 											},

pkg/schedulingtypes/plugin.go

@@ -36,7 +36,7 @@ import (
 )
 
 const (
-	replicasPath = "spec.replicas"
+	replicasPath = "/spec/replicas"
 )
 
 type Plugin struct {
@@ -199,27 +199,45 @@ func setOverrides(obj *unstructured.Unstructured, overridesMap util.OverridesMap
 
 func updateOverridesMap(overridesMap util.OverridesMap, replicasMap map[string]int64) {
 	// Remove replicas override for clusters that are not scheduled
-	for clusterName, clusterOverridesMap := range overridesMap {
+	for clusterName, clusterOverrides := range overridesMap {
 		if _, ok := replicasMap[clusterName]; !ok {
-			delete(clusterOverridesMap, replicasPath)
+			for i, overrideItem := range clusterOverrides {
+				if overrideItem.Path == replicasPath {
+					clusterOverrides = append(clusterOverrides[:i], clusterOverrides[i+1:]...)
+					overridesMap[clusterName] = clusterOverrides
+					break
+				}
+			}
 		}
 	}
 	// Add/update replicas override for clusters that are scheduled
 	for clusterName, replicas := range replicasMap {
-		clusterOverridesMap, ok := overridesMap[clusterName]
-		if !ok {
-			clusterOverridesMap = make(util.ClusterOverridesMap)
-			overridesMap[clusterName] = clusterOverridesMap
+		replicasOverrideFound := false
+		for _, overrideItem := range overridesMap[clusterName] {
+			if overrideItem.Path == replicasPath {
+				overrideItem.Value = replicas
+				replicasOverrideFound = true
+				break
+			}
+		}
+		if !replicasOverrideFound {
+			clusterOverrides, exist := overridesMap[clusterName]
+			if !exist {
+				clusterOverrides = util.ClusterOverrides{}
+			}
+			clusterOverrides = append(clusterOverrides, util.ClusterOverride{Path: replicasPath, Value: replicas})
+			overridesMap[clusterName] = clusterOverrides
 		}
-		clusterOverridesMap[replicasPath] = replicas
 	}
 }
 
 func OverrideUpdateNeeded(overridesMap util.OverridesMap, result map[string]int64) bool {
 	resultLen := len(result)
 	checkLen := 0
 	for clusterName, clusterOverridesMap := range overridesMap {
-		for path, rawValue := range clusterOverridesMap {
+		for _, overrideItem := range clusterOverridesMap {
+			path := overrideItem.Path
+			rawValue := overrideItem.Value
 			if path != replicasPath {
 				continue
 			}

scripts/deploy-federated-nginx.sh

@@ -89,7 +89,7 @@ echo "cluster2: $(curl -s ${IP2}:${PORT2})"
 echo
 echo "Updating override of federated deployment nginx to increase 'replicas' to 2 in cluster2."
 kubectl patch federateddeployment nginx -n ${TEST_NS} --type=merge --patch \
-    '{"spec" : {"overrides": [{"clusterName" : "cluster2", "clusterOverrides": [{"path": "spec.replicas", "value" : 2}]}]}}'
+    '{"spec" : {"overrides": [{"clusterName" : "cluster2", "clusterOverrides": [{"path": "/spec/replicas", "value" : 2}]}]}}'
 echo "Checking 'replicas' of deployment nginx in cluster2."
 util::wait-for-condition 'replicas updated in cluster2' \
     "(kubectl get deployment nginx -n ${TEST_NS} --context cluster2 -o jsonpath='{.status.replicas}' | grep "^2$") &> /dev/null" 120