feat(ingestor-api): expose ingestor handler role

[emileten]

• added a new public read only handler_role property to the StacIngestor construct. As a consequence, I had to move the handler_role creation to the constructor.

• the role name is now automatically generated by AWS. We were manually defining it before so that users could import the role with its name (e.g to modify the data access role policy), but because we're now directly using the role object (i.e an iam.Role object) from the property, the name does not matter. Since manually defining a role name is restrictive (see the documentation of the parameter on the API reference), I think it's better to let AWS choose it.

BREAKING CHANGE: the role name is automatically generated by AWS and thus users can not use the name that was specified before, but should directly interact with the new property (iam.Role) that we are adding.

https://github.com/developmentseed/aws-asdi-pgstac/issues/20, #38

[alukach]

@emileten @sharkinsspatial Sorry to come in after the merge, but I have some concerns about this change.

Why is it better to let AWS choose the IAM Role name? Prior to this change, we were letting implementers bring the role that they were using for data access. It's a pretty important role in that this is the singular entity that needs to be granted read-access to data for the purposes of validating connectivity.

the role name is automatically generated by AWS and thus users can not use the name that was specified before, but should directly interact with the new property (iam.Role) that we are adding.

But what about situations where the role needs access to datasources that are not managed within the same AWS Account as CDK is targeting? For example, imagine a NASA DAAC that permits access to a role. This will be done by that third-party entity with whatever method they choose. For that reason, it is advantageous to be able to offer flexibility to the users of cdk-pgSTAC by allowing them to bring their own roles to the construct (possibly something created outside of CDK)

[emileten]

@alukach just to clarify, because your comment sounds like it's about the data access role.

BREAKING CHANGE: the role name is automatically generated by AWS and thus users can not use the name that was
specified before, but should directly interact with the new property (iam.Role) that we are adding.

This is about the stac ingestor API lambda execution role. Not the data access role. We've always been creating the handler role inside cdk-pgstac, before and after this merge. The change of that merge is that before, we were choosing the handler role a name, and now, we're letting AWS choose it.

Does your comment still apply ?

[alukach]

Yeah, you are correct, I was indeed mistaken. Let me think on this...

[alukach]

@emileten Take a look at this (no longer applicable) description. The intention of the manual name was so that the Data Access Role (possibly created outside of the scope of this CDK codebase) could be conveniently configured to permit the execution/handler role to grant access. For example, in the VEDA Project, there is a manually created Data Access Role. It has a policy to allow it to be assumed by any role matching the pattern stac-ingestion-api-*.

[emileten]

I see @alukach. Just to be more transparent. The rationale for that change was, using explicit names sometimes causes issues when resources are destroyed. I didn't provide enough motivation in the description of this PR, but here is the idea (trying to clarify a slack message I sent some time ago...).

• in deployment X, create a role and give it a name you choose.
• have it assume some other external role. For this, in that external role's trust relationship, manually add the name of the above role.

Now, if for some reason you delete the role of deployment X, something weird happens. In the assumed role trust relationship, the name of the deleted role is replaced by a key id, something that is useless. Even when recreating the deleted role with the same name, the trust relationship isn't updated, and the bug is left there.

Maybe that's what the warning means in the role_name parameter of the role constructor : https://docs.aws.amazon.com/cdk/api/v2/python/aws_cdk.aws_iam/Role.html.

But the general idea of the change was 'using a role name is restrictive, and it looks like we don't use that feature anymore, so let AWS choose the name'.

[emileten]

For example, in the VEDA Project, there is a manually created Data Access Role. It has a policy to allow it to be > assumed by any role matching the pattern stac-ingestion-api-*.

Even with this change, part of the role ARN remains predictable it seems ? For example, in my current deployment using this branch, the execution role has this ARN : arn:aws:iam::916098889494:role/MAAP-STAC-test-pgSTAC-stacingestorexecutionrole599-1QRRAG5IZJBZZ. It looks like the CDK still plugs the stack name + stacingestorexecutionrole (that one I am not sure where it comes from) to the ARN.

[sharkinsspatial]

@alukach For a bit more context you can have a look at this comment on one of @emileten's other PRs #38 (comment). In essence, we shouldn't rely on naming conventions for establishing trust relationships with external roles as this does not provide a guarantee of the handler role's existence. @emileten noted this issue when trying to create the dataAccessRole and the construct within the same stack as the CF change set has non-deterministic creation ordering when no dependency relationship is established.

If we need to expose the handler role arn for establishing trust relationships with other resources that don't have access to the construct property we should add a CfnOutput or SSM property which would guarantee it's existence.

With respect to the manually created role in the VEDA context, are we able to modify it's trust policy ourselves via

dataAccessRole.assumeRolePolicy?.addStatements(allow_policy);

or can it only updated by GCC operators?