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

(lambda): cross account lambda resolvers can not be created #18781

Closed
vdahlberg opened this issue Feb 2, 2022 · 9 comments · Fixed by #18979
Closed

(lambda): cross account lambda resolvers can not be created #18781

vdahlberg opened this issue Feb 2, 2022 · 9 comments · Fixed by #18979
Labels
@aws-cdk/aws-lambda Related to AWS Lambda bug This issue is a bug. effort/small Small work item – less than a day of effort p1

Comments

@vdahlberg
Copy link

vdahlberg commented Feb 2, 2022

What is the problem?

Can not create cross account lambda function resolver for an appsync api. It tries to add a permission to lambda but can not.
Worked in CDKv1 but not in CDKv2.

Reproduction Steps

import { aws_lambda, Stack, StackProps } from 'aws-cdk-lib';
import { Construct } from 'constructs';
import * as appsync from '@aws-cdk/aws-appsync-alpha';

export class GraphqlbugStack extends Stack {
  constructor(scope: Construct, id: string, props?: StackProps) {
    super(scope, id, props);

    const api = new appsync.GraphqlApi(this, 'Api', {
      name: 'demo',
    });

    const myLambda = aws_lambda.Function.fromFunctionArn(this, "function", "arn:aws:lambda:eu-west-1:111222333444:function:MyLambda")
    const demoDS = api.addLambdaDataSource("lambdaDataSource", myLambda)

    demoDS.createResolver({
      typeName: 'Query',
      fieldName: 'getDemos',
    });
    
  }
}

Then run cdk synth

What did you expect to happen?

Generates a cloudformation template.

What actually happened?

