Runtime type checks of Unstructured content #55297

ash2k · 2017-11-08T06:11:56Z

What this PR does / why we need it:
Adds a flag that can be enabled via UNSTRUCTURED_INVALID_TYPE_DETECTOR environment variable to do strict type checks in Unstructured code. It can be used in unit and integration tests to catch bugs. This is similar to existing flags:

kubernetes/staging/src/k8s.io/apimachinery/pkg/conversion/unstructured/converter.go

Line 81 in 51e653d

    
           DefaultConverter       = NewConverter(parseBool(os.Getenv("KUBE_PATCH_CONVERSION_DETECTOR")))

and

kubernetes/staging/src/k8s.io/client-go/tools/cache/mutation_detector.go

Line 34 in ed42305

    
           mutationDetectionEnabled, _ = strconv.ParseBool(os.Getenv("KUBE_CACHE_MUTATION_DETECTOR"))

Special notes for your reviewer:
This is a follow up for #51940. I don't know how to enable this in unit, integration tests (also when run via Bazel). Any ideas?

Release note:

NONE

/kind feature
/sig api-machinery
/assign @sttts

k8s-github-robot · 2017-11-08T06:12:06Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ash2k
We suggest the following additional approver: sttts

Assign the PR to them by writing /assign @sttts in a comment when ready.

Associated issue: 51940

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

Needs approval from an approver in each of these OWNERS Files:

staging/src/k8s.io/apimachinery/pkg/OWNERS

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

sttts · 2017-11-08T10:56:03Z

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

@@ -99,6 +122,9 @@ func NestedInt64(obj map[string]interface{}, fields ...string) (int64, bool) {
 		return 0, false
 	}
 	i, ok := val.(int64)
