Add regional endpoint support for aws_api_gateway_rest_api and aws_api_gateway_domain_name #2866

handcraftedbits · 2018-01-04T19:56:48Z

Fixed: #2195.

Note that there's no real way to write acceptance tests for regional domain names in aws_api_gateway_domain_name -- an ACM-managed certificate is required. Seems like that bit isn't tested for edge-optimized domain names either.

…_domain_name.

jonathan-kosgei · 2018-02-13T14:47:53Z

Hey, when will this be available?

harsh-innovify · 2018-02-16T12:09:55Z

Eagerly waiting for this

jonathan-kosgei · 2018-03-02T17:44:03Z

☹️

handcraftedbits · 2018-03-02T17:45:38Z

Yeah I'm not sure what to do to get more attention to this PR. Maybe thumbs up for #2195?

jonathan-kosgei · 2018-03-04T10:38:39Z

@handcraftedbits done. I've run out of comments I can leave emojis on.

I'm thinking of just forking this and building it with this pr.

@apparentlymart

bflad

Sorry this took so long to get an initial review 😓 and thanks for your patience. I have some questions about the implementation below before we continue. Please let me know if you have any questions or do not have time to continue working on this.

bflad · 2018-04-05T19:00:17Z

aws/resource_aws_api_gateway_rest_api.go

