Fix broken multi-account/multi-region functionality #9541

viren-nadkarni · 2023-11-02T13:23:51Z

Motivation

This PR fixes multi-account/multi-region region functionality that misbehave in non-default region and accounts.

In effect, fixes following tests:

viren-nadkarni · 2023-11-02T13:26:44Z

tests/aws/test_integration.py


        # create target S3 bucket
-        aws_client.s3.create_bucket(Bucket=bucket_name)
+        s3_create_bucket(Bucket=bucket_name)


Using this fixture ensures the the appropriate LocationConstraint is added automatically.

viren-nadkarni · 2023-11-02T13:27:13Z

tests/aws/services/sqs/test_sqs_backdoor.py

+        monkeypatch.setattr(config, "SQS_ENDPOINT_STRATEGY", strategy)
+


The parametrized value for SQS endpoint strategy was not being taken into account.

viren-nadkarni · 2023-11-02T13:28:48Z

tests/aws/services/logs/test_logs.py

+            Principal=f"logs.{TEST_AWS_REGION_NAME}.amazonaws.com",
            Action="lambda:InvokeFunction",
-            SourceArn=f"arn:aws:logs:{config.AWS_REGION_US_EAST_1}:{account_id}:log-group:{logs_log_group}:*",
+            SourceArn=f"arn:aws:logs:{TEST_AWS_REGION_NAME}:{account_id}:log-group:{logs_log_group}:*",


Assertions expected the us-east-1 region even when the resources were created in a different region (by setting TEST_AWS_REGION_NAME)

coveralls · 2023-11-02T14:08:59Z

coverage: 83.985% (-0.02%) from 84.009%
when pulling 17c34bd on fix-xaccount-test
into d099d08 on master.

github-actions · 2023-11-02T14:11:01Z

LocalStack Community integration with Pro

      2 files       2 suites 1h 4m 27s ⏱️
2 328 tests 2 029 ✔️ 299 💤 0 ❌
2 329 runs 2 029 ✔️ 300 💤 0 ❌

Results for commit 17c34bd.

♻️ This comment has been updated with latest results.

TO BE REVERTED BEFORE MERGE

viren-nadkarni · 2023-11-03T08:03:46Z

localstack/services/logs/provider.py

-                client = connect_to().firehose
+                client = connect_to(
+                    aws_access_key_id=arn_data["account"], region_name=arn_data["region"]
+                ).firehose


Ensures logs are sent to correct account and region.

Identified in following tests:

test_put_subscription_filter_lambda

test_put_subscription_filter_firehose

test_put_subscription_filter_kinesis

viren-nadkarni · 2023-11-03T08:04:21Z

tests/aws/services/dynamodb/test_dynamodb.py


        # assert ARN formats
-        expected_arn_prefix = "arn:aws:dynamodb:" + aws_stack.get_local_region()
+        expected_arn_prefix = "arn:aws:dynamodb:" + TEST_AWS_REGION_NAME


Ensures expected value is correctly constructed.

Fixes this test: test_stream_spec_and_region_replacement

viren-nadkarni · 2023-11-03T08:06:52Z

tests/aws/services/stepfunctions/legacy/test_stepfunctions_legacy.py

        assert result["ResponseMetadata"]["HTTPStatusCode"] == 200
        assert result["loggingConfiguration"] == {"level": "OFF", "includeExecutionData": False}
    finally:
+        custom_client.stepfunctions.delete_state_machine(stateMachineArn=result["stateMachineArn"])


The test did not use custom_client to create the statemachine and then failed during retrieval using this client.

Affected test: test_default_logging_configuration

viren-nadkarni · 2023-11-03T08:46:56Z

tests/aws/services/sqs/test_sqs.py

        queue_url = sqs_create_queue(QueueName=queue_name)
        account_id, region_name, queue_name_from_url = parse_queue_url(queue_url)
-        assert account_id == DEFAULT_AWS_ACCOUNT_ID
+        assert account_id == TEST_AWS_ACCOUNT_ID


Assertion was done against the wrong account

bentsku

Nice changes! I have some comments regarding the lambda changes, I'd also like the opinion of @dfangl (for IAM, maybe future?) or maybe a lambda service owner, but I believe we should use the ARN there. Good catch for the S3 buckets as well, more cleanups! 🧹

