Title: About updating audit events when function gains new arguments
Type: Stage: patch review
Components: Versions:
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: gousaiyang, steve.dower
Priority: normal Keywords: patch

Created on 2021-04-06 23:04 by gousaiyang, last changed 2021-04-07 11:38 by steve.dower.

Pull Requests
URL Status Linked Edit
PR 25239 open gousaiyang, 2021-04-07 01:19
Messages (4)
msg390386 - (view) Author: Saiyang Gou (gousaiyang) * Date: 2021-04-06 23:04
I'm not sure what to do when a function already being audited gains new arguments which are also worth auditing. For example, issue 38144 brings new `root_dir` and `dir_fd` arguments to `glob.glob`. Currently `glob.glob` is already audited, with `(pathname, recursive)` as the event schema. However, the new `root_dir` and `dir_fd` arguments are important context for the glob operation and is also worth collecting into the event (as a comparison, `os.*` audit events already include `dir_fd`). The problem is how to incorporate the new arguments.

- Solution 1: Add new arguments to the existing event schema (on feature releases). In this ways, hooks expecting an arg tuple of the original length will be broken. Hooks taking arguments by subscripts (e.g. `pathname = args[0]`) may still work if we only append new arguments to the end of the event schema and never insert arguments in the middle.
- Solution 2: Add a new audit event. In this `glob` case, we add a new event which might be called `glob.glob.v2` or `glob.glob.3_10` or some other way. This will make sure the event schema is always fixed across different feature releases. But hooks may need to correctly handle all "versions" of events (e.g. do some thing like `if event.startswith('glob.glob')` instead of `if event == 'glob.glob'`).

I guess more audited functions may gain new arguments in the future so this problem is unavoidable. I would like some suggestions from Steve. Thanks!
msg390394 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2021-04-07 00:44
Solution 1 is definitely out, as PyArg_ParseTuple is a very valid way to implement hooks but won't handle longer tuples.

For Solution 2, we already have some events that use a slash to indicate "extra" information, so depending on context I'd either do "glob.glob/2" or add both "glob.glob/root_dir" and "glob.glob/dir_fd" events.

I suspect in this case, the "glob.glob/2" name is better, because all the data really has to come together. Sending those values through separately might make more sense if there's some other linkable id (such as for socket events).

Nobody should handle hooks that they don't expect with anything other than *args (and even then they should be careful, as hooks may pass through MB or GB of data, so logging or printing them directly may be a bad idea).
msg390401 - (view) Author: Saiyang Gou (gousaiyang) * Date: 2021-04-07 06:08
And one more question, should we ever remove an old version of event when we add a new version of it? A function whose signature got repeatedly changed may result in several versions of audit event raised together on a single operation, which is probably not good for performance. But removing the old event will make those hooks which are not written to handle the new version of the event unable to receive the old event at all, which doesn't look good.
msg390424 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2021-04-07 11:38
To remove an event, it needs to be deprecated as any public facing feature would be deprecated. I'm not sure how we'd go about publicising it though. There's currently no way to raise a warning, so it'd just have to be very clearly advertised in documentation (as a breaking change).

Easiest to assume that an event will be permanent. If there's a significant impact due to an event that we want to avoid, we can address it then.
Date User Action Args
2021-04-07 11:38:56steve.dowersetmessages: + msg390424
2021-04-07 06:08:50gousaiyangsetmessages: + msg390401
2021-04-07 01:19:24gousaiyangsetkeywords: + patch
stage: patch review
pull_requests: + pull_request23977
2021-04-07 00:44:14steve.dowersetmessages: + msg390394
2021-04-06 23:04:31gousaiyangcreate