Skip to content

Add a fluent Kubernetes API #406

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

Closed
wants to merge 13 commits into from

Conversation

admilazz
Copy link
Contributor

@admilazz admilazz commented Apr 8, 2020

This change adds a fluent API that is concise, flexible, and works well with custom resources and custom actions. The basic flow is that from the client, you can create a request, and given a request you can modify or execute it. If c is a Kubernetes client, then these expressions create different types of requests:

c.New<V1Pod>() // list all pods in all namespaces
c.New<V1Pod>("ns") // list all pods in a namespace
c.New<V1Pod>("ns", "name") // get a single pod
c.New<V1Pod>("ns", "name").Delete() // delete a pod by name
c.New(pod).Post() // create a pod
c.New(pod, setBody: false).Delete() // delete a pod if you have the object already
c.New<V1Pod>("ns").LabelSelector("X").FieldSelector("Y") // list pods with label and field selectors
c.New(pod).Status().Put() // update a pod's status
c.New(pod).Subresource("log") // read a pod's logs
c.New(pod).Put().DryRun(true) // simulate replacing a resource
c.New().RawUri("/apis/…") // specify a raw URL
etc...

It works with both built-in and custom resource type. For advanced operations, you can:

  • Use custom resource types and HTTP verbs
  • Modify request and content headers
  • Modify the query string
  • Access subresources
  • Stream the request or upgrade it for web socket or SPDY connections -- I can send a PR for exec over SPDY later (addressing issue MuxedStream for STDIN does not close itself properly #397 )
  • Stream the response and access the raw network connection
  • Specify group, version, and kind manually, or specify the entire raw URL
  • Configure watches -- I can send a PR for an improved watch type later

To execute any request so constructed, it's simply: req.ExecuteAsync(). This returns a response object that gives you full access to the status code, response headers, and response stream. (It does not throw an exception if an HTTP error response is returned.) It also has convenience methods for reading and deserializing the response in various ways.

A request can be executed multiple times, including in parallel. (I.e. a request is not modified or consumed by executing it.) You can clone a request if you want to write a method that takes a request and executes a modified version of it without mutating the original.

If you want to execute a request, check the status (including throwing exceptions for errors), and deserialize the body all in one go, it's simply req.ExecuteAsync<T>().

To assist with modifying resources, it supports an atomic get-modify-update operation via req.ReplaceAsync<T>(...).

As mentioned above, I also have improved watching and exec implemented (over both SPDY and web sockets), and I can send PRs for those later after handling legal and other issues.

NOTE: This PR builds on and includes changes from two other PRs (#404 and #405). I'm not sure how best to represent that in a PR, but the files actually added or changed in this specific PR are:

  • Kubernetes.ConfigInit.cs
  • Kubernetes.Fluent.cs
  • Kubernetes.WebSocket.cs
  • KubernetesRequest.cs
  • Scheme.cs
  • Kubernetes.Fluent.Tests.cs

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 8, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: admilazz
To complete the pull request process, please assign krabhishek8260
You can assign the PR to them by writing /assign @krabhishek8260 in a comment when ready.

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

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Apr 8, 2020
@admilazz
Copy link
Contributor Author

admilazz commented Apr 8, 2020

/assign krabhishek8260

@admilazz
Copy link
Contributor Author

admilazz commented Apr 8, 2020

I'll add some unit tests as well.

@macsux
Copy link
Contributor

macsux commented Apr 9, 2020

This is similar to https://github.com/tintoy/dotnet-kube-client. I personally prefer that approach vs the autogenerated one we have right now as part of official SDK, because autogenerated stuff is too verbose and doesn't properly cover CRDs. I think we need buy-in from maintainers to move into that direction. If there is a buy-in, then I think we need to have a longer discussion about what the API syntax should look like (potentially compare to what the KubeClient does), as they tackle some extra scenarios. For example, as quoted off their page

Some operations in the Kubernetes API can return a different response depending on the arguments passed in. For example, a request to delete a v1/Pod returns the existing v1/Pod (as a PodV1 model) if the caller specifies DeletePropagationPolicy.Foreground but returns a v1/Status (as a StatusV1 model) if any other type of DeletePropagationPolicy is specified.

To handle this type of polymorphic response KubeClient uses the KubeResultV1 model (and its derived implementations, KubeResourceResultV1 and KubeResourceListResultV1).

KubeResourceResultV1 can be implicitly cast to a TResource or a StatusV1, so consuming code can continue to use the client as if it expects an operation to return only a resource or expects it to return only a StatusV1:

As far as the proposed syntax in this PR, I would prefer to see List/Get type operations to be explicit rather than part of .New<T> which doesn't really feel intuitive.

@admilazz
Copy link
Contributor Author

admilazz commented Apr 9, 2020

Hi, macsux. I appreciate the comment.

Taking a look now at dotnet-kube-client, I think it seems completely different. Their client appears to be a strongly typed client that works with specific Kubernetes types. For example, in their example you do client.PodsV1().Delete(…). That's not to say it's better or worse, but that model requires the generation (manually or otherwise) of lots of code to add all the operations for all the different Kubernetes resource types, and then there's the question of whether their client can work for custom resources, custom operations, etc. I do like their client better than the current C# client, but I like my fluent library better still. (I'll admit to being biased. :-)

By contrast, the fluent client I'm proposing is completely generic. It has zero knowledge about pods, for example. It just works with any type and any operation. That means it can be very small, requiring no code generation, and very flexible. But the user has to know how to use it correctly.

Some operations in the Kubernetes API can return a different response depending on the arguments passed in. For example, a request to delete a v1/Pod returns the existing v1/Pod (as a PodV1 model) if the caller specifies DeletePropagationPolicy.Foreground but returns a v1/Status (as a StatusV1 model) if any other type of DeletePropagationPolicy is specified.

This fluent library can handle that just fine. E.g. r.ExecuteAsync<V1Pod>() versus r.ExecuteAsync<V1Status>(). Or, in the unlikely case that you don't know which kind of delete request you're sending, you can r.ExecuteAsync() and figure out it out dynamically. (Though usually you don't need the object returned from a DELETE. In that case you can just execute it without deserializing the response.)

As far as the proposed syntax in this PR, I would prefer to see List/Get type operations to be explicit rather than part of .New which doesn't really feel intuitive.

That starts restraining the possibilities and making assumptions, though. List and Get are both GET requests, and they differ only in the type of object returned. Imagine a resource with a subresource. If that subresource is called "backups", then you would expect GETting it to return a list of backups. But if the subresource is called "backupStatus" then you'd expect it to return a single object. The URL has exactly the same form (with only "backups" replaced by "backupStatus"). That is to say, whether a request is a list request cannot be determined in general from the request itself. It requires external knowledge. Now, it can be done for built-in types because you can make assumptions about how built-in Kubernetes resources work, but once you get into custom resources you can no longer make those assumptions. I think that having a single, general-purpose interface that works the same way for all types is better than adding methods like List() which may or may not work depending on the type of resource being accessed. Or, if List() and Get() are just synonyms and do exactly the same thing, then List() would add nothing but eye candy...

In case there was a misunderstanding above, New doesn't know about list or get either, since under the hood they are both GET requests. It makes no assumptions about what will be returned.

@macsux
Copy link
Contributor

macsux commented Apr 9, 2020

@admilazz
Copy link
Contributor Author

admilazz commented Apr 9, 2020

Regarding the label selector builder, that seems like the kind of thing that can easily be added (possibly via extension methods):

r.LabelSelectorBuilder().HasLabel("x").LabelEquals("a", "b").Apply()

or, perhaps simpler and cleaner...

r.BuildLabelSelector(b => b.HasLabel("x").LabelEquals("a", "b"))

It seems like the kind of thing I'd want to support if it already existed in the library, but if it doesn't already exist I'd like to try to keep this PR as slim as possible, at least until I see whether it gains any traction. (Also, if I added a label selector builder I'd also feel compelled to add a field selector builder and probably something around field managers, for consistency. :-P)

@@ -41,9 +42,8 @@ public Kubernetes(KubernetesClientConfiguration config, HttpClient httpClient) :
public Kubernetes(KubernetesClientConfiguration config, HttpClient httpClient, bool disposeHttpClient) : this(httpClient, disposeHttpClient)
{
ValidateConfig(config);
CaCerts = config.SslCaCerts;
SkipTlsVerify = config.SkipTlsVerify;
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this is getting dropped?

Copy link
Contributor Author

@admilazz admilazz Apr 9, 2020

Choose a reason for hiding this comment

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

The config object is saved below in this.config = config, and they are accessible from there. (I'm not 100% sure what you mean by "getting dropped".)

Copy link
Member

Choose a reason for hiding this comment

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

any detail reason for this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it to save the entire config object rather than just two fields out of it, because it was useful to have it when instantiating a request - in particular to construct the credentials. Now that I use the ServiceCredentials object from the Kubernetes client, this can probably be reverted. But it's not like any information is being lost here. :-)

@brendandburns
Copy link
Contributor

fwiw, I'm not opposed to this client, but the group/version/kind info isn't something that developers typically think about when they create objects programattically.

e.g. when you do new V1Pod(...) the GVK isn't populated so you're going to have a bunch of developers who need to do the boilerplate:

var pod = new V1Pod();
pod.Group = "";
pod.Version = "v1";
pod.Kind = "Pod";

@admilazz
Copy link
Contributor Author

admilazz commented Apr 9, 2020

fwiw, I'm not opposed to this client, but the group/version/kind info isn't something that developers typically think about when they create objects programattically.
e.g. when you do new V1Pod(...) the GVK isn't populated so you're going to have a bunch of developers who need to do the boilerplate:
var pod = new V1Pod();
pod.Group = "";
pod.Version = "v1";
pod.Kind = "Pod";

True. In my own code, I have a simple generic helper function that instantiates a type and fills out those fields. I could add something like that here as well.

P.S. What do you think of New() for the request as opposed to, say, Request()? (When I originally wrote this, it was its own class and not a partial Kubernetes class, but now it has to share space with many other methods, so a more specific name may be better.

EDIT: I renamed New -> Request and added a new New method that creates an object and fills out the API group/version and Kind.

@admilazz admilazz mentioned this pull request Apr 10, 2020
@tg123
Copy link
Member

tg123 commented Apr 11, 2020

After looking into the PR,
My understanding is that this is something same as
Azure management sdk Fluent vs Azure management sdk

I suggest start a new nuget package Kubenetes.Fluent which depends on current SDK
The same way as Azure SDK.

The reason is that a SDK providing 2 different ways to use is really confused.
The sharing compoments are models only.
In additional, fluent use a different httpclient from autorest which also contributes to inconsistency.

{
#region KubernetesRequest
/// <summary>Represents a single request to Kubernetes.</summary>
public sealed class KubernetesRequest : ICloneable
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this can be combined with autorest

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean?

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 11, 2020
@admilazz
Copy link
Contributor Author

I suggest start a new nuget package Kubenetes.Fluent which depends on current SDK
The sharing compoments are models only.

Some significant code to construct credentials and manage SSL certificates is shared as well. In order to put this in a separate library, that code would have to be made public or else it would have to be duplicated into the new library, or else the fluent API would become a little less flexible. I'm not opposed to putting this in a separate library - and I probably will do that if it's not accepted here - but I am somewhat opposed to copying the code for credentials and SSL certificate handling.

It's not simply a different way to do the same thing, however. It does add some significant new capabilities. But I agree that there's a lot of overlap too.

In additional, fluent use a different httpclient from autorest which also contributes to inconsistency.

Actually, it uses the same HttpClient as the Autorest client does.

@brendandburns
Copy link
Contributor

I agree with @tg123 that this should be a different package (it doesn't necessarily have to be a different Nuget, even just a different namespace seems ok to me)

Can you see how invasive it is to make the necessary SSL stuff public, I feel like something like an Kubernetes.authorizeRequest(<request-object-interface>) should work pretty decently as a public interface.

If it seems really terrible to make that stuff public, moving this into a different class FluentKubernetes is a reasonable compromise.

@admilazz
Copy link
Contributor Author

I agree with @tg123 that this should be a different package (it doesn't necessarily have to be a different Nuget, even just a different namespace seems ok to me)

Okay, I'll put it in a new namespace and use extension methods rather than a partial class.

Can you see how invasive it is to make the necessary SSL stuff public

Well if it's just in a new namespace it can be internal. :-)

P.S. The blocking PR at the moment is the "convenient model extensions" one.

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 15, 2020
@macsux
Copy link
Contributor

macsux commented Apr 17, 2020

fwiw, I'm not opposed to this client, but the group/version/kind info isn't something that developers typically think about when they create objects programattically.

There is a new extension method that I've added to address this as part of other PR
https://github.com/macsux/kubernetes-client-csharp/blob/feature/crd/src/KubernetesClient/Extensions.cs#L31-L40

APIVersion and Kind should really be thought of as part of serialization type metadata then the actual payload.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 17, 2020
@k8s-ci-robot
Copy link
Contributor

@admilazz: PR needs rebase.

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.

@macsux
Copy link
Contributor

macsux commented Apr 17, 2020

Some significant code to construct credentials and manage SSL certificates is shared as well. In order to put this in a separate library, that code would have to be made public or else it would have to be duplicated into the new library, or else the fluent API would become a little less flexible. I'm not opposed to putting this in a separate library - and I probably will do that if it's not accepted here - but I am somewhat opposed to copying the code for credentials and SSL certificate handling.

Just create a new project and make it a friend of the main one, same way we did it for tests in #421

@admilazz
Copy link
Contributor Author

admilazz commented Apr 17, 2020

I guess I'll put this in a separate project, then.

@admilazz admilazz closed this Apr 17, 2020
@macsux
Copy link
Contributor

macsux commented Apr 22, 2020

When I meant separate project I meant within the same SDK solution - moving towards a modular approach, not a separate GitHub project. Would love to see this code as part of main SDK

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants