From a0d3ec5615837296e9a1b967c6cf0ad5146e5966 Mon Sep 17 00:00:00 2001 From: Lucas <12496191+lucashuy@users.noreply.github.com> Date: Tue, 22 Aug 2023 17:25:38 -0700 Subject: [PATCH 1/3] feat: Added environment variable validation for Terraform arguments (#5809) * Added environment variable validation * Added integration tests * Added typing and more detailed doc string --- .../copy_terraform_built_artifacts.py | 38 ++++++++++++++ .../terraform/hooks/prepare/hook.py | 52 ++++++++++++++++++- samcli/lib/hook/exceptions.py | 4 ++ ...uild_terraform_applications_other_cases.py | 34 ++++++++++++ .../terraform/hooks/prepare/test_hook.py | 43 ++++++++++++++- 5 files changed, 169 insertions(+), 2 deletions(-) diff --git a/samcli/hook_packages/terraform/copy_terraform_built_artifacts.py b/samcli/hook_packages/terraform/copy_terraform_built_artifacts.py index 53a7d4d4ec..d4be990d61 100644 --- a/samcli/hook_packages/terraform/copy_terraform_built_artifacts.py +++ b/samcli/hook_packages/terraform/copy_terraform_built_artifacts.py @@ -56,6 +56,16 @@ LOG = logging.getLogger(__name__) TF_BACKEND_OVERRIDE_FILENAME = "z_samcli_backend_override" +TF_BLOCKED_ARGUMENTS = [ + "-target", + "-destroy", +] +TF_ENVIRONMENT_VARIABLE_DELIM = "=" +TF_ENVIRONMENT_VARIABLES = [ + "TF_CLI_ARGS", + "TF_CLI_ARGS_plan", + "TF_CLI_ARGS_apply", +] class ResolverException(Exception): @@ -276,6 +286,31 @@ def create_backend_override(): cli_exit() +def validate_environment_variables(): + """ + Validate that the Terraform environment variables do not contain blocked arguments. + """ + for env_var in TF_ENVIRONMENT_VARIABLES: + env_value = os.environ.get(env_var, "") + + trimmed_arguments = [] + # get all trimmed arguments in a list and split on delim + # eg. + # "-foo=bar -hello" => ["-foo", "-hello"] + for argument in env_value.split(" "): + cleaned_argument = argument.strip() + cleaned_argument = cleaned_argument.split(TF_ENVIRONMENT_VARIABLE_DELIM)[0] + + trimmed_arguments.append(cleaned_argument) + + if any([argument in TF_BLOCKED_ARGUMENTS for argument in trimmed_arguments]): + LOG.error( + "Environment variable '%s' contains " + "a blocked argument, please validate it does not contain: %s" % (env_var, TF_BLOCKED_ARGUMENTS) + ) + cli_exit() + + if __name__ == "__main__": # Gather inputs and clean them argparser = argparse.ArgumentParser( @@ -314,6 +349,9 @@ def create_backend_override(): target = arguments.target json_str = arguments.json + # validate environment variables do not contain blocked arguments + validate_environment_variables() + if target and json_str: LOG.error("Provide either --target or --json. Do not provide both.") cli_exit() diff --git a/samcli/hook_packages/terraform/hooks/prepare/hook.py b/samcli/hook_packages/terraform/hooks/prepare/hook.py index b9adf26c56..0d8c8ce322 100644 --- a/samcli/hook_packages/terraform/hooks/prepare/hook.py +++ b/samcli/hook_packages/terraform/hooks/prepare/hook.py @@ -12,7 +12,11 @@ from samcli.hook_packages.terraform.hooks.prepare.constants import CFN_CODE_PROPERTIES from samcli.hook_packages.terraform.hooks.prepare.translate import translate_to_cfn -from samcli.lib.hook.exceptions import PrepareHookException, TerraformCloudException +from samcli.lib.hook.exceptions import ( + PrepareHookException, + TerraformCloudException, + UnallowedEnvironmentVariableArgumentException, +) from samcli.lib.utils import osutils from samcli.lib.utils.subprocess_utils import LoadingPatternError, invoke_subprocess_with_loading_pattern @@ -32,6 +36,17 @@ "a plan file using the --terraform-plan-file flag." ) +TF_BLOCKED_ARGUMENTS = [ + "-target", + "-destroy", +] +TF_ENVIRONMENT_VARIABLE_DELIM = "=" +TF_ENVIRONMENT_VARIABLES = [ + "TF_CLI_ARGS", + "TF_CLI_ARGS_plan", + "TF_CLI_ARGS_apply", +] + def prepare(params: dict) -> dict: """ @@ -55,6 +70,8 @@ def prepare(params: dict) -> dict: if not output_dir_path: raise PrepareHookException("OutputDirPath was not supplied") + _validate_environment_variables() + LOG.debug("Normalize the terraform application root module directory path %s", terraform_application_dir) if not os.path.isabs(terraform_application_dir): terraform_application_dir = os.path.normpath(os.path.join(os.getcwd(), terraform_application_dir)) @@ -215,3 +232,36 @@ def _generate_plan_file(skip_prepare_infra: bool, terraform_application_dir: str raise PrepareHookException(f"Error occurred when invoking a process:\n{e}") from e return dict(json.loads(result.stdout)) + + +def _validate_environment_variables() -> None: + """ + Validate that the Terraform environment variables do not contain blocked arguments. + + Raises + ------ + UnallowedEnvironmentVariableArgumentException + Raised when a Terraform related environment variable contains a blocked value + """ + for env_var in TF_ENVIRONMENT_VARIABLES: + env_value = os.environ.get(env_var, "") + + trimmed_arguments = [] + # get all trimmed arguments in a list and split on delim + # eg. + # "-foo=bar -hello" => ["-foo", "-hello"] + for argument in env_value.split(" "): + cleaned_argument = argument.strip() + cleaned_argument = cleaned_argument.split(TF_ENVIRONMENT_VARIABLE_DELIM)[0] + + trimmed_arguments.append(cleaned_argument) + + if any([argument in TF_BLOCKED_ARGUMENTS for argument in trimmed_arguments]): + message = ( + "Environment variable '%s' contains a blocked argument, please validate it does not contain: %s" + % ( + env_var, + TF_BLOCKED_ARGUMENTS, + ) + ) + raise UnallowedEnvironmentVariableArgumentException(message) diff --git a/samcli/lib/hook/exceptions.py b/samcli/lib/hook/exceptions.py index 72cbc3e69d..a4b162826b 100644 --- a/samcli/lib/hook/exceptions.py +++ b/samcli/lib/hook/exceptions.py @@ -24,3 +24,7 @@ class PrepareHookException(UserException): class TerraformCloudException(UserException): pass + + +class UnallowedEnvironmentVariableArgumentException(UserException): + pass diff --git a/tests/integration/buildcmd/test_build_terraform_applications_other_cases.py b/tests/integration/buildcmd/test_build_terraform_applications_other_cases.py index 4be9038c19..324eb66631 100644 --- a/tests/integration/buildcmd/test_build_terraform_applications_other_cases.py +++ b/tests/integration/buildcmd/test_build_terraform_applications_other_cases.py @@ -548,3 +548,37 @@ def test_build_and_invoke_lambda_functions(self, function_identifier, expected_o overrides=None, expected_result={"statusCode": 200, "body": expected_output}, ) + + +@skipIf( + (not RUN_BY_CANARY and not CI_OVERRIDE), + "Skip Terraform test cases unless running in CI", +) +class TestBuildTerraformApplicationsWithBlockedEnvironVariables(BuildTerraformApplicationIntegBase): + terraform_application = Path("terraform/simple_application") + + @parameterized.expand( + [ + ("TF_CLI_ARGS", "-destroy"), + ("TF_CLI_ARGS", "-target=some.module"), + ("TF_CLI_ARGS_plan", "-destroy"), + ("TF_CLI_ARGS_plan", "-target=some.module"), + ("TF_CLI_ARGS_apply", "-destroy"), + ("TF_CLI_ARGS_apply", "-target=some.module"), + ] + ) + def test_blocked_env_variables(self, env_name, env_value): + cmdlist = self.get_command_list(hook_name="terraform", beta_features=True) + + env_variables = os.environ.copy() + env_variables[env_name] = env_value + + _, stderr, return_code = self.run_command(cmdlist, env=env_variables) + + process_stderr = stderr.strip() + self.assertRegex( + process_stderr.decode("utf-8"), + "Error: Environment variable '%s' contains a blocked argument, please validate it does not contain: ['-destroy', '-target']" + % env_name, + ) + self.assertNotEqual(return_code, 0) diff --git a/tests/unit/hook_packages/terraform/hooks/prepare/test_hook.py b/tests/unit/hook_packages/terraform/hooks/prepare/test_hook.py index 6fa264e788..99413b33b2 100644 --- a/tests/unit/hook_packages/terraform/hooks/prepare/test_hook.py +++ b/tests/unit/hook_packages/terraform/hooks/prepare/test_hook.py @@ -6,12 +6,18 @@ from tests.unit.hook_packages.terraform.hooks.prepare.prepare_base import PrepareHookUnitBase from samcli.hook_packages.terraform.hooks.prepare.hook import ( + TF_ENVIRONMENT_VARIABLES, + _validate_environment_variables, prepare, _update_resources_paths, TF_CLOUD_EXCEPTION_MESSAGE, TF_CLOUD_HELP_MESSAGE, ) -from samcli.lib.hook.exceptions import PrepareHookException, TerraformCloudException +from samcli.lib.hook.exceptions import ( + PrepareHookException, + TerraformCloudException, + UnallowedEnvironmentVariableArgumentException, +) from samcli.lib.utils.subprocess_utils import LoadingPatternError @@ -492,3 +498,38 @@ def test_prints_tf_cloud_help_message(self, mock_subprocess_loader): prepare(self.prepare_params) self.assertEqual(ex.exception.message, TF_CLOUD_HELP_MESSAGE) + + @parameterized.expand( + [ + ("-destroy",), + ("-target=my.module.resource",), + ("-destroy -target=my.module.resource",), + ("-target=my.module.resource -destroy",), + ] + ) + @patch("samcli.hook_packages.terraform.hooks.prepare.hook.os") + def test_environment_variable_check_fails(self, argument, get_mock): + get_mock.environ.get.return_value = argument + + with self.assertRaises(UnallowedEnvironmentVariableArgumentException): + _validate_environment_variables() + + @parameterized.expand( + [ + ("-not-actually-argument",), + ("something",), + ("",), + ] + ) + @patch("samcli.hook_packages.terraform.hooks.prepare.hook.os") + def test_environment_variable_check_passes(self, argument, get_mock): + get_mock.environ.get.return_value = argument + + _validate_environment_variables() + + @patch("samcli.hook_packages.terraform.hooks.prepare.hook._validate_environment_variables") + def test_prepare_method_fails_environment_variables(self, validate_mock): + validate_mock.side_effect = [UnallowedEnvironmentVariableArgumentException("message")] + + with self.assertRaises(UnallowedEnvironmentVariableArgumentException): + prepare(self.prepare_params) From cae580d4ef344ae419b7996e292f15b6459fa629 Mon Sep 17 00:00:00 2001 From: Daniel Mil <84205762+mildaniel@users.noreply.github.com> Date: Tue, 22 Aug 2023 17:48:56 -0700 Subject: [PATCH 2/3] feat: Report TF custom plan file usage in telemetry (#5825) * feat: Report TF custom plan file usage in telemetry * Read property from config --- .../_utils/custom_options/hook_name_option.py | 22 +++++++ samcli/lib/telemetry/event.py | 33 +++++++--- .../test_hook_package_id_option.py | 61 +++++++++++++++++-- tests/unit/lib/telemetry/test_event.py | 40 +++++++++++- 4 files changed, 143 insertions(+), 13 deletions(-) diff --git a/samcli/commands/_utils/custom_options/hook_name_option.py b/samcli/commands/_utils/custom_options/hook_name_option.py index af730ee2f5..eb333b37bd 100644 --- a/samcli/commands/_utils/custom_options/hook_name_option.py +++ b/samcli/commands/_utils/custom_options/hook_name_option.py @@ -4,6 +4,7 @@ import logging import os +from typing import Any, Mapping import click @@ -18,9 +19,12 @@ ) from samcli.lib.hook.exceptions import InvalidHookWrapperException from samcli.lib.hook.hook_wrapper import IacHookWrapper, get_available_hook_packages_ids +from samcli.lib.telemetry.event import EventName, EventTracker LOG = logging.getLogger(__name__) +PLAN_FILE_OPTION = "terraform_plan_file" + class HookNameOption(click.Option): """ @@ -66,6 +70,8 @@ def handle_parse_result(self, ctx, opts, args): # capture exceptions from prepare hook to emit in track_command c = Context.get_current_context() c.exception = ex + finally: + record_hook_telemetry(opts, ctx) return super().handle_parse_result(ctx, opts, args) @@ -215,3 +221,19 @@ def _read_parameter_value(param_name, opts, ctx, default_value=None): Read SAM CLI parameter value either from the parameters list or from the samconfig values """ return opts.get(param_name, ctx.default_map.get(param_name, default_value)) + + +def record_hook_telemetry(opts: Mapping[str, Any], ctx: click.Context): + """ + Emit metrics related to hooks based on the options passed into the command + + Parameters + ---------- + opts: Mapping[str, Any] + Mapping between a command line option and its value + ctx: Context + Command context properties + """ + plan_file_param = _read_parameter_value(PLAN_FILE_OPTION, opts, ctx) + if plan_file_param: + EventTracker.track_event(EventName.HOOK_CONFIGURATIONS_USED.value, "TerraformPlanFile") diff --git a/samcli/lib/telemetry/event.py b/samcli/lib/telemetry/event.py index 2c35748951..96e5fe45cb 100644 --- a/samcli/lib/telemetry/event.py +++ b/samcli/lib/telemetry/event.py @@ -28,6 +28,7 @@ class EventName(Enum): SYNC_FLOW_END = "SyncFlowEnd" BUILD_WORKFLOW_USED = "BuildWorkflowUsed" CONFIG_FILE_EXTENSION = "SamConfigFileExtension" + HOOK_CONFIGURATIONS_USED = "HookConfigurationsUsed" class UsedFeature(Enum): @@ -61,6 +62,10 @@ class EventType: ] _WORKFLOWS = [f"{config.language}-{config.dependency_manager}" for config in ALL_CONFIGS] + _HOOK_CONFIGURATIONS = [ + "TerraformPlanFile", + ] + _event_values = { # Contains allowable values for Events EventName.USED_FEATURE: [event.value for event in UsedFeature], EventName.BUILD_FUNCTION_RUNTIME: INIT_RUNTIMES, @@ -72,6 +77,7 @@ class EventType: EventName.SYNC_FLOW_END: _SYNC_FLOWS, EventName.BUILD_WORKFLOW_USED: _WORKFLOWS, EventName.CONFIG_FILE_EXTENSION: list(FILE_MANAGER_MAPPER.keys()), + EventName.HOOK_CONFIGURATIONS_USED: _HOOK_CONFIGURATIONS, } @staticmethod @@ -148,6 +154,7 @@ class EventTracker: _events: List[Event] = [] _event_lock = threading.Lock() _session_id: Optional[str] = None + _command_name: Optional[str] = None MAX_EVENTS: int = 50 # Maximum number of events to store before sending @@ -205,8 +212,9 @@ def track_event( Event(event_name, event_value, thread_id=thread_id, exception_name=exception_name) ) - # Get the session ID (needed for multithreading sending) - EventTracker._set_session_id() + # Get properties from the click context (needed for multithreading sending) + EventTracker._set_context_property("_session_id", "session_id") + EventTracker._set_context_property("_command_name", "command_path") if len(EventTracker._events) >= EventTracker.MAX_EVENTS: should_send = True @@ -235,17 +243,27 @@ def send_events() -> threading.Thread: return send_thread @staticmethod - def _set_session_id() -> None: + def _set_context_property(event_prop: str, context_prop: str) -> None: """ - Get the session ID from click and save it locally. + Set a click context property in the event so that it is emitted when the metric is sent. + This is required since the event is sent in a separate thread and no longer has access + to the click context that the command was initially called with. As a workaround, we set + the property here first so that it's available when calling the metrics endpoint. + + Parameters + ---------- + event_prop: str + Property name to be stored in the event and consumed when emitting the metric + context_prop: str + Property name for the target property from the context object """ - if not EventTracker._session_id: + if not getattr(EventTracker, event_prop): try: ctx = Context.get_current_context() if ctx: - EventTracker._session_id = ctx.session_id + setattr(EventTracker, event_prop, getattr(ctx, context_prop)) except RuntimeError: - LOG.debug("EventTracker: Unable to obtain session ID") + LOG.debug("EventTracker: Unable to obtain %s", context_prop) @staticmethod def _send_events_in_thread(): @@ -264,6 +282,7 @@ def _send_events_in_thread(): telemetry = Telemetry() metric = Metric("events") metric.add_data("sessionId", EventTracker._session_id) + metric.add_data("commandName", EventTracker._command_name) metric.add_data("metricSpecificAttributes", msa) telemetry.emit(metric) diff --git a/tests/unit/commands/_utils/custom_options/test_hook_package_id_option.py b/tests/unit/commands/_utils/custom_options/test_hook_package_id_option.py index ad6656fdd2..5675d5e772 100644 --- a/tests/unit/commands/_utils/custom_options/test_hook_package_id_option.py +++ b/tests/unit/commands/_utils/custom_options/test_hook_package_id_option.py @@ -1,10 +1,10 @@ from unittest import TestCase -from unittest.mock import MagicMock, patch +from unittest.mock import MagicMock, patch, Mock import os import click -from samcli.commands._utils.custom_options.hook_name_option import HookNameOption +from samcli.commands._utils.custom_options.hook_name_option import HookNameOption, record_hook_telemetry from samcli.lib.hook.exceptions import InvalidHookWrapperException @@ -68,12 +68,18 @@ def test_invalid_coexist_options(self, iac_hook_wrapper_mock): f"Parameters hook-name, and {','.join(self.invalid_coexist_options)} cannot be used together", ) + @patch("samcli.commands._utils.custom_options.hook_name_option.record_hook_telemetry") @patch("samcli.commands._utils.custom_options.hook_name_option.update_experimental_context") @patch("samcli.commands._utils.custom_options.hook_name_option.prompt_experimental") @patch("samcli.commands._utils.custom_options.hook_name_option.os.getcwd") @patch("samcli.commands._utils.custom_options.hook_name_option.IacHookWrapper") def test_valid_hook_package_with_only_hook_id_option( - self, iac_hook_wrapper_mock, getcwd_mock, prompt_experimental_mock, update_experimental_context_mock + self, + iac_hook_wrapper_mock, + getcwd_mock, + prompt_experimental_mock, + update_experimental_context_mock, + record_hook_telemetry_mock, ): iac_hook_wrapper_mock.return_value = self.iac_hook_wrapper_instance_mock prompt_experimental_mock.return_value = True @@ -104,12 +110,18 @@ def test_valid_hook_package_with_only_hook_id_option( ) self.assertEqual(opts.get("template_file"), self.metadata_path) + @patch("samcli.commands._utils.custom_options.hook_name_option.record_hook_telemetry") @patch("samcli.commands._utils.custom_options.hook_name_option.update_experimental_context") @patch("samcli.commands._utils.custom_options.hook_name_option.prompt_experimental") @patch("samcli.commands._utils.custom_options.hook_name_option.os.getcwd") @patch("samcli.commands._utils.custom_options.hook_name_option.IacHookWrapper") def test_valid_hook_package_with_other_options( - self, iac_hook_wrapper_mock, getcwd_mock, prompt_experimental_mock, update_experimental_context_mock + self, + iac_hook_wrapper_mock, + getcwd_mock, + prompt_experimental_mock, + update_experimental_context_mock, + record_hook_telemetry_mock, ): iac_hook_wrapper_mock.return_value = self.iac_hook_wrapper_instance_mock prompt_experimental_mock.return_value = True @@ -145,13 +157,20 @@ def test_valid_hook_package_with_other_options( "/path/path", ) self.assertEqual(opts.get("template_file"), self.metadata_path) + record_hook_telemetry_mock.assert_called_once() + @patch("samcli.commands._utils.custom_options.hook_name_option.record_hook_telemetry") @patch("samcli.commands._utils.custom_options.hook_name_option.update_experimental_context") @patch("samcli.commands._utils.custom_options.hook_name_option.prompt_experimental") @patch("samcli.commands._utils.custom_options.hook_name_option.os.getcwd") @patch("samcli.commands._utils.custom_options.hook_name_option.IacHookWrapper") def test_valid_hook_package_with_other_options_from_sam_config( - self, iac_hook_wrapper_mock, getcwd_mock, prompt_experimental_mock, update_experimental_context_mock + self, + iac_hook_wrapper_mock, + getcwd_mock, + prompt_experimental_mock, + update_experimental_context_mock, + record_hook_telemetry_mock, ): iac_hook_wrapper_mock.return_value = self.iac_hook_wrapper_instance_mock prompt_experimental_mock.return_value = True @@ -188,6 +207,7 @@ def test_valid_hook_package_with_other_options_from_sam_config( ) self.assertEqual(opts.get("template_file"), self.metadata_path) + @patch("samcli.commands._utils.custom_options.hook_name_option.record_hook_telemetry") @patch("samcli.commands._utils.custom_options.hook_name_option.update_experimental_context") @patch("samcli.commands._utils.custom_options.hook_name_option.prompt_experimental") @patch("samcli.commands._utils.custom_options.hook_name_option.os.getcwd") @@ -200,6 +220,7 @@ def test_valid_hook_package_with_skipping_prepare_hook_and_built_path_exists( getcwd_mock, prompt_experimental_mock, update_experimental_context_mock, + record_hook_telemetry_mock, ): iac_hook_wrapper_mock.return_value = self.iac_hook_wrapper_instance_mock prompt_experimental_mock.return_value = True @@ -222,6 +243,7 @@ def test_valid_hook_package_with_skipping_prepare_hook_and_built_path_exists( self.iac_hook_wrapper_instance_mock.prepare.assert_not_called() self.assertEqual(opts.get("template_file"), None) + @patch("samcli.commands._utils.custom_options.hook_name_option.record_hook_telemetry") @patch("samcli.commands._utils.custom_options.hook_name_option.GlobalConfig") @patch("samcli.commands._utils.custom_options.hook_name_option.update_experimental_context") @patch("samcli.commands._utils.custom_options.hook_name_option.prompt_experimental") @@ -236,6 +258,7 @@ def test_valid_hook_package_with_disable_terraform_beta_feature( prompt_experimental_mock, update_experimental_context_mock, global_config_mock, + record_hook_telemetry_mock, ): iac_hook_wrapper_mock.return_value = self.iac_hook_wrapper_instance_mock prompt_experimental_mock.return_value = False @@ -260,6 +283,7 @@ def test_valid_hook_package_with_disable_terraform_beta_feature( self.iac_hook_wrapper_instance_mock.prepare.assert_not_called() self.assertEqual(opts.get("template_file"), None) + @patch("samcli.commands._utils.custom_options.hook_name_option.record_hook_telemetry") @patch("samcli.commands._utils.custom_options.hook_name_option.update_experimental_context") @patch("samcli.commands._utils.custom_options.hook_name_option.prompt_experimental") @patch("samcli.commands._utils.custom_options.hook_name_option.os.getcwd") @@ -272,6 +296,7 @@ def test_valid_hook_package_with_no_beta_feature_option( getcwd_mock, prompt_experimental_mock, update_experimental_context_mock, + record_hook_telemetry_mock, ): iac_hook_wrapper_mock.return_value = self.iac_hook_wrapper_instance_mock prompt_experimental_mock.return_value = False @@ -294,6 +319,7 @@ def test_valid_hook_package_with_no_beta_feature_option( self.iac_hook_wrapper_instance_mock.prepare.assert_not_called() self.assertEqual(opts.get("template_file"), None) + @patch("samcli.commands._utils.custom_options.hook_name_option.record_hook_telemetry") @patch("samcli.commands._utils.custom_options.hook_name_option.update_experimental_context") @patch("samcli.commands._utils.custom_options.hook_name_option.prompt_experimental") @patch("samcli.commands._utils.custom_options.hook_name_option.os.getcwd") @@ -306,6 +332,7 @@ def test_valid_hook_package_with_beta_feature_option( getcwd_mock, prompt_experimental_mock, update_experimental_context_mock, + record_hook_telemetry_mock, ): iac_hook_wrapper_mock.return_value = self.iac_hook_wrapper_instance_mock prompt_experimental_mock.return_value = False @@ -338,6 +365,7 @@ def test_valid_hook_package_with_beta_feature_option( ) self.assertEqual(opts.get("template_file"), self.metadata_path) + @patch("samcli.commands._utils.custom_options.hook_name_option.record_hook_telemetry") @patch("samcli.commands._utils.custom_options.hook_name_option.update_experimental_context") @patch("samcli.commands._utils.custom_options.hook_name_option.prompt_experimental") @patch("samcli.commands._utils.custom_options.hook_name_option.os.getcwd") @@ -350,6 +378,7 @@ def test_valid_hook_package_with_beta_feature_option_in_sam_config( getcwd_mock, prompt_experimental_mock, update_experimental_context_mock, + record_hook_telemetry_mock, ): metadata_path = "path/metadata.json" cwd_path = "path/current" @@ -387,6 +416,7 @@ def test_valid_hook_package_with_beta_feature_option_in_sam_config( ) self.assertEqual(opts.get("template_file"), metadata_path) + @patch("samcli.commands._utils.custom_options.hook_name_option.record_hook_telemetry") @patch("samcli.commands._utils.custom_options.hook_name_option.GlobalConfig") @patch("samcli.commands._utils.custom_options.hook_name_option.update_experimental_context") @patch("samcli.commands._utils.custom_options.hook_name_option.prompt_experimental") @@ -401,6 +431,7 @@ def test_valid_hook_package_with_beta_feature_option_in_environment_variable( prompt_experimental_mock, update_experimental_context_mock, global_config_mock, + record_hook_telemetry_mock, ): metadata_path = "path/metadata.json" cwd_path = "path/current" @@ -441,6 +472,7 @@ def test_valid_hook_package_with_beta_feature_option_in_environment_variable( ) self.assertEqual(opts.get("template_file"), metadata_path) + @patch("samcli.commands._utils.custom_options.hook_name_option.record_hook_telemetry") @patch("samcli.commands._utils.custom_options.hook_name_option.update_experimental_context") @patch("samcli.commands._utils.custom_options.hook_name_option.prompt_experimental") @patch("samcli.commands._utils.custom_options.hook_name_option.os.getcwd") @@ -453,6 +485,7 @@ def test_valid_hook_package_with_skipping_prepare_hook_and_built_path_does_not_e getcwd_mock, prompt_experimental_mock, update_experimental_context_mock, + record_hook_telemetry_mock, ): iac_hook_wrapper_mock.return_value = self.iac_hook_wrapper_instance_mock prompt_experimental_mock.return_value = True @@ -485,6 +518,7 @@ def test_valid_hook_package_with_skipping_prepare_hook_and_built_path_does_not_e ) self.assertEqual(opts.get("template_file"), self.metadata_path) + @patch("samcli.commands._utils.custom_options.hook_name_option.record_hook_telemetry") @patch("samcli.commands._utils.custom_options.hook_name_option.update_experimental_context") @patch("samcli.commands._utils.custom_options.hook_name_option.prompt_experimental") @patch("samcli.commands._utils.custom_options.hook_name_option.os.getcwd") @@ -497,6 +531,7 @@ def test_valid_hook_package_with_use_container_and_build_image( getcwd_mock, prompt_experimental_mock, update_experimental_context_mock, + record_hook_telemetry_mock, ): iac_hook_wrapper_mock.return_value = self.iac_hook_wrapper_instance_mock prompt_experimental_mock.return_value = True @@ -532,6 +567,7 @@ def test_valid_hook_package_with_use_container_and_build_image( ) self.assertEqual(opts.get("template_file"), self.metadata_path) + @patch("samcli.commands._utils.custom_options.hook_name_option.record_hook_telemetry") @patch("samcli.commands._utils.custom_options.hook_name_option.update_experimental_context") @patch("samcli.commands._utils.custom_options.hook_name_option.prompt_experimental") @patch("samcli.commands._utils.custom_options.hook_name_option.os.getcwd") @@ -544,6 +580,7 @@ def test_invalid_hook_package_with_use_container_and_no_build_image( getcwd_mock, prompt_experimental_mock, update_experimental_context_mock, + record_hook_telemetry_mock, ): iac_hook_wrapper_mock.return_value = self.iac_hook_wrapper_instance_mock prompt_experimental_mock.return_value = True @@ -570,6 +607,7 @@ def test_invalid_hook_package_with_use_container_and_no_build_image( ): hook_name_option.handle_parse_result(ctx, opts, args) + @patch("samcli.commands._utils.custom_options.hook_name_option.record_hook_telemetry") @patch("samcli.commands._utils.custom_options.hook_name_option.update_experimental_context") @patch("samcli.commands._utils.custom_options.hook_name_option.prompt_experimental") @patch("samcli.commands._utils.custom_options.hook_name_option.os.getcwd") @@ -580,6 +618,7 @@ def test_invalid_parameter_hook_with_invalid_project_root_directory( getcwd_mock, prompt_experimental_mock, update_experimental_context_mock, + record_hook_telemetry_mock, ): iac_hook_wrapper_mock.return_value = self.iac_hook_wrapper_instance_mock prompt_experimental_mock.return_value = True @@ -606,6 +645,7 @@ def test_invalid_parameter_hook_with_invalid_project_root_directory( ): hook_name_option.handle_parse_result(ctx, opts, args) + @patch("samcli.commands._utils.custom_options.hook_name_option.record_hook_telemetry") @patch("samcli.commands._utils.custom_options.hook_name_option.update_experimental_context") @patch("samcli.commands._utils.custom_options.hook_name_option.prompt_experimental") @patch("samcli.commands._utils.custom_options.hook_name_option.os.getcwd") @@ -620,6 +660,7 @@ def test_valid_parameter_hook_with_valid_absolute_project_root_directory( getcwd_mock, prompt_experimental_mock, update_experimental_context_mock, + record_hook_telemetry_mock, ): iac_hook_wrapper_mock.return_value = self.iac_hook_wrapper_instance_mock prompt_experimental_mock.return_value = True @@ -655,6 +696,7 @@ def test_valid_parameter_hook_with_valid_absolute_project_root_directory( ) self.assertEqual(opts.get("template_file"), self.metadata_path) + @patch("samcli.commands._utils.custom_options.hook_name_option.record_hook_telemetry") @patch("samcli.commands._utils.custom_options.hook_name_option.update_experimental_context") @patch("samcli.commands._utils.custom_options.hook_name_option.prompt_experimental") @patch("samcli.commands._utils.custom_options.hook_name_option.os.getcwd") @@ -669,6 +711,7 @@ def test_valid_parameter_hook_with_valid_relative_project_root_directory( getcwd_mock, prompt_experimental_mock, update_experimental_context_mock, + record_hook_telemetry_mock, ): iac_hook_wrapper_mock.return_value = self.iac_hook_wrapper_instance_mock prompt_experimental_mock.return_value = True @@ -704,6 +747,7 @@ def test_valid_parameter_hook_with_valid_relative_project_root_directory( ) self.assertEqual(opts.get("template_file"), self.metadata_path) + @patch("samcli.commands._utils.custom_options.hook_name_option.record_hook_telemetry") @patch("samcli.commands._utils.custom_options.hook_name_option.update_experimental_context") @patch("samcli.commands._utils.custom_options.hook_name_option.prompt_experimental") @patch("samcli.commands._utils.custom_options.hook_name_option.os.getcwd") @@ -716,6 +760,7 @@ def test_valid_hook_package_with_use_container_false_and_no_build_image( getcwd_mock, prompt_experimental_mock, update_experimental_context_mock, + record_hook_telemetry_mock, ): iac_hook_wrapper_mock.return_value = self.iac_hook_wrapper_instance_mock prompt_experimental_mock.return_value = True @@ -749,3 +794,9 @@ def test_valid_hook_package_with_use_container_false_and_no_build_image( None, ) self.assertEqual(opts.get("template_file"), self.metadata_path) + + @patch("samcli.commands._utils.custom_options.hook_name_option.EventTracker") + def test_record_hook_telemetry(self, event_tracker_mock): + opts = {"terraform_plan_file": "my_plan.json"} + record_hook_telemetry(opts, Mock()) + event_tracker_mock.track_event.assert_called_once_with("HookConfigurationsUsed", "TerraformPlanFile") diff --git a/tests/unit/lib/telemetry/test_event.py b/tests/unit/lib/telemetry/test_event.py index b8dc01fd34..33202303cf 100644 --- a/tests/unit/lib/telemetry/test_event.py +++ b/tests/unit/lib/telemetry/test_event.py @@ -7,6 +7,9 @@ from typing import List, Tuple from unittest import TestCase from unittest.mock import ANY, Mock, patch + +from parameterized import parameterized + from samcli.cli.context import Context from uuid import UUID, uuid4 @@ -163,6 +166,7 @@ def test_events_get_sent(self, telemetry_mock): "ci": ANY, "pyversion": ANY, "samcliVersion": ANY, + "commandName": ANY, "metricSpecificAttributes": { "events": [ { @@ -212,10 +216,44 @@ def test_session_id_set(self, context_mock): context_mock.return_value = mock EventTracker._session_id = None - EventTracker._set_session_id() + EventTracker._set_context_property("_session_id", "session_id") self.assertEqual(EventTracker._session_id, "123") + @patch("samcli.cli.context.Context.get_current_context") + def test_command_name_set(self, context_mock): + mock = Mock() + mock.command_path = "sam cool command" + context_mock.return_value = mock + + EventTracker._command_name = None + EventTracker._set_context_property("_command_name", "command_path") + + self.assertEqual(EventTracker._command_name, "sam cool command") + + @patch("samcli.cli.context.Context.get_current_context") + def test_set_context_properties(self, context_mock): + event_prop = "event_prop" + context_prop = "context_prop" + context_value = "context_value" + + mock = Mock() + setattr(mock, context_prop, context_value) + context_mock.return_value = mock + + setattr(EventTracker, event_prop, None) + EventTracker._set_context_property(event_prop, context_prop) + + self.assertEqual(getattr(EventTracker, event_prop), context_value) + + @patch("samcli.lib.telemetry.event.LOG") + @patch("samcli.cli.context.Context.get_current_context") + def test_throws_handles_context_not_read(self, context_mock, log_mock): + context_mock.side_effect = RuntimeError("Failed") + EventTracker._command_name = None + EventTracker._set_context_property("_command_name", "command_path") + log_mock.debug.assert_called_once_with("EventTracker: Unable to obtain %s", "command_path") + class TestTrackLongEvent(TestCase): @patch("samcli.lib.telemetry.event.EventTracker.send_events") From 3dc46e5b28eba1dca70376fd76addfd2402d9bb6 Mon Sep 17 00:00:00 2001 From: Wing Fung Lau <4760060+hawflau@users.noreply.github.com> Date: Tue, 22 Aug 2023 23:01:12 -0700 Subject: [PATCH 3/3] Fix sam remote invoke to take profile properly (#5823) * Fix sam remote invoke to take profile properly * error handling --- samcli/commands/remote/invoke/cli.py | 19 ++++++++++++++++--- tests/unit/commands/remote/invoke/test_cli.py | 12 ++++++++++-- 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/samcli/commands/remote/invoke/cli.py b/samcli/commands/remote/invoke/cli.py index 0318566b4a..030a8dbcac 100644 --- a/samcli/commands/remote/invoke/cli.py +++ b/samcli/commands/remote/invoke/cli.py @@ -117,6 +117,12 @@ def do_cli( """ Implementation of the ``cli`` method """ + from botocore.exceptions import ( + NoCredentialsError, + NoRegionError, + ProfileNotFound, + ) + from samcli.commands.exceptions import UserException from samcli.commands.remote.remote_invoke_context import RemoteInvokeContext from samcli.lib.remote_invoke.exceptions import ( @@ -127,9 +133,9 @@ def do_cli( from samcli.lib.remote_invoke.remote_invoke_executors import RemoteInvokeExecutionInfo from samcli.lib.utils.boto_utils import get_boto_client_provider_with_config, get_boto_resource_provider_with_config - boto_client_provider = get_boto_client_provider_with_config(region_name=region) - boto_resource_provider = get_boto_resource_provider_with_config(region_name=region) try: + boto_client_provider = get_boto_client_provider_with_config(region_name=region, profile=profile) + boto_resource_provider = get_boto_resource_provider_with_config(region_name=region, profile=profile) with RemoteInvokeContext( boto_client_provider=boto_client_provider, boto_resource_provider=boto_resource_provider, @@ -142,5 +148,12 @@ def do_cli( ) remote_invoke_context.run(remote_invoke_input=remote_invoke_input) - except (ErrorBotoApiCallException, InvalideBotoResponseException, InvalidResourceBotoParameterException) as ex: + except ( + ErrorBotoApiCallException, + InvalideBotoResponseException, + InvalidResourceBotoParameterException, + ProfileNotFound, + NoCredentialsError, + NoRegionError, + ) as ex: raise UserException(str(ex), wrapped_from=ex.__class__.__name__) from ex diff --git a/tests/unit/commands/remote/invoke/test_cli.py b/tests/unit/commands/remote/invoke/test_cli.py index fe9179a891..2d30ccabc5 100644 --- a/tests/unit/commands/remote/invoke/test_cli.py +++ b/tests/unit/commands/remote/invoke/test_cli.py @@ -3,6 +3,11 @@ from parameterized import parameterized +from botocore.exceptions import ( + ProfileNotFound, + NoCredentialsError, + NoRegionError, +) from samcli.commands.remote.invoke.cli import do_cli from samcli.lib.remote_invoke.remote_invoke_executors import RemoteInvokeOutputFormat from samcli.lib.remote_invoke.exceptions import ( @@ -85,8 +90,8 @@ def test_remote_invoke_command( config_env=self.config_env, ) - patched_get_boto_client_provider_with_config.assert_called_with(region_name=self.region) - patched_get_boto_resource_provider_with_config.assert_called_with(region_name=self.region) + patched_get_boto_client_provider_with_config.assert_called_with(region_name=self.region, profile=self.profile) + patched_get_boto_resource_provider_with_config.assert_called_with(region_name=self.region, profile=self.profile) mock_remote_invoke_context.assert_called_with( boto_client_provider=given_client_provider, @@ -106,6 +111,9 @@ def test_remote_invoke_command( (InvalideBotoResponseException,), (ErrorBotoApiCallException,), (InvalidResourceBotoParameterException,), + (ProfileNotFound,), + (NoCredentialsError,), + (NoRegionError,), ] ) @patch("samcli.commands.remote.remote_invoke_context.RemoteInvokeContext")