Useful helper functions for Unstructured #51940

ash2k · 2017-09-05T08:39:33Z

Which issue this PR fixes:
Fixes #40790

Release note:

NONE

/kind feature
/sig api-machinery
/area client-libraries
/assign @sttts @liggitt

k8s-ci-robot · 2017-09-05T08:39:40Z

Hi @ash2k. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

sttts · 2017-09-05T09:23:43Z

/ok-to-test

sttts · 2017-09-05T09:26:17Z

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

-	nested := getNestedField(obj, fields...)
+// NestedInt64Pointer returns the *int64 value of a nested field.
+// Returns nil if value is not found or is not an int64/*int64.
+func NestedInt64Pointer(obj map[string]interface{}, fields ...string) *int64 {


why a pointer and not the value?

The other funcs return values or clones, never the original object or a reference to it.

sttts · 2017-09-05T09:28:02Z

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

+// NestedString returns the string value of a nested field.
+// Returns empty string if value is not found or is not a string.
+func NestedString(obj map[string]interface{}, fields ...string) string {
+	if str, ok := NestedField(obj, fields...).(string); ok {


I would prefer to see errors in the result if the type is wrong.

Same for the other type-casts below.

sttts · 2017-09-05T09:28:33Z

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

+// NestedStringSlice returns the []string value of a nested field.
+// Returns nil if value is not found or is not a []interface{}.
+func NestedStringSlice(obj map[string]interface{}, fields ...string) []string {
+	if m, ok := NestedField(obj, fields...).([]interface{}); ok {
 		strSlice := make([]string, 0, len(m))
 		for _, v := range m {
 			if str, ok := v.(string); ok {


I would prefer to return an error if the cast fails.

sttts · 2017-09-05T09:28:51Z

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

+// NestedStringMap returns the map[string]string value of a nested field.
+// Returns nil if value is not found or is not a map[string]interface{}.
+func NestedStringMap(obj map[string]interface{}, fields ...string) map[string]string {
+	if m, ok := NestedField(obj, fields...).(map[string]interface{}); ok {
 		strMap := make(map[string]string, len(m))
 		for k, v := range m {
 			if str, ok := v.(string); ok {


same here, I prefer an error in this case

sttts · 2017-09-05T09:31:09Z

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

-			m = m[field].(map[string]interface{})
+	for _, field := range fields[:len(fields)-1] {
+		if _, ok := m[field].(map[string]interface{}); !ok {
+			m[field] = make(map[string]interface{})


this is unexpected that everything of wrong type is overridden.

Also here we need errors.

Well, I agree with you. This is how it is implemented right now, I just made it public. I'm happy to change it.
Do you want all of these functions to return an error? Maybe bool for getters?

bool is also fine, maybe even better.

lavalamp · 2017-09-07T20:44:04Z

/assign @krousey

ash2k · 2017-10-31T11:16:36Z

@sttts Would be great to get this merged into 1.9, PTAL.

sttts · 2017-11-01T09:23:53Z

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

 		return
 	}
 	ts, _ := timestamp.MarshalQueryParameter()
 	u.setNestedField(ts, "metadata", "deletionTimestamp")
 }

 func (u *Unstructured) GetDeletionGracePeriodSeconds() *int64 {
-	return getNestedInt64Pointer(u.Object, "metadata", "deletionGracePeriodSeconds")
+	val, ok := NestedInt64(u.Object, "metadata", "deletionGracePeriodSeconds")


related #54803

sttts · 2017-11-01T09:28:53Z

Can you squash commit 2, 3 and 5?

sttts · 2017-11-01T09:31:25Z

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

-func setNestedField(obj map[string]interface{}, value interface{}, fields ...string) {
+// SetNestedField sets the value of a nested field.
+// Returns false if value cannot be set because one of the nesting levels is not a map[string]interface{}.
+func SetNestedField(obj map[string]interface{}, value interface{}, fields ...string) bool {


I wished we had a debugging/strict-test mode where value is type checked. We use(d) to write arbitrary values into objects (#54803) and the compiler cannot help us with type checks.

I can send a follow up PR with a environment variable flag a-la

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"))

Would be great to run unit and maybe even integration tests with this.

Follow up in #55297

sttts · 2017-11-01T09:36:15Z

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

+	delete(m, fields[len(fields)-1])
+}
+
+func getNestedString(obj map[string]interface{}, fields ...string) string {


nit: nestedStringOrDefault with default string arg?

getNestedString() is used in lots of places and default is an empty string everywhere. That is why I kept it as it is. Also I didn't want to touch the name because it would've increased the PR size even further. I can send a follow-up PR with rename if needed.

Makes sense.

sttts · 2017-11-01T09:37:23Z

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

-func setNestedSlice(obj map[string]interface{}, value []string, fields ...string) {
+// SetNestedStringSlice sets the string slice value of a nested field.
+// Returns false if value cannot be set because one of the nesting levels is not a map[string]interface{}.
+func SetNestedStringSlice(obj map[string]interface{}, value []string, fields ...string) bool {


For the setters the bool return value feels unnatural.

Does an explicit error here look better? Something like that in #55168 (comment)?

sttts · 2017-11-01T09:42:22Z

Having triaged issues in the old code recently, this PR is big step forward in cleaning up this area. Left a few comments, but I think we are not far from getting this in.

@liggitt @wojtek-t are you fine with the benchmarks? Any further concerns?

No functional changes

ash2k · 2017-11-01T10:06:06Z

Rebased+squashed. I left first commit unchanged - it is just moving code around, no changes. Second one with all the changes.

sttts · 2017-11-01T10:15:09Z

Waiting for @liggitt @wojtek-t's ok performance-wise. Will do a final review pass then.

ash2k · 2017-11-01T11:40:55Z

/retest

deads2k · 2017-11-01T15:31:39Z

@liggitt @wojtek-t are you fine with the benchmarks? Any further concerns?

.08% change? I don't think they can really be against it.

sttts · 2017-11-01T15:34:07Z

@deads2k feel free to go through the code. More eyes help in this highly untyped and fragile area.

deads2k · 2017-11-01T19:44:54Z

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

+	}
+
+	gvk := obj.GetObjectKind().GroupVersionKind()
+	if len(gvk.Kind) == 0 {


Straight move? I'd expect unstructured objects to always have a gvk.Version too.

Yes, I haven't touched this code. See 7aeaedd#diff-31522f2d01b79a28cf7d5adaa03d5b26L685

deads2k · 2017-11-01T19:50:58Z

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

@@ -478,31 +272,47 @@ func (u *Unstructured) GetDeletionTimestamp() *metav1.Time {

 func (u *Unstructured) SetDeletionTimestamp(timestamp *metav1.Time) {
 	if timestamp == nil {
-		u.setNestedField(nil, "metadata", "deletionTimestamp")
+		RemoveNestedField(u.Object, "metadata", "deletionTimestamp")


I'd expect a similar remove on SetCreationTimestamp

Creation timestamp is not optional, at least the argument of the Go func is not. So at worst, we miss a feature here on the Go level.

deads2k · 2017-11-01T19:52:48Z

@deads2k feel free to go through the code. More eyes help in this highly untyped and fragile area.

It lgtm, but this area of the code is full of weird gotchas. @sttts if you like it in the morning, I think it's fine.

/approve

sttts · 2017-11-02T06:45:18Z

/lgtm

k8s-github-robot · 2017-11-02T06:45:26Z

[APPROVALNOTIFIER] This PR is APPROVED

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

Associated issue: 40790

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~~ [deads2k,sttts]

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

k8s-github-robot · 2017-11-02T07:43:15Z

Automatic merge from submit-queue (batch tested with PRs 54787, 51940). If you want to cherry-pick this change to another branch, please follow the instructions here.

smarterclayton · 2017-11-20T00:52:27Z

These shouldn't be in this package - I'm moving them into runtime, because meta/v1/unstructured is only for "typed" unstructured (i.e., anything that obeys our metadata rules), not generic to unstructured.

ash2k · 2017-11-20T01:53:52Z

That makes sense. Thanks for an update.

k8s-ci-robot assigned liggitt and sttts Sep 5, 2017

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

k8s-github-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Sep 5, 2017

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 Sep 5, 2017

sttts reviewed Sep 5, 2017

View reviewed changes

ash2k mentioned this pull request Sep 6, 2017

Do deep copy instead of to and from JSON encoding #51526

Merged

mbohlool assigned mbohlool and unassigned sttts and liggitt Sep 7, 2017

k8s-ci-robot assigned krousey Sep 7, 2017

mbohlool assigned sttts and liggitt and unassigned krousey Sep 7, 2017

ash2k changed the title ~~Useful helper functions for Unstructured~~ [WIP] Useful helper functions for Unstructured Sep 18, 2017

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 18, 2017

ash2k force-pushed the unstructured-helpers branch from cbb565a to 36c5315 Compare September 25, 2017 12:01

k8s-ci-robot removed the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 25, 2017

sttts reviewed Nov 1, 2017

View reviewed changes

ash2k added 2 commits November 1, 2017 20:51

Split unstructured.go into several parts

7aeaedd

No functional changes

Useful helper functions for Unstructured

7c10cbc

ash2k force-pushed the unstructured-helpers branch from 55604c9 to 7c10cbc Compare November 1, 2017 10:04

ash2k mentioned this pull request Nov 1, 2017

apimachinery: fix Unstructured deepcopy panic #54803

Closed

juanvallejo mentioned this pull request Nov 1, 2017

output empty creationTimestamp as null #53464

Merged

deads2k reviewed Nov 1, 2017

View reviewed changes

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 1, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 2, 2017

k8s-github-robot merged commit c3f3137 into kubernetes:master Nov 2, 2017

ash2k deleted the unstructured-helpers branch November 2, 2017 09:56

ash2k mentioned this pull request Nov 8, 2017

Runtime type checks of Unstructured content #55297

Closed

Useful helper functions for Unstructured #51940

Useful helper functions for Unstructured #51940

Conversation

ash2k commented Sep 5, 2017

k8s-ci-robot commented Sep 5, 2017

sttts commented Sep 5, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lavalamp commented Sep 7, 2017

ash2k commented Oct 31, 2017

Choose a reason for hiding this comment

sttts commented Nov 1, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts commented Nov 1, 2017

ash2k commented Nov 1, 2017

sttts commented Nov 1, 2017

ash2k commented Nov 1, 2017

deads2k commented Nov 1, 2017

sttts commented Nov 1, 2017

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

deads2k commented Nov 1, 2017

sttts commented Nov 2, 2017

k8s-github-robot commented Nov 2, 2017

k8s-github-robot commented Nov 2, 2017

smarterclayton commented Nov 20, 2017

ash2k commented Nov 20, 2017