Added support for appsync_graphql_api schema dn appsync_resolver #6451
Conversation
ghost
added
size/XL
|
@mathoshek Thanks for implementing this. I tested it out on my AWS account and I ran into trouble with a race condition I think. I applied a terraform configuration that had a aws_appsync_graphql_api resource and multiple aws_appsync_resolver resources and I got the following error: So I think there is an issue with AWS providing status = "SUCCESS" from the get-schema-creation-status API and it actually not being ready. I think that we should create a work around for this. What I added was to the |
|
After running apply and destroy a few times I decided to add the following patch: diff --git a/aws/resource_aws_appsync_resolver.go b/aws/resource_aws_appsync_resolver.go
index 2f11754af..e1221661a 100644
--- a/aws/resource_aws_appsync_resolver.go
+++ b/aws/resource_aws_appsync_resolver.go
@@ -3,9 +3,13 @@ package aws
import (
"fmt"
"github.com/aws/aws-sdk-go/aws"
+ "github.com/aws/aws-sdk-go/aws/awserr"
"github.com/aws/aws-sdk-go/service/appsync"
+ "github.com/hashicorp/terraform/helper/resource"
"github.com/hashicorp/terraform/helper/schema"
+ "regexp"
"strings"
+ "time"
)
func resourceAwsAppsyncResolver() *schema.Resource {
@@ -67,7 +71,19 @@ func resourceAwsAppsyncResolverCreate(d *schema.ResourceData, meta interface{})
ResponseMappingTemplate: aws.String(d.Get("response_template").(string)),
}
- _, err := conn.CreateResolver(input)
+ err := resource.Retry(30*time.Second, func() *resource.RetryError {
+ _, err := conn.CreateResolver(input)
+ pattern := regexp.MustCompile("Schema is currently being altered, please wait until that is complete\\.")
+ if err != nil {
+ if awsErr, ok := err.(awserr.Error); ok {
+ if awsErr.Code() == "ConcurrentModificationException" && pattern.MatchString(awsErr.Message()) {
+ return resource.RetryableError(err)
+ }
+ }
+ return resource.NonRetryableError(err)
+ }
+ return nil
+ })
if err != nil {
return err
}
@@ -120,7 +136,19 @@ func resourceAwsAppsyncResolverUpdate(d *schema.ResourceData, meta interface{})
ResponseMappingTemplate: aws.String(d.Get("response_template").(string)),
}
- _, err := conn.UpdateResolver(input)
+ err := resource.Retry(30*time.Second, func() *resource.RetryError {
+ _, err := conn.UpdateResolver(input)
+ pattern := regexp.MustCompile("Schema is currently being altered, please wait until that is complete\\.")
+ if err != nil {
+ if awsErr, ok := err.(awserr.Error); ok {
+ if awsErr.Code() == "ConcurrentModificationException" && pattern.MatchString(awsErr.Message()) {
+ return resource.RetryableError(err)
+ }
+ }
+ return resource.NonRetryableError(err)
+ }
+ return nil
+ })
if err != nil {
return err
}
@@ -143,7 +171,19 @@ func resourceAwsAppsyncResolverDelete(d *schema.ResourceData, meta interface{})
FieldName: aws.String(fieldName),
}
- _, err = conn.DeleteResolver(input)
+ err = resource.Retry(30*time.Second, func() *resource.RetryError {
+ _, err := conn.DeleteResolver(input)
+ pattern := regexp.MustCompile("Schema is currently being altered, please wait until that is complete\\.")
+ if err != nil {
+ if awsErr, ok := err.(awserr.Error); ok {
+ if awsErr.Code() == "ConcurrentModificationException" && pattern.MatchString(awsErr.Message()) {
+ return resource.RetryableError(err)
+ }
+ }
+ return resource.NonRetryableError(err)
+ }
+ return nil
+ })
if err != nil {
return err
}This retries the actions on the resolver if the schema is being modified. Is there a better approach? |
|
Hello and thank you for the review! Indeed I didn't test with two or more resolvers, scenario which tonight I want to catch in an acc test maybe. I'll come back with a commit tonight. |
|
@kurtmc I managed to catch the error in a test and reused the retryOnAwsCode function. What do you think? |
|
@mathoshek that is really nice. I pulled this branch and ran apply and destroy a couple of times and everything went smoothly. btw I am not affiliated with terraform or the terraform-aws-provider so I think we need someone who is to review and accept this. I am happy to run tests for this against my AWS account if that helps getting this merged. |
@kurtmc, I'm not familiar with Go, so I can't judge what's happening by the PR code, but the "ConcurrentModificationException" error appeared in my code too. We deploy resolvers with JS code using AWS SDK. The reason for the error was not that the schema wasn't ready, but because I tried to create few resolvers in parallel. After I changed the code to create resolvers serially the error stopped happening. Maybe it is better to change the code to a "serial mode" rather than do retries? |
|
@wips I don't think it's possible in this resource implementation. The responsibility of this terraform resource is to create/update/delete a single resolver, but a common use case is to create multiple resolvers in terraform, and you would do this by creating multiple blocks that would look something like this: You could potentially make |
|
@kurtmc, ok, I see your point - we might have resolvers for different schemas and those resolvers won't conflict with one another, so we can't just create/update all resolvers serially. Anyway, my point is that catching the Is it possible to look at the Sorry if my questions are dumb, I don't know Go and have never contributed into terraform - just trying to make your contributions more effective. Thank you for what you're doing @mathoshek and @kurtmc. |
|
We have a handy helper for creating a provider-wide mutex: // Handle ConcurrentModificationException: Schema is currently being altered, please wait until that is complete.
mutexKey := fmt.Sprintf("appsync-schema-%s", d.Get("api_id").(string))
awsMutexKV.Lock(mutexKey)
defer awsMutexKV.Unlock(mutexKey)Adjust the mutex key and usage as necessary. 😄 We could also have the AWS Go SDK automatically retry on these errors, see also: |
|
Hi thanks a lot for this development, is there any plan to release this with next versions? |
|
Hello, it's not clear for me from @bflad if the solution is ok or if I should change to a provider-wide mutex :( |
|
If we know multiple resources are going to contend with each other and require retries on a concurrency error, implementing the mutex is likely the best option since we will not know how long to reasonably wait/retry. |
|
@bflad I think we still need the retry in place, just in case we have a scenario where the resolvers are being modified by another process for example someone using the AWS console directly. |
|
It certainly won't hurt to keep the retry logic in there too. |
|
This is great! There is anyway I can help to get this merged? It seems to be in the back burner at the moment |
|
There's a definite need for this - I have an application with three schemas and dozens of resolvers that need to be hand stitched after an environment build by Terraform. It's the biggest missing piece for my circumstances. |
|
I also very need this functionality. Please merge it ASAP. |
|
Hello @mathoshek. I wonder if you had a chance to look at this? |
|
Hi @mathoshek - Any updates on when you think this could be ready to be merged? |
|
Hello everyone! I think this Pull Request is ready. I kindly ask admins (@bflad) to have another check and if everything is ok, to merge it! Thank you all! |
|
Nice work @mathoshek - Hopefully @bflad will expedite this change!! |
|
I'm not actively reviewing these at the moment, but FYI there are currently two pull requests for the |
|
@bflad I admit #4840 also contains the |
|
This is ready for rebase as I just merged #4840. 👍 Please do feel free to include some of your |
bflad
added
the
waiting-response
|
@bflad when this expected to be released? |
ghost
removed
the
waiting-response
|
@bflad This is ready for another check! If anything else needs to be done, please let me know. |
There was a problem hiding this comment.
Overall this looked great, @mathoshek! 🚀 Thanks for your contributions here. I left some minor notes below which I addressed in a followup commit (2cb743b) so we can get this released today in version 2.4.0 of the Terraform AWS Provider.
Output from acceptance testing:
--- PASS: TestAccAWSAppsyncGraphqlApi_basic (10.60s)
--- PASS: TestAccAWSAppsyncGraphqlApi_AuthenticationType_AWSIAM (10.74s)
--- PASS: TestAccAWSAppsyncGraphqlApi_AuthenticationType_OpenIDConnect (11.77s)
--- PASS: TestAccAWSAppsyncGraphqlApi_AuthenticationType_APIKey (11.92s)
--- PASS: TestAccAWSAppsyncGraphqlApi_LogConfig (12.05s)
--- PASS: TestAccAWSAppsyncGraphqlApi_AuthenticationType_AmazonCognitoUserPools (13.99s)
--- PASS: TestAccAWSAppsyncGraphqlApi_Name (15.09s)
--- PASS: TestAccAWSAppsyncGraphqlApi_OpenIDConnectConfig_IatTTL (15.71s)
--- PASS: TestAccAWSAppsyncGraphqlApi_AuthenticationType (15.73s)
--- PASS: TestAccAWSAppsyncGraphqlApi_OpenIDConnectConfig_Issuer (15.93s)
--- PASS: TestAccAWSAppsyncGraphqlApi_OpenIDConnectConfig_ClientID (16.14s)
--- PASS: TestAccAWSAppsyncGraphqlApi_OpenIDConnectConfig_AuthTTL (16.48s)
--- PASS: TestAccAWSAppsyncGraphqlApi_disappears (16.59s)
--- PASS: TestAccAWSAppsyncGraphqlApi_UserPoolConfig_AwsRegion (18.45s)
--- PASS: TestAccAWSAppsyncGraphqlApi_UserPoolConfig_DefaultAction (19.66s)
--- PASS: TestAccAWSAppsyncGraphqlApi_LogConfig_FieldLogLevel (22.89s)
--- PASS: TestAccAWSAppsyncGraphqlApi_Schema (33.03s)
--- PASS: TestAccAwsAppsyncResolver_disappears (14.96s)
--- PASS: TestAccAwsAppsyncResolver_DataSource (20.58s)
--- PASS: TestAccAwsAppsyncResolver_ResponseTemplate (22.09s)
--- PASS: TestAccAwsAppsyncResolver_basic (23.40s)
--- PASS: TestAccAwsAppsyncResolver_RequestTemplate (30.72s)
--- PASS: TestAccAwsAppsyncResolver_multipleResolvers (48.68s)
| @@ -167,7 +166,7 @@ func resourceAwsAppsyncGraphqlApiCreate(d *schema.ResourceData, meta interface{} | |||
|
|
|||
There was a problem hiding this comment.
Not related to this pull request -- when running the acceptance testing for aws_appsync_graphql_api, was hitting this pretty often with random tests and our default 20 concurrency:
--- FAIL: TestAccAWSAppsyncGraphqlApi_Schema (11.25s)
testing.go:538: Step 0 error: Error applying: 1 error occurred:
* aws_appsync_graphql_api.test: 1 error occurred:
* aws_appsync_graphql_api.test: ConcurrentModificationException: Can't create new GraphQL API, a GraphQL API creation is already in progress.
--- FAIL: TestAccAWSAppsyncGraphqlApi_OpenIDConnectConfig_ClientID (14.78s)
testing.go:538: Step 0 error: Error applying: 1 error occurred:
* aws_appsync_graphql_api.test: 1 error occurred:
* aws_appsync_graphql_api.test: ConcurrentModificationException: Can't create new GraphQL API, a GraphQL API creation is already in progress.
Judging by the lack of error context before ConcurrentModificationException here and from the debug logs this was occurring during the CreateGraphqlApi call. In a followup commit to ensure a great user experience, I went ahead and added the error context to that call so we can know where this similar looking error is happening (return fmt.Errorf("error creating AppSync GraphQL API: %d", err)) and updated the AWS Go SDK AppSync client to automatically enable retries for that call in aws/config.go:
client.appsyncconn.Handlers.Retry.PushBack(func(r *request.Request) {
if r.Operation.Name == "CreateGraphqlApi" {
if isAWSErr(r.Error, appsync.ErrCodeConcurrentModificationException, "a GraphQL API creation is already in progress") {
r.Retryable = aws.Bool(true)
}
}
})I mention this here because we may be able to remove the wrapping retryOnAwsCode calls below in the aws_graphql_resolver resource by adding other API calls with their appropriate error messaging to the above logic. If you are interested in simplifying the code after this pull request, we'd happily accept that. 👍
| }) | ||
|
|
||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
We should return error context here for operators and code maintainers:
| return err | |
| return fmt.Errorf("error creating AppSync Resolver: %s", err) |
| } | ||
|
|
||
| resp, err := conn.GetResolver(input) | ||
| if err != nil { |
There was a problem hiding this comment.
Prior to returning an error when an AppSync Resolver cannot be found (which can happen when the GraphQL API or Resolver is deleted outside Terraform), we should instead attempt to catch the error and signal to Terraform that this resource needs to be recreated instead.
| if err != nil { | |
| if isAWSErr(err, appsync.ErrCodeNotFoundException, "") { | |
| log.Printf("[WARN] AppSync Resolver (%s) not found, removing from state", d.Id()) | |
| d.SetId("") | |
| return nil | |
| } | |
| if err != nil { |
By returning the appsync.GraphqlApi and appsync.Resolver API objects from the Exists testing functions for both resources, this can be tested with the following:
func TestAccAwsAppsyncResolver_disappears(t *testing.T) {
var api1 appsync.GraphqlApi
var resolver1 appsync.Resolver
rName := fmt.Sprintf("tfacctest%d", acctest.RandInt())
appsyncGraphqlApiResourceName := "aws_appsync_graphql_api.test"
resourceName := "aws_appsync_resolver.test"
resource.ParallelTest(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckAwsAppsyncResolverDestroy,
Steps: []resource.TestStep{
{
Config: testAccAppsyncResolver_basic(rName),
Check: resource.ComposeTestCheckFunc(
testAccCheckAwsAppsyncGraphqlApiExists(appsyncGraphqlApiResourceName, &api1),
testAccCheckAwsAppsyncResolverExists(resourceName, &resolver1),
testAccCheckAwsAppsyncResolverDisappears(&api1, &resolver1),
),
ExpectNonEmptyPlan: true,
},
},
})
}
func testAccCheckAwsAppsyncResolverDisappears(api *appsync.GraphqlApi, resolver *appsync.Resolver) resource.TestCheckFunc {
return func(s *terraform.State) error {
conn := testAccProvider.Meta().(*AWSClient).appsyncconn
input := &appsync.DeleteResolverInput{
ApiId: api.ApiId,
FieldName: resolver.FieldName,
TypeName: resolver.TypeName,
}
_, err := conn.DeleteResolver(input)
return err
}
}Prior to resource code update:
--- FAIL: TestAccAwsAppsyncResolver_disappears (12.60s)
testing.go:538: Step 0 error: Error on follow-up refresh: 1 error occurred:
* aws_appsync_resolver.test: 1 error occurred:
* aws_appsync_resolver.test: aws_appsync_resolver.test: NotFoundException: No resolver found.
After code update:
--- PASS: TestAccAwsAppsyncResolver_disappears (17.10s)
|
|
||
| resp, err := conn.GetResolver(input) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
We should return error context here for operators and code maintainers:
| return err | |
| return fmt.Errorf("error getting AppSync Resolver (%s): %s", d.Id(), err) |
| }) | ||
|
|
||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
We should return error context here for operators and code maintainers:
| return err | |
| return fmt.Errorf("error updating AppSync Resolver (%s): %s", d.Id(), err) |
| }) | ||
|
|
||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
We should return error context here for operators and code maintainers:
| return err | |
| return fmt.Errorf("error deleting AppSync Resolver (%s): %s", d.Id(), err) |
| _, err = conn.GetResolver(input) | ||
| if err != nil { | ||
| if isAWSErr(err, appsync.ErrCodeNotFoundException, "") { | ||
| return nil |
There was a problem hiding this comment.
For test configurations that include multiple aws_appsync_resolver resources, this should continue the for loop instead of exiting immediately to ensure that all of the resources have been properly deleted.
| return nil | |
| continue |
… removed outside Terraform, allow automatic retries for AppSync CreateGraphqlApi calls Reference: #6451 Output from acceptance testing: ``` --- PASS: TestAccAWSAppsyncGraphqlApi_basic (10.60s) --- PASS: TestAccAWSAppsyncGraphqlApi_AuthenticationType_AWSIAM (10.74s) --- PASS: TestAccAWSAppsyncGraphqlApi_AuthenticationType_OpenIDConnect (11.77s) --- PASS: TestAccAWSAppsyncGraphqlApi_AuthenticationType_APIKey (11.92s) --- PASS: TestAccAWSAppsyncGraphqlApi_LogConfig (12.05s) --- PASS: TestAccAWSAppsyncGraphqlApi_AuthenticationType_AmazonCognitoUserPools (13.99s) --- PASS: TestAccAWSAppsyncGraphqlApi_Name (15.09s) --- PASS: TestAccAWSAppsyncGraphqlApi_OpenIDConnectConfig_IatTTL (15.71s) --- PASS: TestAccAWSAppsyncGraphqlApi_AuthenticationType (15.73s) --- PASS: TestAccAWSAppsyncGraphqlApi_OpenIDConnectConfig_Issuer (15.93s) --- PASS: TestAccAWSAppsyncGraphqlApi_OpenIDConnectConfig_ClientID (16.14s) --- PASS: TestAccAWSAppsyncGraphqlApi_OpenIDConnectConfig_AuthTTL (16.48s) --- PASS: TestAccAWSAppsyncGraphqlApi_disappears (16.59s) --- PASS: TestAccAWSAppsyncGraphqlApi_UserPoolConfig_AwsRegion (18.45s) --- PASS: TestAccAWSAppsyncGraphqlApi_UserPoolConfig_DefaultAction (19.66s) --- PASS: TestAccAWSAppsyncGraphqlApi_LogConfig_FieldLogLevel (22.89s) --- PASS: TestAccAWSAppsyncGraphqlApi_Schema (33.03s) --- PASS: TestAccAwsAppsyncResolver_disappears (14.96s) --- PASS: TestAccAwsAppsyncResolver_DataSource (20.58s) --- PASS: TestAccAwsAppsyncResolver_ResponseTemplate (22.09s) --- PASS: TestAccAwsAppsyncResolver_basic (23.40s) --- PASS: TestAccAwsAppsyncResolver_RequestTemplate (30.72s) --- PASS: TestAccAwsAppsyncResolver_multipleResolvers (48.68s) ```
|
This has been released in version 2.4.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. |
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks! |
ghost
locked and limited conversation to collaborators
Fixes #2705
Changes proposed in this pull request:
Output from acceptance testing: