-
Notifications
You must be signed in to change notification settings - Fork 305
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
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: admilazz 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 |
/assign krabhishek8260 |
I'll add some unit tests as well. |
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
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 |
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 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.
This fluent library can handle that just fine. E.g.
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. |
Just heads up, you may want to integrate the label selector builder. I have it coded specifically for informers pieces that are part of a different active PR, but it seems like your code can leverage same pattern: |
Regarding the label selector builder, that seems like the kind of thing that can easily be added (possibly via extension methods):
or, perhaps simpler and cleaner...
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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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".)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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. :-)
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
|
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. |
After looking into the PR, I suggest start a new nuget package The reason is that a SDK providing 2 different ways to use is really confused. |
{ | ||
#region KubernetesRequest | ||
/// <summary>Represents a single request to Kubernetes.</summary> | ||
public sealed class KubernetesRequest : ICloneable |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean?
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.
Actually, it uses the same HttpClient as the Autorest client does. |
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 If it seems really terrible to make that stuff public, moving this into a different class |
Okay, I'll put it in a new namespace and use extension methods rather than a partial class.
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. |
There is a new extension method that I've added to address this as part of other PR APIVersion and Kind should really be thought of as part of serialization type metadata then the actual payload. |
@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. |
Just create a new project and make it a friend of the main one, same way we did it for tests in #421 |
I guess I'll put this in a separate project, then. |
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 |
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:It works with both built-in and custom resource type. For advanced operations, you can:
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: