fix(go/v4): resolve create webhook target when several share 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
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.

Author: prash2512

pkg/plugins/golang/v4/webhook.go

@@ -241,29 +241,82 @@ func (p *createWebhookSubcommand) PostScaffold() error {
 	return nil
 }
 
-// updateResourceFromConfig copies existing resource configuration from PROJECT file.
+// updateResourceFromConfig fills res with the configuration recorded for its
+// Group/Version/Kind in the PROJECT file: Domain, Path, Plural, External, Core and Module.
+//
+// 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.
 func (p *createWebhookSubcommand) updateResourceFromConfig(res *resource.Resource) error {
-	// Match by Group, Version, and Kind because external APIs may have
-	// a different domain than the project domain.
 	resources, err := p.config.GetResources()
 	if err != nil {
 		return fmt.Errorf("failed to load resources from project configuration: %w", err)
 	}
 
-	for _, existingRes := range resources {
-		if existingRes.Group == res.Group &&
-			existingRes.Version == res.Version &&
-			existingRes.Kind == res.Kind {
-			p.resource.Domain = existingRes.Domain
-			p.resource.Path = existingRes.Path
-			p.resource.Plural = existingRes.Plural
-			p.resource.External = existingRes.External
-			p.resource.Core = existingRes.Core
-			p.resource.Module = existingRes.Module
-			break
+	// Collect every recorded resource sharing this Group/Version/Kind.
+	var matches []resource.Resource
+	for _, r := range resources {
+		if r.Group == res.Group && r.Version == res.Version && r.Kind == res.Kind {
+			matches = append(matches, r)
+		}
+	}
+
+	var existingRes resource.Resource
+	switch len(matches) {
+	case 0:
+		return nil // nothing recorded for this GVK; keep res as built from the flags
+	case 1:
+		existingRes = matches[0]
+	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
+				}
+			}
+		} 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
+				}
+			}
+		}
+		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, ", "),
+				)
+			}
+			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, ", "),
+			)
 		}
 	}
 
+	res.Domain = existingRes.Domain
+	res.Path = existingRes.Path
+	res.Plural = existingRes.Plural
+	res.External = existingRes.External
+	res.Core = existingRes.Core
+	res.Module = existingRes.Module
+
 	return nil
 }
 

pkg/plugins/golang/v4/webhook_test.go

@@ -223,6 +223,160 @@ var _ = Describe("createWebhookSubcommand", func() {
 		Expect(err.Error()).To(ContainSubstring("or pass --external-api-path for an external type"))
 	})
 