+	if !ok && invalidTypeDetectionEnabled {


My understanding of the bool in the return values is to allow this situation. It's a bit strange to panic.

sttts · 2017-11-08T10:57:43Z

Why not check the types on all the setters and every code path which creates an Unstructured. Then we get caller which is responsible for wrong types. If everything "incoming" is type-correct, we don't have to check in the getters.

sttts · 2017-11-08T11:00:06Z

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

@@ -183,6 +231,9 @@ func setNestedFieldNoCopy(obj map[string]interface{}, value interface{}, fields
 			if valMap, ok := val.(map[string]interface{}); ok {
 				m = valMap
 			} else {
+				if invalidTypeDetectionEnabled {


do we do deep type checking? We should, go recursively go into the map[string]interface{}, compare the deepcopy func.

mbohlool · 2017-11-09T21:07:09Z

/cc @lavalamp

lavalamp · 2017-11-09T21:10:53Z

I am terrified by having test code that gets compiled into production binaries. It's also not obvious to me that things that trigger this would definitely be errors.

sttts · 2017-11-10T09:11:30Z

It's also not obvious to me that things that trigger this would definitely be errors.

We have a strict implicit typing of the content in Unstructured. The algorithms like deepcopy and serialization depend on that and fall over or worse, return inconsistent results. These are errors. If we don't find them in tests, we will hit them in production. We can choose: add additional type-checking in unit or integration tests (not in e2e, so e2e will be exactly like production) or live with the uncertainity that we never checked the types.

sttts · 2017-11-13T17:21:55Z

/ok-to-test

ash2k · 2017-11-15T11:26:32Z

Why not check the types on all the setters and every code path which creates an Unstructured. Then we get caller which is responsible for wrong types. If everything "incoming" is type-correct, we don't have to check in the getters.

@sttts You are right, this should be enough and we already do that - for any slice/map/interface{} data we do a deep copy which checks types recursively. This code was more to catch issues introduced by putting invalid types directly into Object field of an Unstructured. If we don't care about that this PR is not needed. I'm ok to close it, no problem.
I have checked (briefly) existing k/k code that uses the Object field and only found one issue in a test.

kubernetes/test/e2e/apimachinery/garbage_collector.go

Line 922 in 4c8539c

"ownerReferences": []map[string]string{

This should be

"ownerReferences": []interface{}{
	map[string]interface{}{
		// ...
	},
},

However I'd like to keep some cleanups from this PR - can submit another one if we decide this one is not needed.

sttts · 2017-11-15T12:12:53Z

for any slice/map/interface{} data we do a deep copy which checks types recursively.

but we don't panic on type-error, but should probably.

Yes, please keep the cleanups!

ash2k · 2017-11-15T20:55:44Z

for any slice/map/interface{} data we do a deep copy which checks types recursively.

but we don't panic on type-error, but should probably.

What do you mean? We do panic in json deep copy:

kubernetes/staging/src/k8s.io/apimachinery/pkg/conversion/unstructured/converter.go

Line 451 in 7c10cbc

panic(fmt.Errorf("cannot deep copy %T", x))

sttts · 2017-11-16T09:46:11Z

What do you mean? We do panic in json deep copy:

right, so every code path with SetNestedField at the end is fine. We have SetNestedSlice and SetNestedMap which do not deepcopy. But if my Gogland is right, they are not used. Can we remove them?

ash2k · 2017-11-16T10:01:35Z

Both of them just delegate to SetNestedField(). So they do deep copy.

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

Lines 226 to 228 in 7c10cbc

    
           func SetNestedMap(obj map[string]interface{}, value map[string]interface{}, fields ...string) bool { 
        
           	return SetNestedField(obj, value, fields...) 
        
           }

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

Lines 210 to 212 in 7c10cbc

    
           func SetNestedSlice(obj map[string]interface{}, value []interface{}, fields ...string) bool { 
        
           	return SetNestedField(obj, value, fields...) 
        
           }

sttts · 2017-11-16T10:37:13Z

Both of them just delegate to SetNestedField(). So they do deep copy.

Right, should take some more time reviewing this or change my glasses :) Then we are on the safe side. Good!

@sttts

Automatic merge from submit-queue (batch tested with PRs 55615, 56010, 55990). 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>. Unstructured cleanups **What this PR does / why we need it**: Cleanups for `Unstructured`/`UnstructuredList` extracted from #55297. **Release note**: ```release-note NONE ``` /sig api-machinery /kind enhancement /assign @sttts

k8s-github-robot · 2017-11-21T00:23:56Z

@ash2k PR needs rebase

ash2k · 2017-11-27T09:04:56Z

Most of the checks have been incorporated into #55168. I'm closing this PR, at least for now. Please reopen if this functionality is needed.

k8s-ci-robot assigned sttts Nov 8, 2017

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 8, 2017

ash2k mentioned this pull request Nov 8, 2017

Useful helper functions for Unstructured #51940

Merged

sttts reviewed Nov 8, 2017

View reviewed changes

k8s-ci-robot requested a review from lavalamp November 9, 2017 21:07

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 13, 2017

ash2k force-pushed the unstructured-check-types branch from ecd026f to 5936771 Compare November 18, 2017 22:02

Runtime type checks of Unstructured content

f0ac746

ash2k force-pushed the unstructured-check-types branch from 5936771 to f0ac746 Compare November 19, 2017 08:06

ash2k mentioned this pull request Nov 19, 2017

Unstructured cleanups #56010

Merged

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 21, 2017

ash2k mentioned this pull request Nov 22, 2017

apiextensions: add subresources for custom resources #55168

Merged

ash2k closed this Nov 27, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Runtime type checks of Unstructured content #55297

Runtime type checks of Unstructured content #55297

ash2k commented Nov 8, 2017

k8s-github-robot commented Nov 8, 2017

sttts Nov 8, 2017

sttts commented Nov 8, 2017

sttts Nov 8, 2017

mbohlool commented Nov 9, 2017

lavalamp commented Nov 9, 2017

sttts commented Nov 10, 2017

sttts commented Nov 13, 2017

ash2k commented Nov 15, 2017

sttts commented Nov 15, 2017

ash2k commented Nov 15, 2017

sttts commented Nov 16, 2017

ash2k commented Nov 16, 2017

sttts commented Nov 16, 2017

k8s-github-robot commented Nov 21, 2017

ash2k commented Nov 27, 2017

Runtime type checks of Unstructured content #55297

Runtime type checks of Unstructured content #55297

Conversation

ash2k commented Nov 8, 2017

k8s-github-robot commented Nov 8, 2017

sttts Nov 8, 2017

Choose a reason for hiding this comment

sttts commented Nov 8, 2017

sttts Nov 8, 2017

Choose a reason for hiding this comment

mbohlool commented Nov 9, 2017

lavalamp commented Nov 9, 2017

sttts commented Nov 10, 2017

sttts commented Nov 13, 2017

ash2k commented Nov 15, 2017

sttts commented Nov 15, 2017

ash2k commented Nov 15, 2017

sttts commented Nov 16, 2017

ash2k commented Nov 16, 2017

sttts commented Nov 16, 2017

k8s-github-robot commented Nov 21, 2017

ash2k commented Nov 27, 2017