flux2 icon indicating copy to clipboard operation
flux2 copied to clipboard

Published 20 hours ago • verified publisher icon fluxcd

Review and enforce CRD Validation

Open pjbgf opened this issue 3 years ago • 5 comments

In the run-up to maturing flux APIs, we should review all CRD validation and ensure that they align with expected input.

Controllers:

  • [ ] source-controller
  • [ ] kustomize-controller
  • [ ] helm-controller
  • [ ] notification-controller
  • [ ] image-reflector-controller
  • [ ] image-automation-controller

Additional points to consider:

  • Use kubebuilder validation and add tests, as per https://github.com/fluxcd/helm-controller/pull/520.
  • .Spec fields should be marked as required across all CRDs.
  • String fields should have a max length and pattern defined (when possible).
  • We should use reference types from pkg, and also improve its current validation: https://github.com/fluxcd/pkg/blob/main/apis/meta/reference_types.go

pjbgf avatar Aug 17 '22 10:08 pjbgf

Hey @pjbgf I would like work on this. I am not sure about couple of points, can you please clarify:

  • Point 1: The validation and tests needs to be added to ValueKey and TargetPath fields only?
  • Point 2: Got it
  • Point3: max length and patterns be defined on reference types from pkg linked in Point 4?

Santosh1176 avatar Aug 18 '22 20:08 Santosh1176

@Santosh1176 here's more info:

  1. The PR was just an example on how to implement the validation, as in added via +kubebuilder:validation and then tested with testenv. We need to review each CRD for all controllers, and make sure that all fields are validated.
  2. OK
  3. The validation will depend on each field at hand, so may need some research on how that field is being used, and what rules it has to abide by.
  4. Those reference types at the moment only have +required, they lack max length and pattern validation. In some CRD we may be using other types instead of them to refer to Kubernetes objects, when that's the case we must change that.

pjbgf avatar Aug 18 '22 21:08 pjbgf

@pjbgf Any reference/guidance for max and min length properties for strings fields?

Santosh1176 avatar Aug 22 '22 19:08 Santosh1176

@Santosh1176 unfortunately this will be on a case-by-case basis. Feel free to propose some validation for one of the CRDs and during the review we will provide you with some feedback.

pjbgf avatar Aug 23 '22 17:08 pjbgf

@Santosh1176 I am interested contributing to this issue as well, have you made any progress since?

What about I take one of those task item? for example, the notification controller?

Cheers.

snebel29 avatar Sep 06 '22 10:09 snebel29