Add conflict detection feature to apply strategy

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

@@ -419,3 +419,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

+/*
+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

+/*
+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{}