From 93b4e2cc371186235325cf8b076add83943b85c8 Mon Sep 17 00:00:00 2001 From: Simon Kok Date: Fri, 9 Aug 2024 09:47:13 +0200 Subject: [PATCH] Fix validation of notification endpoint to raise when empty Issue: #747 ## Why? When no notification endpoint is configured, it did not warm about this. Instead, the execution of the main bootstrap pipeline failed. ## What? * Add validation logic to confirm that a notification endpoint is configured. * When an email address is configured, it should contain the '@' character too. --- docs/installation-guide.md | 2 +- .../bootstrap_repository/adf-build/config.py | 42 +++++++++++------- .../adf-build/tests/test_config.py | 44 +++++++++++++++++++ 3 files changed, 70 insertions(+), 18 deletions(-) diff --git a/docs/installation-guide.md b/docs/installation-guide.md index 1ae295b40..9af70d87d 100644 --- a/docs/installation-guide.md +++ b/docs/installation-guide.md @@ -391,7 +391,7 @@ Guide](./admin-guide.md#adfconfig). #### Parameter MainNotificationEndpoint -Optional, default value: (empty) +Required on installation, optional on update, default value: (empty) Example: `jane@example.com` diff --git a/src/lambda_codebase/initial_commit/bootstrap_repository/adf-build/config.py b/src/lambda_codebase/initial_commit/bootstrap_repository/adf-build/config.py index 0b9b99d44..d07b60487 100644 --- a/src/lambda_codebase/initial_commit/bootstrap_repository/adf-build/config.py +++ b/src/lambda_codebase/initial_commit/bootstrap_repository/adf-build/config.py @@ -76,19 +76,30 @@ def _validate(self): "adfconfig.yml is missing required properties. " "Please see the documentation." ) from None - - try: - if self.config.get("scp"): - assert self.config.get("scp").get("keep-default-scp") in [ - "enabled", - "disabled", - ] - except AssertionError: + if "" in ( + self.cross_account_access_role, + self.deployment_account_region, + self.notification_endpoint, + ): + raise InvalidConfigError( + "adfconfig.yml is missing required properties, set as ''. " + "Please see the documentation." + ) from None + if self.notification_type == "email" and "@" not in self.notification_endpoint: raise InvalidConfigError( - "Configuration settings for organizations should be either " - "enabled or disabled" + "The main-notification-endpoint configured in adfconfig.yml, " + "is configured as an email, but lacks the '@' character. " + "Please see the documentation." ) from None + if self.config.get("scp"): + valid_options = ["enabled", "disabled"] + if self.config.get("scp").get("keep-default-scp") not in valid_options: + raise InvalidConfigError( + "Configuration settings for organizations should be either " + "enabled or disabled" + ) from None + if isinstance(self.deployment_account_region, list): if len(self.deployment_account_region) > 1: raise InvalidConfigError( @@ -134,18 +145,15 @@ def _parse_config(self): # TODO Investigate why this only considers the first notification # endpoint. Seems like a bug, it should support multiple. - adf_config_notification_type = self.config.get("main-notification-endpoint")[ - 0 - ].get("type") + main_notification_endpoint = (self.config.get("main-notification-endpoint") or [{}])[0] self.notification_type = ( - "lambda" if adf_config_notification_type == "slack" else "email" + "lambda" if main_notification_endpoint.get("type") == "slack" else "email" ) - self.notification_endpoint = self.config.get("main-notification-endpoint")[ - 0 - ].get("target") + self.notification_endpoint = main_notification_endpoint.get("target", "") self.notification_channel = ( None if self.notification_type == "email" else self.notification_endpoint ) + self.extensions = self.config_contents.get("extensions", {}) self._configure_default_extensions_behavior() diff --git a/src/lambda_codebase/initial_commit/bootstrap_repository/adf-build/tests/test_config.py b/src/lambda_codebase/initial_commit/bootstrap_repository/adf-build/tests/test_config.py index 3246130a1..872743fd5 100644 --- a/src/lambda_codebase/initial_commit/bootstrap_repository/adf-build/tests/test_config.py +++ b/src/lambda_codebase/initial_commit/bootstrap_repository/adf-build/tests/test_config.py @@ -60,6 +60,12 @@ def test_raise_validation_remove_deployment_target_region(cls): assert cls._parse_config() +def test_raise_validation_no_notification_endpoint(cls): + cls.config.pop("main-notification-endpoint", None) + with raises(InvalidConfigError): + assert cls._parse_config() + + def test_raise_validation_length_deployment_target_region(cls): cls.config_contents["regions"]["deployment-account"] = [ "region1", @@ -69,6 +75,44 @@ def test_raise_validation_length_deployment_target_region(cls): assert cls._parse_config() +def test_raise_validation_empty_roles(cls): + cls.config_contents["roles"]["cross-account-access"] = "" + with raises(InvalidConfigError): + assert cls._parse_config() + + +def test_raise_validation_empty_deployment_region(cls): + cls.config_contents["regions"]["deployment-account"] = "" + with raises(InvalidConfigError): + assert cls._parse_config() + + +def test_raise_validation_zero_notification_endpoint(cls): + cls.config["main-notification-endpoint"] = [] + with raises(InvalidConfigError): + assert cls._parse_config() + + +def test_raise_validation_empty_obj_notification_endpoint(cls): + cls.config["main-notification-endpoint"] = [{}] + with raises(InvalidConfigError): + assert cls._parse_config() + + +def test_raise_validation_empty_email_notification_endpoint(cls): + cls.config["main-notification-endpoint"] = [{"target": ""}] + with raises(InvalidConfigError): + assert cls._parse_config() + + +def test_raise_validation_no_at_in_email_notification_endpoint(cls): + cls.config["main-notification-endpoint"] = [ + {"target": "some-str", "type": "email"}, + ] + with raises(InvalidConfigError): + assert cls._parse_config() + + def test_sorted_regions(cls): cls.config_contents["regions"]["deployment-account"] = [ "us-east-1",