Skip to content
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.

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

fix(lambda): expose underlying function's role on the alias #2024

Merged
merged 2 commits into from
Mar 19, 2019

Conversation

sam-goodwin
Copy link
Contributor

Fixes #2023


Pull Request Checklist

  • Testing
    • Unit test added (prefer not to modify an existing test, otherwise, it's probably a breaking change)
    • CLI change?: coordinate update of integration tests with team
    • cdk-init template change?: coordinated update of integration tests with team
  • Docs
    • jsdocs: All public APIs documented
    • README: README and/or documentation topic updated
  • Title and Description
    • Change type: title prefixed with fix, feat will appear in changelog
    • Title: use lower-case and doesn't end with a period
    • Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
    • Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
  • Sensitive Modules (requires 2 PR approvers)
    • IAM Policy Document (in @aws-cdk/aws-iam)
    • EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
    • Grant APIs (only if not based on official documentation with a reference)

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

@sam-goodwin sam-goodwin requested a review from a team as a code owner March 15, 2019 08:36
@rix0rrr
Copy link
Contributor

rix0rrr commented Mar 15, 2019

I approved this, but is the functionArn going to have the right value in the cases where we want to use it?

I still approve the refactoring to add an invokeArn or some such, so functionArn can definitely always point to the function that we're going to need to grant IAM permissions on.

@sam-goodwin
Copy link
Contributor Author

It will be ${functionArn}:${aliasName}.

is the functionArn going to have the right value in the cases where we want to use it?

Good question, I'm not sure. What are the cases in which the underlying functionArn should be used instead of the alias's arn? It will work correctly for any invocation policy or permission (grantInvoke, asStepFunctionsTaskResource, asBucketNotificationDestination, etc.). I wonder if some metrics would be broken, like metricThrottles?

@rix0rrr
Copy link
Contributor

rix0rrr commented Mar 15, 2019

Bee tee dubs, what does the alias even point to? Is it a Version?

@rix0rrr
Copy link
Contributor

rix0rrr commented Mar 15, 2019

Ah I see they're very strict:

https://docs.aws.amazon.com/lambda/latest/dg/versioning-aliases-permissions.html

So for IAM permissions we need to use the fully qualified ARN that matches the ARN of Version/Alias object.

For metrics we always need the real Lambda's functionName.

But I think right now the functionName field might contain the ARN, because Lambda is very flexible in how it allows functions to be addressed (functionName or ARN are both okay), and most of the input fields are called functionName I think. So users would be tempted to write:

new ConstructThatCallsLambda(this, 'CTCL', {
   functionName: myFunction.functionName
});

At that point, if myFunction is actually an Alias, we want functionName to contain the ARN so that we're actually calling the Alias and not the Function directly.

EDIT: It's probably fine if Alias doesn't have the metricsXxx() functions? In that case, there's no mess-up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

lambda.Alias does not expose the role associated with the lambda.Function
3 participants