feat!: Updates to support websocket API Gateway #54

bryantbiggs · 2021-09-09T16:16:47Z

Description

Additional changes

Minimum supported Terraform AWS provider raised to v5.37.0 to support recent bug fixes in the provider
Default values for api_key_selection_expression, route_selection_expression variables set to null (still matches prior value v4.x version but is set as null now)
The input data structure for routes (was integrations) has been updated and now uses optional inputs

Added

Support for creating a websocket API endpoint
Support for creating Route53 alias records for custom domain names w/ support for multiple sub-domains using a wildcard API Gateway custom domain name
Support for creating ACM certificate for custom domain
Support for automatically deploying the stage when updates have been made (for Websocket, HTTP is always auto-deployed by the API)

Modified

Stage access log group settings are now embedded into the stage_access_log_settings variable
API mapping is created automatically when using a custom domain
Default values of 500 and 1000 have been set for throttling_burst_limit and throttling_rate_limit respectively to ensure users do not face errors when deploying APIs for the first time and not configuring these
Default values for the log group name ("/aws/apigateway/${var.name}/${var.stage_name}") and retention period (30) have been provided for the stage access logs log group

Removed

None

See UPGRADE-5.0.md for full list of changes and notes on upgrade path

Motivation and Context

updates for websocket support

Closes #7
Closes #8
Closes #42
Closes #46
Closes #61

Breaking Changes

Yes

How Has This Been Tested?

I have updated at least one of the examples/* to demonstrate and validate my change(s)
I have tested and validated these changes using one or more of the provided examples/* projects
I have executed pre-commit run -a on my pull request

antonbabenko

Looks pretty good, just minor comments to be consistent in examples.

examples/complete-http/main.tf

main.tf

lllama · 2021-09-09T21:16:29Z

Not sure if this affects #46 at all. That PR had a working example of a websockets API. I can update if needed.

antonbabenko · 2021-09-10T07:37:17Z

@lllama Thanks for the WebSocket example in #46!

I think it will be great if @bryantbiggs merges your example into this PR and update it to follow the rest of the examples we have.

…way-v2 into chore/updates-for-websocket-support

Co-authored-by: Anton Babenko <[email protected]>

antonbabenko

Looks almost perfect. Could you also describe breaking changes and upgrade path in UPGRADE.md as we do in other modules already?

examples/websocket/main.tf

main.tf

…ws-apigateway-v2 into chore/updates-for-websocket-support

…way-v2 into chore/updates-for-websocket-support

…tion

choudharyac09 · 2024-05-13T18:28:28Z

Can We merge this pull request?

bryantbiggs · 2024-05-13T18:47:55Z

Can We merge this pull request?

Its being worked on, its currently in draft mode

choudharyac09 · 2024-05-14T20:52:20Z

@antonbabenko Can you please review the pull request?

antonbabenko · 2024-05-14T20:55:45Z

@antonbabenko Can you please review the pull request?

Yes, as soon as I can. Probably, tomorrow.

antonbabenko

Looks good, a few comments here and there. I didn't run examples though.

UPGRADE-5.0.md

examples/complete-http/main.tf

main.tf

Co-authored-by: Anton Babenko <[email protected]>

antonbabenko

I ran it on my real project and found a few minor things here and there. LGTM after that :)

examples/complete-http/main.tf

antonbabenko · 2024-05-30T09:24:05Z

examples/complete-http/outputs.tf

-  value       = module.lambda_function.lambda_function_arn
+output "test_curl_command" {
+  description = "Curl command to test API endpoint using mTLS"
+  value       = "curl --key ./my-key.key --cert ./my-cert.pem https://customer1.${replace(var.domain_name, "*.", "")} | jq"


Maybe this can be used instead of external curl - https://registry.terraform.io/providers/hashicorp/http/latest/docs/data-sources/http ?

This data source supports ca_cert_pem, but not key. It may be enough for this case.

I was unable to get that data source to work without the key. This isn't required to run the example, but rather a helpful reminder of the syntax to call the API using the cert/key - and curl/jq are usually quite common on machines these days

antonbabenko · 2024-05-30T09:27:40Z

examples/complete-http/variables.tf

+variable "domain_name" {
+  description = "Custom domain name to use on API Gateway endpoint"
+  type        = string
+  default     = "*.terraform-aws-modules.modules.tf"


Suggested change

default = "*.terraform-aws-modules.modules.tf"

default = "terraform-aws-modules.modules.tf"

Can we have it without a wildcard in the default_name? The description does not match the value :)

Reverted in 6154fc6

I also added a small section that demonstrates the use of wildcard custom domains and mapping subdomains to that wildcard domain in the readme

antonbabenko · 2024-05-30T10:38:02Z

outputs.tf

-  value       = try(aws_apigatewayv2_stage.default[0].invoke_url, "")
-}
-
-output "default_apigatewayv2_stage_domain_name" {


Please revert this output. Such a striped domain name is needed for Cloudfront.

re-added in 6154fc6

antonbabenko · 2024-05-30T10:51:19Z

variables.tf

+    data_trace_enabled       = optional(bool, false)
+    detailed_metrics_enabled = optional(bool, false)
+    logging_level            = optional(string)
+    throttling_burst_limit   = optional(number, 500)


These throttling_burst_limit and throttling_rate_limit defaults are being used instead of the one specified in stage_default_route_settings.

I think we should not have these defaults here and just use the settings in stage_default_route_settings. WDYT?

yes, I would normally agree and normally not like to have to set arbitrary numbers either as a default value. However, when testing I kept getting errors only to discover that things were set to 0, and there are various upstream issues both in Terraform and other projects:

[Bug]: Empty aws_apigatewayv2_stage.default_route_settings.throttling_burst_limit results in 0, not "not configured" hashicorp/terraform-provider-aws#27674

[Bug]: Removal of the Default route throttling for an API GatewayV2 sets the limits to 0 hashicorp/terraform-provider-aws#30373

so its really a matter of - do we set nothing, and its up to users to set something if they get 429s due to a limit of 0, or do we set something so it works out of the box, but there is a cap that again would require users to set a value that suits their use case? I am ok either way, I don't know what the right way is - either set something (across the board) or set nothing (null - again, across the board)

Thanks for pointing out the issues. I've encountered this problem in the past, but I didn't realize the reason.

Can we not have a default number here (null) but then use coalesce() in the code to get the provided value (if specified) or the value from stage_default_route_settings? This way, it will be closer to the expected behavior.

good point - updated to coalesce these with the stage default route settings and removed defaults from the route section of the variable in 6154fc6

main.tf

antonbabenko

Awesome! Thank you for taking into account all my comments. I am active user of this module myself :)

## [5.0.0](v4.0.0...v5.0.0) (2024-06-04) ### ⚠ BREAKING CHANGES * Updates to support websocket API Gateway (#54) ### Features * Updates to support websocket API Gateway ([#54](#54)) ([30c9db0](30c9db0))

antonbabenko · 2024-06-04T15:31:00Z

This PR is included in version 5.0.0 🎉

github-actions · 2024-07-06T02:07:21Z

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

chore: updates to support websocket API Gateway

8bd0adb

bryantbiggs requested a review from antonbabenko September 9, 2021 16:19

chore: add variable for default stage name

665781f

antonbabenko reviewed Sep 9, 2021

View reviewed changes

examples/complete-http/main.tf Show resolved Hide resolved

main.tf Outdated Show resolved Hide resolved

main.tf Outdated Show resolved Hide resolved

Fixed docs

f5b064a

antonbabenko mentioned this pull request Sep 23, 2021

Support authorization_scopes and request_models on routes #56

Closed

1 task

bryantbiggs and others added 2 commits October 2, 2021 08:29

Merge branch 'master' of github.com:bryantbiggs/terraform-aws-apigate…

da05c40

…way-v2 into chore/updates-for-websocket-support

Update main.tf

7016f1d

Co-authored-by: Anton Babenko <[email protected]>

bryantbiggs marked this pull request as draft October 2, 2021 17:33

bryantbiggs force-pushed the chore/updates-for-websocket-support branch from cacb480 to 402fa78 Compare October 2, 2021 17:38

chore: stash work in progress

0b888d9

bryantbiggs force-pushed the chore/updates-for-websocket-support branch from 402fa78 to 0b888d9 Compare October 2, 2021 17:44

bryantbiggs added 2 commits October 2, 2021 15:07

chore: it works so far

cfc7b2e

chore: cleanup and doc updates

a57c321

bryantbiggs force-pushed the chore/updates-for-websocket-support branch from f3bb532 to a57c321 Compare October 2, 2021 20:45

bryantbiggs marked this pull request as ready for review October 2, 2021 20:47

bryantbiggs requested a review from antonbabenko October 2, 2021 20:47

antonbabenko reviewed Oct 3, 2021

View reviewed changes

examples/websocket/main.tf Outdated Show resolved Hide resolved

examples/websocket/main.tf Outdated Show resolved Hide resolved

examples/websocket/main.tf Outdated Show resolved Hide resolved

main.tf Outdated Show resolved Hide resolved

bryantbiggs added 4 commits November 19, 2021 10:08

Merge branch 'master' of github.com:terraform-aws-modules/terraform-a…

a34b3c5

…ws-apigateway-v2 into chore/updates-for-websocket-support

chore: update pre-commit

a0967ac

Merge branch 'master' of github.com:bryantbiggs/terraform-aws-apigate…

e866133

…way-v2 into chore/updates-for-websocket-support

chore: update release configs based on EKS findings

b5fee7b

bryantbiggs marked this pull request as draft January 6, 2022 14:56

bryantbiggs added 5 commits January 6, 2022 09:56

chore: updating examples

7430d4b

refactor: update output value names

d4cd83a

chore: update variables

97d1c28

fix: set throttling limits to default to -1 to avoid traffic disrup…

c362535

…tion

chore: updating variables and adding new attributes

0f322e7

fix: Update upgrade template; only deploy when not an HTTP API

6ba213e

bryantbiggs force-pushed the chore/updates-for-websocket-support branch 2 times, most recently from b35afe7 to 781949b Compare May 13, 2024 23:53

chore: Update upgrade guide

1690b9c

bryantbiggs force-pushed the chore/updates-for-websocket-support branch from 781949b to 1690b9c Compare May 14, 2024 00:00

bryantbiggs marked this pull request as ready for review May 14, 2024 00:02

bryantbiggs requested a review from antonbabenko May 14, 2024 00:02

bryantbiggs removed the wip label May 14, 2024

feat: Add tag

95fcb7a

antonbabenko reviewed May 17, 2024

View reviewed changes

bryantbiggs mentioned this pull request May 20, 2024

chore: Add Python zip archive that is agnostic of a specific Python runtime version terraform-aws-modules/terraform-aws-lambda#573

Merged

3 tasks

bryantbiggs and others added 3 commits May 20, 2024 11:59

Update main.tf

1908469

Co-authored-by: Anton Babenko <[email protected]>

Update UPGRADE-5.0.md

f0be2e4

Co-authored-by: Anton Babenko <[email protected]>

feat: Updates from PR review

d6e694c

bryantbiggs force-pushed the chore/updates-for-websocket-support branch from 182d16c to d6e694c Compare May 21, 2024 00:05

antonbabenko reviewed May 30, 2024

View reviewed changes

bryantbiggs added 2 commits June 4, 2024 10:43

chore: Updates from PR review feedback

6154fc6

fix: Wrapper pre-commit hook still hates Linux machines

3fc2d44

bryantbiggs requested a review from antonbabenko June 4, 2024 14:53

antonbabenko approved these changes Jun 4, 2024

View reviewed changes

bryantbiggs merged commit 30c9db0 into terraform-aws-modules:master Jun 4, 2024
8 checks passed

bryantbiggs deleted the chore/updates-for-websocket-support branch June 4, 2024 15:30

github-actions bot locked as resolved and limited conversation to collaborators Jul 6, 2024

	default = "*.terraform-aws-modules.modules.tf"
	default = "terraform-aws-modules.modules.tf"

Uh oh!

feat!: Updates to support websocket API Gateway #54

feat!: Updates to support websocket API Gateway #54

Uh oh!

Conversation

bryantbiggs commented Sep 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Additional changes

Added

Modified

Removed

See UPGRADE-5.0.md for full list of changes and notes on upgrade path

Motivation and Context

Breaking Changes

How Has This Been Tested?

Uh oh!

antonbabenko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lllama commented Sep 9, 2021

Uh oh!

antonbabenko commented Sep 10, 2021

Uh oh!

antonbabenko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

choudharyac09 commented May 13, 2024

Uh oh!

bryantbiggs commented May 13, 2024

Uh oh!

choudharyac09 commented May 14, 2024

Uh oh!

antonbabenko commented May 14, 2024

Uh oh!

antonbabenko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

antonbabenko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bryantbiggs commented Sep 9, 2021 •

edited

Loading