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

Support placeholders #3

Open
mheyman opened this issue Apr 23, 2021 · 6 comments
Open

Support placeholders #3

mheyman opened this issue Apr 23, 2021 · 6 comments
Labels
enhancement New feature or request

Comments

@mheyman
Copy link

mheyman commented Apr 23, 2021

Currently, placeholders are not supported in the search or replacement text. It would be really nice to be able to use them.

@francislavoie
Copy link
Member

The problem with placeholders is that they're dynamic. The regexp are compiled ahead of time for performance. If they weren't, then it would be much slower.

While the replacements don't need to be compiled, they are prepared ahead of time to be passed to https://pkg.go.dev/golang.org/x/text/transform which performs the actual replacements (via https://github.com/icholy/replace which implements the regexp part). It might be possible to use replace.RegexpStringFunc instead of replace.RegexpString, but the tricky bit is getting the request into this function, @mholt might have ideas.

@mholt
Copy link
Member

mholt commented May 2, 2021

Yeah, so, we can add placeholders BUT, as Francis said, the values are compiled ahead of time (so we could only perform replacements on things like env variables and other, non-HTTP-request-specific variables), for performance reasons. Is that sufficient? What's your actual use case?

@mholt mholt added the enhancement New feature or request label May 2, 2021
@mheyman
Copy link
Author

mheyman commented May 3, 2021

My use case has to do with, among other things modifying javascript returned through a reverse proxy. It does require HTTP-request specific variables but no regex - although regex would help :-)

My case, there will probably never be more than 30 versions of a regex so lazy creation of the compiled regex and keeping a lru capped at 100 or 1000 of them would be more than enough for our code.

@mholt
Copy link
Member

mholt commented May 3, 2021

@mheyman To clarify, all replacements are processed at config load (startup), not at every request, whether using regular expressions or not:

// construct each transformation
transforms := make([]transform.Transformer, 0, len(h.Replacements))
for _, repl := range h.Replacements {
var tr transform.Transformer
if repl.re != nil {
tr = replace.RegexpString(repl.re, repl.Replace)
} else {
tr = replace.String(repl.Search, repl.Replace)
}
transforms = append(transforms, tr)
}
h.transformChain = transform.Chain(transforms...)

Unless there's a clever solution I haven't thought of, it's likely that implementing this feature will cause a performance regression.

@mheyman
Copy link
Author

mheyman commented May 3, 2021

I would hope there would be no performance regression because in the case where the information is available at config load, no change in ServeHTTP() is required. But, if using placeholders, a different (slower-running) ServeHTTPWithPlacehoders() stage could do the transform in a more flexible way - perhaps with an lru of compiled-as-needed regex (I'm not a go coder so I wouldn't presume to know the best way to go about this).

In our use case, we probably wouldn't notice the slowdown this alternative middleware would cause over the fast path because we only have a few users hitting the pages over an already-slow link. Also, like I mentioned earlier, there would only be 30ish different possible searches in our case so a reasonably sized LRU cache would easily hold all our possible searches. We can code these 30ish search strings manually in the current implementation but it means rewriting the config and restarting the reverse proxy when any of the 30ish proxied services change (happens often). Additionally, we may not always have insight into when things change except when things break. And we'd rather not run with that method of operation :-)

@mholt
Copy link
Member

mholt commented May 3, 2021

I guess that's technically possible, but it adds a lot of complexity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants