From 51d1da1e945c691eb8b41d274e145b356f29f7ac Mon Sep 17 00:00:00 2001 From: Phillip Wittrock Date: Tue, 31 Oct 2017 14:37:37 -0700 Subject: [PATCH] Support retainkeys strategy in new apply merge code --- pkg/kubectl/apply/strategy/BUILD | 1 + .../apply/strategy/retain_keys_test.go | 195 ++++++++++++++++++ .../apply/strategy/retain_keys_visitor.go | 60 +++++- .../apply/strategy/strategic_visitor.go | 17 +- 4 files changed, 266 insertions(+), 7 deletions(-) create mode 100644 pkg/kubectl/apply/strategy/retain_keys_test.go diff --git a/pkg/kubectl/apply/strategy/BUILD b/pkg/kubectl/apply/strategy/BUILD index b489a954517..e1810a7862f 100644 --- a/pkg/kubectl/apply/strategy/BUILD +++ b/pkg/kubectl/apply/strategy/BUILD @@ -25,6 +25,7 @@ go_test( "replace_map_list_test.go", "replace_map_test.go", "replace_primitive_list_test.go", + "retain_keys_test.go", "suite_test.go", "utils_test.go", ], diff --git a/pkg/kubectl/apply/strategy/retain_keys_test.go b/pkg/kubectl/apply/strategy/retain_keys_test.go new file mode 100644 index 00000000000..481da91a098 --- /dev/null +++ b/pkg/kubectl/apply/strategy/retain_keys_test.go @@ -0,0 +1,195 @@ +/* +Copyright 2017 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package strategy_test + +import ( + . "github.com/onsi/ginkgo" + + "k8s.io/kubernetes/pkg/kubectl/apply/strategy" +) + +var _ = Describe("Merging fields with the retainkeys strategy", func() { + Context("where some fields are only defined remotely", func() { + It("should drop those fields ", func() { + recorded := create(` +apiVersion: extensions/v1beta1 +kind: Deployment +spec: + strategy: +`) + local := create(` +apiVersion: extensions/v1beta1 +kind: Deployment +spec: + strategy: + type: Recreate +`) + remote := create(` +apiVersion: extensions/v1beta1 +kind: Deployment +spec: + strategy: + type: RollingUpdate + rollingUpdate: + maxUnavailable: 1 + maxSurge: 1 +`) + expected := create(` +apiVersion: extensions/v1beta1 +kind: Deployment +spec: + strategy: + type: Recreate +`) + run(strategy.Create(strategy.Options{}), recorded, local, remote, expected) + }) + }) + + Context("where some fields are defined both locally and remotely", func() { + It("should merge those fields", func() { + recorded := create(` +apiVersion: extensions/v1beta1 +kind: Deployment +spec: + strategy: +`) + local := create(` +apiVersion: extensions/v1beta1 +kind: Deployment +spec: + strategy: + type: RollingUpdate + rollingUpdate: + maxUnavailable: 2 +`) + remote := create(` +apiVersion: extensions/v1beta1 +kind: Deployment +spec: + strategy: + type: RollingUpdate + rollingUpdate: + maxSurge: 1 +`) + expected := create(` +apiVersion: extensions/v1beta1 +kind: Deployment +spec: + strategy: + type: RollingUpdate + rollingUpdate: + maxUnavailable: 2 + maxSurge: 1 +`) + run(strategy.Create(strategy.Options{}), recorded, local, remote, expected) + }) + }) + + Context("where the elements are in a list and some fields are only defined remotely", func() { + It("should drop those fields ", func() { + recorded := create(` +apiVersion: apps/v1beta1 +kind: Deployment +spec: + template: + spec: +`) + local := create(` +apiVersion: apps/v1beta1 +kind: Deployment +spec: + template: + spec: + volumes: + - name: cache-volume + emptyDir: +`) + remote := create(` +apiVersion: apps/v1beta1 +kind: Deployment +spec: + template: + spec: + volumes: + - name: cache-volume + hostPath: + path: /tmp/cache-volume +`) + expected := create(` +apiVersion: apps/v1beta1 +kind: Deployment +spec: + template: + spec: + volumes: + - name: cache-volume + emptyDir: +`) + run(strategy.Create(strategy.Options{}), recorded, local, remote, expected) + }) + }) + + Context("where the elements are in a list", func() { + It("the fields defined both locally and remotely should be merged", func() { + recorded := create(` +apiVersion: apps/v1beta1 +kind: Deployment +spec: + template: + spec: +`) + local := create(` +apiVersion: apps/v1beta1 +kind: Deployment +spec: + template: + spec: + volumes: + - name: cache-volume + hostPath: + path: /tmp/cache-volume + emptyDir: +`) + remote := create(` +apiVersion: apps/v1beta1 +kind: Deployment +spec: + template: + spec: + volumes: + - name: cache-volume + hostPath: + path: /tmp/cache-volume + type: Directory +`) + expected := create(` +apiVersion: apps/v1beta1 +kind: Deployment +spec: + template: + spec: + volumes: + - name: cache-volume + hostPath: + path: /tmp/cache-volume + type: Directory + emptyDir: +`) + run(strategy.Create(strategy.Options{}), recorded, local, remote, expected) + }) + }) +}) diff --git a/pkg/kubectl/apply/strategy/retain_keys_visitor.go b/pkg/kubectl/apply/strategy/retain_keys_visitor.go index 2483af0fab3..b901285be1e 100644 --- a/pkg/kubectl/apply/strategy/retain_keys_visitor.go +++ b/pkg/kubectl/apply/strategy/retain_keys_visitor.go @@ -16,4 +16,62 @@ limitations under the License. package strategy -// TODO: Write this +import ( + "fmt" + "k8s.io/kubernetes/pkg/kubectl/apply" +) + +func createRetainKeysStrategy(options Options, strategic *delegatingStrategy) retainKeysStrategy { + return retainKeysStrategy{ + &mergeStrategy{strategic, options}, + strategic, + options, + } +} + +// retainKeysStrategy merges the values in an Element into a single Result, +// dropping any fields omitted from the local copy. (but merging values when +// defined locally and remotely) +type retainKeysStrategy struct { + merge *mergeStrategy + strategic *delegatingStrategy + options Options +} + +// MergeMap merges the type instances in a TypeElement into a single Result +// keeping only the fields defined locally, but merging their values with +// the remote values. +func (v retainKeysStrategy) MergeType(e apply.TypeElement) (apply.Result, error) { + // No merge logic if adding or deleting a field + if result, done := v.merge.doAddOrDelete(&e); done { + return result, nil + } + + elem := map[string]apply.Element{} + for key := range e.GetLocalMap() { + elem[key] = e.GetValues()[key] + } + return v.merge.doMergeMap(elem) +} + +// MergeMap returns an error. Only TypeElements can have retainKeys. +func (v retainKeysStrategy) MergeMap(e apply.MapElement) (apply.Result, error) { + return apply.Result{}, fmt.Errorf("Cannot use retainkeys with map element %v", e.Name) +} + +// MergeList returns an error. Only TypeElements can have retainKeys. +func (v retainKeysStrategy) MergeList(e apply.ListElement) (apply.Result, error) { + return apply.Result{}, fmt.Errorf("Cannot use retainkeys with list element %v", e.Name) +} + +// MergePrimitive returns an error. Only TypeElements can have retainKeys. +func (v retainKeysStrategy) MergePrimitive(diff apply.PrimitiveElement) (apply.Result, error) { + return apply.Result{}, fmt.Errorf("Cannot use retainkeys with primitive element %v", diff.Name) +} + +// MergeEmpty returns an empty result +func (v retainKeysStrategy) MergeEmpty(diff apply.EmptyElement) (apply.Result, error) { + return v.merge.MergeEmpty(diff) +} + +var _ apply.Strategy = &retainKeysStrategy{} diff --git a/pkg/kubectl/apply/strategy/strategic_visitor.go b/pkg/kubectl/apply/strategy/strategic_visitor.go index bd198e7e9fa..eae1f9230ad 100644 --- a/pkg/kubectl/apply/strategy/strategic_visitor.go +++ b/pkg/kubectl/apply/strategy/strategic_visitor.go @@ -23,9 +23,10 @@ import ( // delegatingStrategy delegates merging fields to other visitor implementations // based on the merge strategy preferred by the field. type delegatingStrategy struct { - options Options - merge mergeStrategy - replace replaceStrategy + options Options + merge mergeStrategy + replace replaceStrategy + retainKeys retainKeysStrategy } // createDelegatingStrategy returns a new delegatingStrategy @@ -35,18 +36,20 @@ func createDelegatingStrategy(options Options) *delegatingStrategy { } v.replace = createReplaceStrategy(options, v) v.merge = createMergeStrategy(options, v) + v.retainKeys = createRetainKeysStrategy(options, v) return v } // MergeList delegates visiting a list based on the field patch strategy. // Defaults to "replace" func (v delegatingStrategy) MergeList(diff apply.ListElement) (apply.Result, error) { - // TODO: Support retainkeys switch diff.GetFieldMergeType() { case apply.MergeStrategy: return v.merge.MergeList(diff) case apply.ReplaceStrategy: return v.replace.MergeList(diff) + case apply.RetainKeysStrategy: + return v.retainKeys.MergeList(diff) default: return v.replace.MergeList(diff) } @@ -55,12 +58,13 @@ func (v delegatingStrategy) MergeList(diff apply.ListElement) (apply.Result, err // MergeMap delegates visiting a map based on the field patch strategy. // Defaults to "merge" func (v delegatingStrategy) MergeMap(diff apply.MapElement) (apply.Result, error) { - // TODO: Support retainkeys switch diff.GetFieldMergeType() { case apply.MergeStrategy: return v.merge.MergeMap(diff) case apply.ReplaceStrategy: return v.replace.MergeMap(diff) + case apply.RetainKeysStrategy: + return v.retainKeys.MergeMap(diff) default: return v.merge.MergeMap(diff) } @@ -69,12 +73,13 @@ func (v delegatingStrategy) MergeMap(diff apply.MapElement) (apply.Result, error // MergeType delegates visiting a map based on the field patch strategy. // Defaults to "merge" func (v delegatingStrategy) MergeType(diff apply.TypeElement) (apply.Result, error) { - // TODO: Support retainkeys switch diff.GetFieldMergeType() { case apply.MergeStrategy: return v.merge.MergeType(diff) case apply.ReplaceStrategy: return v.replace.MergeType(diff) + case apply.RetainKeysStrategy: + return v.retainKeys.MergeType(diff) default: return v.merge.MergeType(diff) }