-
Notifications
You must be signed in to change notification settings - Fork 188
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.
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
allow path patterns in --allow-paths and --ignore-paths #135
allow path patterns in --allow-paths and --ignore-paths #135
Conversation
For context, we had a discussion on whether to allow regex matching when implementing the |
Should we add some sort of a test for this? It was a bit hard to validate the correctness of the change |
I wonder what will be the best way to test a branch the main function. 🤔
Le mar. 27 juil. 2021 à 08:31, Filip Petkovski ***@***.***> a
écrit :
… Should we add some sort of a test for this? It was a bit hard to validate
the correctness of the change
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#135 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABOC3AFLWM6RL7LTOVY6ZJTTZZHFDANCNFSM5A7ZBK5A>
.
|
You can extract this section into a function that returns found/not found and test it in isolation |
cc @s-urbaniak since it seems to overlap with the discussion happening in #130 |
oh sorry, i missed this thread (still iterating over concrete mentions after PTO). Are there any concerns using https://pkg.go.dev/path#Match as outlined in #130 (comment)? |
Updated PR with path pattern match for both --alow-paths and --ignore-paths |
main.go
Outdated
@@ -27,6 +27,7 @@ import ( | |||
"net/url" | |||
"os" | |||
"os/signal" | |||
pathmodule "path" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason we cannot leave this as "path"
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because "path" is already used as a symbol for variable in the scope, I rename the module import to avoid confusing the compiler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is reasonable to rename the local variable to p
. The usage distance of that variable is bound to the for loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, i will rename local variable 👍
main.go
Outdated
found = true | ||
found, err = pathmodule.Match(path, req.URL.Path) | ||
if err != nil { | ||
klog.Fatalf("Allowed path pattern is not correct: %s", path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will exit the whole process with error code 255 at runtime. Note this is a handler not bootstrapping code. Please simply return
in this case.
According to https://pkg.go.dev/path#Match:
The only possible returned error is ErrBadPattern, when pattern is malformed.
We could leverage this and iterate before the handler in the bootstrapping main goroutine:
for _, path := range cfg.allowPaths {
_, err := path.Match(path, "")
if err != nil {
klog.Fatalf("Failed to verify allow path: %s", path)
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, I will replace the klog.Fatalf by a simple return.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check for allowPaths and ignorePaths are added,too.
@raptorsun this change lgtm in general, but please add some test, and at least one e2e test. |
OK, more e2e test will be added.
Le mer. 28 juil. 2021 à 17:50, Sergiusz Urbaniak ***@***.***>
a écrit :
… @raptorsun <https://github.com/raptorsun> this change lgtm in general,
but please add some test, and at least one e2e test.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#135 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABOC3AA7VHITKWVWYIPRWETT2ARLZANCNFSM5A7ZBK5A>
.
|
E2E tests have been added. 🧪 |
lgtm, thank you for the contribution 🎉 |
This pull request make it possible to use URL patterns matching.
For example, we can use --allow-paths="/pre/fix1/*" to allow URL such as "/pre/fix1/AAA", "/pre/fix1/BBB"
We intent to make it possible to proxy requests using URL suffixes delimited by slashes as a method to pass arguments, such as this uses case to pass through query of label values to Prometheus. The URL can be /api/v1/label//values. Because keeping an exhaust list of labels' names is both time-costing and complicated especially when new labels are created during runtime. So we believe allowing pattern match for URL is a good solution.