Skip to content

Commit

Permalink
Refactor IAM policy creation (#1742)
Browse files Browse the repository at this point in the history
Summary:
Pull Request resolved: #1742

In preparation for limiting the PCE bucket access, update the AWS CLI policy
interface to take a policy template file path.

Reviewed By: ankushksingh

Differential Revision: D40406016

fbshipit-source-id: 022042a4a985df813e5784655a026b25df00fe9e
  • Loading branch information
marksliva authored and facebook-github-bot committed Oct 18, 2022
1 parent 87e84fa commit da413e4
Show file tree
Hide file tree
Showing 6 changed files with 26 additions and 10 deletions.
2 changes: 2 additions & 0 deletions fbpcs/infra/cloud_bridge/deploy.sh
Original file line number Diff line number Diff line change
Expand Up @@ -377,6 +377,7 @@ deploy_aws_resources() {
python3 cli.py create aws \
--add_iam_policy \
--policy_name "$policy_name" \
--template_path "$fb_pc_iam_policy" \
--region "$region" \
--firehose_stream_name "$firehose_stream_name" \
--data_ingestion_lambda_name "$data_ingestion_lambda_name" \
Expand Down Expand Up @@ -425,6 +426,7 @@ table_name=${s3_bucket_data_pipeline//-/_}
data_upload_key_path="semi-automated-data-ingestion"
query_results_key_path="query-results"
data_ingestion_lambda_name="cb-data-ingestion-stream-processor${tag_postfix}"
fb_pc_iam_policy="/terraform_deployment/fbpcs/infra/cloud_bridge/deployment_helper/aws/iam_policies/fb_pc_iam_policy.json"

if "$undeploy"
then
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -124,16 +124,14 @@ def delete_user(self, user_name: str) -> None:
def create_policy(
self,
policy_name: str,
template_path: str,
policy_params: PolicyParams,
user_name: Optional[str] = None,
) -> None:
self.log.info(f"Adding policy {policy_name}")

# directly reading the json file from iam_policies folder
# TODO: pass the policy to be added from cli.py when we need more granular control

policy_json_data = self.read_json_file(
file_name="iam_policies/fb_pc_iam_policy.json", policy_params=policy_params
file_name=template_path, policy_params=policy_params
)

try:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,9 @@ def create(self) -> None:
events_data_crawler_arn=self.cli_args.events_data_crawler_arn,
)
self.aws_deployment_helper_obj.create_policy(
policy_name=self.cli_args.policy_name, policy_params=policy_params
policy_name=self.cli_args.policy_name,
template_path=self.cli_args.template_path,
policy_params=policy_params,
)

if self.cli_args.attach_iam_policy:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,13 @@ def with_create_iam_policy_parser_arguments(
"--policy_name", type=str, required=False, help="Policy name to be created"
)

iam_policy_command_group.add_argument(
"--template_path",
type=str,
required=False,
help="Policy template file to use",
)

iam_policy_command_group.add_argument(
"--firehose_stream_name",
type=str,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ def test_delete_user(self) -> None:
pass

def test_create_policy(self) -> None:
template_path = "iam_policies/fb_pc_iam_policy.json"
test_policy_name = "TestIamPolicyName"
test_policy_params = {"test-key-1": "test-val-1"}
test_user_name = "test-user-name"
Expand All @@ -73,11 +74,11 @@ def test_create_policy(self) -> None:
with self.subTest("basic"):
# Test
self.aws_deployment_helper.create_policy(
test_policy_name, test_policy_params
test_policy_name, template_path, test_policy_params
)
# Assert
self.aws_deployment_helper.read_json_file.assert_called_once_with(
file_name="iam_policies/fb_pc_iam_policy.json",
file_name=template_path,
policy_params=test_policy_params,
)
self.aws_deployment_helper.iam.create_policy.assert_called_once_with(
Expand All @@ -93,7 +94,7 @@ def test_create_policy(self) -> None:
)
# Test
self.aws_deployment_helper.create_policy(
test_policy_name, test_policy_params
test_policy_name, "test_template_path", test_policy_params
)
# Assert
self.aws_deployment_helper.iam.create_policy.assert_called_once_with(
Expand All @@ -111,7 +112,7 @@ def test_create_policy(self) -> None:
)
# Test
self.aws_deployment_helper.create_policy(
test_policy_name, test_policy_params
test_policy_name, "test_template_path", test_policy_params
)
# Assert
self.aws_deployment_helper.log.error.assert_called_once()
Expand All @@ -124,7 +125,10 @@ def test_create_policy(self) -> None:
)
# Test
self.aws_deployment_helper.create_policy(
test_policy_name, test_policy_params, test_user_name
test_policy_name,
"test_template_path",
test_policy_params,
test_user_name,
)
# Assert
self.aws_deployment_helper.log.error.assert_called_once()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ class TestAwsDeploymentHelperTool(unittest.TestCase):
def test_create(self, mock_aws_deployment_helper: MagicMock) -> None:
test_user_name = "test-user"
test_policy_name = "test-policy"
test_template_path = "test-template-path"
test_region = "us-east-1"
test_firehose_stream_name = "test-firehose"
test_data_bucket_name = "test-data-bucket"
Expand Down Expand Up @@ -58,6 +59,7 @@ def test_create(self, mock_aws_deployment_helper: MagicMock) -> None:
cli_args = self.setup_cli_args_mock()
cli_args.add_iam_policy = True
cli_args.policy_name = test_policy_name
cli_args.template_path = test_template_path
cli_args.region = test_region
cli_args.firehose_stream_name = test_firehose_stream_name
cli_args.data_bucket_name = test_data_bucket_name
Expand All @@ -74,6 +76,7 @@ def test_create(self, mock_aws_deployment_helper: MagicMock) -> None:

aws_deployment_helper_tool.aws_deployment_helper_obj.create_policy.assert_called_once_with(
policy_name=test_policy_name,
template_path=test_template_path,
policy_params=PolicyParams(
firehose_stream_name=test_firehose_stream_name,
data_bucket_name=test_data_bucket_name,
Expand Down

0 comments on commit da413e4

Please sign in to comment.