kinvolk / lokomotive

🪦 DISCONTINUED Further Lokomotive development has been discontinued. Lokomotive is a 100% open-source, easy to use and secure Kubernetes distribution from the volks at Kinvolk
https://kinvolk.io/lokomotive-kubernetes/
Apache License 2.0
321 stars 49 forks source link

All HCL fields should have "optional" struct tag and we should validate the fields manually #239

Open invidian opened 4 years ago

invidian commented 4 years ago

Or we can use some kind of validation library, e.g. https://github.com/go-ozzo/ozzo-validation.

The motivation is, required field in HCL (so one without optional), can be just satisfied with foo = "", and usually we care to have actual value in the property. Not validating fields early means, that we will start provisioning the cluster with bad values and it will most likely fail during the process. To provide better user experience, we should validate all the fields early and return as many errors to the user as possible in one shot (so to avoid returning just one error at a time, as this is then very annoying to fix).

Additionally, all validation rules should include unit tests.

johananl commented 4 years ago

I'm not sure we want to just skip the HCL enforcement by setting optional on all fields.

Let's differentiate between the various types of "wrong config":

The tree cases are built on top of one another:

The first case - an omitted knob - is handled for us "for free" by the HCL library. If we set all knobs to optional we would have to re-implement the logic from the HCL library in cases where all we want to check is that a knob was specified.

I'd say that only if we never want to accept an empty string as a valid value for a knob it could make sense to skip the HCL validation altogether. In other cases I'd consider adding a "2nd layer" of validation on top of the HCL one for the relevant knobs.