Skip to content

filters/builtin: drop specified header value #3457

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.

Sign up for GitHub

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
+127 −51
Draft

filters/builtin: drop specified header value #3457

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion docs/reference/filters.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,18 +163,25 @@ but appends the provided value to the already existing ones.

### dropRequestHeader

Removes a header from the request
Removes a header or a specific value from the request.

Parameters:

* header name (string)
* header value (string) - optional

Example:

```
foo: * -> dropRequestHeader("User-Agent") -> "https://backend.example.org";
```

Drop exactly matching value and keep others:

```
foo: * -> dropRequestHeader("Connection", "Upgrade") -> "https://backend.example.org";
```

### modResponseHeader

Same as [modRequestHeader](#modrequestheader), only for responses
Expand Down
151 changes: 104 additions & 47 deletions filters/builtin/header_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"strings"
"testing"

"github.com/stretchr/testify/assert"
"github.com/zalando/skipper/eskip"
"github.com/zalando/skipper/filters"
"github.com/zalando/skipper/filters/filtertest"
Expand Down Expand Up @@ -85,12 +86,7 @@ func testHeaders(t *testing.T, got, expected http.Header) {
delete(got, n)
}
}

if !compareHeaders(got, expected) {
printHeader(t, expected, "invalid header", "expected")
printHeader(t, got, "invalid header", "got")
t.Error("invalid header")
}
assert.Equal(t, expected, got)
}

