diff --git a/providers/microsoft/psrp/docs/changelog.rst b/providers/microsoft/psrp/docs/changelog.rst index cf1576c90c878..3aa1eafa5f56b 100644 --- a/providers/microsoft/psrp/docs/changelog.rst +++ b/providers/microsoft/psrp/docs/changelog.rst @@ -27,6 +27,17 @@ Changelog --------- +Main +.... + +Breaking changes +~~~~~~~~~~~~~~~~~ + +* ``PsrpOperator`` no longer derives ``task_id`` from ``cmdlet``. ``cmdlet`` is a template field, + so validating and reading it in ``__init__`` operated on the un-rendered Jinja expression; + the parameter validation now runs in ``execute()``. Pass ``task_id`` explicitly when using + ``cmdlet``. + 3.2.6 ..... diff --git a/providers/microsoft/psrp/src/airflow/providers/microsoft/psrp/operators/psrp.py b/providers/microsoft/psrp/src/airflow/providers/microsoft/psrp/operators/psrp.py index 4b64dc6dac578..511f6ac45991c 100644 --- a/providers/microsoft/psrp/src/airflow/providers/microsoft/psrp/operators/psrp.py +++ b/providers/microsoft/psrp/src/airflow/providers/microsoft/psrp/operators/psrp.py @@ -56,8 +56,7 @@ class PsrpOperator(BaseOperator): :param command: command to execute on remote host. (templated) :param powershell: powershell to execute on remote host. (templated) :param cmdlet: - cmdlet to execute on remote host (templated). Also used as the default - value for `task_id`. + cmdlet to execute on remote host (templated). :param arguments: When using the `cmdlet` or `powershell` option, use `arguments` to provide arguments (templated). @@ -106,15 +105,6 @@ def __init__( psrp_session_init: Command | None = None, **kwargs, ) -> None: - args = {command, powershell, cmdlet} - if not exactly_one(*args): - raise ValueError("Must provide exactly one of 'command', 'powershell', or 'cmdlet'") - if arguments and not (powershell or cmdlet): - raise ValueError("Arguments only allowed with 'powershell' or 'cmdlet'") - if parameters and not (powershell or cmdlet): - raise ValueError("Parameters only allowed with 'powershell' or 'cmdlet'") - if cmdlet: - kwargs.setdefault("task_id", cmdlet) super().__init__(**kwargs) self.conn_id = psrp_conn_id self.command = command @@ -128,6 +118,12 @@ def __init__( self.psrp_session_init = psrp_session_init def execute(self, context: Context) -> list[Any] | None: + if not exactly_one(*{self.command, self.powershell, self.cmdlet}): + raise ValueError("Must provide exactly one of 'command', 'powershell', or 'cmdlet'") + if self.arguments and not (self.powershell or self.cmdlet): + raise ValueError("Arguments only allowed with 'powershell' or 'cmdlet'") + if self.parameters and not (self.powershell or self.cmdlet): + raise ValueError("Parameters only allowed with 'powershell' or 'cmdlet'") with ( PsrpHook( self.conn_id, diff --git a/providers/microsoft/psrp/tests/unit/microsoft/psrp/operators/test_psrp.py b/providers/microsoft/psrp/tests/unit/microsoft/psrp/operators/test_psrp.py index 8d2fb922d6ed2..26fbdfdd84a1a 100644 --- a/providers/microsoft/psrp/tests/unit/microsoft/psrp/operators/test_psrp.py +++ b/providers/microsoft/psrp/tests/unit/microsoft/psrp/operators/test_psrp.py @@ -41,12 +41,31 @@ class ExecuteParameter(NamedTuple): class TestPsrpOperator: def test_no_command_or_powershell(self): exception_msg = "Must provide exactly one of 'command', 'powershell', or 'cmdlet'" + op = PsrpOperator(task_id="test_task_id", psrp_conn_id=CONNECTION_ID) with pytest.raises(ValueError, match=exception_msg): - PsrpOperator(task_id="test_task_id", psrp_conn_id=CONNECTION_ID) + op.execute(None) - def test_cmdlet_task_id_default(self): - operator = PsrpOperator(cmdlet="Invoke-Foo", psrp_conn_id=CONNECTION_ID) - assert operator.task_id == "Invoke-Foo" + @pytest.mark.parametrize( + ("kwargs", "match"), + [ + ({"command": "hostname", "arguments": ["x"]}, "Arguments only allowed"), + ({"command": "hostname", "parameters": {"k": "v"}}, "Parameters only allowed"), + ], + ) + def test_arguments_and_parameters_require_powershell_or_cmdlet(self, kwargs, match): + op = PsrpOperator(task_id="test_task_id", psrp_conn_id=CONNECTION_ID, **kwargs) + with pytest.raises(ValueError, match=match): + op.execute(None) + + def test_command_validated_after_rendering(self): + # command is a template field: __init__-time validation saw the raw Jinja (truthy), + # so an expression that renders to an empty string slipped through. The check must + # run on the rendered value in execute(). + op = PsrpOperator(task_id="test", psrp_conn_id=CONNECTION_ID, command="{{ '' }}") + op.render_template_fields({}) + assert op.command == "" + with pytest.raises(ValueError, match="exactly one"): + op.execute(None) @pytest.mark.parametrize("do_xcom_push", [True, False]) @pytest.mark.parametrize( @@ -116,14 +135,14 @@ def test_execute(self, hook_impl, parameter, had_errors, rc, do_xcom_push): assert ps.mock_calls == expected_ps_calls def test_securestring_sandboxed(self): - op = PsrpOperator(psrp_conn_id=CONNECTION_ID, cmdlet="test") + op = PsrpOperator(task_id="test_task_id", psrp_conn_id=CONNECTION_ID, cmdlet="test") template = op.get_template_env().from_string("{{ 'foo' | securestring }}") with pytest.raises(AirflowException): template.render() @patch.object(BaseOperator, "get_template_env") def test_securestring_native(self, get_template_env): - op = PsrpOperator(psrp_conn_id=CONNECTION_ID, cmdlet="test") + op = PsrpOperator(task_id="test_task_id", psrp_conn_id=CONNECTION_ID, cmdlet="test") get_template_env.return_value = NativeEnvironment() template = op.get_template_env().from_string("{{ 'foo' | securestring }}") rendered = template.render() diff --git a/scripts/ci/prek/validate_operators_init_exemptions.txt b/scripts/ci/prek/validate_operators_init_exemptions.txt index b78123db0dc66..daece1defc01b 100644 --- a/scripts/ci/prek/validate_operators_init_exemptions.txt +++ b/scripts/ci/prek/validate_operators_init_exemptions.txt @@ -61,7 +61,6 @@ providers/google/src/airflow/providers/google/cloud/transfers/gcs_to_local.py::G providers/google/src/airflow/providers/google/marketing_platform/operators/campaign_manager.py::GoogleCampaignManagerDeleteReportOperator providers/microsoft/azure/src/airflow/providers/microsoft/azure/transfers/gcs_to_wasb.py::GCSToAzureBlobStorageOperator providers/microsoft/azure/src/airflow/providers/microsoft/azure/transfers/oracle_to_azure_data_lake.py::OracleToAzureDataLakeOperator -providers/microsoft/psrp/src/airflow/providers/microsoft/psrp/operators/psrp.py::PsrpOperator providers/oracle/src/airflow/providers/oracle/transfers/oracle_to_oracle.py::OracleToOracleOperator providers/papermill/src/airflow/providers/papermill/operators/papermill.py::PapermillOperator providers/ssh/src/airflow/providers/ssh/operators/ssh.py::SSHOperator