🐛 fix support for external types by allowing the domain be empty, and properly generate the sample for cert-manager. #4217
Conversation
k8s-ci-robot
added
cncf-cla: yes
k8s-ci-robot
added
the
size/M
camilamacedo86
changed the title
|
Hi @fischor Could you please validate it? |
pkg/plugins/golang/v4/scaffolds/internal/templates/controllers/controller.go
Outdated
Show resolved
Hide resolved
camilamacedo86
force-pushed
the
fix-rbac-generattion-external-types
branch
from
66c56f1 to
2c4c72e
Compare
k8s-ci-robot
added
size/XL
camilamacedo86
changed the title
camilamacedo86
changed the title
camilamacedo86
force-pushed
the
fix-rbac-generattion-external-types
branch
from
2c4c72e to
af9c537
Compare
k8s-ci-robot
added
size/L
camilamacedo86
changed the title
k8s-ci-robot
added
the
do-not-merge/work-in-progress
camilamacedo86
force-pushed
the
fix-rbac-generattion-external-types
branch
from
af9c537 to
7c28288
Compare
camilamacedo86
changed the title
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
Hi @fischor, Thank you so much! That’s exactly the help I was looking for. We discussed the scenarios to ensure that it will work well. Your comment made me think: actually, CertManager does not define domains for their APIs, just the group. So, we must accommodate both scenarios—allowing the API to be scaffolded with only the group or with both the group and the domain. In this way, see that we are fixing here how we scaffold and allowing the scaffold be done with an empty domain. |
|
|
||
| ```shell | ||
| kubebuilder create api --group certmanager --version v1 --kind Certificate --controller=true --resource=false --external-api-path=github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1 --external-api-domain=cert-manager.io | ||
| kubebuilder create api --group cert-manager.io --version v1 --kind Certificate --controller=true --resource=false --external-api-path=github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1 |
There was a problem hiding this comment.
Seems to work like this. But its a bit counter-intuitive. For me "cert-manager.io" feels like a domain without a group.
But of course, we need a group since that is used e.g. as folder name. Can you explain why is the group mandatory? Where is it used except as folder name?
There was a problem hiding this comment.
My point here is more about how we recommend using the feature and if we should or not make the domain mandatory at this point. We could suggest that "io" is treated as the domain and "cert-manager" as the group to provide a complete scaffold structure. At the same time, we can allow the group to remain empty for those who prefer to tweak the configuration manually. There's no need to block this flexibility at the moment but it might generate invalid scaffolds.
Note that APIGroup technically can ONLY be empty when scaffolding core types and not external types.
Kubebuilder utilizes the API group in several places, including code generation, and RBAC markers, all of which are critical for scaffolding Kubernetes controllers. If the group is left empty, the scaffolding process generated might be incomplete or invalid. Also, we provides two project layouts:
- Single group: where all APIs belong to one group.
- Multigroup: where multiple groups are supported under one project.
Without proper directory structure, scaffolding might becomes problematic, as resources won't be correctly organized accordingly.
Why would we recommend keeping the group empty instead of the domain?
k8s-ci-robot
added
the
needs-rebase
camilamacedo86
changed the title
k8s-ci-robot
added
the
do-not-merge/work-in-progress
camilamacedo86
force-pushed
the
fix-rbac-generattion-external-types
branch
from
7c28288 to
c39536d
Compare
k8s-ci-robot
removed
the
needs-rebase
camilamacedo86
changed the title
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
camilamacedo86
force-pushed
the
fix-rbac-generattion-external-types
branch
from
c39536d to
d4b826d
Compare
camilamacedo86
changed the title
camilamacedo86
changed the title
camilamacedo86
changed the title
camilamacedo86
force-pushed
the
fix-rbac-generattion-external-types
branch
from
d4b826d to
ed613d5
Compare
camilamacedo86
changed the title
… properly generate the sample for cert-manager. - Allowing the domain be empty since some scenarios this value might not be required - Use `io` as the domain to fix the scaffold sample for cert-manager and update the example in the documentation.
camilamacedo86
force-pushed
the
fix-rbac-generattion-external-types
branch
from
ed613d5 to
9dd5480
Compare
| "specified together when referencing an external API.") | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Hi @fischor,
In my opinion, groups should not be empty when dealing with non-core types in scaffolds. The concepts should stay aligned with Kubernetes itself. We cannot have Kubebuilder diverging from core Kubernetes principles. However, in order to not block others scenarios we should not make mandatory users provide the Domain.
What are your thoughts?
There was a problem hiding this comment.
I am refering to the "group" (--group) as used in kubebuilder, not the Kubernetes apiGroup.
The apiGroup will not be empty if you do --domain cert-manager.io --group "", it will be cert-manager.io then.
My thoughts are that is does not make sense to have --domain and --group for external APIs, since these are kubebuilder specific entities and not all external APIs use that scheme. Maybe just --api-group is better to specify the (full) apiGroup for external APIs.
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: camilamacedo86, varshaprasad96 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 |
4 participants
ioas the domain to fix the scaffold sample for cert-manager and update the example in the documentation.