func TestHeader(t *testing.T) {
Expand Down Expand Up @@ -132,10 +128,11 @@ func TestHeader(t *testing.T) {
requestHeader: http.Header{"X-Test-Name": []string{"value0", "value1"}},
expectedHeader: http.Header{"X-Test-Request-Name": []string{"value"}},
}, {
msg: "set outgoing host on set",
args: []interface{}{"Host", "www.example.org"},
valid: true,
host: "www.example.org",
msg: "set outgoing host on set",
args: []interface{}{"Host", "www.example.org"},
valid: true,
host: "www.example.org",
expectedHeader: http.Header{},
}, {
msg: "set request header from path params",
args: []interface{}{"X-Test-Name", "Mit ${was} zu ${wo}"},
Expand All @@ -161,10 +158,11 @@ func TestHeader(t *testing.T) {
requestHeader: http.Header{"X-Test-Name": []string{"value0", "value1"}},
expectedHeader: http.Header{"X-Test-Request-Name": []string{"value0", "value1", "value"}},
}, {
msg: "append outgoing host on set",
args: []interface{}{"Host", "www.example.org"},
valid: true,
host: "www.example.org",
msg: "append outgoing host on set",
args: []interface{}{"Host", "www.example.org"},
valid: true,
host: "www.example.org",
expectedHeader: http.Header{},
}, {
msg: "append request header from path params",
args: []interface{}{"X-Test-Name", "a ${foo}ter"},
Expand All @@ -186,19 +184,46 @@ func TestHeader(t *testing.T) {
expectedHeader: http.Header{"X-Test-Request-Name": []string{"Value"}},
}},
"dropRequestHeader": {{
msg: "drop request header when none",
args: []interface{}{"X-Test-Name"},
valid: true,
msg: "drop request header when none",
args: []interface{}{"X-Test-Name"},
valid: true,
expectedHeader: http.Header{},
}, {
msg: "drop request header when exists",
args: []interface{}{"X-Test-Name"},
valid: true,
requestHeader: http.Header{"X-Test-Name": []string{"value0", "value1"}},
msg: "drop request header when exists",
args: []interface{}{"X-Test-Name"},
valid: true,
requestHeader: http.Header{"X-Test-Name": []string{"value0", "value1"}},
expectedHeader: http.Header{},
}, {
msg: "name parameter is case-insensitive",
args: []interface{}{"x-test-name"},
valid: true,
requestHeader: http.Header{"X-Test-Name": []string{"value0", "value1"}},
msg: "drop request header when does not exist",
args: []interface{}{"X-Test-Does-Not-Exist"},
valid: true,
requestHeader: http.Header{"X-Test-Name": []string{"value0", "value1"}},
expectedHeader: http.Header{"X-Test-Request-Name": []string{"value0", "value1"}},
}, {
msg: "name parameter is case-insensitive",
args: []interface{}{"x-test-name"},
valid: true,
requestHeader: http.Header{"X-Test-Name": []string{"value0", "value1"}},
expectedHeader: http.Header{},
}, {
msg: "drop matching value",
args: []interface{}{"X-Test-Name", "bar"},
valid: true,
requestHeader: http.Header{"X-Test-Name": []string{"foo", "bar", "baz"}, "X-Test-Name2": []string{"qux"}},
expectedHeader: http.Header{"X-Test-Request-Name": []string{"foo", "baz"}, "X-Test-Request-Name2": []string{"qux"}},
}, {
msg: "ignore non-matching",
args: []interface{}{"X-Test-Name", "qux"},
valid: true,
requestHeader: http.Header{"X-Test-Name": []string{"foo", "bar", "baz"}, "X-Test-Name2": []string{"qux"}},
expectedHeader: http.Header{"X-Test-Request-Name": []string{"foo", "bar", "baz"}, "X-Test-Request-Name2": []string{"qux"}},
}, {
msg: "drop matching value name parameter is case-insensitive",
args: []interface{}{"x-test-name", "bar"},
valid: true,
requestHeader: http.Header{"X-Test-Name": []string{"foo", "bar", "baz"}, "X-Test-Name2": []string{"qux"}},
expectedHeader: http.Header{"X-Test-Request-Name": []string{"foo", "baz"}, "X-Test-Request-Name2": []string{"qux"}},
}},
"setResponseHeader": {{
msg: "set response header when none",
Expand All @@ -220,9 +245,10 @@ func TestHeader(t *testing.T) {
valid: true,
expectedHeader: http.Header{"X-Test-Name": []string{"a small barter"}},
}, {
msg: "set response header from path params when missing",
args: []interface{}{"X-Test-Name", "a ${foo}ter"},
valid: true,
msg: "set response header from path params when missing",
args: []interface{}{"X-Test-Name", "a ${foo}ter"},
valid: true,
expectedHeader: http.Header{},
}, {
msg: "name parameter is case-insensitive",
args: []interface{}{"x-test-name", "Value"},
Expand Down Expand Up @@ -261,19 +287,46 @@ func TestHeader(t *testing.T) {
expectedHeader: http.Header{"X-Test-Name": []string{"Value"}},
}},
"dropResponseHeader": {{
msg: "drop response header when none",
args: []interface{}{"X-Test-Name"},
valid: true,
msg: "drop response header when none",
args: []interface{}{"X-Test-Name"},
valid: true,
expectedHeader: http.Header{},
}, {
msg: "drop response header when exists",
args: []interface{}{"X-Test-Name"},
valid: true,
responseHeader: http.Header{"X-Test-Name": []string{"value0", "value1"}},
expectedHeader: http.Header{},
}, {
msg: "drop response header when does not exist",
args: []interface{}{"X-Test-Does-Not-Exist"},
valid: true,
responseHeader: http.Header{"X-Test-Name": []string{"value0", "value1"}},
expectedHeader: http.Header{"X-Test-Name": []string{"value0", "value1"}},
}, {
msg: "name parameter is case-insensitive",
args: []interface{}{"x-test-name"},
valid: true,
responseHeader: http.Header{"X-Test-Name": []string{"value0", "value1"}},
expectedHeader: http.Header{},
}, {
msg: "drop matching value",
args: []interface{}{"X-Test-Name", "bar"},
valid: true,
responseHeader: http.Header{"X-Test-Name": []string{"foo", "bar", "baz"}, "X-Test-Name2": []string{"qux"}},
expectedHeader: http.Header{"X-Test-Name": []string{"foo", "baz"}, "X-Test-Name2": []string{"qux"}},
}, {
msg: "ignore non-matching",
args: []interface{}{"X-Test-Name", "qux"},
valid: true,
responseHeader: http.Header{"X-Test-Name": []string{"foo", "bar", "baz"}, "X-Test-Name2": []string{"qux"}},
expectedHeader: http.Header{"X-Test-Name": []string{"foo", "bar", "baz"}, "X-Test-Name2": []string{"qux"}},
}, {
msg: "drop matching value name parameter is case-insensitive",
args: []interface{}{"x-test-name", "bar"},
valid: true,
responseHeader: http.Header{"X-Test-Name": []string{"foo", "bar", "baz"}, "X-Test-Name2": []string{"qux"}},
expectedHeader: http.Header{"X-Test-Name": []string{"foo", "baz"}, "X-Test-Name2": []string{"qux"}},
}},
"setContextRequestHeader": {{
msg: "set request header from context",
Expand All @@ -282,11 +335,12 @@ func TestHeader(t *testing.T) {
valid: true,
expectedHeader: http.Header{"X-Test-Request-Foo": []string{"bar"}},
}, {
msg: "set request host header from context",
args: []interface{}{"Host", "foo"},
context: map[string]interface{}{"foo": "www.example.org"},
valid: true,
host: "www.example.org",
msg: "set request host header from context",
args: []interface{}{"Host", "foo"},
context: map[string]interface{}{"foo": "www.example.org"},
valid: true,
host: "www.example.org",
expectedHeader: http.Header{},
}, {
msg: "name parameter is case-insensitive",
args: []interface{}{"x-test-foo", "foo"},
Expand All @@ -302,11 +356,12 @@ func TestHeader(t *testing.T) {
requestHeader: http.Header{"X-Test-Foo": []string{"bar"}},
expectedHeader: http.Header{"X-Test-Request-Foo": []string{"bar", "baz"}},
}, {
msg: "append request host header from context",
args: []interface{}{"Host", "foo"},
context: map[string]interface{}{"foo": "www.example.org"},
valid: true,
host: "www.example.org",
msg: "append request host header from context",
args: []interface{}{"Host", "foo"},
context: map[string]interface{}{"foo": "www.example.org"},
valid: true,
host: "www.example.org",
expectedHeader: http.Header{},
}, {
msg: "name parameter is case-insensitive",
args: []interface{}{"x-test-foo", "foo"},
Expand Down Expand Up @@ -356,9 +411,10 @@ func TestHeader(t *testing.T) {
msg: "invalid target header name",
args: []interface{}{"X-Test-Foo", 42},
}, {
msg: "no header to copy",
args: []interface{}{"X-Test-Foo", "X-Test-Bar"},
valid: true,
msg: "no header to copy",
args: []interface{}{"X-Test-Foo", "X-Test-Bar"},
valid: true,
expectedHeader: http.Header{},
}, {
msg: "copy header",
args: []interface{}{"X-Test-Foo", "X-Test-Bar"},
Expand Down Expand Up @@ -414,9 +470,10 @@ func TestHeader(t *testing.T) {
msg: "invalid target header name",
args: []interface{}{"X-Test-Foo", 42},
}, {
msg: "no header to copy",
args: []interface{}{"X-Test-Foo", "X-Test-Bar"},
valid: true,
msg: "no header to copy",
args: []interface{}{"X-Test-Foo", "X-Test-Bar"},
valid: true,
expectedHeader: http.Header{},
}, {
msg: "copy header",
args: []interface{}{"X-Test-Foo", "X-Test-Bar"},
Expand Down
18 changes: 15 additions & 3 deletions filters/builtin/headerfilter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ package builtin

import (
"fmt"
"net/textproto"
"slices"
"strings"

"github.com/zalando/skipper/eskip"
Expand Down Expand Up @@ -47,7 +49,7 @@ type headerFilter struct {
func headerFilterConfig(typ headerType, config []interface{}) (string, string, *eskip.Template, error) {
switch typ {
case dropRequestHeader, dropResponseHeader:
if len(config) != 1 {
if len(config) < 1 || len(config) > 2 {
return "", "", nil, filters.ErrInvalidFilterParameters
}
default:
Expand Down Expand Up @@ -281,7 +283,12 @@ func (f *headerFilter) Request(ctx filters.FilterContext) {
ctx.SetOutgoingHost(f.value)
}
case dropRequestHeader:
header.Del(f.key)
if f.value == "" {
header.Del(f.key)
} else {
k := textproto.CanonicalMIMEHeaderKey(f.key)
header[k] = slices.DeleteFunc(header[k], func(v string) bool { return v == f.value })
Copy link
Member

Choose a reason for hiding this comment

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

I think value should also be case insensitive.

Copy link
Member Author

Choose a reason for hiding this comment

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

My idea was to have a low-level primitive to drop a specific value.
For setting and adding header we do not do any modification to the value.

To support regexp we may consider changing modRequestHeader such that it deletes the matching header if "the replacement (string)" parameter is empty (or absent) or maybe add a whole new filter.

Copy link
Member

Choose a reason for hiding this comment

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

I think we should add another filter dropRequestHeaderRegexp maybe?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but I'd wait until we have a usecase for regexp value.
This change is useful to mitigate #3456

Copy link
Member

Choose a reason for hiding this comment

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

Could a third boolean parameter to enable case-insensitive comparison be an alternative to regex?

Copy link
Member Author

Choose a reason for hiding this comment

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

It could be done like that of course but lets wait for the usecase.

I initially considered to add a new filter dropRequestHeaderValue then decided to extend existing one but we should wary about how much logic we cram into it.

These header filters are basic primitives and building blocks and I think we should keep them simple. For higher-level or interesting logic we can always add new filters.

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 think we should add another filter dropRequestHeaderRegexp maybe?

Indeed, thanks, a new dropRequestHeaderRegexp would be better because exact value match is a very simple regexp (foo -> ^foo$ ), stdlib handles it as a special case of fast prefix check, we already use it e.g. in HeaderRegexp and it covers many other usecases.

Copy link
Member Author

Choose a reason for hiding this comment

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

Could a third boolean parameter to enable case-insensitive comparison be an alternative to regex?

This could be achieved by regexp flags (e.g. ^(?i)foo$)

}
case setContextRequestHeader:
valueFromContext(ctx, f.key, f.value, true, header.Set)
case appendContextRequestHeader:
Expand Down Expand Up @@ -313,7 +320,12 @@ func (f *headerFilter) Response(ctx filters.FilterContext) {
case depResponseHeader:
header.Add(f.key, f.value)
case dropResponseHeader:
header.Del(f.key)
if f.value == "" {
header.Del(f.key)
} else {
k := textproto.CanonicalMIMEHeaderKey(f.key)
header[k] = slices.DeleteFunc(header[k], func(v string) bool { return v == f.value })
}
case setContextResponseHeader:
valueFromContext(ctx, f.key, f.value, false, header.Set)
case appendContextResponseHeader:
Expand Down