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

client-gen generates empty "core" group clientsets when "api" package directory is in use for input APIs #167

Open
shaneutt opened this issue Feb 14, 2024 · 10 comments

Comments

@shaneutt
Copy link
Member

Last year we added a v4 project layout in Kubebuilder which utilizes api/ as the directory for APIs to better follow the package naming standards.

I have some kubebuilder-based projects where I am migrating from the v3 layout to the v4 layout. As part of this transition I'm moving my APIs from the apis/ directory to the api/ directory.

Using the latest main (da01854 at the time of writing) the clientset generators break, emitting empty files to a core directory. It seems using api trips some special rules in client-gen.

I dug into the code and did some dlv debugging on this and found an assumption in the code:

func (g Group) NonEmpty() string {

func (g Group) NonEmpty() string {
	if g == "api" {
		return "core"
	}
	return string(g)
}

I understanding why that assumption exists for upstream, but given that Kubebuilder layout v4 now defaults to an api/ directory it seems like this might be problematic for downstream users.

During my debugging I was able to hack around this a bit to get something that emitted a working client for me, but I wanted to stop and mention this before trying to submit a patch to see if anyone had any thoughts on the matter, or some keen insights?

@delthas
Copy link

delthas commented Feb 20, 2024

Hi, I have the exact same issue with a default kubebuilder scaffolding. Actually, I was about to send this issue when I saw yours:


Hi,

The default kubebuilder scaffolding generated by:

kubebuilder init --domain example.com
kubebuilder create api --group group --version v1 --kind Foo

results in the following files:

./api/v1/foo_types.go
./api/v1/groupversion_info.go

(with // +groupName=group.example.com as expected.)

In order to generate a typed client for this API, I added //+genclient to ./api/v1/foo_types.go. However, running client-gen results in no output and an error:

client-gen -n client --input api/v1 --input-base example.com/project -o . -p example.com/project --trim-path-prefix example.com/project -v 10
I0220 13:12:11.969569  203278 parse.go:383] importPackage example.com/project/core/v1
I0220 13:12:11.969622  203278 parse.go:330] addDir example.com/project/core/v1
I0220 13:12:11.982155  203278 parse.go:404] unable to import "example.com/project/core/v1": no required module provides package example.com/project/core/v1; to add it:
	go get example.com/project/core/v1

Indeed, example.com/project/core/v1 does not exist; it should really be example.com/project/api/v1 (corresponding to what I passed client-gen); but apparently api is rewritten to core, then client-gen tries to look in the folder core, which does not exist.

The following workaround works, and a typed client for my resource is correctly generated:

ln -s api core
client-gen -n client --input api/v1 --input-base example.com/project -o . -p example.com/project --trim-path-prefix example.com/project

It seems this is due to this line, which rewrites api to core: https://github.com/kubernetes/code-generator/blob/master/cmd/client-gen/types/types.go#L45

@shaneutt
Copy link
Member Author

Thanks for adding on, I have a feeling this is probably a common problem for anyone using layout v4.

@camilamacedo86 I just wanted to ping you in here NOT because there's anything you need to do, but because I was working with you last in kubebuilder on the layout stuff a while back it seemed reasonable to assume you might wanna be aware of this problem.

@camilamacedo86
Copy link

camilamacedo86 commented Feb 20, 2024

Hi @shaneutt

Thank you for letting me know. IHMO is a bug/something that needs to be changed in the code generator.
Any project that tries to follow the Golang standards will not work within since it seems very logical to have a path called api to store the APIs, right? Please let me know if I missed something.

Kubebuilder officially supports the controller-gen since that is what we use by default, but it would be great if the code-generator could change that and work well with Kubebuilder. I know that some users have started to use it within.

@camilamacedo86
Copy link

camilamacedo86 commented Feb 22, 2024

Hi @shaneutt,

OutOfScope of this issue

