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

appsync apikey missing dependency on schema? #8168

Closed
serverlessunicorn opened this issue May 23, 2020 · 6 comments · Fixed by #9737
Closed

appsync apikey missing dependency on schema? #8168

serverlessunicorn opened this issue May 23, 2020 · 6 comments · Fixed by #9737
Assignees
Labels
@aws-cdk/aws-appsync Related to AWS AppSync bug This issue is a bug. in-progress This issue is being actively worked on. p1

Comments

@serverlessunicorn
Copy link

serverlessunicorn commented May 23, 2020

I hit ConcurrentModificationExceptions consistently if I don't manually insert a dependency between each Resolver and the schema of its appsync API. (Note that the dependency between the resolver and the API itself is ok.)

Reproduction Steps

Create any appsync schema with multiple dynamo resolvers (though probably any type will have the same problem).

Error Log

ConcurrentModification exceptions, followed by rollback, unless you manually add every dependency by hand

Environment

  • CLI Version : 1.39
  • Framework Version:
  • **OS : macos **
  • Language : python

Other


This is 🐛 Bug Report

@serverlessunicorn serverlessunicorn added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels May 23, 2020
@SomayaB SomayaB added the @aws-cdk/aws-appsync Related to AWS AppSync label May 26, 2020
@MrArnoldPalmer
Copy link
Contributor

Oooh this makes sense. Just to clarify, when you say manually add dependency you mean with Construct.addDependency right?

@MrArnoldPalmer MrArnoldPalmer added p1 and removed needs-triage This issue or PR still needs to be triaged. labels May 27, 2020
@serverlessunicorn
Copy link
Author

serverlessunicorn commented May 28, 2020 via email

@fulghum
Copy link
Contributor

fulghum commented Aug 4, 2020

I was talking with @serverlessunicorn about this issue and it sounds like it's hitting him and his dev team more often lately...

The error coming back is:
CREATE_FAILED for AWS::AppSync::ApiKey | Schema is currently being altered, please wait until that is complete. (Service: AWSAppSync; Status Code: 409; Error Code: ConcurrentModificationException

I see that we already have a dependency declared from the Resolver resource to the API schema resource:

this.resolver.addDependsOn(props.api.schema);

Are we missing a dependency from the API key resource to the API schema resource as well?

this._apiKey = this.createAPIKey(apiKeyConfig);

Any other ideas?

@BryanPan342 BryanPan342 self-assigned this Aug 4, 2020
@BryanPan342
Copy link
Contributor

@serverlessunicorn I've been trying to recreate this bug but to no avail. I'm guessing the issue has a higher likelihood of arising when the GraphQL Schema is bigger?

@fulghum suggested to test by adding a dependency between the Api Key and the Api Schema. Like so

api.apiKey().addDependency(api.schema);

Unfortunately the property apiKey in cdk returns a string not the CfnResource. So there is no easy way I can see to add a dependency between ApiKey and the schema at the moment.

I can add a link between ApiKey and Schema in this PR #9122 and push for it to get merged by the end of next week. If that sounds alright with the two of you?

@fulghum
Copy link
Contributor

fulghum commented Aug 13, 2020

Thanks @BryanPan342 😄 Is there an easy way to use the Escape Hatch mechanism to pop open the construct and get the direct CfnResource for the key and schema? Sharing a snippet of that code would enable @serverlessunicorn to try out the idea in his code. In particular, that would prevent him from having to dig around for the right node names in the L2 construct library code to access the right resource objects through the escape hatch.

Otherwise... if we are confident that adding an explicit dependency between the Key and the Schema resource objects is safe and correct, then it would be great to get that change in. I'm not super deep on AppSync, so I will defer to you and @MrArnoldPalmer or @shivlaks for the safe thing to do there.

Thanks for helping us close up this gap! 😄

@BryanPan342
Copy link
Contributor

BryanPan342 commented Aug 14, 2020

@fulghum I found a way to make your own api key through the L1 constructs.

Thankfully, the CfnGraphQLSchema is exposed by the GraphQL Api 😀 so the work just becomes creating an Api Key that is attached to the GraphQL API.

@serverlessunicorn I hope this workaround helps!

In order to do this and prevent big headache I'm going to list two scenarios:

  1. If you want Api Key to be the default authentication type:

    Default Authentication Workaround

    If you want your API to default to Api Key authentication, first set the AuthorizationType to IAM because it requires no configuration.

     api = appsync.GraphQLApi(self, 'api',
       name='demo',
       schema_definition_file=os.path.join(os.getcwd(), "schema.graphql"),
       authorization_config=appsync.AuthorizationConfig(
         default_authorization=appsync.AuthorizationMode(
           authorization_type=appsync.AuthorizationType.IAM
         ),
       )
     )

    Next, override the CloudFormation property AuthenticationType from AWS_IAM to API_KEY

     # Expose CfnResource
     cfn_api = api.node.default_child 
    
     # Override its property to have default AuthenticationType to be ApiKey
     cfn_api.add_property_override("AuthenticationType", "API_KEY")

    Finally, create the CfnApiKey for your GraphQL API

     # Create Api Key
     api_key = appsync.CfnApiKey(self, 'ApiKey', api_id=api.api_id)
    
     # Add Dependency to Schema
     api_key.add_depends_on(api.schema)
  2. If you want Api Key to be an additional Authentication Mode:

    Additional Authentication Workaround

    Given any GraphQL Api configuration (as long as you don't specify containing configuration for ApiKey)

    Override the CloudFormation property AdditionalAuthenticationProviders to include API_KEY.

     # Expose CfnResource
     cfn_api = api.node.default_child 
    
     # Override Property to add API_KEY
     # (if you have other additional providers, you will have to add them here as well)
     cfn_api.add_property_override("AdditionalAuthenticationProviders", [ 
         { "AuthenticationType": "API_KEY"} 
     ])

    Finally, create the CfnApiKey for your GraphQL API

     # Create Api Key
     api_key = appsync.CfnApiKey(self, 'ApiKey', api_id=api.api_id)
    
     # Add Dependency to Schema
     api_key.add_depends_on(api.schema)

To learn more about Escape Hatches, see these docs

The most important part of all of this is to override the CloudFormation Property, such that AppSync recognizes that Api Key is a legal authentication type: without it, the Api Key will not work.

@BryanPan342 BryanPan342 changed the title appsync Resolver missing dependency on schema? appsync apikey missing dependency on schema? Aug 18, 2020
@BryanPan342 BryanPan342 added the in-progress This issue is being actively worked on. label Aug 18, 2020
@mergify mergify bot closed this as completed in #9737 Aug 19, 2020
mergify bot pushed a commit that referenced this issue Aug 19, 2020
Added dependency between the CfnApiKey and CfnSchema. The dependency here is to prevent a `ConcurrencyModificationError` as seen in #8168. We allow this dependency to exist because from referencing the [docs](https://docs.aws.amazon.com/appsync/latest/APIReference/API_CreateApiKey.html#API_CreateApiKey_Errors) there shouldn't be any issue between creating an api key before or after schema creation. 

Also make ApiKeyConfig correctly configure the ApiKey when used in `additionalAuthorizationModes`.

Fixes #9736
Fixes #8168 

----

*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-appsync Related to AWS AppSync bug This issue is a bug. in-progress This issue is being actively worked on. p1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants