- 
                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 6 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 | 
|---|---|---|
|  | @@ -82,6 +82,31 @@ def default_message(qualified_name: str) -> str: | |
| ) | ||
|  | ||
|  | ||
| # TODO(amazzeo): is NondeterminisimError appropriate as a subclass? | ||
|         
                  VegetarianOrc marked this conversation as resolved.
              Outdated
          
            Show resolved
            Hide resolved | ||
| class UnintentionalPassthroughError(temporalio.workflow.NondeterminismError): | ||
| """Error that occurs when a workflow unintentionally passes an import to the sandbox when | ||
| the import notification policy includes :py:attr:`temporalio.workflow.SandboxImportNotificationPolicy.RAISE_ON_NON_PASSTHROUGH`. | ||
|  | ||
| Attributes: | ||
| qualified_name: Fully qualified name of what was passed through to the sandbox. | ||
| """ | ||
|  | ||
| def __init__( | ||
| self, qualified_name: str, *, override_message: Optional[str] = None | ||
| ) -> None: | ||
| """Create an unintentional passthrough error.""" | ||
| super().__init__( | ||
| override_message | ||
| or UnintentionalPassthroughError.default_message(qualified_name) | ||
| ) | ||
| self.qualified_name = qualified_name | ||
|  | ||
| @staticmethod | ||
| def default_message(qualified_name: str) -> str: | ||
| """Get default message for unintentional passthrough.""" | ||
| return f"Module {qualified_name} was not intentionally passed through to the sandbox." | ||
|         
                  VegetarianOrc marked this conversation as resolved.
              Outdated
          
            Show resolved
            Hide resolved | ||
|  | ||
|  | ||
| @dataclass(frozen=True) | ||
| class SandboxRestrictions: | ||
| """Set of restrictions that can be applied to a sandbox.""" | ||
|  | @@ -110,6 +135,44 @@ class methods (including __init__, etc). The check compares the against the | |
| fully qualified path to the item. | ||
| """ | ||
|  | ||
| import_notification_policy: temporalio.workflow.SandboxImportNotificationPolicy | ||
|          | ||
| """ | ||
| The import notification policy to use when an import is triggered during workflow loading or execution. See :py:class:`temporalio.workflow.SandboxImportNotificationPolicy` for options. | ||
| """ | ||
|  | ||
| import_notification_policy_default: ClassVar[ | ||
| temporalio.workflow.SandboxImportNotificationPolicy | ||
| ] | ||
| """ | ||
| The default import notification policy. Equivalent to :py:attr:`temporalio.workflow.SandboxImportNotificationPolicy.WARN_ON_DYNAMIC_IMPORT`. | ||
| """ | ||
|  | ||
| import_notification_policy_all_warnings: ClassVar[ | ||
| temporalio.workflow.SandboxImportNotificationPolicy | ||
| ] | ||
| """ | ||
| An import notification policy that enables warnings on dynamic imports during a | ||
| workflow and imports that are unintentionally passed through to the sandbox. | ||
| Equivalent to :py:attr:`temporalio.workflow.SandboxImportNotificationPolicy.WARN_ON_DYNAMIC_IMPORT` | :py:attr:`temporalio.workflow.SandboxImportNotificationPolicy.WARN_ON_UNINTENTIONAL_PASSTHROUGH` | ||
| """ | ||
|  | ||
| import_notification_policy_require_explicit_passthrough: ClassVar[ | ||
| temporalio.workflow.SandboxImportNotificationPolicy | ||
| ] | ||
| """ | ||
| An import notification policy that enables warnings on dynamic imports during a | ||
| workflow and raises an error when imports are unintentionally passed through to the sandbox. | ||
| Equivalent to :py:attr:`temporalio.workflow.SandboxImportNotificationPolicy.WARN_ON_DYNAMIC_IMPORT` | :py:attr:`temporalio.workflow.SandboxImportNotificationPolicy.RAISE_ON_UNINTENTIONAL_PASSTHROUGH` | ||
| """ | ||
|  | ||
| import_notification_policy_silent: ClassVar[ | ||
| temporalio.workflow.SandboxImportNotificationPolicy | ||
| ] | ||
| """ | ||
| An import notification policy that disables all warnings and errors. | ||
| Equivalent to :py:attr:`temporalio.workflow.SandboxImportNotificationPolicy.SILENT`. | ||
| """ | ||
|  | ||
|         
                  VegetarianOrc marked this conversation as resolved.
              Outdated
          
            Show resolved
            Hide resolved | ||
| passthrough_all_modules: bool = False | ||
| """ | ||
| Pass through all modules, do not sandbox any modules. This is the equivalent | ||
|  | @@ -170,6 +233,12 @@ def with_passthrough_all_modules(self) -> SandboxRestrictions: | |
| """ | ||
| return dataclasses.replace(self, passthrough_all_modules=True) | ||
|  | ||
| def with_import_policy( | ||
|         
                  VegetarianOrc marked this conversation as resolved.
              Outdated
          
            Show resolved
            Hide resolved | ||
| self, policy: temporalio.workflow.SandboxImportNotificationPolicy | ||
| ) -> SandboxRestrictions: | ||
| """Create a new restriction set with the given import notification policy as the :py:attr:`import_policy`.""" | ||
| return dataclasses.replace(self, import_notification_policy=policy) | ||
|  | ||
|  | ||
| # We intentionally use specific fields instead of generic "matcher" callbacks | ||
| # for optimization reasons. | ||
|  | @@ -305,10 +374,12 @@ def access_matcher( | |
| if not child_matcher: | ||
| return None | ||
| matcher = child_matcher | ||
|  | ||
| if not context.is_runtime and matcher.only_runtime: | ||
| return None | ||
| if not matcher.match_self: | ||
| return None | ||
|  | ||
| return matcher | ||
|  | ||
| def match_access( | ||
|  | @@ -496,6 +567,21 @@ def with_child_unrestricted(self, *child_path: str) -> SandboxMatcher: | |
| } | ||
| ) | ||
|  | ||
| SandboxRestrictions.import_notification_policy_default = ( | ||
| temporalio.workflow.SandboxImportNotificationPolicy.WARN_ON_DYNAMIC_IMPORT | ||
| ) | ||
| SandboxRestrictions.import_notification_policy_all_warnings = ( | ||
| temporalio.workflow.SandboxImportNotificationPolicy.WARN_ON_DYNAMIC_IMPORT | ||
| | temporalio.workflow.SandboxImportNotificationPolicy.WARN_ON_UNINTENTIONAL_PASSTHROUGH | ||
| ) | ||
| SandboxRestrictions.import_notification_policy_require_explicit_passthrough = ( | ||
| temporalio.workflow.SandboxImportNotificationPolicy.WARN_ON_DYNAMIC_IMPORT | ||
| | temporalio.workflow.SandboxImportNotificationPolicy.RAISE_ON_UNINTENTIONAL_PASSTHROUGH | ||
| ) | ||
| SandboxRestrictions.import_notification_policy_silent = ( | ||
| temporalio.workflow.SandboxImportNotificationPolicy.SILENT | ||
| ) | ||
|  | ||
| # sys.stdlib_module_names is only available on 3.10+, so we hardcode here. A | ||
| # test will fail if this list doesn't match the latest Python version it was | ||
| # generated against, spitting out the expected list. This is a string instead | ||
|  | @@ -798,6 +884,7 @@ def _public_callables(parent: Any, *, exclude: Set[str] = set()) -> Set[str]: | |
| passthrough_modules=SandboxRestrictions.passthrough_modules_default, | ||
| invalid_modules=SandboxMatcher.none, | ||
| invalid_module_members=SandboxRestrictions.invalid_module_members_default, | ||
| import_notification_policy=SandboxRestrictions.import_notification_policy_default, | ||
| ) | ||
|  | ||
|  | ||
|  | @@ -819,6 +906,7 @@ def unwrap_if_proxied(v: Any) -> Any: | |
| def __init__(self) -> None: | ||
| """Create a restriction context.""" | ||
| self.is_runtime = False | ||
| self.in_activation = False | ||
|  | ||
|  | ||
| @dataclass | ||
|  | ||
| 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_sandbox_import_notification_policy() -> ( | ||
|         
                  VegetarianOrc marked this conversation as resolved.
              Outdated
          
            Show resolved
            Hide resolved | ||
| 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.