Add request headers to remove #196
Conversation
| @@ -99,6 +101,7 @@ internal class EnvoyIngressRoutesFactoryTest { | |||
|
|
|||
| // then | |||
| routeConfig | |||
| .hasRequestHeaderToRemove("x-via-vip") | |||
There was a problem hiding this comment.
This test looks like a big bag for all the routes features we have. Couldn't we split it somehow?
There was a problem hiding this comment.
Ok, I've added separated test
| @@ -123,6 +123,7 @@ class RoutesProperties { | |||
| var admin = AdminRouteProperties() | |||
| var status = StatusRouteProperties() | |||
| var authorization = AuthorizationProperties() | |||
| var headersToRemove: List<String> = emptyList() | |||
There was a problem hiding this comment.
How does it behave in Spring Boot? Does Spring Boot add headers properly when this list is immutable?
There was a problem hiding this comment.
It works, just for consistency, I've changed it to mutableList
| @@ -123,6 +123,7 @@ class RoutesProperties { | |||
| var admin = AdminRouteProperties() | |||
| var status = StatusRouteProperties() | |||
| var authorization = AuthorizationProperties() | |||
| var headersToRemove: List<String> = emptyList() | |||
There was a problem hiding this comment.
Please add a description to docs.
There was a problem hiding this comment.
Good point, I've added
There was a problem hiding this comment.
LGTM, just one request to change property format.
The biggest risk with this change is that we rely on undocumented Envoy feature, that the headers are removed AFTER all http filters processed the request. If this will change in the future, the RBAC filter will stop working correctly.
Ideally we should create an integration test to be sure that it works correctly with RBAC. But as I see, we don't even have a test for ip-based + header-selector-matching principal, so it would require writing it from scratch.
If you want to write such test then great, but I think we can also accept this risk and merge it as it is.
| @@ -123,6 +123,7 @@ class RoutesProperties { | |||
| var admin = AdminRouteProperties() | |||
| var status = StatusRouteProperties() | |||
| var authorization = AuthorizationProperties() | |||
| var headersToRemove: List<String> = emptyList() | |||
There was a problem hiding this comment.
IMO routes properties is not the best place for this property, because we set it only on ingress routes, not egress routes.
Maybe we should create ingress section on the same level as egress section?:
envoy:
snapshot:
egress:
...
ingress:
headersToRemove:
There was a problem hiding this comment.
I've moved property as you suggested
I guess we can proceed with this PR and create a separate issue for suggested tests. |
| when { | ||
| properties.ingress.headersToRemove.isNotEmpty() -> { | ||
| builder.addAllRequestHeadersToRemove(properties.ingress.headersToRemove) | ||
| } |
There was a problem hiding this comment.
Not sure if when is better than if when we have only one branch, but why not :)
Allow sanitize local service request headers