🐛 (go/v4): return an error when --controller-name is invalid instead of failing silently

[onkar717]

Description

updateControllers in pkg/plugins/golang/options.go discarded both calls to Controllers.AddController with _ =. AddController validates the name as a DNS-1035 label and rejects duplicates, but the error was never propagated anywhere.

validateController in pkg/plugins/golang/v4/api.go only checks emptiness/duplicates against the resource already persisted in the PROJECT file, and never runs its format check for a brand-new resource (p.config.GetResource returns an error for a resource that doesn't exist yet, so validateController returns nil immediately).

Net effect: for a new API, an invalid --controller-name (e.g. Backup_Ctrl, which fails DNS-1035 validation uppercase/underscore not allowed) was silently discarded. res.Controllers stayed empty, and the scaffolding logic in api.go fell back to the default kind-derived controller name instead, while the CLI reported success. The user's chosen
name was silently ignored.

Found this by reading the controller-scaffolding path end to end while looking for real, self-contained bugs to contribute. It's a genuine correctness gap (not defensive-only) reachable by any snake_case or otherwise-invalid --controller-name on first use, and the affected file had zero test coverage for ControllerName at all beforehand.

What this PR does

• updateControllers and UpdateResource (pkg/plugins/golang/options.go) now return error and propagate both AddController calls instead of discarding them.
• Propagates that error through the three call sites (pkg/plugins/golang/v4/api.go, pkg/plugins/golang/v4/webhook.go,
  pkg/plugins/golang/deploy-image/v1alpha1/api.go), wrapped as "error updating resource: %w" to match the existing convention already used right below each of those call sites.
• Adds a ControllerName validation test block in options_test.go: valid name (succeeds, controller added), invalid name (returns an error,
  Controllers stays empty instead of silently falling back), and duplicate name (returns an error).
• Updates the five pre-existing bare options.UpdateResource(...) calls in the same test file to assert Succeed(), since they now discard the same class of error the production code used to.

Verified locally: go build ./..., the golang/v4/deploy-imageGinkgo suites all pass, golangci-lint run (pinned v2.12.2, repo config) reports 0 issues. Also verified the new tests aren't vacuous by temporarily reverting the fix both the "invalid name" and "duplicate name" cases went red with the expected failure messages, then passed again after reverting
back.

Fixes: none filed this PR is the record; happy to file one if a maintainer would prefer it tracked separately.

[kubernetes-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: onkar717
Once this PR has been reviewed and has the lgtm label, please assign varshaprasad96 for approval. For more information see the Code Review Process.

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

Details

Needs approval from an approver in each of these files:

• OWNERS

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

[kubernetes-prow]

Welcome @onkar717!

It looks like this is your first PR to kubernetes-sigs/kubebuilder 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kubebuilder has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[kubernetes-prow]

Hi @onkar717. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work.

Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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

[camilamacedo86]

/ok-to-test

[camilamacedo86]

Hi @onkar717,

Thank you so much for looking into this.

I reviewed the changes, and I think we need to address a few points before merging. Could you please take another look?

I centralized my findings below and used AI to help organize the wording.

1. validateController still skips new resources

In pkg/plugins/golang/v4/api.go, the early return mentioned in the PR description is still unchanged:

existingRes, err := p.config.GetResource(p.resource.GVK)
if err != nil {
    return nil // New resources are not validated.
}

Because --controller-name is a flag value, it should be validated before this return:

if p.options.ControllerName != "" {
    if err := (resource.Controller{Name: p.options.ControllerName}).Validate(); err != nil {
        return fmt.Errorf("invalid '--controller-name': %w", err)
    }
}

2. InjectResource validates after mutating the configuration

In the same file, UpdateResource runs before validateController, so the deeper AddController error is always returned first.

The validation should happen before the configuration is mutated:

if err := p.validateController(); err != nil {
    return err
}

if err := p.options.UpdateResource(p.resource, p.config); err != nil {
    return fmt.Errorf("error updating resource: %w", err)
}

validateAPI() can remain as the final call. validateController only reads the flags, GVK, and configuration, so this reordering should be safe.

Items 1 and 2 need to be addressed together. Applying only one of them would not change the current behavior.

3. Simplify the error message

The current error is:

error updating resource: error adding controller "Backup_Ctrl": invalid controller name "Backup_Ctrl": ...

The controller name appears twice. Controller.Validate() already returns a complete error, so the wrapper only needs to identify the flag.

4. One discarded error remains

In pkg/model/resource/resource.go:283, the following error is still ignored:

_ = r.Controllers.AddController(defaultName)

This follows the same legacy migration path. Since Resource.Update already returns an error, this should be a straightforward one-line fix.

This path is used when merging existing PROJECT file configurations.

5. Two call sites cannot currently trigger this error

webhook.go never sets DoController, and deploy-image/api.go never sets ControllerName.

At the moment, only create api --controller-name can trigger this validation error. Keeping the error propagation in the other call sites is fine, but the PR description should clarify this.

6. Add tests one layer higher

The new tests call Options.UpdateResource directly. Please also add:

• A CLI-level test confirming that create api --controller-name=Bad_Name is rejected.
• A test covering the legacy migration path in resource.go.

[camilamacedo86]

The description says validateController "only checked existing resources,
skipping validation for new ones", but the function is never changed here — the
api.go diff is only the wrap around UpdateResource. This early return is
what skips new resources, and it is still there:

existingRes, err := p.config.GetResource(p.resource.GVK)
if err != nil {
    // Resource does not exist yet, no validation needed
    return nil
}

The PR routes around it by surfacing the error from AddController deep in the
data layer. Both changes are worth having, but the flag itself should be checked
where the other flags are checked:

// The name must be valid even when the resource is new and has nothing to compare against.
if p.options.ControllerName != "" {
    if err := (resource.Controller{Name: p.options.ControllerName}).Validate(); err != nil {
        return fmt.Errorf("invalid '--controller-name': %w", err)
    }
}

[camilamacedo86]

I think that 2/3 call can never produce an error. webhook.go never sets DoController, so updateControllers sn't called at all. deploy-image/api.go sets DoController = true but never sets ControllerName, so it hits the early return nil. Only create api --controller-name=... can actually fail. Keeping the plumbing is fine, but the PR description should say so — otherwise a reviewer assumes webhook behavior changed.

Could you please check it out?

[camilamacedo86]

I think that to solve what you want to solve unless I misunderstood it we would need to:

Current:

      existingRes, err := p.config.GetResource(p.resource.GVK)
      if err != nil {
              // Resource does not exist yet, no validation needed
              return nil
      }

Change to:

      // The name must be rce is new and has nothingcompare against.
      if p.options.Control
              if err := (resource.Controller{Name: p.options.ControllerName}).
err != nil {
                      return fmt.Errorf("invalid '--controller-name': %w", err
              }
      }

      existingRes, err := p.config.GetResource(p.resource.GVK)
      if err != nil {
              // Resource does not exist yet, no further validation needed
              return nil
      }

[camilamacedo86]

Hi @onkar717

We use the PR title as release notes, I hope that you do not mind I tried to change it for users POV. Could you please check it out? if you think that has a better wording please feel free to change it too.

[Copilot]

Pull request overview

This PR fixes a correctness gap in the Go plugins’ controller scaffolding flow by ensuring invalid --controller-name values fail fast instead of being silently ignored and falling back to the default controller name.

Changes:

• Change Options.UpdateResource / updateControllers to return error and propagate Controllers.AddController validation/duplicate errors.
• Propagate and wrap UpdateResource errors at the create api / create webhook call sites (including deploy-image).
• Add unit tests covering valid, invalid, and duplicate ControllerName scenarios; update existing tests to assert UpdateResource success.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
pkg/plugins/golang/options.go	Return/propagate errors from controller updates instead of discarding AddController results.
pkg/plugins/golang/v4/api.go	Handle UpdateResource errors and return a wrapped failure instead of continuing.
pkg/plugins/golang/v4/webhook.go	Handle UpdateResource errors and return a wrapped failure instead of continuing.
pkg/plugins/golang/deploy-image/v1alpha1/api.go	Handle UpdateResource errors in deploy-image API scaffolding path.
pkg/plugins/golang/options_test.go	Add ControllerName validation tests and update existing cases to assert UpdateResource succeeds.

[camilamacedo86]

I think we need move with #5941
Could you please give a look?