Skip to content
This repository has been archived by the owner on May 15, 2023. It is now read-only.

Updated terraform-google-conversion to latest version #85

Merged

Conversation

onetwopunch
Copy link
Contributor

@onetwopunch onetwopunch commented Oct 28, 2019

Fixes: #81
Enables fix for: #52

  • Updated a few dependencies: terraform-google-conversion and terraform-provider-google
  • Changed dependency on terraform to terraform-plugin-sdk
  • Updated integration test template since diskEncryptionKey no longer is part of the output

Outdated: Initial ask for assistance

When running integration tests on Docker as well as running terraform-validator after a build from my host produces the same panic error:

root@c9d8fdd904ad:/terraform-validator# make test-integration
go test -tags= -v ./test
panic: gob: registering duplicate types for "*tfdiags.rpcFriendlyDiag": *tfdiags.rpcFriendlyDiag != *tfdiags.rpcFriendlyDiag

goroutine 1 [running]:
encoding/gob.RegisterName(0x18af868, 0x18, 0x1bed6e0, 0x0)
	/usr/local/go/src/encoding/gob/type.go:820 +0x5d7
encoding/gob.Register(0x1bed6e0, 0x0)
	/usr/local/go/src/encoding/gob/type.go:874 +0x123
github.com/hashicorp/terraform-plugin-sdk/internal/tfdiags.init.0()
	/terraform-validator/vendor/github.com/hashicorp/terraform-plugin-sdk/internal/tfdiags/rpc_friendly.go:58 +0x39
FAIL	github.com/GoogleCloudPlatform/terraform-validator/test	0.040s
make: *** [Makefile:29: test-integration] Error 1

I couldn't find much online about this, except this closed issue that seems to indicate there is a duplicate of the tfdiags package somewhere that is breaking gob encoding since they are named the same but are different.

The only two modules that call this package are terraform and terraform-plugin-sdk, both of which have been in use.

I also noticed that we have not updated the terraform-google-conversion module since Sept 4 where the commit was 791ccb94729b. The git log shows we have 58 commits between then and now:

$ git log 791ccb94729b..HEAD --pretty=oneline | wc -l
      58

Before I systematically try to determine which commit introduced this error, I was hoping someone who has worked with this tool longer than I might know a place to start of the top of their head.

@onetwopunch
Copy link
Contributor Author

CC @morgante @yukinying

@morgante
Copy link
Contributor

I haven't seen this yet, so not sure what's causing it.

One thing we can try is to get off the dependency on the terraform module and instead rely on terraform-plugin-sdk module, which provides its own helper package. Not sure if it'll fix this, but it is the long-term direction from HashiCorp.

@onetwopunch
Copy link
Contributor Author

That's good information. Dug a bit deeper and it turns out that since we didn't actually pull in GoogleCloudPlatform/terraform-google-conversion@940fcdf into terraform-validator til now, it seems we never tested that this change didn't break anything in terraform-validator. Since terraform-plugin-sdk and terraform have packages named the same thing, and packages are global AFAIK, you can't have both in the same project or you get errors. We currently use terraform-plugin-sdk in terraform-google-conversion and terraform indirectly from terraform-google-provider.

