Make UnstructuredContent return contents without mutating the source #62063

ash2k · 2018-04-03T12:22:54Z

What this PR does / why we need it:
This PR solves the issues described in #56316

Before this change:

A call to UnstructuredContent() potentially modified Object
The values returned by UnstructuredContent() could be manipulated to modify the value in Object. Going through the history it looks like this behavior was added before the addition of SetUnstructuredContent(). IMO it makes more sense now to use SetUnstructuredContent() or make changes to the exposed Object property
UnstructuredList did not implement the behavior described in the godoc. The godoc stated that the value returned should be mutable, but if u.Object == nil the map returned had no effect on Object

With this PR I'm proposing UnstructuredContent() returns the data without providing the contract of a mutable map. It also ensures all implementations of the Unstructured interface abide by the doc

Which issue(s) this PR fixes:
Fixes #56316

Special notes for your reviewer:
This PR continues work started in #57713.

Release note:

NONE

/kind bug
/sig api-machinery
/cc sttts deads2k

ash2k · 2018-04-03T12:33:35Z

/retest

sttts · 2018-04-03T12:47:56Z

staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured_list.go

-	out := u.Object
-	if out == nil {
-		out = make(map[string]interface{})
+	out := make(map[string]interface{}, len(u.Object)+1)


The contents may not have items.

Ok, so the case in line 62 should not happen?

u.Object may or may not contain items. We can remove if in the loop because the key will be overwritten anyway. It made sense to have it before (in the previous PR) because a deep copy was made here and skipping items reduced the unnecessary work. WDYT?

I've removed the if.

sttts · 2018-04-03T12:49:25Z

Could we make Unstructured/UnstructuredList.Object private?

ash2k · 2018-04-03T23:23:36Z

Could we make Unstructured/UnstructuredList.Object private?

It is quite convenient to be able to construct one of these as a literal, especially as part of a bigger literal. For example https://github.com/atlassian/smith/blob/ba1ff1e867c74ee29f1172257b5b555134ca2e77/pkg/controller/spec_processor_test.go#L332-L338

However I can also see value in hiding it and making everyone access the contents via the methods. Maybe we can have a constructor NewUnstructured(map[string]interface{}) to replace literals?
BTW, the constructor and the setter can do type checks if, say, an environment variable is set to catch misuse. Ok, that would, I think, add enough value to do it. WDYT?

sttts · 2018-04-04T07:39:30Z

A constructor sounds good.

ash2k · 2018-04-04T10:52:34Z

@sttts can we merge this one first and then I'll submit another one with the constructor change? It is going to be a big PR and I think it makes sense to keep the changes separate. Also this is a bugfix and a refactoring - should be separate.

sttts · 2018-04-04T12:00:14Z

pkg/printers/internalversion/describe_test.go

@@ -2133,14 +2133,14 @@ URL:	http://localhost
 				"name":              "MyName",
 				"namespace":         "MyNamespace",
 				"creationTimestamp": "2017-04-01T00:00:00Z",
-				"resourceVersion":   123,
+				"resourceVersion":   int64(123),


this is unrelated. Can you move it to another PR? (I cannot approve this).

Most likely it was causing a failure because 123 literal has an int type by default but deep copy only likes float64 and int64. But this fix is no longer needed because we are not making a deep copy.

sttts · 2018-04-04T12:00:50Z

staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured_test.go

+	var u Unstructured
+	content := u.UnstructuredContent()
+	expContent := make(map[string]interface{})
+	assert.EqualValues(t, expContent, content)


Can you check that the actual object stays the same? Make a deep copy before and then compare afterwards.

sttts · 2018-04-04T12:00:59Z

staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured_test.go

@@ -0,0 +1,30 @@
+/*
+Copyright 2017 The Kubernetes Authors.


sttts · 2018-04-04T12:01:20Z

can we merge this one first and then I'll submit another one with the constructor change?

yes, makes sense.

sttts · 2018-04-04T12:35:28Z

staging/src/k8s.io/apimachinery/pkg/runtime/converter.go

 	switch x := x.(type) {
 	case map[string]interface{}:
+		if x == nil {


does this really exist?

Yes, without this change (and below for completeness) the newly added check in the test (deep copy and compare) was failing. https://play.golang.org/p/KASoa2DeTnp

This will change the output - this code used to produce non-nil maps and slices for typed nils but now will produce typed nils. JSON encoding will change - typed nils as null in JSON (https://play.golang.org/p/Uz2ajLEQx9A).

I think this change is a good one - it makes the deep copy more precise.

We should never see typed nils somewhere deep inside IMO. On the toplevel maybe.

Compare https://play.golang.org/p/R45uKKqDsm6

Yes, we should not see them, but they exist. I'm fine with removing this fix from this PR if that's what you think is best. However I think that way deep copy is more correct. It just mirrors the input precisely. If there are no typed nils, we don't introduce them here. If they create a problem somewhere then the place that "spawns" them should be fixed. Or, even better, a place that does not handle them well could be made more robust.

Also, I'm not sure what exactly you wanted me to compare? typed and untyped nils are both marshaled into null, yes :)

Have we ever seen this "cannot deep copy" panic? You say that they show up. Do you know where?

Panic? Current code does not cause any panics AFAIK. Are you talking about failing test without these changes? It does not panic, it just fails because objects are not equal. They are not equal because the deep copied Object field ("expected" in the test) is an empty map and the original is `nil.

func TestNilUnstructuredContent(t *testing.T) { var u Unstructured uCopy := u.DeepCopy() content := u.UnstructuredContent() expContent := make(map[string]interface{}) assert.EqualValues(t, expContent, content) assert.Equal(t, uCopy, &u) }

--- FAIL: TestNilUnstructuredContent (0.00s) Error Trace: unstructured_test.go:31 Error: Not equal: expected: &unstructured.Unstructured{Object:map[string]interface {}{}} actual: &unstructured.Unstructured{Object:map[string]interface {}(nil)} Diff: --- Expected +++ Actual @@ -1,4 +1,3 @@ (*unstructured.Unstructured)({ - Object: (map[string]interface {}) { - } + Object: (map[string]interface {}) <nil> })

ash2k · 2018-04-05T06:09:56Z

/retest

sttts · 2018-04-05T10:27:14Z

staging/src/k8s.io/apimachinery/pkg/runtime/converter.go

 		clone := make([]interface{}, len(x))
 		for i, v := range x {
 			clone[i] = DeepCopyJSONValue(v)
 		}
 		return clone
-	case string, int64, bool, float64, nil, encodingjson.Number:
+	case string, int64, bool, float64, encodingjson.Number:


why not keep the nil and remove the if clause above?

Good question. My IntelliJ is complaining that nil is not a valid type but Go compiler is happy 🤷‍♂️ It actually works fine https://play.golang.org/p/PBIyhFgWn8H
I'll revert this change.

Don't feel strong about it. Was just curious.

ash2k · 2018-04-05T12:06:52Z

/retest

sttts · 2018-04-05T12:17:20Z

the verify job is broken: #62162

/lgtm
/approve

k8s-ci-robot · 2018-04-05T12:17:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ash2k, sttts

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~staging/src/k8s.io/apimachinery/pkg/OWNERS~~ [sttts]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

fejta-bot · 2018-04-05T14:51:52Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

fejta-bot · 2018-04-05T17:39:53Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

fejta-bot · 2018-04-05T20:27:52Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

k8s-github-robot · 2018-04-05T23:09:55Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2018-04-06T00:03:56Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

wenjiaswe · 2018-04-12T20:08:08Z

cc @mbohlool

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. release-note-none Denotes a PR that doesn't merit a release note. labels Apr 3, 2018

k8s-ci-robot requested review from deads2k and sttts April 3, 2018 12:22

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Apr 3, 2018

ash2k mentioned this pull request Apr 3, 2018

Ensure UnstructuredContent returns a deep copy without mutating contents #57713

Closed

sttts reviewed Apr 3, 2018

View reviewed changes

Make UnstructuredContent return contents without mutating the source

e82e8b4

Remove check for items

d5fdac3

ash2k force-pushed the no-mutation-unstructuredcontent branch from 5d14bf4 to d5fdac3 Compare April 4, 2018 10:55

sttts reviewed Apr 4, 2018

View reviewed changes

Support typed nils; test empty Unstructured is not mutated

53e8fd0

sttts reviewed Apr 4, 2018

View reviewed changes

sttts reviewed Apr 5, 2018

View reviewed changes

Put nil back into switch

1fcd199

k8s-ci-robot assigned sttts Apr 5, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 5, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 5, 2018

k8s-github-robot merged commit 2dee210 into kubernetes:master Apr 6, 2018

ash2k deleted the no-mutation-unstructuredcontent branch April 6, 2018 01:29

ash2k mentioned this pull request Apr 12, 2018

Constructor for Unstructured[List] #62500

Closed

Make UnstructuredContent return contents without mutating the source #62063

Make UnstructuredContent return contents without mutating the source #62063

Conversation

ash2k commented Apr 3, 2018

ash2k commented Apr 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts commented Apr 3, 2018

ash2k commented Apr 3, 2018

sttts commented Apr 4, 2018

ash2k commented Apr 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts commented Apr 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ash2k commented Apr 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ash2k commented Apr 5, 2018

sttts commented Apr 5, 2018

k8s-ci-robot commented Apr 5, 2018

fejta-bot commented Apr 5, 2018

fejta-bot commented Apr 5, 2018

fejta-bot commented Apr 5, 2018

k8s-github-robot commented Apr 5, 2018

k8s-github-robot commented Apr 6, 2018

wenjiaswe commented Apr 12, 2018