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

Added support for appsync_graphql_api schema dn appsync_resolver #6451

Conversation

@mathoshek
Copy link
Contributor

commented Nov 13, 2018

Fixes #2705

Changes proposed in this pull request:

  • Added schema as a parameter of appsync_graphql_api
  • Added support for appsync_resolver

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAWSAppsyncGraphqlApi'

?       github.com/terraform-providers/terraform-provider-aws   [no test files]
=== RUN   TestAccAWSAppsyncGraphqlApi_basic
=== PAUSE TestAccAWSAppsyncGraphqlApi_basic
=== RUN   TestAccAWSAppsyncGraphqlApi_AuthenticationType
=== PAUSE TestAccAWSAppsyncGraphqlApi_AuthenticationType
=== RUN   TestAccAWSAppsyncGraphqlApi_AuthenticationType_APIKey
=== PAUSE TestAccAWSAppsyncGraphqlApi_AuthenticationType_APIKey
=== RUN   TestAccAWSAppsyncGraphqlApi_AuthenticationType_AWSIAM
=== PAUSE TestAccAWSAppsyncGraphqlApi_AuthenticationType_AWSIAM
=== RUN   TestAccAWSAppsyncGraphqlApi_AuthenticationType_AmazonCognitoUserPools
=== PAUSE TestAccAWSAppsyncGraphqlApi_AuthenticationType_AmazonCognitoUserPools
=== RUN   TestAccAWSAppsyncGraphqlApi_AuthenticationType_OpenIDConnect
=== PAUSE TestAccAWSAppsyncGraphqlApi_AuthenticationType_OpenIDConnect
=== RUN   TestAccAWSAppsyncGraphqlApi_LogConfig
=== PAUSE TestAccAWSAppsyncGraphqlApi_LogConfig
=== RUN   TestAccAWSAppsyncGraphqlApi_LogConfig_FieldLogLevel
=== PAUSE TestAccAWSAppsyncGraphqlApi_LogConfig_FieldLogLevel
=== RUN   TestAccAWSAppsyncGraphqlApi_OpenIDConnectConfig_AuthTTL
=== PAUSE TestAccAWSAppsyncGraphqlApi_OpenIDConnectConfig_AuthTTL
=== RUN   TestAccAWSAppsyncGraphqlApi_OpenIDConnectConfig_ClientID
=== PAUSE TestAccAWSAppsyncGraphqlApi_OpenIDConnectConfig_ClientID
=== RUN   TestAccAWSAppsyncGraphqlApi_OpenIDConnectConfig_IatTTL
=== PAUSE TestAccAWSAppsyncGraphqlApi_OpenIDConnectConfig_IatTTL
=== RUN   TestAccAWSAppsyncGraphqlApi_OpenIDConnectConfig_Issuer
=== PAUSE TestAccAWSAppsyncGraphqlApi_OpenIDConnectConfig_Issuer
=== RUN   TestAccAWSAppsyncGraphqlApi_Name
=== PAUSE TestAccAWSAppsyncGraphqlApi_Name
=== RUN   TestAccAWSAppsyncGraphqlApi_UserPoolConfig_AwsRegion
=== PAUSE TestAccAWSAppsyncGraphqlApi_UserPoolConfig_AwsRegion
=== RUN   TestAccAWSAppsyncGraphqlApi_UserPoolConfig_DefaultAction
=== PAUSE TestAccAWSAppsyncGraphqlApi_UserPoolConfig_DefaultAction
=== RUN   TestAccAWSAppsyncGraphqlApi_Schema
=== PAUSE TestAccAWSAppsyncGraphqlApi_Schema
=== CONT  TestAccAWSAppsyncGraphqlApi_basic
=== CONT  TestAccAWSAppsyncGraphqlApi_OpenIDConnectConfig_ClientID
=== CONT  TestAccAWSAppsyncGraphqlApi_AuthenticationType_APIKey
=== CONT  TestAccAWSAppsyncGraphqlApi_OpenIDConnectConfig_AuthTTL
=== CONT  TestAccAWSAppsyncGraphqlApi_Schema
=== CONT  TestAccAWSAppsyncGraphqlApi_UserPoolConfig_DefaultAction
=== CONT  TestAccAWSAppsyncGraphqlApi_UserPoolConfig_AwsRegion
=== CONT  TestAccAWSAppsyncGraphqlApi_Name
=== CONT  TestAccAWSAppsyncGraphqlApi_OpenIDConnectConfig_Issuer
=== CONT  TestAccAWSAppsyncGraphqlApi_OpenIDConnectConfig_IatTTL
=== CONT  TestAccAWSAppsyncGraphqlApi_AuthenticationType_AmazonCognitoUserPools
=== CONT  TestAccAWSAppsyncGraphqlApi_LogConfig_FieldLogLevel
=== CONT  TestAccAWSAppsyncGraphqlApi_LogConfig
=== CONT  TestAccAWSAppsyncGraphqlApi_AuthenticationType_OpenIDConnect
=== CONT  TestAccAWSAppsyncGraphqlApi_AuthenticationType
=== CONT  TestAccAWSAppsyncGraphqlApi_AuthenticationType_AWSIAM
--- FAIL: TestAccAWSAppsyncGraphqlApi_LogConfig_FieldLogLevel (7.50s)
    testing.go:538: Step 0 error: Error applying: 1 error occurred:
                * aws_iam_role.test: 1 error occurred:
                * aws_iam_role.test: Error creating IAM Role tf-acc-test-2023853567406482821: EntityAlreadyExists: Role with name tf-acc-test-2023853567406482821 already exists.
                status code: 409, request id: e69b4bfb-e77f-11e8-8518-f195001367f9




