Add Dataset integration tests - S3 Share requests

[SofiaSazonova]

Feature or Bugfix

• Tests

Detail

• module share_base
• bugfix delete_env requires env_object not envUri
• TEMPORARY: hardcoded dataset_uri --> I wait for dataset module

Relates

• Integration tests executed on a real deployment as part of the CICD - Shares #1376

Security

Please answer the questions below briefly where applicable, or write N/A. Based on
OWASP 10.

• Does this PR introduce or modify any input fields or queries - this includes
  fetching data from storage outside the application (e.g. a database, an S3 bucket)?
  • Is the input sanitized?
  • What precautions are you taking before deserializing the data you consume?
  • Is injection prevented by parametrizing queries?
  • Have you ensured no eval or similar functions are used?
• Does this PR introduce any functionality or component that requires authorization?
  • How have you ensured it respects the existing AuthN/AuthZ mechanisms?
  • Are you logging failed auth attempts?
• Are you using or adding any cryptographic features?
  • Do you use a standard proven implementations?
  • Are the used keys controlled by the customer? Where are they stored?
• Are you introducing any new policies/roles/users?
  • Have you used the least-privilege principle? How?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[dlpzx]

The naming does not match any of the other tests, we could call them something closer to the other modules test_shares and test_shares_backwards_compatibility

[dlpzx]

We could also add assertions on the principal of the share and a new test for create_share_object for consumption roles that makes sure that the groupUri and the principal are correct

[dlpzx]

Once the dataset PRs are merged we should add tests on getShareObject with the filters used to list shared items. Making sure it lists the tables, folders and bucket defining them explicitly instead of depending on a list of items

[dlpzx]

My main concern is that we cannot test test_reject_share without running test_submit_object_no_auto_approval before. The tests are coupled, which in this case it might be alright, we are testing test_share_workflow succeed in parts. I would just clearly indicate that as a comment and group them all together (all tests on share1 together and all tests on share3 together to show the path). @petrkalos wdyt?

[dlpzx]

Concern: how do we ensure there are items and this is not an empty list? As a next step after datasets PR, for the validation we would 1) s3 get object to the bucket and access point shares 2) run an athena query on the glue tables

[petrkalos]

Maybe assert_that(items).extracting('itemType').contains('foo1', 'foo2', 'foo3') docs?

[petrkalos]

Maybe assert_that(items).extracting('status').contains_only(ShareItemStatus.Revoke_Succeeded.value)?

[petrkalos]

I'd guess you need that because the account exceeded the number of buckets. Here is a script I wrote that can be run as a lambda which will clean up all the "orphan" (buckets that do not belong to any cfn stack) buckets. We can later extend this to other resources that are being left behind and maybe run it as part of the pytest teardown or as part of the pipeline.

p.s One piece that is currently missing is that if S3 buckets have access points it will fail to delete them.

import logging
import sys
from concurrent.futures.thread import ThreadPoolExecutor

import boto3
from botocore.exceptions import ClientError

logging.getLogger().setLevel(logging.INFO)
if not logging.getLogger().hasHandlers():
    logging.getLogger().addHandler(logging.StreamHandler(sys.stdout))

logger = logging.getLogger(__name__)

session = boto3.session.Session()
s3client = session.client('s3')
s3resource = session.resource('s3')


def is_dataall_bucket(bucket) -> bool:
    try:
        tags = {tag['Key']: tag['Value'] for tag in bucket.Tagging().tag_set}
        return 'testUser' in tags.get('Creator', '') and tags.get('Environment', '').startswith('test')
    except ClientError as e:
        return False


def is_orphan_bucket(bucket):
    region = s3client.get_bucket_location(Bucket=bucket.name)['LocationConstraint'] or 'us-east-1'
    cfnclient = session.client('cloudformation', region_name=region)
    try:
        return not cfnclient.describe_stack_resources(PhysicalResourceId=bucket.name)
    except ClientError as e:
        return 'does not exist' in e.response['Error']['Message']


def delete_bucket(bucket):
    bucket_versioning = bucket.Versioning()
    if bucket_versioning.status == 'Enabled':
        bucket.object_versions.delete()
    else:
        bucket.objects.all().delete()
    bucket.delete()


def cleanup_bucket(bucket):
    try:
        logger.info(f'checking {bucket.name=}')
        if is_dataall_bucket(bucket) and is_orphan_bucket(bucket):
            logger.info(f'deleting {bucket.name}')
            delete_bucket(bucket)
    except Exception:
        logger.exception(f'something went wrong when deleting {bucket.name=}')


def run():
    with ThreadPoolExecutor(max_workers=8) as tpe:
        for _ in tpe.map(cleanup_bucket, s3resource.buckets.all()):
            ...


def lambda_handler(event, context):
    run()


if __name__ == '__main__':
    lambda_handler(None, None)

[petrkalos]

The term second and first account are a bit confusing here. We have 3 types of accounts

• DevOps/Tooling account
• Service/Deployment account
• Environment accounts

All environment accounts must trust the service/deployment account and not the first account. Although environment and service account can be the same account we neither encourage this nor use it in our own pipeline.

Would you mind make it clearer in the doc?

[petrkalos]

nit: I'd make those 2 private and provide a higher level blocking method for queries. Perhaps you can use the boto3 waiter as well.

[petrkalos]

I think the use of aws_profiles will not play very well with CodeBuild, instead I propose to use the existing infrastracture (see session_env1_aws_client).

As discussed offline you need this account to test consumption roles. By using the integration test account you can solve it using the two following patterns

1. (simpler) add the already created (during env deployment) integration-test role directly as a consumption role. Current (CodeBuild/Local) account have already permissions to assume this role so you can use STS to assume it and then run S3 queries to make sure that the share was succesful.
2. (more complex) use the integration-test role to create new roles in the target account that you will register as consumption roles. Then proceed with assuming those roles and testing for S3 access.

[SofiaSazonova]

I implemeted via AssumeRole

[petrkalos]

Instead of parametrizing the tests you can parametrize a fixture and then all tests that are using this fixture will run as many times as the parameters of the fixture.

For example in this case you can do something like...

@pytest.fixture(params=["Group", "ConsumptionRole"])
def principal1(request, group5, consumption_role_1):
    """
    :return: tuple with (principalUri, principalType)
    """
    if request.param is 'Group':
        yield (group5, request.param)
    else:
        yield (consumption_role_1.consumptionRoleUri, request.param)

[SofiaSazonova]

implemented

[petrkalos]

Similar to my previous comment do you think it's possible to use parametrized fixtures here as well to avoid the getfixturevalue?

[SofiaSazonova]

implemented

[petrkalos]

nit: logging

[SofiaSazonova]

removed

[petrkalos]

I am no IAM expert but do we need this? I think the first principal (line 46) will allow all roles from account_id to assume this role. Check this

[SofiaSazonova]

No, it doesn't work. Assumed role is processed differently. I tried without it and got Access Denied, so I had to explicitly add this.

[petrkalos]

nit: shorthand...

    def __init__(self, region, session = boto3.Session()):
       ...

[SofiaSazonova]

done

[petrkalos]

this looks awesome 👍

[petrkalos]

nit: I'd make this a dict and then do a json.dumps

[SofiaSazonova]

done

[petrkalos]

did you decide to have a persistent environment for speed or there are other reasons?

[SofiaSazonova]

1. Speed up.
2. Later we will need persistent shares as well

[petrkalos]

I think @dlpzx made a very good point in another PR about using persistent env/shares.
If we use them without forcing an update (and wait for it to complete) then we might not be testing the latest changes but if we do that then we might as well create a new env every time.

I think we should be able to use persistent envs with the argument of speed only for not very significant features AND obviously to test backwards compatibility (but for that we should still force update and wait).