Welcome @knight42!

It looks like this is your first PR to kubernetes-sigs/controller-tools 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/controller-tools has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

/assign @pwittrock

any progress? or is there a way to workaround this situation

@yuchengwu just declare a new type for the pointer type.

@knight42 thanks, it works!

Does this version release in v0.2.2? I have the same issue when using controller-gen.

@knight42 PR has to fix conflicts.

Would be great to merge it, for dependent tools : operator-framework/operator-sdk#2485

rebased

/assign @DirectXMan12

This is not a fix for the listed issue, and needs more elaboration in the description

@DirectXMan12 I wonder why this is not a fix? I think the problem of the listed issue is that for the time being controller-gen does not accept pointers as map values and IMHO my patch should fix this problem as shown below.

$ tree .
.
├── api.go
├── go.mod
└── go.sum

0 directories, 3 files

$ cat api.go
package crd

import (
	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

// +kubebuilder:object:root=true
// +kubebuilder:object:generate=true
// +groupName=example.org

type Env struct {
	A int    `json:"a"`
	B string `json:"b"`
}

type App struct {
	metav1.TypeMeta   `json:",inline"`
	metav1.ObjectMeta `json:"metadata,omitempty"`

	Envs map[string]*Env `json:"envs,omitempty"`
}

using the latest controller-gen(9d85d10)

$ /tmp/controller-gen-9d85d107d crd paths=./... output:crd:stdout

// invalid crd omitted

/private/tmp/crd/api.go:20:18: map values must be a named type, not *ast.StarExpr
Error: not all generators ran successfully
run `controller-gen crd paths=./... output:crd:stdout -w` to see all available markers, or `controller-gen crd paths=./... output:crd:stdout -h` for usage

my patch:

$ /tmp/controller-gen-fix crd paths=./... output:crd:stdout

---
apiVersion: apiextensions.k8s.io/v1beta1
kind: CustomResourceDefinition
metadata:
  annotations:
    controller-gen.kubebuilder.io/version: (devel)
  creationTimestamp: null
  name: apps.example.org
spec:
  group: example.org
  names:
    kind: App
    listKind: AppList
    plural: apps
    singular: app
  scope: Namespaced
  validation:
    openAPIV3Schema:
      properties:
        apiVersion:
          description: 'APIVersion defines the versioned schema of this representation
            of an object. Servers should convert recognized schemas to the latest
            internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources'
          type: string
        envs:
          additionalProperties:
            properties:
              a:
                type: integer
              b:
                type: string
            required:
            - a
            - b
            type: object
          type: object
        kind:
          description: 'Kind is a string value representing the REST resource this
            object represents. Servers may infer this from the endpoint the client
            submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds'
          type: string
        metadata:
          type: object
      type: object
  version: crd
  versions:
  - name: crd
    served: true
    storage: true
status:
  acceptedNames:
    kind: ""
    plural: ""
  conditions: []
  storedVersions: []

The listed issue is that controller-gen is trying to generate a CRD definition for a type not used in the CRD. It's breaking because it's not types we accept in a CRD, but the actually issue is that the user didn't intend for that type to be parsed

@DirectXMan12 This needs to be fixed regardless, right? This is a case I am hitting too where I refer to a pointer to a struct defined in the CRD in a map and controller-gen complains. Much like the example in #317 (comment).

Or is there a strict guideline that map values cannot be pointers?

@DirectXMan12 well... you're right, I have misunderstood, sorry about that. I've removed the "Fix ..." line in the description, but I think that pointers should be accepted as map values nevertheless.

Hey guys, any news regarding that PR ? we had to fork operator-sdk in order to use that PR https://github.com/Orange-OpenSource/casskop/blame/master/documentation/development.md#L62

Or is there a strict guideline that map values cannot be pointers?

In generally, you're pretty much never supposed to use maps in k8s. When you do, it's pretty much exclusively map[string-ish]string-ish. There's a couple of exceptions for legacy cases, but in general, if you're using a pointer, it's an indication that you might be doing something against k8s practices.

More specifically, map[string-ish]struct-ish is canonically not supposed to be a thing.

I read in the API conventions that maps are not preferred. But then there is kubernetes/kubernetes#8190 (comment) :)

Anyway, we have a bunch of code, based on operator-sdk (which didn't have checks for maps) which we are moving to kubebuilder v2, that uses maps (unfortunately) which we cannot change right away (without bumping apiversion and writing conversion). So we are not able to use controller-tools to generate a CRD :(

the key there is "non-standard keys", I believe. See https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#lists-of-named-subobjects-preferred-over-maps

We are also interested in this. While we don't use maps in the spec section of our CRDs, we do use them extensively in our status fields.

The biggest benefit for us would be that this would significantly clean up our duplicated types. We already work around the map issue by re-defining our pointer types, so whether or not this gets accepted, we will still use maps. I think the biggest benefit of this PR is that it eliminates the need of an (ugly) workaround.

We don't use it in spec too, just in status. So the reason for "list are preferred over maps" that it's hard to distinguish between magic keywords and just strings (as described in kubernetes/kubernetes#2004 (comment)) is not really applicable I think. But yeah, we can workaround it for now as this is presumably a bigger API decision not to support maps.

@seglberg Is the workaround just to declare a new type for the pointer to old type like type PtrFoo *Foo? I tried that, but deepcopy seems to have issues when you do that and causes build failures.

api/v1beta1/zz_generated.deepcopy.go:285:10: (*in).DeepCopyInto undefined (type PtrNodeStatus has no field or method DeepCopyInto)

@rajathagasthya Did you run make generate?

@knight42 Yep, I don't see deepcopy funcs generated for the new type.

@rajathagasthya same for me. I can not see deepcopy generated for new type as well.

Any updates on this issue? Is the change acceptable? @DirectXMan12 @knight42

I am using kubebuilder2 and also have this issue. The worst thing is I am using some common packages in my API definition, the dependency has map pointer usage as well. Without the change, I can not move forward..

any status on this? I too am using kubebuilderv2 and have bumped into this issue

@ifilonenko: changing LGTM is restricted to collaborators

At this point, I've gotten enough feedback that this is seriously blocking folks that I'm gonna reconsider and approve this. I want to follow up with a "warn about this thing" PR, though, since we still don't recommend this, but there's enough cases (esp around older CRs, embedded types from other systems, etc) that I think I was a bit too firm on blocking this earlier. Apologies for the delays/troubles I've caused with holding this up.

/approve

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DirectXMan12, knight42

The full list of commands accepted by this bot can be found here.

The pull request process is described here

/lgtm

Could you release a new version contains this fix？ @DirectXMan12 @ifilonenko

@DirectXMan12 when can we expect a new version like asked 👆 ?