Merge pull request #60947 from fanzhangio/replace

Automatic merge from submit-queue (batch tested with PRs 60990, 60947, 45275, 60565, 61091). 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>.

Add conflict detection feature to apply strategy

- Add DetectConflictor interface on Element level. Implemented it for particular elements.
- If Options.FailOnConflict is enabled, Merge will detect conflict by invoking doConflictDecect for particular element,
  returning ConflictError with details.
- Add tests, including use case examples and illustration. For example: list, map, and complicated combination.

**What this PR does / why we need it**:
Apply is being rewritten under pkg/kubectl/apply/strategy based on visitor pattern. The new merge and replace code should check for conflicts between the recorded value and the remote value, and optionally return an error if they do not match with the field and details. A conflict is if the same field is specified in BOTH the recorded and the remote values of an object, but does not match.

**Which issue(s) this PR fixes**:
Fixes #60945 
https://github.com/kubernetes/kubectl/issues/97

**Release note**:

```release-note
NONE
```

pkg/kubectl/apply/BUILD

@@ -6,6 +6,7 @@ go_library(
         "doc.go",
         "element.go",
         "empty_element.go",
+        "error.go",
         "list_element.go",
         "map_element.go",
         "primitive_element.go",

pkg/kubectl/apply/element.go

@@ -416,3 +416,8 @@ func (e HasElementData) HasLocal() bool {
 func (e HasElementData) HasRemote() bool {
 	return e.remoteSet
 }
+
+// ConflictDetector defines the capability to detect conflict. An element can examine remote/recorded value to detect conflict.
+type ConflictDetector interface {
+	HasConflict() error
+}

pkg/kubectl/apply/error.go

@@ -0,0 +1,37 @@
+/*
+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 apply
+
+import "fmt"
+
+// ConflictError represents a conflict error occurred during the merge operation.
+type ConflictError struct {
+	element Element
+}
+
+// NewConflictError returns a ConflictError with detailed conflict information in element
+func NewConflictError(e PrimitiveElement) *ConflictError {
+	return &ConflictError{
+		element: e,
+	}
+}
+
+// Error implements error
+func (c *ConflictError) Error() string {
+	return fmt.Sprintf("conflict detected, recorded value (%+v) and remote value (%+v)",
+		c.element.GetRecorded(), c.element.GetRemote())
+}

pkg/kubectl/apply/list_element.go

@@ -65,3 +65,17 @@ func sliceCast(i interface{}) []interface{} {
 	}
 	return i.([]interface{})
 }
+
+// HasConflict returns ConflictError if fields in recorded and remote of ListElement conflict
+func (e ListElement) HasConflict() error {
+	for _, item := range e.Values {
+		if item, ok := item.(ConflictDetector); ok {
+			if err := item.HasConflict(); err != nil {
+				return err
+			}
+		}
+	}
+	return nil
+}
+
+var _ ConflictDetector = &ListElement{}

pkg/kubectl/apply/map_element.go

@@ -70,3 +70,17 @@ func mapCast(i interface{}) map[string]interface{} {
 	}
 	return i.(map[string]interface{})
 }
+
+// HasConflict returns ConflictError if some elements in map conflict.
+func (e MapElement) HasConflict() error {
+	for _, item := range e.GetValues() {
+		if item, ok := item.(ConflictDetector); ok {
+			if err := item.HasConflict(); err != nil {
+				return err
+			}
+		}
+	}
+	return nil
+}
+
+var _ ConflictDetector = &MapElement{}

pkg/kubectl/apply/primitive_element.go

@@ -16,6 +16,8 @@ limitations under the License.
 
 package apply
 
+import "reflect"
+
 // PrimitiveElement contains the recorded, local and remote values for a field
 // of type primitive
 type PrimitiveElement struct {
@@ -32,3 +34,21 @@ func (e PrimitiveElement) Merge(v Strategy) (Result, error) {
 }
 
 var _ Element = &PrimitiveElement{}
+
+// HasConflict returns ConflictError if primitive element has conflict field.
+// Conflicts happen when either of the following conditions:
+// 1. A field is specified in both recorded and remote values, but does not match.
+// 2. A field is specified in recorded values, but missing in remote values.
+func (e PrimitiveElement) HasConflict() error {
+	if e.HasRecorded() && e.HasRemote() {
+		if !reflect.DeepEqual(e.GetRecorded(), e.GetRemote()) {
+			return NewConflictError(e)
+		}
+	}
+	if e.HasRecorded() && !e.HasRemote() {
+		return NewConflictError(e)
+	}
+	return nil
+}
+
+var _ ConflictDetector = &PrimitiveElement{}

pkg/kubectl/apply/strategy/BUILD

@@ -18,6 +18,7 @@ go_library(
 go_test(
     name = "go_default_xtest",
     srcs = [
+        "merge_conflict_test.go",
         "merge_map_list_test.go",
         "merge_map_test.go",
         "merge_primitive_list_test.go",

pkg/kubectl/apply/strategy/merge_conflict_test.go

@@ -0,0 +1,206 @@
+/*
+Copyright 2018 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("Comparing fields of remote and recorded ", func() {
+	Context("Test conflict in map fields of remote and recorded", func() {
+		It("If conflicts found, expected return error", func() {
+			recorded := create(`
+apiVersion: apps/v1beta1
+kind: Deployment
+spec:
+  foo1: "key1"
+`)
+			local := create(`
+apiVersion: apps/v1beta1
+kind: Deployment
+spec:
+  foo2: "baz2-1"
+`)
+			remote := create(`
+apiVersion: apps/v1beta1
+kind: Deployment
+spec:
+  foo1: "baz1-0"
+`)
+
+			expect := hasConflict
+			// map fields have conflict : recorded {foo1 : "key1"}, remote {foo1 : "baz1-0"}
+			runConflictTest(strategy.Create(strategy.Options{FailOnConflict: true}), recorded, local, remote, expect)
+		})
+	})
+
+	Context("Test conflict in list fields of remote and recorded ", func() {
+		It("If conflicts found, expected return false", func() {
+			recorded := create(`
+apiVersion: apps/v1beta1
+kind: Deployment
+metadata:
+  finalizers:
+  - "a"
+  - "b"
+  - "d"
+`)
+			local := create(`
+apiVersion: apps/v1beta1
+kind: Deployment
+metadata:
+  finalizers:
+  - "a"
+  - "b"
+`)
+			remote := create(`
+apiVersion: apps/v1beta1
+kind: Deployment
+metadata:
+  finalizers:
+  - "a"
+  - "b"
+  - "c"
+`)
+			expect := hasConflict
+			// primatie lists have conflicts: recorded [a, b, d], remote [a, b, c]
+			runConflictTest(strategy.Create(strategy.Options{FailOnConflict: true}), recorded, local, remote, expect)
+		})
+	})
+
+	Context("Test conflict in Map-List fields of remote and recorded ", func() {
+		It("should leave the item", func() {
+			recorded := create(`
+apiVersion: apps/v1beta1
+kind: Deployment
+spec:
+  template:
+    spec:
+      containers:
+      - name: item1
+        image: image1
+`)
+			local := create(`
+apiVersion: apps/v1beta1
+kind: Deployment
+spec:
+  template:
+    spec:
+      containers:
+      - name: item2
+        image: image2
+`)
+			remote := create(`
+apiVersion: apps/v1beta1
+kind: Deployment
+spec:
+  template:
+    spec:
+      containers:
+        - name: item1
+          image: image3
+`)
+			expect := hasConflict
+			// map list has conflict :  recorded {containers: [ {name: item1, image: image1} ]} , remote {containers: [ {name: item1, image: image3} ]}
+			runConflictTest(strategy.Create(strategy.Options{FailOnConflict: true}), recorded, local, remote, expect)
+		})
+	})
+
+	Context("Test conflicts in nested map field", func() {
+		It("If conflicts found, expected return error", func() {
+			recorded := create(`
+apiVersion: apps/v1beta1
+kind: Deployment
+spec:
+  foo1:
+    name: "key1"
+`)
+			local := create(`
+apiVersion: apps/v1beta1
+kind: Deployment
+spec:
+  foo1:
+    name: "baz1-0"
+`)
+			remote := create(`
+apiVersion: apps/v1beta1
+kind: Deployment
+spec:
+  foo1:
+    name: "baz1-1"
+`)
+			expect := hasConflict
+			// nested map has conflict : recorded {foo1: {name: "key1"}}, remote {foo1: {name : "baz1-1"}}
+			runConflictTest(strategy.Create(strategy.Options{FailOnConflict: true}), recorded, local, remote, expect)
+		})
+	})
+
+	Context("Test conflicts in complicated map, list", func() {
+		It("Should catch conflict in key-value in map element", func() {
+			recorded := create(`
+apiVersion: apps/v1beta1
+kind: Deployment
+spec:
+  template:
+    spec:
+      containers:
+      - name: container
+        ports:
+        - containerPort: 8080
+          protocol: TCP
+          hostPort: 2020
+        - containerPort: 8080
+          protocol: UDP
+          hostPort: 2022
+`)
+			local := create(`
+apiVersion: apps/v1beta1
+kind: Deployment
+spec:
+  template:
+    spec:
+      containers:
+      - name: container
+        ports:
+        - containerPort: 8080
+          protocol: TCP
+          hostPort: 2020
+`)
+			remote := create(`
+apiVersion: apps/v1beta1
+kind: Deployment
+spec:
+  template:
+    spec:
+      containers:
+      - name: container
+        ports:
+        - containerPort: 8080
+          protocol: TCP
+          hostPort: 2020
+        - containerPort: 8080
+          protocol: UDP
+          hostPort: 2022
+          hostIP: "127.0.0.1"
+`)
+			expect := noConflict
+			runConflictTest(strategy.Create(strategy.Options{FailOnConflict: true}), recorded, local, remote, expect)
+		})
+	})
+})

pkg/kubectl/apply/strategy/merge_visitor.go

@@ -41,7 +41,10 @@ func (v mergeStrategy) MergeList(e apply.ListElement) (apply.Result, error) {
 	if result, done := v.doAddOrDelete(e); done {
 		return result, nil
 	}
-
+	// Detect conflict in ListElement
+	if err := v.doConflictDetect(e); err != nil {
+		return apply.Result{}, err
+	}
 	// Merge each item in the list and append it to the list
 	merged := []interface{}{}
 	for _, value := range e.Values {
@@ -76,7 +79,10 @@ func (v mergeStrategy) MergeMap(e apply.MapElement) (apply.Result, error) {
 	if result, done := v.doAddOrDelete(e); done {
 		return result, nil
 	}
-
+	// Detect conflict in MapElement
+	if err := v.doConflictDetect(e); err != nil {
+		return apply.Result{}, err
+	}
 	return v.doMergeMap(e.GetValues())
 }
 
@@ -86,7 +92,10 @@ func (v mergeStrategy) MergeType(e apply.TypeElement) (apply.Result, error) {
 	if result, done := v.doAddOrDelete(e); done {
 		return result, nil
 	}
-
+	// Detect conflict in TypeElement
+	if err := v.doConflictDetect(e); err != nil {
+		return apply.Result{}, err
+	}
 	return v.doMergeMap(e.GetValues())
 }
 
@@ -145,4 +154,9 @@ func (v mergeStrategy) MergeEmpty(diff apply.EmptyElement) (apply.Result, error)
 	return apply.Result{Operation: apply.SET}, nil
 }
 
+// doConflictDetect returns error if element has conflict
+func (v mergeStrategy) doConflictDetect(e apply.Element) error {
+	return v.strategic.doConflictDetect(e)
+}
+
 var _ apply.Strategy = &mergeStrategy{}

pkg/kubectl/apply/strategy/replace_visitor.go

@@ -65,11 +65,13 @@ func (v replaceStrategy) MergeEmpty(e apply.EmptyElement) (apply.Result, error)
 // replace returns the local value if specified, otherwise it returns the remote value
 // this works regardless of the approach
 func (v replaceStrategy) doReplace(e apply.Element) (apply.Result, error) {
-	// TODO: Check for conflicts
+
 	if result, done := v.doAddOrDelete(e); done {
 		return result, nil
 	}
-
+	if err := v.doConflictDetect(e); err != nil {
+		return apply.Result{}, err
+	}
 	if e.HasLocal() {
 		// Specified locally, set the local value
 		return apply.Result{Operation: apply.SET, MergedResult: e.GetLocal()}, nil
@@ -97,4 +99,9 @@ func (v replaceStrategy) doAddOrDelete(e apply.Element) (apply.Result, bool) {
 	return apply.Result{}, false
 }
 
+// doConflictDetect returns error if element has conflict
+func (v replaceStrategy) doConflictDetect(e apply.Element) error {
+	return v.strategic.doConflictDetect(e)
+}
+
 var _ apply.Strategy = &replaceStrategy{}

pkg/kubectl/apply/strategy/strategic_visitor.go

@@ -96,4 +96,14 @@ func (v delegatingStrategy) MergeEmpty(diff apply.EmptyElement) (apply.Result, e
 	return v.merge.MergeEmpty(diff)
 }
 
+// doConflictDetect detects conflicts in element when option enabled, return error if conflict happened.
+func (v delegatingStrategy) doConflictDetect(e apply.Element) error {
+	if v.options.FailOnConflict {
+		if e, ok := e.(apply.ConflictDetector); ok {
+			return e.HasConflict()
+		}
+	}
+	return nil
+}
+
 var _ apply.Strategy = &delegatingStrategy{}

pkg/kubectl/apply/strategy/test_swagger.json

@@ -81,7 +81,7 @@
         "spec": {
           "description": "Specification of the desired behavior of the Deployment.",
           "$ref": "#/definitions/io.k8s.api.apps.v1beta1.DeploymentSpec"
-        },
+        }
       },
       "x-kubernetes-group-version-kind": [
         {

pkg/kubectl/apply/strategy/utils_test.go

@@ -32,6 +32,11 @@ import (
 	tst "k8s.io/kubernetes/pkg/kubectl/cmd/util/openapi/testing"
 )
 
+const (
+	hasConflict = true
+	noConflict  = false
+)
+
 var fakeResources = tst.NewFakeResources(filepath.Join("..", "..", "..", "..", "api", "openapi-spec", "swagger.json"))
 
 // run parses the openapi and runs the tests
@@ -64,3 +69,17 @@ func create(config string) map[string]interface{} {
 
 	return result
 }
+
+func runConflictTest(instance apply.Strategy, recorded, local, remote map[string]interface{}, isConflict bool) {
+	parseFactory := parse.Factory{Resources: fakeResources}
+	parsed, err := parseFactory.CreateElement(recorded, local, remote)
+	Expect(err).Should(Not(HaveOccurred()))
+
+	merged, err := parsed.Merge(instance)
+	if isConflict {
+		Expect(err).Should(HaveOccurred())
+	} else {
+		Expect(err).ShouldNot(HaveOccurred())
+		Expect(merged.Operation).Should(Equal(apply.SET))
+	}
+}

pkg/kubectl/apply/type_element.go

@@ -40,4 +40,17 @@ func (e TypeElement) GetValues() map[string]Element {
 	return e.Values
 }
 
+// HasConflict returns ConflictError if some elements in type conflict.
+func (e TypeElement) HasConflict() error {
+	for _, item := range e.GetValues() {
+		if item, ok := item.(ConflictDetector); ok {
+			if err := item.HasConflict(); err != nil {
+				return err
+			}
+		}
+	}
+	return nil
+}
+
 var _ Element = &TypeElement{}
+var _ ConflictDetector = &TypeElement{}