Skip to content
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

Merged
merged 1 commit into from
Jul 30, 2021
Merged

allow path patterns in --allow-paths and --ignore-paths #135

merged 1 commit into from
Jul 30, 2021

Conversation

raptorsun
Copy link
Contributor

@raptorsun raptorsun commented Jul 26, 2021

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.

@simonpasquier
Copy link
Contributor

For context, we had a discussion on whether to allow regex matching when implementing the --allow-paths flag in #83. At that time it was dismissed since we had no concrete use case.

@fpetkovski
Copy link

Should we add some sort of a test for this? It was a bit hard to validate the correctness of the change

@raptorsun
Copy link
Contributor Author

raptorsun commented Jul 27, 2021 via email

@fpetkovski
Copy link

You can extract this section into a function that returns found/not found and test it in isolation

@simonpasquier
Copy link
Contributor

cc @s-urbaniak since it seems to overlap with the discussion happening in #130

@s-urbaniak
Copy link
Collaborator

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)?

@raptorsun raptorsun changed the title allow trailing wildcard in --allow-paths allow path patterns in --allow-paths and --ignore-paths Jul 27, 2021
@raptorsun
Copy link
Contributor Author

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"
Copy link
Collaborator

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"?

Copy link
Contributor Author

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.

Copy link
Collaborator

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.

Copy link
Contributor Author

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)
Copy link
Collaborator

@s-urbaniak s-urbaniak Jul 28, 2021

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)
  }
}

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

main.go Show resolved Hide resolved
@s-urbaniak
Copy link
Collaborator

@raptorsun this change lgtm in general, but please add some test, and at least one e2e test.

@raptorsun
Copy link
Contributor Author

raptorsun commented Jul 28, 2021 via email

@raptorsun
Copy link
Contributor Author

E2E tests have been added. 🧪

@s-urbaniak
Copy link
Collaborator

lgtm, thank you for the contribution 🎉

@s-urbaniak s-urbaniak merged commit 8f1fd65 into brancz:master Jul 30, 2021
@raptorsun raptorsun deleted the feature/trailing_wildcards branch September 24, 2021 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants