search

elastic / cloud-on-k8s

Elastic Cloud on Kubernetes
Other
2.56k stars 695 forks source link

Revisit config struct #1838

Open anyasabo opened 4 years ago

anyasabo commented 4 years ago

We should verify we need pkg/apis/common/v1alpha1/config.Config. It references that it is a workaround for this issue: https://github.com/kubernetes-sigs/kubebuilder/issues/528 but according to that issue and https://github.com/kubernetes-sigs/controller-tools/pull/126 https://github.com/kubernetes-sigs/controller-tools/issues/294

There are now either resolutions or workarounds, so we should revisit our approach.

Originally added in https://github.com/elastic/cloud-on-k8s/pull/1030

racevedoo commented 4 years ago

@anyasabo is anyone working on this? I'd like to work on it. Basically we should remove the Config struct in favor of something like json.RawMessage, as mentioned in the issues, right?

pebrc commented 4 years ago

I took another look at this:

https://github.com/kubernetes-sigs/controller-tools/pull/126 was closed without merging the support for interface{} types. The argument being that json.RawMessage (or runtime.RawExtension for k8s types) should suffice unless there is strong user demand for explicit interface{} support.

That leaves us with three options: using json.RawMessage, lobbying for interface{} support in controller-tools, do nothing.

Use json.RawMessage

Switching our current implementation to json.RawMessage does seem to have disadvantages over our current approach. We would have to work with json.RawMessage whereever we programmatically build Config structs which is marginally less convenient than working with map[string]interface{} given the potential for typos, invalid JSON and lack of editor support

Lobby for interface{} support in controller-tools

Somewhat unlikely to succeed given that our use case is expressable via json.RawMessage. Also with k8s moving towards structural schemas we might want to revisit our current approach of embedding arbitrary config objects inside our CRDs (even though this might continue to work with structural schemas if we use the x-kubernetes-preserve-unknown-fields: true OpenAPI vendor extension).

Do nothing

I am tempted to go with this last option for now, until we have to deal with structural schemas.

thbkrkr commented 4 years ago

Since lobbying seems very difficult and there is no improvement in using json.RawMessage, doing nothing seems to me the best option as well.