aws-amplify / amplify-category-api

The AWS Amplify CLI is a toolchain for simplifying serverless web and mobile development. This plugin provides functionality for the API category, allowing for the creation and management of GraphQL and REST based backends for your amplify project.
https://docs.amplify.aws/
Apache License 2.0
89 stars 79 forks source link

Gen 2 - REST API Custom Auth - Deployment failed: Error [ValidationError]: Circular dependency between resources #2375

Open alexwhb opened 8 months ago

alexwhb commented 8 months ago

Amplify CLI Version

System: OS: macOS 14.3.1 CPU: (16) arm64 Apple M3 Max Memory: 446.17 MB / 48.00 GB Shell: /bin/zsh Binaries: Node: 18.18.0 - ~/.nvm/versions/node/v18.18.0/bin/node Yarn: undefined - undefined npm: 9.8.1 - ~/.nvm/versions/node/v18.18.0/bin/npm pnpm: undefined - undefined NPM Packages: @aws-amplify/backend: 0.13.0-beta.9 @aws-amplify/backend-cli: 0.12.0-beta.10 aws-amplify: 6.0.21 aws-cdk: 2.133.0 aws-cdk-lib: 2.133.0 typescript: 5.4.2 AWS environment variables: AWS_STS_REGIONAL_ENDPOINTS = regional AWS_NODEJS_CONNECTION_REUSE_ENABLED = 1 AWS_SDK_LOAD_CONFIG = 1 No CDK environment variables%

Question

I'm attempting to setup a REST API but am struggling with setting up my auth system. I'm trying to do a very simple lambda auth function that just checks a auth token header. with restAuth

I'm thinking this is likely something I'm doing wrong... not a bug with Amplify, though I'm not totally sure.

but whenever I try deploying this I get this error:

The CloudFormation deployment has failed.
Caused By: :x: Deployment failed: Error [ValidationError]: Circular dependency between resources: [data7552DF31, apigatewaystackE9277FBE, function1351588B]

I know it's definitely related to the restAuth code, because as soon as I comment that out it deploys without issue. What could I be doing wrong?

Here's what my backend ts file looks like:

export const backend = defineBackend({
    auth,
    data,
    myRestHandler,
    getEntitlements,
    loginRestHandler,
    addEntitlementsByCreditRest,
    restAuth
});

const apiGatewayStack = backend.createStack("apigateway-stack");

const myAPI = new LambdaRestApi(apiGatewayStack, "MyApi", {
    handler: backend.myRestHandler.resources.lambda,
    proxy: false,
});

// THIS IS THE CODE CAUSING ISSUES
const restAuthMod = new TokenAuthorizer(apiGatewayStack, 'user-Auth', {
    handler: backend.restAuth.resources.lambda,
});

const account = myAPI.root.addResource('account')
const loginRest = new LambdaIntegration(backend.loginRestHandler.resources.lambda)

account.addResource('login')
    .addMethod('POST', loginRest)

const getEntitlementsRest = new LambdaIntegration(backend.getEntitlements.resources.lambda)

account.addResource('library', {
    defaultMethodOptions: {authorizationType: AuthorizationType.CUSTOM, authorizer: restAuthMod}
}).addMethod('GET', getEntitlementsRest)

account.addResource('add-to-library', {
   // defaultMethodOptions: {authorizationType: AuthorizationType.CUSTOM, authorizer: restAuthMod}
}).addResource('credits')
  .addMethod('POST', new LambdaIntegration(backend.addEntitlementsByCreditRest.resources.lambda))

backend.addOutput({

    custom: {
        apiId: myAPI.restApiId,
        apiEndpoint: myAPI.url,
        apiName: myAPI.restApiName,
        apiRegion: Stack.of(apiGatewayStack).region,
    },
});
phani-srikar commented 8 months ago

Hi @alexwhb, I assume you've previously defined the variables like restAuth you're passing to create the backend. Instead of fetching the needed variables like backend. restAuth, can you try directly using restAuth? Could you share the full snippet with definitions for restAuth and if possible other variables so we can better assist you?

alexwhb commented 8 months ago

@phani-srikar Absolutely.

