Add testing for escaped log messages #7219
Conversation
nnshah1
commented
May 14, 2024
•
nnshah1
commented
- Test for escaped and unescaped log formats
- Test for log injection
- Minor updates to test search patterns
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…nce-server/server into nnshah1-log-format
rmccorm4
changed the title
| if match: | ||
| validate_table(obj) | ||
| elif escaped: | ||
| validate_protobuf(obj) |
There was a problem hiding this comment.
Deferred to follow-up PR, but would be nice to have some comments explaining the validations that the only acceptable multi-line logs will be tables/protobuf (perhaps json?), but will otherwise be restricted to single-line logs if escaped correctly.
| @@ -148,7 +155,7 @@ for BACKEND in $BACKENDS; do | |||
| RET=1 | |||
| fi | |||
|
|
|||
| grep "] pinned" ${ENSEMBLE_NAME}.nonpinned.server.log | |||
| grep "] \"Pinned memory pool is created" ${ENSEMBLE_NAME}.nonpinned.server.log | |||
There was a problem hiding this comment.
Note: This is checking that the following line is not found in the log. This was previously "passing" because "pinned" is never found, this is updating the check to be more correct to look for the correct pinned memory message, and assert it's not found when disabled.
| else: | ||
| # Make sure the max resource limit is never set to 3 when | ||
| # explicit limit of 10 is set. | ||
| self.assertNotIn("Resource: R1\t Count: 3", f.read()) | ||
| self.assertNotIn("Resource: R1\\t Count: 3", f.read()) |
There was a problem hiding this comment.
Note for possible follow-up, if these end up poorly escaped, maybe we can consider embedding some something like 4 spaces " " literally in the string rather than using \t.
There was a problem hiding this comment.
was also thinking these could be a table - or a json instead
| self._server_process.wait() | ||
|
|
||
| def _launch_server(self, escaped=None): | ||
| cmd = ["tritonserver"] |
There was a problem hiding this comment.
follow-up - Why not one of the following?:
- Start server in
test.shlike other tests - Use in-process python API within python test (See Iman's iterative scheduling test)
- Do it this way with subprocess in python, but move it into a common util we can start to re-use in other tests if reducing bash usage.
There was a problem hiding this comment.
I'm thinking #3 - reason to do it here was to easily name the logs to match the test cases - and to also control escaping / non escaping via environment variable. Would have been difficult to reuse the start server / seperate out the cases there in bash.
in process api is an option - but log injection was specifically a network issue - so wanted to also stay true to the original intent - I haven't tried to verify if the same holds true if accessing the in process api directly
Co-authored-by: Ryan McCormick <[email protected]>
|
|
||
| module_directory = os.path.split(os.path.abspath(__file__))[0] | ||
|
|
||
| test_model_directory = os.path.abspath(os.path.join(module_directory, "log_models")) |
There was a problem hiding this comment.
I'm surprised the codeql scanner didn't complain about this
| self._server_options["log-warning"] = 1 | ||
| self._server_options["log-format"] = "default" | ||
| self._server_options["model-repository"] = os.path.abspath( | ||
| os.path.join(module_directory, "log_models") |
There was a problem hiding this comment.
follow-up: Use test_model_directory defined at top?
