feat(allowlist): support both v1 and v1alpha2 InferencePool APIs with flag #474

googs1025 · 2025-11-21T08:24:09Z

add new pool-group flag to choose which inferencepool to support

pierDipi

/lgtm

pierDipi · 2025-11-21T10:31:46Z

pkg/sidecar/proxy/allowlist.go

+		if errors.IsNotFound(err) {
+			return false, nil // GroupVersion not supported
+		}
+		return false, fmt.Errorf("failed to discover resources for %s: %w", gvr.GroupVersion(), err)


nit: as the function is generic and doesn't necessarily assume InfPool resource

Suggested change

return false, fmt.Errorf("failed to discover resources for %s: %w", gvr.GroupVersion(), err)

return false, fmt.Errorf("failed to discover resources for %s: %w", gvr.String(), err)

shmuelk · 2025-11-23T15:09:21Z

@googs1025 If I understand this PR correctly, it is trying to figure which version of the InferencePool CRD is installed.

What happens if both CRDs are installed? I believe make env-dev-kind installs both versions of the InferencePool CRD. It actually uses the v1 version, but both CRDs are installed.

googs1025 · 2025-11-24T00:45:19Z

@googs1025 If I understand this PR correctly, it is trying to figure which version of the InferencePool CRD is installed.

What happens if both CRDs are installed? I believe make env-dev-kind installs both versions of the InferencePool CRD. It actually uses the v1 version, but both CRDs are installed.

@shmuelk thanks for feed back.

This PR handles that scenario explicitly by using a prioritized candidate list:

var candidateGVRs = []schema.GroupVersionResource{
	{Group: "inference.networking.k8s.io", Version: "v1", Resource: inferencePoolResource},                // ← Preferred
	{Group: "inference.networking.x-k8s.io", Version: "v1alpha2", Resource: inferencePoolResource}, // ← Fallback
}

This logic use this list in order and picks the first GVR that is supported by the cluster. So if both CRDs exist, it will always prefer v1 API (inference.networking.k8s.io/v1) and only fall back to v1alpha2 if v1 is not present.

This matches the behavior of upstream components like the Ingress Gateway (IGW), which also support both versions but prioritize the stable one.

zdtsw · 2025-11-25T09:59:49Z

pkg/sidecar/proxy/allowlist.go

+//
+// This approach aligns with upstream Ingress Gateway (IGW) behavior, which also supports
+// both API versions concurrently (see issue #462).
+var candidateGVRs = []schema.GroupVersionResource{


isGVRSupported() is generic function, should candidateGVRs be more specific as it is only on inferencepools for now

change to inferencePoolGVRCandidates

zdtsw · 2025-11-26T07:38:11Z

/lgtm

github-actions · 2025-11-26T07:38:21Z

Cannot apply the lgtm label because Error: zdtsw is not included in the reviewers role in the OWNERS file

shmuelk · 2025-11-26T11:06:31Z

pkg/sidecar/proxy/allowlist.go

-		Resource: inferencePoolResource,
-	}
+	av.logger.Info("starting SSRF protection allowlist validator",
+		"namespace", av.namespace, "poolName", av.poolName, "gvr", av.gvr.String())


av.gvr is never set

oh... my bad. done

shmuelk · 2025-11-26T11:12:17Z

@googs1025 Thank you for your continued efforts on this PR.

I added an additional comment on the code as it seemed that something was missing in the code.

Additionally, I have an issue with the overall design of the code.

You check what CRDs are installed. If the InferencePool V1 CRD is installed, you watch on V1 InferencePool objects. If that CRD isn't installed, you will watch on v1alpha2 InferencePool objects. Bottom line you watch on one of the two possible types.

The problem is in the case where the user has both CRDs installed and for a variety of reasons is still using the v1alpha2 version of the InferencePool.

I see two possible solutions:

Create watcher's on all version's of the InferencePool that are found.
Remove the discovery code and simply add a command line parameter in which the user tells you which InferencePool type they are using. The default should be the V1 version. This BTW is exactly what the GIE does.

googs1025 · 2025-11-26T13:32:17Z

@googs1025 Thank you for your continued efforts on this PR.

I added an additional comment on the code as it seemed that something was missing in the code.

Additionally, I have an issue with the overall design of the code.