--- PASS: TestAccAWSAppsyncGraphqlApi_basic (12.95s)
--- PASS: TestAccAWSAppsyncGraphqlApi_AuthenticationType_APIKey (12.95s)
--- PASS: TestAccAWSAppsyncGraphqlApi_AuthenticationType_AWSIAM (14.42s)
--- PASS: TestAccAWSAppsyncGraphqlApi_AuthenticationType_AmazonCognitoUserPools (15.63s)
--- PASS: TestAccAWSAppsyncGraphqlApi_AuthenticationType_OpenIDConnect (16.44s)
--- PASS: TestAccAWSAppsyncGraphqlApi_Name (19.53s)
--- PASS: TestAccAWSAppsyncGraphqlApi_OpenIDConnectConfig_AuthTTL (20.80s)
--- PASS: TestAccAWSAppsyncGraphqlApi_OpenIDConnectConfig_IatTTL (20.88s)
--- PASS: TestAccAWSAppsyncGraphqlApi_OpenIDConnectConfig_ClientID (20.93s)
--- PASS: TestAccAWSAppsyncGraphqlApi_LogConfig (21.34s)
--- PASS: TestAccAWSAppsyncGraphqlApi_AuthenticationType (22.01s)
--- PASS: TestAccAWSAppsyncGraphqlApi_UserPoolConfig_AwsRegion (22.73s)
--- PASS: TestAccAWSAppsyncGraphqlApi_UserPoolConfig_DefaultAction (24.09s)
--- PASS: TestAccAWSAppsyncGraphqlApi_Schema (27.43s)
--- PASS: TestAccAWSAppsyncGraphqlApi_OpenIDConnectConfig_Issuer (35.40s)
FAIL
FAIL    github.com/terraform-providers/terraform-provider-aws/aws       37.141s
$ make testacc TESTARGS='-run=TestAccAwsAppsyncResolver'

