Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CRDGenerator: Add support for size constraints #5836

Open
baloo42 opened this issue Mar 25, 2024 · 7 comments
Open

CRDGenerator: Add support for size constraints #5836

baloo42 opened this issue Mar 25, 2024 · 7 comments
Labels
component/crd-generator Related to the CRD generator enhancement

Comments

@baloo42
Copy link
Contributor

baloo42 commented Mar 25, 2024

Is your enhancement related to a problem? Please describe

At the moment the CRDGenerator does not support size constraints:

  • minItems
  • minProperties
  • minLength
  • maxItems
  • maxProperties
  • maxLength

Describe the solution you'd like

A new annotation @Size allows to define min and/or max constraints on a field or its accessors:

@Target({ ElementType.ANNOTATION_TYPE, ElementType.FIELD, ElementType.METHOD })
@Retention(RetentionPolicy.RUNTIME)
public @interface Size {
  long min() default 0;
  long max() default Long.MAX_VALUE;
}

The CRDGenerator detects this annotation and adds JSONSchema constraints dependending on the java type:

Java Type Constraints
String minLength
maxLength
Array/List minItems
maxItems
Map minProperties
maxProperties

If the annotation is used on an unsupported type, a warning message is logged.

Describe alternatives you've considered

No response

Additional context

Size constraints become more important if someone wants to use CEL Validation rules on fields of a CRD.
Because of the cost calculations of a CEL rule, constraints on strings/lists/maps are often only allowed if limits are defined.

@rohanKanojia
Copy link
Member

@andreaTP : Could you please share your thoughts on this one? Do you think this could be a good addition to CRD Generator?

@andreaTP
Copy link
Member

andreaTP commented Apr 2, 2024

Hi @baloo42 and thanks a lot for this proposal.

This intersects with the discussion around CEL and validation, I would prefer to have a(shared) understanding of the final target from this effort before commenting on (perceived?)"sub-tasks" like this one.

Can you please come up with a design proposal along with a mocked/dummy dev-UX sample?

@baloo42
Copy link
Contributor Author

baloo42 commented Apr 3, 2024

This intersects with the discussion around CEL and validation, I would prefer to have a(shared) understanding of the final target from this effort before commenting on (perceived?)"sub-tasks" like this one.

In my opinion it is independent of CEL/Kubernetes Validation Rules:
minLength/maxLength, minItems/maxItems, minProperties/maxProperties are JSON Schema / OpenAPI validation constraints and can be useful even without the combination with Kubernetes Validation Rules.
If we look at other similar projects like Spring-Doc / swagger-core, they implemented such constraints by detecting the JSR-303 annotation @Size and adding the appropriate constraints. We should do the same but as previously decided with own annotations:

If a field or one of its accessors is annotated with io.fabric8.generator.annotation.Size

public class ExampleSpec {
  @Size(min = 1, max = 3)
  String stringWithLowerAndUpperLimits;
  @Size(min = 1, max = 3)
  List<String> listWithLowerAndUpperLimits;
  @Size(min = 1, max = 3)
  String[] arrayWithLowerAndUpperLimits;
  @Size(min = 1, max = 3)
  Map<String, String> mapWithLowerAndUpperLimits;
}

The field will have the minLength/maxLength or minItems/maxItems or minProperties/maxProperties properties in the generated CRD, such as:

          spec:
            properties:
              stringWithLowerAndUpperLimits:
                maxLength: 3
                minLength: 1
                type: string
              listWithLowerAndUpperLimits:
                items:
                  type: string
                maxItems: 3
                minItems: 1
                type: array
              arrayWithLowerAndUpperLimits:
                items:
                  type: string
                maxItems: 3
                minItems: 1
                type: array
              mapWithLowerAndUpperLimits:
                additionalProperties:
                  type: string
                maxProperties: 3
                minProperties: 1
                type: object
            type: object

see PoC: baloo42#4

And what about the relation to Kubernetes Validation rules?

Size constraints become more important if someone wants to use CEL Validation rules on fields of a CRD.
Because of the cost calculations of a CEL rule, constraints on strings/lists/maps are often only allowed if limits are defined.

