Add detach support which is compatible with hcl2 used for terraform 0.12 #45

btromanova · 2019-08-15T13:23:37Z

This fixes #44

Context:
--detach flag is used in astro to make terraform migrations easier. When terraform resources need to be moved or deleted terraform state subcommands are used to manipulate the state file.
If the remote backend is configured, state file will be uploaded after state manipulation is successful. Which is not desired while migration script is being tested. With detach option astro inits the module (this downloads the state file from remote), modifies the configuration to remove remote backend (which makes terraform use "local" backend) and runs terraform init -force-copy again to re-initialize with new backend. See more in https://www.terraform.io/docs/backends/config.html#unconfiguring-a-backend.

There were two issues with --detach flag that were uncovered by terraform 0.12 upgrade:

astro failed if there was a file in the config that contained terraform block, but didn't contain backend block. This bug exists with the older versions of terraform. This is fixed by changing astDel to astDelIfExists.
To remove remote backend from config, astro walks and modifies the HCL AST tree.
Terraform 0.12 switched to the new language (HCL2), so old parsing mechanism stopped working.
I wasn't able to find a way to walk and modify tree with https://godoc.org/github.com/hashicorp/hcl2 (similar issue reported https://github.com/hashicorp/hcl2/issues/23), so I'm using regexp to do this for the simple configurations. That unblocks us for the terraform 0.12 upgrade, but may be not a good solution for the long term.

btromanova · 2019-08-15T13:32:18Z

