bluek8s / kubedirector

Kubernetes Director (aka KubeDirector) for deploying and managing stateful applications on Kubernetes
Apache License 2.0
401 stars 91 forks source link

Fix CRD auto generation #459

Open aleshchynskyi opened 3 years ago

aleshchynskyi commented 3 years ago

I noticed that kubedirector currently doesn't support operator-sdk generate crds feature due to few reasons:

  1. It contains custom JSON unmarshalling code (decode.go) and according to the comment on SetupPackage generator cannot handle it.
  2. Current manually written CRDs include couple of validation constraints.

In my opinion, both cases can be resolved by:

  1. Using ignore tag (see golang docs); json:"-" and inline tag json:",inline" on SetupPackage's fields.
  2. Using KubeBuilder CRD validation tags (see KubeBuilder Docs)

This feature can be especially useful when putting structs from other libraries like k8s api (such as Affinity) into KubeDirector's CRDs to avoid manually declaring all the OpenAPI properties.

joel-bluedata commented 3 years ago

Sounds promnising yeah.

aleshchynskyi commented 2 years ago

There's a drawback in KubeBuilder tags, such as impossibility to put a contraint to values of the array or the map. It requires either:

For example:

//+kubebuilder:validation:MinLength=1
//+kubebuilder:validation:MaxLength=15
//+kubebuilder:validation:Pattern="^[a-z0-9]([-a-z0-9]*[a-z0-9])?$"
ServiceIDs []string `json:"serviceIDs"`

won't work.

But:

ServiceIDs []ServiceID `json:"serviceIDs"`

//+kubebuilder:validation:MinLength=1
//+kubebuilder:validation:MaxLength=15
//+kubebuilder:validation:Pattern="^[a-z0-9]([-a-z0-9]*[a-z0-9])?$"
type ServiceID string

will work. Except it requires all the usages to be aware of ServiceID causing syntax errors.

Another example:

MinResources *corev1.ResourceList `json:"minResources,omitempty"`

Since corev1.ResourceList is in external library, we cannot alter its constraints. Hence the only option is dropping constraints.

joel-bluedata commented 2 years ago

I guess in the latter case we could define our own ResourceList type and internally convert it as necessary.

Not worth the churn for this release (0.7.0) but it does still seem like something we should figure out how to do.