--> see https://kubernetes.io/blog/2022/09/23/crd-validation-rules-beta/#runtime-cost-limits-for-crd-validation-rules

In addition to the estimated cost limit, CEL keeps track of actual cost while evaluating a CEL expression and will halt execution of the expression if a limit is exceeded.

With the estimated cost limit already in place, the runtime cost limit is rarely encountered. But it is possible. For example, it might be encountered for a large resource composed entirely of a single large list and a validation rule that is either evaluated on each element in the list, or traverses the entire list.

CRD authors can ensure the runtime cost limit will not be exceeded in much the same way the estimated cost limit is avoided: by setting maxItems, maxProperties and maxLength on array, map and string types.

@andreaTP
Copy link
Member

andreaTP commented Apr 3, 2024

OpenAPI validation constraints

right, thanks for pointing this out, in this case we would need to support more constructs, e.g.:
multipleOf, uniqueItems, exclusiveMaximum etc.
do you mind crafting a comprehensive list?

Thanks a lot for the POC and the rest, highly useful!

On top of my mind:

  • probably we should consider a better naming other than Size or figure out a little hierarchy of validators
  • those things are useful even outside the generator, options are:
    • a separate(optional?) validation module
    • baked into the core models

cc. @shawkins and @manusa

@baloo42
Copy link
Contributor Author

baloo42 commented Apr 3, 2024

right, thanks for pointing this out, in this case we would need to support more constructs, e.g.:
multipleOf, uniqueItems, exclusiveMaximum etc.
do you mind crafting a comprehensive list?

No problem, I will create a new issue with an overview.
There is already an outdated issue #3768. I think this can be closed afterwards.

Notes:

probably we should consider a better naming other than Size

Why is the name Size bad? (I like it, because it results in the same behaviour as developers are used to with JSR-303).
We could also use something like @Schema and @ArraySchema as it is used in swagger-core. This could be also useful for additional features like x-kubernetes-list-type but requires more annotations and maybe a hierarchy as you said.

or figure out a little hierarchy of validators

In my opinion, we don't have to implement a hierarchy (yet). @Min and @Max define a range where an integer or float is valid. The value to define the limit is of the type float and can be also negative.
In contrast, the values ​​for the limits of a @Size constraint can only be positive and an integer.

those things are useful even outside the generator, options are:
a separate(optional?) validation module
baked into the core models

Can you explain this more in detail?

@andreaTP
Copy link
Member

andreaTP commented Apr 3, 2024

No problem, I will create a new issue with an overview.

Appreciated, thanks 🙏

uniqueItems is not supported by Kubernetes.

mentioning because its extremely interesting the nuance here ( ref ) :

  • uniqueItems is supported
  • uniqueItems cannot be set to true

🤦‍♂️

Can you explain this more in detail?

Yes, everything we are talking about in this thread is not related to the CRDGenerator but to CRDs in general.
We should evaluate placing those new annotations in io.fabric8.kubernetes.model.annotation so that they can be used (to be defined "how") by:

  • crd-generator
  • java-generator
  • plain Java definition of CRDs

Does it make more sense now?

cc. @metacosm as he demonstrated interest in chipping in this discussion.

@baloo42
Copy link
Contributor Author

baloo42 commented Apr 3, 2024

mentioning because its extremely interesting the nuance here ( ref ) :

uniqueItems is supported
uniqueItems cannot be set to true

Yeah, it's very misleading.

Yes, everything we are talking about in this thread is not related to the CRDGenerator but to CRDs in general.
We should evaluate placing those new annotations in io.fabric8.kubernetes.model.annotation so that they can be used (to be defined "how") by:

  • crd-generator
  • java-generator
  • plain Java definition of CRDs
    Does it make more sense now?

Yes, now it makes sense. I agree with you that the annotations should be stored centrally so that they can be reused.
And if I read your comment in #5851 (comment) again, I agree with you too, that this also applies for a "validation module" to implement something like "offline" validation / "client-side" validations.

@manusa manusa added component/crd-generator Related to the CRD generator enhancement labels Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/crd-generator Related to the CRD generator enhancement
Projects
None yet
Development

No branches or pull requests

4 participants