fix(go/v4): resolve core/project collisions and single-match domain in create webhook

Address review on the webhook GVK resolution:

- When no --external-api-domain is given and several resources share the
  Group/Version/Kind, resolve to the non-external (core/project) entry
  instead of the first match, preferring the project when a core and
  project entry collide. The command only refuses when every match is
  external, so core and project types never hit the ambiguity error.
- Refuse a command whose --external-api-domain matches no recorded resource
  unless an --external-api-path also defines a new one, consistently for a
  single or multiple matches.
- Name the recorded domains in the ambiguity error and distinguish a
  missing domain from one that matched nothing.
- Update the --external-api-domain flag help to mention resource selection.

Resolution logic is a single length-based switch with two pure helpers
(selectByDomain, selectNonExternal); tests cover every branch through the
create webhook command with a decision table documenting the cases.

Author: prash2512

pkg/plugins/golang/v4/webhook.go

@@ -115,8 +115,9 @@ func (p *createWebhookSubcommand) BindFlags(fs *pflag.FlagSet) {
 			"Used to scaffold webhooks for resources defined outside this project")
 
 	fs.StringVar(&p.options.ExternalAPIDomain, "external-api-domain", "",
-		"Domain name for the external API (e.g., cert-manager.io). "+
-			"Used to generate accurate RBAC markers and permissions for the external resources")
+		"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")
 
 	fs.StringVar(&p.options.ExternalAPIModule, "external-api-module", "",
 		"External API module with optional version (e.g., github.com/cert-manager/cert-manager@v1.18.2)")
