Merge pull request #54872 from pwittrock/apply

Automatic merge from submit-queue (batch tested with PRs 52367, 53363, 54989, 54872, 54643). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Support retainkeys strategy for new merge code

- Some prefactoring for retainkeys
- Add retainkeys strategy


```release-note
NONE
```

pkg/kubectl/apply/element.go

@@ -78,16 +78,14 @@ type FieldMeta interface {
 
 // FieldMetaImpl implements FieldMeta
 type FieldMetaImpl struct {
-	// The type of merge strategy to use for this field
+	// MergeType is the type of merge strategy to use for this field
 	// maybe "merge", "replace" or "retainkeys"
-	// TODO: There maybe multiple strategies, so this may need to be a slice, map, or struct
-	// Address this in a follow up in the PR to introduce retainkeys strategy
 	MergeType string
 
-	// The merge key to use when the MergeType is "merge" and underlying type is a list
+	// MergeKeys are the merge keys to use when the MergeType is "merge" and underlying type is a list
 	MergeKeys MergeKeys
 
-	// The openapi type of the field - "list", "primitive", "map"
+	// Type is the openapi type of the field - "list", "primitive", "map"
 	Type string
 
 	// Name contains of the field

pkg/kubectl/apply/parse/factory.go

@@ -78,13 +78,13 @@ func (v *ElementBuildingVisitor) getItem(s proto.Schema, name string, data apply
 		reflect.String:
 		p, err := getPrimitive(s)
 		if err != nil {
-			return nil, fmt.Errorf("expected openapi Primitive, was %T for %v", s, kind)
+			return nil, fmt.Errorf("expected openapi Primitive, was %T for %v (%v)", s, kind, err)
 		}
 		return &primitiveItem{name, p, data}, nil
 	case reflect.Array, reflect.Slice:
 		a, err := getArray(s)
 		if err != nil {
-			return nil, fmt.Errorf("expected openapi Array, was %T for %v", s, kind)
+			return nil, fmt.Errorf("expected openapi Array, was %T for %v (%v)", s, kind, err)
 		}
 		return &listItem{
 			Name:  name,
@@ -106,7 +106,7 @@ func (v *ElementBuildingVisitor) getItem(s proto.Schema, name string, data apply
 		// If it looks like a map, and no openapi type is found, default to mapItem
 		m, err := getMap(s)
 		if err != nil {
-			return nil, fmt.Errorf("expected openapi Kind or Map, was %T for %v", s, kind)
+			return nil, fmt.Errorf("expected openapi Kind or Map, was %T for %v (%v)", s, kind, err)
 		}
 		return &mapItem{
 			Name: name,

pkg/kubectl/apply/parse/list_element.go

@@ -45,7 +45,7 @@ func (v ElementBuildingVisitor) mergeListElement(meta apply.FieldMetaImpl, item
 func (v ElementBuildingVisitor) doPrimitiveList(meta apply.FieldMetaImpl, item *listItem) (*apply.ListElement, error) {
 	result := &apply.ListElement{
 		FieldMetaImpl: apply.FieldMetaImpl{
-			MergeType: "merge",
+			MergeType: apply.MergeStrategy,
 			Name:      item.Name,
 		},
 		ListElementData: item.ListElementData,
@@ -101,7 +101,7 @@ func (v ElementBuildingVisitor) doMapList(meta apply.FieldMetaImpl, item *listIt
 	key := meta.GetFieldMergeKeys()
 	result := &apply.ListElement{
 		FieldMetaImpl: apply.FieldMetaImpl{
-			MergeType: "merge",
+			MergeType: apply.MergeStrategy,
 			MergeKeys: key,
 			Name:      item.Name,
 		},

pkg/kubectl/apply/parse/openapi.go

@@ -18,6 +18,7 @@ package parse
 
 import (
 	"fmt"
+	"strings"
 
 	"k8s.io/kube-openapi/pkg/util/proto"
 )
@@ -123,6 +124,22 @@ func (v *kindSchemaVisitor) VisitKind(result *proto.Kind) {
 // VisitReference implements openapi
 func (v *kindSchemaVisitor) VisitReference(reference proto.Reference) {
 	reference.SubSchema().Accept(v)
+	if v.Err == nil {
+		v.Err = copyExtensions(reference.GetPath().String(), reference.GetExtensions(), v.Result.Extensions)
+	}
+}
+
+func copyExtensions(field string, from, to map[string]interface{}) error {
+	// Copy extensions from field to type for references
+	for key, val := range from {
+		if curr, found := to[key]; found {
+			// Don't allow the same extension to be defined both on the field and on the type
+			return fmt.Errorf("Cannot override value for extension %s on field %s from %v to %v",
+				key, field, curr, val)
+		}
+		to[key] = val
+	}
+	return nil
 }
 
 type mapSchemaVisitor struct {
@@ -139,6 +156,9 @@ func (v *mapSchemaVisitor) VisitMap(result *proto.Map) {
 // VisitReference implements openapi
 func (v *mapSchemaVisitor) VisitReference(reference proto.Reference) {
 	reference.SubSchema().Accept(v)
+	if v.Err == nil {
+		v.Err = copyExtensions(reference.GetPath().String(), reference.GetExtensions(), v.Result.Extensions)
+	}
 }
 
 type arraySchemaVisitor struct {
@@ -150,11 +170,41 @@ type arraySchemaVisitor struct {
 func (v *arraySchemaVisitor) VisitArray(result *proto.Array) {
 	v.Result = result
 	v.Kind = "array"
+	v.Err = copySubElementPatchStrategy(result.Path.String(), result.GetExtensions(), result.SubType.GetExtensions())
+}
+
+// copyPatchStrategy copies the strategies to subelements to the subtype
+// e.g. PodTemplate.Volumes is a []Volume with "x-kubernetes-patch-strategy": "merge,retainKeys"
+// the "retainKeys" strategy applies to merging Volumes, and must be copied to the sub element
+func copySubElementPatchStrategy(field string, from, to map[string]interface{}) error {
+	// Check if the parent has a patch strategy extension
+	if ext, found := from["x-kubernetes-patch-strategy"]; found {
+		strategy, ok := ext.(string)
+		if !ok {
+			return fmt.Errorf("Expected string value for x-kubernetes-patch-strategy on %s, was %T",
+				field, ext)
+		}
+		// Check of the parent patch strategy has a sub patch strategy, and if so copy to the sub type
+		if strings.Contains(strategy, ",") {
+			strategies := strings.Split(strategy, ",")
+			if len(strategies) != 2 {
+				// Only 1 sub strategy is supported
+				return fmt.Errorf(
+					"Expected between 0 and 2 elements for x-kubernetes-patch-merge-strategy by got %v",
+					strategies)
+			}
+			to["x-kubernetes-patch-strategy"] = strategies[1]
+		}
+	}
+	return nil
 }
 
 // MergePrimitive implements openapi
 func (v *arraySchemaVisitor) VisitReference(reference proto.Reference) {
 	reference.SubSchema().Accept(v)
+	if v.Err == nil {
+		v.Err = copyExtensions(reference.GetPath().String(), reference.GetExtensions(), v.Result.Extensions)
+	}
 }
 
 type primitiveSchemaVisitor struct {
@@ -171,4 +221,7 @@ func (v *primitiveSchemaVisitor) VisitPrimitive(result *proto.Primitive) {
 // VisitReference implements openapi
 func (v *primitiveSchemaVisitor) VisitReference(reference proto.Reference) {
 	reference.SubSchema().Accept(v)
+	if v.Err == nil {
+		v.Err = copyExtensions(reference.GetPath().String(), reference.GetExtensions(), v.Result.Extensions)
+	}
 }

pkg/kubectl/apply/parse/util.go

@@ -101,13 +101,20 @@ func getFieldMeta(s proto.Schema, name string) (apply.FieldMetaImpl, error) {
 	m := apply.FieldMetaImpl{}
 	if s != nil {
 		ext := s.GetExtensions()
-		if s, found := ext["x-kubernetes-patch-strategy"]; found {
+		if e, found := ext["x-kubernetes-patch-strategy"]; found {
-			strategy, ok := s.(string)
+			strategy, ok := e.(string)
 			if !ok {
 				return apply.FieldMetaImpl{}, fmt.Errorf("Expected string for x-kubernetes-patch-strategy by got %T", s)
 			}
-			// TODO: Support multi-strategy
+
-			m.MergeType = strategy
+			// Take the first strategy if there are substrategies.
+			// Sub strategies are copied to sub types in openapi.go
+			strategies := strings.Split(strategy, ",")
+			if len(strategies) > 2 {
+				return apply.FieldMetaImpl{}, fmt.Errorf("Expected between 0 and 2 elements for x-kubernetes-patch-merge-strategy by got %v", strategies)
+			}
+			// For lists, choose the strategy for this type, not the subtype
+			m.MergeType = strategies[0]
 		}
 		if k, found := ext["x-kubernetes-patch-merge-key"]; found {
 			key, ok := k.(string)

pkg/kubectl/apply/parse/visitor.go

@@ -54,7 +54,7 @@ func (v ElementBuildingVisitor) CreateListElement(item *listItem) (apply.Element
 	if err != nil {
 		return nil, err
 	}
-	if meta.GetFieldMergeType() == "merge" {
+	if meta.GetFieldMergeType() == apply.MergeStrategy {
 		return v.mergeListElement(meta, item)
 	}
 	return v.replaceListElement(meta, item)

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",
     ],

pkg/kubectl/apply/strategy/merge_visitor.go

@@ -29,13 +29,13 @@ func createMergeStrategy(options Options, strategic *delegatingStrategy) mergeSt
 	}
 }
 
-// mergeStrategy creates a patch to merge a local file value into a remote field value
+// mergeStrategy merges the values in an Element into a single Result
 type mergeStrategy struct {
 	strategic *delegatingStrategy
 	options   Options
 }
 
-// MergeList creates a patch to merge a local list field value into a remote list field value
+// MergeList merges the lists in a ListElement into a single Result
 func (v mergeStrategy) MergeList(e apply.ListElement) (apply.Result, error) {
 	// No merge logic if adding or deleting a field
 	if result, done := v.doAddOrDelete(e); done {
@@ -70,26 +70,32 @@ func (v mergeStrategy) MergeList(e apply.ListElement) (apply.Result, error) {
 	return apply.Result{Operation: apply.SET, MergedResult: merged}, nil
 }
 
-// MergeMap creates a patch to merge a local map field into a remote map field
+// MergeMap merges the maps in a MapElement into a single Result
 func (v mergeStrategy) MergeMap(e apply.MapElement) (apply.Result, error) {
-	return v.doMergeMap(e)
-}
-
-// MergeType creates a patch to merge a local map field into a remote map field
-func (v mergeStrategy) MergeType(e apply.TypeElement) (apply.Result, error) {
-	return v.doMergeMap(e)
-}
-
-// do merges a recorded, local and remote map into a new object
-func (v mergeStrategy) doMergeMap(e apply.MapValuesElement) (apply.Result, error) {
 	// No merge logic if adding or deleting a field
 	if result, done := v.doAddOrDelete(e); done {
 		return result, nil
 	}
 
+	return v.doMergeMap(e.GetValues())
+}
+
+// MergeMap merges the type instances in a TypeElement into a single Result
+func (v mergeStrategy) MergeType(e apply.TypeElement) (apply.Result, error) {
+	// No merge logic if adding or deleting a field
+	if result, done := v.doAddOrDelete(e); done {
+		return result, nil
+	}
+
+	return v.doMergeMap(e.GetValues())
+}
+
+// do merges a recorded, local and remote map into a new object
+func (v mergeStrategy) doMergeMap(e map[string]apply.Element) (apply.Result, error) {
+
 	// Merge each item in the list
 	merged := map[string]interface{}{}
-	for key, value := range e.GetValues() {
+	for key, value := range e {
 		// Recursively merge the map element before adding the value to the map
 		result, err := value.Merge(v.strategic)
 		if err != nil {
@@ -134,7 +140,7 @@ func (v mergeStrategy) MergePrimitive(diff apply.PrimitiveElement) (apply.Result
 	return apply.Result{}, fmt.Errorf("Cannot merge primitive element %v", diff.Name)
 }
 
-// MergeEmpty
+// MergeEmpty returns an empty result
 func (v mergeStrategy) MergeEmpty(diff apply.EmptyElement) (apply.Result, error) {
 	return apply.Result{Operation: apply.SET}, nil
 }

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)
+		})
+	})
+})

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{}

pkg/kubectl/apply/strategy/strategic_visitor.go

@@ -16,14 +16,17 @@ limitations under the License.
 
 package strategy
 
-import "k8s.io/kubernetes/pkg/kubectl/apply"
+import (
+	"k8s.io/kubernetes/pkg/kubectl/apply"
+)
 
 // delegatingStrategy delegates merging fields to other visitor implementations
 // based on the merge strategy preferred by the field.
 type delegatingStrategy struct {
-	options Options
+	options    Options
-	merge   mergeStrategy
+	merge      mergeStrategy
-	replace replaceStrategy
+	replace    replaceStrategy
+	retainKeys retainKeysStrategy
 }
 
 // createDelegatingStrategy returns a new delegatingStrategy
@@ -33,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 "merge":
+	case apply.MergeStrategy:
 		return v.merge.MergeList(diff)
-	case "replace":
+	case apply.ReplaceStrategy:
 		return v.replace.MergeList(diff)
+	case apply.RetainKeysStrategy:
+		return v.retainKeys.MergeList(diff)
 	default:
 		return v.replace.MergeList(diff)
 	}
@@ -53,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 "merge":
+	case apply.MergeStrategy:
 		return v.merge.MergeMap(diff)
-	case "replace":
+	case apply.ReplaceStrategy:
 		return v.replace.MergeMap(diff)
+	case apply.RetainKeysStrategy:
+		return v.retainKeys.MergeMap(diff)
 	default:
 		return v.merge.MergeMap(diff)
 	}
@@ -67,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 "merge":
+	case apply.MergeStrategy:
 		return v.merge.MergeType(diff)
-	case "replace":
+	case apply.ReplaceStrategy:
 		return v.replace.MergeType(diff)
+	case apply.RetainKeysStrategy:
+		return v.retainKeys.MergeType(diff)
 	default:
 		return v.merge.MergeType(diff)
 	}

pkg/kubectl/apply/visitor.go

@@ -56,8 +56,13 @@ type Result struct {
 	MergedResult interface{}
 }
 
-// MapValuesElement exposes how to get the field / key - value pairs out of a Map or Type Element
+const (
-type MapValuesElement interface {
+	// MergeStrategy is the strategy to merge the local and remote values
-	Element
+	MergeStrategy = "merge"
-	GetValues() map[string]Element
+
-}
+	// RetainKeysStrategy is the strategy to merge the local and remote values, but drop any fields not defined locally
+	RetainKeysStrategy = "retainKeys"
+
+	// ReplaceStrategy is the strategy to replace the remote value with the local value
+	ReplaceStrategy = "replace"
+)