resource/aws_api_gateway_deployment: Do not delete stages in use by other deployments #3896

mdlavin · 2018-03-23T12:57:25Z

When a deployment has been updated in a way that forces a create/delete, the newly deployed deployment can be left in a bad state. Here how the code used to work:

A new deployment is created that points to stage 'test'
The old deployment is marked for deletion and was associated with 'test'
The 'test' stage is deleted
The deposed deployment is deleted
The new deployment is now pointing at a stage that does not exist

Here is how the code works now:

A new deployment is created that points to stage 'test'
The old deployment is marked for deletion and was associated with 'test'
The 'test' stage is not deleted because it is no longer associated with the deposed deployment
The deposed deployment is deleted
The new deployment is still happy =)

…ther deployments

mdlavin · 2018-05-31T21:15:57Z

I just double checked and this PR still cleanly applies to master and passes the tests. Any chance of a merge soon before a conflict appears?

mdlavin · 2018-06-01T01:26:57Z

If anybody is interested in testing out this feature in a patched v1.21.0 version, I've made some Alpine Linux x64 binaries available here: https://github.com/lifeomic/terraform-provider-aws/releases/tag/v1.21.0_patched_5f7d0def

mdlavin · 2018-09-07T12:49:13Z

@bflad is there anything I can do to help this get merged into an official release?

mdlavin · 2018-09-13T20:21:30Z

If anybody is interested in testing out this feature in a patched v1.36.0 version, I've made some Alpine Linux x64 binaries available here: https://github.com/lifeomic/terraform-provider-aws/releases/tag/v1.36.0_patched_f2d0f833c

bflad · 2018-09-13T21:39:50Z

At first glance, I believe this change seems appropriate. We don't have too much precedent of resources that create other unmanaged resources though. 😅

More forward looking: I am not convinced that we should allow the deployment resource to create stages at all (and hence delete them too), as it leaves Terraform management in a precarious state with unmanaged resources or resources that need to be imported. Unfortunately, I do not believe the API Gateway API provides any mechanism to prevent this behavior beyond only sending the deployment to the "default" stage.

bflad

Some initial comments below -- please reach out with any questions!

bflad · 2018-09-13T21:44:05Z

aws/resource_aws_api_gateway_deployment.go

