rewrite api module to use provided traits from k8s_openapi #157

clux · 2020-02-29T00:39:38Z

This has grown to be a semi-rewrite of Api...

RawApi renamed to Resource (much better tests now)
Api revamped
openapi feature removed (now always depend on non-default-features version)
runtime now use MetaContent trait for types with ObjectMeta
examples + readme updated to showcase new api
complications (below) resolved

so glad I'm doing this before kubecon... this is such a huge change.

Notably, this makes our Api work with ANY kubernetes type exported by k8s_openapi, removes a bunch of duplicated structs from apimachinery so we can focus on what's unique to this library; the runtime.

A few cons:

openapi versions of apimachinery are quite Option heavy, leading to less ergonomic manipulation of their values (we can impl helpers later)
Object<P, U> is currently broken - it needs to implement k8s_openapi::Resource via codegen now
codegen bundled with k8s-openapi-derive has drawbacks (see comment below)

first commit for #150

clux · 2020-02-29T09:33:39Z

Tried to make some compatibility layer thing with:

#[cfg(feature = "openapi")]
impl<P, U> k8s_openapi::Metadata for Object<P, U>
where
    P: Clone,
    U: Clone,
{
    type Ty = ObjectMeta;
    fn metadata(&self) -> Option<&Self::Ty> {
        Some(&self.metadata)
    }
}

but that won't work because the Metadata trait seems to require the Resource trait implemented (and we need to do that separately.

It's also a bit awkward with the Metadata trait returning Option<MetadataType> (because it's catering to list type i guess?), even though in all normal object structs the Metadata is there. That's the one thing we have relied on, even for snowflakes (which are gone in this branch).

clux · 2020-02-29T22:59:08Z

Currently fixes #158, fixes #150, fixes #25, fixes #96

Arnavion · 2020-03-01T01:25:18Z

It's also a bit awkward with the Metadata trait returning Option<MetadataType> (because it's catering to list type i guess?)

No. List<T> is also a resource with metadata, and thus also impls Metadata with Ty = ListMeta.

The reason that Metadata::metadata() returns an Option is that metadata is optional. Eg Pod::metadata and List::metadata. And that in turn is because the openapi spec says it's optional. (That said, I've always wondered exactly when the metadata can not be specified, since it's required for create / patch from the user, and always sent in responses from the API server. Is there a situation where the API server doesn't send it.)

clux · 2020-03-01T08:58:27Z

The reason that Metadata::metadata() returns an Option is that metadata is optional. Eg Pod::metadata and List::metadata. And that in turn is because the openapi spec says it's optional. (That said, I've always wondered exactly when the metadata can not be specified, since it's required for create / patch from the user, and always sent in responses from the API server. Is there a situation where the API server doesn't send it.)

Yeah, that makes sense. Having investigated more, it looks like the kubernetes api is just super-conservative on optionality. Just like PodSpec on Pod. How can that ever be optional? The API definitely hard limits this on even input.

Interestingly, trying to create a Pod one without PodSpec will complain that spec.containers: Required value., which is the same error you get if you supply spec: {}. Maybe it's conflating optionality and defaulting, or maybe their internal data type they accept through the api, and the type they really must have before creating it differ.

At any rate, it's a shame because this causes so many guaranteed unwraps in controllers, and I really would just like to fail to even deserialize an object if it doesn't satisfy my assumptions about it.

clux · 2020-03-01T09:11:21Z

No. List<T> is also a resource with metadata, and thus also impls Metadata with Ty = ListMeta.

(What I meant is that types with ListMeta has a much more restrictive offering in their metadata. I was speculating that maybe the optionality of metadata somehow was somehow related to the sparseness of metadata in LIST calls, but as you've pointed out, it's just plain marked optional even for types with Metadata<Ty=ObjectMeta>)

Arnavion · 2020-03-01T11:33:56Z

Interestingly, trying to create a Pod one without PodSpec will complain that spec.containers: Required value., which is the same error you get if you supply spec: {}.

The golang code defines Pod.spec as type PodSpec, so its zero value is an actual object with zeroed fields rather than nil. So both {} and { "spec": {} } will deserialize the same way - to an object with zeroed fields.

clux · 2020-03-01T13:56:31Z