+	Context("when several external resources share the same Group/Version/Kind", func() {
+		const (
+			pathIO    = "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1"
+			pathK8sIO = "github.com/cert-manager/cert-manager/pkg/apis/acme/v1"
+			domainIO  = "cert-manager.io"
+			domainK8s = "cert-manager.k8s.io"
+		)
+
+		// storeExternal records an external resource sharing crewGroup/v1/captainKind but under
+		// the given domain. Recorded resources are unique by full GVK, so distinct domains produce
+		// distinct entries that collide only on Group/Version/Kind.
+		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())
+		}
+
+		// storedByDomain re-reads the recorded resources keyed by domain, to assert entries are
+		// left untouched.
+		storedByDomain := func() map[string]resource.Resource {
+			stored, err := cfg.GetResources()
+			Expect(err).NotTo(HaveOccurred())
+			byDomain := make(map[string]resource.Resource, len(stored))
+			for _, s := range stored {
+				byDomain[s.Domain] = s
+			}
+			return byDomain
+		}
+
+		It("should refuse and leave the PROJECT untouched when no domain is given", func() {
+			Expect(subCmd.InjectConfig(cfg)).To(Succeed())
+			storeExternal(domainIO, pathIO)
+			storeExternal(domainK8s, pathK8sIO)
+			subCmd.options.DoDefaulting = true
+
+			err := subCmd.InjectResource(res)
+
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("match more than one resource"))
+			Expect(err.Error()).To(ContainSubstring("pass --external-api-domain"))
+			Expect(err.Error()).To(ContainSubstring(domainIO))
+			Expect(err.Error()).To(ContainSubstring(domainK8s))
+
+			// Both entries keep their path and domain.
+			byDomain := storedByDomain()
+			Expect(byDomain).To(HaveLen(2))
+			Expect(byDomain[domainIO].Path).To(Equal(pathIO))
+			Expect(byDomain[domainK8s].Path).To(Equal(pathK8sIO))
+		})
+
+		It("should refuse regardless of the order the resources are recorded", func() {
+			Expect(subCmd.InjectConfig(cfg)).To(Succeed())
+			// Reverse order from the previous test: nothing may depend on file order.
+			storeExternal(domainK8s, pathK8sIO)
+			storeExternal(domainIO, pathIO)
+			subCmd.options.DoDefaulting = true
+
+			err := subCmd.InjectResource(res)
+
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("match more than one resource"))
+		})
+
+		It("should work on the resource named by --external-api-domain and leave the other alone", func() {
+			Expect(subCmd.InjectConfig(cfg)).To(Succeed())
+			storeExternal(domainIO, pathIO)
+			storeExternal(domainK8s, pathK8sIO)
+			subCmd.options.DoDefaulting = true
+			subCmd.options.ExternalAPIDomain = domainK8s
+
+			err := subCmd.InjectResource(res)
+
+			Expect(err).NotTo(HaveOccurred())
+			Expect(res.External).To(BeTrue())
+			Expect(res.Domain).To(Equal(domainK8s))
+			Expect(res.Path).To(Equal(pathK8sIO))
+
+			// The unnamed entry is untouched.
+			Expect(storedByDomain()[domainIO].Path).To(Equal(pathIO))
+		})
+
+		It("should refuse when the given domain matches none of them", func() {
+			Expect(subCmd.InjectConfig(cfg)).To(Succeed())
+			storeExternal(domainIO, pathIO)
+			storeExternal(domainK8s, pathK8sIO)
+			subCmd.options.DoDefaulting = true
+			subCmd.options.ExternalAPIDomain = "does-not-exist.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("does-not-exist.io"))
+			Expect(err.Error()).To(ContainSubstring(domainIO))
+			Expect(err.Error()).To(ContainSubstring(domainK8s))
+		})
+
+		It("should refuse without a domain even when one entry has an empty domain", func() {
+			Expect(subCmd.InjectConfig(cfg)).To(Succeed())
+			// An empty flag must not silently select the empty-domain entry.
+			storeExternal("", pathK8sIO)
+			storeExternal(domainIO, pathIO)
+			subCmd.options.DoDefaulting = true
+
+			err := subCmd.InjectResource(res)
+
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("match more than one resource"))
+		})
+
+		It("should prefer the non-external entry when no domain is given", func() {
+			Expect(subCmd.InjectConfig(cfg)).To(Succeed())
+			// A project (non-external) entry shares the GVK with an external one.
+			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())
+			storeExternal(domainK8s, pathK8sIO)
+			subCmd.options.DoDefaulting = true
+
+			err := subCmd.InjectResource(res)
+
+			// Resolves to the non-external entry, not the external one, without a domain.
+			Expect(err).NotTo(HaveOccurred())
+			Expect(res.External).To(BeFalse())
+		})
+	})
+
+	It("should resolve a core type without --external-api-domain", func() {
+		Expect(subCmd.InjectConfig(cfg)).To(Succeed())
+		// A single recorded core type must resolve without --external-api-domain.
+		Expect(cfg.AddResource(resource.Resource{
+			GVK:      res.GVK,
+			Core:     true,
+			Path:     "k8s.io/api/core/v1",
+			Webhooks: &resource.Webhooks{},
+		})).To(Succeed())
+		subCmd.options.DoDefaulting = true
+
+		err := subCmd.InjectResource(res)
+
+		Expect(err).NotTo(HaveOccurred())
+		Expect(res.Core).To(BeTrue())
+	})
+
 	Context("isValidVersion", func() {
 		BeforeEach(func() {
 			res = &resource.Resource{