feat(ingestor-api): user provided role ARN

[emileten]

Breaking change : we're now expecting the stac ingestor handler role to exist and need its ARN, and we change the dataAccessRole parameter to dataAccessRoleArn. This allows me to solve what I poorly described here., which essentially means both the aws-asdi and maap sides that use cdk-pgstac are broken as is, because there is a two-way dependency between the data access role and the stac ingestor api handler role.

I tried several solutions that didn't involve changing things here but instead in the user sides (aws-asdi, maap). None were working, so I resorted to this, and I actually think it makes much more sense to handle the role creation and cross role dependencies as a separate component.

• add a stacIngestorRoleArn string property to the StacIngestor class. Whereas before, we were creating this role as part of the StacIngestor construct, now we are assuming this role already exists (with the right policies), and import it using the user provided role ARN, which this new property represents.
• change the dataAccessRole property name to dataAccessRoleArn, and it's now not anymore a iam.Role object but a string representing the ARN of that role, which is only used when passed to the lambda that will assume that role when verifying data on S3. Removed the line where we call its grantAssumeRole method : it doesn't do anything.

The MAAP deployment relies on this. Here is how this gets used.

[sharkinsspatial]

@emileten I think we can apply a slightly cleaner solution for this. To summarize, when using the StacIngestor construct in a Typescript CDK stack, CDK does not properly infer the dependency on the construct's handler_role when attempting to modify the dataAccessRole's trust policy via assumeRolePolicy?.addStatements. Attempting to reference the handler_role via fromRoleArn and then using it directly in a PolicyStatement results in a Invalid principal in policy on deployment because the handler_role is referenced in the policy before it has been created.

To remedy this we need to expose the handler_role directly as a property of the StacIngestor construct class (rather than employing fromRoleArn) and use this property to construct our PolicyStatement so CDK can correctly infer the dependency. I've modified your template example to demonstrate this sharkinsspatial/template-assume-role-aws-typescript-cdk@90ba945.

I'd recommend closing this PR in favor of a new one which

1. Exposes handler_role as property of the StacIngestor construct.
2. Removes the unnecessary grantAssumeRole (which can't actually make a policy update to an external resource in this case).
3. Modify the consumer stacks to using the StacIngestor to update the trust policy with code similar to this

const stac_ingestor = new StacIngestor(this, "stac-ingestor", dataAccessRole)
const allow_policy = new iam.PolicyStatement({
      actions: ['sts:AssumeRole'],
      principals: [stac_ingestor.handlerRole],
      effect: iam.Effect.ALLOW
    });

dataAccessRole.assumeRolePolicy?.addStatements(allow_policy);

ref https://github.com/developmentseed/aws-asdi-pgstac/issues/20

[emileten]

Thanks @sharkinsspatial. I understand the idea of forcing the dependency through the usage of a property. I see that we are trying to force the CDK to first create the data access role without the trust relationship added, then create the stac ingestor, and then only add the trust relationship.

Just wanted to make sure your suggestion works (not only deploys, which I guess you tested, but also, lambda tests actually runs, which you probably haven't tested because it's using one of my account buckets), and I just confirmed 👍

Remaining question -- why do you think this is cleaner? Because it's a smaller change? Or because you think it's better to have the handler role created where its lambda is created -- in the latter case I am curious what the reasons are ?

[sharkinsspatial]

@emileten I didn’t test the bucket access but I did test to confirm that the call to assume_role was working (which is the main concern).

I’d prefer this approach as our Lambda role is self contained within the construct. If we need to modify it in the future with additional permissions we can do so without issues. Creating this role outside of the construct would not allow us to modify its permissions.

[emileten]

Thanks @sharkinsspatial. Closing this and will reopen a new one.