extproc: custom processors per path and serve /v1/models #325

nacx · 2025-02-11T16:30:21Z

Commit Message

extproc: custom processors per path and serve /v1/models

Refactors the server processing to allow registering custom Processors for different request paths,
and adds a custom processor for requests to /v1/models that returns an immediate response based
on the models that are configured in the filter configuration.

Related Issues/PRs (if applicable)

Related discussion: #186

Special notes for reviewers (if applicable)

~~Opening as a draft to discuss on the proposed approach.~~

In order to return a direct response for some paths, we need to be able to customize the behaviour. Instead of messing
up with the translator, this PR proposes a change to refactor the Server and allow it to define custom processors per request path. This would make it easier to extend the functionality of the filter in the future, when more endpoints are supported.

This is used in the current PR to serve the /v1/models endpoint and return a computed list of models based on the configured routes. Right now we take advantage of exact header matching being the only option, but should be good enough for a first implementation.

~~The current PR doesn't add a feature flag to enable/disable this endpoint handling, but it should be easy to add if we feel the approach proposed in this PR is good.~~

nacx · 2025-02-11T16:31:30Z

cmd/extproc/mainlib/main.go

@@ -89,10 +89,12 @@ func Main() {
 		log.Fatalf("failed to listen: %v", err)
 	}

-	server, err := extproc.NewServer[*extproc.Processor](l, extproc.NewProcessor)
+	server, err := extproc.NewServer(l)


The generics were tying the server to a concrete processor type, but now it allows defining custom processors per request path, so the generics made no sense anymore. I've removed them and kept the code using the ProcessorIface

nacx · 2025-02-11T16:32:41Z

internal/extproc/chatcompletion_processor.go

