Expose versioned query parameters and make watch an operation on List #5763

smarterclayton · 2015-03-22T00:45:55Z

Sub-resources are needed to properly secure proxying to the node (as per agreement in authz discussions), because we need to tie "resource" and "verb" to an action. The /proxy endpoint is too open to give to anyone but cluster admins. As part of #3481 and the general "bastioning" of the API server, we want to introduce new sub resources that allow specific, typed access to a backend. Examples:

Proxy to a pod's port: /pods/foo/proxy/5000/some/resource
Proxy to a service's port: /services/foo/proxy/5000/some/resource/path
Expose logs for a pod: /pods/foo/logs?container=test -> returns "text/plain" stream
Expose logs for a resource backed by a pod /jobs/foo/logs ->
Parameterize pod logs with query options: /pods/foo/logs?container=test&since=30s
Handle remote execution: /pods/foo/exec?container=test&cmd=/bin/bash&arg=-c&arg=echo+$foo -> upgrades connection to SPDY/HTTP2
Handle portforwarding: /pods/foo/portforward/5000?container=test
Allow binaries to be posted to an endpoint (with a given type) and converted by RESTStorage or apiserver into the underlying object - essentially hand off the request body to storage.

This requires three pieces of function:

Allow query parameters to be properly versioned across a set of distinct resources and verbs
Return an input stream from a RESTStorage Get or Post call
Upgrade an HTTP connection to a higher protocol (exec) or hijack it (port forward) or use it to proxy other requests (proxy)

Ideally we would also ensure we have methods that allow proper introspection through swagger

Handle content type negotiation when returning an input stream
Annotate request parameters with metadata for query params

This pull request will contain the changes to enable that functionality through a set of targeted refactors. @csrwng @lavalamp @bgrant0607 @liggitt

googlebot · 2015-03-22T00:45:56Z

Thanks for your pull request.

It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA) at https://cla.developers.google.com/.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check the information on your CLA or see this help article on setting the email on your git commits.

Once you've done that, please reply here to let us know. If you signed the CLA as a corporation, please let us know the company's name.

smarterclayton · 2015-03-23T03:33:36Z

A bunch of these commits are in other pulls, only the last few are specific.

bgrant0607 · 2015-03-23T20:29:34Z

I just have to ref. #3652. :-)

smarterclayton · 2015-03-24T04:10:42Z