You check what CRDs are installed. If the InferencePool V1 CRD is installed, you watch on V1 InferencePool objects. If that CRD isn't installed, you will watch on v1alpha2 InferencePool objects. Bottom line you watch on one of the two possible types.

The problem is in the case where the user has both CRDs installed and for a variety of reasons is still using the v1alpha2 version of the InferencePool.

I see two possible solutions:

Create watcher's on all version's of the InferencePool that are found.

Remove the discovery code and simply add a command line parameter in which the user tells you which InferencePool type they are using. The default should be the V1 version. This BTW is exactly what the GIE does.

thanks for this great feed back 😄 this make more sense to me. We should align with GIE upstream.
use option 2.

Remove the discovery code and simply add a command line parameter in which the user tells you which InferencePool type they are using. The default should be the V1 version. This BTW is exactly what the GIE does.

shmuelk · 2025-11-27T11:11:40Z

@googs1025 This looks much better. You have a lint failure. Please add a comment for the exported field.

… auto-discovery Signed-off-by: CYJiang <googs1025@gmail.com>

shmuelk · 2025-11-27T12:47:21Z

/lgtm
/approve

github-project-automation bot added this to llm-d-inference-scheduler Nov 21, 2025

pierDipi approved these changes Nov 21, 2025

View reviewed changes

googs1025 force-pushed the fix/allowlistValidator branch 3 times, most recently from 70ebbf1 to cf90c1e Compare November 21, 2025 12:57

googs1025 force-pushed the fix/allowlistValidator branch 2 times, most recently from 5da9581 to 5403cbf Compare November 24, 2025 05:49

zdtsw reviewed Nov 25, 2025

View reviewed changes

googs1025 force-pushed the fix/allowlistValidator branch from 5403cbf to 24df901 Compare November 26, 2025 00:42

shmuelk requested changes Nov 26, 2025

View reviewed changes

github-project-automation bot moved this to In review in llm-d-inference-scheduler Nov 26, 2025

googs1025 force-pushed the fix/allowlistValidator branch from 24df901 to 29a9d31 Compare November 26, 2025 13:31

googs1025 changed the title ~~feat(allowlist): support both v1 and v1alpha2 InferencePool APIs with auto-discovery~~ feat(allowlist): support both v1 and v1alpha2 InferencePool APIs with flag Nov 27, 2025

googs1025 force-pushed the fix/allowlistValidator branch from 29a9d31 to a6ceb58 Compare November 27, 2025 03:56

feat(allowlist): support both v1 and v1alpha2 InferencePool APIs with…

2a65147

… auto-discovery Signed-off-by: CYJiang <googs1025@gmail.com>

googs1025 force-pushed the fix/allowlistValidator branch from a6ceb58 to 2a65147 Compare November 27, 2025 11:30

shmuelk approved these changes Nov 27, 2025

View reviewed changes

github-actions bot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 27, 2025

github-actions bot approved these changes Nov 27, 2025

View reviewed changes

github-actions bot merged commit e11b6f1 into llm-d:main Nov 27, 2025
6 checks passed

github-project-automation bot moved this from In review to Done in llm-d-inference-scheduler Nov 27, 2025

	return false, fmt.Errorf("failed to discover resources for %s: %w", gvr.GroupVersion(), err)
	return false, fmt.Errorf("failed to discover resources for %s: %w", gvr.String(), err)

feat(allowlist): support both v1 and v1alpha2 InferencePool APIs with flag #474

feat(allowlist): support both v1 and v1alpha2 InferencePool APIs with flag #474

Uh oh!

Conversation

googs1025 commented Nov 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pierDipi left a comment

Choose a reason for hiding this comment

Uh oh!

pierDipi Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

googs1025 Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

shmuelk commented Nov 23, 2025

Uh oh!

googs1025 commented Nov 24, 2025

Uh oh!

zdtsw Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

googs1025 Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

zdtsw commented Nov 26, 2025

Uh oh!

github-actions bot commented Nov 26, 2025

Uh oh!

shmuelk Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

googs1025 Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

shmuelk commented Nov 26, 2025

Uh oh!

googs1025 commented Nov 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shmuelk commented Nov 27, 2025

Uh oh!

shmuelk commented Nov 27, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

googs1025 commented Nov 21, 2025 •

edited

Loading

googs1025 commented Nov 26, 2025 •

edited

Loading