kubedirector icon indicating copy to clipboard operation
kubedirector copied to clipboard

Fix CRD auto generation

Open aleshchynskyi opened this issue 3 years ago • 3 comments

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.

aleshchynskyi avatar Apr 08 '21 20:04 aleshchynskyi

Sounds promnising yeah.

joel-bluedata avatar Apr 09 '21 19:04 joel-bluedata

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:

  • dropping the constraint
  • creating a type alias for the value and adding a tag to the type

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.

aleshchynskyi avatar Oct 13 '21 16:10 aleshchynskyi

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.

joel-bluedata avatar Oct 13 '21 17:10 joel-bluedata