upstream/kubernetes
1
0
Fork 0
mirror of https://github.com/kubernetes/kubernetes.git synced 2026-03-10 10:30:55 -04:00
Code Issues Projects Releases Packages Wiki Activity Actions
kubernetes/test
History
Kubernetes Submit Queue 2cbb07a439
Merge pull request #55871 from atlassian/unstructured-converter-no-mutation 
Automatic merge from submit-queue. 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>.

Fix potential unexpected object mutation that can lead to data races

**What this PR does / why we need it**:
In #51526 I introduced an optimization - do a deep copy instead of to and from JSON roundtrip to convert anything that implements `runtime.Unstructured`. I just discovered that the method that is used there `UnstructuredContent()` in both `Unstructured` and `UnstructuredList` may mutate the original object.
2008750398/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured.go (L87-L92)
7c10cbc642/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured_list.go (L58-L75)
This is problematic because previously (before #51526) there was no mutation and because this is unexpected and may lead to data races - it is bad behaviour to mutate original object when you just want a copy of it.
This PR fixes the issue.

Without the fix the tests I've added are failing because when comparison is done original object is not the same:
```
converter_test.go:154: Object changed, diff: 
object.Object[items]:
  a: []interface {}{}
  b: <nil>
converter_test.go:154: Object changed, diff: 
object.Object[items]:
  a: []interface {}{map[string]interface {}{"kind":"Pod"}}
  b: <nil>
```

However the underlying issue is not fixed here - `UnstructuredContent()` is brittle and dangerous. Method name does not imply that it mutates data when you call it. And godoc does not mention that either:
509df603b1/staging/src/k8s.io/apimachinery/pkg/runtime/interfaces.go (L233-L249)
Something needs to be done about it IMO.
Also `UnstructuredContent()` implementation in `UnstructuredList` does not implement the behaviour required by godoc in `runtime.Unstructured`.

**Release note**:
```release-note
NONE
```
/kind bug
/sig api-machinery
/assign @sttts
2017-11-20 08:58:37 -08:00
..
conformance Update BUILD file to include e2e_node tests 2017-11-17 17:28:29 +08:00
e2e Merge pull request #55871 from atlassian/unstructured-converter-no-mutation 2017-11-20 08:58:37 -08:00
e2e_node Merge pull request #55898 from dashpole/fix_flaky_allocatable 2017-11-18 13:13:24 -08:00
fixtures add test for convert 2017-11-01 01:14:24 -07:00
images generated bazel 2017-11-17 21:02:47 -08:00
integration Merge pull request #55392 from sttts/sttts-remove-policy-v1alpha1 2017-11-17 00:18:17 -08:00
kubemark Bump addon manager version used to 6.5 2017-11-15 11:34:46 +01:00
list update BUILD files 2017-10-15 18:18:13 -07:00
soak Update generated files 2017-11-09 12:14:08 +01:00
utils bump base images to debian stretch 2017-11-10 09:54:10 -06:00
BUILD Add conformance test regression test. 2017-10-27 15:31:20 -07:00
OWNERS Add shyamjvs to test/OWNERS 2017-11-08 15:44:56 +01:00
test_owners.csv remove tpr from test_owners.csv 2017-11-03 21:17:10 +05:30
test_owners.json Remove all traces of federation 2017-10-26 13:37:37 -07:00