kubernetes-sigs / controller-tools

Tools to use with the controller-runtime libraries
Apache License 2.0
734 stars 419 forks source link

controller-gen: Type aliases to array types don't work with +listType/+listMapKey markers #988

Open ahmetb opened 5 months ago

ahmetb commented 5 months ago

Summary

If Go type of a CRD has a field that uses +listType or +listMapKey markers, but the field (struct member) is a typedef to a slice type, the field is not detected as an "array" field, and these markers cannot be applied to it.

Example

// +k8s:deepcopy-gen=true
type Conditions []Condition

type FooStatus struct{
    // +patchMergeKey=type
    // +patchStrategy=merge
    // +listType=map
    // +listMapKey=type
    Conditions Conditions `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type"`
}

Fails with error:

controller-gen crd paths=./api/... [...]

[...]/status_types.go:35:2: must apply listType to an array, found
[...]/status_types.go:35:2: must apply listMapKey to an array, found

Details

This reproes in HEAD (as well as v0.14, v0.15).

If we do not use the declaration for the slice type (type Conditions []Condition) in the root object and instead use []Conditions as the struct member type, it works as expected.

I think this is happening probably because the tool uses AST parsing, and Conditions struct member doesn't appear like an array in this snippet, it calls structToSchema instead of arrayToSchema.

/kind bug

JoelSpeed commented 5 months ago

Have you tried applying the list type directly to the type declaration for Conditions instead of the field itself?

ahmetb commented 5 months ago

@JoelSpeed that seems to work (thanks!), albeit a bit awkward:

// +k8s:deepcopy-gen=true
// +listType=map
// +listMapKey=type
type Conditions []Condition

type FooStatus struct{
    Conditions Conditions `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type"`
}

It correctly generates:

#...
              conditions:
                type: array
                x-kubernetes-list-map-keys:
                - type
                x-kubernetes-list-type: map

In my opinion, let's keep the bug open. It seems like it would be possible to evaluate the parsed AST to resolve the specified type as a typedef (ast.Ident) and realize the underlying type is of an array type.

k8s-triage-robot commented 2 months ago

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

You can:

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

sbueringer commented 2 months ago

/remove-lifecycle stale /lifecycle frozen