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

About updating audit events when function gains new arguments #87922

Closed
gousaiyang mannequin opened this issue Apr 6, 2021 · 5 comments
Closed

About updating audit events when function gains new arguments #87922

gousaiyang mannequin opened this issue Apr 6, 2021 · 5 comments

Comments

@gousaiyang
Copy link
Mannequin

gousaiyang mannequin commented Apr 6, 2021

BPO 43756
Nosy @zooba, @gousaiyang
PRs
  • bpo-43756: Add new audit event to incorporate new arguments added to glob.glob #25239
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2021-04-21.23:24:35.224>
    created_at = <Date 2021-04-06.23:04:31.933>
    labels = []
    title = 'About updating audit events when function gains new arguments'
    updated_at = <Date 2021-04-21.23:24:35.224>
    user = 'https://github.com/gousaiyang'

    bugs.python.org fields:

    activity = <Date 2021-04-21.23:24:35.224>
    actor = 'steve.dower'
    assignee = 'none'
    closed = True
    closed_date = <Date 2021-04-21.23:24:35.224>
    closer = 'steve.dower'
    components = []
    creation = <Date 2021-04-06.23:04:31.933>
    creator = 'gousaiyang'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 43756
    keywords = ['patch']
    message_count = 5.0
    messages = ['390386', '390394', '390401', '390424', '391552']
    nosy_count = 2.0
    nosy_names = ['steve.dower', 'gousaiyang']
    pr_nums = ['25239']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue43756'
    versions = []

    @gousaiyang
    Copy link
    Mannequin Author

    gousaiyang mannequin commented Apr 6, 2021

    I'm not sure what to do when a function already being audited gains new arguments which are also worth auditing. For example, bpo-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!

    @zooba
    Copy link
    Member

    zooba commented Apr 7, 2021

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

    @gousaiyang
    Copy link
    Mannequin Author

    gousaiyang mannequin commented Apr 7, 2021

    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.

    @zooba
    Copy link
    Member

    zooba commented Apr 7, 2021

    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.

    @zooba
    Copy link
    Member

    zooba commented Apr 21, 2021

    New changeset a32f8fe by Saiyang Gou in branch 'master':
    bpo-43756: Add new audit event for new arguments added to glob.glob (GH-25239)
    a32f8fe

    @zooba zooba closed this as completed Apr 21, 2021
    @zooba zooba closed this as completed Apr 21, 2021
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    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

    No branches or pull requests

    1 participant