CRD-Generator v7 - Orga

[baloo42]

@shawkins has announced some important details on the future of the CRD-Generator in a PR comment.

He has also started with the refactoring:
https://github.com/fabric8io/kubernetes-client/tree/crd-generator-v7-development

I also see, that the crd-generator-apt module will be removed. Do you plan a replacement?

Yes, we have discussed adding a maven plugin and a gradle plugin for non-quarkus / josdk based usage. Maybe something utilizing jandax for discovery?

Where things stand:

• there are a couple of hacks, noted with TODOs, due to choices we made about where to pick up annotations from. Nothing too crazy.

• the usage of CustomResource has been made optional

• the core logic is no longer static. It's not quite wired all the way up to accept an ObjectMapper, but that shouldn't be too hard

• parallelism has not been wired back in. It will be straight-forward to add that back if needed (either separate ResolvingContexts, or make sure that it's thread-safe)

• there are some generation differences, there are three tests failing due to:

  • The required list was previously unordered, now it is.
  • The printer column type is taken directly from the property - but in the test it's expecting a string instead of an array type for one of the columns. What is the expectation here?
  • It now better understands the anygetter / setter logic, so more properties are marked as preserve unknown

• all sundrio has been removed including the apt module

  • the crd-generator/test tests have not yet been rewired to work. In the interim I could just manually perform generation.

Next steps:

• create a formal development branch? We'll probably want the flurry of work that is being done to target both 6.x and the next state.
• maven and gradle plugins
• PR to retarget the quarkus logic once we can allow for the objectmapper to be passed in

Related:

• change the default time related serialization to a string, rather than number / array

I think here is a better place for further discussing it, so I created this topic.

[baloo42]

I like that we go forward with a maven/gradle plugin and Jackson. It should be much easier to create tests as we don't need something like googles compile-testing lib as I suggested in #5896.
But I think some approval tests would be nice. So the first thing I can offer is to extract the approval tests from the above PR and create a new PR with v6 as target and only approval tests. Those approval tests could be reused in v7 and compared with v6 so that we can easily identify differences.

If the maintainers are fine with this strategy, please assign the corresponding issue to me: #5850

[shawkins]

Seems good to me. cc @manusa @andreaTP

[andreaTP]

Good for me 👍

[manusa]

Totally, starting with some tests that ensure the changes won't break anything is a must. Thanks for taking care of this 🙌

[shawkins]

Some background - it's been recognized for some time that there is some mismatch and overlap with Jackson functionality with the crd generator - #5600 #5324 (comment) and it's something that we've talked about informally well before these issues / comments.

With the amount of activity hitting the crd generator it seemed wise to revisit this so that we are building on as solid of a foundation as possible.

After seeing that a POC level of this code was working without too major of an effort, I decided to take it to the logical conclusion of fully removing the usage of sundrio - which the branch now is representative of.

Even further thoughts, not in the comment above:

• pushing changes even further upstream that would allow for less code in our crd generator. Jackson providing facilities to get the class / member for a given part of the schema, adding support for setting the pattern on the schema from the JsonFormat annotation, support for jakarta validation classes, etc.
• removing the v1beta1 support - that doesn't seem needed if we are being forward looking. We can still leave in the abstraction for creating the schema though.
• support for union schemas - currently it will just throw an exception.

[baloo42]

removing the v1beta1 support - that doesn't seem needed if we are being forward looking. We can still leave in the abstraction for creating the schema though.

👍

pushing changes even further upstream that would allow for less code in our crd generator. Jackson providing facilities to get the class / member for a given part of the schema, adding support for setting the pattern on the schema from the JsonFormat annotation, support for jakarta validation classes, etc.

I'm fine in general with this, but I want to dig a little bit deeper.

Jacksons JSON-Schema module supports JSON-Schema v3 (and another module JSON-Schema v4).

The Schema for Kubernetes is based on OpenAPI 3.0 which is both a subset and a superset of JSON-Schema Draft 5. But as Kubernetes adds and removes some keywords it's not 100% comparable.

The question is: Is Jackson's JSON-Schema generator enough?

In a previous project 2 years ago (not Kubernetes related) I had some problems with it and replaced it by victools/jsonschema-generator. It's more customizable out of the box and supports also jakarta.validation and jackson annotations.

Docs: https://victools.github.io/jsonschema-generator/

Did you consider this project?

[shawkins]

The question is: Is Jackson's JSON-Schema generator enough?

My short answer is that I believe so. Staying close to jackson, since we're already so heavily reliant, seems like we'll have fewer surprises. Our custom annotation handling will pose a significant integration hurdle with whatever framework we use - and the way the logic is now structured we'll use our own reflection to compensate for any short-comings in jackson. Based upon what we currently support there's not really many pain points in the development branch.

Did you consider this project?

No. We did briefly look at some others offering alternatives or enhancements to jackson schema generation, but ultimately it doesn't seem to rise to level of additional functionality that would warrant an entirely new dependency, or at least something outside of what is already provided via jackson.

[baloo42]

Ok then let's go on with Jackson's Schema Generator.

victools/jsonschema-generator is also based on Jackson but implements it's own logic on inspecting Java classes. So the first step is anyways to get rid of sundrio and move closer to Jackson.

[baloo42]

Some Feedback on this comment in the code regarding validation rules:

kubernetes-client/crd-generator/api/src/main/java/io/fabric8/crd/generator/AbstractJsonSchema.java

Line 165 in 039b0c6

// TODO: the old logic allowed for picking up the annotation from both the getter and the field

// TODO: the old logic allowed for picking up the annotation from both the getter and the field
// this requires a messy hack by convention because there doesn't seem to be a way to all annotations
// nor does jackson provide the field

This implementation came from me and I personally don't need that validation rule annotations from the getter and the field get merged together. With the old implementation it was just very easy to implement and if looking at other projects they supported it as well. I think we should define here a general strategy either supporting it or not.

Suggestion:

If a field and it's getter is annotated with the same non-repeatable annotation, the annotaion from the getter wins.
If a field and it's getter is annotated with the same repeatable annotation, all annotations are used.

[shawkins]

If a field and it's getter is annotated with the same non-repeatable annotation, the annotaion from the getter wins.

This is currently how the getAnnotation works for a BeanProperty.

If a field and it's getter is annotated with the same repeatable annotation, all annotations are used.

This is what this logic is emulating. The ValidationRule should be the only one where this is a concern.

[baloo42]

You are right, ValidationRule should be the only one where this is the case.

[shawkins]

I've aligned the pr to main in preparation for creating the fork instead. This brings up another implementation difference.

@matteriben jackson does not actually change the serialization type when using a non-string shape with a string property. I've updated the JsonSchemaTest test to expect strings, just so it will pass, but wanted to double check on the expectation - do you expect jackson to actually serialize as the shape type? And if so when deserializing, is the expectation also to rely on jackon's implicit type conversion / widening?

[matteriben]

@shawkins

Edit: I found your latest branch here and it looks good to me. Thank you! Original response below: https://github.com/shawkins/kubernetes-client/tree/crd-generator-v7-development

I'm not sure I understand, also I'm not sure where I can see your latest changes.

jackson does not actually change the serialization type when using a non-string shape with a string property.

I think you mean these test cases I added?

  @JsonFormat(shape = JsonFormat.Shape.BOOLEAN)
  public String getBool() {
    return bool;
  }

  @JsonFormat(shape = JsonFormat.Shape.NUMBER)
  public String getNum() {
    return num;
  }

  @JsonFormat(shape = JsonFormat.Shape.NUMBER_FLOAT)
  public String getNumFloat() {
    return numFloat;
  }

  @JsonFormat(shape = JsonFormat.Shape.NUMBER_INT)
  public String getNumInt() {
    return numInt;
  }

    assertEquals(type.apply("boolean").build(), spec.get("bool"));
    assertEquals(type.apply("number").build(), spec.get("num"));
    assertEquals(type.apply("number").build(), spec.get("numFloat"));
    assertEquals(type.apply("integer").build(), spec.get("numInt"));

I've updated the JsonSchemaTest test to expect strings, just so it will pass, but wanted to double check on the expectation - do you expect jackson to actually serialize as the shape type?

I don't actually have use cases that depends on the test examples above (string property with non-string shape). I was unaware that Jackson ignores the shape for a string property.

The case I have a use for is when shape is string and the property is non-string.

  @JsonFormat(shape = JsonFormat.Shape.STRING, pattern = "yyyy-MM-dd'T'HH:mm:ssVV")
  public java.time.ZonedDateTime getIssuedAt() {
    return issuedAt;
  }

assertEquals(type.apply("string").build(), spec.get("issuedAt"));

[andreaTP]

Thanks a lot for this effort!
It's shaping up pretty well!

I encourage you to start comparing with a corpus of CRDSs currently generated (maybe we can get some of those from projects using JOSDK).
Another great example that comes to mind is the Keycloak Realm Import CRD, it's already included somewhere and using approval tests on it can give us a good indicator of what's going on.

[shawkins]

@andreaTP Comparing the keycloak realm import I see that old generator includes two fields that are not actually serialized. This is due to the old logic primarily considering fields. I believe this will be okay with the olm upgrade process because the properties will not be in use - but will have to double check.