Resolve inconsistent processing logic in mux.ServeHTTP #536
Conversation
Codecov Report
@@ Coverage Diff @@
## main #536 +/- ##
==========================================
- Coverage 80.63% 76.64% -4.00%
==========================================
Files 88 94 +6
Lines 10122 10807 +685
==========================================
+ Hits 8162 8283 +121
- Misses 1510 2064 +554
- Partials 450 460 +10
Continue to review full report at Codecov.
|
|
IMHO, packages in the The |
Good point, I updated the PR and the description. It now involves less refactoring |
|
@aniaan please help to review this PR. |
pkg/object/httpserver/mux.go
Outdated
|
|
||
| if !path.pass(ctx) { | ||
| m.handleIPNotAllow(ctx) | ||
| return | ||
| return IPNotAllowed, path |
There was a problem hiding this comment.
A little suggestion, our path matching rule should be matchPath + matchMethod + matchHeader, after the above three conditions are matched, only then request is considered to match the path, so only when the above conditions are met, should we check whether the IP is passed.
if !path.matchMethod(ctx) {
continue
}
// at least one path has correct method
methodAllowed = true
var searchResult SearchResult
if path.hasHeaders() {
if !path.matchHeaders(ctx) {
continue
}
searchResult = FoundSkipCache
} else {
searchResult = Found
}
if !path.pass(ctx) {
return IPNotAllowed, nil
}
return searchResult, pathThere was a problem hiding this comment.
In this case, we can implement the following rule configuration
rules:
- paths:
- pathPrefix: /pipeline
method: POST
headers:
X-version: v1
ip:
AllowIPs:
- "8.8.8.8"
backend: post-v1-demo
- pathPrefix: /pipeline
method: POST
headers:
X-version: v2
ip:
AllowIPs:
- "9.9.9.9"
backend: post-v2-demoThere was a problem hiding this comment.
Thanks for review @aniaan ! I agree that it's better to have unit tests here so that it would be easier to make these modifications, however currently it would require adding unit test for the whole httpserver module.
There was a problem hiding this comment.
I like your suggestion about routing based on IP and headers.. I think that when the path does not define them, route should match all headers and IPs. If header or IP is defined, it will be used to match and request causes MethodNotAllowed if no correct header or IP is present in the request.
So this will match requests with correct path and method and any IP or any or none header
pathPrefix: /pipeline
method: POST
backend: post-v2-demoand in your example it's MethodNotAllowed if request path is correct but header or IP is not present in the request. @aniaan Is this what you mean?
There was a problem hiding this comment.
Maybe my description is not very accurate, I want to say is that a path matching rule is path + method + headers, ip is not part of the path matching, we have to put the headers check to the front, if the path + method + headers are matched to complete, that the user is matching the rule, and then check whether the ip is passed, ip if passed, go backend, not passed, directly return 403 response.
samutamm
force-pushed
the
duplicate-path-fix-535
branch
from
6354ed9
to
aff281a
Compare
|
Good refactoring, but please fix the errors |
Fixed! |
* separate path search to own function * move to pathsearch to utils module and unittest * remove useless file * move SearchPath to original httpserver module * make fmt * update SearchPath and add unittest for it * fix path NotFound case and add tests * fix code analysis issue
Fix #535.
At high level this PR aims to fix the issue, that when there are two
httpserver.muxrules, with same path but different Method, only first one is considered.This involves following changes:
SearchPathmuxPathpublic:MuxPathso that it can be used as a return type