🐛 fix(go/v4): resolve create webhook target when several share a GVK - #5932
🐛 fix(go/v4): resolve create webhook target when several share a GVK#5932prash2512 wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: prash2512 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 |
|
Hi @prash2512. 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 Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain 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. |
f148488 to
d039a51
Compare
9f285a7 to
0dfbd2d
Compare
| if !found { | ||
| return fmt.Errorf( | ||
| "group %q, version %q and kind %q match more than one resource: "+ | ||
| "pass --external-api-domain to choose the one to work on", |
There was a problem hiding this comment.
we should only return it for external types
There was a problem hiding this comment.
Good catch!!
I missed to handle the fallback case to core type when external domain is not passed. Fixed in latest commit.
|
|
||
| Expect(err).To(HaveOccurred()) | ||
| Expect(err.Error()).To(ContainSubstring("match more than one resource")) | ||
| }) |
There was a problem hiding this comment.
Have we coverage that it works well without pass the domain when is a core type?
There was a problem hiding this comment.
Covered it in latest update
There was a problem hiding this comment.
Pull request overview
This PR fixes kubebuilder create webhook selecting the wrong recorded external resource when multiple entries share the same Group/Version/Kind under different domains, by refusing ambiguous matches and requiring --external-api-domain to select the intended resource.
Changes:
- Update
updateResourceFromConfigto collect all Group/Version/Kind matches and require--external-api-domainwhen ambiguous. - Add unit tests to cover ambiguous selection, order-independence, correct selection by domain, and no-match domain refusal.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/plugins/golang/v4/webhook.go | Updates resource lookup to avoid “first match wins” and require explicit domain selection when multiple external resources share a G/V/K. |
| pkg/plugins/golang/v4/webhook_test.go | Adds unit coverage for multi-match ambiguity and selection by --external-api-domain. |
0dfbd2d to
ee64539
Compare
|
Thanks for the review! This is the gist of the logic, please check if it matches your thoughts on the fix. Which recorded resource a
Errors list the recorded domains and distinguish "no domain given" from "domain given but no match". |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
pkg/plugins/golang/v4/webhook.go:297
- The ambiguous-resource error lists recorded domains without quoting them. If a recorded resource has an empty domain, the message becomes hard to read (e.g.,
domains: , cert-manager.io) and it’s unclear that one of the candidates is the empty string. Quoting each domain makes the output unambiguous and consistent with the rest of the message’s %q formatting.
domains := make([]string, len(matches))
for i, m := range matches {
domains[i] = m.Domain
}
| } else { | ||
| // No domain named: use the non-external (core/project) entry, if any. | ||
| for _, m := range matches { | ||
| if !m.External { |
There was a problem hiding this comment.
Core resources also have External == false. A recorded core
apps//v1/Deployment and project API apps.example.com/v1/Deployment can
therefore both match. This loop again chooses whichever appears first in
PROJECT, preserving the order-dependent wrong-resource bug. Prefer the match
whose domain equals res.Domain, or require the non-external match to be
unique.
There was a problem hiding this comment.
Fixed. It no longer takes the first non-external match - on a project+core collision it keeps the one whose domain matches res.Domain (the project's), so it's order-independent. It only errors when every match is external.
| for i, m := range matches { | ||
| domains[i] = m.Domain | ||
| } | ||
| if domain != "" { |
There was a problem hiding this comment.
Once two external resources share a G/V/K, a valid command supplying a new
--external-api-domain and --external-api-path is rejected because the domain
is not already recorded. This makes adding the second variant work but
prevents adding a third. An unmatched domain should only be rejected when no
external path was supplied; otherwise the flags fully describe a new
resource.
There was a problem hiding this comment.
Good point - fixed. An unmatched --external-api-domain is only rejected when there's no --external-api-path; with a path the flags describe a new resource, so it proceeds. Adding a third variant works now.
| res = &resValue | ||
| if err != nil { | ||
| if !p.resource.External && !p.resource.Core { | ||
| return fmt.Errorf( |
There was a problem hiding this comment.
--external-api-domain now selects among recorded resources, but its help
text only mentions RBAC generation. Suggested wording: “Domain for the
external API (e.g., cert-manager.io); selects the recorded resource when
multiple resources share a group, version, and kind, and is used to generate
RBAC markers.”
…e a GVK create webhook matched a recorded resource by Group, Version and Kind and took the first hit. When a project records more than one external resource sharing a GVK under different domains, the webhook was bound to whichever entry the PROJECT file listed first, and passing --external-api-domain to select another failed outright. updateResourceFromConfig now collects every Group/Version/Kind match: a single match is recovered as before; with several, --external-api-domain selects one, otherwise the non-external (core/project) entry is used, preferring the project when a core and project entry collide; it refuses and names the recorded domains when it still cannot tell which was meant, leaving the PROJECT file unchanged. An unmatched --external-api-domain is only rejected when no --external-api-path is given, so a further variant can still be added. create api is unaffected; it already looks resources up by the full GVK.
ac3800f to
8b27fcd
Compare
|
Thank you for the detailed review! All three comments addressed. Additionally tightened two related things while here
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
pkg/plugins/golang/v4/webhook.go:267
- Spoke-version validation runs before options.UpdateResource applies --external-api-domain to the resource (that only happens when --external-api-path is set). With the new isValidVersion matching on QualifiedGroup (group+domain), this can validate spokes against the project domain for a new external resource (no PROJECT record yet), and then later change res.Domain to the external domain—accepting spokes that don't exist for the intended API.
domain := p.options.ExternalAPIDomain
var selected *resource.Resource
|
/ok-to-test |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (2)
pkg/plugins/golang/v4/webhook.go:120
- The --external-api-domain help text diverges from the wording used for the same flag in
create api(pkg/plugins/golang/v4/api.go:120-122), which can make--helpoutput feel inconsistent across subcommands. Consider keeping the shared first sentence consistent and appending the create-webhook-specific behavior.
fs.StringVar(&p.options.ExternalAPIDomain, "external-api-domain", "",
"Domain for the external API (e.g., cert-manager.io). Selects the recorded resource when "+
"several share a group, version, and kind, and is used to generate accurate RBAC markers "+
"and permissions for the external resources")
pkg/plugins/golang/v4/webhook.go:350
resolutionErrorbuilds the domain list without quoting, which can make the message ambiguous when a recorded domain is empty (e.g. it renders as a leading/trailing comma with no obvious "empty" entry). Quoting the domains makes the error actionable in that edge case.
domains := make([]string, len(candidates))
for i, c := range candidates {
domains[i] = c.Domain
}
8b27fcd to
3c9c9d6
Compare
|
/test pull-kubebuilder-test |
|
Hi @camilamacedo86 |
|
Hi @camilamacedo86 |
create webhookmatched a recorded resource by Group, Version and Kind and took the first hit. When a project records more than one external resource that shares a GVK under different domains, it silently bound the webhook to whichever entry the PROJECT file listed first, and passing --external-api-domain to select another one failed outright.Collect every Group/Version/Kind match instead of taking the first. With a single match, behave as before. With several, use --external-api-domain to select one, and return an error when it is missing or matches none so the command never guesses and leaves the PROJECT file unchanged.
Fixes #5931
Change
updateResourceFromConfignow collects every Group/Version/Kind match instead of taking the first:--external-api-domainto select one, and returns anerror when it is missing or matches none — so the command never guesses and
leaves the PROJECT file unchanged.
create apiis unaffected; it already looks resources up by the full GVK.Tests
Unit specs in
webhook_test.go:--external-api-domain→ works on that resource, leaves the other alone;--external-api-domainmatching none → refuses;