Skip to content

Conversation

@camilamacedo86
Copy link
Member

@camilamacedo86 camilamacedo86 commented Oct 14, 2024

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

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 14, 2024
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 14, 2024
@camilamacedo86 camilamacedo86 changed the title fix: use Resource.Domain for RBAC markers when Resource.External is true 🐛 fix: use Resource.Domain for RBAC markers when Resource.External is true Oct 14, 2024
@camilamacedo86
Copy link
Member Author

camilamacedo86 commented Oct 14, 2024

Hi @fischor

Could you please validate it?
I think we need to use the domain to generate the RBAC in this case
Then, do we really need the domain?

@camilamacedo86 camilamacedo86 force-pushed the fix-rbac-generattion-external-types branch from 66c56f1 to 2c4c72e Compare October 17, 2024 13:08
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 17, 2024
@camilamacedo86 camilamacedo86 changed the title 🐛 fix: use Resource.Domain for RBAC markers when Resource.External is true 🐛 ix: support for external types with optional domain fields Oct 17, 2024
@camilamacedo86 camilamacedo86 changed the title 🐛 ix: support for external types with optional domain fields 🐛 fix: support for external types with optional domain fields Oct 17, 2024
@camilamacedo86 camilamacedo86 force-pushed the fix-rbac-generattion-external-types branch from 2c4c72e to af9c537 Compare October 17, 2024 15:26
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 17, 2024
@camilamacedo86 camilamacedo86 changed the title 🐛 fix: support for external types with optional domain fields WIP: 🐛 fix: support for external types with optional domain fields Oct 17, 2024
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 17, 2024
@camilamacedo86 camilamacedo86 force-pushed the fix-rbac-generattion-external-types branch from af9c537 to 7c28288 Compare October 17, 2024 21:10
@camilamacedo86 camilamacedo86 changed the title WIP: 🐛 fix: support for external types with optional domain fields 🐛 fix: support for external types with optional domain fields Oct 17, 2024
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 17, 2024
@camilamacedo86
Copy link
Member Author

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.

@kubernetes-sigs kubernetes-sigs deleted a comment from k8s-ci-robot Oct 17, 2024

```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
Copy link

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

@camilamacedo86 camilamacedo86 Oct 18, 2024

Choose a reason for hiding this comment

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

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 k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 18, 2024
@camilamacedo86 camilamacedo86 changed the title 🐛 fix: support for external types with optional domain fields WIP: 🐛 fix: support for external types with optional domain fields Oct 18, 2024
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 18, 2024
@camilamacedo86 camilamacedo86 force-pushed the fix-rbac-generattion-external-types branch from 7c28288 to c39536d Compare October 18, 2024 11:48
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 18, 2024
@camilamacedo86 camilamacedo86 changed the title WIP: 🐛 fix: support for external types with optional domain fields 🐛 fix: add support for external types by ensuring the domain is specified, and properly generate the sample for cert-manager Oct 18, 2024
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 18, 2024
@camilamacedo86 camilamacedo86 force-pushed the fix-rbac-generattion-external-types branch from c39536d to d4b826d Compare October 18, 2024 11:53
@camilamacedo86 camilamacedo86 changed the title 🐛 fix: add support for external types by ensuring the domain is specified, and properly generate the sample for cert-manager 🐛 fix: add support for external types by ensuring the domain is specified, and properly generate the sample for cert-manager. Oct 18, 2024
@camilamacedo86 camilamacedo86 changed the title 🐛 fix: add support for external types by ensuring the domain is specified, and properly generate the sample for cert-manager. 🐛 fix support for external types by ensuring the domain is specified, and properly generate the sample for cert-manager. Oct 18, 2024
@camilamacedo86 camilamacedo86 changed the title 🐛 fix support for external types by ensuring the domain is specified, and properly generate the sample for cert-manager. 🐛 WIP fix support for external types by ensuring the domain is specified, and properly generate the sample for cert-manager. Oct 18, 2024
@camilamacedo86 camilamacedo86 force-pushed the fix-rbac-generattion-external-types branch from d4b826d to ed613d5 Compare October 22, 2024 02:48
@camilamacedo86 camilamacedo86 changed the title 🐛 WIP fix support for external types by ensuring the domain is specified, and properly generate the sample for cert-manager. 🐛 fix support for external types by allowing the domain be empty, and properly generate the sample for cert-manager. Oct 22, 2024
… 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 camilamacedo86 force-pushed the fix-rbac-generattion-external-types branch from ed613d5 to 9dd5480 Compare October 22, 2024 02:49
"specified together when referencing an external API.")
}
}

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Member

@varshaprasad96 varshaprasad96 left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

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

[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:
  • OWNERS [camilamacedo86,varshaprasad96]

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 merged commit 60c13c6 into kubernetes-sigs:master Oct 22, 2024
24 checks passed
@camilamacedo86 camilamacedo86 deleted the fix-rbac-generattion-external-types branch October 22, 2024 03:32
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. release-blocker 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