What about the default region changes? I believe some of the test logic was always expecting us-east-1 to be the default, especially for S3, and snapshots might need to be regenerated.

localstack/services/logs/provider.py

bentsku · 2023-11-03T09:13:28Z

localstack/services/logs/provider.py

            if ":lambda:" in destination_arn:
-                client = connect_to(region_name=extract_region_from_arn(destination_arn)).lambda_
+                client = connect_to(
+                    aws_access_key_id=arn_data["account"], region_name=arn_data["region"]
+                ).lambda_
                lambda_name = arns.lambda_function_name(destination_arn)
                client.invoke(FunctionName=lambda_name, Payload=json.dumps(event))


Same as commented above, I believe we shouldn't extract the lambda function name but use the ARN to invoke the function:

client = connect_to(region_name=extract_region_from_arn(destination_arn)).lambda_ client.invoke(FunctionName=destination_arn, Payload=json.dumps(event))

localstack/services/logs/provider.py

dfangl · 2023-11-07T17:05:34Z

Thanks for the tag @bentsku ! You are right, for cross account/region requests which are done on behalf of the service, we should definitely work on having the proper account just in the request body as the target resource, and use the request metadata to set the service name, instead of setting the account id as access key id. If we do the latter, IAM for example would not detect it as a cross-account request, as the originating account is the same as the targets. Ideally, all services extract the account id from the arn if provided, to access the correct account.

This reverts commit 9589c56.

bentsku

LGTM! This is looking great! thanks for addressing the comments! There's just this part with the role and with_assumed_role and the else part which I'm not sure I understand the intricacies, but the rest is all good to me!

There's just 2 S3 tests that are not cleaning up due to the use of localstack.utils.aws.resources.create_s3_bucket now. Maybe we could add its cleanup to the cleanups fixture.

Thanks a lot again for all of these fixes! 🚀

bentsku · 2023-11-20T11:24:43Z

localstack/services/logs/provider.py

+    else:
+        factory = connect_to(aws_access_key_id=arn_data["account"], region_name=arn_data["region"])


q: is this a fallback? this whole with_assumed_role addition is looking really nice 😄 I lack some knowledge in the IAM part, so this kind of things I don't feel qualified enough to review.

You're correct, if a role ARN is available, IAM enforcement can be performed. Else there is a fallback.

This usage is similar to elsewhere, eg:

localstack/localstack/services/sns/publisher.py

Lines 646 to 652 in 05832fc

if role_arn := subscriber.get("SubscriptionRoleArn"):

factory = connect_to.with_assumed_role(

role_arn=role_arn, service_principal=ServicePrincipal.sns, region_name=region

)

else:

account_id = extract_account_id_from_arn(subscriber["Endpoint"])

factory = connect_to(aws_access_key_id=account_id, region_name=region)

or

localstack/localstack/services/firehose/provider.py

Lines 734 to 739 in 05832fc

if role_arn := s3_destination_description.get("RoleARN"):

factory = connect_to.with_assumed_role(

role_arn=role_arn, service_principal=ServicePrincipal.firehose

)

else:

factory = connect_to()

IAM enforcement can be performed in both cases. However, only if the aws_access_key_id is set to the INTERNAL_AWS_ACCESS_KEY_ID value. There should be no case, other than resource creation, to set the account id as access key id for internal clients, in my opinion. The first snippet by viren for example leads to no IAM enforcement in the else case, which is neither parity nor should happen.

tests/aws/services/s3/test_s3.py

viren-nadkarni added 2 commits November 2, 2023 18:14

Create buckets in a region agnostic manner

0298634

Monkeypatch parametrised strategy as env config

45d20de

viren-nadkarni self-assigned this Nov 2, 2023

viren-nadkarni changed the title ~~Fix xaccount test~~ Fix internal clients not using proper credentials Nov 2, 2023

viren-nadkarni added the semver: patch Non-breaking changes which can be included in patch releases label Nov 2, 2023

viren-nadkarni commented Nov 2, 2023

View reviewed changes

Use proper credentials for internal clients in logs

4173d4f

viren-nadkarni force-pushed the fix-xaccount-test branch from 89d8626 to 4173d4f Compare November 2, 2023 13:27

viren-nadkarni commented Nov 2, 2023

View reviewed changes

viren-nadkarni and others added 6 commits November 3, 2023 11:23

[DEBUG] Use non default account and region

9589c56

TO BE REVERTED BEFORE MERGE

Fix assertion against the wrong value

4bcfe8b

Use test region when building ARN

6b36f46

Use helper as a region agnostic way to create bucket

e7fcf78

Ensure queue resolution can happen in multi-account settings

6fd75e7

Ensure custom client is used to create the resource

abc36a3

viren-nadkarni changed the title ~~Fix internal clients not using proper credentials~~ Fix broken multi-account/multi-region functionality Nov 3, 2023

viren-nadkarni commented Nov 3, 2023

View reviewed changes

Create unique buckets to avoid name collision

68a9632

viren-nadkarni marked this pull request as ready for review November 3, 2023 08:53

viren-nadkarni requested review from MEPalma, baermat, bentsku, dominikschubert and thrau as code owners November 3, 2023 08:53

viren-nadkarni requested review from sannya-singal and removed request for MEPalma, baermat, bentsku, giograno, silv-io and steffyP November 3, 2023 08:54

bentsku reviewed Nov 3, 2023

View reviewed changes

viren-nadkarni requested a review from dfangl November 3, 2023 11:30

Invoke Lambda by ARNs

ebdcceb

viren-nadkarni marked this pull request as draft November 9, 2023 09:22

viren-nadkarni force-pushed the fix-xaccount-test branch from 160bfa8 to ebdcceb Compare November 9, 2023 09:22

Add todo

6e4643b

viren-nadkarni added this to the Playground milestone Nov 10, 2023

viren-nadkarni added 6 commits November 13, 2023 17:39

Use compliant internal calls

a375667

Merge branch 'master' into fix-xaccount-test

e307e24

Merge branch 'master' into fix-xaccount-test

9b1c182

Fix arg

b610bb4

Revert "[DEBUG] Use non default account and region"

6b74aa6

This reverts commit 9589c56.

Merge branch 'master' into fix-xaccount-test

05832fc

viren-nadkarni marked this pull request as ready for review November 14, 2023 10:21

viren-nadkarni requested review from bentsku and removed request for sannya-singal November 14, 2023 10:22

bentsku approved these changes Nov 20, 2023

View reviewed changes

viren-nadkarni added 2 commits November 20, 2023 17:06

Merge branch 'master' into fix-xaccount-test

bc710ce

Cleanup buckets created with helper

17c34bd

viren-nadkarni force-pushed the fix-xaccount-test branch from 4cf4e20 to 17c34bd Compare November 20, 2023 11:42

viren-nadkarni merged commit 177773b into master Nov 20, 2023

viren-nadkarni deleted the fix-xaccount-test branch November 20, 2023 12:21

		monkeypatch.setattr(config, "SQS_ENDPOINT_STRATEGY", strategy)

		else:
		factory = connect_to(aws_access_key_id=arn_data["account"], region_name=arn_data["region"])

	if role_arn := subscriber.get("SubscriptionRoleArn"):
	factory = connect_to.with_assumed_role(
	role_arn=role_arn, service_principal=ServicePrincipal.sns, region_name=region
	)
	else:
	account_id = extract_account_id_from_arn(subscriber["Endpoint"])
	factory = connect_to(aws_access_key_id=account_id, region_name=region)

	if role_arn := s3_destination_description.get("RoleARN"):
	factory = connect_to.with_assumed_role(
	role_arn=role_arn, service_principal=ServicePrincipal.firehose
	)
	else:
	factory = connect_to()

Uh oh!

Fix broken multi-account/multi-region functionality #9541

Fix broken multi-account/multi-region functionality #9541

Uh oh!

Conversation

viren-nadkarni commented Nov 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Nov 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Nov 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

LocalStack Community integration with Pro

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bentsku left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dfangl commented Nov 7, 2023

Uh oh!

bentsku left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

viren-nadkarni commented Nov 2, 2023 •

edited

Loading

coveralls commented Nov 2, 2023 •

edited

Loading

github-actions bot commented Nov 2, 2023 •

edited

Loading

bentsku left a comment •

edited

Loading