feat: Refactor discovery logic

[alexandernorth]

What this PR does / why we need it:
As requested in #2854, this PR refactors the discovery process to allow for the addition of APIService discovery.

How does this change affect the cardinality of KSM: (increases, decreases or does not change cardinality)
None

Which issue(s) this PR fixes: (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged)

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: alexandernorth
Once this PR has been reviewed and has the lgtm label, please assign dgrisonnet 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

[k8s-ci-robot]

This issue is currently awaiting triage.

If kube-state-metrics contributors determine this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

[k8s-triage-robot]

Unknown CLA label state. Rechecking for CLA labels.

Send feedback to sig-contributor-experience at kubernetes/community.

/check-cla
/easycla

[Copilot]

Pull request overview

This pull request refactors the custom resource discovery logic in preparation for adding APIService discovery support (as outlined in PR #2854). The refactoring introduces an abstraction layer that separates the discovery mechanism from the resource extraction logic, enabling multiple discovery sources to coexist.

Changes:

• Introduced an extractor interface pattern to decouple resource discovery from extraction logic
• Refactored CRDiscoverer to use source-based tracking instead of nested map structure
• Changed from direct field access to constructor pattern and interface-based access for stop channels
• Updated metric naming and help text to reflect generic "Custom Resource discovery" instead of CRD-specific terminology

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 11 comments.

Show a summary per file

File	Description
internal/discovery/types.go	Complete refactoring of CRDiscoverer with new source-based resource tracking, added extractor interface, and improved encapsulation
internal/discovery/gvk_extractors.go	New file implementing crdExtractor for extracting resources from CRD objects
internal/discovery/discovery.go	Refactored discovery logic to use extractor pattern and improved goroutine handling
internal/discovery/discovery_test.go	Updated tests to work with new API, added comprehensive test cases for new functionality
pkg/app/server.go	Updated to use NewCRDiscoverer constructor and renamed metric variables for clarity
internal/store/builder.go	Changed from direct map access to StopChanProvider interface with proper error handling
pkg/builder/types/interfaces.go	Added StopChanProvider interface for cleaner abstraction
pkg/customresourcestate/config.go	Updated method call from ResolveGVKToGVKPs to Resolve

Comments suppressed due to low confidence (1)

internal/discovery/discovery_test.go:365

• The tests don't cover concurrent access scenarios. Given that CRDiscoverer will be accessed concurrently (informer callbacks updating, Resolve/GetStopChan reading), it would be valuable to add tests that verify thread-safety, especially around the UpdateSource/DeleteSource operations that close channels while GetStopChan might be reading them.

func TestCheckAndResetUpdated(t *testing.T) {
	discoverer := newTestCRDiscoverer()

	// Initially not updated
	if discoverer.CheckAndResetUpdated() {
		t.Fatal("expected wasUpdated to be false initially")
	}

	// Add a resource
	discoverer.UpdateSource("crd:testobjects.testgroup", []*DiscoveredResource{
		{
			GroupVersionKind: schema.GroupVersionKind{Group: "testgroup", Version: "v1", Kind: "TestObject1"},
			Plural:           "testobjects1",
		},
	})

	// Should be updated now
	if !discoverer.CheckAndResetUpdated() {
		t.Fatal("expected wasUpdated to be true after UpdateSource")
	}

	// Should be reset
	if discoverer.CheckAndResetUpdated() {
		t.Fatal("expected wasUpdated to be false after CheckAndResetUpdated")
	}
}

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[alexandernorth]

@mrueg firstly thank you for starting to look into this. I apologise for taking a while to address the comments but I have now updated the PR with the feedback and left comments.

Please let me know if I can do anything else to help make this ready for merge.

@dgrisonnet I also add you as you looked into it as part of #2854 before I split the PRs

[Copilot]

Pull request overview

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

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.