have basic kubectl crud agnostic of registered types #36085

deads2k · 2016-11-02T19:25:57Z

Makes kubectl get agnostic to scheme (baked in API types). This means that it will now work against generic API servers that are "kube shaped".

This is similar to the work done for kubectl create last release. I'll split out the smaller command. kubectl get looks a lot different, but this eliminates all special casing for TPR in those cases.

@fabianofranz

This change is

deads2k · 2016-11-02T19:27:08Z

pkg/api/meta/help.go

@@ -39,6 +39,14 @@ func GetItemsPtr(list runtime.Object) (interface{}, error) {
 	if err != nil {
 		return nil, err
 	}
+
+	// if we're a runtime.Unstructured, check to see if we have an `items` key
+	if unstructured, ok := list.(*runtime.Unstructured); ok {


@smarterclayton api/meta changes to more cleanly handle the way the unstructured decoder works. It doesn't make an UnstructuredList by default, which is annoying.

This needs a test.

deads2k · 2016-11-02T19:27:46Z

pkg/api/meta/help.go

@@ -117,6 +125,13 @@ func SetList(list runtime.Object, objects []runtime.Object) error {
 	slice := reflect.MakeSlice(items.Type(), len(objects), len(objects))
 	for i := range objects {
 		dest := slice.Index(i)
+
+		// check to see if you're directly assignable
+		if reflect.TypeOf(objects[i]).AssignableTo(dest.Type()) {


When you pass an UnstructuredList into this method, you don't want to dereference the items like you do in EnforcePtr below.

This needs a test.

easier: if dest.Kind() == reflect.Ptr

easier: if dest.Kind() == reflect.Ptr

I prefer this express and I think this check is slightly different.

deads2k · 2016-11-02T19:28:11Z

pkg/kubectl/cmd/get.go

-				return err
+			// take the filtered items and create a new list for display
+			list := &runtime.UnstructuredList{
+				Object: map[string]interface{}{


This looks nasty, but it serializes correctly and v1.List does not serialize correctly. :(

Doesn't this lose resourceVersion?

Doesn't this lose resourceVersion?

Will fix.

Actually, turns out HEAD doesn't preserve the resourceversion.

or selflink

deads2k · 2016-11-02T19:28:52Z

pkg/kubectl/kubectl.go

@@ -48,28 +48,6 @@ func makeImageList(spec *api.PodSpec) string {
 	return strings.Join(listOfImages(spec), ",")
 }

-func NewThirdPartyResourceMapper(gvs []unversioned.GroupVersion, gvks []unversioned.GroupVersionKind) (meta.RESTMapper, error) {


@sttts @smarterclayton so happy!

deads2k · 2016-11-02T19:29:38Z

pkg/kubectl/resource_printer.go

+
+	// check if the object is unstructured.  If so, let's attempt to convert it to a type we can understand before
+	// trying to print, since the printers are keyed by type.  This is extremely expensive.
+	if objBytes, err := runtime.Encode(api.Codecs.LegacyCodec(), obj); err == nil {


@smarterclayton will you let me get away with this to have the capability? It only hurts kubectl.

can you not check whether this is unstructured first? Or is it now always unstructured coming in to here? Not sure why you have to blanket encode?

can you not check whether this is unstructured first? Or is it now always unstructured coming in to here? Not sure why you have to blanket encode?

I think its almost always unstructured, but I'm not certain enough to assume it.

I will gate on unstructured and unknown to start.

deads2k · 2016-11-02T19:30:12Z

pkg/kubectl/sorting_printer.go

+	case reflect.Interface:
+		switch itype := i.Interface().(type) {
+		case uint8:
+			if jtype, ok := j.Interface().(uint8); ok {


Check precedence carefully through here.

k8s-ci-robot · 2016-11-02T19:49:42Z

Jenkins GCE Node e2e failed for commit 90d9fa4. Full PR test history.

The magic incantation to run this job again is @k8s-bot node e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

deads2k · 2016-11-03T17:25:38Z

This gets pretty deep into scheme/codec stuff. Moving to @smarterclayton.

This is ready for review.

deads2k · 2016-11-03T17:27:09Z

pkg/kubectl/cmd/get_test.go

-// 2. if the specified output-version is a recognized, valid Scheme, then the list should use that scheme, and otherwise it will default to the client version, registered.GroupOrDie(api.GroupName).GroupVersion.String() in this test;
-// 3a. if the specified output-version is a recognized, valid Scheme, in which the requested object (replicationcontroller) can be represented, then the object should be returned using that version;
-// 3b. otherwise if the specified output-version is unrecognized, but the requested object (replicationcontroller) is recognized by the client's codec, then it will be converted to the client version, registered.GroupOrDie(api.GroupName).GroupVersion.String() in this test.
-func TestGetUnknownSchemaObjectListGeneric(t *testing.T) {


We no longer do anything with --output-version. I'd like to leave disconnecting/doc-ing that to a followup, but this this makes us generic for basic CRUD.

deads2k · 2016-11-03T17:27:48Z

pkg/kubectl/cmd/get_test.go

+		t.Fatal(actual)
+	}
+	for i, obj := range actual {
+		actualObj, err := runtime.Decode(


encodes the unstructured (or whatever) and then decodes that back into internal for the comparison which this package is using.

smarterclayton · 2016-11-03T17:46:51Z

pkg/kubectl/resource_printer.go

@@ -2447,6 +2533,20 @@ func (j *JSONPathPrinter) PrintObj(obj runtime.Object, w io.Writer) error {
 		}
 	}

+	if unknown, ok := obj.(*runtime.Unknown); ok {


twitch

Found inside a list somewhere I think. Given the way this code works, the change here is actually correct. Well, more correct than it was before. The whole thing needs some rethinking.

smarterclayton · 2016-11-03T17:48:18Z

pkg/kubectl/resource_printer.go

@@ -2256,9 +2265,78 @@ func (h *HumanReadablePrinter) PrintObj(obj runtime.Object, output io.Writer) er
 		}
 		return resultValue.Interface().(error)
 	}
+
+	// we don't recognize this type, but we can still attempt to print some reasonable information about.
+	unstructured, ok := obj.(*runtime.Unstructured)


Keep in mind unstructured really is "generic API v1" when used this way (with accessors). So this code only works until someone changes metadata and then is horribly broken again. Which is not inconsistent with what we've said up till now, that we won't change v1 metadata until we have server side transformation (we can't, effectively).

k8s-ci-robot · 2016-11-03T18:01:34Z

Jenkins GCE etcd3 e2e failed for commit 29d2a1d. Full PR test history.

The magic incantation to run this job again is @k8s-bot gce etcd3 e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-11-03T18:04:40Z

Jenkins GCE e2e failed for commit 29d2a1d. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-11-03T18:19:49Z

Jenkins GCI GCE e2e failed for commit 29d2a1d. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

deads2k · 2016-11-03T19:17:42Z

comments addressed.

sttts · 2016-11-04T14:09:50Z

Much better now without the patch to GetFieldsPtr.

k8s-github-robot · 2016-11-04T14:40:11Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2016-11-04T15:17:01Z

Automatic merge from submit-queue

smarterclayton · 2016-11-04T21:04:40Z

Fixing the failures?

deads2k · 2016-11-07T13:52:23Z

Fixing the failures?

which failures on what?

mengqiy · 2016-11-10T02:30:11Z

@deads2k I'm working on isssue #36007, which is forked from #35791.
In #35791, people have discussed that there is a problem related how we register TPR. But it seems that piece of code has been removed by this PR.
Maybe I miss something. In current codebase, it seems that only UnstructuredObject() register TPR, while Object() don't do that.

Why we only do that for UnstructuredObject()?

deads2k · 2016-11-10T12:38:34Z

@deads2k I'm working on isssue #36007, which is forked from #35791.
In #35791, people have discussed that there is a problem related how we register TPR. But it seems that piece of code has been removed by this PR.
Maybe I miss something. In current codebase, it seems that only UnstructuredObject() register TPR, while Object() don't do that.

Why we only do that for UnstructuredObject()?

As I recall, there's something wrong with the implementation of --validate that doesn't properly handle missing swagger so someone built a side-signal way to skip it instead of handling the case cleanly. It's been a while since I looked though.

pwittrock · 2016-12-02T22:06:48Z

@deads2k

It looks like this changed the kubectl -o jsonpath behavior to now fail if it cannot find the field. We should make sure the release notes are clear about this as it could cause scripts that were depending on the old behavior to fail (e.g. if the script checks if the field exists by querying for it and looks for an empty value)

smarterclayton · 2016-12-02T22:16:33Z

Hrm - that may have been something even older that I did in 1.4.

pwittrock · 2016-12-02T22:20:46Z

@smarterclayton I suspect it is this PR. 1.4 has the "return empty" behavior, and the new behavior actually breaks version skew tests. This PR updated a kubectl.go e2e test to work around the change in behavior, but it is failing when running the old tests against a new client/server.

We could potentially revert to the old behavior by using the work you implemented in #31714

pwittrock · 2016-12-02T22:29:17Z

@bgrant0607 FYI

I am not sure if we consider this a api compatibility breaking change. The json path doc does explicitly state what the behavior is when the field is not present.

1.4 behavior:

If the field is present in the struct, but missing from the JSON, return empty string
If the field is missing from the struct, exit 1 with an error

1.5 behavior:

If the field is missing from the JSON, exit 1 with an error

I have not been able to find a record of the original intention when the field is missing from the JSON. It would probably be best to support either behavior by exposing a flag to control the knobs introduced in #31714. I am not sure it is worth the risk introduced to get that into 1.5 though.

bgrant0607 · 2016-12-02T22:42:03Z

@pwittrock @smarterclayton Is there a reason why we wouldn't make the 1.4 behavior the default?

pwittrock · 2016-12-02T22:52:08Z

Making the 1.4 behavior the default makes the most sense to me. My biggest concern would be around getting the change cherrypicked and tested in time for the 1.5 release. Today I will look into if this can be done as a simple change.

pwittrock · 2016-12-02T22:52:36Z

Created issue #37991 for tracking

pwittrock · 2016-12-02T22:56:48Z

@smarterclayton Looking more closely at the PR, you are right that it doesn't seem like this should have caused the change in behavior we are seeing. Still investigating the root cause.

smarterclayton · 2016-12-03T01:35:26Z

According to JSONPath the quasi spec, no expression that has no field should return errors. We insert that extra logic ourselves. On Dec 2, 2016, at 5:56 PM, Phillip Wittrock <notifications@github.com> wrote: @smarterclayton <https://github.com/smarterclayton> Looking more closely at the PR, you are right that it doesn't seem like this should have caused the change in behavior we are seeing. Still investigating the root cause. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#36085 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p9WOApkxGKw3UVW25xteSrgeZYfJks5rEKI4gaJpZM4KnplK> .

googlebot added the cla: yes label Nov 2, 2016

deads2k commented Nov 2, 2016

View reviewed changes

k8s-github-robot assigned janetkuo Nov 2, 2016

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Nov 2, 2016

deads2k added this to the v1.5 milestone Nov 3, 2016

deads2k force-pushed the cli-05-generic-print branch from 90d9fa4 to 29d2a1d Compare November 3, 2016 17:24

deads2k changed the title ~~[WIP] have basic kubectl crud agnostic of registered types~~ have basic kubectl crud agnostic of registered types Nov 3, 2016

deads2k assigned smarterclayton and unassigned janetkuo Nov 3, 2016

deads2k commented Nov 3, 2016

View reviewed changes

k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 3, 2016

smarterclayton reviewed Nov 3, 2016

View reviewed changes

deads2k force-pushed the cli-05-generic-print branch from 29d2a1d to e667fb3 Compare November 3, 2016 19:17

deads2k added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Nov 3, 2016

deads2k force-pushed the cli-05-generic-print branch from a6d8fe9 to 61673c4 Compare November 4, 2016 13:05

deads2k added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 4, 2016

This was referenced Nov 4, 2016

make kubectl replace generic #36061

Closed

make kubectl delete generic #36158

Closed

ncdc added cla: human-approved and removed cla: human-approved labels Nov 4, 2016

deads2k added the cla: human-approved label Nov 4, 2016

k8s-github-robot merged commit 5a2c473 into kubernetes:master Nov 4, 2016

mfojtik mentioned this pull request Nov 7, 2016

Fix AsVersionedObject to deal with external schema #36066

Closed

pwittrock mentioned this pull request Dec 2, 2016

Changes to Kubectl behavior Release Notes #34746

Closed

chentao1596 mentioned this pull request Dec 5, 2016

WIP:kubelet: support multi-headers when getting pod from HTTP source #38089

Closed

ncdc mentioned this pull request Dec 20, 2016

kubectl get broken for list #38907

Closed

deads2k deleted the cli-05-generic-print branch February 1, 2017 17:28

romdim mentioned this pull request Jul 18, 2017

Getting error group map[rbac.authorization...] while reading state succeeded songsterr/drone-kubernetes-job#1

Closed

have basic kubectl crud agnostic of registered types #36085

have basic kubectl crud agnostic of registered types #36085

Conversation

deads2k commented Nov 2, 2016 • edited

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 Nov 4, 2016 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 2, 2016

deads2k commented Nov 3, 2016

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

k8s-ci-robot commented Nov 3, 2016

k8s-ci-robot commented Nov 3, 2016

k8s-ci-robot commented Nov 3, 2016

deads2k commented Nov 3, 2016

sttts commented Nov 4, 2016

k8s-github-robot commented Nov 4, 2016

k8s-github-robot commented Nov 4, 2016

smarterclayton commented Nov 4, 2016

deads2k commented Nov 7, 2016

mengqiy commented Nov 10, 2016

deads2k commented Nov 10, 2016

pwittrock commented Dec 2, 2016

smarterclayton commented Dec 2, 2016

pwittrock commented Dec 2, 2016

pwittrock commented Dec 2, 2016

bgrant0607 commented Dec 2, 2016

pwittrock commented Dec 2, 2016

pwittrock commented Dec 2, 2016

pwittrock commented Dec 2, 2016

smarterclayton commented Dec 3, 2016 via email

deads2k commented Nov 2, 2016 •

edited

sttts Nov 4, 2016 •

edited