restAuth is defined in functions/rest-auth the resource.ts looks like this:

import {defineFunction} from "@aws-amplify/backend";

export const restAuth = defineFunction( )

I'm just looking up a sessionID in my dynomo table to validate it. Note data client is just the Amplify graphQL client. Also note, while this function is defined, I've never actually run it because of that deployment issue when I try to connect it to my REST API. So my implementation is a best effort first attempt. Probably some modification will be needed. and the handler.ts looks like this:

import {dataClient} from "../../utils/data-client"; 
export type SessionInfo = {
    userId: string;
    createdAt: string;
    sessionId: string; // this is the userId
} | undefined

// Function to look up SessionID in the Sessions table
async function lookupSessionID(sessionID: string): Promise<SessionInfo | null> {
    const {data, errors} = await dataClient.models.Session.get({id: sessionID})

    if (errors) {
        throw new Error(errors.join(','))
    }

    // const session = await getSession(sessionID)
    console.log(JSON.stringify(data), data?.userSessionsId, sessionID)

    if (data == null || data.userSessionsId == null) {
        throw new Error("Invalid session")
    }

    return {
        userId: data.userSessionsId,
        sessionId: data.id,
        createdAt: data.createdAt,
    };
}

export const handler = async (
    event: any
) => {
    console.log(event)

    const sessionInfo = await lookupSessionID(event.authorizationToken)

    console.log(`RESPONSE: ${JSON.stringify(sessionInfo, null, 2)}`);

    return {
        principalId: 'user',
        policyDocument: {
            Version: '2012-10-17',
            Statement: [{
                Action: 'execute-api:Invoke',
                Effect: sessionInfo == null ? 'Deny': 'Allow',
                Resource: event.methodArn
            }],
        },
        context: sessionInfo // this is our session info to pass to the rest handler
    };
};

Here's a slightly abbreviated version of my schema:

const schema = a.schema({
    Session: a.model({
        user: a.belongsTo('User'),
        appCode: a.string().required()
    }).authorization([a.allow.public('iam')]),

    User: a.model({
        entitlements: a.hasMany('Entitlement'),
        sessions: a.hasMany('Session'),
        credits: a.integer(),
    }).authorization([a.allow.public('iam')]),
    Entitlement: a
        .model({
            user: a.belongsTo('User'),
            productId: a.string().required(),
            squareImageUrl: a.string().required(),
            bookId: a.string().required(),
        }).authorization([a.allow.public('iam')]),

})
    .authorization([
        a.allow.resource(loginRestHandler),
        a.allow.resource(restAuth).to(['query']),
        a.allow.resource(getEntitlements),
    ]);

export type Schema = ClientSchema<typeof schema>;

export const data = defineData({
    schema,
    name: "MyLibrary",
    functions: {},

    authorizationModes: {
        defaultAuthorizationMode: 'iam',
    },
});

Let me know if any other details would be helpful. Happy to provide them.

phani-srikar commented 8 months ago

Instead of fetching the needed variables like backend. restAuth, can you try directly using restAuth?

Can you try this and let us know if it works?

alexwhb commented 8 months ago

@phani-srikar Thanks for getting back to me. Do you mean like this:

# where rest auth is just my imported defineFunction()
const addToLibrary = account.addResource('add-to-library', {
   defaultMethodOptions: {authorizationType: AuthorizationType.CUSTOM, authorizer: restAuth}
})

or are you meaning like this:

const restAuthMod = new TokenAuthorizer(apiGatewayStack, 'user-Auth', {
     handler: restAuth
});

then using restAuthMod here:

const addToLibrary = account.addResource('add-to-library', {
   defaultMethodOptions: {authorizationType: AuthorizationType.CUSTOM, authorizer: restAuthMod}
})

Or something else?

I tried the above two and unless I'm missing something the types are not appropriate.

chrisbonifacio commented 7 months ago

Hi @alexwhb apologies for the delay. Could you share more of your code or provide a minimal sample repo that reproduces the issue so that we can troubleshoot this on our end?

I tried to use the code you shared but I'm missing some things like:

myRestHandler,
getEntitlements,
loginRestHandler,
addEntitlementsByCreditRest
chrisbonifacio commented 7 months ago

