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

✨ Make the anonymous field of the groupName marker optional #1000

Merged
merged 1 commit into from
Jul 16, 2024

Conversation

twz123
Copy link
Contributor

@twz123 twz123 commented Jul 4, 2024

Upstream introduced the use of empty groupName markers in kubernetes/kubernetes#125162. This breaks groupName parsing because its field is non-optional. Add an additional mustOptional function that takes anonymous field definitions and turns them into optional ones.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 4, 2024
@k8s-ci-robot
Copy link
Contributor

Welcome @twz123!

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

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 4, 2024
@k8s-ci-robot
Copy link
Contributor

Hi @twz123. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jul 4, 2024
@twz123 twz123 changed the title ✨ Make the anonymous value of the groupName marker optional ✨ Make the anonymous field of the groupName marker optional Jul 4, 2024
@sbueringer
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 11, 2024
@sbueringer
Copy link
Member

@twz123 Implementation looks good. Let's please add some test coverage. (I think somewhere below pkg/crd/testdata something that uses groupName=)

Upstream introduced the use of empty groupName markers in k/k PR 125162.
This breaks groupName parsing because its field is non-optional. Add an
additional mustOptional function that takes anonymous field definitions
and turns them into optional ones.

Signed-off-by: Tom Wieczorek <twieczorek@mirantis.com>
@twz123 twz123 force-pushed the optional-groupname branch from 31c93d2 to 7c1db6b Compare July 11, 2024 15:17
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 11, 2024
@twz123
Copy link
Contributor Author

twz123 commented Jul 11, 2024

@twz123 Implementation looks good. Let's please add some test coverage. (I think somewhere below pkg/crd/testdata something that uses groupName=)

Done!

@sbueringer
Copy link
Member

/retest

@@ -22,7 +22,7 @@ import (

func init() {
AllDefinitions = append(AllDefinitions,
must(markers.MakeDefinition("groupName", markers.DescribesPackage, "")).
Copy link
Member

@sbueringer sbueringer Jul 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sttts Does it sound reasonable for you to just adjust the marker here to accept an "empty" groupName marker ("// +groupName=") without any additional handling for the empty string?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Context: Without this PR controller-gen fails like this now (with API types that use types from k8s.io/api@v0.31.0-alpha.3)

/home/prow/go/pkg/mod/k8s.io/api@v0.31.0-alpha.3/core/v1/doc.go:21:1: missing argument "" (at )
/home/prow/go/pkg/mod/k8s.io/api@v0.31.0-alpha.3/core/v1/doc.go:21:1: missing argument "" (at )
https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api/10803/pull-cluster-api-verify-main/1811372226943913984

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

without any additional handling for the empty string?

This is about embedding corev1 types in some CRD, right? So the groupName of the corev1 package does not really play a role for the CRD, does it? So yes, I think just parsing it without falling over, and then ignoring it outside of actual CRD API package sounds right.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is about embedding corev1 types in some CRD, right?

Yes exactly

So the groupName of the corev1 package does not really play a role for the CRD, does it?

This is my understanding, yes

Copy link
Member

@sbueringer sbueringer Jul 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if anyone is doing it, but theoretically folks could use controller-gen to generate CRDs for types of the k/k core API. In that case they would end up with an empty group in the CRD with groupName= (see the test in this PR: pkg/crd/testdata/testdata._cronjobs.yaml)

Not sure which group would be correct in this (probably hypothetical) case. But folks could just specify whatever group they want via groupName, so I assume it's fine

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, thx!

I think then let's do the same in controller-gen for now. We can still modify this behavior later (or offer some additional markers) if people want something else for CRD generation for the core API group

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@twz123 WDYT about handling the empty group here:

Name: defaultPlural + "." + groupKind.Group,
to use "core" for the CRD name suffix if .Group is an empty string?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current name generated in the test case also seems to be invalid:

The CustomResourceDefinition "cronjobs." is invalid:
* metadata.name: Invalid value: "cronjobs.": a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')
* metadata.name: Invalid value: "cronjobs.": must be spec.names.plural+"."+spec.group

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sttts I was trying to deploy a CRD with name & group like this, but I'm getting these errors with a regular kube-apiserver (kindest/node:v1.30.0)

The CustomResourceDefinition "cronjobs.core" is invalid:
* metadata.name: Invalid value: "cronjobs.core": must be spec.names.plural+"."+spec.group
* spec.group: Required value

CRD:

apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
  annotations:
    controller-gen.kubebuilder.io/version: (devel)
  name: cronjobs.core
spec:
  group: ""
  names:
    kind: CronJob
    listKind: CronJobList
    plural: cronjobs
    singular: cronjob
  scope: Namespaced
...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Kube does is not allowed today. You cannot mix native and CRD api groups.

@sbueringer
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 15, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 98d1e914f29072f51bc71936ebe0a015b8994ee9

@sbueringer
Copy link
Member

Let's go ahead with this fix to unblock folks that just want to generate CRD for "non-core" apiGroups. I think this should be 99,9% of controller-gen users. Let's see if we want to do a follow-up based on the discussion here: #1000 (comment)

@sbueringer
Copy link
Member

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer, twz123

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

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 16, 2024
@k8s-ci-robot k8s-ci-robot merged commit b1ff5ff into kubernetes-sigs:master Jul 16, 2024
7 checks passed
@twz123 twz123 deleted the optional-groupname branch July 17, 2024 18:22
2uasimojo added a commit to 2uasimojo/hive that referenced this pull request Sep 30, 2024
NOTE: At this level, k8s.io/apis started using an empty `groupName`
annotation, which broke the version of controller-gen we were using to
generate CRDs. This was fixed via
kubernetes-sigs/controller-tools#1000 which
became available in controller-tools v0.16. We used to get our
controller-gen via build-machinery-go, and were using the current
default version v0.9.2; so until that default is changed upstream (see
openshift/build-machinery-go#93), we have to
bypass build-machinery-go entirely. Hopefully the Makefile changes in
this commit can be reverted once that issue is resolved.

NOTE: This also prompted an upgrade to controller-runtime v0.19.0.

HIVE-2616
2uasimojo added a commit to 2uasimojo/hive that referenced this pull request Oct 1, 2024
At v1.31, k8s.io/apis started using an empty groupName annotation, which
broke the version of controller-gen we were using to generate CRDs. This
was fixed via kubernetes-sigs/controller-tools#1000 which became
available in controller-tools v0.16.

We used to get our controller-gen via build-machinery-go, and were using
the current default version v0.9.2; so until that default is changed
upstream (see openshift/build-machinery-go#93), we have to bypass
build-machinery-go entirely. The Makefile changes in this commit might
be reverted once that issue is resolved.

...or we might polish it up and keep BMG out of the loop for
controller-gen, possibly cutting over to using
https://github.com/openshift/api/tree/master/tools/codegen

Needed by HIVE-2616
Related to HIVE-2626
hjiawei added a commit to hjiawei/calico that referenced this pull request Dec 7, 2024
This change regenerates Calico manifests using the latest controller-gen
utility. The newer release fixes an issue [1] where groupName is optional
from k8s 1.31. This optional field breaks our outdated controller-gen.

[1] kubernetes-sigs/controller-tools#1000
hjiawei added a commit to hjiawei/calico that referenced this pull request Dec 7, 2024
This change regenerates Calico manifests using the latest controller-gen
utility. The newer release fixes an issue [1] where groupName is optional
from k8s 1.31. This optional field breaks our outdated controller-gen.

[1] kubernetes-sigs/controller-tools#1000
hjiawei added a commit to hjiawei/calico that referenced this pull request Dec 7, 2024
This change regenerates Calico manifests using the latest controller-gen
utility. The newer release fixes an issue [1] where groupName is optional
from k8s 1.31. This optional field breaks our outdated controller-gen.

[1] kubernetes-sigs/controller-tools#1000
hjiawei added a commit to hjiawei/calico that referenced this pull request Dec 7, 2024
This change regenerates Calico manifests using the latest controller-gen
utility. The newer release fixes an issue [1] where groupName is optional
from k8s 1.31. This optional field breaks our outdated controller-gen.

[1] kubernetes-sigs/controller-tools#1000
hjiawei added a commit to hjiawei/calico that referenced this pull request Dec 7, 2024
This change regenerates Calico manifests using the latest controller-gen
utility. The newer release fixes an issue [1] where groupName is optional
from k8s 1.31. This optional field breaks our outdated controller-gen.

[1] kubernetes-sigs/controller-tools#1000
hjiawei added a commit to hjiawei/calico that referenced this pull request Dec 12, 2024
This change regenerates Calico manifests using the latest controller-gen
utility. The newer release fixes an issue [1] where groupName is optional
from k8s 1.31. This optional field breaks our outdated controller-gen.

[1] kubernetes-sigs/controller-tools#1000
hjiawei added a commit to hjiawei/calico that referenced this pull request Dec 13, 2024
This change regenerates Calico manifests using the latest controller-gen
utility. The newer release fixes an issue [1] where groupName is optional
from k8s 1.31. This optional field breaks our outdated controller-gen.

[1] kubernetes-sigs/controller-tools#1000
sridhartigera pushed a commit to sridhartigera/calico that referenced this pull request Dec 18, 2024
This change regenerates Calico manifests using the latest controller-gen
utility. The newer release fixes an issue [1] where groupName is optional
from k8s 1.31. This optional field breaks our outdated controller-gen.

[1] kubernetes-sigs/controller-tools#1000
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants