Give addresses beta support, add internal addresses

[negz]

This PR (hopefully) ties together the existing work in #488. Specifically it:

• Adds support for specifying the internal IP address you wish to reserve per Adding functionality for static internal addresses to be configured #525
• Adds documentation for all the new knobs
• Addresses some PR feedback that came in after Give addresses beta support, add internal addresses. #488 was approved.
• Resolves some merge conflicts

I haven't contributed to Terraform before, but I'm pretty sure this works as expected:

$ GOOGLE_XPN_HOST_PROJECT=REDACTED \
   GOOGLE_PROJECT=REDACTED \
   GOOGLE_REGION=us-central1 \
   GOOGLE_USE_DEFAULT_CREDENTIALS=1 \
   TF_ACC=1 \
   go test -v . -run "^TestAccComputeAddress_internal"
=== RUN   TestAccComputeAddress_internal
--- PASS: TestAccComputeAddress_internal (114.13s)
PASS
ok      github.com/terraform-providers/terraform-provider-google/google 114.153s

[negz]

Ping. Is there anything I can do to help get this merged? :)

[paddycarver]

Echoing @rosbo's comment: we should explicitly document (or use Default to set) the default value here.

Source: https://github.com/terraform-providers/terraform-provider-google/pull/488/files#r142192254

[paddycarver]

@rosbo mentioned we don't actually need to convert here: https://github.com/terraform-providers/terraform-provider-google/pull/488/files#r142192681

[negz]

@paddycarver Thanks for the review! Please take another look - comments should be addressed.

[paddycarver]

Looks like we're almost there, @negz! Thanks for all the work on this. It looks like a couple tests are failing now. I think we need:

• to set address_type on read, so import works and we can detect drift
• to convert the self_link to v1, so the import test passes and we have consistent self_links: https://github.com/terraform-providers/terraform-provider-google/blob/57449a88a24bdfa070b0e3dbb6da99d82b092678/google/resource_compute_instance.go#L971

[negz]

@paddycarver Whoops, my mistake! Should have run all the tests. I believe I've corrected everything you mentioned, but I added a new test TestAccComputeAddress_importInternal and can't seem to get it to work:

$ TF_ACC=1 go test -v -timeout=120m -run "^TestAccComputeAddress_.+" .
=== RUN   TestAccComputeAddress_importBasic
=== RUN   TestAccComputeAddress_importInternal
=== RUN   TestAccComputeAddress_basic
=== RUN   TestAccComputeAddress_internal
--- PASS: TestAccComputeAddress_internal (306.37s)
--- PASS: TestAccComputeAddress_importBasic (23.33s)
--- PASS: TestAccComputeAddress_basic (23.44s)
--- FAIL: TestAccComputeAddress_importInternal (317.17s)
        testing.go:434: Step 1 error: ImportStateVerify attributes not equivalent. Difference is shown below. Top is actual, bottom is expected.

                (map[string]string) (len=2) {
                 (string) (len=12) "address_type": (string) (len=8) "EXTERNAL",
                 (string) (len=10) "subnetwork": (string) ""
                }


                (map[string]string) (len=2) {
                 (string) (len=12) "address_type": (string) (len=8) "INTERNAL",
                 (string) (len=10) "subnetwork": (string) (len=121) "https://www.googleapis.com/compute/v1/projects/planet-k8s-staging/regions/us-east1/subnetworks/subnetwork-test-j1sb4wplcw"
                }

FAIL
exit status 1
FAIL    github.com/terraform-providers/terraform-provider-google/google 623.560s

It seems as if we're trying to use the v1 API when importing google_compute_address.internal_with_subnet despite that resource having schema fields set that require v0beta - i.e. subnetwork and address_type.

Any idea how I can get this test to pass?

[rosbo]

Update AddressVersionedFeatures to set the default value for the address_type field.

See an example here:
https://github.com/terraform-providers/terraform-provider-google/blob/master/google/resource_compute_firewall.go#L23

[negz]

@rosbo Your suggested change makes sense, thanks.

I found that the test still failed after setting the default value for address_type, but passed after I also explicitly called resourceComputeAddressRead from within resourceComputeAddressParseState.

Without having dug into the mechanics of how Terraform imports work I had assumed something under the hood was triggering a read subsequent to the import function being called to determine the resource's ID, but it seems that's not the case?

I believe all integration tests matching ^TestAccComputeAddress_.+ are now passing.

Updates made.

[paddycarver]

I'm :/ about explicitly calling Read, as that should be unnecessary, and is a bit of a weird situation--it should be automatically Read after calling Import, anyways, I believe? I looked into this some, and can't explain the behaviour--without your fix, I can't find anything that would set EXTERNAL in state the way the test is saying it is. @rosbo, any thoughts? I'll also ask a few other people on the Terraform team if they have any thoughts here, because this is decidedly odd.

[negz]

@paddycarver Thanks for following up - glad I wasn't just missing something obvious.

[rosbo]

I found why the additional call to the read method inside the import method makes the test pass.

The read method takes a *schema.ResourceData as an argument often called d. d stores, among other things, the attributes values and the id. The read method IS called automatically after the import method is called. However, the ResourceData passed as an argument to the read method has an empty attributes map and only the id is set. Inside the read method, we check whether we should use the v1 or the beta api. Since the attribute address_type is not set (remember, the attributes map in d is empty), it calls the v1 API. The v1 API does not set the address_type and it therefor defaults to EXTERNAL.

The extra call to read inside the import method means that the read method is now called twice. The second time the read method is called the attributes values are set (the first call to read sets them). However, the address_type was set to EXTERNAL so it shouldn't trigger the beta API right? That is correct, the reason why the beta API is called is because of a bug introduced in this PR. The address field is marked as a beta field. The field is computed. If the user does not specify an address field, we put the value generated by the server. This means address is always set which means that the beta API is always used even if the user does not set the address field.

A few things to fix:

• the address field should not be marked as beta. The field is present in the v1 API.
• open question: How to detect that the import method should use the beta API... I need to think more about a solution for this.

Does that mean that import is broken for any beta resource?
Yes, but running terraform apply fixes it. However, if the beta field is ForceNew, this is bad because the newly imported resource gets recreated!

Let's say you want to import the following resource:

resource "google_compute_address" "test" {
  name = "test"
  address_type = "INTERNAL"
  subnetwork = "https://www.googleapis.com/compute/v1/projects/my-project/regions/us-central1/subnetworks/default"
}

When you run terraform import google_compute_address.test test

• terraform show has address_type set to EXTERNAL (this is wrong!)
• terraform plan shows a diff.
• terraform apply will fix the diff by either calling update or create (depending on ForceNew value for the field) and the state should be properly set afterwards.

[rosbo]

testAccComputeAddress_internal creates 3 internal addresses.

I would add 2 extra test steps below to test that the import worked properly for the resource google_compute_address.internal and internal_with_subnet_and_address too. These checks are cheap and the resources gets created anyway as of now.

[negz]

@rosbo I've removed address from the beta fields, and removed the double-read bit.

Given that we've discovered imports are broken for any beta resources, how would you feel merging this PR (perhaps without the internal import acceptance tests?) and addressing the beta import issue separately?

[rosbo]

This sounds reasonable to me. Please comment out the import acceptance tests and a quick comment explaining why the test is commented-out.

Thanks

[negz]

@rosbo will do. Raised #694 to track the separate issue for now.

[rosbo]

Thanks for filling the new issue :)

[negz]

@rosbo I've commented out the offending tests:

$ go test -timeout=1200s -v . -run "^TestAccComputeAddress_.+$"
=== RUN   TestAccComputeAddress_importBasic
=== RUN   TestAccComputeAddress_basic
=== RUN   TestAccComputeAddress_internal
--- PASS: TestAccComputeAddress_internal (308.59s)
--- PASS: TestAccComputeAddress_importBasic (23.31s)
--- PASS: TestAccComputeAddress_basic (23.35s)
PASS
ok      github.com/terraform-providers/terraform-provider-google/google 331.967s

[ghost]

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. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!