resource/api_gateway_authorizer: support authorizer type as COGNITO_USER_POOLS #3156

loivis · 2018-01-27T01:50:30Z

Fixes #1106
Duplicate of #2189

I missed existing issue and pr when I started. Anyway I continued with this new pr.

QUSTION: type changed to ForceNew

Because COGNITO_USER_POOLS is different with TOKEN/REQUEST. But when changing between TOKEN and REQUEST, it doesn't required new resource. To conditionally ForceNew, the only way I can think of is to have some logic in update function that will detect type change and call create function. I was wondering if terraform has already supported conditional ForceNew someway or we just go with always ForceNew.

loivis · 2018-01-27T01:51:25Z

⎇  make testacc TEST=./aws TESTARGS='-run=TestAccAWSAPIGatewayAuthorizer_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSAPIGatewayAuthorizer_ -timeout 120m
=== RUN   TestAccAWSAPIGatewayAuthorizer_basic
--- PASS: TestAccAWSAPIGatewayAuthorizer_basic (50.11s)
=== RUN   TestAccAWSAPIGatewayAuthorizer_cognito
--- PASS: TestAccAWSAPIGatewayAuthorizer_cognito (34.12s)
=== RUN   TestAccAWSAPIGatewayAuthorizer_switchAuthType
--- PASS: TestAccAWSAPIGatewayAuthorizer_switchAuthType (83.78s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	168.058s

loivis · 2018-01-27T01:59:07Z

Also introduced randString in acceptance test, which makes code change more than needed for this pr. I think it's ok to include it here instead of opening another pr. No worth for that.

loivis · 2018-02-01T07:38:42Z

@bflad Do you have time to take a look? Thanks.

bflad · 2018-02-01T11:20:52Z

Not at the current time. Something that is an immediate red flag is this though:

I missed existing issue and pr when I started. Anyway I continued with this new pr.

We really do not like ignoring someone's contributions and the original author is asking for help in their issue. Can you help them instead of ignoring their contribution?

loivis · 2018-02-01T11:37:46Z

Ok, understand. I’ll try to see how to commit changes to the other pr and merge my codes there if possible. Anyway I’m closing this pr. Thanks for your comments and explanation. mvh

…

在 2018年2月1日，12:20，Brian Flad ***@***.***> 写道： Not at the current time. Something that is an immediate red flag is this though: I missed existing issue and pr when I started. Anyway I continued with this new pr. We really do not like ignoring someone's contributions and the original author is asking for help in their issue. Can you help them instead of ignoring their contribution? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

loivis · 2018-02-01T15:15:11Z

Back to this PR again with close of #2189.

bflad · 2018-02-01T15:40:22Z

Thanks for the followup @loivis! Sorry if I came off a little mean earlier, that was not my intention.

loivis · 2018-02-01T16:00:41Z

No need sorry, not at all. It's good to know how we're managing issue/prs and follow the same.

loivis · 2018-03-02T20:41:40Z

@bflad do you have time to take a look? Thanks.

bflad

Hi @loivis! This is looking pretty good. I left you some initial feedback. Please ping me again when its ready to be looked at again or if you do not have time to implement 👍

Testing is currently passing for me:

=== RUN   TestAccAWSAPIGatewayAuthorizer_cognito
--- PASS: TestAccAWSAPIGatewayAuthorizer_cognito (14.86s)
=== RUN   TestAccAWSAPIGatewayAuthorizer_switchAuthType
--- PASS: TestAccAWSAPIGatewayAuthorizer_switchAuthType (48.30s)
=== RUN   TestAccAWSAPIGatewayAuthorizer_basic
--- PASS: TestAccAWSAPIGatewayAuthorizer_basic (81.04s)

bflad · 2018-03-03T03:24:21Z

aws/resource_aws_api_gateway_authorizer.go

@@ -55,6 +56,11 @@ func resourceAwsApiGatewayAuthorizer() *schema.Resource {
 				Type:     schema.TypeString,
 				Optional: true,
 			},
+			"provider_arns": &schema.Schema{


Minor nitpick: the &schema.Schema here (copy paste 😄 ) is extra

bflad · 2018-03-03T03:25:25Z

aws/resource_aws_api_gateway_authorizer.go

@@ -55,6 +56,11 @@ func resourceAwsApiGatewayAuthorizer() *schema.Resource {
 				Type:     schema.TypeString,
 				Optional: true,
 			},
+			"provider_arns": &schema.Schema{
+				Type:     schema.TypeList,


Does the ordering of the values matter for the provider_arns attribute? If not, we should probably switch this to schema.TypeSet 👍

bflad · 2018-03-03T03:30:43Z

aws/resource_aws_api_gateway_authorizer.go

@@ -21,7 +21,7 @@ func resourceAwsApiGatewayAuthorizer() *schema.Resource {
 		Schema: map[string]*schema.Schema{
 			"authorizer_uri": &schema.Schema{
 				Type:     schema.TypeString,
-				Required: true,
+				Optional: true,


Sorry I'm unfamiliar with this resource, but is this required for TOKEN/REQUEST? Just curious if that was an oversight before, if we should add documentation, or CustomizeDiff for continued plan-time validation

@bflad Indeed CustomizeDiff should be used here. But it doesn't seem to get the argument. Did I miss something?
https://github.com/loivis/terraform-provider-aws/blob/9726a5187a1fb34a8edbc660e0d3968d8e46a3f0/aws/resource_aws_api_gateway_authorizer.go#L277-L280

I think you need to perform two checks, one for existence and one for string length:

v, ok := diff.GetOk("authorizer_uri") if ok { if v.(string) == "" { return errors.New("authorizer_uri must be non-empty when type is ...") } } else { return errors.New("authorizer_uri is required when type is ...") }

You'll need to do the same with len() == 0 for provider_arns

I added a few lines to debug

args := []string{"authorizer_uri", "name", "rest_api_id", "identity_source", "type", "identity_validation_expression", "authorizer_credentials"} for _, arg := range args { val, ok := diff.GetOk(arg) log.Printf("[DEBUG] %s: #%s#, #%v#", arg, val.(string), ok) }

It gives me logs:

2018/03/05 21:50:54 [DEBUG] authorizer_uri: ##, #false# 2018/03/05 21:50:54 [DEBUG] name: #tf-acctest-igw-authorizer-yddwa40#, #true# 2018/03/05 21:50:54 [DEBUG] rest_api_id: ##, #false# 2018/03/05 21:50:54 [DEBUG] identity_source: #method.request.header.Authorization#, #true# 2018/03/05 21:50:54 [DEBUG] type: #TOKEN#, #true# 2018/03/05 21:50:54 [DEBUG] identity_validation_expression: ##, #false# 2018/03/05 21:50:54 [DEBUG] authorizer_credentials: ##, #false#

name, identity_source, type are arguments with default value. It seems like diff.GetOk() doesn't return value for arguments without default value. This is what makes me confused.

Ah, this is happening because the values are computed:

https://github.com/hashicorp/terraform/blob/ea50f455edca3acaa0a6ab372b861e6cd2d386aa/helper/schema/resource_diff.go#L339

Unfortunately, I'm not sure of a workaround at the moment. For now, we can keep that logic outside CustomizeDiff. 🙁

bflad · 2018-03-03T03:32:42Z

aws/resource_aws_api_gateway_authorizer.go

@@ -210,3 +228,38 @@ func resourceAwsApiGatewayAuthorizerDelete(d *schema.ResourceData, meta interfac

 	return nil
 }
+
+func diffProviderARNsOp(prefix string, old, new []interface{}) (ops []*apigateway.PatchOperation) {
+	// providerARNs can't be empty, so add first and then remove


Thank you for this comment 👍

bflad · 2018-03-03T03:33:55Z

aws/resource_aws_api_gateway_authorizer_test.go

 	"github.com/hashicorp/terraform/helper/resource"
 	"github.com/hashicorp/terraform/terraform"
 )

 func TestAccAWSAPIGatewayAuthorizer_basic(t *testing.T) {
 	var conf apigateway.Authorizer
+	rString := acctest.RandString(7)


bflad · 2018-03-03T03:40:33Z

website/docs/r/api_gateway_authorizer.html.markdown

@@ -113,3 +113,4 @@ The following arguments are supported:
 	For `TOKEN` type, this value should be a regular expression. The incoming token from the client is matched
 	against this expression, and will proceed if the token matches. If the token doesn't match,
 	the client receives a 401 Unauthorized response.
+* `provider_arns` - (Optional, required for type `COGNITO_USER_POOLS`) A list of the Amazon Cognito user pool ARNs. Each element is of this format: `arn:aws:cognito-idp:{region}:{account_id}:userpool/{user_pool_id}`.


Would you mind taking a look at adding a CustomizeDiff function that adds plan-time validation for this situation? Maybe something similar to aws_dynamodb_table function can work here too:

https://github.com/terraform-providers/terraform-provider-aws/blob/0cd8e5a0e6572740511f54c44c483a0549ecee2e/aws/validators.go#L2236

bflad · 2018-03-03T03:41:00Z

website/docs/r/api_gateway_authorizer.html.markdown

 	For `TOKEN` type, this must be a well-formed Lambda function URI in the form of
 	`arn:aws:apigateway:{region}:lambda:path/{service_api}`. e.g. `arn:aws:apigateway:us-west-2:lambda:path/2015-03-31/functions/arn:aws:lambda:us-west-2:012345678912:function:my-function/invocations`
 * `name` - (Required) The name of the authorizer
 * `rest_api_id` - (Required) The ID of the associated REST API
 * `identity_source` - (Optional) The source of the identity in an incoming request.
 	Defaults to `method.request.header.Authorization`. For `REQUEST` type, this may be a comma-separated list of values, including headers, query string parameters and stage variables - e.g. `"method.request.header.SomeHeaderName,method.request.querystring.SomeQueryStringName,stageVariables.SomeStageVariableName"`
-* `type` - (Optional) The type of the authorizer. Possible values are `TOKEN` and `REQUEST`.
+* `type` - (Optional) The type of the authorizer. Possible values are `TOKEN` for a Lambda function using a single authorization token submitted in a custom header, `REQUEST` for a Lambda function using incoming request parameters, or `COGNITO_USER_POOLS` for using an Amazon Cognito user pool.


bflad · 2018-03-03T03:41:55Z

aws/resource_aws_api_gateway_authorizer_test.go

-resource "aws_api_gateway_authorizer" "test" {
-  name = "tf-acc-test-authorizer"
-  rest_api_id = "${aws_api_gateway_rest_api.test.id}"
+func testAccAWSAPIGatewayAuthorizerConfig_lambda(apiGatewayName, authorizerName, lambdaName string) string {


😍 thank you

bflad · 2018-03-03T03:49:31Z

aws/resource_aws_api_gateway_authorizer.go

@@ -41,6 +41,7 @@ func resourceAwsApiGatewayAuthorizer() *schema.Resource {
 				Type:     schema.TypeString,
 				Optional: true,
 				Default:  "TOKEN",
+				ForceNew: true,


Can you add plan-time validation here please?

ValidateFunc: validation.StringInSlice([]string{ apigateway.AuthorizerTypeCognitoUserPools, apigateway.AuthorizerTypeRequest, apigateway.AuthorizerTypeToken }, false),

bflad · 2018-03-03T03:57:25Z

To answer your question above:

I was wondering if terraform has already supported conditional ForceNew someway or we just go with always ForceNew.

You can use a CustomizeDiff function to do that. 😄 You can check diff.HasChange("type") and if it does use o,n := diff.GetChange("type") to selectively call a diff.ForceNew("type") appropriately 🎉

…SER_POOLS

…hema'

bflad · 2018-03-06T11:45:13Z

aws/resource_aws_api_gateway_authorizer.go

@@ -181,6 +199,12 @@ func resourceAwsApiGatewayAuthorizerUpdate(d *schema.ResourceData, meta interfac
 			Value: aws.String(d.Get("identity_validation_expression").(string)),
 		})
 	}
+	if d.HasChange("provider_arns") {


This can be simplified using schema.Set Difference(), here's an example in action:

https://github.com/terraform-providers/terraform-provider-aws/blob/90e2e8df03e4ded02c825169bc64b3dae747b9a0/aws/resource_aws_api_gateway_integration.go#L338-L361

…eDiff; add acceptance tests for type validation

loivis · 2018-03-06T18:49:51Z

⎇  make testacc TEST=./aws TESTARGS='-run=TestAccAWSAPIGatewayAuthorizer_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSAPIGatewayAuthorizer_ -timeout 120m
=== RUN   TestAccAWSAPIGatewayAuthorizer_basic
--- PASS: TestAccAWSAPIGatewayAuthorizer_basic (54.47s)
=== RUN   TestAccAWSAPIGatewayAuthorizer_cognito
--- PASS: TestAccAWSAPIGatewayAuthorizer_cognito (33.99s)
=== RUN   TestAccAWSAPIGatewayAuthorizer_switchAuthType
--- PASS: TestAccAWSAPIGatewayAuthorizer_switchAuthType (89.65s)
=== RUN   TestAccAWSAPIGatewayAuthorizer_authTypeValidation
--- PASS: TestAccAWSAPIGatewayAuthorizer_authTypeValidation (44.02s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	222.195s

bflad

This looks fantastic, thanks for the great work! 🚀

4 tests passed (all tests)
=== RUN   TestAccAWSAPIGatewayAuthorizer_authTypeValidation
--- PASS: TestAccAWSAPIGatewayAuthorizer_authTypeValidation (29.27s)
=== RUN   TestAccAWSAPIGatewayAuthorizer_switchAuthType
--- PASS: TestAccAWSAPIGatewayAuthorizer_switchAuthType (64.32s)
=== RUN   TestAccAWSAPIGatewayAuthorizer_basic
--- PASS: TestAccAWSAPIGatewayAuthorizer_basic (105.69s)
=== RUN   TestAccAWSAPIGatewayAuthorizer_cognito
--- PASS: TestAccAWSAPIGatewayAuthorizer_cognito (220.05s)

bflad · 2018-03-09T19:55:12Z

This has been released in version 1.11.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

ghost · 2020-04-07T17:40:24Z

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 added the size/L Managed by automation to categorize the size of a PR. label Jan 27, 2018

loivis force-pushed the resource-api-gateway-authorizer-support-cognito-user-pool branch from f13113d to 7db27dc Compare January 27, 2018 01:55

ghost added the size/L Managed by automation to categorize the size of a PR. label Jan 27, 2018

bflad added enhancement Requests to existing resources that expand the functionality or scope. service/apigateway Issues and PRs that pertain to the apigateway service. labels Jan 28, 2018

loivis mentioned this pull request Feb 1, 2018

resource/aws_api_gateway_authorizer: Add support for Cognito User Pool authorizer #2189

Closed

loivis closed this Feb 1, 2018

loivis reopened this Feb 1, 2018

bflad reviewed Mar 3, 2018

View reviewed changes

bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Mar 3, 2018

ghost added the size/L Managed by automation to categorize the size of a PR. label Mar 3, 2018

xiaowei.wang added 3 commits March 3, 2018 13:52

resource/api_gateway_authorizer: support authorizer type as COGNITO_U…

8c8c25f

…SER_POOLS

resource/api_gateway_authorizer: remove extra copy/pasted '&schema.Sc…

3cd1b02

…hema'

resource/api_gateway_authorizer: add ValidateFunc for authorizer type

f32ffbd

loivis force-pushed the resource-api-gateway-authorizer-support-cognito-user-pool branch from a2f4a49 to f32ffbd Compare March 3, 2018 12:55

ghost added the size/L Managed by automation to categorize the size of a PR. label Mar 3, 2018

resource/api_gateway_authorizer: add CustomizeDiff

dfea540

loivis force-pushed the resource-api-gateway-authorizer-support-cognito-user-pool branch from 9726a51 to dfea540 Compare March 5, 2018 20:52

ghost added the size/L Managed by automation to categorize the size of a PR. label Mar 5, 2018

bflad reviewed Mar 6, 2018

View reviewed changes

xiaowei.wang added 2 commits March 6, 2018 19:39

resource/api_gateway_authorizer: simplify logic on provider_arns changes

dbf93c6

resource/api_gateway_authorizer: move type validation out of Customiz…

6463c99

…eDiff; add acceptance tests for type validation

bflad approved these changes Mar 7, 2018

View reviewed changes

bflad removed the waiting-response Maintainers are waiting on response from community or contributor. label Mar 7, 2018

bflad added this to the v1.11.0 milestone Mar 7, 2018

bflad merged commit 075c4a2 into hashicorp:master Mar 7, 2018

bflad added a commit that referenced this pull request Mar 7, 2018

Update CHANGELOG for #3156

884ddd1

loivis deleted the resource-api-gateway-authorizer-support-cognito-user-pool branch March 7, 2018 17:40

mattburgess mentioned this pull request Mar 8, 2018

Add Cognito Authorizer support for API Gateway #3612

Closed

ghost locked and limited conversation to collaborators Apr 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resource/api_gateway_authorizer: support authorizer type as COGNITO_USER_POOLS #3156

resource/api_gateway_authorizer: support authorizer type as COGNITO_USER_POOLS #3156

loivis commented Jan 27, 2018

loivis commented Jan 27, 2018

loivis commented Jan 27, 2018

loivis commented Feb 1, 2018

bflad commented Feb 1, 2018

loivis commented Feb 1, 2018 via email

loivis commented Feb 1, 2018

bflad commented Feb 1, 2018

loivis commented Feb 1, 2018

loivis commented Mar 2, 2018

bflad left a comment

bflad Mar 3, 2018

bflad Mar 3, 2018

bflad Mar 3, 2018

loivis Mar 3, 2018 •

edited

Loading

bflad Mar 5, 2018 •

edited

Loading

loivis Mar 5, 2018

bflad Mar 6, 2018

bflad Mar 3, 2018

bflad Mar 3, 2018

bflad Mar 3, 2018

bflad Mar 3, 2018

bflad Mar 3, 2018

bflad Mar 3, 2018

bflad commented Mar 3, 2018

bflad Mar 6, 2018

loivis Mar 6, 2018

loivis commented Mar 6, 2018

bflad left a comment

bflad commented Mar 9, 2018

ghost commented Apr 7, 2020

resource/api_gateway_authorizer: support authorizer type as COGNITO_USER_POOLS #3156

resource/api_gateway_authorizer: support authorizer type as COGNITO_USER_POOLS #3156

Conversation

loivis commented Jan 27, 2018

loivis commented Jan 27, 2018

loivis commented Jan 27, 2018

loivis commented Feb 1, 2018

bflad commented Feb 1, 2018

loivis commented Feb 1, 2018 via email

loivis commented Feb 1, 2018

bflad commented Feb 1, 2018

loivis commented Feb 1, 2018

loivis commented Mar 2, 2018

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loivis Mar 3, 2018 • edited Loading

Choose a reason for hiding this comment

bflad Mar 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bflad commented Mar 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loivis commented Mar 6, 2018

bflad left a comment

Choose a reason for hiding this comment

bflad commented Mar 9, 2018

ghost commented Apr 7, 2020

loivis Mar 3, 2018 •

edited

Loading

bflad Mar 5, 2018 •

edited

Loading