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

CRD generation does not support defaults, enums are incorrect #5324

Closed
ggallen opened this issue Jul 10, 2023 · 10 comments
Closed

CRD generation does not support defaults, enums are incorrect #5324

ggallen opened this issue Jul 10, 2023 · 10 comments
Labels
component/crd-generator Related to the CRD generator status/stale

Comments

@ggallen
Copy link

ggallen commented Jul 10, 2023

Describe the bug

Using the CRD generator, defaults are not handled at all and enumerations are handled incorrectly.

Fabric8 Kubernetes Client version

SNAPSHOT

Steps to reproduce

See this example class:

package com.redhat.agogos.core;

import com.fasterxml.jackson.annotation.JsonInclude;
import com.fasterxml.jackson.databind.annotation.JsonSerialize;
import com.fasterxml.jackson.databind.ser.std.ToStringSerializer;
import io.fabric8.kubernetes.model.annotation.Group;
import io.fabric8.kubernetes.model.annotation.Kind;
import io.fabric8.kubernetes.model.annotation.Version;
import io.quarkus.runtime.annotations.RegisterForReflection;
import lombok.ToString;

@ToString
@JsonInclude(JsonInclude.Include.NON_NULL)
@RegisterForReflection
@Kind("Example")
@Group("agogos.redhat.com")
@Version("v1alpha1")
public class Example {

    @JsonSerialize(using = ToStringSerializer.class)
    public enum ResourceStatus {
        NEW("New"),
        INITIALIZING("Initializing"),
        READY("Ready"),
        FAILED("Failed");

        private final String printable;

        private ResourceStatus(String printable) {
            this.printable = printable;
        }

        @Override
        public String toString() {
            return printable;
        }
    }

    public ResourceStatus status = ResourceStatus.NEW;
}

This generates the following CRD:

# Generated by Fabric8 CRDGenerator, manual edits might get overwritten!
apiVersion: apiextensions.k8s.io/v1beta1
kind: CustomResourceDefinition
metadata:
  name: examples.agogos.redhat.com
spec:
  group: agogos.redhat.com
  names:
    kind: Example
    plural: examples
    singular: example
  scope: Cluster
  validation:
    openAPIV3Schema:
      properties:
        status:
          enum:
          - NEW
          - INITIALIZING
          - READY
          - FAILED
          type: string
      type: object
  versions:
  - name: v1alpha1
    served: true
    storage: true

There are two issues here:

  1. There is no default value for status.
  2. The enumerations are not using the toString() representations.

Expected behavior

The CRD should look like this:

# Generated by Fabric8 CRDGenerator, manual edits might get overwritten!
apiVersion: apiextensions.k8s.io/v1beta1
kind: CustomResourceDefinition
metadata:
  name: examples.agogos.redhat.com
spec:
  group: agogos.redhat.com
  names:
    kind: Example
    plural: examples
    singular: example
  scope: Cluster
  validation:
    openAPIV3Schema:
      properties:
        status:
          default: New
          enum:
          - New
          - Initializing
          - Ready
          - Failed
          type: string
      type: object
  versions:
  - name: v1alpha1
    served: true
    storage: true

Runtime

minikube

Kubernetes API Server version

1.25.3@latest

Environment

Linux

Fabric8 Kubernetes Client Logs

No response

Additional context

No response

@shawkins
Copy link
Contributor

The enumerations are not using the toString() representations.

The crd generator is only aware of select jackson annotations. It's mostly based upon sundrio, not jackson. In this case I think the generator is aware of JsonProperty, but not JsonSerialize.

Using something like https://github.com/FasterXML/jackson-module-jsonSchema has been talked about briefly, but that is not suitable annoation processing logic - the whole crd generator would need to be re-designed around running as a separate build step so it could use the compiled classes.

@metacosm
Copy link
Collaborator

We could make a special case for this particular use case, which seems to be rather common, though. Handling the default value probably would be more tricky since, at least in this case, it would mean understanding the code that the field is assigned this value by default, which is not how the generator works at all (it only looks at types and annotations, not the code itself).

@manusa manusa added the component/crd-generator Related to the CRD generator label Jul 11, 2023
@andreaTP
Copy link
Member

The analysis made by @shawkins makes total sense to me.

Regarding the two issues discussed here:

  • Supporting JsonSerialize seems to be a pretty massive effort given all the knobs
  • Supporting the default can be challenging too since we are using sundrio, (might be easier in a Quarkus BuildStep or with a plain AnnotationProcessor infrastructure)

@shawkins
Copy link
Contributor

Using something like https://github.com/FasterXML/jackson-module-jsonSchema has been talked about briefly

Poking around there also seems to be quite a few challenges. It also does not understand things like custom serialization via JsonSerialize either - with that annotation, it gives up and emits:

anEnum:
    type: "string"

It supports a different set of constraint annotations, jakarta.validation.constraints.*
It does not support marking an enum constant as ignored.
Support for SchemaFrom/Swap would need to be completely re-thought.
It produces a v3 json schema that would need to be translated into the expected openapi dialect.
It is driven off of bean properties, not fields (this is preferable to what sundrio is doing, but it's yet another thing to align)
It doesn't seem to support defaults either - on the field or via the JsonProperty default argument.

And I'm sure there's a lot more to be found after digging into it.

Perhaps we should do something similar when we see jsonserialize - effectively widen, rather than creating a potentially incorrect schema. Or alternatively warn.

@metacosm
Copy link
Collaborator

I poked around a little bit as well, I was thinking we could instantiate the JsonSerializer referenced by the annotation and call it when outputting the value but even in the case of a simple ToStringSerializer this would mean being able to instantiate the property itself and then provide the appropriate parameters to the serialize method. So this isn't indeed trivially solved, even if we were to focus on this very specific use case.

@shawkins
Copy link
Contributor

I brought up in our meeting that it may be possible to bridge between jackson and sundrio. However that does seem like a very difficult task - the JavaType logic is just one part of it, all of the com.fasterxml.jackson.databind.introspect would need abstracted to work with sundrio and you'd have to at the very least convert to Annotation instances. I'd say the initial assessment stands the jackson logic could only run after the the classes were compiled.

@stale
Copy link

stale bot commented Oct 11, 2023

This issue has been automatically marked as stale because it has not had any activity since 90 days. It will be closed if no further activity occurs within 7 days. Thank you for your contributions!

@stale stale bot added the status/stale label Oct 11, 2023
@andreaTP
Copy link
Member

This issue is completely solved as of today:

  • enumerations are now cleaned up
  • default support landed

Please, re-open if you find any additional bugs or edge cases!

@bbende
Copy link

bbende commented Oct 11, 2024

@andreaTP I found this issue while searching to see if there was a way to get a generated CRD to use toString() for the enum values, rather than name(). You mentioned "enumerations are now cleaned up", does that mean there is a way to specify to use toString(), or is there an annotation that can be applied to do this? My enum currently uses @JsonValue on a getter to specify that value to serialize, plus it also provides toString() using the same value.

@andreaTP
Copy link
Member

@bbende do you mind opening a new issue?

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 status/stale
Projects
None yet
Development

No branches or pull requests

6 participants