/Users/.../node_modules/aws-cdk-lib/aws-lambda/lib/function-base.ts:212
              throw new Error('Cannot modify permission to lambda function. Function is either imported or $LATEST version. '
                    ^
Error: Cannot modify permission to lambda function. Function is either imported or $LATEST version. If the function is imported from the same account use `fromFunctionAttributes()` API with the `sameEnvironment` flag.
    at Object.addToResourcePolicy (/Users/.../node_modules/aws-cdk-lib/aws-lambda/lib/function-base.ts:212:21)
    at Function.addToPrincipalOrResource (/Users/.../node_modules/aws-cdk-lib/aws-iam/lib/grant.ts:77:45)
    at Import.grantInvoke (/Users/.../node_modules/aws-cdk-lib/aws-lambda/lib/function-base.ts:195:25)
    at new LambdaDataSource (/Users/.../node_modules/@aws-cdk/aws-appsync-alpha/lib/data-source.ts:197:26)
    at GraphqlApi.addLambdaDataSource (/Users/.../node_modules/@aws-cdk/aws-appsync-alpha/lib/graphqlapi-base.ts:106:12)
    at new GraphqlbugStack (/Users/.../lib/graphqlbug-stack.ts:15:24)
    at Object.<anonymous> (/Users/.../bin/graphqlbug.ts:7:1)
    at Module._compile (node:internal/modules/cjs/loader:1101:14)
    at Module.m._compile (/Users/.../node_modules/ts-node/src/index.ts:1056:23)
    at Module._extensions..js (node:internal/modules/cjs/loader:1153:10)
Subprocess exited with error 1

CDK CLI Version

2.9.0

Framework Version

No response

Node.js Version

v16.13.2

OS

macOS Catalina

Language

Typescript

Language Version

TypeScript (4.5.5)

Other information

No response

@vdahlberg vdahlberg added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Feb 2, 2022
@github-actions github-actions bot added the @aws-cdk/aws-lambda Related to AWS Lambda label Feb 2, 2022
@ryparker ryparker added the p1 label Feb 2, 2022
@kaizencc kaizencc changed the title (@aws-cdk/aws-appsync-alpha): cross account lambda resolvers can not be created (appsync): cross account lambda resolvers can not be created Feb 3, 2022
@kaizencc kaizencc added @aws-cdk/aws-appsync Related to AWS AppSync and removed @aws-cdk/aws-lambda Related to AWS Lambda labels Feb 3, 2022
@kaizencc
Copy link
Contributor

kaizencc commented Feb 3, 2022

The issue here is that an imported lambda cannot be modified; this is true for both v1 and v2. Can you share what was working in v1?

I think the issue here is that we cannot grantInvoke an imported lambda. So we should not do this if the lambda is imported. It will then be on the user to make sure the imported lambda has the right permissions.

props.lambdaFunction.grantInvoke(this);

@kaizencc kaizencc added effort/small Small work item – less than a day of effort and removed needs-triage This issue or PR still needs to be triaged. labels Feb 3, 2022
@kaizencc kaizencc removed their assignment Feb 3, 2022
@vdahlberg
Copy link
Author

I was confused when it didn't work on cdk v1.143.0 (same error as above). However, when downgrading to cdk v1.130.0, it does work.

This is the example synths fine with cdk v1.130.0;

import * as cdk from '@aws-cdk/core';
import * as appsync from '@aws-cdk/aws-appsync';
import * as lambda from '@aws-cdk/aws-lambda'

export class Appsynccdk1BugStack extends cdk.Stack {
  constructor(scope: cdk.Construct, id: string, props?: cdk.StackProps) {
    super(scope, id, props);

    const api = new appsync.GraphqlApi(this, 'Api', {
      name: 'demo',
    });

    const fn = lambda.Function.fromFunctionArn(this, "mylambda", "arn:aws:lambda:eu-west-1:111222333444:function:MyLambda")

    const dataSource = api.addLambdaDataSource("lambdaDataSource", fn)


    dataSource.createResolver({
      typeName: 'Query',
      fieldName: 'getDemos'
    })

  }
}

So somewhere between cdk v1.130.0 and cdk v1.143.0 it stopped working.

@sseidel16
Copy link

sseidel16 commented Feb 11, 2022

This issue is not necessarily related to appsync. It is an issue when grantInvoke is called on a cross-account lambda. The stack trace shows that appsync is calling grantInvoke internally. This used to work and now it does not.

I am also having this issue after upgrading cdk to 1.143.0. Calling grantInvoke on a cross-account lambda fails with the above error. Previously, my stacks worked just fine, and calling grantInvoke only modified the iam.IGrantable identity, which is not a cross-account resource, so this seems like it should work fine. What changed?

I discovered this happened going from version 1.38.2 to 1.39.0. But looking at the changes, I cannot seem to find what broke it.

@sseidel16
Copy link

It looks like going from version 1.38.2 to 1.39.0 causes the issue.

@kaizencc
Copy link
Contributor

Apologies for my initial comment, it does not paint the full picture here. Here's what I think:

The commit in question here is #18255, thank you @sseidel16 for narrowing down the versions so that this commit was easy to find. What the commit changes is that it grabs the environment from the function arn during lambda.fromFunctionAttributes(). This function gets called from lambda.fromFunctionArn() as well.

So at CDK v1.138.2, we did not supply the environment (region/account) during fromFunctionAttributes(), which was a bug. We start doing that in v1.139.0.

Now, why this causes your code to break:

const permissionNode = this._functionNode().tryFindChild(identifier);
if (!permissionNode) {
throw new Error('Cannot modify permission to lambda function. Function is either imported or $LATEST version. '
+ 'If the function is imported from the same account use `fromFunctionAttributes()` API with the `sameEnvironment` flag.');
}

This is the offending code, and I find it perfectly reasonable. We cannot modify your imported lambda, and we are trying to. So the question becomes, why did this not happen before?

When we call lambda.grantInvoke(), under the hood the function calls iam.Grant.addToPrincipalOrResource. And inside that function we consider the environment of the lambda:

const resourceAndPrincipalAccountComparison = options.grantee.grantPrincipal.principalAccount
? cdk.Token.compareStrings(options.resource.env.account, options.grantee.grantPrincipal.principalAccount)
: undefined;
// if both accounts are tokens, we assume here they are the same
const equalOrBothUnresolved = resourceAndPrincipalAccountComparison === cdk.TokenComparison.SAME
|| resourceAndPrincipalAccountComparison == cdk.TokenComparison.BOTH_UNRESOLVED;
const sameAccount: boolean = resourceAndPrincipalAccountComparison
? equalOrBothUnresolved
// if the principal doesn't have an account (for example, a service principal),
// we should modify the resource's trust policy
: false;
// If we added to the principal AND we're in the same account, then we're done.
// If not, it's a different account and we must also add a trust policy on the resource.
if (result.success && sameAccount) {
return result;
}

Specifically, options.resource.env.account. In CDK v1.138.2, we would not send in the environment of the imported lambda, and instead default to the current stack's account (which is a bug). In CDK v1.139.0, we would provide the environment of the imported lambda.

So the variable resourceAndPrincipalAccountComparison has changed in this use case. We used to (wrongly) consider the imported lambda as the same account. We now (correctly) find that we are granting to a cross-account lambda and "we must also add a trust policy on the resource." Obviously, we cannot actually alter the imported lambda trust policy but we try anyway, and that's the error.

tl;dr: we fixed a bug that your use case relied on. I have a feeling that while your code deployed successfully in CDK v1.138.2, the imported lambda did not have the permissions to allow appsync to call it.

@skinny85, you were the contributor who fixed the initial bug, can you please confirm my logic and suggest what you think we should do here?

@kaizencc kaizencc changed the title (appsync): cross account lambda resolvers can not be created (lambda): cross account lambda resolvers can not be created Feb 14, 2022
@kaizencc kaizencc added @aws-cdk/aws-lambda Related to AWS Lambda and removed @aws-cdk/aws-appsync Related to AWS AppSync labels Feb 14, 2022
@sseidel16
Copy link

sseidel16 commented Feb 14, 2022

@kaizen3031593 thanks for the explanation. This really comes down to the line "we must also add a trust policy on the resource." I assumed grantInvoke only modified the iam.IGrantable.

I believe it's not necessarily correct that "the imported lambda did not have the permissions to allow appsync to call it." Cross-account lambdas are often owned by other teams and these permissions are then added separately.

We have a use case currently where the cross-account lambda is owned by somebody else, and they have added the proper permissions to it manually. The only thing needed is to allow the invoke action on our lambda's role. The entire flow worked prior (build, deployment, invoke), and now breaks because it's trying to modify the resource as well (and this has already been done manually).

I am not saying the current behavior is correct or incorrect. It simply is whether the imported resource should be modified. I would argue strongly that it should not be, since it is an imported resource, but I understand if others disagree.

@kaizencc
Copy link
Contributor

We seem to have stumbled on a rabbit hole here that was most recently modified here: #11369 (and that PR links to at least 2 others trying to do the same thing).

There are a few attributes you can try from fromFunctionAttributes() that are related: sameEnvironment and allowPermissions. But after hearing the use case you describe, I wonder if these will both still result in deploy time failures (since there is no way to attempt to modify imported resources). Perhaps we need to introduce a property like skipPermissions to address this use case.

At any rate, I'm a bit out of my depth and I'll wait for @skinny85 to weigh in.

@skinny85
Copy link
Contributor

I agree with everything @kaizen3031593 said 🙂.

@mergify mergify bot closed this as completed in #18979 Feb 16, 2022
mergify bot pushed a commit that referenced this issue Feb 16, 2022
…ured permissions (#18979)

There have been a few reports of regressions after #18228 was merged in v1.139.0: #18781, #18967.

AFAIK, #18228 fixed a bug in our environment configuring logic for `fromFunctionAttributes()`, so the environment is now being properly passed to the `functionBase` that CDK creates. The bug was being relied on by CDK customers for a particular use case: calling `grantInvoke()` on imported cross-account lambdas with pre-configured permissions. In general, `grantInvoke()` modifies the trust policy of the resource if it is not from the same environment. In the case of cross-account lambdas, this is impossible, and results in a synth-time error. CDK users know this and have modified the lambda permissions in question to have the correct permissions. These users do not need permissions added to the cross-account lambda, so it is not an error.

To support this use case, I'm introducing `skipPermissions` as a property for `fromFunctionAttributes()`. When set, we skip this synth-time check all together. Beware, users who set this property are now on their own for the permissions of their cross-account lambda.

```ts
const fn = lambda.Function.fromFunctionAttributes(stack, 'Function', {
  functionArn: 'arn:aws:lambda:us-east-1:123456789012:function:MyFn',
  skipPermissions: true,
});
```

Closes #18781. 

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this issue Feb 21, 2022
…ured permissions (aws#18979)

There have been a few reports of regressions after aws#18228 was merged in v1.139.0: aws#18781, aws#18967.

AFAIK, aws#18228 fixed a bug in our environment configuring logic for `fromFunctionAttributes()`, so the environment is now being properly passed to the `functionBase` that CDK creates. The bug was being relied on by CDK customers for a particular use case: calling `grantInvoke()` on imported cross-account lambdas with pre-configured permissions. In general, `grantInvoke()` modifies the trust policy of the resource if it is not from the same environment. In the case of cross-account lambdas, this is impossible, and results in a synth-time error. CDK users know this and have modified the lambda permissions in question to have the correct permissions. These users do not need permissions added to the cross-account lambda, so it is not an error.

To support this use case, I'm introducing `skipPermissions` as a property for `fromFunctionAttributes()`. When set, we skip this synth-time check all together. Beware, users who set this property are now on their own for the permissions of their cross-account lambda.

```ts
const fn = lambda.Function.fromFunctionAttributes(stack, 'Function', {
  functionArn: 'arn:aws:lambda:us-east-1:123456789012:function:MyFn',
  skipPermissions: true,
});
```

Closes aws#18781. 

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-lambda Related to AWS Lambda bug This issue is a bug. effort/small Small work item – less than a day of effort p1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants