fix: reset previously set/removed values in per-backend header mutation #1387
Conversation
Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
mathetake
changed the title
| 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.
This TODO summarizes what was happening with the bug being fixed here
| // 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.
ext_proc trace is just a noise as well as connection stuff as well
Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
| 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.
this simply re-tests the headermutator internal so it's overlapping with the tests there, hence removed it.
| require.Equal(t, "custom-value", headers["x-custom"]) | ||
| }) | ||
|
|
||
| t.Run("header mutations restored on retry", func(t *testing.T) { |
Codecov Report❌ Patch coverage is
❌ Your project status has failed because the head coverage (77.43%) is below the target coverage (86.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #1387 +/- ##
==========================================
+ Coverage 77.42% 77.43% +0.01%
==========================================
Files 131 131
Lines 16543 16569 +26
==========================================
+ Hits 12809 12831 +22
- Misses 3099 3101 +2
- Partials 635 637 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Takeshi Yoneda <[email protected]>
|
ok ready to go! |
| // 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) |
There was a problem hiding this comment.
Just TOL, should we merge this loop and the loop over original headers? If I understand this loop (if no removals or additions) will make headers equal to orignalHeaders. Should we start with headers = originalHeaders and go ahead with remaining mutations?
Initially I skipped this to avoid any header that was already added and was different from original header before entering mutation but as we are making them same now just wondering if extra loop is required
There was a problem hiding this comment.
Let's do it in a follow up later
**Description** fix site rendering Signed-off-by: Xiaolin Lin <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
6 participants
Description
This fixes a bug in the per-backend header mutator where the previous mutation in retry case causes incorrect header presence, absence or incorrect header values.