Introduce skip-name-validation flag and consolidate tests by anhdle-sso · Pull Request #7075 · GoogleCloudPlatform/k8s-config-connector

anhdle-sso · 2026-03-17T19:46:27Z

BRIEF Change description

Introduce skip-name-validation flag to bypass duplicate controller name checks during registration and consolidate related tests.

WHY do we need this change?

In integration tests and certain multi-manager scenarios, creating multiple managers in the same process can lead to conflicts in controller-runtime's global controller name registry. This flag allows for opting out of that check when such conflicts are expected or managed.

Special notes for your reviewer:

Merged pkg/controller/kccmanager/kccmanager_skipnamevalidation_test.go into pkg/controller/kccmanager/kccmanager_test.go to maintain a cleaner package structure.
Ran make fmt to ensure proper code formatting.

Does this PR add something which needs to be 'release noted'?

Added a `--skip-name-validation` flag to Config Connector controllers to bypass duplicate controller name checks during registration.

Reviewer reviewed release note.

Additional documentation e.g., references, usage docs, etc.:

NONE

Intended Milestone

Reviewer tagged PR with the actual milestone.

Tests you have done

Ran all tests in pkg/controller/kccmanager package:
go test -v ./pkg/controller/kccmanager -tags=integration

Run make ready-pr to ensure this PR is ready for review.
Perform necessary E2E testing for changed resources.

This change introduces a 'skip-name-validation' flag across various controllers to bypass the duplicate controller name check during registration. This is particularly useful for integration tests where multiple managers might be created in the same process, avoiding conflicts in controller-runtime's global name registry. Additionally, consolidated 'kccmanager_skipnamevalidation_test.go' into 'kccmanager_test.go'.

- Use k8s.io/utils/ptr.To instead of taking address of struct fields for controller options. - Clarify SkipNameValidation comments and flag description. - Add note about global state in controller-runtime during tests.

cheftako

Approving for now. Lets get these cleaned up soon.

cheftako · 2026-03-17T23:04:03Z

 	flag.CommandLine.AddGoFlagSet(goflag.CommandLine)
 	flag.BoolVar(&enablePprof, "enable-pprof", false, "Enable the pprof server.")
 	flag.IntVar(&pprofPort, "pprof-port", 6060, "The port that the pprof server binds to if enabled.")
+	flag.BoolVar(&skipNameValidation, "skip-name-validation", false, "option to bypass the duplicate controller name check during registration; false by default")


Nit: Dislike booleans in APIs. Also not sure why we need this control in the deletion defender.

cheftako · 2026-03-17T23:04:39Z

 	// Register the registration controller, which will dynamically create controllers for
 	// all our resources.
