- 
                Notifications
    You must be signed in to change notification settings 
- Fork 140
Sandbox importing #1187
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
Sandbox importing #1187
Changes from all commits
29f934f
              5087483
              f4dc5a6
              9c975f3
              b0af7e5
              83f9269
              e463afe
              b037033
              15a2382
              ba04dca
              1bf5ee4
              2d1842d
              1284b85
              66d07a8
              File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
|  | @@ -13,7 +13,7 @@ | |
| from contextlib import contextmanager | ||
| from dataclasses import dataclass | ||
| from datetime import datetime, timedelta, timezone | ||
| from enum import Enum, IntEnum | ||
| from enum import Enum, Flag, IntEnum, auto | ||
| from functools import partial | ||
| from random import Random | ||
| from typing import ( | ||
|  | @@ -1410,6 +1410,20 @@ async def wait_condition( | |
| _sandbox_unrestricted = threading.local() | ||
| _in_sandbox = threading.local() | ||
| _imports_passed_through = threading.local() | ||
| _sandbox_import_notification_policy_override = threading.local() | ||
|  | ||
|  | ||
| class SandboxImportNotificationPolicy(Flag): | ||
| """Defines the behavior taken when modules are imported into the sandbox after the workflow is initially loaded or unintentionally missing from the passthrough list.""" | ||
|  | ||
| SILENT = auto() | ||
| """Allow imports that do not violate sandbox restrictions and no warnings are generated.""" | ||
| WARN_ON_DYNAMIC_IMPORT = auto() | ||
| """Allows dynamic imports that do not violate sandbox restrictions but issues a warning when an import is triggered in the sandbox after initial workflow load.""" | ||
| WARN_ON_UNINTENTIONAL_PASSTHROUGH = auto() | ||
| """Allows imports that do not violate sandbox restrictions but issues a warning when an import is triggered in the sandbox that was unintentionally passed through.""" | ||
| RAISE_ON_UNINTENTIONAL_PASSTHROUGH = auto() | ||
| """Raise an error when an import is triggered in the sandbox that was unintentionally passed through.""" | ||
|  | ||
|  | ||
| class unsafe: | ||
|  | @@ -1498,6 +1512,35 @@ def imports_passed_through() -> Iterator[None]: | |
| finally: | ||
| _imports_passed_through.value = False | ||
|  | ||
| @staticmethod | ||
| def current_import_notification_policy_override() -> ( | ||
| Optional[SandboxImportNotificationPolicy] | ||
| ): | ||
| """Gets the current import notification policy override if one is set.""" | ||
| applied_policy = getattr( | ||
| _sandbox_import_notification_policy_override, | ||
| "value", | ||
| None, | ||
| ) | ||
| return applied_policy | ||
|  | ||
| @staticmethod | ||
| @contextmanager | ||
| def sandbox_import_notification_policy( | ||
| There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hrmm, remind me, did we decide not to have a kind of  There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My thought here was that this could be combined with the pre-defined policies to achieve a similar effect. It's a bit more wordy but feels okay to me to use something like with workflow.unsafe.sandbox_import_notification_policy(
    SandboxRestrictions.import_notification_policy_silent
):
    #...That being said, I think it makes just as much sense to have a method like you're suggesting that applies the correct policy. I put a little time into thinking about this before opening the PR but couldn't settle on any wording that would make the behavior being applied immediately obvious. Very happy to add any options you'd recommend and/or remove this one. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can leave off a  There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good. Along these same lines, do you still think it's worth removing the various static helpers per your other comment? I think either way makes sense. IMO they're helpful, but as you pointed out are pretty straightforward to reason about even without them. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do think it's still worth removing them. Having static, wordy helper constants just to help you set an enum flag seems a bit off. The ergonomics of setting an enum flag are good enough on their own IMO.         
                  cretz marked this conversation as resolved.
              Show resolved
            Hide resolved | ||
| policy: SandboxImportNotificationPolicy, | ||
| ) -> Iterator[None]: | ||
| """Context manager to apply the given import notification policy.""" | ||
| original_policy = _sandbox_import_notification_policy_override.value = getattr( | ||
| _sandbox_import_notification_policy_override, | ||
| "value", | ||
| None, | ||
| ) | ||
| _sandbox_import_notification_policy_override.value = policy | ||
| try: | ||
| yield None | ||
| finally: | ||
| _sandbox_import_notification_policy_override.value = original_policy | ||
|  | ||
|  | ||
| class LoggerAdapter(logging.LoggerAdapter): | ||
| """Adapter that adds details to the log about the running workflow. | ||
|  | ||
Uh oh!
There was an error while loading. Please reload this page.