diff --git a/pkg/kubectl/apply/BUILD b/pkg/kubectl/apply/BUILD index 02443472b2e..419388c1d4a 100644 --- a/pkg/kubectl/apply/BUILD +++ b/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", diff --git a/pkg/kubectl/apply/element.go b/pkg/kubectl/apply/element.go index 60ac3ca16e2..8e7e14800f1 100644 --- a/pkg/kubectl/apply/element.go +++ b/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 +} diff --git a/pkg/kubectl/apply/error.go b/pkg/kubectl/apply/error.go new file mode 100644 index 00000000000..2e7a3d49976 --- /dev/null +++ b/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()) +} diff --git a/pkg/kubectl/apply/list_element.go b/pkg/kubectl/apply/list_element.go index 86be39ed1f4..b271d388214 100644 --- a/pkg/kubectl/apply/list_element.go +++ b/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{} diff --git a/pkg/kubectl/apply/map_element.go b/pkg/kubectl/apply/map_element.go index 9fcd4e86b88..c24ac40199e 100644 --- a/pkg/kubectl/apply/map_element.go +++ b/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{} diff --git a/pkg/kubectl/apply/primitive_element.go b/pkg/kubectl/apply/primitive_element.go index 847ee1a29ab..cc80c7b0326 100644 --- a/pkg/kubectl/apply/primitive_element.go +++ b/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{} diff --git a/pkg/kubectl/apply/strategy/BUILD b/pkg/kubectl/apply/strategy/BUILD index 2013ea7c273..d9652e35655 100644 --- a/pkg/kubectl/apply/strategy/BUILD +++ b/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", diff --git a/pkg/kubectl/apply/strategy/merge_conflict_test.go b/pkg/kubectl/apply/strategy/merge_conflict_test.go new file mode 100644 index 00000000000..73ca6ca720a --- /dev/null +++ b/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) + }) + }) +}) diff --git a/pkg/kubectl/apply/strategy/merge_visitor.go b/pkg/kubectl/apply/strategy/merge_visitor.go index 94a9d701f18..5a1b1a003ca 100644 --- a/pkg/kubectl/apply/strategy/merge_visitor.go +++ b/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{} diff --git a/pkg/kubectl/apply/strategy/replace_visitor.go b/pkg/kubectl/apply/strategy/replace_visitor.go index 5bf2f55d192..047d53ff1bb 100644 --- a/pkg/kubectl/apply/strategy/replace_visitor.go +++ b/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{} diff --git a/pkg/kubectl/apply/strategy/strategic_visitor.go b/pkg/kubectl/apply/strategy/strategic_visitor.go index eae1f9230ad..8c2d592b05b 100644 --- a/pkg/kubectl/apply/strategy/strategic_visitor.go +++ b/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{} diff --git a/pkg/kubectl/apply/strategy/test_swagger.json b/pkg/kubectl/apply/strategy/test_swagger.json index 2bdd1265723..57f69314d60 100644 --- a/pkg/kubectl/apply/strategy/test_swagger.json +++ b/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": [ { diff --git a/pkg/kubectl/apply/strategy/utils_test.go b/pkg/kubectl/apply/strategy/utils_test.go index 1ca687bac2b..9a958b78fd6 100644 --- a/pkg/kubectl/apply/strategy/utils_test.go +++ b/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)) + } +} diff --git a/pkg/kubectl/apply/type_element.go b/pkg/kubectl/apply/type_element.go index 0462123fb10..2528db87d1d 100644 --- a/pkg/kubectl/apply/type_element.go +++ b/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{}