-			return nil
+		})
+		if err != nil {
+			return resource.RetryableError(err)


A few notes:

It seems like we should not retry on errors calling GetStage, especially any error

As its fairly unexpected that the deletion of a resource is requiring a read call to begin with, we should also return a helpful message here, e.g. return fmt.Errorf("error getting referenced stage: %s", err)

Seems like we should skip deleting the stage if its not found, instead of returning the stage not found error

I'll make the changes you suggest

bflad · 2018-09-13T21:49:58Z

aws/resource_aws_api_gateway_deployment.go

+		// If the stage has been updated to point at a different deployment, then
+		// the stage should not be removed then this deployment is deleted.
+		var shouldDeleteStage = true
+		if *stage.DeploymentId != d.Id() {


To prevent potential panics, this should perform a nil check and use the SDK helper function for the string value:

if stage == nil || aws.StringValue(stage.DeploymentId) != d.Id() { shouldDeleteState = false }

Good catch! I'll make that change

bflad · 2018-09-13T21:57:18Z

aws/resource_aws_api_gateway_deployment_test.go

+	return func(s *terraform.State) error {
+		conn := testAccProvider.Meta().(*AWSClient).apigateway
+
+		restApiId := aws.String(s.RootModule().Resources["aws_api_gateway_rest_api.test"].Primary.ID)


Nitpick: Instead of hardcoding this reference, we should prefer to pass into the function (any of these):

The REST API resource name

The actual apigateway.RestApi object (e.g. from testAccCheckAWSAPIGatewayRestAPIExists())

The deployment resource name, then reference its rest_api_id attribute

Personally, I'd go with the last option since this is explicitly named with Deployment. 😄

I'll see how hard the last bullet is. Seems like a good approach to me

…ed state lookups

mdlavin · 2018-09-18T22:54:06Z

@bflad I believe that I have addressed all of your feedback. Let me know if there is anything else I should change.

bflad

Thanks @mdlavin 🚀 Two non-blocking nits (which I'll fix on merge), but otherwise LGTM.

--- PASS: TestAccAWSAPIGatewayDeployment_basic (5.56s)
--- PASS: TestAccAWSAPIGatewayDeployment_createBeforeDestoryUpdate (40.73s)

bflad · 2018-10-02T18:06:21Z

aws/resource_aws_api_gateway_deployment_test.go

+		restApiId := aws.String(deploymentResource.Primary.Attributes["rest_api_id"])
+
+		req := &apigateway.GetStageInput{
+			StageName: &stageName,


Nitpick: the stage name can be fetched from attributes as well (deploymentResource.Primary.Attributes["stage_name"]) instead of a parameter passed in from the acceptance test

bflad · 2018-10-02T18:09:33Z

aws/resource_aws_api_gateway_deployment.go

+		if err != nil {
+			// If the stage is already deleted then there is no need to attempt deletion
+			apigatewayErr, _ := err.(awserr.Error)
+			if apigatewayErr.Code() == "NotFoundException" {


Nitpick: we have a helper function for this type of logic and prefer the AWS Go SDK constants, e.g. isAWSErr(err, apigateway.ErrCodeNotFoundException, "")

``` --- PASS: TestAccAWSAPIGatewayDeployment_basic (20.41s) --- PASS: TestAccAWSAPIGatewayDeployment_createBeforeDestoryUpdate (33.28s) ```

ghost · 2020-04-03T17:21:07Z

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!

resource/aws_api_gateway_deployment: Do not delete stages in use by o…

71a627e

…ther deployments

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

bflad added bug Addresses a defect in current functionality. service/apigateway Issues and PRs that pertain to the apigateway service. labels Mar 23, 2018

szilveszter mentioned this pull request Apr 9, 2018

Potential service outage if no custom domain used in API Gateway rackerlabs/xplat-terraform-modules#30

Open

bflad reviewed Sep 13, 2018

View reviewed changes

resource/aws_api_gateway_deployment: Do not retry on deletion of fail…

9bb2e22

…ed state lookups

ghost added size/L Managed by automation to categorize the size of a PR. service/apigateway Issues and PRs that pertain to the apigateway service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Sep 18, 2018

bflad approved these changes Oct 2, 2018

View reviewed changes

bflad added this to the v1.39.0 milestone Oct 2, 2018

bflad merged commit f9829c3 into hashicorp:master Oct 2, 2018

bflad added a commit that referenced this pull request Oct 2, 2018

Update CHANGELOG for #3896

ce0d57c

bflad added a commit that referenced this pull request Oct 2, 2018

resource/aws_api_gateway_deployment: Minor adjustments after #3896

8973a04

``` --- PASS: TestAccAWSAPIGatewayDeployment_basic (20.41s) --- PASS: TestAccAWSAPIGatewayDeployment_createBeforeDestoryUpdate (33.28s) ```

ghost locked and limited conversation to collaborators Apr 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resource/aws_api_gateway_deployment: Do not delete stages in use by other deployments #3896

resource/aws_api_gateway_deployment: Do not delete stages in use by other deployments #3896

mdlavin commented Mar 23, 2018

mdlavin commented May 31, 2018

mdlavin commented Jun 1, 2018

mdlavin commented Sep 7, 2018

mdlavin commented Sep 13, 2018 •

edited

Loading

bflad commented Sep 13, 2018

bflad left a comment

bflad Sep 13, 2018

mdlavin Sep 18, 2018

bflad Sep 13, 2018

mdlavin Sep 18, 2018

bflad Sep 13, 2018

mdlavin Sep 18, 2018

mdlavin commented Sep 18, 2018

bflad left a comment

bflad Oct 2, 2018

bflad Oct 2, 2018

ghost commented Apr 3, 2020

resource/aws_api_gateway_deployment: Do not delete stages in use by other deployments #3896

resource/aws_api_gateway_deployment: Do not delete stages in use by other deployments #3896

Conversation

mdlavin commented Mar 23, 2018

mdlavin commented May 31, 2018

mdlavin commented Jun 1, 2018

mdlavin commented Sep 7, 2018

mdlavin commented Sep 13, 2018 • edited Loading

bflad commented Sep 13, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdlavin commented Sep 18, 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

ghost commented Apr 3, 2020

mdlavin commented Sep 13, 2018 •

edited

Loading