Have updated all examples, and fixed all tests to work with the current setup. Removed Object because there was no way to actually implement the required Resource trait except via codegen, so need to go down that route (which is fine, that's a nice pattern).

Have tried out the k8s-openapi-derive setup first. This gets most of the way there, and actually lets us have a custom thing that implements k8s_openapi::Resource. See crd_api example.

However, it's not super customizable atm and merging as it stands would (afaikt) make us drop support for manipulating status objects. Have raised Arnavion/k8s-openapi#62 upstream. But might explore an additional approach.

@teozkr how do you feel about this pr?

nightkr · 2020-03-01T14:23:40Z

Big fan of this in concept, but I'm a bit out of order right now so I haven't done a proper review yet (pizzas are bloody dangerous, as it turns out..).

That said, I don't think this resolves (my interpretation of) #150, although it did end up getting a bit derailed in this direction (sorry!). In particular, namespace is still implied (which bubbles up from Resource aka RawApi). It would also be great IMO if Api's K was given to the operations (so the path became Api::get::<K> rather than Api::<K>::get), so that a single Api instance could be shared. I'll sketch up a PR for that when I feel a bit better.

nightkr · 2020-03-01T14:27:14Z

And for the record, 👍 from me on requiring k8s-openapi and dropping Object.

clux · 2020-03-01T16:58:15Z

In particular, namespace is still implied (which bubbles up from Resource aka RawApi). It would also be great IMO if Api's K was given to the operations (so the path became Api::get::<K> rather than Api::<K>::get), so that a single Api instance could be shared. I'll sketch up a PR for that when I feel a bit better.

Yeah, I realised that this only addresses the "derailed part" of this issue, and didn't want to change that world in one go, as this PR is already quite big. But I agree that this could be quite nice.

That said, I am not sure I understand how connection pooling would be improved given that the underlying connection is already shared.

nightkr · 2020-03-01T17:05:19Z

Right, forgot about the distinction between Api (which is currently (at least) unique per K) and APIClient (which is shared and owns the connection pool). My bad.

nightkr · 2020-03-01T17:23:11Z

src/api/crds.rs

+///
+/// This is the smallest amount of info we need to run the API against a CR
+/// The version, and group must be set by the user.
+pub struct CustomResource {


This looks like a leftover from the days of feature-gated k8s-openapi.

This is a temporary new thing until I have a fully reliable derive crate. This will be removed at some point. (All the other autogenerated Api::v1Pod disappeared, but the manual Api::customResource had to go somewhere temporaily).

nightkr · 2020-03-01T17:24:03Z

src/api/crds.rs

+}
+
+/// Make Resource useable on CRDs without k8s_openapi
+impl From<CustomResource> for Resource {


They seem to match 1:1, so.. why not just have CrBuilder build a Resource directly? (Assuming that this is still worthwhile to keep.)

I could've called it a ResourceBuilder perhaps - might be more appropriate - but it's only really useful for custom resources, that was why. It's almost the same, it infers the api_version from version and kind.

nightkr · 2020-03-01T17:25:40Z

src/api/metadata.rs

+    fn resource_ver(&self) -> Option<String>;
+    fn name(&self) -> String;
+    fn namespace(&self) -> Option<String>;


Very minor nitpick, but.. I think all of these could borrow the String instead?

Yeah, can do. Was flipping between when implementing it. Switching now. Want to rename the trait to something shorter anyway. Maybe just straight Meta.

Actually, I might still clone in the non-expensive getters like Meta::name , because it's really conflicting in all the examples where we need to burrow multiple parts from the main object:

let name = Meta::name(&pod); let phase = pod.status.as_ref().unwrap().phase.as_ref().unwrap(); let containers = pod.spec.as_ref().unwrap() .containers .into_iter() .map(|c| c.name) .collect::<Vec<_>>(); info!("Found initial pod {} ({}) with {:?}", name, phase, containers);

If we all burrowed from pod in Meta, then the least significant burrow for the name would prevent us from burrowing pod.spec (cannot move out of shared reference).

The client code already knows what Metadata::Ty will be, so it should just be able to access the field directly (let meta = &pod.metadata.name;). Then again, I can see that it would be a pain to change all the examples again.. :/

Once you do that you quickly hit move out of burrowed context since metadata is optional. I was trying a bunch of different configurations, and have gone for the most ergonomic one so far, maybe there is a better one, but also kind of just want to get this done now :D

Fair enough!

nightkr · 2020-03-01T17:29:48Z

src/api/object.rs

+    Added(K),
+    Modified(K),
+    Deleted(K),
+    Error(ErrorResponse),


This wasn't really added here, but.. maybe the error conditions should be moved out into a Result<WatchEvent<K>> instead? This should probably be a separate issue rather than blocking this PR.

Yeah, that might be something worth raising. Have also considered picking up the upstream WatchEvent from k8s_openapi which in its latest version is very similar to the one we have (although with even more error separation).

nightkr · 2020-03-01T17:32:20Z

src/api/object.rs

+/// Note that this is only used internally within reflectors and informers,
+/// and is generally produced from list/watch/delete collection queries on an `Resource`.
+///
+/// This is now equivalent to k8s_openapi::List<T>, but with extra convenience impls


Is this machinery worth it to be able to say for x in list.iter() {} rather than for x in list.items.iter() {}? Personally, I almost prefer the latter..

if feels nicer to beginners who can do node like things like:

for pod in api.list() { println!("{:?}", pod) }

and it's common pattern to implement for wrapper types like this. I don't think there's a real cost other than the couple of extra lines in here?

For one, k8s_openapi::List implements a bunch of traits that kube::api::List doesn't (Debug, PartialEq, Serialize, ...).

Yeah. that is true. Probably worth moving this over as well 😪

src/api/openapi.rs

this reduces a bunch of unwrap mechanics on metadata in examples.

clux · 2020-03-01T19:38:02Z

This thing is too big to fail.

clux added 3 commits February 29, 2020 07:25

change root details of RawApi to match k8s_openapi::Resource

34dbdb4

first commit for #150

crbuilder tweaks

0cd8a6b

fmt

8c2100f

clux force-pushed the openapi-traits branch from 197323d to 8c2100f Compare February 29, 2020 07:26

an acceptable Crd solution

8d38d98

clux force-pushed the openapi-traits branch from e295e77 to 8d38d98 Compare February 29, 2020 09:14

clux added 7 commits February 29, 2020 09:37

all of api compiling

f9d5f08

factor listparams builder out of runtime - fixes #158

b11ea47

make informers work (see pod_informer)

7a44d17

make reflectors work

051e543

clippy + fmt

04d012b

rename raw.rs to resource.rs

7a6da49

change rawapi interface - no need for type param

30a6630

This was referenced Feb 29, 2020

implement more bindings to the standard api-resources #25

Closed

Clone some more generics from openapi #96

Closed

clux added 4 commits March 1, 2020 00:33

start hard-depending on k8s-openapi

25d5d35

fmt

52c1ac9

fmt + clippy

19668c8

stray comments + feature gates

eb88028

clux changed the title ~~change root details of RawApi to match k8s_openapi::Resource~~ rewrite api module to use provided traits from k8s_openapi Mar 1, 2020

clux added 2 commits March 1, 2020 00:50

ok clippy

ea3199d

tests..

7ad8598

Arnavion mentioned this pull request Mar 1, 2020

Does the Metadata trait need to depend on Resource? Arnavion/k8s-openapi#61

Closed

remove Object

5a3ccff

clux added 6 commits March 1, 2020 12:51

fix examples

93c2334

get crd example mostly working

b095efc

rename file

3ebdc29

normalize comments is annoying

cf7bfac

last example + clippy

d731f38

doc tests

97a16b1

ci test fix

a674092

clux marked this pull request as ready for review March 1, 2020 13:59

clux added 2 commits March 1, 2020 13:59

last reference to openapi feature

278a3dc

quick readme tweak

17a0f85

nightkr reviewed Mar 1, 2020

View reviewed changes

MetaContent -> Meta.

d7cda41

this reduces a bunch of unwrap mechanics on metadata in examples.

clux merged commit 52e04a3 into master Mar 1, 2020

clux deleted the openapi-traits branch March 1, 2020 19:38

clux mentioned this pull request Mar 1, 2020

better codegen for custom resources #159

Closed

nightkr mentioned this pull request Mar 2, 2020

[0.27.x] Rename all ObjectMeta fields to camelCase #163

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rewrite api module to use provided traits from k8s_openapi #157

rewrite api module to use provided traits from k8s_openapi #157

clux commented Feb 29, 2020 •

edited

Loading

clux commented Feb 29, 2020

clux commented Feb 29, 2020

Arnavion commented Mar 1, 2020

clux commented Mar 1, 2020

clux commented Mar 1, 2020

Arnavion commented Mar 1, 2020

clux commented Mar 1, 2020

nightkr commented Mar 1, 2020

nightkr commented Mar 1, 2020

clux commented Mar 1, 2020 •

edited

Loading

nightkr commented Mar 1, 2020 •

edited

Loading

nightkr Mar 1, 2020

clux Mar 1, 2020 •

edited

Loading

nightkr Mar 1, 2020

clux Mar 1, 2020 •

edited

Loading

nightkr Mar 1, 2020

clux Mar 1, 2020

clux Mar 1, 2020

nightkr Mar 1, 2020

clux Mar 1, 2020

nightkr Mar 1, 2020

nightkr Mar 1, 2020

clux Mar 1, 2020

nightkr Mar 1, 2020

clux Mar 1, 2020

nightkr Mar 1, 2020

clux Mar 1, 2020 •

edited

Loading

clux commented Mar 1, 2020

rewrite api module to use provided traits from k8s_openapi #157

rewrite api module to use provided traits from k8s_openapi #157

Conversation

clux commented Feb 29, 2020 • edited Loading

clux commented Feb 29, 2020

clux commented Feb 29, 2020

Arnavion commented Mar 1, 2020

clux commented Mar 1, 2020

clux commented Mar 1, 2020

Arnavion commented Mar 1, 2020

clux commented Mar 1, 2020

nightkr commented Mar 1, 2020

nightkr commented Mar 1, 2020

clux commented Mar 1, 2020 • edited Loading

nightkr commented Mar 1, 2020 • edited Loading

Choose a reason for hiding this comment

clux Mar 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clux Mar 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clux Mar 1, 2020 • edited Loading

Choose a reason for hiding this comment

clux commented Mar 1, 2020

clux commented Feb 29, 2020 •

edited

Loading

clux commented Mar 1, 2020 •

edited

Loading

nightkr commented Mar 1, 2020 •

edited

Loading

clux Mar 1, 2020 •

edited

Loading

clux Mar 1, 2020 •

edited

Loading

clux Mar 1, 2020 •

edited

Loading