UPDATE: I was able to reproduce the issue on this repo and branch:

https://github.com/chrisbonifacio/amplify-gen2-app/tree/circular-dep-error

We will report back once we've found a solution

alexwhb commented 7 months ago

@chrisbonifacio Awesome!!! You're the best. Thanks for looking into this. My temporary solution is to just wrap all my lambdas in auth middleware, but it'll be super nice to have auth decoupled.

LukaASoban commented 6 months ago

@chrisbonifacio were you ever able to find a solution to this one? :)

LukaASoban commented 6 months ago

@alexwhb could you explain what you meant here by wrapping them all in auth middleware?

alexwhb commented 6 months ago

@LukaASoban ya I just use the middy library which allows you to wrap your handlers in different middleware. I use that for authentication as well as input validation. The downside to this is the handlers themselves are doing the authentication every single request, so it's certainly not as efficient since there's no caching.

phani-srikar commented 5 months ago

@LukaASoban can you try this workaround to re-arrange your resources b/w stacks - https://github.com/aws-amplify/amplify-backend/issues/1552#issuecomment-2138331128

dpilch commented 2 months ago

Adding the TokenAuthorizer creates a cyclical dependency between the API GW stack and the lambda function stack. You can break the dependency by supplying the auth function ARN to the TokenAuthorizer.

const func = lambda.Function.fromFunctionArn(
  apiGatewayStack,
  "AuthFunction",
  backend.restAuth.resources.lambda.functionArn
);
const restAuthMod = new TokenAuthorizer(apiGatewayStack, "user-Auth", {
  handler: func,
});
LukaASoban commented 1 month ago

@dpilch would you say the above is the recommended approach to remove this dependency or are you all working on a permanent fix where I don't have to do this for every function?

dpilch commented 1 month ago

The root issue is that all functions in Gen 2 are defined in a single CFN stack. When any function relies on the data stack and the data stack relies on any function there will be a circular dependency between the stacks. If there is just a single link you should only need to use the workaround once. If there are many links you will have to use workaround many times unfortunately.

We don't have a fix in the works at the moment because it doesn't appear that this a common issue. If others are experiencing this issue please comment so we can adjust our priority.

zelbazk commented 1 month ago

bump. As projects get larger, dependency between any function with any data resource becomes more likely.

wethanet commented 1 month ago

Definitely an issue for us. Really common to have a function triggered by a data stream and a function requiring access to data

6ixNugget commented 4 weeks ago

The root issue is that all functions in Gen 2 are defined in a single CFN stack. When any function relies on the data stack and the data stack relies on any function there will be a circular dependency between the stacks. If there is just a single link you should only need to use the workaround once. If there are many links you will have to use workaround many times unfortunately.

We don't have a fix in the works at the moment because it doesn't appear that this a common issue. If others are experiencing this issue please comment so we can adjust our priority.

I'm very confused by this statement. Data must depend on functions if we are setting per function authorization rules, allowing only limited data access to the functions. In this line of logics, you are saying no functions should have dependencies on data? Am I missing something here?

And of course, if my understanding is correct, this will not only be a "common issue", it will be the single most repeated design pattern. So i guess I must have confused myself. Please enlighten me.

zelbazk commented 4 weeks ago

Yeah what we've started doing is creating NestedStacks and NodeJsFunctions from CDK instead of defineFunction set up through definedBackend. It's a shame, defineFunction is 100% better DX than NodeJsFunction, but the latter allows us to avoid the circular dependency issues, if this helps anyone in the mean time.

why-silvio commented 2 weeks ago

100% an issue. For example, I am creating a SQS queue in backend.ts and then I want to send a message to this queue inside a lambda function. How can I get the SQS URL to the lambda function? When I try to add it as an environment variable to the function I get the circular dependency issue. Is there any other way to do this?

Case 2: I want to trigger a state machine inside a lambda Case 3: I want to simply invoke another lambda within a lambda

Right now I am helping myself with placing the ARN of the dev or prod lambda hardcoded in an env file (but this approach has many downsides and the sandbox approach doesn't work).

@dpilch I saw you are very active on this topic, do you have a hint?