This pull request now changes watch from being a separate resource (although it's still exposed for legacy purposes at the old paths) to GET /api/v1beta1/pods?watch=true&resourceVersion=10. I will change the v1beta3 clients to handle this behavior.

bgrant0607 · 2015-03-25T01:46:04Z

pkg/api/conversion.go

@@ -28,12 +30,48 @@ import (
 var Codec = runtime.CodecFor(Scheme, "")

 func init() {
+	Scheme.AddDefaultingFuncs(


Why is this necessary? I'd like to nuke this whole file.

Smart defaulting for ListOptions deserialized from an incoming request. Rather than make all people using it set those to empty and risk doing something wrong.

----- Original Message -----

@@ -28,12 +30,48 @@ import (
var Codec = runtime.CodecFor(Scheme, "")

func init() {

Scheme.AddDefaultingFuncs(

Why is this necessary? I'd like to nuke this whole file.

Reply to this email directly or view it on GitHub:
https://github.com/GoogleCloudPlatform/kubernetes/pull/5763/files#r27089694

bgrant0607 · 2015-03-25T02:06:20Z

pkg/runtime/helper.go

@@ -23,6 +23,7 @@ import (
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/conversion"
 )

+// TODO: move me to pkg/api/meta


What belongs in pkg/runtime vs. elsewhere?

Originally pkg/runtime was an opinionated part of conversion. Things strictly related to decoding generic objects in JSON showed up here. I think where we are today is that we want to have the core "smart object" model (pkg/smartobjects) which can do defaulting, conversion, new, etc, and then a set of serialization choices (JSON, query, protobufs, msgpack, whatever) laid on top of that (orthogonal to). The things in pkg/runtime that relate to the "scheme" (conversion, defaults) should move to smart objects. The things related to JSON serialization should be split out. I'm still torn on what we should do w.r.t. interfaces for helpful methods.

Ideally, I would think the smart objects code would have the ability to generate boilerplate helper methods (like ObjectMeta() or SetObjectMeta(...)) to your scheme. That would reduce the amount of code in api/meta (which is really about just having a common interface on all of our api objects to get and set metadata). We had just balked at adding one getter per type as being painful and hadn't got further.

bgrant0607 · 2015-03-25T02:27:36Z

I'm basically ok with the changes in this PR so far.

I'm also 100% on board with the shift towards subresources as opposed to API prefixes, and am more or less ok with the proposed API paths.

The difference between proxy and portforward is http vs. tcp? Why does portforward need a container name and not just a port? Do you also intend to support fd forwarding, for stdin/stdout/stderr (aka attach - #1521)?

I'd like to see an /endpoints subresource on services, pods, and nodes, and perhaps even on replication controllers. I could see proxy, etc. building upon that functionality.

An argument could be made that we should put the container name in the path, such as /pods/foo/containers/test/logs. Container subresources have been proposed before -- #1900. We don't necessarily need to allow mutation via that path right now, however.

We could similarly put ports in the path: /pods/foo/ports/5000.

Not directly related to this PR, but for proxy, we'll need to resolve the problems mentioned in #4440 at some point. The rewriting performed by the golang library only works for links within the same server, but not for links to other intra-cluster resources.

smarterclayton · 2015-03-25T04:00:35Z

The difference between proxy and portforward is http vs. tcp?

Yes I think so

Why does portforward need a container name and not just a port?

Good point, port is sufficient

Do you also intend to support fd forwarding, for stdin/stdout/stderr (aka attach - #1521)?

Exec does. One problem with all "upgrade" endpoints is that you have to full represent your request via query parameters or request headers, or you have to introduce custom client framing into the connection establishment protocol. It was non-obvious what the best solution was (an example is passing a remote exec that has a massive argument list - 64kb or more, which exceeds the amount of data you'll be able to send via headers even in http2).

An argument could be made that we should put the container name in the path, such as /pods/foo/containers/test/logs. Container subresources have been proposed before -- #1900. We don't necessarily need to allow mutation via that path right now, however.

It should be possible in api_installer to extract the subpath for a sub resource (below it) and pass that into the sub resource. Just hadn't hit the use case yet.

smarterclayton · 2015-03-25T21:13:38Z

Comments addressed, ptal

smarterclayton · 2015-03-26T16:34:54Z

poke :)

bgrant0607 · 2015-03-26T20:23:36Z

LGTM.

(Sorry, was OOO yesterday).

Needs to be rebased, unfortunately.

Convert url.Values -> an object, with appropriate versioning. ListOptions should also expose parameter names to swagger.

GET /pods?watch=true&resourceVersion=10 will now function equivalent to GET /watch/pods.

Formalize v1beta1 and v1beta3 style APIs in our test cases.

smarterclayton · 2015-03-27T21:25:18Z

Travis was a flake.

bgrant0607 · 2015-03-27T21:35:20Z

Merging.

…sioned Expose versioned query parameters and make watch an operation on List

feihujiang · 2015-12-03T08:48:11Z

pkg/api/meta/restmapper.go

@@ -14,6 +14,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */

+// TODO: move everything in this file to pkg/api/rest


@smarterclayton Sorry to refer this old PR and disturb you. I saw this todo and want to fix it, but I didn't catch why this file belongs to rest package, not meta package? Could you explain this? Thanks very much!

Will try to respond tomorrow

googlebot added the cla: no label Mar 22, 2015

smarterclayton mentioned this pull request Mar 22, 2015

Make label and field selector query strings versionable. #5233

Merged

smarterclayton force-pushed the get_input_parameters_versioned branch from 5b403ac to d86729c Compare March 22, 2015 23:01

smarterclayton mentioned this pull request Mar 22, 2015

Removing dead code in labels package and changing LabelSelector type #5769

Merged

smarterclayton changed the title ~~WIP - Handle versioning query parameters via conversions~~ WIP - Enable apiserver to handle more types of input and output Mar 22, 2015

This was referenced Mar 22, 2015

Add a bastion to apiserver (reverse lookup by IP address + proxy) #3481

Closed

Provide easy debug network access to services #1863

Closed

smarterclayton force-pushed the get_input_parameters_versioned branch 2 times, most recently from d1e7208 to 7382414 Compare March 23, 2015 03:08

smarterclayton force-pushed the get_input_parameters_versioned branch from 7382414 to 936ccfc Compare March 23, 2015 05:15

smarterclayton added area/apiserver area/api Indicates an issue on api area. labels Mar 23, 2015

bgrant0607 mentioned this pull request Mar 23, 2015

Consider not handling YAML in APIserver #1004

Closed

smarterclayton force-pushed the get_input_parameters_versioned branch 2 times, most recently from 6b35a1c to 1f126a5 Compare March 23, 2015 21:17

bgrant0607 mentioned this pull request Mar 23, 2015

apiserver proxy rewrites v1beta1 URLs to include a namespace in the path #5339

Closed

smarterclayton force-pushed the get_input_parameters_versioned branch from 1f126a5 to 7a17ece Compare March 24, 2015 01:31

bgrant0607 self-assigned this Mar 24, 2015

smarterclayton mentioned this pull request Mar 24, 2015

Support input streams being returned by the APIserver #5851

Merged

bgrant0607 reviewed Mar 25, 2015
View reviewed changes

smarterclayton force-pushed the get_input_parameters_versioned branch from 906a345 to 2bf9e92 Compare March 25, 2015 21:12

smarterclayton changed the title ~~WIP - Enable apiserver to handle more types of input and output~~ Expose versioned query parameters and make watch an operation on List Mar 25, 2015

smarterclayton force-pushed the get_input_parameters_versioned branch from 2bf9e92 to 1396739 Compare March 26, 2015 00:47

bgrant0607 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 26, 2015

smarterclayton force-pushed the get_input_parameters_versioned branch from 1396739 to a1e49b6 Compare March 26, 2015 21:12

smarterclayton added 3 commits March 27, 2015 15:03

Convert List query parameters via object conversion

1618c39

Convert url.Values -> an object, with appropriate versioning. ListOptions should also expose parameter names to swagger.

Move watch to being a resthandler resource and expose it on LIST

eb0eff6

GET /pods?watch=true&resourceVersion=10 will now function equivalent to GET /watch/pods.

Add more extensive tests to apiserver for variations in version

870da68

Formalize v1beta1 and v1beta3 style APIs in our test cases.

smarterclayton force-pushed the get_input_parameters_versioned branch from a1e49b6 to 870da68 Compare March 27, 2015 19:04

bgrant0607 added a commit that referenced this pull request Mar 27, 2015

Merge pull request #5763 from smarterclayton/get_input_parameters_ver…

f0da47b

…sioned Expose versioned query parameters and make watch an operation on List

bgrant0607 merged commit f0da47b into kubernetes:master Mar 27, 2015

nikhiljindal mentioned this pull request Mar 30, 2015

Updating proxy to return 301 to add a "/" at the end for #4958 (attempt 2) #6175

Merged

bgrant0607 mentioned this pull request Jun 5, 2015

Support for /watch in api paths should be removed #8337

Closed

0xmichalis mentioned this pull request Oct 29, 2015

Run serialization tests against upstream types and update v1beta3 openshift/origin#5485

Merged

feihujiang reviewed Dec 3, 2015
View reviewed changes

feihujiang mentioned this pull request Dec 4, 2015

Support subresource of node proxy #18010

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose versioned query parameters and make watch an operation on List #5763

Expose versioned query parameters and make watch an operation on List #5763

smarterclayton commented Mar 22, 2015

googlebot commented Mar 22, 2015

smarterclayton commented Mar 23, 2015

bgrant0607 commented Mar 23, 2015

smarterclayton commented Mar 24, 2015

bgrant0607 Mar 25, 2015

smarterclayton Mar 25, 2015

bgrant0607 Mar 25, 2015

smarterclayton Mar 25, 2015

bgrant0607 commented Mar 25, 2015

smarterclayton commented Mar 25, 2015

smarterclayton commented Mar 25, 2015

smarterclayton commented Mar 26, 2015

bgrant0607 commented Mar 26, 2015

smarterclayton commented Mar 27, 2015

bgrant0607 commented Mar 27, 2015

feihujiang Dec 3, 2015

smarterclayton Dec 3, 2015

Expose versioned query parameters and make watch an operation on List #5763

Expose versioned query parameters and make watch an operation on List #5763

Conversation

smarterclayton commented Mar 22, 2015

googlebot commented Mar 22, 2015

smarterclayton commented Mar 23, 2015

bgrant0607 commented Mar 23, 2015

smarterclayton commented Mar 24, 2015

bgrant0607 Mar 25, 2015

Choose a reason for hiding this comment

smarterclayton Mar 25, 2015

Choose a reason for hiding this comment

bgrant0607 Mar 25, 2015

Choose a reason for hiding this comment

smarterclayton Mar 25, 2015

Choose a reason for hiding this comment

bgrant0607 commented Mar 25, 2015

smarterclayton commented Mar 25, 2015

smarterclayton commented Mar 25, 2015

smarterclayton commented Mar 26, 2015

bgrant0607 commented Mar 26, 2015

smarterclayton commented Mar 27, 2015

bgrant0607 commented Mar 27, 2015

feihujiang Dec 3, 2015

Choose a reason for hiding this comment

smarterclayton Dec 3, 2015

Choose a reason for hiding this comment