+)
+
+// NewChatCompletionProcessor creates a new processor for the chat completino requests
+func NewChatCompletionProcessor(config *processorConfig, logger *slog.Logger) ProcessorIface {


This is the Processor object that I've renamed to ChatCompletionProcessor as now we can have different processors per request path. The contents are just a verbatim copy (modulo the type name change).

nacx · 2025-02-11T16:35:13Z

internal/extproc/server.go

@@ -125,6 +160,17 @@ func (s *Server[P]) process(p P, stream extprocv3.ExternalProcessor_ProcessServe
 			return status.Errorf(codes.Unknown, "cannot receive stream request: %v", err)
 		}

+		// If we're processing the request headers, read the :path header to instantiate the
+		// right processor.
+		if headers := req.GetRequestHeaders().GetHeaders(); headers != nil {


This is the main change in this PR.

In order to allow having custom processors per-request path we need to access the :path header. We can't instantiate the ProcessorIface object until we've read the first request headers message. This is why I instantiate the processor here, and reuse it for the following messages.

mathetake

I think I like the general direction! One question: then we don't need translator.Factory but instead maybe need concrete translator.NewChatCompletionTranslator, etc since it's currently translator.Factory takes path as an input?

nacx · 2025-02-11T19:33:22Z

I think that's correct. We don't need the factory as it is today, but we still need to let the processor decide what translator to use, depending on the selected backend. I can refactor and clean that up in a follow-up PR if that makes sense?

mathetake · 2025-02-11T19:47:19Z

yeah let's do a follow up refactor - that makes sense

internal/extproc/processor.go

cmd/extproc/mainlib/main.go

mathetake

Only nits!!!

cmd/extproc/mainlib/main.go

internal/apischema/openai/openai.go

internal/apischema/openai/openai_test.go

internal/extproc/chatcompletion_processor.go

internal/extproc/models_processor.go

mathetake · 2025-02-12T17:37:09Z

internal/extproc/server.go

@@ -251,3 +298,17 @@ func filterSensitiveBody(resp *extprocv3.ProcessingResponse, logger *slog.Logger
 	}
 	return filteredResp
 }
+
+func getHeader(headers *corev3.HeaderMap, name string) string {


nit: so basically i see we (unnecessarily) iterate through all headers here and headersToMap twice. would it be possible to use headersToMap before the processor selection and pass the constructed map to processorForPath and eventually to newProcessor?

I thought about this, but was unsure about modifying the processor factory methods to accept a request headers map. I can change that if you feel it is OK

yeah let's do change

mathetake · 2025-02-12T17:40:14Z

internal/extproc/models_processor.go

+// configuration.
+// Since it returns an immediate response after processing the headers, the rest of the methods of the
+// ProcessorIface are not implemented. Those should never be called.
+type ModelsProcessor struct {


nit does this need to be exported

…n the filter config Signed-off-by: Ignasi Barrera <[email protected]>

nacx · 2025-02-12T21:29:39Z

OK, I've addressed all the review comments and rebased to the latest main. I squashed the commits because otherwise there were too many conflicts to resolve. I hope it doesn't complicate much the final review.

mathetake · 2025-02-12T21:50:16Z

internal/extproc/server.go

+		// If we're processing the request headers, read the :path header to instantiate the
+		// right processor.
+		if headers := req.GetRequestHeaders().GetHeaders(); headers != nil {
+			p, err = s.processorForPath(headersToMap(headers))


I think this will create a new p each time the grpc stream receives a new message? maybe this block should be conditioned on if p != nil { ... } ?

ah headers := req.GetRequestHeaders().GetHeaders(); headers != nil implicitly does that - maybe I would like a comment at least on this

The req.GetRequestHeaders() will only be non-nil when it is a RequestHeaders message, so I think this will be executed only once? Or does ext_proc, for the same request, send N messages of type ProcessingRequest_RequestHeaders ?

func (x *ProcessingRequest) GetRequestHeaders() *HttpHeaders { if x, ok := x.GetRequest().(*ProcessingRequest_RequestHeaders); ok { return x.RequestHeaders } return nil }

I'll add a comment :)

Signed-off-by: Ignasi Barrera <[email protected]>

mathetake

LGTM!

**Commit Message** extproc: remove the path from the translator factory Removes the path from the translator factory, now that there is a dedicated processor for the chat completion endpoint. **Related Issues/PRs (if applicable)** Follow-up for: #325 (review) **Special notes for reviewers (if applicable)** Note that I don't remove the `Factory` type completely so that only the right translator is instantiated and only when needed. --------- Signed-off-by: Ignasi Barrera <[email protected]>

**Commit Message** e2e: add test for the models endpoint using the openai client **Related Issues/PRs (if applicable)** Related to #325 **Special notes for reviewers (if applicable)** N/A --------- Signed-off-by: Ignasi Barrera <[email protected]>

**Commit Message** This removes the unnecessary suffix "Iface" from extproc.ProcessorIface interface. **Related Issues/PRs (if applicable)** Follow up on #325 Signed-off-by: Takeshi Yoneda <[email protected]>

**Commit Message** extproc: decouple router package from api paths This is a follow-up change to decouple the router package from the API paths. Now that we have specialized processors per API path it does not make sense for the router package to have path-related logic. Now each processor is responsible for instantiating the right body parser for the path they're processing. **Related Issues/PRs (if applicable)** Follow-up for #325 and #334 **Special notes for reviewers (if applicable)** N/A --------- Signed-off-by: Ignasi Barrera <[email protected]> Co-authored-by: Takeshi Yoneda <[email protected]>

) **Commit Message** extproc: custom processors per path and serve /v1/models Refactors the server processing to allow registering custom Processors for different request paths, and adds a custom processor for requests to `/v1/models` that returns an immediate response based on the models that are configured in the filter configuration. **Related Issues/PRs (if applicable)** Related discussion: envoyproxy#186 --------- Signed-off-by: Ignasi Barrera <[email protected]> Signed-off-by: Loong <[email protected]>

**Commit Message** extproc: remove the path from the translator factory Removes the path from the translator factory, now that there is a dedicated processor for the chat completion endpoint. **Related Issues/PRs (if applicable)** Follow-up for: envoyproxy#325 (review) **Special notes for reviewers (if applicable)** Note that I don't remove the `Factory` type completely so that only the right translator is instantiated and only when needed. --------- Signed-off-by: Ignasi Barrera <[email protected]> Signed-off-by: Loong <[email protected]>

**Commit Message** e2e: add test for the models endpoint using the openai client **Related Issues/PRs (if applicable)** Related to envoyproxy#325 **Special notes for reviewers (if applicable)** N/A --------- Signed-off-by: Ignasi Barrera <[email protected]> Signed-off-by: Loong <[email protected]>

**Commit Message** This removes the unnecessary suffix "Iface" from extproc.ProcessorIface interface. **Related Issues/PRs (if applicable)** Follow up on envoyproxy#325 Signed-off-by: Takeshi Yoneda <[email protected]> Signed-off-by: Loong <[email protected]>

**Commit Message** extproc: decouple router package from api paths This is a follow-up change to decouple the router package from the API paths. Now that we have specialized processors per API path it does not make sense for the router package to have path-related logic. Now each processor is responsible for instantiating the right body parser for the path they're processing. **Related Issues/PRs (if applicable)** Follow-up for envoyproxy#325 and envoyproxy#334 **Special notes for reviewers (if applicable)** N/A --------- Signed-off-by: Ignasi Barrera <[email protected]> Co-authored-by: Takeshi Yoneda <[email protected]> Signed-off-by: Loong <[email protected]>

nacx commented Feb 11, 2025

View reviewed changes

nacx changed the title ~~extproc: handle requests to /v1/models based on the declared models in the filter config~~ extproc: custom processors per path and serve /v1/models Feb 11, 2025

nacx force-pushed the models branch from 9d77236 to f0c290c Compare February 11, 2025 16:38

mathetake reviewed Feb 11, 2025

View reviewed changes

nacx commented Feb 11, 2025

View reviewed changes

internal/extproc/processor.go Outdated Show resolved Hide resolved

nacx force-pushed the models branch from 31da2cc to 2fb8c08 Compare February 11, 2025 23:35

nacx marked this pull request as ready for review February 11, 2025 23:35

nacx requested a review from a team as a code owner February 11, 2025 23:35

mathetake reviewed Feb 11, 2025

View reviewed changes

cmd/extproc/mainlib/main.go Outdated Show resolved Hide resolved

nacx force-pushed the models branch 2 times, most recently from 125b6bc to 9fa41ef Compare February 12, 2025 09:16

mathetake reviewed Feb 12, 2025

View reviewed changes

nacx force-pushed the models branch from e3f0359 to 195129e Compare February 12, 2025 21:28

extproc: handle requests to /v1/models based on the declared models i…

45db636

…n the filter config Signed-off-by: Ignasi Barrera <[email protected]>

nacx force-pushed the models branch from 195129e to 45db636 Compare February 12, 2025 21:28

mathetake reviewed Feb 12, 2025

View reviewed changes

add comment

4f180f1

Signed-off-by: Ignasi Barrera <[email protected]>

mathetake approved these changes Feb 12, 2025

View reviewed changes

mathetake merged commit f07a7ff into envoyproxy:main Feb 12, 2025
17 checks passed

nacx deleted the models branch February 12, 2025 22:04

nacx mentioned this pull request Feb 12, 2025

extproc: remove the path from the translator factory #334

Merged

nacx mentioned this pull request Feb 13, 2025

e2e: add models test using the openai client #338

Merged

mathetake mentioned this pull request Feb 14, 2025

extproc: removes Iface suffix from processor #342

Merged

nacx mentioned this pull request Feb 17, 2025

extproc: decouple router package from api paths #352

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

extproc: custom processors per path and serve /v1/models #325

extproc: custom processors per path and serve /v1/models #325

nacx commented Feb 11, 2025 •

edited

Loading

nacx Feb 11, 2025

nacx Feb 11, 2025 •

edited

Loading

nacx Feb 11, 2025

mathetake left a comment

nacx commented Feb 11, 2025

mathetake commented Feb 11, 2025

mathetake left a comment

mathetake Feb 12, 2025 •

edited

Loading

nacx Feb 12, 2025

mathetake Feb 12, 2025

mathetake Feb 12, 2025

nacx commented Feb 12, 2025

mathetake Feb 12, 2025

mathetake Feb 12, 2025

nacx Feb 12, 2025 •

edited

Loading

nacx Feb 12, 2025

nacx Feb 12, 2025

mathetake left a comment

extproc: custom processors per path and serve /v1/models #325

extproc: custom processors per path and serve /v1/models #325

Conversation

nacx commented Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

nacx Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathetake left a comment

Choose a reason for hiding this comment

nacx commented Feb 11, 2025

mathetake commented Feb 11, 2025

mathetake left a comment

Choose a reason for hiding this comment

mathetake Feb 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nacx commented Feb 12, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nacx Feb 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathetake left a comment

Choose a reason for hiding this comment

nacx commented Feb 11, 2025 •

edited

Loading

nacx Feb 11, 2025 •

edited

Loading

mathetake Feb 12, 2025 •

edited

Loading

nacx Feb 12, 2025 •

edited

Loading