optimize the awx.main.redact SCM URL sanitizer regex #6254
Conversation
|
Build failed.
|
ryanpetrello
force-pushed
the
redact-faster
branch
from
9874bdc
to
3dcca1a
Compare
ryanpetrello
changed the title
ryanpetrello
changed the title
|
@chrismeyersfsu @AlanCoding this is very, very slow if the >>> def _x():
... t = time.time()
... UriCleaner.remove_sensitive('x' * 150000)
... print(time.time() - t)
...
>>> _x()
105.74454760551453We very recently started doing this filtering in the callback receiver to address a bug (though as far as I can tell, this has always been slow for large JSON blobs, and we've always paid this high cost in various API endpoints): |
|
Build succeeded.
|
awx/main/redact.py
Outdated
| @@ -8,7 +8,7 @@ | |||
|
|
|||
| class UriCleaner(object): | |||
| REPLACE_STR = REPLACE_STR | |||
| SENSITIVE_URI_PATTERN = re.compile(r'(\w+:(\/?\/?)[^\s]+)', re.MULTILINE) # NOQA | |||
| SENSITIVE_URI_PATTERN = re.compile(r'((http|https|ssh):(\/?\/?)[^\s]+)', re.MULTILINE) # NOQA | |||
There was a problem hiding this comment.
\w+ is too greedy for really large strings that don't contain URLs; if we're parsing basic auth out of URLs, and we're talking about project updates, there's only so many protocols we care about here in practice.
even worse is a really long string that happens to have a <word>: followed by lots of characters that aren't spaces.
There was a problem hiding this comment.
https://www.git-scm.com/docs/git-clone#_git_urls_a_id_urls_a
From what I can tell, the only transports that actually allow username/pass as part of the netloc are http/s and ssh.
There was a problem hiding this comment.
Would accidentally using a username and password in a git netloc be a common enough mistake? If so, would not including it in this match pattern prevent it from being redacted from error logs, etc.?
There was a problem hiding this comment.
Yea, that's a fair point - I guess somebody could put in something that doesn't work, like:
git://user:pass@host
I'll add it.
|
How fast is |
|
@AlanCoding it gets much slower the larger the target string is. Before: >>> timeit.timeit("import re; re.compile('(\w+:(\/?\/?)[^\s]+)', re.MULTILINE).search('x'*150000)", number=1)
94.14911386510357After: timeit.timeit("import re; re.compile('((http|https|ssh):(\/?\/?)[^\s]+)', re.MULTILINE).search('x'*150000)", number=1)
0.0009962848853319883 |
ryanpetrello
force-pushed
the
redact-faster
branch
from
3dcca1a
to
1d6f42b
Compare
| def test_large_string_performance(): | ||
| length = 100000 | ||
| redacted = UriCleaner.remove_sensitive('x' * length) | ||
| assert len(redacted) == length |
There was a problem hiding this comment.
How does this really test anything? If it unreasonably takes 100 seconds, doesn't that just mean that the test runs for that long?
ryanpetrello
force-pushed
the
redact-faster
branch
from
1d6f42b
to
7e3865c
Compare
|
Build succeeded.
|
|
I would suggest using The reason the performance is bad is that If you limit the length that you think a scheme could possibly be (and I'm just arbitrarily picking 20 here), you eliminate the performance problem while maintaining generality with regard to schemes. |
|
@ghjm that's a great point, and I like it much better. Thanks for the input - I'll adjust this PR. |
\w+ is too greedy for large strings that don't contain URLs
ryanpetrello
force-pushed
the
redact-faster
branch
from
7e3865c
to
c95624e
Compare
|
Build succeeded.
|
|
Build succeeded (gate pipeline).
|
No description provided.