Skip to content

Commit

Permalink
Fix validation of notification endpoint to raise when empty
Browse files Browse the repository at this point in the history
Issue: awslabs#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.
  • Loading branch information
sbkok committed Aug 9, 2024
1 parent ee9cebd commit 93b4e2c
Show file tree
Hide file tree
Showing 3 changed files with 70 additions and 18 deletions.
2 changes: 1 addition & 1 deletion docs/installation-guide.md
Original file line number Diff line number Diff line change
Expand Up @@ -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`

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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()

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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",
Expand Down

0 comments on commit 93b4e2c

Please sign in to comment.