-
Notifications
You must be signed in to change notification settings - Fork 114
fix: reset previously set/removed values in per-backend header mutation #1387
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 8 commits
cc5c32e
19ecccc
31ce3b1
ba27892
04e1496
e0defcf
4d7bf86
b760c6e
5fea734
8daaed6
ccee34b
0fa251b
1e4ca5a
111db0c
1a31278
aae64fd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,7 @@ import ( | |
extprocv3 "github.com/envoyproxy/go-control-plane/envoy/service/ext_proc/v3" | ||
|
||
"github.com/envoyproxy/ai-gateway/internal/filterapi" | ||
"github.com/envoyproxy/ai-gateway/internal/internalapi" | ||
) | ||
|
||
type HeaderMutator struct { | ||
|
@@ -62,8 +63,8 @@ func (h *HeaderMutator) Mutate(headers map[string]string, onRetry bool) *extproc | |
} | ||
} | ||
|
||
// Restore original headers on retry, only if not being removed, set or not already present. | ||
if onRetry && h.originalHeaders != nil { | ||
if onRetry { | ||
// Restore original headers on retry, only if not being removed, set or not already present. | ||
for h, v := range h.originalHeaders { | ||
key := strings.ToLower(h) | ||
_, isRemoved := removedHeadersSet[key] | ||
|
@@ -76,7 +77,31 @@ func (h *HeaderMutator) Mutate(headers map[string]string, onRetry bool) *extproc | |
}) | ||
} | ||
} | ||
// 1. Remove any headers that were added in the previous attempt (not part of original headers and not being set now). | ||
// 2. Restore any original headers that were modified in the previous attempt (and not being set now). | ||
for key := range headers { | ||
key = strings.ToLower(key) | ||
|
||
// Skip Envoy AI Gateway headers since some of them are populated after the originalHeaders are captured. | ||
// This should be safe since these headers are managed by Envoy AI Gateway itself, not expected to be | ||
// modified by users via header mutation API. | ||
if strings.HasPrefix(key, internalapi.EnvoyAIGatewayHeaderPrefix) || strings.HasPrefix(key, ":") { | ||
continue | ||
} | ||
if _, set := setHeadersSet[key]; set { | ||
mathetake marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
continue | ||
} | ||
originalValue, exists := h.originalHeaders[key] | ||
if !exists { | ||
delete(headers, key) | ||
headerMutation.RemoveHeaders = append(headerMutation.RemoveHeaders, key) | ||
} else { | ||
// Restore original value. | ||
headers[key] = originalValue | ||
headerMutation.SetHeaders = append(headerMutation.SetHeaders, &corev3.HeaderValueOption{ | ||
Header: &corev3.HeaderValue{Key: key, RawValue: []byte(originalValue)}, | ||
mathetake marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}) | ||
} | ||
} | ||
} | ||
|
||
return headerMutation | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -769,100 +769,6 @@ func TestMessagesProcessorUpstreamFilter_ProcessRequestHeaders_WithHeaderMutatio | |
require.Equal(t, "custom-value", headers["x-custom"]) | ||
}) | ||
|
||
t.Run("header mutations restored on retry", func(t *testing.T) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same |
||
headers := map[string]string{ | ||
":path": "/anthropic/v1/messages", | ||
"x-ai-eg-model": "claude-3-sonnet", | ||
// "x-custom" is not present in current headers, so it can be restored. | ||
"x-new-header": "new-value", // Already set from previous mutation. | ||
} | ||
|
||
// Create request body. | ||
requestBody := &anthropicschema.MessagesRequest{ | ||
"model": "claude-3-sonnet", | ||
"max_tokens": 1000, | ||
"messages": []any{map[string]any{"role": "user", "content": "Hello"}}, | ||
} | ||
requestBodyRaw := []byte(`{"model": "claude-3-sonnet", "max_tokens": 1000, "messages": [{"role": "user", "content": "Hello"}]}`) | ||
|
||
// Create header mutations that don't remove x-custom (so it can be restored). | ||
headerMutations := &filterapi.HTTPHeaderMutation{ | ||
Remove: []string{"authorization", "x-api-key"}, | ||
Set: []filterapi.HTTPHeader{{Name: "x-new-header", Value: "updated-value"}}, | ||
} | ||
|
||
// Create mock translator. | ||
mockTranslator := mockAnthropicTranslator{ | ||
t: t, | ||
expRequestBody: requestBody, | ||
expForceRequestBodyMutation: true, // This is a retry request. | ||
retHeaderMutation: &extprocv3.HeaderMutation{}, | ||
retBodyMutation: &extprocv3.BodyMutation{}, | ||
retErr: nil, | ||
} | ||
|
||
// Create mock metrics. | ||
chatMetrics := metrics.NewMessagesFactory(noop.NewMeterProvider().Meter("test"), map[string]string{})() | ||
|
||
// Create processor. | ||
processor := &messagesProcessorUpstreamFilter{ | ||
config: &processorConfig{}, | ||
requestHeaders: headers, | ||
logger: slog.Default(), | ||
metrics: chatMetrics, | ||
translator: mockTranslator, | ||
originalRequestBody: requestBody, | ||
originalRequestBodyRaw: requestBodyRaw, | ||
handler: &mockBackendAuthHandler{}, | ||
onRetry: true, // This is a retry request. | ||
} | ||
|
||
// Use the same headers map as the original headers (this simulates the router filter's requestHeaders). | ||
originalHeaders := map[string]string{ | ||
":path": "/anthropic/v1/messages", | ||
"x-ai-eg-model": "claude-3-sonnet", | ||
"authorization": "bearer original-token", // This will be removed, so won't be restored. | ||
"x-api-key": "original-secret", // This will be removed, so won't be restored. | ||
"x-custom": "original-custom", // This won't be removed, so can be restored. | ||
"x-new-header": "original-value", // This will be set, so won't be restored. | ||
} | ||
processor.headerMutator = headermutator.NewHeaderMutator(headerMutations, originalHeaders) | ||
|
||
ctx := context.Background() | ||
response, err := processor.ProcessRequestHeaders(ctx, nil) | ||
|
||
require.NoError(t, err) | ||
require.NotNil(t, response) | ||
|
||
commonRes := response.Response.(*extprocv3.ProcessingResponse_RequestHeaders).RequestHeaders.Response | ||
|
||
// Check that header mutations were applied. | ||
require.NotNil(t, commonRes.HeaderMutation) | ||
// RemoveHeaders should be empty because authorization/x-api-key don't exist in current headers. | ||
require.Empty(t, commonRes.HeaderMutation.RemoveHeaders) | ||
require.Len(t, commonRes.HeaderMutation.SetHeaders, 2) // Updated header + restored header. | ||
|
||
// Check that x-custom header was restored on retry (it's not being removed or set). | ||
var restoredHeader *corev3.HeaderValueOption | ||
var updatedHeader *corev3.HeaderValueOption | ||
for _, h := range commonRes.HeaderMutation.SetHeaders { | ||
switch h.Header.Key { | ||
case "x-custom": | ||
restoredHeader = h | ||
case "x-new-header": | ||
updatedHeader = h | ||
} | ||
} | ||
require.NotNil(t, restoredHeader) | ||
require.Equal(t, []byte("original-custom"), restoredHeader.Header.RawValue) | ||
require.NotNil(t, updatedHeader) | ||
require.Equal(t, []byte("updated-value"), updatedHeader.Header.RawValue) | ||
|
||
// Check that headers were updated in the request headers. | ||
require.Equal(t, "updated-value", headers["x-new-header"]) | ||
require.Equal(t, "original-custom", headers["x-custom"]) | ||
}) | ||
|
||
t.Run("no header mutations when mutator is nil", func(t *testing.T) { | ||
headers := map[string]string{ | ||
":path": "/anthropic/v1/messages", | ||
|
@@ -1023,7 +929,7 @@ func TestMessagesProcessorUpstreamFilter_SetBackend_WithHeaderMutations(t *testi | |
|
||
// Test retry scenario - original headers should be restored. | ||
testHeaders := map[string]string{ | ||
"x-existing": "current-value", // This exists, so won't be restored. | ||
"x-existing": "current-value", | ||
} | ||
mutation := p.headerMutator.Mutate(testHeaders, true) // onRetry = true. | ||
|
||
|
@@ -1043,6 +949,6 @@ func TestMessagesProcessorUpstreamFilter_SetBackend_WithHeaderMutations(t *testi | |
require.Equal(t, []byte("original-value"), restoredHeader.Header.RawValue) | ||
require.Equal(t, "original-value", testHeaders["x-custom"]) | ||
// x-existing should not be restored because it already exists. | ||
mathetake marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
require.Equal(t, "current-value", testHeaders["x-existing"]) | ||
require.Equal(t, "existing-value", testHeaders["x-existing"]) | ||
}) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -61,10 +61,6 @@ func TestWithTestUpstream(t *testing.T) { | |
testUpstreamAzureBackend, | ||
testUpstreamGCPVertexAIBackend, | ||
testUpstreamGCPAnthropicAIBackend, | ||
// TODO: this shouldn't be needed. The previous per-backend headers shouldn't affect the subsequent retries. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This TODO summarizes what was happening with the bug being fixed here |
||
{Name: "testupstream-openai-always-200", Schema: openAISchema, HeaderMutation: &filterapi.HTTPHeaderMutation{ | ||
Set: []filterapi.HTTPHeader{{Name: testupstreamlib.ResponseStatusKey, Value: "200"}}, | ||
}}, | ||
{ | ||
Name: "testupstream-openai-5xx", Schema: openAISchema, HeaderMutation: &filterapi.HTTPHeaderMutation{ | ||
Set: []filterapi.HTTPHeader{{Name: testupstreamlib.ResponseStatusKey, Value: "500"}}, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -262,8 +262,8 @@ func requireEnvoy(t testing.TB, | |
"--concurrency", strconv.Itoa(max(runtime.NumCPU(), 2)), | ||
// This allows multiple Envoy instances to run in parallel. | ||
"--base-id", strconv.Itoa(time.Now().Nanosecond()), | ||
// Add debug logging for extproc. | ||
"--component-log-level", "ext_proc:trace,http:debug,connection:debug", | ||
// Add debug logging for http. | ||
"--component-log-level", "http:debug", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ext_proc trace is just a noise as well as connection stuff as well |
||
) | ||
|
||
// wait for the ready message or exit. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this simply re-tests the headermutator internal so it's overlapping with the tests there, hence removed it.