fix(elbv2): race condition for Lambda backends #7598

rix0rrr · 2020-04-24T16:09:20Z

DISCUSSION TOPIC:

What I'd actually like to do is break the return signature of role.addToPolicy(): boolean to a struct, otherwise it's going to be very hard to reliably implement this. Fairly sure this won't cause issues because few people will be checking the return value of addToPolicy(), but of course we technically speaking aren't allowed to do this.

Commit Message

fix(elbv2): race condition for Lambda backends

Lambda Targets can get registered into a TargetGroup before the
lambda:Invoke permissions are added. Since the TargetGroup does
a permission test, deployment will fail.

Fixes #4663.

Adding a long-overdue mechanism to the IAM library to depend on Grants
that just got created, to ensure permissions are created before the
resources that depend on them (in effect: Grants are made
IDependable).

Relates to the root cause of #7236.

End Commit Message

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

Lambda Targets can get registered into a TargetGroup before the `lambda:Invoke` permissions are added. Since the TargetGroup does a permission test, deployment will fail. Fixes #4663. Adding a long-overdue mechanism to the IAM library to depend on Grants that just got created, to ensure permissions are created before the resources that depend on them (in effect: `Grant`s are made `IDependable`). Relates to the root cause of #7236.

aws-cdk-automation · 2020-04-24T16:16:31Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 28f3232
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

flemjame-at-amazon · 2020-04-25T01:14:01Z

I hit this issue and had no idea what the heck was happening. Subsequent synthesis and deployment worked fine, which really confused me.

I think this is a reasonable breaking change to make, especially given what's being fixed.

nija-at · 2020-04-28T08:57:57Z

What I'd actually like to do is break the return signature of role.addToPolicy(): boolean to a struct, otherwise it's going to be very hard to reliably implement this. Fairly sure this won't cause issues because few people will be checking the return value of addToPolicy(), but of course we technically speaking aren't allowed to do this.

Agree with your point. Not concerned about breaking a function changes its return type from void to something specific.

rix0rrr · 2020-04-28T09:02:03Z

Agree with your point. Not concerned about breaking a function changes its return type from void to something specific.

No but unfortunately it's changing from boolean to something specific :(

aws-cdk-automation · 2020-04-28T12:48:26Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 9c633e0
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-04-28T15:11:39Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: b626a06
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-04-28T16:16:59Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: ae4cf9d
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

packages/@aws-cdk/aws-ecr/lib/repository.ts

eladb · 2020-05-04T13:51:14Z

packages/@aws-cdk/aws-iam/lib/principals.ts

+    return this.addToIdentityPolicy(statement).statementAdded;
+  }
+
+  public addToIdentityPolicy(statement: PolicyStatement): AddToIdentityPolicyResult {


Should these be called addToPrincipalPolicy?

Maybe. Right now it's:

Principal: anything you can grant permissions to, that goes into a policy (user, role, but also service principal or account principal)

Identity: an IAM identity you create (user, role, group).

Policies only go on Identities, for example "adding to a service principal's policy" doesn't make any sense.

However, I will grant that this distinction might not be obvious, and it might confuse more than it helps.

Thoughts?

I am okay with that distinction although I also think it's not 100% required in this case and addToPrincipalPolicy would have been a bit clearer

eladb · 2020-05-04T13:54:09Z

packages/@aws-cdk/aws-iam/lib/principals.ts

+  /**
+   * Dependable which allows depending on the policy change being applied
+   *
+   * @default - Required if `statementAdded` is true.


If this is always required if statementAdded is true then why do we need statementAdded? We can rename this field to addedBy or owner and require that it is set when a statement was added.

owner doesn't really cover the load of what I'm trying to do here, which is strictly offer a way to add a dependency and nothing else.

As for statementAdded, yeah maybe it's superfluous. On the other hand it does make for more readable consumer code to say:

if (result.statementAdded) { /* do thing */ }

Rather than:

if (result.policyDependable !== undefined) { /* do a thing */ }

Open to a better name for the dependable though...