?       github.com/terraform-providers/terraform-provider-aws   [no test files]
=== RUN   TestAccAwsAppsyncResolver_basic
=== PAUSE TestAccAwsAppsyncResolver_basic
=== RUN   TestAccAwsAppsyncResolver_DataSource
=== PAUSE TestAccAwsAppsyncResolver_DataSource
=== RUN   TestAccAwsAppsyncResolver_RequestTemplate
=== PAUSE TestAccAwsAppsyncResolver_RequestTemplate
=== RUN   TestAccAwsAppsyncResolver_ResponseTemplate
=== PAUSE TestAccAwsAppsyncResolver_ResponseTemplate
=== RUN   TestAccAwsAppsyncResolver_multipleResolvers
=== PAUSE TestAccAwsAppsyncResolver_multipleResolvers
=== CONT  TestAccAwsAppsyncResolver_basic
=== CONT  TestAccAwsAppsyncResolver_ResponseTemplate
=== CONT  TestAccAwsAppsyncResolver_DataSource
=== CONT  TestAccAwsAppsyncResolver_multipleResolvers
=== CONT  TestAccAwsAppsyncResolver_RequestTemplate
--- PASS: TestAccAwsAppsyncResolver_basic (14.96s)
--- PASS: TestAccAwsAppsyncResolver_DataSource (23.95s)
--- PASS: TestAccAwsAppsyncResolver_multipleResolvers (30.04s)
--- PASS: TestAccAwsAppsyncResolver_ResponseTemplate (38.69s)
--- PASS: TestAccAwsAppsyncResolver_RequestTemplate (45.33s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       47.645s
Albert Chmilevski
@kurtmc

This comment has been minimized.

Copy link
Contributor

commented Nov 13, 2018

@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:

Error: Error applying plan:                                                                                                                                                                                                                                                 
                                                                                                                                                                                                                                                                                           
1 error(s) occurred:                                                                                                                                                                                                                                                                                                                                                                                                                                                           
* aws_appsync_resolver.x: 1 error(s) occurred:                                                                                                                                                                                                
                                                                          
* aws_appsync_resolver.x: ConcurrentModificationException: Schema is currently being altered, please wait until that is complete.
        status code: 409, request id: XXXXX

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

ContinuousTargetOccurence: 3,

to the resource.StateChangeConf in the startSchemaCreationAndWaitForItToBeActive method. I don't know that the best practice is for this, but this seems to work well enough for me.

@kurtmc

This comment has been minimized.

Copy link
Contributor

commented Nov 14, 2018

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?

@mathoshek

This comment has been minimized.

Copy link
Contributor Author

commented Nov 14, 2018

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.

@mathoshek

This comment has been minimized.

Copy link
Contributor Author

commented Nov 14, 2018

@kurtmc I managed to catch the error in a test and reused the retryOnAwsCode function. What do you think?

@kurtmc

This comment has been minimized.

Copy link
Contributor

commented Nov 15, 2018

@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

kurtmc approved these changes Nov 15, 2018

@wips

This comment has been minimized.

Copy link

commented Nov 27, 2018

This retries the actions on the resolver if the schema is being modified. Is there a better approach?

@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?

@kurtmc

This comment has been minimized.

Copy link
Contributor

commented Nov 27, 2018

@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:

resource "aws_appsync_resolver" "a" {
...
}

resource "aws_appsync_resolver" "b" {
...
}

You could potentially make a a dependency of b to force terraform to create these resources in serial but I don't think we should rely on people doing that.

@wips

This comment has been minimized.

Copy link

commented Nov 28, 2018

@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 ConcurrentModificationException exception from AWS and retrying might not be a good option. Despite that eventually, after few retries, resolvers will be created, all those failed requests are just waste of resources.

Is it possible to look at the api_id attribute of a resolver and create resolvers for the same API serially?

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.

@bflad

This comment has been minimized.

Copy link
Member

commented Dec 5, 2018

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:

// Workaround for https://github.com/aws/aws-sdk-go/issues/1376
client.kinesisconn.Handlers.Retry.PushBack(func(r *request.Request) {
if !strings.HasPrefix(r.Operation.Name, "Describe") && !strings.HasPrefix(r.Operation.Name, "List") {
return
}
err, ok := r.Error.(awserr.Error)
if !ok || err == nil {
return
}
if err.Code() == kinesis.ErrCodeLimitExceededException {
r.Retryable = aws.Bool(true)
}
})
// Workaround for https://github.com/aws/aws-sdk-go/issues/1472
client.appautoscalingconn.Handlers.Retry.PushBack(func(r *request.Request) {
if !strings.HasPrefix(r.Operation.Name, "Describe") && !strings.HasPrefix(r.Operation.Name, "List") {
return
}
err, ok := r.Error.(awserr.Error)
if !ok || err == nil {
return
}
if err.Code() == applicationautoscaling.ErrCodeFailedResourceAccessException {
r.Retryable = aws.Bool(true)
}
})
// See https://github.com/aws/aws-sdk-go/pull/1276
client.dynamodbconn.Handlers.Retry.PushBack(func(r *request.Request) {
if r.Operation.Name != "PutItem" && r.Operation.Name != "UpdateItem" && r.Operation.Name != "DeleteItem" {
return
}
if isAWSErr(r.Error, dynamodb.ErrCodeLimitExceededException, "Subscriber limit exceeded:") {
r.Retryable = aws.Bool(true)
}
})
client.kinesisconn.Handlers.Retry.PushBack(func(r *request.Request) {
if r.Operation.Name == "CreateStream" {
if isAWSErr(r.Error, kinesis.ErrCodeLimitExceededException, "simultaneously be in CREATING or DELETING") {
r.Retryable = aws.Bool(true)
}
}
if r.Operation.Name == "CreateStream" || r.Operation.Name == "DeleteStream" {
if isAWSErr(r.Error, kinesis.ErrCodeLimitExceededException, "Rate exceeded for stream") {
r.Retryable = aws.Bool(true)
}
}
})
client.storagegatewayconn.Handlers.Retry.PushBack(func(r *request.Request) {
// InvalidGatewayRequestException: The specified gateway proxy network connection is busy.
if isAWSErr(r.Error, storagegateway.ErrCodeInvalidGatewayRequestException, "The specified gateway proxy network connection is busy") {
r.Retryable = aws.Bool(true)
}
})

@ArieLevs

This comment has been minimized.

Copy link

commented Jan 8, 2019

Hi thanks a lot for this development, is there any plan to release this with next versions?

@mathoshek

This comment has been minimized.

Copy link
Contributor Author

commented Jan 8, 2019

Hello, it's not clear for me from @bflad if the solution is ok or if I should change to a provider-wide mutex :(

@bflad

This comment has been minimized.

Copy link
Member

commented Jan 10, 2019

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.

@kurtmc

This comment has been minimized.

Copy link
Contributor

commented Jan 10, 2019

@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.

@bflad

This comment has been minimized.

Copy link
Member

commented Jan 10, 2019

It certainly won't hurt to keep the retry logic in there too.

@juankaram

This comment has been minimized.

Copy link

commented Jan 31, 2019

This is great! There is anyway I can help to get this merged? It seems to be in the back burner at the moment

@bengiddins

This comment has been minimized.

Copy link

commented Feb 1, 2019

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.

@Dgadavin

This comment has been minimized.

Copy link

commented Feb 19, 2019

I also very need this functionality. Please merge it ASAP.
@kurtmc Could you please give a brief status of this PR? I saw that you approve it but it somehow not merged. Do you plan to merge it or no?
Thanks a lot.
Maybe I or some other guys can help with this feature.

@mathoshek

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2019

Hello and sorry for the long absence. I think @bflad wants a small improvement to this PR that I haven't had time to do. I intend to have it done next week.

@Dgadavin

This comment has been minimized.

Copy link

commented Mar 1, 2019

@mathoshek Thank you very much!

@zentavr

This comment has been minimized.

Copy link

commented Mar 7, 2019

Hello @mathoshek. I wonder if you had a chance to look at this?
Thanks in advance!

@leonfs

This comment has been minimized.

Copy link

commented Mar 13, 2019

Hi @mathoshek - Any updates on when you think this could be ready to be merged?

@mathoshek

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2019

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!

@leonfs

This comment has been minimized.

Copy link

commented Mar 14, 2019

Nice work @mathoshek - Hopefully @bflad will expedite this change!!

@bflad

This comment has been minimized.

Copy link
Member

commented Mar 14, 2019

I'm not actively reviewing these at the moment, but FYI there are currently two pull requests for the aws_appsync_graphql_api resource schema functionality (#4840 and #6451). Generally to speed up resolution in these situations the maintainers would prefer iterating with a previous contributor until their contribution is ready (community members here are more than welcome to submit reviews on the previous work), rebasing newer pull requests on top of their contribution, or getting them to agree that a later implementation is preferred over theirs to close it out. If nothing changes in this regard before this is ready to be reviewed then the maintainers will need to work out something between both contributors at that later time and further delay the process of getting this functionality in.

@mathoshek

This comment has been minimized.

Copy link
Contributor Author

commented Mar 17, 2019

@bflad I admit #4840 also contains the aws_appsync_graphql_api resource schema functionality. Since @beauknowssoftware was the first one, I have nothing agains him having the Pull Request merged first, I will fix merge conflicts and maybe add improvements found in my PR. I can keep this PR with the functionality for AppSync Resolvers. Please let me know the decision the maintainer team takes.

@bflad

This comment has been minimized.

Copy link
Member

commented Mar 26, 2019

This is ready for rebase as I just merged #4840. 👍 Please do feel free to include some of your aws_appsync_graphql_api resource updates including the additional testing and schema update handling. Ping me when this is ready again and I should be able to get it reviewed quickly. Thanks so much.

@zentavr

This comment has been minimized.

Copy link

commented Mar 26, 2019

@bflad when this expected to be released?

@hashibot hashibot bot removed the waiting-response label Mar 26, 2019

mathoshek added some commits Mar 26, 2019

@mathoshek

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2019

@bflad This is ready for another check! If anything else needs to be done, please let me know.

@bflad bflad self-assigned this Mar 28, 2019

@bflad bflad self-requested a review Mar 28, 2019

@bflad

bflad approved these changes Mar 28, 2019

Copy link
Member

left a 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{}

This comment has been minimized.

Copy link
@bflad

bflad Mar 28, 2019

Member

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

This comment has been minimized.

Copy link
@bflad

bflad Mar 28, 2019

Member

We should return error context here for operators and code maintainers:

Suggested change
return err
return fmt.Errorf("error creating AppSync Resolver: %s", err)
}

resp, err := conn.GetResolver(input)
if err != nil {

This comment has been minimized.

Copy link
@bflad

bflad Mar 28, 2019

Member

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.

Suggested change
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

This comment has been minimized.

Copy link
@bflad

bflad Mar 28, 2019

Member

We should return error context here for operators and code maintainers:

Suggested change
return err
return fmt.Errorf("error getting AppSync Resolver (%s): %s", d.Id(), err)
})

if err != nil {
return err

This comment has been minimized.

Copy link
@bflad

bflad Mar 28, 2019

Member

We should return error context here for operators and code maintainers:

Suggested change
return err
return fmt.Errorf("error updating AppSync Resolver (%s): %s", d.Id(), err)
})

if err != nil {
return err

This comment has been minimized.

Copy link
@bflad

bflad Mar 28, 2019

Member

We should return error context here for operators and code maintainers:

Suggested change
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

This comment has been minimized.

Copy link
@bflad

bflad Mar 28, 2019

Member

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.

Suggested change
return nil
continue

@bflad bflad added this to the v2.4.0 milestone Mar 28, 2019

@bflad bflad merged commit 8363ab0 into terraform-providers:master Mar 28, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

bflad added a commit that referenced this pull request Mar 28, 2019

service/appsync: Address PR #6451 feedback, add testing for resources…
… 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)
```

bflad added a commit that referenced this pull request Mar 28, 2019

@bflad

This comment has been minimized.

Copy link
Member

commented Mar 29, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.