If we want to migrate to terraform-plugin-sdk in terraform-google-conversion, we also have to do it in terraform-google-provider. To fix a few issues in terraform-validator (namely #81 and #52), we need changes to be picked up in terraform-google-conversion, so I propose changing back to use the same module as terraform-google-provider and wait til they migrate to terraform-plugin-sdk. Thoughts? @morgante @slevenick @ndmckinley

@morgante
Copy link
Contributor

@onetwopunch I think our long term goal is actually to remove dependencies on terraform-google-provider entirely from terraform-validator and terraform-google-conversion. This might be a good opportunity to do that. @danawillow Thoughts?

Since terraform-plugin-sdk and terraform have packages named the same thing, and packages are global AFAIK, you can't have both in the same project or you get errors.

I'm not sure that's accurate? I'm pretty sure you can import two identically-named packages, so long as you alias them.

@onetwopunch
Copy link
Contributor Author

@morgante Interesting that we're thinking of removing that dependency. I think that might solve it as well.

I'm pretty sure you can import two identically-named packages, so long as you alias them.

You're absolutely right but the problem is that these packages are invoked by the same name in two dependencies of not terraform-validator itself. I tried replacing references to terraform in terraform-validator with terraform-plugin-sdk, and still getting errors:

$ go test -mod=vendor  $(go list ./... | grep -v validator/test)
# github.com/GoogleCloudPlatform/terraform-validator/converters/google
converters/google/convert.go:93:26: impossible type assertion:
	*"github.com/hashicorp/terraform-plugin-sdk/helper/schema".Provider does not implement "github.com/hashicorp/terraform/terraform".ResourceProvider (wrong type for Apply method)
		have Apply(*"github.com/hashicorp/terraform-plugin-sdk/terraform".InstanceInfo, *"github.com/hashicorp/terraform-plugin-sdk/terraform".InstanceState, *"github.com/hashicorp/terraform-plugin-sdk/terraform".InstanceDiff) (*"github.com/hashicorp/terraform-plugin-sdk/terraform".InstanceState, error)
		want Apply(*"github.com/hashicorp/terraform/terraform".InstanceInfo, *"github.com/hashicorp/terraform/terraform".InstanceState, *"github.com/hashicorp/terraform/terraform".InstanceDiff) (*"github.com/hashicorp/terraform/terraform".InstanceState, error)
# github.com/GoogleCloudPlatform/terraform-validator/converters/google [github.com/GoogleCloudPlatform/terraform-validator/converters/google.test]
converters/google/convert.go:93:26: impossible type assertion:
	*"github.com/hashicorp/terraform-plugin-sdk/helper/schema".Provider does not implement "github.com/hashicorp/terraform/terraform".ResourceProvider (wrong type for Apply method)
		have Apply(*"github.com/hashicorp/terraform-plugin-sdk/terraform".InstanceInfo, *"github.com/hashicorp/terraform-plugin-sdk/terraform".InstanceState, *"github.com/hashicorp/terraform-plugin-sdk/terraform".InstanceDiff) (*"github.com/hashicorp/terraform-plugin-sdk/terraform".InstanceState, error)
		want Apply(*"github.com/hashicorp/terraform/terraform".InstanceInfo, *"github.com/hashicorp/terraform/terraform".InstanceState, *"github.com/hashicorp/terraform/terraform".InstanceDiff) (*"github.com/hashicorp/terraform/terraform".InstanceState, error)```

Please let me know if I'm missing something with go modules since I don't have a ton of experience using both modules and vendor directories together.

@danawillow
Copy link

Agreed that this would be a good opportunity to remove dependencies on TPG, but I'm confused about the actual problem that's occurring, since we don't use github.com/hashicorp/terraform anymore in the provider: https://github.com/terraform-providers/terraform-provider-google/blob/master/go.mod

@onetwopunch
Copy link
Contributor Author

@danawillow Thanks for pointing that out, I think we must also have a dated version of the provider. I'll try to update that as well and get back to you.

@onetwopunch
Copy link
Contributor Author

onetwopunch commented Oct 30, 2019

@morgante this is ready for re-review.

@morgante
Copy link
Contributor

@onetwopunch Can you make sure to update the 0.12 file as well? Or we can remove it entirely.

See: https://github.com/GoogleCloudPlatform/terraform-validator/blob/master/Makefile#L15

@morgante
Copy link
Contributor

Also, the example config isn't working for me:

../bin/terraform-validator convert tfplan.json --project gcp-devops-iac
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0xa0 pc=0x27b484e]

goroutine 1 [running]:
github.com/GoogleCloudPlatform/terraform-google-conversion/google.resolveImageImageExists(0xc00046a000, 0xc0004ce6e0, 0x1a, 0xc0004ce6c0, 0x19, 0x5, 0xc0004ce6c0, 0x19)
	/Users/morgantep/code/google/validator/terraform-validator/vendor/github.com/GoogleCloudPlatform/terraform-google-conversion/google/image.go:47 +0x3e
github.com/GoogleCloudPlatform/terraform-google-conversion/google.resolveImage(0xc00046a000, 0xc0004ce6e0, 0x1a, 0xc0004ce6c0, 0x19, 0x1, 0x30df13e, 0x9, 0xc000044230)
	/Users/morgantep/code/google/validator/terraform-validator/vendor/github.com/GoogleCloudPlatform/terraform-google-conversion/google/image.go:164 +0x1652
github.com/GoogleCloudPlatform/terraform-google-conversion/google.resourceComputeDiskEncoder(0x3538bc0, 0xc0001f7380, 0x2ce84a0, 0xc00046a000, 0xc00051c300, 0x1, 0x0, 0x0)
	/Users/morgantep/code/google/validator/terraform-validator/vendor/github.com/GoogleCloudPlatform/terraform-google-conversion/google/compute_disk.go:347 +0x56d
github.com/GoogleCloudPlatform/terraform-google-conversion/google.GetComputeDiskApiObject(0x3538bc0, 0xc0001f7380, 0xc00046a000, 0x3144c5c, 0x4b, 0xc00004c1e0)
	/Users/morgantep/code/google/validator/terraform-validator/vendor/github.com/GoogleCloudPlatform/terraform-google-conversion/google/compute_disk.go:323 +0x1365
github.com/GoogleCloudPlatform/terraform-google-conversion/google.GetComputeDiskCaiObject(0x3538bc0, 0xc0001f7380, 0xc00046a000, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0xc000043d50, ...)
	/Users/morgantep/code/google/validator/terraform-validator/vendor/github.com/GoogleCloudPlatform/terraform-google-conversion/google/compute_disk.go:226 +0xdc
github.com/GoogleCloudPlatform/terraform-validator/converters/google.(*Converter).AddResource(0xc000209d40, 0x352f740, 0xc00019cd00, 0xc000209da0, 0xc000110460)
	/Users/morgantep/code/google/validator/terraform-validator/converters/google/convert.go:140 +0x14c
github.com/GoogleCloudPlatform/terraform-validator/tfgcv.ReadPlannedAssets(0x7ffeefbff893, 0xb, 0x7ffeefbff8a9, 0xe, 0x0, 0x0, 0xc0001f6d00, 0x0, 0x0, 0x0, ...)
	/Users/morgantep/code/google/validator/terraform-validator/tfgcv/planned_assets.go:93 +0x6b3
github.com/GoogleCloudPlatform/terraform-validator/cmd.glob..func2(0x4836800, 0xc0001eb500, 0x1, 0x3, 0x0, 0x0)
	/Users/morgantep/code/google/validator/terraform-validator/cmd/convert.go:49 +0x92
github.com/spf13/cobra.(*Command).execute(0x4836800, 0xc0001eb470, 0x3, 0x3, 0x4836800, 0xc0001eb470)
	/Users/morgantep/code/google/validator/terraform-validator/vendor/github.com/spf13/cobra/command.go:762 +0x465
github.com/spf13/cobra.(*Command).ExecuteC(0x4836cc0, 0x0, 0x0, 0x29861c4)
	/Users/morgantep/code/google/validator/terraform-validator/vendor/github.com/spf13/cobra/command.go:850 +0x2fc
github.com/spf13/cobra.(*Command).Execute(...)
	/Users/morgantep/code/google/validator/terraform-validator/vendor/github.com/spf13/cobra/command.go:800
github.com/GoogleCloudPlatform/terraform-validator/cmd.Execute()
	/Users/morgantep/code/google/validator/terraform-validator/cmd/root.go:77 +0x2e
main.main()
	/Users/morgantep/code/google/validator/terraform-validator/main.go:20 +0x20

@onetwopunch
Copy link
Contributor Author

@morgante I fixed the issue with the convert and it still passes tests but I had to add this
https://github.com/GoogleCloudPlatform/terraform-validator/pull/85/files#diff-747eeb1f3694357a501140221061b33eR93-R97

converter.ConfigureBasePaths(cfg)
if err := cfg.LoadAndValidate(); err != nil {
	return nil, errors.Wrap(err, "configuring")
}

I'm not sure why it was removed in 00b0c33 since I don't think there was a PR associated with this commit. Let me know your thoughts on whether this is ok to add back in.

CC @yukinying

@yukinying
Copy link
Contributor

I'm not sure why it was removed in 00b0c33 since I don't think there was a PR associated with this commit. Let me know your thoughts on whether this is ok to add back in.

The reason that was removed is that terraform-google-conversion should not perform online operation. That was discussed with @danawillow and @chrisst a while ago. It looks like we may have missed patching the resolveImage call by making it error out when an image path provided is not of the form of /project/<project>/global/image/<image> or global/image/<image>, as image path not in those form would trigger an online lookup.

@onetwopunch
Copy link
Contributor Author

@yukinying @morgante Just rebased off Albert's changes in #90

@morgante morgante merged commit 786f0ef into GoogleCloudPlatform:master Nov 5, 2019
@onetwopunch onetwopunch deleted the upgrade-conversion-dep branch November 5, 2019 18:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

panic: runtime error: index out of range [0] with length 0
4 participants