@@ -246,78 +247,120 @@ func (p *createWebhookSubcommand) PostScaffold() error {
 //
 // The lookup is by Group/Version/Kind rather than the full GVK because res still carries the
 // project domain, while an external resource keeps its own. Only external APIs can register the
-// same Group/Version/Kind under different domains, so when several match: --external-api-domain
-// selects the intended one; without it the non-external (core/project) entry is used; and when
-// every match is external the request is ambiguous and refused.
+// same Group/Version/Kind under different domains, so when several match --external-api-domain
+// selects the intended one; without it resolution falls to the non-external (core/project) entry.
 func (p *createWebhookSubcommand) updateResourceFromConfig(res *resource.Resource) error {
 	resources, err := p.config.GetResources()
 	if err != nil {
 		return fmt.Errorf("failed to load resources from project configuration: %w", err)
 	}
 
 	// Collect every recorded resource sharing this Group/Version/Kind.
-	var matches []resource.Resource
+	var candidates []resource.Resource
 	for _, r := range resources {
 		if r.Group == res.Group && r.Version == res.Version && r.Kind == res.Kind {
-			matches = append(matches, r)
+			candidates = append(candidates, r)
 		}
 	}
 
-	var existingRes resource.Resource
-	switch len(matches) {
+	domain := p.options.ExternalAPIDomain
+	var selected *resource.Resource
+	switch len(candidates) {
 	case 0:
 		return nil // nothing recorded for this GVK; keep res as built from the flags
 	case 1:
-		existingRes = matches[0]
+		if domain == "" {
+			selected = &candidates[0] // recover the single record
+		} else {
+			selected, err = selectByDomain(candidates, domain, p.options.ExternalAPIPath)
+		}
 	default:
-		// Several entries share this GVK — only external APIs can, under different domains.
-		domain := p.options.ExternalAPIDomain
-		found := false
-		if domain != "" {
-			// A domain was named: use the entry that carries it.
-			for _, m := range matches {
-				if m.Domain == domain {
-					existingRes, found = m, true
-					break
-				}
-			}
+		if domain == "" {
+			selected, err = selectNonExternal(candidates, res.Domain)
 		} else {
-			// No domain named: use the non-external (core/project) entry, if any.
-			for _, m := range matches {
-				if !m.External {
-					existingRes, found = m, true
-					break
-				}
-			}
+			selected, err = selectByDomain(candidates, domain, p.options.ExternalAPIPath)
 		}
-		if !found {
-			domains := make([]string, len(matches))
-			for i, m := range matches {
-				domains[i] = m.Domain
-			}
-			if domain != "" {
-				return fmt.Errorf(
-					"no resource matches --external-api-domain %q for group %q, version %q and kind %q "+
-						"(recorded domains: %s)",
-					domain, res.Group, res.Version, res.Kind, strings.Join(domains, ", "),
-				)
+	}
+	if err != nil {
+		return err
+	}
+	if selected == nil {
+		return nil // the flags describe a new resource
+	}
+
+	res.Domain = selected.Domain
+	res.Path = selected.Path
+	res.Plural = selected.Plural
+	res.External = selected.External
+	res.Core = selected.Core
+	res.Module = selected.Module
+
+	return nil
+}
+
+// selectByDomain resolves the candidate carrying the given --external-api-domain, for any number
+// of candidates. When none carries it, a non-empty external path means the flags describe a new
+// resource (nil, nil); otherwise the request is refused.
+func selectByDomain(candidates []resource.Resource, domain, externalPath string) (*resource.Resource, error) {
+	for i := range candidates {
+		if candidates[i].Domain == domain {
+			return &candidates[i], nil
+		}
+	}
+	if externalPath != "" {
+		return nil, nil
+	}
+	return nil, resolutionError(candidates, domain)
+}
+
+// selectNonExternal resolves several same-GVK candidates when no domain is given: the target is
+// the non-external (core/project) entry. When a core and project entry collide it keeps the
+// project (its domain equals projectDomain); when every candidate is external it refuses.
+func selectNonExternal(candidates []resource.Resource, projectDomain string) (*resource.Resource, error) {
+	var nonExternal []resource.Resource
+	for _, c := range candidates {
+		if !c.External {
+			nonExternal = append(nonExternal, c)
+		}
+	}
+
+	switch len(nonExternal) {
+	case 0:
+		return nil, resolutionError(candidates, "")
+	case 1:
+		return &nonExternal[0], nil
+	default:
+		// A core and a project resource share the GVK; keep the project one.
+		for i := range nonExternal {
+			if nonExternal[i].Domain == projectDomain {
+				return &nonExternal[i], nil
 			}
-			return fmt.Errorf(
-				"group %q, version %q and kind %q match more than one resource (domains: %s): "+
-					"pass --external-api-domain to choose the one to work on",
-				res.Group, res.Version, res.Kind, strings.Join(domains, ", "),
-			)
 		}
+		return nil, resolutionError(candidates, "")
 	}
+}
 
-	res.Domain = existingRes.Domain
-	res.Path = existingRes.Path
-	res.Plural = existingRes.Plural
-	res.External = existingRes.External
-	res.Core = existingRes.Core
-	res.Module = existingRes.Module
+// resolutionError reports why the candidates could not be resolved to one, naming their domains.
+// With a named domain it reports that none matched; without one it asks for --external-api-domain.
+func resolutionError(candidates []resource.Resource, domain string) error {
+	g, v, k := candidates[0].Group, candidates[0].Version, candidates[0].Kind
+	domains := make([]string, len(candidates))
+	for i, c := range candidates {
+		domains[i] = c.Domain
+	}
 
-	return nil
+	if domain != "" {
+		return fmt.Errorf(
+			"no resource matches --external-api-domain %q for group %q, version %q and kind %q "+
+				"(recorded domains: %s)",
+			domain, g, v, k, strings.Join(domains, ", "),
+		)
+	}
+	return fmt.Errorf(
+		"group %q, version %q and kind %q match more than one resource (domains: %s): "+
+			"pass --external-api-domain to choose the one to work on",
+		g, v, k, strings.Join(domains, ", "),
+	)
 }
 
 // Helper function to validate spoke versions

pkg/plugins/golang/v4/webhook_test.go

@@ -27,6 +27,9 @@ import (
 	goPlugin "sigs.k8s.io/kubebuilder/v4/pkg/plugins/golang"
 )
 
+// coreAPIPath is the Go import path recorded for a core (Kubernetes built-in) resource in tests.
+const coreAPIPath = "k8s.io/api/core/v1"
+
 var _ = Describe("createWebhookSubcommand", func() {
 	var (
 		subCmd *createWebhookSubcommand
@@ -223,6 +226,23 @@ var _ = Describe("createWebhookSubcommand", func() {
 		Expect(err.Error()).To(ContainSubstring("or pass --external-api-path for an external type"))
 	})
 
+	// updateResourceFromConfig resolves which recorded resource a create-webhook command refers to.
+	// The specs below cover the full decision, by number of recorded resources sharing the
+	// Group/Version/Kind and the flags given:
+	//
+	//	#  | matches | --external-api-domain | matches a record? | --external-api-path | outcome
+	//	---+---------+-----------------------+-------------------+---------------------+-------------------------
+	//	1  |   0     | any                   | -                 | any                 | keep res from the flags
+	//	2  |   1     | unset                 | -                 | -                   | use the record
+	//	3  |   1     | set                   | yes               | -                   | use the record
+	//	4  |   1     | set                   | no                | set                 | create a new resource
+	//	5  |   1     | set                   | no                | unset               | error (domain not found)
+	//	6  |  >=2    | set                   | yes               | -                   | use the record
+	//	7  |  >=2    | set                   | no                | set                 | create a new resource
+	//	8  |  >=2    | set                   | no                | unset               | error (domain not found)
+	//	9  |  >=2    | unset                 | -                 | -                   | error (all external)
+	//	10 |  >=2    | unset                 | - (1 non-external)| -                   | use the non-external one
+	//	11 |  >=2    | unset                 | - (project+core)  | -                   | use the project one
 	Context("when several external resources share the same Group/Version/Kind", func() {
 		const (
 			pathIO    = "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1"
@@ -358,6 +378,123 @@ var _ = Describe("createWebhookSubcommand", func() {
 			Expect(err).NotTo(HaveOccurred())
 			Expect(res.External).To(BeFalse())
 		})
+
+		It("should create a new external variant when the domain is new and a path is given", func() {
+			Expect(subCmd.InjectConfig(cfg)).To(Succeed())
+			storeExternal(domainIO, pathIO)
+			storeExternal(domainK8s, pathK8sIO)
+			subCmd.options.DoDefaulting = true
+			// A new domain plus a path fully describes a third external resource.
+			subCmd.options.ExternalAPIDomain = "cert-manager.new.io"
+			subCmd.options.ExternalAPIPath = "github.com/cert-manager/cert-manager/pkg/apis/new/v1"
+
+			err := subCmd.InjectResource(res)
+
+			Expect(err).NotTo(HaveOccurred())
+			Expect(res.External).To(BeTrue())
+			Expect(res.Domain).To(Equal("cert-manager.new.io"))
+		})
+
+		It("should resolve to the core entry over an external one when no domain is given", func() {
+			Expect(subCmd.InjectConfig(cfg)).To(Succeed())
+			storeExternal(domainK8s, pathK8sIO)
+			// A core entry shares the GVK with the external one.
+			Expect(cfg.AddResource(resource.Resource{
+				GVK:      resource.GVK{Group: crewGroup, Domain: "k8s.io", Version: "v1", Kind: captainKind},
+				Core:     true,
+				Path:     coreAPIPath,
+				Webhooks: &resource.Webhooks{},
+			})).To(Succeed())
+			subCmd.options.DoDefaulting = true
+
+			err := subCmd.InjectResource(res)
+
+			// The lone non-external (core) entry wins; the external one is left out.
+			Expect(err).NotTo(HaveOccurred())
+			Expect(res.Core).To(BeTrue())
+			Expect(res.External).To(BeFalse())
+		})
+
+		It("should keep the project entry when a project and core entry collide", func() {
+			Expect(subCmd.InjectConfig(cfg)).To(Succeed())
+			// A project entry (res's own domain) and a core entry share the GVK.
+			Expect(cfg.AddResource(resource.Resource{
+				GVK:      resource.GVK{Group: crewGroup, Domain: testIO, Version: "v1", Kind: captainKind},
+				Path:     "github.com/example/test/api/v1",
+				Webhooks: &resource.Webhooks{},
+			})).To(Succeed())
+			Expect(cfg.AddResource(resource.Resource{
+				GVK:      resource.GVK{Group: crewGroup, Domain: "k8s.io", Version: "v1", Kind: captainKind},
+				Core:     true,
+				Path:     coreAPIPath,
+				Webhooks: &resource.Webhooks{},
+			})).To(Succeed())
+			subCmd.options.DoDefaulting = true
+
+			err := subCmd.InjectResource(res)
+
+			// The project entry (domain == res.Domain) is kept, not the core one.
+			Expect(err).NotTo(HaveOccurred())
+			Expect(res.Core).To(BeFalse())
+			Expect(res.External).To(BeFalse())
+		})
+	})
+
+	Context("when a single external resource shares the Group/Version/Kind", func() {
+		const (
+			pathIO   = "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1"
+			domainIO = "cert-manager.io"
+		)
+
+		storeExternal := func(domain, path string) {
+			Expect(cfg.AddResource(resource.Resource{
+				GVK:      resource.GVK{Group: crewGroup, Domain: domain, Version: "v1", Kind: captainKind},
+				External: true,
+				Path:     path,
+				Webhooks: &resource.Webhooks{},
+			})).To(Succeed())
+		}
+
+		It("should use the recorded resource when --external-api-domain matches it", func() {
+			Expect(subCmd.InjectConfig(cfg)).To(Succeed())
+			storeExternal(domainIO, pathIO)
+			subCmd.options.DoDefaulting = true
+			subCmd.options.ExternalAPIDomain = domainIO
+
+			err := subCmd.InjectResource(res)
+
+			Expect(err).NotTo(HaveOccurred())
+			Expect(res.External).To(BeTrue())
+			Expect(res.Path).To(Equal(pathIO))
+			Expect(res.Domain).To(Equal(domainIO))
+		})
+
+		It("should refuse when --external-api-domain does not match and no path is given", func() {
+			Expect(subCmd.InjectConfig(cfg)).To(Succeed())
+			storeExternal(domainIO, pathIO)
+			subCmd.options.DoDefaulting = true
+			subCmd.options.ExternalAPIDomain = "cert-manager.k8s.io"
+
+			err := subCmd.InjectResource(res)
+
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("no resource matches --external-api-domain"))
+			Expect(err.Error()).To(ContainSubstring(domainIO))
+		})
+
+		It("should create a new external resource when the domain is new and a path is given", func() {
+			Expect(subCmd.InjectConfig(cfg)).To(Succeed())
+			storeExternal(domainIO, pathIO)
+			subCmd.options.DoDefaulting = true
+			subCmd.options.ExternalAPIDomain = "cert-manager.k8s.io"
+			subCmd.options.ExternalAPIPath = "github.com/cert-manager/cert-manager/pkg/apis/acme/v1"
+
+			err := subCmd.InjectResource(res)
+
+			Expect(err).NotTo(HaveOccurred())
+			Expect(res.External).To(BeTrue())
+			Expect(res.Domain).To(Equal("cert-manager.k8s.io"))
+		})
 	})
 
 	It("should resolve a core type without --external-api-domain", func() {
@@ -366,7 +503,7 @@ var _ = Describe("createWebhookSubcommand", func() {
 		Expect(cfg.AddResource(resource.Resource{
 			GVK:      res.GVK,
 			Core:     true,
-			Path:     "k8s.io/api/core/v1",
+			Path:     coreAPIPath,
 			Webhooks: &resource.Webhooks{},
 		})).To(Succeed())
 		subCmd.options.DoDefaulting = true