controller-tools icon indicating copy to clipboard operation
controller-tools copied to clipboard

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

Open ahmetb opened this issue 1 year ago • 7 comments

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

ahmetb avatar Jun 12 '24 04:06 ahmetb

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

JoelSpeed avatar Jun 12 '24 08:06 JoelSpeed

@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.

ahmetb avatar Jun 12 '24 16:06 ahmetb

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

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

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue as fresh with /remove-lifecycle stale
  • Close this issue with /close
  • Offer to help out with Issue Triage

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

/lifecycle stale

k8s-triage-robot avatar Sep 10 '24 16:09 k8s-triage-robot

/remove-lifecycle stale /lifecycle frozen

sbueringer avatar Sep 10 '24 17:09 sbueringer

This is general problem - I'm noticing even a type aliases for int types break different markers

eg. the min/max markers check the type to be an integer.

https://github.com/kubernetes-sigs/controller-tools/blob/23dd517d714eb79eb9fac3d51485b09189e3f742/pkg/crd/markers/validation.go#L329

When in fact the json schema is a reference to another json schema (which is of type int).

In GatewayAPI I cloned the markers and removed these checks.

eg. https://github.com/kubernetes-sigs/gateway-api/pull/3750/files#diff-60aa98d089853570c7377202b3ee936b1abae4a770d2465dedcfa268eef2f3c1R31

dprotaso avatar Apr 17 '25 14:04 dprotaso

So these markers need to recurse these references or maybe they need to be flattened?

dprotaso avatar Apr 17 '25 14:04 dprotaso

I expect we need to follow the aliases to find out the underlying type and apply the checks that we are applying to the underlying type once we hit a valid openapi type

JoelSpeed avatar Apr 28 '25 16:04 JoelSpeed