Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Client package proposal #15730

Merged
merged 1 commit into from
Dec 9, 2015
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
349 changes: 349 additions & 0 deletions docs/proposals/client-package-structure.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,349 @@
<!-- BEGIN MUNGE: UNVERSIONED_WARNING -->

<!-- BEGIN STRIP_FOR_RELEASE -->

<img src="http://kubernetes.io/img/warning.png" alt="WARNING"
width="25" height="25">
<img src="http://kubernetes.io/img/warning.png" alt="WARNING"
width="25" height="25">
<img src="http://kubernetes.io/img/warning.png" alt="WARNING"
width="25" height="25">
<img src="http://kubernetes.io/img/warning.png" alt="WARNING"
width="25" height="25">
<img src="http://kubernetes.io/img/warning.png" alt="WARNING"
width="25" height="25">

<h2>PLEASE NOTE: This document applies to the HEAD of the source tree</h2>

If you are using a released version of Kubernetes, you should
refer to the docs that go with that version.

<strong>
The latest release of this document can be found
[here](http://releases.k8s.io/release-1.1/docs/proposals/client-package-structure.md).

Documentation for other releases can be found at
[releases.k8s.io](http://releases.k8s.io).
</strong>
--

<!-- END STRIP_FOR_RELEASE -->

<!-- END MUNGE: UNVERSIONED_WARNING -->

<!-- BEGIN MUNGE: GENERATED_TOC -->

- [Client: layering and package structure](#client-layering-and-package-structure)
- [Desired layers](#desired-layers)
- [Transport](#transport)
- [RESTClient/request.go](#restclientrequestgo)
- [Mux layer](#mux-layer)
- [High-level: Individual typed](#high-level-individual-typed)
- [High-level, typed: Discovery](#high-level-typed-discovery)
- [High-level: Dynamic](#high-level-dynamic)
- [High-level: Client Sets](#high-level-client-sets)
- [Package Structure](#package-structure)
- [Client Guarantees (and testing)](#client-guarantees-and-testing)

<!-- END MUNGE: GENERATED_TOC -->

# Client: layering and package structure

## Desired layers

### Transport

The transport layer is concerned with round-tripping requests to an apiserver
somewhere. It consumes a Config object with options appropriate for this.
(That's most of the current client.Config structure.)

Transport delivers an object that implements http's RoundTripper interface
and/or can be used in place of http.DefaultTransport to route requests.

Transport objects are safe for concurrent use, and are cached and reused by
subsequent layers.

Tentative name: "Transport".
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1


It's expected that the transport config will be general enough that third
parties (e.g., OpenShift) will not need their own implementation, rather they
can change the certs, token, etc., to be appropriate for their own servers,
etc..

Action items:
* Split out of current client package into a new package. (@krousey)

### RESTClient/request.go

RESTClient consumes a Transport and a Codec (and optionally a group/version),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel we use Codec a lot. Maybe we could clarify what this is? I think ideally it should be concerned with encoding and decoding only and not have anything to do with converting to different versions. Perhaps something like

type Codec interface {
    Encode(runtime.Object) []byte
    Decode([]byte]) runtime.Object
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that we can simplify the codec down to that, though with errors added. A Codec for a particular version should have the property that it consumes/produces objects of that version, or an error.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think consuming an *http.Client instead of a Transport makes more sense. The current RESTClient is constructed, then an *http.Client using a transport is shoved in. The current RESTClient and Request are all build around using an *http.Client and not a transport anyway. This also allows a user to customize the client (e.g. timeouts) before building the RESTClient instead of changing it afterwards. Seems better to me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that consuming *http.Client seems better.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer an interface rather than a concrete type. We've had several
places where the go http client was not usable as is (due to limitations
for proxying, http handling, and it's retry / timeout behavior). Since a
caller could extract the transport later it's not a huge deal in the short
term.

On Nov 12, 2015, at 7:39 PM, krousey notifications@github.com wrote:

In docs/proposals/client-package-structure.md
#15730 (comment):

+Transport objects are safe for concurrent use, and are cached and reused by
+subsequent layers.
+
+Tentative name: "Transport".
+
+It's expected that the transport config will be general enough that third
+parties (e.g., OpenShift) will not need their own implementation, rather they
+can change the Host field to point at their own servers, etc..
+
+Action items:
+* Split out of current client package into a new package. (@krousey)
+
+### RESTClient/request.go
+
+RESTClient consumes a Transport and a Codec (and optionally a group/version),

I think consuming an *http.Client instead of a Transport makes more sense.
The current RESTClient is constructed, then an *http.Client using a
transport is shoved in. The current RESTClient and Request are all build
around using an *http.Client and not a transport anyway. This also allows a
user to customize the client (e.g. timeouts) before building the RESTClient
instead of changing it afterwards. Seems better to me.


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

and produces something that implements the interface currently in request.go.
That is, with a RESTClient, you can write chains of calls like:

`c.Get().Path(p).Param("name", "value").Do()`

RESTClient is generically usable by any client for servers exposing REST-like
semantics. It provides helpers that benefit those following api-conventions.md,
but does not mandate them. It provides a higher level http interface that
abstracts transport, wire serialization, retry logic, and error handling.
Kubernetes-like constructs that deviate from standard HTTP should be bypassable.
Every non-trivial call made to a remote restful API from Kubernetes code should
go through a rest client.

The group and version may be empty when constructing a RESTClient. This is valid
for executing discovery commands. The group and version may be overridable with
a chained function call.

Ideally, no semantic behavior is built into RESTClient, and RESTClient will use
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How will the codec specify this semantic content? Any examples?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will add an example. It'll take the form of having the codec encode a struct containing the query parameters.

the Codec it was constructed with for all semantic operations, including turning
options objects into URL query parameters. Unfortunately, that is not true of
today's RESTClient, which may have some semantic information built in. We will
remove this.

RESTClient should not make assumptions about the format of data produced or
consumed by the Codec. Currently, it is JSON, but we want to support binary
protocols in the future.

The Codec would look something like this:

```go
type Codec interface {
Encode(runtime.Object) ([]byte, error)
Decode([]byte]) (runtime.Object, error)

// Used to version-control query parameters
EncodeParameters(optionsObject runtime.Object) (url.Values, error)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about DecodeParameters(Into) ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just removed it because of @krousey, since it's only needed by the server.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added comment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are parameters really special? Are we ever going to need to encode to headers? How does a client know what to decode to? I feel like two codecs (BodyCodec and URLCodec) would be better than one codec doing both.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with that.


// Not included here since the client doesn't need it, but a corresponding
// DecodeParametersInto method would be available on the server.
}
```

There should be one codec per version. RESTClient is *not* responsible for
converting between versions; if a client wishes, they can supply a Codec that
does that. But RESTClient will make the assumption that it's talking to a single
group/version, and will not contain any conversion logic. (This is a slight
change from the current state.)

As with Transport, it is expected that 3rd party providers following the api
conventions should be able to use RESTClient, and will not need to implement
their own.

Action items:
* Split out of the current client package. (@krousey)
* Possibly, convert to an interface (currently, it's a struct). This will allow
extending the error-checking monad that's currently in request.go up an
additional layer.
* Switch from ParamX("x") functions to using types representing the collection
of parameters and the Codec for query parameter serialization.
* Any other Kubernetes group specific behavior should also be removed from
RESTClient.

### Mux layer

(See TODO at end; this can probably be merged with the "client set" concept.)

The client muxer layer has a map of group/version to cached RESTClient, and
knows how to construct a new RESTClient in case of a cache miss (using the
discovery client mentioned below). The ClientMux may need to deal with multiple
transports pointing at differing destinations (e.g. OpenShift or other 3rd party
provider API may be at a different location).

When constructing a RESTClient generically, the muxer will just use the Codec
the high-level dynamic client would use. Alternatively, the user should be able
to pass in a Codec-- for the case where the correct types are compiled in.

Tentative name: ClientMux

Action items:
* Move client cache out of kubectl libraries into a more general home.
* TODO: a mux layer may not be necessary, depending on what needs to be cached.
If transports are cached already, and RESTClients are extremely light-weight,
there may not need to be much code at all in this layer.

### High-level: Individual typed

Our current high-level client allows you to write things like
`c.Pods("namespace").Create(p)`; we will insert a level for the group.

That is, the system will be:

`clientset.GroupName().NamespaceSpecifier().Action()`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is clientset supposed to be here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, added explanation

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you miss the resource (Pod etc.) in the chain? I thought it would be clientset.GroupName().Resource().NamespaceSpecifier().Action(),
and it's clientset.GroupName().Resource() that "returns the generated client that this section is about", not clientset.GroupName().

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@caesarxuchao It seems the resource also takes a namespace in his example on line 148. So it would be like clientset.GroupName().Pods("namespace").Create(p) if I'm understanding correctly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @krousey. I think you are right.


Where:
* `clientset` is a thing that holds multiple individually typed clients (see
below).
* `GroupName()` returns the generated client that this section is about.
* `NamespaceSpecifier()` may take a namespace parameter or nothing.
* `Action` is one of Create/Get/Update/Delete/Watch, or appropriate actions
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to settle on the pattern for sub resources. You don't have to do it here, but I'd like the current state called out as "insufficiently defined"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

from the type's subresources.
* It is TBD how we'll represent subresources and their actions. This is
inconsistent in the current clients, so we'll need to define a consistent
format. Possible choices:
* Insert a `.Subresource()` before the `.Action()`
* Flatten subresources, such that they become special Actions on the parent
resource.

The types returned/consumed by such functions will be e.g. api/v1, NOT the
current version inspecific types. The current internal-versioned client is
inconvenient for users, as it does not protect them from having to recompile
their code with every minor update. (We may continue to generate an
internal-versioned client for our own use for a while, but even for our own
components it probably makes sense to switch to specifically versioned clients.)

We will provide this structure for each version of each group. It is infeasible
to do this manually, so we will generate this. The generator will accept both
swagger and the ordinary go types. The generator should operate on out-of-tree
sources AND out-of-tree destinations, so it will be useful for consuming
out-of-tree APIs and for others to build custom clients into their own
repositories.

Typed clients will be constructabale given a ClientMux; the typed constructor will use
the ClientMux to find or construct an appropriate RESTClient. Alternatively, a
typed client should be constructable individually given a config, from which it
will be able to construct the appropriate RESTClient.

Typed clients do not require any version negotiation. The server either supports
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does someone write a client code library that supports multiple server versions? Do they have to also duplicate all of their higher level logic that takes the code (MyFuncV1 c.Client.Pods.Get() vs MyFuncV2 c.Client.Pods.Get())? Do they have to use conversions?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Options:

  1. Use the dynamic client (yuk).
  2. At run-time, choose a codec that can convert from what the server speaks to what you wrote. This is essentially what we do today, only we want to do it in a way that doesn't require the client code to change every time we make a change (no "internal" version).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You really hate the dynamic client, huh? :)

I think I'm ok with 2, but I'll want to think about it some. I do regret that we don't have per field addition schema versions on the individual objects.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.

the client's group/version, or it does not. However, there are ways around this:
* If you want to use a typed client against a server's API endpoint and the
server's API version doesn't match the client's API version, you can construct
the client with a RESTClient using a Codec that does the conversion (this is
basically what our client does now).
* Alternatively, you could use the dynamic client.

Action items:
* Move current typed clients into new directory structure (described below)
* Finish client generation logic. (@caesarxuchao, @lavalamp)

#### High-level, typed: Discovery

A `DiscoveryClient` is necessary to discover the api groups, versions, and
resources a server supports. It's constructable given a RESTClient. It is
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it need a RESTClient? A http.Client is enough.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, never mind. I see RESTClient.Client is a http.Client.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should ever use standard http client in our codebase. See previous statements about RESTClient offering help and abstraction that handles transport as well

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RESTClient has apiVersion and codec. The discovery client shouldn't have those, it's version agnostic and the messages it handles are not encoded to a version.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RESTClient does not require apiVersion. The absence of an apiversion is
not an error. It should use codec - why does discovery have to be tied to
JSON? And codec being absent could very well support the null codec (or a
simple JSON codec)

RESTClient exists to abstract access to RESTful apis. It also exists to
unify client-server interactions over HTTP with conventions. Having two
code paths to communicate to a server is not desirable.

On Mon, Oct 19, 2015 at 4:52 PM, Chao Xu notifications@github.com wrote:

In docs/proposals/client-package-structure.md
#15730 (comment)
:

+current version inspecific types.
+
+We will provide this structure for each version of each group. It is infeasible
+to do this manually, so we will generate this. The generator will accept both
+swagger and the ordinary go types.
+
+Typed clients will be constructabale given a MetaClient.
+
+Action items:
+* Move current typed clients into new directory structure (described below)
+* Finish client generation logic. (@lavalamp)
+
+#### High-level, typed: Discovery
+
+A DiscoveryClient is necessary to discover the api groups, versions, and
+resources a server supports. It's constructable given a RESTClient. It is

RESTClient has apiVersion and codec. The discovery client shouldn't have
those, it's version agnostic and the messages it handles are not encoded to
a version.


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

consumed by both the ClientMux and users who want to iterate over groups,
versions, or resources. (Example: namespace controller.)

The DiscoveryClient is *not* required if you already know the group/version of
the resource you want to use: you can simply try the operation without checking
first, which is lower-latency anyway as it avoids an extra round-trip.

Action items:
* Refactor existing functions to present a sane interface, as close to that
offered by the other typed clients as possible. (@caeserxuchao)
* Use a RESTClient to make the necessary API calls.
* Make sure that no discovery happens unless it is explicitly requested. (Make
sure SetKubeDefaults doesn't call it, for example.)

### High-level: Dynamic

The dynamic client lets users consume apis which are not compiled into their
binary. It will provide the same interface as the typed client, but will take
and return `runtime.Object`s instead of typed objects. There is only one dynamic
client, so it's not necessary to generate it, although optionally we may do so
depending on whether the typed client generator makes it easy.

A dynamic client is constructable given a config, group, and version. It will
use this to construct a RESTClient with a Codec which encodes/decodes to
'Unstructured' `runtime.Object`s. The group and version may be from a previous
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this assume a JSON encoding, which may or may not be the case?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added text

invocation of a DiscoveryClient, or they may be known by other means.

For now, the dynamic client will assume that a JSON encoding is allowed. In the
future, if we have binary-only APIs (unlikely?), we can add that to the
discovery information and construct an appropriate dynamic Codec.

Action items:
* A rudimentary version of this exists in kubectl's builder. It needs to be
moved to a more general place.
* Produce a useful 'Unstructured' runtime.Object, which allows for easy
Object/ListMeta introspection.

### High-level: Client Sets

Because there will be multiple groups with multiple versions, we will provide an
aggregation layer that combines multiple typed clients in a single object.

We do this to:
* Deliver a concrete thing for users to consume, construct, and pass around. We
don't want people making 10 typed clients and making a random system to keep
track of them.
* Constrain the testing matrix. Users can generate a client set at their whim
against their cluster, but we need to make guarantees that the clients we
shipped with v1.X.0 will work with v1.X+1.0, and vice versa. That's not
practical unless we "bless" a particular version of each API group and ship an
official client set with earch release. (If the server supports 15 groups with
2 versions each, that's 2^15 different possible client sets. We don't want to
test all of them.)

A client set is generated into its own package. The generator will take the list
of group/versions to be included. Only one version from each group will be in
the client set.

A client set is constructable at runtime from either a ClientMux or a transport
config (for easy one-stop-shopping).

An example:

```go
import (
api_v1 "k8s.io/kubernetes/pkg/client/typed/generated/v1"
ext_v1beta1 "k8s.io/kubernetes/pkg/client/typed/generated/extensions/v1beta1"
net_v1beta1 "k8s.io/kubernetes/pkg/client/typed/generated/net/v1beta1"
"k8s.io/kubernetes/pkg/client/typed/dynamic"
)

type Client interface {
API() api_v1.Client
Extensions() ext_v1beta1.Client
Net() net_v1beta1.Client
// ... other typed clients here.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to confirm Dynamic is also part of this, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I see now that GroupVersion() below is returning dynamic.Client


// Included in every set
Discovery() discovery.Client
GroupVersion(group, version string) dynamic.Client
}
```

Note that a particular version is chosen for each group. It is a general rule
for our API structure that no client need care about more than one version of
each group at a time.

This is the primary deliverable that people would consume. It is also generated.

Action items:
* This needs to be built. It will replace the ClientInterface that everyone
passes around right now.

## Package Structure

```
pkg/client/
----------/transport/ # transport & associated config
----------/restclient/
----------/clientmux/
----------/typed/
----------------/discovery/
----------------/generated/
--------------------------/<group>/
----------------------------------/<version>/
--------------------------------------------/<resource>.go
----------------/dynamic/
----------/clientsets/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will we have a HEAD or latest clientset? The one that we will use with the latest HEAD code.

---------------------/release-1.1/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking more about this, am wondering if we really need to duplicate it here in HEAD.
The client will be there in the corresponding release branch.

How will cherrypicks in release branch work? Will we do the cherrypick, generate the client in release branch and then copy it back here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure of the answers to these questions, but I really want go get to Just Work™. And I don't think go get let's you specify a git tag.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want the tests to continue to be run, so it's worth leaving in HEAD, I think. @krousey's point is legit, too.

---------------------/release-1.2/
---------------------/the-test-set-you-just-generated/
```

`/clientsets/` will retain their contents until they reach their expire date.
e.g., when we release v1.N, we'll remove clientset v1.(N-3). Clients from old
releases live on and continue to work (i.e., are tested) without any interface
changes for multiple releases, to give users time to transition.

## Client Guarantees (and testing)

Once we release a clientset, we will not make interface changes to it. Users of
that client will not have to change their code until they are deliberately
upgrading their import. We probably will want to generate some sort of stub test
with a clienset, to ensure that we don't change the interface.


<!-- BEGIN MUNGE: GENERATED_ANALYTICS -->
[![Analytics](https://kubernetes-site.appspot.com/UA-36037335-10/GitHub/docs/proposals/client-package-structure.md?pixel)]()
<!-- END MUNGE: GENERATED_ANALYTICS -->