Damn I don't.... Can we make this "@experimental"?

eladb · 2020-05-04T13:54:50Z

What I'd actually like to do is break the return signature of role.addToPolicy(): boolean to a struct, otherwise it's going to be very hard to reliably implement this. Fairly sure this won't cause issues because few people will be checking the return value of addToPolicy(), but of course we technically speaking aren't allowed to do this.

This seems like something we should include in our v2.0 work. Add it to the list in aws/aws-cdk-rfcs#79

aws-cdk-automation · 2020-05-12T09:12:23Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 22e0db5
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-05-12T10:43:50Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: a799e13
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2020-05-12T10:44:13Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

rix0rrr requested a review from a team April 24, 2020 16:09

rix0rrr self-assigned this Apr 24, 2020

mergify bot added the contribution/core This is a PR that came from AWS. label Apr 24, 2020

Introduce new method after all

9c633e0

rix0rrr marked this pull request as ready for review April 28, 2020 13:29

Fix some more missed cases

b626a06

Merge branch 'master' into huijbers/elb-lambda-perms

ae4cf9d

eladb reviewed May 4, 2020

View reviewed changes

eladb mentioned this pull request May 5, 2020

CDK v2.0 aws/aws-cdk-rfcs#79

Closed

12 tasks

rix0rrr requested review from eladb and a team May 6, 2020 08:56

eladb added the pr/do-not-merge This PR should not be merged at this time. label May 7, 2020

eladb approved these changes May 7, 2020

View reviewed changes

Make @experimental, rename to addToPrincipalPolicy

22e0db5

rix0rrr removed the pr/do-not-merge This PR should not be merged at this time. label May 12, 2020

Merge branch 'master' into huijbers/elb-lambda-perms

a799e13

mergify bot merged commit 1819a6b into master May 12, 2020

mergify bot deleted the huijbers/elb-lambda-perms branch May 12, 2020 10:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(elbv2): race condition for Lambda backends #7598

fix(elbv2): race condition for Lambda backends #7598

rix0rrr commented Apr 24, 2020 •

edited

Loading

aws-cdk-automation commented Apr 24, 2020

flemjame-at-amazon commented Apr 25, 2020

nija-at commented Apr 28, 2020

rix0rrr commented Apr 28, 2020

aws-cdk-automation commented Apr 28, 2020

aws-cdk-automation commented Apr 28, 2020

aws-cdk-automation commented Apr 28, 2020

eladb May 4, 2020

rix0rrr May 5, 2020

eladb May 7, 2020

eladb May 4, 2020

rix0rrr May 5, 2020

eladb May 7, 2020

eladb commented May 4, 2020

aws-cdk-automation commented May 12, 2020

aws-cdk-automation commented May 12, 2020

mergify bot commented May 12, 2020

fix(elbv2): race condition for Lambda backends #7598

fix(elbv2): race condition for Lambda backends #7598

Conversation

rix0rrr commented Apr 24, 2020 • edited Loading

Commit Message

End Commit Message

aws-cdk-automation commented Apr 24, 2020

AWS CodeBuild CI Report

flemjame-at-amazon commented Apr 25, 2020

nija-at commented Apr 28, 2020

rix0rrr commented Apr 28, 2020

aws-cdk-automation commented Apr 28, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Apr 28, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Apr 28, 2020

AWS CodeBuild CI Report

eladb May 4, 2020

Choose a reason for hiding this comment

rix0rrr May 5, 2020

Choose a reason for hiding this comment

eladb May 7, 2020

Choose a reason for hiding this comment

eladb May 4, 2020

Choose a reason for hiding this comment

rix0rrr May 5, 2020

Choose a reason for hiding this comment

eladb May 7, 2020

Choose a reason for hiding this comment

eladb commented May 4, 2020

aws-cdk-automation commented May 12, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented May 12, 2020

AWS CodeBuild CI Report

mergify bot commented May 12, 2020

rix0rrr commented Apr 24, 2020 •

edited

Loading