grafana / kindsys

a kind system for schematizing objects
Apache License 2.0
12 stars 0 forks source link

Introduce `Resource`, refine `Kind` interfaces, define standard Grafana shape <> K8s shape transforms #22

Closed sdboyer closed 1 year ago

sdboyer commented 1 year ago

This PR introduces:

All that said, this PR is very messy. The unmarshaling and remarshaling and stages? They just didn't come out at all the way i'd originally imagined.

The main goal is to get us to a minimally working state that at least sorta captures this k8s shape->grafana shape transform, which has been really confusing for folks and the source of noteworthy divergence between what's in core/the apiserver vs. what's happening in app sdk/kindsys. The reason to stop here, in this minimally working state, is that it can unblock iterative progress on several threads of potentially concurrent work:

and i'm forgetting...at least one. So yes, this is a mess, and i'm now pretty convinced that we need to revisit some of the assumptions in the kind schema format. But it works with core, for dashboards, and we can move iteratively to fix these things from here.

sdboyer commented 1 year ago

whoops, i missed grafana/grafana#70287 - updating accordingly

sdboyer commented 1 year ago

Actually...hmm, not sure if that's a good idea. That PR isn't against main - would be good to merge it back in. Kindsys shouldn't reflect what's there until it's in main.

IfSentient commented 1 year ago

@IfSentient does it even make sense to pull this in now, then we can iteratively work on combining what's here with what's there?

I think pulling in the resource.Object interface and its associated types should be fine, the only thing that gives me pause is that there's a possibility that it's still in flux. I was already looking at how to reasonably separate out the resource package, or at least its constructs (go submodules proved to have too many pain points) so that it can be imported without the associated kubernetes dependencies from the app-sdk's k8s package, and the grafana-plugin-sdk-go's dependencies that we need in the plugin package.

Ultimately, I'm still also trying to determine where the border between kindsys and the app-sdk is, because it's a tricky question to draw a hard border. Right now, kindsys is only used in the SDK for codegen purposes, so is it less ergonomic for a user to have to import both the SDK and kindsys when working with kinds, or does it make sense to try to slice up the SDK into smaller pieces that exist independently of each other (submodules is I guess the only real solution here aside from multiple repos, which re-introduces the same pain points as using kindsys).

sdboyer commented 1 year ago

After some chatting, and seeing the needed work in grafana/grafana#70728, i'm going to copy over the resource subpackage from app sdk, as well as this file, which contains the logic for going back and forth between the object form we schematize in our kinds, and k8s native object form.

sdboyer commented 1 year ago

This has uncovered a significant divergence between what's being done in grafana, and what's in app-sdk/kindsys. Fortunately, i think we can nail this one pretty dead-on, and add a whole lot of clarity while we do.

sdboyer commented 1 year ago

i've pushed up a handwavy sketch of what i think the key, top-level interfaces for kinds are:

 ┌----> Core -------> TypedCore
 |
Kind -> Custom -----> TypedCustom
 |
 └----> Composable -> TypedComposable (TODO)

This doesn't compile yet, and there are methods not yet even written on them. But i think the docs are there, which in itself is a big deal given all the confusion we've had. The only thing that's missing is a treatment of the K8s shape <> Grafana shape transform relation.

i've also starting pulling over the relevant types from app sdk for defining Resource.

i'll be continuing to iterate on this through the day!

sdboyer commented 1 year ago

Fun note - these interfaces are where we're going to (finally!) see runtime composition. i made a separate commit with what i expect those methods will look like. I think it's suggestive of the expected use pattern here - whether a given core kind supports composition or not, it should fit tidily into the system we have, and be easy to manage generically.

sdboyer commented 1 year ago

Tests are failing in the current state, and i have to refactor the encoder to mirror changes in the decoder. And there's lots of half-baked method implementations that panic. So, definitely not ready to merge.

Buuuut...this all fundamentally works: saving dashboards passes through the new kind.Validate() method introduced in this PR, and works.

sdboyer commented 1 year ago

This is basically ready for review.

i need to rewrite the issue description (goal has shifted considerably since i first wrote it) and probably could stand to do another pass at just cleaning up what's in the code. But given that this PR has evolved to the goal of just trying to get to an intermediate state that unblocks several other paths of work we can work on concurrently, it's gonna be...messy.

sdboyer commented 1 year ago

Re: kind schema format - I'd really like to just have us specify new subresources, instead of doing this trickery with annotations. It will be cleaner to implement, simplify what the decoders have to do, and most critically, the relation between what app devs write in schema vs. what the fields on their generated types will just be obvious. Unstructured could become something more like:

// THIS IS HANDWAVING, PLZ READ IT AS A STRONG OPINION LOOSELY HELD
type UnstructuredResource struct {
    APIVersion string
    Kind string

    // just the standard k8s metadata fields
    Meta K8sMetadata `json:"metadata"`
    // kindsys specified fields, univeral to all resource kinds/GRDs
    GrafanaMeta GrafanaMetadata `json:"grafanaMetadata"`
    // kind-specific metadata
    KindMeta map[string]any `json:"customMetadata"`

    Spec   map[string]any `json:"spec,omitempty"`
    Status map[string]any `json:"status,omitempty"`
}

Now, CRDs don't allow additional subresources, which means we'll need to keep some shim in place to make all this work in cloud (where we still have to operate using a CRD representation with all those constraints). But since we're settled now on having our own aggregated apiserver, we could decide this is our target format and keep the CRD representation an implementation detail, rather than having to answer questions like "do we EVER want to serialize a grafana resource to a grafana shape, or do we only ever use k8s shape?"