Skip to content

Conversation

@zhengkezhou1
Copy link
Member

@zhengkezhou1 zhengkezhou1 commented May 23, 2025

Description

  • Motivation: Add new configuration fields to allow customizing the full API endpoint for LLM providers. This enables more flexible integration with various LLM services by allowing custom endpoint.
  • What changed: Add PathOverride and AuthHeaderOverride fields to LLMProvider.
  • Related issues: Fixes hostOverride setting causes incorrect requests in AI Gateway #11225

Change Type

/kind new_feature

Changelog

Add `PathOverride` and `AuthHeaderOverride` fields for custom LLM provider endpoints

Additional Notes

@github-actions github-actions bot added kind/feature Categorizes issue or PR as related to a new feature. release-note labels May 23, 2025
@zhengkezhou1
Copy link
Member Author

I will add E2E tests soon...

}

model := "gpt-4"
//TODO: Verify path override is correctly applied in transformation template
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, a unit test at the plugin level would be good to add here. You can also test the output envoy config by adding a setup test similar to how this deepseek test is structured: https://github.com/kgateway-dev/kgateway/blob/main/internal/kgateway/setup/testdata/standard/ai-deepseek-prompt-guard.yaml

If you run the setup tests with the input config defined, it will write the output file for you. This can be a quick way to validate the path replacement is correct!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah! Nevermind I see you've already created internal/kgateway/setup/testdata/standard/ai-custom-url-out.yaml 🎉

@zhengkezhou1 zhengkezhou1 changed the title feat: Introduce PathOverride field for AI Backend custom API path feat: add FullPathOverride field for custom LLM provider endpoints May 26, 2025
@zhengkezhou1
Copy link
Member Author

It works after adding the following:

...
fullPathOverride:
    path: "/api/v1/chat/completions"
...

image

@zhengkezhou1 zhengkezhou1 force-pushed the feat/introduce-pathOverride-field branch from 4d6e317 to 6f0f689 Compare May 26, 2025 16:01
@zhengkezhou1 zhengkezhou1 force-pushed the feat/introduce-pathOverride-field branch from 6f0f689 to 5015b12 Compare May 29, 2025 15:52
@zhengkezhou1 zhengkezhou1 changed the title feat: add FullPathOverride field for custom LLM provider endpoints feat: Allow overriding default LLM provider endpoints May 29, 2025
Comment on lines 316 to 319
path := "/api/v1/chat/completions"
prefix := "Bearer"
header := "Authorization"
//TODO: Verify path override is correctly applied in transformation template
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In current testing, it seems that there is no way to validate these new inputs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can confirm this at the setup_test level by checking the path is correctly replaced!

@zhengkezhou1 zhengkezhou1 force-pushed the feat/introduce-pathOverride-field branch 2 times, most recently from f0bdb92 to 8f6e334 Compare May 30, 2025 11:18
@zhengkezhou1 zhengkezhou1 marked this pull request as draft May 30, 2025 11:20
@zhengkezhou1 zhengkezhou1 force-pushed the feat/introduce-pathOverride-field branch from 8f6e334 to b0c7c3c Compare May 30, 2025 12:21
@zhengkezhou1 zhengkezhou1 marked this pull request as ready for review May 30, 2025 13:08
@zhengkezhou1 zhengkezhou1 force-pushed the feat/introduce-pathOverride-field branch 2 times, most recently from fb4e7c0 to 1ac7470 Compare May 30, 2025 17:44
Comment on lines 46 to 47
// Only supported for OpenAI and Anthropic compatible APIs for now.
FullPath *string `json:"path"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Only supported for OpenAI and Anthropic compatible APIs for now.
FullPath *string `json:"path"`
FullPath *string `json:"fullPath"`

base_url=TEST_OPENAI_BASE_URL,
default_headers={"custom-header":"custom-prefix"} if overrideProvider else None,
api_key=None if overrideProvider else "passthrough" if passthrough else "FAKE",
base_url=TEST_OPENAI_BASE_URL if overrideProvider else TEST_OVERRIDE_BASE_URL,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe I made an error; it should be base_url = TEST_OVERRIDE_BASE_URL if overrideProvider else TEST_OPENAI_BASE_URL. 😳

@npolshakova @andy-fong Additionally, should we test Routing, Streaming, PromptGuard, etc., under the condition of TEST_OVERRIDE_PROVIDER? 🤔 Please let me know. Thank you!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of the PromptGuard settings should change the path, so it's fine to skip adding a test there.

Gemini and VertexAI change the path depending on whether streaming is enabled or not, but OpenAI and other providers just use stream: true in the body. Since we're now always overriding the path if the user sets the override here, let's add a small streaming test with gemini to show that the override path is set correctly in that case.

Copy link
Contributor

@andy-fong andy-fong Jun 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zhengkezhou1 again, thank you for taking the time for the contribution. That's a good question and I understand why you use an env to set an override base url; however, the various TEST_*_BASE_URL are meant to test the different providers on the various features. Adding a TEST_OVERRIDE_BASE_URL and following the test "pattern" lead you to the question of should you also add tests for Routing, Streaming, PromptGuard ...etc. I think you are on the right thinking but it's also a red flag because it basically will double all the tests we need to write for all existing and future features.

So, step back and think about it. This override applies to all providers; however, it's not going to change the response from the LLM providers for various features. So, instead of adding the path override tests to all existing test suites, it might be better to test this as a separate feature for each provider once. That way, you would just follow the existing test pattern and just use the TEST_OPENAI_BASE_URL for example and have a new yaml file under testdata to turn on this path override feature for each provider. However, for upstream, you don't need a mock LLM response but rather can use something like http-echo to return you the request headers and path as a json object, then the test just verify the request path is the one we expect.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I agree with Andy. Let's keep the current approach with TEST_OVERRIDE_BASE_URL for now for just the routing e2e test, then as a follow up let's clean up the tests and create a standalone test suite for the path override.

The follow up test PR should have a separate test for each provider once (keep original TEST_OPENAI_BASE_URL and create a new testdata file with values to use).

@zhengkezhou1 zhengkezhou1 force-pushed the feat/introduce-pathOverride-field branch from 1307393 to 359a03b Compare June 2, 2025 11:25
Signed-off-by: zhengkezhou1 <[email protected]>
@zhengkezhou1 zhengkezhou1 force-pushed the feat/introduce-pathOverride-field branch from 359a03b to c8a4d90 Compare June 2, 2025 12:09
Signed-off-by: zhengkezhou1 <[email protected]>
Copy link
Contributor

@npolshakova npolshakova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Let's just add the streaming test and this looks good to go!

Let's handle this in a follow up PR: #11282 (comment)

@npolshakova npolshakova added this pull request to the merge queue Jun 3, 2025
Merged via the queue into kgateway-dev:main with commit 1c721e1 Jun 3, 2025
20 checks passed
@zhengkezhou1 zhengkezhou1 deleted the feat/introduce-pathOverride-field branch July 2, 2025 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/feature Categorizes issue or PR as related to a new feature. release-note

Projects

None yet

Development

Successfully merging this pull request may close these issues.

hostOverride setting causes incorrect requests in AI Gateway

3 participants