Commit 14362160 authored by Kubernetes Submit Queue's avatar Kubernetes Submit Queue Committed by GitHub

Merge pull request #38665 from ymqytw/fix_list_of_primitives

Automatic merge from submit-queue (batch tested with PRs 39834, 38665) Use parallel list for deleting items from a primitive list with merge strategy Implemented parallel list for deleting items from a primitive list with merge strategy. Ref: [design doc](https://github.com/kubernetes/community/blob/master/contributors/devel/api-conventions.md#list-of-primitives) fixes #35163 and #32398 When using parallel list, we don't need to worry about version skew. When an old APIServer gets a new patch like: ```yaml metadata: $deleteFromPrimitiveList/finalizers: - b finalizers: - c ``` It won't fail and work as before, because the parallel list will be dropped during json decoding. Remaining issue: There is no check when creating a set (primitive list with merge strategy). Duplicates may get in. It happens in two cases: 1) Creation using POST 2) Creating a list that doesn't exist before using PATCH Fixing the first case is the beyond the scope of this PR. The second case can be fixed in this PR if we need that. cc: @pwittrock @kubernetes/kubectl @kubernetes/sig-api-machinery ```release-note Fix issue around merging lists of primitives when using PATCH or kubectl apply. ```
parents effeb20a 9a8ef008
...@@ -46,13 +46,35 @@ type StrategicMergePatchTestCase struct { ...@@ -46,13 +46,35 @@ type StrategicMergePatchTestCase struct {
StrategicMergePatchTestCaseData StrategicMergePatchTestCaseData
} }
type StrategicMergePatchRawTestCase struct {
Description string
StrategicMergePatchRawTestCaseData
}
type StrategicMergePatchTestCaseData struct { type StrategicMergePatchTestCaseData struct {
// Original is the original object (last-applied config in annotation)
Original map[string]interface{} Original map[string]interface{}
TwoWay map[string]interface{} // Modified is the modified object (new config we want)
Modified map[string]interface{} Modified map[string]interface{}
Current map[string]interface{} // Current is the current object (live config in the server)
Current map[string]interface{}
// TwoWay is the expected two-way merge patch diff between original and modified
TwoWay map[string]interface{}
// ThreeWay is the expected three-way merge patch
ThreeWay map[string]interface{} ThreeWay map[string]interface{}
Result map[string]interface{} // Result is the expected object after applying the three-way patch on current object.
Result map[string]interface{}
}
// The meaning of each field is the same as StrategicMergePatchTestCaseData's.
// The difference is that all the fields in StrategicMergePatchRawTestCaseData are json-encoded data.
type StrategicMergePatchRawTestCaseData struct {
Original []byte
Modified []byte
Current []byte
TwoWay []byte
ThreeWay []byte
Result []byte
} }
type MergeItem struct { type MergeItem struct {
...@@ -1797,6 +1819,125 @@ testCases: ...@@ -1797,6 +1819,125 @@ testCases:
other: b other: b
`) `)
var strategicMergePatchRawTestCases = []StrategicMergePatchRawTestCase{
{
Description: "delete items in lists of scalars",
StrategicMergePatchRawTestCaseData: StrategicMergePatchRawTestCaseData{
Original: []byte(`
mergingIntList:
- 1
- 2
- 3
`),
TwoWay: []byte(`
$deleteFromPrimitiveList/mergingIntList:
- 3
`),
Modified: []byte(`
mergingIntList:
- 1
- 2
`),
Current: []byte(`
mergingIntList:
- 1
- 2
- 3
- 4
`),
ThreeWay: []byte(`
$deleteFromPrimitiveList/mergingIntList:
- 3
`),
Result: []byte(`
mergingIntList:
- 1
- 2
- 4
`),
},
},
{
Description: "delete all duplicate items in lists of scalars",
StrategicMergePatchRawTestCaseData: StrategicMergePatchRawTestCaseData{
Original: []byte(`
mergingIntList:
- 1
- 2
- 3
- 3
`),
TwoWay: []byte(`
$deleteFromPrimitiveList/mergingIntList:
- 3
`),
Modified: []byte(`
mergingIntList:
- 1
- 2
`),
Current: []byte(`
mergingIntList:
- 1
- 2
- 3
- 3
- 4
`),
ThreeWay: []byte(`
$deleteFromPrimitiveList/mergingIntList:
- 3
`),
Result: []byte(`
mergingIntList:
- 1
- 2
- 4
`),
},
},
{
Description: "add and delete items in lists of scalars",
StrategicMergePatchRawTestCaseData: StrategicMergePatchRawTestCaseData{
Original: []byte(`
mergingIntList:
- 1
- 2
- 3
`),
TwoWay: []byte(`
$deleteFromPrimitiveList/mergingIntList:
- 3
mergingIntList:
- 4
`),
Modified: []byte(`
mergingIntList:
- 1
- 2
- 4
`),
Current: []byte(`
mergingIntList:
- 1
- 2
- 3
- 4
`),
ThreeWay: []byte(`
$deleteFromPrimitiveList/mergingIntList:
- 3
`),
Result: []byte(`
mergingIntList:
- 1
- 2
- 4
`),
},
},
}
func TestStrategicMergePatch(t *testing.T) { func TestStrategicMergePatch(t *testing.T) {
testStrategicMergePatchWithCustomArguments(t, "bad original", testStrategicMergePatchWithCustomArguments(t, "bad original",
"<THIS IS NOT JSON>", "{}", mergeItem, errBadJSONDoc) "<THIS IS NOT JSON>", "{}", mergeItem, errBadJSONDoc)
...@@ -1818,6 +1959,11 @@ func TestStrategicMergePatch(t *testing.T) { ...@@ -1818,6 +1959,11 @@ func TestStrategicMergePatch(t *testing.T) {
testTwoWayPatch(t, c) testTwoWayPatch(t, c)
testThreeWayPatch(t, c) testThreeWayPatch(t, c)
} }
for _, c := range strategicMergePatchRawTestCases {
testTwoWayPatchForRawTestCase(t, c)
testThreeWayPatchForRawTestCase(t, c)
}
} }
func testStrategicMergePatchWithCustomArguments(t *testing.T, description, original, patch string, dataStruct interface{}, err error) { func testStrategicMergePatchWithCustomArguments(t *testing.T, description, original, patch string, dataStruct interface{}, err error) {
...@@ -1849,12 +1995,32 @@ func testTwoWayPatch(t *testing.T, c StrategicMergePatchTestCase) { ...@@ -1849,12 +1995,32 @@ func testTwoWayPatch(t *testing.T, c StrategicMergePatchTestCase) {
testPatchApplication(t, original, actual, modified, c.Description) testPatchApplication(t, original, actual, modified, c.Description)
} }
func testTwoWayPatchForRawTestCase(t *testing.T, c StrategicMergePatchRawTestCase) {
original, expected, modified := twoWayRawTestCaseToJSONOrFail(t, c)
actual, err := CreateTwoWayMergePatch(original, modified, mergeItem)
if err != nil {
t.Errorf("error: %s\nin test case: %s\ncannot create two way patch:\noriginal:%s\ntwoWay:%s\nmodified:%s\ncurrent:%s\nthreeWay:%s\nresult:%s\n",
err, c.Description, c.Original, c.TwoWay, c.Modified, c.Current, c.ThreeWay, c.Result)
return
}
testPatchCreation(t, expected, actual, c.Description)
testPatchApplication(t, original, actual, modified, c.Description)
}
func twoWayTestCaseToJSONOrFail(t *testing.T, c StrategicMergePatchTestCase) ([]byte, []byte, []byte) { func twoWayTestCaseToJSONOrFail(t *testing.T, c StrategicMergePatchTestCase) ([]byte, []byte, []byte) {
return testObjectToJSONOrFail(t, c.Original, c.Description), return testObjectToJSONOrFail(t, c.Original, c.Description),
testObjectToJSONOrFail(t, c.TwoWay, c.Description), testObjectToJSONOrFail(t, c.TwoWay, c.Description),
testObjectToJSONOrFail(t, c.Modified, c.Description) testObjectToJSONOrFail(t, c.Modified, c.Description)
} }
func twoWayRawTestCaseToJSONOrFail(t *testing.T, c StrategicMergePatchRawTestCase) ([]byte, []byte, []byte) {
return yamlToJSONOrError(t, c.Original),
yamlToJSONOrError(t, c.TwoWay),
yamlToJSONOrError(t, c.Modified)
}
func testThreeWayPatch(t *testing.T, c StrategicMergePatchTestCase) { func testThreeWayPatch(t *testing.T, c StrategicMergePatchTestCase) {
original, modified, current, expected, result := threeWayTestCaseToJSONOrFail(t, c) original, modified, current, expected, result := threeWayTestCaseToJSONOrFail(t, c)
actual, err := CreateThreeWayMergePatch(original, modified, current, mergeItem, false) actual, err := CreateThreeWayMergePatch(original, modified, current, mergeItem, false)
...@@ -1896,6 +2062,47 @@ func testThreeWayPatch(t *testing.T, c StrategicMergePatchTestCase) { ...@@ -1896,6 +2062,47 @@ func testThreeWayPatch(t *testing.T, c StrategicMergePatchTestCase) {
testPatchApplication(t, current, actual, result, c.Description) testPatchApplication(t, current, actual, result, c.Description)
} }
func testThreeWayPatchForRawTestCase(t *testing.T, c StrategicMergePatchRawTestCase) {
original, modified, current, expected, result := threeWayRawTestCaseToJSONOrFail(t, c)
actual, err := CreateThreeWayMergePatch(original, modified, current, mergeItem, false)
if err != nil {
if !IsConflict(err) {
t.Errorf("error: %s\nin test case: %s\ncannot create three way patch:\noriginal:%s\ntwoWay:%s\nmodified:%s\ncurrent:%s\nthreeWay:%s\nresult:%s\n",
err, c.Description, c.Original, c.TwoWay, c.Modified, c.Current, c.ThreeWay, c.Result)
return
}
if !strings.Contains(c.Description, "conflict") {
t.Errorf("unexpected conflict: %s\nin test case: %s\ncannot create three way patch:\noriginal:%s\ntwoWay:%s\nmodified:%s\ncurrent:%s\nthreeWay:%s\nresult:%s\n",
err, c.Description, c.Original, c.TwoWay, c.Modified, c.Current, c.ThreeWay, c.Result)
return
}
if len(c.Result) > 0 {
actual, err := CreateThreeWayMergePatch(original, modified, current, mergeItem, true)
if err != nil {
t.Errorf("error: %s\nin test case: %s\ncannot force three way patch application:\noriginal:%s\ntwoWay:%s\nmodified:%s\ncurrent:%s\nthreeWay:%s\nresult:%s\n",
err, c.Description, c.Original, c.TwoWay, c.Modified, c.Current, c.ThreeWay, c.Result)
return
}
testPatchCreation(t, expected, actual, c.Description)
testPatchApplication(t, current, actual, result, c.Description)
}
return
}
if strings.Contains(c.Description, "conflict") || len(c.Result) < 1 {
t.Errorf("error: %s\nin test case: %s\nexpected conflict did not occur:\noriginal:%s\ntwoWay:%s\nmodified:%s\ncurrent:%s\nthreeWay:%s\nresult:%s\n",
err, c.Description, c.Original, c.TwoWay, c.Modified, c.Current, c.ThreeWay, c.Result)
return
}
testPatchCreation(t, expected, actual, c.Description)
testPatchApplication(t, current, actual, result, c.Description)
}
func threeWayTestCaseToJSONOrFail(t *testing.T, c StrategicMergePatchTestCase) ([]byte, []byte, []byte, []byte, []byte) { func threeWayTestCaseToJSONOrFail(t *testing.T, c StrategicMergePatchTestCase) ([]byte, []byte, []byte, []byte, []byte) {
return testObjectToJSONOrFail(t, c.Original, c.Description), return testObjectToJSONOrFail(t, c.Original, c.Description),
testObjectToJSONOrFail(t, c.Modified, c.Description), testObjectToJSONOrFail(t, c.Modified, c.Description),
...@@ -1904,6 +2111,14 @@ func threeWayTestCaseToJSONOrFail(t *testing.T, c StrategicMergePatchTestCase) ( ...@@ -1904,6 +2111,14 @@ func threeWayTestCaseToJSONOrFail(t *testing.T, c StrategicMergePatchTestCase) (
testObjectToJSONOrFail(t, c.Result, c.Description) testObjectToJSONOrFail(t, c.Result, c.Description)
} }
func threeWayRawTestCaseToJSONOrFail(t *testing.T, c StrategicMergePatchRawTestCase) ([]byte, []byte, []byte, []byte, []byte) {
return yamlToJSONOrError(t, c.Original),
yamlToJSONOrError(t, c.Modified),
yamlToJSONOrError(t, c.Current),
yamlToJSONOrError(t, c.ThreeWay),
yamlToJSONOrError(t, c.Result)
}
func testPatchCreation(t *testing.T, expected, actual []byte, description string) { func testPatchCreation(t *testing.T, expected, actual []byte, description string) {
sorted, err := sortMergeListsByName(actual, mergeItem) sorted, err := sortMergeListsByName(actual, mergeItem)
if err != nil { if err != nil {
...@@ -1989,6 +2204,24 @@ func jsonToYAML(j []byte) ([]byte, error) { ...@@ -1989,6 +2204,24 @@ func jsonToYAML(j []byte) ([]byte, error) {
return y, nil return y, nil
} }
func yamlToJSON(y []byte) ([]byte, error) {
j, err := yaml.YAMLToJSON(y)
if err != nil {
return nil, fmt.Errorf("yaml to json failed: %v\n%v\n", err, y)
}
return j, nil
}
func yamlToJSONOrError(t *testing.T, y []byte) []byte {
j, err := yamlToJSON(y)
if err != nil {
t.Errorf("%v", err)
}
return j
}
func TestHasConflicts(t *testing.T) { func TestHasConflicts(t *testing.T) {
testCases := []struct { testCases := []struct {
A interface{} A interface{}
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment