✨ add a controller-name for multi-controller#5474
✨ add a controller-name for multi-controller#5474guilhem wants to merge 3 commits intokubernetes-sigs:masterfrom
Conversation
Fix kubernetes-sigs#5473 Signed-off-by: Guilhem Lettron <glettron@akamai.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: guilhem The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Welcome @guilhem! |
|
Hi @guilhem. 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 Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
| kind: Memcached | ||
| path: example.com/memcached-operator/api/v1alpha1 | ||
| version: v1alpha1 | ||
| - controller: true |
There was a problem hiding this comment.
How it would be if instead of replicated GKV for each controller
we have:
controller
name:
- nameA
- nameB
- nameC
I think that would be a more clean structure
It would either facilitate the implementation for alpha generate.
WDYT?
There was a problem hiding this comment.
That's a cleaner structure indeed :)
Is something like that ok?
controllers:
- name: nameA
- name: nameB
- name: nameCIt gives the opportunity to add more fields in the future for each controllers.
There was a problem hiding this comment.
or, if we want to prevent an array:
controllers:
nameA: {}
nameB: {}
nameC: {}There was a problem hiding this comment.
I just fear when using the cli:
When the first name hasn't been set, we will have to set "nameA" to the first computed and nameB to the one set.
(not really a problem, just something to do)
There was a problem hiding this comment.
When the first name hasn't been set, we will have to set "nameA" to the first computed and nameB to the one set.
We have a method that do the "merge" of GKV.
I think we need to have exactly the same but for the new Controller as well.
We will need to think a lot here to ensure that we have a great API
Because we cannot change easily those after we release the changes.
So maybe it will be acctually like
resource:
controller:
- name: A
controller:
-Name B
The list I think should be of controllers.
ONE API/GKV can have MANY controllers ;-)
So that tomorrow we can either
resource:
controller:
- name: A
- key: value
- key: value
controller:
-Name B
Signed-off-by: Guilhem Lettron <glettron@akamai.com>
| | `resources.api.crdVersion` | The Kubernetes API version (`apiVersion`) used to do the scaffolding for the CRD resource. | | ||
| | `resources.api.namespaced` | The API RBAC permissions which can be namespaced or cluster scoped. | | ||
| | `resources.controller` | Indicates whether a controller was scaffolded for the API. | | ||
| | `resources.controllerName` | **(Optional)** A custom name for the controller, provided via the `--controller-name` flag. When set, it allows multiple controllers for the same GVK. The name is used for the controller file name, the reconciler struct name, and the controller's `Named()` registration. Must be a DNS-1035 label (lowercase alphanumeric and hyphens). | |
There was a problem hiding this comment.
Let's try check out it with a list of Namespaces
controller.names LIST
WDYT?
pkg/config/v3/config.go
Outdated
|
|
||
| for i, r := range c.Resources { | ||
| if res.IsEqualTo(r.GVK) { | ||
| if res.IsEqualTo(r.GVK) && res.ControllerName == r.ControllerName { |
There was a problem hiding this comment.
I think we need a method to check in the LIST something like
resource.HasControllerName()
pkg/config/v3/config_test.go
Outdated
| API: &resource.API{CRDVersion: "v1", Namespaced: true}, | ||
| }, | ||
| { | ||
| GVK: resource.GVK{ |
There was a problem hiding this comment.
We should not duplicate the GKV we could not have more than one on the cluster less yet in the project
| // Validate controller name if provided | ||
| if p.options.ControllerName != "" { | ||
| if !p.options.DoController { | ||
| return errors.New("cannot use '--controller-name' with '--controller=false'") |
There was a problem hiding this comment.
Valiations should be in the preScaffold
We probably need to add this one inside of the method that validate GKV already
pkg/plugins/golang/v4/api.go
Outdated
| } | ||
| if errs := validation.IsDNS1035Label(p.options.ControllerName); len(errs) != 0 { | ||
| return fmt.Errorf("invalid controller name %q: must be a DNS-1035 label "+ | ||
| "(lowercase, alphanumeric, hyphens, starting with a letter)", p.options.ControllerName) |
pkg/plugins/golang/v4/api.go
Outdated
| } | ||
|
|
||
| // When adding a named controller for an existing GVK, skip API re-scaffolding | ||
| if p.options.ControllerName != "" && p.options.DoAPI { |
There was a problem hiding this comment.
I think it is not accurate.
At the same way that we have many controllers for a core API we should be able to have many controllers for a external type too.
pkg/plugins/golang/options.go
Outdated
|
|
||
| // ControllerName is an optional custom name for the controller. | ||
| // When set, allows scaffolding multiple controllers for the same GVK. | ||
| ControllerName string |
There was a problem hiding this comment.
Lets try to do a list
But we need to properly think in how we will design the API here
I think we will need a new struct Controller and then inside of it we add name
Therefore, in the future we can add other specs for Controller if we have the need.
camilamacedo86
left a comment
There was a problem hiding this comment.
HI @guilhem
Thank you for looking on that.
I did some comments.
Mainly IHMO:
- The API must be a design choice that allow us to grow if we need. So we will probably need to have a new struct Controller inside of Resources and that allows pass a List of names.
- The validations for GKV need to be done in the preScaffold. Those should be in the funcs that we have already today in the API
- We will need either ensure that alpha generate work with. So, if we have resources.Crontroller.names we should call create api resource false and controller true with the name for each name after create the API.
Signed-off-by: Guilhem Lettron <glettron@akamai.com>
|
Keywords which can automatically close issues and hashtag(#) mentions are not allowed in commit messages. The list of commits with invalid commit messages:
DetailsInstructions 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. I understand the commands that are listed here. |
|
@camilamacedo86 I changed to new API for PROJECT and passed this PR as a draft to be certain it's not merged until the API is stabilized. |
|
PR needs rebase. DetailsInstructions 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. |
Naive implementation of #5473
This pull request introduces support for scaffolding multiple controllers for the same Group/Version/Kind (GVK) in Kubebuilder, allowing users to split reconciliation responsibilities, run different reconciliation modes, or manage migration scenarios. The main enhancement is the addition of the
--controller-nameflag to thecreate apicommand, along with corresponding updates to documentation, configuration, and code generation logic.Feature: Multiple Controllers for the Same GVK
--controller-nameflag increate api, enabling scaffolding of additional controllers for an existing API. Each controller gets its own file, struct, and unique registration, and the controller name is tracked in the PROJECT file. [1] [2] [3] [4] [5]Validation and CLI Enhancements
Documentation Updates
Code Generation and Template Changes
cmd/main.goand controller test files. [1] [2]These changes make Kubebuilder more flexible for advanced controller patterns and improve clarity and maintainability in projects with complex reconciliation needs.