add x-forwarded-prefix header #382
Conversation
Codecov Report
@@ Coverage Diff @@
## 2.0.x #382 +/- ##
===========================================
- Coverage 71.48% 68.5% -2.99%
===========================================
Files 128 124 -4
Lines 3574 3483 -91
Branches 247 239 -8
===========================================
- Hits 2555 2386 -169
- Misses 893 964 +71
- Partials 126 133 +7
Continue to review full report at Codecov.
|
|
Best not to merge yet as I am seeing intermittent 'bad gateway' errors in k8s and haven't yet determined cause. |
|
Ok the bad gateway errors only happen when I fail to explicitly set a log level for the gateway so not related to this PR. (I guess probably related to logback but haven't investigated further.) This might mean not all null checks necessary as I may have been tricked by the logging problem. Perhaps safest to keep them anyway. |
|
@ryandawsonuk nice work on the PR, I've tried to test it for my use-case but despite trying jitpack and trying to build locally I just can't get it to build. I don't think this has anything to do with your PR just gradle knowledge probably. I'll test it if this gets into a snapshot though, that I can do... |
|
@RobMaskell if it helps I'd think it would work to just include this one class in your project - your examples would be great for further testing this. I'm just overriding the class (https://github.com/ryandawsonuk/activiti-cloud-gateway/tree/develop/src/main/java/org/springframework/cloud/gateway/filter/headers) but I'm not using gradle. |
|
@ryandawsonuk yup that works great thanks. I'd vote for prefix enable property to be true by default, if I had a vote. Hope this PR gets merged. |
| @@ -187,6 +219,32 @@ public HttpHeaders filter(HttpHeaders input, ServerWebExchange exchange) { | |||
| write(updated, X_FORWARDED_PROTO_HEADER, proto, isProtoAppend()); | |||
| } | |||
|
|
|||
| if(isPrefixEnabled()){ | |||
There was a problem hiding this comment.
Some spacing issues if (...) { and more below.
| LinkedHashSet<URI> originalUris = exchange.getAttribute(GATEWAY_ORIGINAL_REQUEST_URL_ATTR); | ||
| URI requestUri = exchange.getAttribute(GATEWAY_REQUEST_URL_ATTR); | ||
|
|
||
| if(originalUris != null && requestUri != null) { |
There was a problem hiding this comment.
Can you add a comment with a general overview of the algorithm?
|
Based on the comments I assume this is labelled waiting for feedback as it's waiting for the security team and not for anything from me? |
| String prefix = originalUri.getPath(); | ||
|
|
||
| //strip trailing slashes before checking if request path is end of original path | ||
| String originalUriPath = originalUri.getPath().substring(0, originalUri.getPath().length() - (originalUri.getPath().endsWith("/") ? 1 : 0)); |
There was a problem hiding this comment.
No need to do a substring if path doesn't end with /. Refactor to something like
private String stripTrailingSlash(URI uri) {
if (uri.getPath().endsWith("/")) {
return uri.getPath().substring(0, uri.getPath().length() - 1);
} else {
return uri.getPath();
}
}
|
@spencergibb Now updated to target 2.0.x |
Adds protected getters for context and scope. Adds new refreshEnvironment() method. fixes spring-cloudgh-382
fixes #314
New PR after closing #377 - this time just using data available in the gw implementation. The idea is that if the path of the url that the gw is routing to is a subset of the url that it is routing from then the difference is the prefix. Also applies if the path of the url being routed to is empty and the routed-from url path is non-empty.
Included quite a few null checks as it should allow for the various different route definition styles and types of request. I've been using this in k8s by overriding the class and found the null checks to be necessary.
Was thinking it could be disabled by default as that's how it is for nginx ingress but I haven't thought of any specific reason not to enable by default.