-	if err := registration.AddDeletionDefender(mgr, &controller.Deps{}); err != nil {
+	if err := registration.AddDeletionDefender(mgr, &controller.Deps{SkipNameValidation: skipNameValidation}); err != nil {


Nit: Seems like we can just pass false here.

cheftako · 2026-03-17T23:24:27Z

 	flag.Float32Var(&rateLimitQps, "qps", 20.0, "The client-side token bucket rate limit qps.")
 	flag.IntVar(&rateLimitBurst, "burst", 30, "The client-side token bucket rate limit burst.")
 	flag.StringVar(&leaderElectionMode, "leader-election-type", "disabled", "Leader election mode. One of: default, multicluster.")
+	flag.BoolVar(&skipNameValidation, "skip-name-validation", false, "option to bypass the global controller name registry in controller-runtime; false by default")


Nit: Dislike booleans in APIs. Also not sure why we need this control in the manager? Seems like we are running the manager from cli tool. We should chat about this. In a 10K namespace cluster, I don't think its safe to spin up 10K pods. I would rather have this run in process and take longer.

cheftako · 2026-03-17T23:29:37Z

 	flag.BoolVar(&enablePprof, "enable-pprof", false, "Enable the pprof server.")
 	flag.IntVar(&pprofPort, "pprof-port", 6060, "The port that the pprof server binds to if enabled.")
 	flag.StringVar(&managerNamespaceIsolation, k8s.ManagerNamespaceIsolationFlag, k8s.ManagerNamespaceIsolationShared, fmt.Sprintf("'%s' if all controller managers run in shared 'cnrm-system' namespace, '%s' if controller managers run in dedicated namespace. Default is '%s'", k8s.ManagerNamespaceIsolationShared, k8s.ManagerNamespaceIsolationDedicated, k8s.ManagerNamespaceIsolationShared))
+	flag.BoolVar(&skipNameValidation, "skip-name-validation", false, "option to bypass the duplicate controller name check during registration; false by default")


Nit: Dislike booleans in APIs. Also not sure why we need this control in the unmanaged detector? Lets get a bug going and clean this up before 1.48

cheftako · 2026-03-17T23:30:43Z


-func Add(mgr manager.Manager, crd *apiextensions.CustomResourceDefinition) error {
+func Add(mgr manager.Manager, crd *apiextensions.CustomResourceDefinition, opt controller.Options) error {
+	if opt.MaxConcurrentReconciles == 0 {


NIT: Should probably be <= 0. Assuming we not support negatives.

cheftako · 2026-03-17T23:31:33Z

 // add adds a new Controller to mgr with r as the reconcile.Reconciler.
-func add(mgr manager.Manager, r *Reconciler) error {
+func add(mgr manager.Manager, r *Reconciler, opt controller.Options) error {
+	if opt.MaxConcurrentReconciles == 0 {


cheftako · 2026-03-17T23:31:48Z

 // add adds a new Controller to mgr with r as the reconcile.Reconciler.
-func add(mgr manager.Manager, r *ReconcileIAMPolicy) error {
+func add(mgr manager.Manager, r *ReconcileIAMPolicy, opt controller.Options) error {
+	if opt.MaxConcurrentReconciles == 0 {


cheftako · 2026-03-17T23:32:05Z

 // add adds a new Controller to mgr with r as the reconcile.Reconciler.
-func add(mgr manager.Manager, r *Reconciler) error {
+func add(mgr manager.Manager, r *Reconciler, opt controller.Options) error {
+	if opt.MaxConcurrentReconciles == 0 {


cheftako · 2026-03-17T23:32:52Z

+func Add(ctx context.Context, mgr manager.Manager, gvk schema.GroupVersionKind, opt controller.Options) error {
 	logger := crlog.FromContext(ctx)

+	if opt.MaxConcurrentReconciles == 0 {


google-oss-prow · 2026-03-17T23:33:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cheftako

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [cheftako]

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

This PR cherry-picks the changes from PR #7075 to the release-1.147 branch

anhdle-sso commented Mar 17, 2026

View reviewed changes

Address review comments: use ptr.To and clarify documentation

0cc5ac7

- Use k8s.io/utils/ptr.To instead of taking address of struct fields for controller options. - Clarify SkipNameValidation comments and flag description. - Add note about global state in controller-runtime during tests.

anhdle-sso mentioned this pull request Mar 17, 2026

[Safer Upgrade] Stop registering deletion defender or unmanage detector during safer upgrade run #7079

Open

cheftako approved these changes Mar 17, 2026

View reviewed changes

google-oss-prow Bot assigned cheftako Mar 17, 2026

google-oss-prow Bot added the lgtm label Mar 17, 2026

google-oss-prow Bot added the approved label Mar 17, 2026

anhdle-sso added this pull request to the merge queue Mar 17, 2026

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Mar 18, 2026

anhdle-sso added this pull request to the merge queue Mar 18, 2026

Merged via the queue into GoogleCloudPlatform:master with commit 3a7898e Mar 18, 2026
166 checks passed

cheftako added this to the 1.148 milestone Mar 20, 2026

This was referenced Mar 20, 2026

Cherrypick PR #7075 to release-1.147 #7133

Closed

Cherry Pick PR #7075 to release-1.147 #7134

Merged

cheftako added a commit that referenced this pull request Mar 20, 2026

Cherry Pick PR #7075 to release-1.147 (#7134)

6608419

This PR cherry-picks the changes from PR #7075 to the release-1.147 branch

Conversation

anhdle-sso commented Mar 17, 2026

BRIEF Change description

WHY do we need this change?

Special notes for your reviewer:

Does this PR add something which needs to be 'release noted'?

Additional documentation e.g., references, usage docs, etc.:

Intended Milestone

Tests you have done

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cheftako left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

google-oss-prow Bot commented Mar 17, 2026

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants