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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/crd/markers/package.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.

mustOptional(markers.MakeDefinition("groupName", markers.DescribesPackage, "")).
sbueringer marked this conversation as resolved.
Show resolved Hide resolved
WithHelp(markers.SimpleHelp("CRD", "specifies the API group name for this package.")),

must(markers.MakeDefinition("versionName", markers.DescribesPackage, "")).
Expand Down
14 changes: 14 additions & 0 deletions pkg/crd/markers/register.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package markers

import (
"fmt"
"reflect"

"sigs.k8s.io/controller-tools/pkg/markers"
Expand Down Expand Up @@ -56,6 +57,19 @@ func must(def *markers.Definition, err error) *definitionWithHelp {
}
}

func mustOptional(def *markers.Definition, err error) *definitionWithHelp {
def = markers.Must(def, err)
if !def.AnonymousField() {
def = markers.Must(def, fmt.Errorf("not an anonymous field: %v", def))
}
field := def.Fields[""]
field.Optional = true
def.Fields[""] = field
return &definitionWithHelp{
Definition: def,
}
}

// AllDefinitions contains all marker definitions for this package.
var AllDefinitions []*definitionWithHelp

Expand Down
20 changes: 17 additions & 3 deletions pkg/crd/parser_integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -100,9 +100,9 @@ var _ = Describe("CRD Generation From Parsing to CustomResourceDefinition", func
}
})

assertCRD := func(pkg *loader.Package, kind, fileName string) {
By(fmt.Sprintf("requesting that the %s CRD be generated", kind))
groupKind := schema.GroupKind{Kind: kind, Group: "testdata.kubebuilder.io"}
assertCRDForGroupKind := func(pkg *loader.Package, groupKind schema.GroupKind, fileName string) {
kind := groupKind.Kind
By(fmt.Sprintf("requesting that the %s CRD be generated in group %s", kind, groupKind.Group))
parser.NeedCRDFor(groupKind, nil)

By(fmt.Sprintf("fixing top level ObjectMeta on the %s CRD", kind))
Expand Down Expand Up @@ -131,6 +131,10 @@ var _ = Describe("CRD Generation From Parsing to CustomResourceDefinition", func
ExpectWithOffset(1, parser.CustomResourceDefinitions[groupKind]).To(Equal(crd), "type not as expected, check pkg/crd/testdata/README.md for more details.\n\nDiff:\n\n%s", cmp.Diff(parser.CustomResourceDefinitions[groupKind], crd))
}

assertCRD := func(pkg *loader.Package, kind, fileName string) {
assertCRDForGroupKind(pkg, schema.GroupKind{Group: "testdata.kubebuilder.io", Kind: kind}, fileName)
}

assertError := func(pkg *loader.Package, kind, errorMsg string) {
By(fmt.Sprintf("requesting that the %s CRD be generated", kind))
groupKind := schema.GroupKind{Kind: kind, Group: "testdata.kubebuilder.io"}
Expand Down Expand Up @@ -183,6 +187,16 @@ var _ = Describe("CRD Generation From Parsing to CustomResourceDefinition", func
assertError(pkgs[0], "CronJob", "is not in 'xxx=xxx' format")
})
})

Context("CronJob API without group", func() {
BeforeEach(func() {
pkgPaths = []string{"./nogroup"}
expPkgLen = 1
})
It("should successfully generate the CronJob CRD", func() {
assertCRDForGroupKind(pkgs[0], schema.GroupKind{Kind: "CronJob"}, "testdata._cronjobs.yaml")
})
})
})

It("should generate plural words for Kind correctly", func() {
Expand Down
29 changes: 29 additions & 0 deletions pkg/crd/testdata/nogroup/types.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
/*

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

// +groupName=
package nogroup

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

// +kubebuilder:object:root=true

// CronJob is the Schema for the cronjobs API
type CronJob struct {
metav1.TypeMeta `json:",inline"`
metav1.ObjectMeta `json:"metadata,omitempty"`
}
41 changes: 41 additions & 0 deletions pkg/crd/testdata/testdata._cronjobs.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
---
apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
annotations:
controller-gen.kubebuilder.io/version: (devel)
name: cronjobs.
spec:
group: ""
names:
kind: CronJob
listKind: CronJobList
plural: cronjobs
singular: cronjob
scope: Namespaced
versions:
- name: nogroup
schema:
openAPIV3Schema:
description: CronJob is the Schema for the cronjobs API
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
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
served: true
storage: true