+			"endpoint_type": {
+				Type:         schema.TypeString,
+				Optional:     true,
+				Default:      "EDGE",


Minor nitpick: We can use the SDK provided constant here: apigateway.EndpointTypeEdge

Missed that, thanks.

bflad · 2018-04-05T19:02:42Z

aws/resource_aws_api_gateway_rest_api.go

+				Type:         schema.TypeString,
+				Optional:     true,
+				Default:      "EDGE",
+				ValidateFunc: validateAwsApiGatewayEndpointType,


Nitpick: This can be simplified with:

ValidateFunc: validation.StringInSlice([]string{ apigateway.EndpointTypeEdge, apigateway.EndpointTypeRegional, }, false),

Fixed, thanks.

bflad · 2018-04-05T19:13:20Z

aws/resource_aws_api_gateway_rest_api.go

@@ -51,6 +51,13 @@ func resourceAwsApiGatewayRestApi() *schema.Resource {
 				Type:     schema.TypeString,
 				Computed: true,
 			},
+
+			"endpoint_type": {


Nothing is currently calling d.Set("endpoint_type", ...) in the read function so Terraform state is not being updated from the API to detect drift.

That said, there are two things about the API and SDK implementation that are jumping out at me here:

It seems this should be nested under an endpoint_configuration block and renamed just type (or types see below) unless there is a good reason to drift from the API. We have gotten bit in the past where we didn't follow the API implementation and future maintenance was harder than it needed to be. I know this is a fairly trivial example, but it also makes it clearer what this is if you have used other AWS SDKs/CloudFormation.

The SDK signature is Types []*string locationName:"types" type:"list" so it seems like the API supports being both EDGE and REGIONAL. Is that the case and is there reason to not support it?

Nothing is currently calling d.Set("endpoint_type", ...) in the read function so Terraform state is not being updated from the API to detect drift.

I can fix that pending discussion...

It seems this should be nested under an endpoint_configuration block and renamed just type (or types see below) unless there is a good reason to drift from the API.

Yeah, I wasn't sure how to approach it considering the situation (see below). Plus there's also an impact on the domain name resource. Right now I think that resource deviates fairly heavily and I kinda continued the trend by having the EDGE/REGIONAL selection be predicated on whether you provide a regional or "normal" certificate. I was trying to avoid having multiple points of failure (i.e., you set type to REGIONAL but provide an edge certificate ARN). So I get the concern, just not sure of the best way to approach it for both of these resources. I'm happy to implement whatever you decide.

The SDK signature is Types []*string locationName:"types" type:"list" so it seems like the API supports being both EDGE and REGIONAL. Is that the case and is there reason to not support it?

I thought the same thing when I was working on this, that it was rather odd that the value is a list when all indications in the UI point to this being an either/or proposition. Indeed, when I set a domain up manually using the CLI in order to force both EDGE and REGIONAL I was greeted with this in my console:

An Edge Optimized Endpoint and a Regional Endpoint are both configured. Please migrate your DNS to either the Regional Domain Name or the Edge Optimized Domain Name and delete the unused endpoint.

So it can be done but I don't think you're supposed to do it. Maybe they have future plans for it though.

Thanks so much for the investigative work. I'll write up a response in the morning.

Sorry for the delayed response. I think in this case we should follow the API types and behaviors. It simplifies maintenance of this resource in Terraform and will present more accurate information about the physical state of the resource if multiple endpoints are currently assigned. At worst we can allow the API errors for incorrect configurations to bubble up and create any plan-time validations tuned for those scenarios.

I also believe that allowing the list configuration of types will allow you migrate from one type to another without downtime while you move DNS records, etc.

Hi All, I started working on this also, then realized that this PR existed. I also thought it was a bit odd that types was a list, however when submitting an update endpoint_configuration request with a blank value, I got this back.

* aws_api_gateway_rest_api.test: BadRequestException: Invalid EndpointTypes specified. Valid options are REGIONAL,EDGE,PRIVATE

It looks like supporting multiple types might be more relevant with the PRIVATE type?
I couldn't find it in the docs anywhere but that might give it more sense.

bflad · 2018-04-05T19:17:11Z

aws/resource_aws_api_gateway_domain_name.go

+	if endpointType != "" {
+		operations = append(operations, &apigateway.PatchOperation{
+			Op:    aws.String("replace"),
+			Path:  aws.String("/endpointConfiguration/types/0"),


Same question here about supporting multiple endpoint types.

bflad · 2018-04-24T14:53:47Z

@handcraftedbits do you have time to continue working on this? No problem if not, just please let us know. Thanks.

handcraftedbits · 2018-04-24T14:55:45Z

@bflad, I unfortunately have been too busy to look back into this. Plus I'm shaky on how to do semi-complicated validation that will be required based on your comments.

bflad · 2018-04-24T16:25:59Z

@handcraftedbits no worries! 😃 Your work here is very much appreciated! I'll try to tackle the more complicated bits, keeping your commits.

tecnobrat · 2018-05-17T17:47:44Z

Hey @bflad sorry to pester but I noticed this keeps getting pushed back. Is there any word on where this sits as far as priority? We could really use this.

bflad · 2018-05-17T20:50:53Z

@tecnobrat no worries and sorry for the delay. Its very high on my priority list. I'm hoping to have it done and merged tomorrow.

tabacco · 2018-05-18T00:18:06Z

That's super exciting, thank you!

danieladams456 · 2018-05-21T19:23:23Z

Thanks for your work @handcraftedbits and @bflad!

bflad · 2018-05-21T19:24:51Z

Hi folks -- I have implemented two followup commits to further align this with the AWS API Gateway API (creating an endpoint_configuration nested argument with types) and bolster the acceptance testing, especially with aws_api_gateway_domain_name resource:

This PR and those commits are now in master and will release with v1.20.0 of the AWS provider later this week. 👍

FYI the configuration looks like this (endpoint_configuration schema is similar for both resources):

resource "aws_api_gateway_rest_api" "example" {
  name        = "regional-example"

  endpoint_configuration {
    types = ["REGIONAL"]
  }
}

bflad · 2018-05-23T21:30:32Z

This has been released in version 1.20.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-05T17:54:29Z

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!

Add regional support for aws_api_gateway_rest_api and aws_api_gateway…

ddfc353

…_domain_name.

Ninir added the enhancement Requests to existing resources that expand the functionality or scope. label Jan 5, 2018

bflad added the service/apigateway Issues and PRs that pertain to the apigateway service. label Jan 11, 2018

bflad reviewed Apr 5, 2018

View reviewed changes

bflad self-assigned this Apr 5, 2018

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

Merge remote-tracking branch 'upstream/master'

2ddacfe

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

Minor regional/edge API gateway implementation tweaks.

4e95621

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

bflad added this to the v1.17.0 milestone Apr 24, 2018

bflad modified the milestones: v1.17.0, v1.18.0 May 2, 2018

bflad modified the milestones: v1.18.0, v1.19.0 May 11, 2018

bflad modified the milestones: v1.19.0, v1.20.0 May 17, 2018

danieladams456 mentioned this pull request May 18, 2018

Feature: AWS Regional API Endpoints #2195

Closed

bflad merged commit 4e95621 into hashicorp:master May 21, 2018

bflad mentioned this pull request May 21, 2018

r/api_gateway_domain_name: Add support for Regional API Endpoints #2290

Closed

bflad added a commit that referenced this pull request May 21, 2018

Update CHANGELOG for #2866

b288cfd

ghost locked and limited conversation to collaborators Apr 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add regional endpoint support for aws_api_gateway_rest_api and aws_api_gateway_domain_name #2866

Add regional endpoint support for aws_api_gateway_rest_api and aws_api_gateway_domain_name #2866

handcraftedbits commented Jan 4, 2018

jonathan-kosgei commented Feb 13, 2018

harsh-innovify commented Feb 16, 2018

jonathan-kosgei commented Mar 2, 2018

handcraftedbits commented Mar 2, 2018

jonathan-kosgei commented Mar 4, 2018 •

edited

Loading

bflad left a comment

bflad Apr 5, 2018

handcraftedbits Apr 6, 2018

bflad Apr 5, 2018

handcraftedbits Apr 6, 2018

bflad Apr 5, 2018 •

edited

Loading

handcraftedbits Apr 6, 2018

bflad Apr 6, 2018

bflad Apr 13, 2018

chrisjharding Apr 18, 2018 •

edited

Loading

bflad Apr 5, 2018

bflad commented Apr 24, 2018

handcraftedbits commented Apr 24, 2018

bflad commented Apr 24, 2018

tecnobrat commented May 17, 2018

bflad commented May 17, 2018

tabacco commented May 18, 2018

danieladams456 commented May 21, 2018

bflad commented May 21, 2018

bflad commented May 23, 2018

ghost commented Apr 5, 2020

Add regional endpoint support for aws_api_gateway_rest_api and aws_api_gateway_domain_name #2866

Add regional endpoint support for aws_api_gateway_rest_api and aws_api_gateway_domain_name #2866

Conversation

handcraftedbits commented Jan 4, 2018

jonathan-kosgei commented Feb 13, 2018

harsh-innovify commented Feb 16, 2018

jonathan-kosgei commented Mar 2, 2018

handcraftedbits commented Mar 2, 2018

jonathan-kosgei commented Mar 4, 2018 • edited Loading

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

Choose a reason for hiding this comment

bflad Apr 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

chrisjharding Apr 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bflad commented Apr 24, 2018

handcraftedbits commented Apr 24, 2018

bflad commented Apr 24, 2018

tecnobrat commented May 17, 2018

bflad commented May 17, 2018

tabacco commented May 18, 2018

danieladams456 commented May 21, 2018

bflad commented May 21, 2018

bflad commented May 23, 2018

ghost commented Apr 5, 2020

jonathan-kosgei commented Mar 4, 2018 •

edited

Loading

bflad Apr 5, 2018 •

edited

Loading

chrisjharding Apr 18, 2018 •

edited

Loading