search

Arnavion / k8s-openapi

Rust definitions of the resource types in the Kubernetes client API
Apache License 2.0
379 stars 41 forks source link

Add `PLURAL_NAME` and `Scope` to `Resource` #98

Closed kazk closed 3 years ago

kazk commented 3 years ago

Continuing from #88 by @MikailBag (I hope you don't mind).

Changes:

Closes #65 Closes #73

Maybe a new version can be released after this?

Arnavion commented 3 years ago

So I started my own implementation of this, but with a more stricter parsing of the operation paths. Specifically it requires the path to be /api/{version}/$rest (for empty group) or /apis/{group}/{version}/$rest (for non-empty group), where $rest is required to be $plural/{name}.

From this, I see these resources as being problematic:

They fall into these buckets:

  1. io.k8s.apimachinery.pkg.apis.meta.v1.APIGroupList is a get-only resource. It's gotten via a global endpoint and is cluster-scoped. The operation path does not contain the plural name.

  2. io.k8s.api.authorization.v1.SubjectAccessReview is a create-only resource. It's created via a global endpoint so it's presumably cluster-scoped. The operation path contains the plural name.

  3. io.k8s.api.authentication.v1.TokenRequest is a create-only resource. It's created via a namespaced resource, though I'm not sure it counts as a sub-resource. The operation path does not contain the plural name.

  4. io.k8s.api.policy.v1beta1.Eviction is a create-only resource that's specifically a subresource of a namespaced resource. The operation path does not contain the plural name.

(For resources of types (2) and (3), it would be interesting to see what the .metadata.self_link of the created object is set to.)

Anyway, defaulting all of these to have PLURAL_NAME: ""; Scope = Cluster; feels weird. The Resource trait just means "something with apiVersion and kind fields", but the point of the plural name and scope is to be able to build a URL for CRUD, which is going to break for these fake values. So I think:

To express the lack of a plural name, PLURAL_NAME should be an Option<&'static str> set to None, rather than defaulting to "".

What do you think? cc @clux and @MikailBag for what kube needs from these two pieces of information.

kazk commented 3 years ago

Anyway, defaulting all of these to have PLURAL_NAME: ""; Scope = Cluster; feels weird.

Yeah, I agree.

I prefer your more strict approach, and your suggestions should still cover our use cases. As far as I know, we'd like to stop guessing plural names for resources if known, and check that the scope is valid (at compile time or run time). And I don't think we need them for subresources ((3) and (4)). Yeah, @MikailBag and @clux knows more since I haven't worked on this area.

By the way, how about using NAME and SINGULAR_NAME to match io.k8s.apimachinery.pkg.apis.meta.v1.APIResource?

kazk commented 3 years ago

io.k8s.api.core.v1.Binding might be an exception :/

clux commented 3 years ago
  • (1) should be considered a cluster-scoped resource or a neither-scoped resource, without a plural name.
  • (2) should be considered a cluster-scoped resource, with a plural name.
  • (3) should be considered a neither-scoped resource, without a plural name.
  • (4) should be considered a neither-scoped resource, without a plural name.

To express the lack of a plural name, PLURAL_NAME should be an Option<&'static str> set to None, rather than defaulting to "".

I think this makes sense, although it's possible that your notion of neither-scoped actually coincides with subresources. At least AFAIKT all the ones in category 3 and 4 have urls that indicate use against another root resource; Eviction sits inside Pod, TokenRequest sits inside ServiceAccount, EphemeralContainers sit inside Pod, Scale sits inside many. That's also how we define subresources in kube (because they need an extra component to construct the url), and we execute all of these as methods on our Api<RootKind>.

Subresource as a scope, therefore, would be ideal if it works out. It could potentially let us implement a portion of the more rest-y subresources in a more generic way.

Some other points:

Arnavion commented 3 years ago

I've pushed the fix for the operation path check so that (2) is also considered cluster-scoped now, and so that (1), (3) and (4) have PLURAL_NAME = None instead of "".

Now the list of "neither-scoped" resources is:

although it's possible that your notion of neither-scoped actually coincides with subresources.

Does it? That's why I was asking about TokenRequest, etc. Even though it's created under a ServiceAccount, it only has a create operation that's a POST on a ServiceAccount, not a PUT on a /serviceaccounts/{service_account_name}/tokenrequests/{token_request_name}. When you create a TokenRequest, what does its .metadata.self_link say? Does it have something like /serviceaccounts/{service_account_name}/tokenrequests/{token_request_name} ?

My point is, given the operation path is /serviceaccounts/{name}/token, it seems to me that you'd want the string "token" to be able to construct the URL dynamically, no? ie for non-subresources you need the plural name (eg "serviceaccounts"), and for subresources you need the "subresource" name (eg "token").

Apart from that, for the io.k8s.apimachinery.pkg.apis.meta.v1 types, I guess they can be hard-coded to be cluster-scoped. And I guess List<T>'s scope should just forward to T's scope?

clux commented 3 years ago

Does it? That's why I was asking about TokenRequest, etc. Even though it's created under a ServiceAccount, it only has a create operation that's a POST on a ServiceAccount, not a PUT on a /serviceaccounts/{service_account_name}/tokenrequests/{token_request_name}. When you create a TokenRequest, what does its .metadata.self_link say? Does it have something like /serviceaccounts/{service_account_name}/tokenrequests/{token_request_name} ?

Yeah, the metadata.selfLink for TokenRequest does seem to have the /serviceaccounts/{service_account_name}/tokenrequests/{token_request_name} form.

My point is, given the operation path is /serviceaccounts/{name}/token, it seems to me that you'd want the string "token" to be able to construct the URL dynamically, no? ie for non-subresources you need the plural name (eg "serviceaccounts"), and for subresources you need the "subresource" name (eg "token").

Yeah, that is true, but for subresources we need both strings. The Resource implementation of ServiceAccount is needed regardless, because everything except that last url component piggy-backs on top of it, and the last string could even be a user supplied &str to a generic Api<ServiceAccount> method. AFAIKT the Resource implementation of TokenRequest would be insufficient stand-alone without also having the Resource implementation of TokenRequestServiceAccount (edit typo), that's why I thought a subresource scope would make more sense than a neither-scope.

Our use case wise, we might be able to do something with implementations on top of a doubly generic type returned by Api<ServiceAccount>::subresource<TokenRequest>.

And I guess List<T>'s scope should just forward to T's scope?

Probably. Though, just like Status, it imo doesn't really feel like it's a true resource, and might not need the impl.

Arnavion commented 3 years ago

Yeah, the metadata.selfLink for TokenRequest does seem to have the /serviceaccounts/{service_account_name}/tokenrequests/{token_request_name} form.

Err, it seems to show the exact opposite. The self-link is the operation path that you POSTed to to create the TokenRequest.

 "metadata": {
   "selfLink": "/api/v1/namespaces/token-demo/serviceaccounts/token-client-test/token",

AFAIKT the Resource implementation of TokenRequest would be insufficient stand-alone without also having the Resource implementation of TokenRequest,

I think one of those is a typo?

Our use case wise, we might be able to do something with implementations on top of a doubly generic type returned by Api<ServiceAccount>::subresource<TokenRequest>

Yes, that kind of use-case is why I mentioned you would want the "token" string somewhere.

So the requirements appear to be:

Options I can think of:

  1. const PLURAL_NAME: Option<&'static str>; const SUBRESOURCE_NAME: Option<&'static str>;. Only one will be set to Some() based on Scope, and the user will have to unwrap() that one. Downside: the unwrap() is noise because it'll always succeed, but it has to be done to satisfy the type-system.

  2. const TWO_HARD_PROBLEMS_IN_COMPUTER_SCIENCE_NAME: &'static str; that is either the plural name or the subresource name based on Scope. Downside: have to name the field something that makes sense despite it having two quite different values.

  3. const TWO_HARD_PROBLEMS_IN_COMPUTER_SCIENCE_NAME: TwoHardProblemsInComputerScienceName; ... enum TwoHardProblemsInComputerScienceName { Resource { plural_name: &'static str }, Subresource { name: &'static str } } Downside: like with (1), the user has to match even though they know from Scope which one is correct, so the user has to write unreachable!() in the other arm.

  4. Anything else?

MikailBag commented 3 years ago

Anything else?

Well, another option: for a subresource, store parent resource as a type parameter (so TokenRequest as Resource::Parent == ServiceAccount, or even impl SubresourceOf<ServiceAccount> for TokenRequest). This way downstream libraries can protect user against invalid combinations, such as using TokenRequest on Pod.

clux commented 3 years ago

Well, another option: for a subresource, store parent resource as a type parameter (so TokenRequest as Resource::Parent == ServiceAccount, or even impl SubresourceOf<ServiceAccount> for TokenRequest). This way downstream libraries can protect user against invalid combinations, such as using TokenRequest on Pod.

I'm not sure you can have a 1-1 relation like that @MikailBag, some subresources (like Scale) can be put on top of more than one resource.

clux commented 3 years ago

Options I can think of:

  1. const PLURAL_NAME: Option<&'static str>; const SUBRESOURCE_NAME: Option<&'static str>;. Only one will be set to Some() based on Scope, and the user will have to unwrap() that one. Downside: the unwrap() is noise because it'll always succeed, but it has to be done to satisfy the type-system.
  2. const TWO_HARD_PROBLEMS_IN_COMPUTER_SCIENCE_NAME: &'static str; that is either the plural name or the subresource name based on Scope. Downside: have to name the field something that makes sense despite it having two quite different values.
  3. const TWO_HARD_PROBLEMS_IN_COMPUTER_SCIENCE_NAME: TwoHardProblemsInComputerScienceName; ... enum TwoHardProblemsInComputerScienceName { Resource { plural_name: &'static str }, Subresource { name: &'static str } } Downside: like with (1), the user has to match even though they know from Scope which one is correct, so the user has to write unreachable!() in the other arm.

Yeah, all of those would work and are reasonable to me. I think 2. makes sense if we call it something like RESOURCE_IDENTIFIER / RESOURCE_SEGMENT / IDENTIFIER. Option 3 would mean we can do an if let Subresouce(identifier) = ... and extract the string + do check for subresource in one go, but if we can already do that check via Scope, then it's kind of just duplicate information. Option 1 similarly has more than one way to check for the same thing: is_subresource is SUBRESOURCE_NAME.is_some() and SCOPE == Subresource, and we'd have to communicate that the Option is linked to a Scope.

I think one of those is a typo?

Sorry, fixed typo.

Arnavion commented 3 years ago

Okay, I've pushed the branch with what is hopefully the final implementation.

Please check https://github.com/Arnavion/k8s-openapi/blob/plural-and-namespaced/k8s-openapi-tests/src/resource.rs and verify everything is categorized correctly.

Does that branch look like it does everything kube needs?

kazk commented 3 years ago

Looks great, @Arnavion. I like URL_PATH_SEGMENT too.

clux commented 3 years ago

That branch looks great. The associated type marker traits under Resource looks like a much more helpful way to mark it than an enum as well. I imagine we can do everything we need with this, without even having to do asserts on the scope.

I think the classification there looks great as well.

k8s_openapi::apimachinery::pkg::apis::meta::v1::Status is the only potentially debatable one. Personally, I don't think it needs to implement Resource at all as it's a common return value (generally for delete requests) rather than something you can "verb" on anything, but I also don't think it matters.