astro/terraform/terraform_remote_state_disable_test.go

 	input := []byte(`
 terraform {
    backend "s3" {}
    }

    provider "aws" {
-    region = "us-east-1"
+      region = "${var.aws_region}"


I changed this to explicitly show that this is HCL 1.0

CorgiMan

LGTM

Take a look at my comment and see if you feel like addressing it :).

CorgiMan · 2019-08-15T17:00:16Z

astro/terraform/terraform_remote_state_disable_utils.go

+	// Regexp to find if any backend configuration exists
+	backendDefinitionRe := regexp.MustCompile(`(?s)[{\s+]backend\s+"[^"]+"\s*{`)
+	// Regexp to find simple backend configuration, which doesn't contain '{}' inside
+	backendBlockRe := regexp.MustCompile(`(?s)(\s*backend\s+"[^"]+"\s*{[^{]*?})`)


would it be easier to, instead of regex, iterate the input as follows:

ix = in.find("\"backend\"") bracket_count = 0 for ; ix<len(in); ix++ { if in[ix]=='{' { bracket_count++ } if in[ix]=='}' { bracket_count-- if bracket_count==0 { end_ix = ix break } } }

That way we support complicated configs as well, right? There are some edge cases of course, but this covers a lot more expected cases.

This does assume that the config is correct HCL of course.

Yep, I thought about this solution. It actually doesn't look as bad at I thought it would.
It's still not ideal, because '{' or '}' can be part of some other lexeme. But in reality this is very unlikely. I'll change the code to what you've suggested, thanks!

I implemented suggested approach and got to writing tests, and I'm not sure if it's a good approach anymore.
The problem is that this config

terraform { backend "local" { constant = "weird_string_{" } }

Will be recognized as correct expression and turned into

terraform {

I think it's better to fail explicitly in this case.
So once the HCL2 library is ready we should parse config using it, but until then, I'd stick with regexp on simple configs and failures on complex ones.

dansimau

Left some comments and questions.

astro/terraform/terraform_remote_state_disable_utils.go

astro/terraform/terraform_remote_state_disable_test.go

astro/terraform/terraform_remote_state_disable_utils.go

dansimau · 2019-09-19T14:57:48Z

astro/terraform/terraform_remote_state_disable_utils.go

+// This method should be rewritten once hcl2 supports AST traversal and modification.
+func deleteTerraformBackendConfigWithHCL2(in []byte) (updatedConfig []byte, err error) {
+	// Regexp to find if any backend configuration exists
+	backendDefinitionRe := regexp.MustCompile(`(?s)[{\s+]backend\s+"[^"]+"\s*{`)


Why do these two regexps need to be different?

I made a decision to only work with simple cases, where backend configuration doesn't have other braces inside. However, I want to notify the user, that may have a complex configuration about the error (that backend configuration was not removed).

The first regex is to find whether any backend configuration exists, second to find backend configuration that doesn't have braces inside.

Please let me know, if you think I need to put more comments about it in the code.

dansimau · 2019-09-19T14:58:58Z

astro/terraform/terraform_remote_state_disable_utils.go

+	// Regexp to find if any backend configuration exists
+	backendDefinitionRe := regexp.MustCompile(`(?s)[{\s+]backend\s+"[^"]+"\s*{`)
+	// Regexp to find simple backend configuration, which doesn't contain '{}' inside
+	backendBlockRe := regexp.MustCompile(`(?s)(\s*backend\s+"[^"]+"\s*{[^{]*?})`)


It might be worth expanding this regexp and commenting it, e.g.:
https://stackoverflow.com/questions/24588352/go-idiom-for-writing-long-regular-expressions-embedded-comments

Ack, thanks for the suggestion, I've added comments.

dansimau · 2019-09-27T09:10:38Z

astro/terraform/terraform_remote_state_disable_test.go

+		expected string
+	}{
+		{
+			config: `


This still looks weird. Is it because of a tabs/spaces mix?

dansimau

Fix the tab/space issue but after that LGTM! THANK YOU FOR YOUR FANTASTIC WORK!!!

btromanova requested a review from CorgiMan August 15, 2019 13:23

btromanova commented Aug 15, 2019

View reviewed changes

CorgiMan approved these changes Aug 15, 2019

View reviewed changes

dansimau reviewed Sep 19, 2019

View reviewed changes

btromanova force-pushed the tr/variable-filters branch 2 times, most recently from 90fa618 to 3144964 Compare September 24, 2019 09:32

btromanova force-pushed the tr/fix-detach branch from 0af9a19 to 310caea Compare September 24, 2019 09:50

btromanova changed the base branch from tr/variable-filters to master September 24, 2019 09:51

btromanova added 8 commits September 27, 2019 08:06

Add test

713a356

Don't pass variable to a module if it doesn't require it

3deaa0f

Move test from integration to internal to speed up the execution

0607675

Smaller diff

bed01b2

Get plan output with terraform 0.12

5e1f603

Feedback: move StringVersionMatches from terraform package to test

e42b365

Add detach support which is compatible with hcl2 used for terraform 0.12

3aa637d

Review feedback

64bb150

btromanova force-pushed the tr/fix-detach branch from 310caea to 64bb150 Compare September 27, 2019 07:27

dansimau reviewed Sep 27, 2019

View reviewed changes

dansimau approved these changes Sep 27, 2019

View reviewed changes

Spaces -> tabs

618e7f2

btromanova merged commit f15067e into master Sep 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add detach support which is compatible with hcl2 used for terraform 0.12 #45

Add detach support which is compatible with hcl2 used for terraform 0.12 #45

btromanova commented Aug 15, 2019 •

edited

Loading

btromanova Aug 15, 2019

CorgiMan left a comment

CorgiMan Aug 15, 2019

btromanova Aug 19, 2019

btromanova Aug 21, 2019

dansimau left a comment

dansimau Sep 19, 2019

btromanova Sep 27, 2019

dansimau Sep 19, 2019

btromanova Sep 27, 2019

dansimau Sep 27, 2019

dansimau left a comment

Add detach support which is compatible with hcl2 used for terraform 0.12 #45

Add detach support which is compatible with hcl2 used for terraform 0.12 #45

Conversation

btromanova commented Aug 15, 2019 • edited Loading

Choose a reason for hiding this comment

CorgiMan 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

dansimau 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

dansimau left a comment

Choose a reason for hiding this comment

btromanova commented Aug 15, 2019 •

edited

Loading