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

Wrong python code generated for arbitrary string -> object mapping fields #163

Open
bernhardloos opened this issue Nov 26, 2024 · 1 comment
Labels
area/codegen Affects quality or correctness of generated code kind/bug Some behavior is incorrect or out of spec

Comments

@bernhardloos
Copy link

What happened?

The generated python code is syntactically correct but doesn't match the CRD and the resource is rejected by the Kubernetes API server.
The problem happens in this CRD https://github.com/red-hat-storage/ocs-operator/blob/release-4.16/deploy/ocs-operator/manifests/storagecluster.crd.yaml#L3895 for the spec.resources element starting in line 3895.

Example

The yaml for the resource element:

              resources:
                additionalProperties:
                  description: ResourceRequirements describes the compute resource
                    requirements.
                  properties:
                    claims:
                      description: "Claims lists the names of resources, defined in
                        spec.resourceClaims, that are used by this container. \n This
                        is an alpha field and requires enabling the DynamicResourceAllocation
                        feature gate. \n This field is immutable. It can only be set
                        for containers."
                      items:
                        description: ResourceClaim references one entry in PodSpec.ResourceClaims.
                        properties:
                          name:
                            description: Name must match the name of one entry in
                              pod.spec.resourceClaims of the Pod where this field
                              is used. It makes that resource available inside a container.
                            type: string
                        required:
                        - name
                        type: object
                      type: array
                      x-kubernetes-list-map-keys:
                      - name
                      x-kubernetes-list-type: map
                    limits:
                      additionalProperties:
                        anyOf:
                        - type: integer
                        - type: string
                        pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$
                        x-kubernetes-int-or-string: true
                      description: 'Limits describes the maximum amount of compute
                        resources allowed. More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/'
                      type: object
                    requests:
                      additionalProperties:
                        anyOf:
                        - type: integer
                        - type: string
                        pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$
                        x-kubernetes-int-or-string: true
                      description: 'Requests describes the minimum amount of compute
                        resources required. If Requests is omitted for a container,
                        it defaults to Limits if that is explicitly specified, otherwise
                        to an implementation-defined value. Requests cannot exceed
                        Limits. More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/'
                      type: object
                  type: object
                description: Resources follows the conventions of and is mapped to
                  CephCluster.Spec.Resources
                type: object

Generated python for this element:
resources: Optional[pulumi.Input[Mapping[str, pulumi.Input[Mapping[str, pulumi.Input[str]]]]]] = None,
This should be something like
resources: Optional[pulumi.Input[Mapping[str, pulumi.Input[Mapping[str, pulumi.Input[Mapping[str, pulumi.Input[str]]]]]]]] = None,
or even better:
resources: Optional[pulumi.Input[Mapping[str, pulumi.Input[StorageClusterSpecResourcesArgs]]]] = None,

Output of pulumi about

crd2pulumi version v1.5.4

Additional context

I'm pretty sure this worked in earlier versions of crd2pulumi.

Contributing

Vote on this issue by adding a 👍 reaction.
To contribute a fix for this issue, leave a comment (and link to your pull request, if you've opened one already).

@bernhardloos bernhardloos added kind/bug Some behavior is incorrect or out of spec needs-triage Needs attention from the triage team labels Nov 26, 2024
@blampe
Copy link
Contributor

blampe commented Dec 4, 2024

I'm guessing this is due to https://github.com/pulumi/pulumi-kubernetes/blob/e065015152324f4359f8a4d4332618be6019dd1c/provider/pkg/gen/typegen.go#L339-L380.

pulumi/pulumi-kubernetes#3347 tracks the issue on p-k.

We recurse or terminate as long as type is present, with recursion happening for arrays and additionalProperties . (Interestingly we also only consider things like x-kubernetes-int-or-string when type is absent.)

So (I'm guessing) it goes resourcesadditionalProperties → done, and that's where the two generated mappings come from.

There are other resources in this CRD which look like the below -- note properties vs. additionalProperties:

resources:
  description:
    "resources represents the minimum resources
    the volume should have. If RecoverVolumeExpansionFailure
    feature is enabled users are allowed to specify resource
    requirements that are lower than previous value but
    must still be higher than capacity recorded in the
    status field of the claim. More info: https://kubernetes.io/docs/concepts/storage/persistent-volumes#resources"
  properties:
    limits:
      additionalProperties:
        anyOf:
          - type: integer
          - type: string
        pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$
        x-kubernetes-int-or-string: true
      description:
        "Limits describes the maximum amount
        of compute resources allowed. More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/"
      type: object
    requests:
      additionalProperties:
        anyOf:
          - type: integer
          - type: string
        pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$
        x-kubernetes-int-or-string: true
      description:
        "Requests describes the minimum amount
        of compute resources required. If Requests is
        omitted for a container, it defaults to Limits
        if that is explicitly specified, otherwise to
        an implementation-defined value. Requests cannot
        exceed Limits. More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/"
      type: object
  type: object

These also generate more reasonable code, e.g.

resources: Optional[pulumi.Input['StorageClusterSpecManagedResourcesCephNonResilientPoolsResourcesArgs']] = None,

@blampe blampe added area/codegen Affects quality or correctness of generated code and removed needs-triage Needs attention from the triage team labels Dec 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/codegen Affects quality or correctness of generated code kind/bug Some behavior is incorrect or out of spec
Projects
None yet
Development

No branches or pull requests

2 participants