diff --git a/charts/kubefed/templates/crds.yaml b/charts/kubefed/templates/crds.yaml index c64d3ab42c..c76830a16b 100644 --- a/charts/kubefed/templates/crds.yaml +++ b/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: diff --git a/example/sample1/federatedconfigmap.yaml b/example/sample1/federatedconfigmap.yaml index 1e0076e6d6..631138d540 100644 --- a/example/sample1/federatedconfigmap.yaml +++ b/example/sample1/federatedconfigmap.yaml @@ -14,6 +14,6 @@ spec: overrides: - clusterName: cluster2 clusterOverrides: - - path: data + - path: /data value: foo: bar diff --git a/example/sample1/federateddeployment.yaml b/example/sample1/federateddeployment.yaml index af8b22ce6f..cda5a97f3b 100644 --- a/example/sample1/federateddeployment.yaml +++ b/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" diff --git a/example/sample1/federatedjob.yaml b/example/sample1/federatedjob.yaml index 048717d4e7..7d3dc639de 100644 --- a/example/sample1/federatedjob.yaml +++ b/example/sample1/federatedjob.yaml @@ -29,5 +29,5 @@ spec: overrides: - clusterName: cluster2 clusterOverrides: - - path: spec.parallelism + - path: /spec/parallelism value: 2 diff --git a/example/sample1/federatedsecret.yaml b/example/sample1/federatedsecret.yaml index c932288355..da9dc770a3 100644 --- a/example/sample1/federatedsecret.yaml +++ b/example/sample1/federatedsecret.yaml @@ -15,6 +15,6 @@ spec: overrides: - clusterName: cluster2 clusterOverrides: - - path: data + - path: /data value: A: null diff --git a/pkg/controller/sync/resource.go b/pkg/controller/sync/resource.go index ce8367819c..06692ed745 100644 --- a/pkg/controller/sync/resource.go +++ b/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 { diff --git a/pkg/controller/util/overrides.go b/pkg/controller/util/overrides.go index e4673cc585..8abb9afae9 100644 --- a/pkg/controller/util/overrides.go +++ b/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 +} diff --git a/pkg/kubefedctl/enable/validation.go b/pkg/kubefedctl/enable/validation.go index 508afdcfc0..74834d9a06 100644 --- a/pkg/kubefedctl/enable/validation.go +++ b/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", }, diff --git a/pkg/schedulingtypes/plugin.go b/pkg/schedulingtypes/plugin.go index 1ad4376997..acc24387c3 100644 --- a/pkg/schedulingtypes/plugin.go +++ b/pkg/schedulingtypes/plugin.go @@ -36,7 +36,7 @@ import ( ) const ( - replicasPath = "spec.replicas" + replicasPath = "/spec/replicas" ) type Plugin struct { @@ -199,19 +199,35 @@ 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 } } @@ -219,7 +235,9 @@ func OverrideUpdateNeeded(overridesMap util.OverridesMap, result map[string]int6 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 } diff --git a/scripts/deploy-federated-nginx.sh b/scripts/deploy-federated-nginx.sh index 466ef862ee..2d075055eb 100755 --- a/scripts/deploy-federated-nginx.sh +++ b/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 diff --git a/test/common/crudtester.go b/test/common/crudtester.go index 2a7ed57ad5..e7f44590cd 100644 --- a/test/common/crudtester.go +++ b/test/common/crudtester.go @@ -19,10 +19,10 @@ package common import ( "context" "fmt" - "reflect" "strings" "time" + "github.com/evanphx/json-patch" "github.com/pkg/errors" apiv1 "k8s.io/api/core/v1" @@ -172,7 +172,7 @@ func (c *FederatedTypeCrudTester) CheckUpdate(fedObject *unstructured.Unstructur kind := apiResource.Kind qualifiedName := util.NewQualifiedName(fedObject) - key := "metadata.labels" + key := "/metadata/labels" value := map[string]interface{}{ "crudtester-operation": "update", util.ManagedByKubeFedLabelKey: util.ManagedByKubeFedLabelValue, @@ -185,16 +185,18 @@ func (c *FederatedTypeCrudTester) CheckUpdate(fedObject *unstructured.Unstructur c.tl.Fatalf("Error retrieving overrides for %s %q: %v", kind, qualifiedName, err) } for clusterName := range c.testClusters { - clusterOverrides, ok := overrides[clusterName] - if !ok { - clusterOverrides = make(util.ClusterOverridesMap) - overrides[clusterName] = clusterOverrides + if _, ok := overrides[clusterName]; !ok { + overrides[clusterName] = util.ClusterOverrides{} } - _, ok = clusterOverrides[key] - if ok { + paths := sets.NewString() + for _, overrideItem := range overrides[clusterName] { + paths.Insert(overrideItem.Path) + } + if paths.Has(key) { c.tl.Fatalf("An override for %q already exists for cluster %q", key, clusterName) } - clusterOverrides[key] = value + paths.Insert(key) + overrides[clusterName] = append(overrides[clusterName], util.ClusterOverride{Path: key, Value: value}) } if err := util.SetOverrides(obj, overrides); err != nil { @@ -544,7 +546,7 @@ func (c *FederatedTypeCrudTester) checkHostNamespaceUnlabeled(client util.Resour } } -func (c *FederatedTypeCrudTester) waitForResource(client util.ResourceClient, qualifiedName util.QualifiedName, expectedOverrides util.ClusterOverridesMap, expectedVersionFunc func() string) error { +func (c *FederatedTypeCrudTester) waitForResource(client util.ResourceClient, qualifiedName util.QualifiedName, expectedOverrides util.ClusterOverrides, expectedVersionFunc func() string) error { err := wait.PollImmediate(c.waitInterval, c.clusterWaitTimeout, func() (bool, error) { expectedVersion := expectedVersionFunc() if len(expectedVersion) == 0 { @@ -564,25 +566,25 @@ func (c *FederatedTypeCrudTester) waitForResource(client util.ResourceClient, qu // Validate that the expected override was applied if len(expectedOverrides) > 0 { - for path, expectedValue := range expectedOverrides { - pathEntries := strings.Split(path, ".") - value, ok, err := unstructured.NestedFieldCopy(clusterObj.Object, pathEntries...) - if err != nil { - c.tl.Fatalf("Error retrieving overridden path: %v", err) - } - if !ok { - c.tl.Fatalf("Missing overridden path %s", path) - } - // Because the result of deserializing an override field differs from the value - // retrieved by NestedFieldCopy, reflection is not able to accurately compare - // numeric types that should otherwise be equal. For example, an override value - // of 2 is deserialized as %!q(float64=2), but the same value retrieved by - // NestedFieldCopy would be '\x02'. String conversion is a hacky way of working - // around this problem, with a fallback to reflection for non-numeric types. - if fmt.Sprintf("%v", expectedValue) != fmt.Sprintf("%v", value) && !reflect.DeepEqual(expectedValue, value) { - c.tl.Errorf("Expected field %s to be %q, got %q", path, expectedValue, value) - return false, nil - } + expectedClusterObject := clusterObj.DeepCopy() + // Applying overrides on copy of received cluster object should not change the cluster object if the overrides are properly applied. + if err := util.ApplyJsonPatch(expectedClusterObject, expectedOverrides); err != nil { + c.tl.Fatalf("Failed to apply json patch: %v", err) + } + + expectedClusterObjectJSON, err := expectedClusterObject.MarshalJSON() + if err != nil { + c.tl.Fatalf("Failed to marshal expected cluster object to json: %v", err) + } + + clusterObjectJSON, err := clusterObj.MarshalJSON() + if err != nil { + c.tl.Fatalf("Failed to marshal cluster object to json: %v", err) + } + + if !jsonpatch.Equal(expectedClusterObjectJSON, clusterObjectJSON) { + c.tl.Errorf("Cluster object is not as expected. expected: %s, actual: %s", expectedClusterObjectJSON, clusterObjectJSON) + return false, nil } } diff --git a/test/common/fixtures/configmaps.yaml b/test/common/fixtures/configmaps.yaml index 96678a1f5f..d6f160b6fd 100644 --- a/test/common/fixtures/configmaps.yaml +++ b/test/common/fixtures/configmaps.yaml @@ -4,6 +4,6 @@ template: foo: bar overrides: - clusterOverrides: - - path: data + - path: /data value: foo: baz diff --git a/test/common/fixtures/deployments.apps.yaml b/test/common/fixtures/deployments.apps.yaml index 9dca10d58e..2953823091 100644 --- a/test/common/fixtures/deployments.apps.yaml +++ b/test/common/fixtures/deployments.apps.yaml @@ -1,7 +1,7 @@ kind: fixture template: spec: - replicas: 1 + replicas: 2 selector: matchLabels: foo: bar @@ -17,5 +17,13 @@ template: command: ["/bin/sh", "-c", "trap : TERM INT; (while true; do sleep 1000; done) & wait"] overrides: - clusterOverrides: - - path: spec.replicas - value: 2 + - path: /spec/replicas + value: 1 + - path: "/spec/template/spec/containers/0/image" + value: "busybox:1.31.0-uclibc" + - path: "/metadata/annotations" + op: "add" + value: + foo: bar + - path: "/metadata/annotations/foo" + op: "remove" diff --git a/test/common/fixtures/jobs.batch.yaml b/test/common/fixtures/jobs.batch.yaml index fc984d2abf..0055c6927e 100644 --- a/test/common/fixtures/jobs.batch.yaml +++ b/test/common/fixtures/jobs.batch.yaml @@ -19,5 +19,5 @@ template: command: ["/bin/sh", "-c", "trap : TERM INT; (while true; do sleep 1000; done) & wait"] overrides: - clusterOverrides: - - path: spec.parallelism + - path: /spec/parallelism value: 2 diff --git a/test/common/fixtures/replicasets.apps.yaml b/test/common/fixtures/replicasets.apps.yaml index 9dca10d58e..171f197ade 100644 --- a/test/common/fixtures/replicasets.apps.yaml +++ b/test/common/fixtures/replicasets.apps.yaml @@ -17,5 +17,5 @@ template: command: ["/bin/sh", "-c", "trap : TERM INT; (while true; do sleep 1000; done) & wait"] overrides: - clusterOverrides: - - path: spec.replicas + - path: /spec/replicas value: 2 diff --git a/test/common/fixtures/secrets.yaml b/test/common/fixtures/secrets.yaml index e8d3391fbe..42a74fa250 100644 --- a/test/common/fixtures/secrets.yaml +++ b/test/common/fixtures/secrets.yaml @@ -5,6 +5,6 @@ template: type: Opaque overrides: - clusterOverrides: - - path: data + - path: /data value: foo: YmF6 diff --git a/test/e2e/crd.go b/test/e2e/crd.go index e005ded4da..f0fd08b244 100644 --- a/test/e2e/crd.go +++ b/test/e2e/crd.go @@ -187,7 +187,7 @@ template: - bal overrides: - clusterOverrides: - - path: bar + - path: /spec/bar value: - fiz - bang