Can a project work with both code-generator and controller-gen?
If so, we could study the benefit to scaffold by default both OR give the option for users opt-in or opt-out.
WDYT?

Could you give a hand for us and raise an issue in the Kubebuilder like Add official support to code-generator, add what would need to do in the scaffold be done for we are able to integrate the project within and if both can be applied at the same time or if we should have an option to allow users opt-in for one or another as the cons/pros of each one of them?

@shaneutt
Copy link
Member Author

Sure sounds good, created: kubernetes-sigs/kubebuilder#3795

@tankbusta
Copy link

Hey folks, hope this is the right spot (first kubernetes issue for me) but I think there's another related issue to this. I'm using the V4 layout and my generated code is using the wrong API Path in the typed version client

From code-generator

func setConfigDefaults(config *rest.Config) error {
	gv := v1alpha1.SchemeGroupVersion
	config.GroupVersion = &gv
	config.APIPath = "/api"
	config.NegotiatedSerializer = scheme.Codecs.WithoutConversion()

	if config.UserAgent == "" {
		config.UserAgent = rest.DefaultKubernetesUserAgent()
	}

	return nil
}

From my understanding, /api is "legacy" whereas /apis is for newer resource types and CRDs. In the code that generates this, I noticed similar logic

apiPath := func(group string) string {
if group == "core" {
return `"/api"`
}
return `"` + g.apiPath + `"`
}

@spacewander
Copy link

If client-go needs to keep the speicial conversion, what about adding a note in the argument flag?

It's time-wasting for the user to figure it out by reading the implementation code.

spacewander added a commit to mosn/htnn that referenced this issue Mar 22, 2024
Also have to rename api/ to apis/, so that client-gen can work.
See kubernetes/code-generator#167

The gateway-api also use apis/ as the directory to hold the CRD types,
so I think this name is roburst enough in the k8s ecosystem.
Signed-off-by: spacewander <spacewanderlzx@gmail.com>
spacewander added a commit to mosn/htnn that referenced this issue Mar 25, 2024
Also have to rename api/ to apis/, so that client-gen can work. See
kubernetes/code-generator#167

The gateway-api also use apis/ as the directory to hold the CRD types,
so I think this name is roburst enough in the k8s ecosystem.
Signed-off-by: spacewander <spacewanderlzx@gmail.com>

---------

Signed-off-by: spacewander <spacewanderlzx@gmail.com>
@camilamacedo86
Copy link

@spacewander,

So, is not this one a bug?
Can someone add an example over how to integrate this project with Kubebuilder in order to not face the issue reported here?

Thank you a lot

@spacewander
Copy link

@spacewander,

So, is not this one a bug? Can someone add an example over how to integrate this project with Kubebuilder in order to not face the issue reported here?

Thank you a lot

I don't know why the author of code-generator added this conversion. Maybe they want to implement some compatibility. If removing this behavior is not possible, the other solution is to add a new argument to bypass this behavior.

@camilamacedo86
Copy link

camilamacedo86 commented Apr 8, 2024

Hi @wojtek-t, @sttts, @deads2k

Would be possible to change the conditional to allow users use it with Kubebuilder OR with any Golang project following the Golang Standard Layout (equally Kubebuilder) ?

See https://github.com/golang-standards/project-layout?tab=readme-ov-file#service-application-directories (it is a good practice create our APIs and keep them in dir called api)

apiPath := func(group string) string {
if group == "core" {
return `"/api"`
}
return `"` + g.apiPath + `"`
}

In Kubebuilder we have mapped the core types in this way:

https://github.com/kubernetes-sigs/kubebuilder/blob/32e0fdc918b9a21485d7b042833402a62d0480f4/pkg/plugins/golang/options.go#L29-L52

I hope that can helps. Sorry I am not acquaintance well with this project. But could we not add this map in this part and check by the kind/group of each of them to know if is a core type or not?

Thank you for your support and understanding.

Cheers,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants