Skip to content

feat(ingestor-api): expose ingestor handler role #39

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
sharkinsspatial merged 2 commits into from
Apr 25, 2023
+18 −18
Merged

feat(ingestor-api): expose ingestor handler role #39

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cf57746
feat(ingestor-api) expose ingestor handler role
emileten Apr 20, 2023
aa08339
change name of variable to comply with formatting rules, remove reado…
emileten Apr 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 18 additions & 18 deletions lib/ingestor-api/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import { Construct } from "constructs";

export class StacIngestor extends Construct {
table: dynamodb.Table;
public handlerRole: iam.Role;

constructor(scope: Construct, id: string, props: StacIngestorProps) {
super(scope, id);
Expand All @@ -31,6 +32,20 @@ export class StacIngestor extends Construct {
...props.apiEnv,
};

this.handlerRole = new iam.Role(this, "execution-role", {
description:
"Role used by STAC Ingestor. Manually defined so that we can choose a name that is supported by the data access roles trust policy",

@alukach alukach Apr 25, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 emileten Apr 25, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 emileten Apr 25, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

assumedBy: new iam.ServicePrincipal("lambda.amazonaws.com"),
managedPolicies: [
iam.ManagedPolicy.fromAwsManagedPolicyName(
"service-role/AWSLambdaBasicExecutionRole",
),
iam.ManagedPolicy.fromAwsManagedPolicyName(
"service-role/AWSLambdaVPCAccessExecutionRole",
),
],
});

const handler = this.buildApiLambda({
table: this.table,
env,
Expand Down Expand Up @@ -91,23 +106,9 @@ export class StacIngestor extends Construct {
dbSecret: secretsmanager.ISecret;
dbVpc: ec2.IVpc;
dbSecurityGroup: ec2.ISecurityGroup;
subnetSelection: ec2.SubnetSelection;
subnetSelection: ec2.SubnetSelection
}): PythonFunction {
const handler_role = new iam.Role(this, "execution-role", {
description:
"Role used by STAC Ingestor. Manually defined so that we can choose a name that is supported by the data access roles trust policy",
roleName: `stac-ingestion-api-${props.stage}`,
assumedBy: new iam.ServicePrincipal("lambda.amazonaws.com"),
managedPolicies: [
iam.ManagedPolicy.fromAwsManagedPolicyName(
"service-role/AWSLambdaBasicExecutionRole",
),
iam.ManagedPolicy.fromAwsManagedPolicyName(
"service-role/AWSLambdaVPCAccessExecutionRole",
),
],
});


const handler = new PythonFunction(this, "api-handler", {
entry: `${__dirname}/runtime`,
index: "src/handler.py",
Expand All @@ -117,7 +118,7 @@ export class StacIngestor extends Construct {
vpc: props.dbVpc,
vpcSubnets: props.subnetSelection,
allowPublicSubnet: true,
role: handler_role,
role: this.handlerRole,
memorySize: 2048,
});

Expand All @@ -132,7 +133,6 @@ export class StacIngestor extends Construct {
);

props.table.grantReadWriteData(handler);
props.dataAccessRole.grantAssumeRole(handler_role);

return handler;
}
Expand Down