From 63b4e6d5c1085b279ede5c9bb448eaaad50fedbe Mon Sep 17 00:00:00 2001 From: shashidharatd Date: Wed, 19 Jun 2019 22:01:45 +0530 Subject: [PATCH 1/6] Replace overrides path with JSON pointer style paths and use Json patch to apply the overrides --- example/sample1/federatedconfigmap.yaml | 2 +- example/sample1/federateddeployment.yaml | 2 +- example/sample1/federatedjob.yaml | 2 +- example/sample1/federatedsecret.yaml | 2 +- pkg/controller/sync/resource.go | 3 +- pkg/controller/util/overrides.go | 44 ++++++++++++++++++++-- pkg/schedulingtypes/plugin.go | 2 +- scripts/deploy-federated-nginx.sh | 2 +- test/common/crudtester.go | 6 ++- test/common/fixtures/configmaps.yaml | 2 +- test/common/fixtures/deployments.apps.yaml | 2 +- test/common/fixtures/jobs.batch.yaml | 2 +- test/common/fixtures/replicasets.apps.yaml | 2 +- test/common/fixtures/secrets.yaml | 2 +- test/e2e/crd.go | 2 +- 15 files changed, 58 insertions(+), 19 deletions(-) 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..67abf32d38 100644 --- a/example/sample1/federateddeployment.yaml +++ b/example/sample1/federateddeployment.yaml @@ -28,5 +28,5 @@ spec: overrides: - clusterName: cluster2 clusterOverrides: - - path: spec.replicas + - path: /spec/replicas value: 5 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..bd903d2cf0 100644 --- a/pkg/controller/sync/resource.go +++ b/pkg/controller/sync/resource.go @@ -184,8 +184,7 @@ func (r *federatedResource) ObjectForCluster(clusterName string) (*unstructured. } if overrides != nil { for path, value := range overrides { - pathEntries := strings.Split(path, ".") - if err := unstructured.SetNestedField(obj.Object, value, pathEntries...); err != nil { + if err := util.ApplyJsonPatch(obj, path, value); err != nil { return nil, err } } diff --git a/pkg/controller/util/overrides.go b/pkg/controller/util/overrides.go index e4673cc585..f9ce2ffd3c 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" @@ -51,9 +52,9 @@ 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 @@ -154,3 +155,40 @@ 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, path string, value interface{}) error { + type jsonPatchOperation struct { + Op string `json:"op"` + Path string `json:"path"` + Value interface{} `json:"value"` + } + + jsonPatch := []jsonPatchOperation{{ + Op: "replace", + Path: path, + Value: value, + }} + jsonPatchBytes, err := json.Marshal(jsonPatch) + 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/schedulingtypes/plugin.go b/pkg/schedulingtypes/plugin.go index 1ad4376997..925be28bd7 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 { 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..5b8c298e40 100644 --- a/test/common/crudtester.go +++ b/test/common/crudtester.go @@ -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, @@ -564,8 +564,10 @@ func (c *FederatedTypeCrudTester) waitForResource(client util.ResourceClient, qu // Validate that the expected override was applied if len(expectedOverrides) > 0 { + // TODO: use the json patch library to patch with the overrides and then compare it with cluster object. for path, expectedValue := range expectedOverrides { - pathEntries := strings.Split(path, ".") + path = strings.TrimPrefix(path, "/") + pathEntries := strings.Split(path, "/") value, ok, err := unstructured.NestedFieldCopy(clusterObj.Object, pathEntries...) if err != nil { c.tl.Fatalf("Error retrieving overridden path: %v", err) 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..171f197ade 100644 --- a/test/common/fixtures/deployments.apps.yaml +++ b/test/common/fixtures/deployments.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/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 From 3844c72ca722004bfd6d63622c0a9a6ce743cd08 Mon Sep 17 00:00:00 2001 From: shashidharatd Date: Wed, 19 Jun 2019 22:07:02 +0530 Subject: [PATCH 2/6] Add a scenario to test overriding container image --- example/sample1/federateddeployment.yaml | 4 ++- test/common/crudtester.go | 40 +++++++++++----------- test/common/fixtures/deployments.apps.yaml | 6 ++-- 3 files changed, 27 insertions(+), 23 deletions(-) diff --git a/example/sample1/federateddeployment.yaml b/example/sample1/federateddeployment.yaml index 67abf32d38..641191541e 100644 --- a/example/sample1/federateddeployment.yaml +++ b/example/sample1/federateddeployment.yaml @@ -28,5 +28,7 @@ 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" diff --git a/test/common/crudtester.go b/test/common/crudtester.go index 5b8c298e40..80018a86b8 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" @@ -564,28 +564,28 @@ func (c *FederatedTypeCrudTester) waitForResource(client util.ResourceClient, qu // Validate that the expected override was applied if len(expectedOverrides) > 0 { - // TODO: use the json patch library to patch with the overrides and then compare it with cluster object. + expectedClusterObject := clusterObj.DeepCopy() for path, expectedValue := range expectedOverrides { - path = strings.TrimPrefix(path, "/") - 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 + // 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, path, expectedValue); 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 + } } return true, nil diff --git a/test/common/fixtures/deployments.apps.yaml b/test/common/fixtures/deployments.apps.yaml index 171f197ade..151c3a036f 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 @@ -18,4 +18,6 @@ template: overrides: - clusterOverrides: - path: /spec/replicas - value: 2 + value: 1 + - path: "/spec/template/spec/containers/0/image" + value: "busybox:1.31.0-uclibc" From eab85860c379238a77d76b579a7bf7c676d6325a Mon Sep 17 00:00:00 2001 From: shashidharatd Date: Fri, 21 Jun 2019 11:45:10 +0530 Subject: [PATCH 3/6] Add optional op field to cluster overrides --- pkg/controller/util/overrides.go | 3 ++- pkg/kubefedctl/enable/validation.go | 3 +++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/pkg/controller/util/overrides.go b/pkg/controller/util/overrides.go index f9ce2ffd3c..9328470b4a 100644 --- a/pkg/controller/util/overrides.go +++ b/pkg/controller/util/overrides.go @@ -28,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 { 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", }, From 9d87a3df25b75df5b832a592d78327f115d98d7e Mon Sep 17 00:00:00 2001 From: shashidharatd Date: Fri, 21 Jun 2019 11:46:44 +0530 Subject: [PATCH 4/6] Convert cluster overrides from map to slice --- pkg/controller/sync/resource.go | 8 ++--- pkg/controller/util/overrides.go | 50 ++++++++++++-------------------- pkg/schedulingtypes/plugin.go | 34 +++++++++++++++++----- test/common/crudtester.go | 26 ++++++++--------- 4 files changed, 61 insertions(+), 57 deletions(-) diff --git a/pkg/controller/sync/resource.go b/pkg/controller/sync/resource.go index bd903d2cf0..06692ed745 100644 --- a/pkg/controller/sync/resource.go +++ b/pkg/controller/sync/resource.go @@ -183,10 +183,8 @@ func (r *federatedResource) ObjectForCluster(clusterName string) (*unstructured. return nil, err } if overrides != nil { - for path, value := range overrides { - if err := util.ApplyJsonPatch(obj, path, value); err != nil { - return nil, err - } + if err := util.ApplyJsonPatch(obj, overrides); err != nil { + return nil, err } } @@ -207,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 9328470b4a..8abb9afae9 100644 --- a/pkg/controller/util/overrides.go +++ b/pkg/controller/util/overrides.go @@ -58,24 +58,17 @@ var invalidPaths = sets.NewString( "/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, @@ -94,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 @@ -158,19 +151,14 @@ func UnstructuredToInterface(rawObj *unstructured.Unstructured, obj interface{}) } // ApplyJsonPatch applies the override on to the given unstructured object. -func ApplyJsonPatch(obj *unstructured.Unstructured, path string, value interface{}) error { - type jsonPatchOperation struct { - Op string `json:"op"` - Path string `json:"path"` - Value interface{} `json:"value"` +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" + } } - - jsonPatch := []jsonPatchOperation{{ - Op: "replace", - Path: path, - Value: value, - }} - jsonPatchBytes, err := json.Marshal(jsonPatch) + jsonPatchBytes, err := json.Marshal(overrides) if err != nil { return err } diff --git a/pkg/schedulingtypes/plugin.go b/pkg/schedulingtypes/plugin.go index 925be28bd7..acc24387c3 100644 --- a/pkg/schedulingtypes/plugin.go +++ b/pkg/schedulingtypes/plugin.go @@ -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/test/common/crudtester.go b/test/common/crudtester.go index 80018a86b8..e7f44590cd 100644 --- a/test/common/crudtester.go +++ b/test/common/crudtester.go @@ -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 { @@ -565,11 +567,9 @@ func (c *FederatedTypeCrudTester) waitForResource(client util.ResourceClient, qu // Validate that the expected override was applied if len(expectedOverrides) > 0 { expectedClusterObject := clusterObj.DeepCopy() - for path, expectedValue := range expectedOverrides { - // 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, path, expectedValue); err != nil { - c.tl.Fatalf("Failed to apply json patch: %v", err) - } + // 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() From dcab3e2a956f94c57332395d1af03ef93cc04208 Mon Sep 17 00:00:00 2001 From: shashidharatd Date: Fri, 21 Jun 2019 11:51:43 +0530 Subject: [PATCH 5/6] Auto generated --- charts/kubefed/templates/crds.yaml | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) 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: From d5d0d7eea446b63ddbf70a5742f30701e112c926 Mon Sep 17 00:00:00 2001 From: shashidharatd Date: Fri, 21 Jun 2019 14:58:30 +0530 Subject: [PATCH 6/6] Add test to validate add and remove type of overrides --- example/sample1/federateddeployment.yaml | 6 ++++++ test/common/fixtures/deployments.apps.yaml | 6 ++++++ 2 files changed, 12 insertions(+) diff --git a/example/sample1/federateddeployment.yaml b/example/sample1/federateddeployment.yaml index 641191541e..cda5a97f3b 100644 --- a/example/sample1/federateddeployment.yaml +++ b/example/sample1/federateddeployment.yaml @@ -32,3 +32,9 @@ spec: 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/test/common/fixtures/deployments.apps.yaml b/test/common/fixtures/deployments.apps.yaml index 151c3a036f..2953823091 100644 --- a/test/common/fixtures/deployments.apps.yaml +++ b/test/common/fixtures/deployments.apps.yaml @@ -21,3 +21,9 @@ overrides: 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"