RouterBgpPeer BFD support #3258

drebes · 2020-03-17T08:07:28Z

Release Note Template for Downstream PRs (will be copied)

compute: Added `bfd` to `google_compute_router_peer`

modular-magician · 2020-03-17T08:12:28Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 1 file changed, 36 insertions(+))
Terraform Beta: Diff ( 3 files changed, 561 insertions(+))

drebes · 2020-03-17T08:20:52Z

This is causing a panic here:

func flattenComputeRouterBgpPeerBfd(v interface{}, d *schema.ResourceData, config *Config) interface{} {
        if v == nil {
                return v
        }
        l := v.([]interface{})

when we read back the bfd block and try to flatten it:

$ make testacc TEST=./google-beta TESTARGS='-run=TestAccComputeRouterPeer_basic'
==> Checking source code against gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 TF_SCHEMA_PANIC_ON_ERROR=1 go test ./google-beta -v -run=TestAccComputeRouterPeer_basic -timeout 240m -ldflags="-X=github.com/terraform-providers/terraform-provider-google-beta/version.ProviderVersion=acc"
=== RUN   TestAccComputeRouterPeer_basic
=== PAUSE TestAccComputeRouterPeer_basic
=== CONT  TestAccComputeRouterPeer_basic
panic: interface conversion: interface {} is map[string]interface {}, not []interface {}

goroutine 572 [running]:
github.com/terraform-providers/terraform-provider-google-beta/google-beta.flattenComputeRouterBgpPeerBfd(0x32c6b60, 0xc000ebe0f0, 0xc000028460, 0xc000897800, 0xc0000ed068, 0x0)
	/Users/drebes/Development/go/src/github.com/terraform-providers/terraform-provider-google-beta/google-beta/resource_compute_router_bgp_peer.go:643 +0x5f1
github.com/terraform-providers/terraform-provider-google-beta/google-beta.resourceComputeRouterBgpPeerRead(0xc000028460, 0x339f140, 0xc000897800, 0x2, 0x2)
...

products/compute/api.yaml

modular-magician · 2020-03-17T22:31:30Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 3 files changed, 43 insertions(+), 3 deletions(-))
Terraform Beta: Diff ( 3 files changed, 567 insertions(+), 3 deletions(-))

danawillow

Tests are failing:

TestAccComputeRouterPeer_enable
  === RUN   TestAccComputeRouterPeer_enable
=== PAUSE TestAccComputeRouterPeer_enable
=== CONT  TestAccComputeRouterPeer_enable
--- FAIL: TestAccComputeRouterPeer_enable (272.74s)
    testing.go:635: Step 2 error: errors during apply:
        
        Error: Error updating RouterBgpPeer "projects/ci-test-project-188019/regions/us-central1/routers/router-peer-test-bveczl1ojl/router-peer-test-bveczl1ojl": googleapi: Error 400: Invalid value for field '': ''. Bgp Peer Enable field is not supported yet., invalid
        
          on /opt/teamcity-agent/temp/buildTmp/tf-test516292606/main.tf line 78:
          (source code not available)
        
        
FAIL

TestAccComputeRouterPeer_bfd
  === RUN   TestAccComputeRouterPeer_bfd
=== PAUSE TestAccComputeRouterPeer_bfd
=== CONT  TestAccComputeRouterPeer_bfd
--- FAIL: TestAccComputeRouterPeer_bfd (273.17s)
    testing.go:635: Step 2 error: errors during apply:
        
        Error: Error updating RouterBgpPeer "projects/ci-test-project-188019/regions/us-central1/routers/router-peer-test-b33119hwzx/router-peer-test-b33119hwzx": googleapi: Error 400: Invalid value for field 'project': 'ci-test-project-188019'. Bgp Peer BFD Min Transmit Interval field is not supported yet., invalid
        
          on /opt/teamcity-agent/temp/buildTmp/tf-test782931738/main.tf line 78:
          (source code not available)
        
        
FAIL

products/compute/api.yaml

danawillow · 2020-03-17T23:32:55Z

products/compute/api.yaml

+          routing information is removed. If set to TRUE, the peer connection
+          can be established with routing information. The default is TRUE.
+        min_version: beta
+        default_value: "true"


Does "true" work here or does it have to be all caps?

Surprisingly it does. Regarding your errors this is because the feature is currently in closed Beta requiring project whitelisting, otherwise the fields are unavailable (and a reason why we shouldn't merge before the open Beta is launched, and the enable = TRUE by default will fail if the project is not whitelisted).

modular-magician · 2020-03-18T22:50:05Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 3 files changed, 43 insertions(+), 3 deletions(-))
Terraform Beta: Diff ( 3 files changed, 572 insertions(+), 3 deletions(-))

modular-magician · 2020-03-18T23:01:10Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 3 files changed, 43 insertions(+), 3 deletions(-))
Terraform Beta: Diff ( 3 files changed, 571 insertions(+), 3 deletions(-))

drebes · 2020-03-18T23:03:44Z

Tests all pass now, but as it still requires project whitelisting we should hold on merging as it would break peers on TPGB.

$ make testacc TEST=./google-beta TESTARGS='-run=TestAccComputeRouterPeer'
==> Checking source code against gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 TF_SCHEMA_PANIC_ON_ERROR=1 go test ./google-beta -v -run=TestAccComputeRouterPeer -timeout 240m -ldflags="-X=github.com/terraform-providers/terraform-provider-google-beta/version.ProviderVersion=acc"
=== RUN   TestAccComputeRouterPeer_basic
=== PAUSE TestAccComputeRouterPeer_basic
=== RUN   TestAccComputeRouterPeer_advertiseMode
=== PAUSE TestAccComputeRouterPeer_advertiseMode
=== RUN   TestAccComputeRouterPeer_enable
=== PAUSE TestAccComputeRouterPeer_enable
=== RUN   TestAccComputeRouterPeer_bfd
=== PAUSE TestAccComputeRouterPeer_bfd
=== CONT  TestAccComputeRouterPeer_basic
=== CONT  TestAccComputeRouterPeer_bfd
=== CONT  TestAccComputeRouterPeer_enable
=== CONT  TestAccComputeRouterPeer_advertiseMode
--- PASS: TestAccComputeRouterPeer_advertiseMode (185.63s)
--- PASS: TestAccComputeRouterPeer_basic (205.13s)
--- PASS: TestAccComputeRouterPeer_bfd (221.41s)
--- PASS: TestAccComputeRouterPeer_enable (227.80s)
PASS
ok  	github.com/terraform-providers/terraform-provider-google-beta/google-beta	227.851s

danawillow · 2020-06-10T20:31:37Z

@drebes what's the current status on this?

drebes · 2020-06-10T20:48:05Z

Sorry, mixed this with the firewall one. This is on hold as the public beta is delayed.

drebes · 2021-10-28T07:13:16Z

BFD finally made it to Public Preview. Unfortunately, BFD on Cloud Routers attached to VPN tunnels has been removed so there is no easy way to implement acceptance tests (Only Partner and Dedicated Interconnect is supported, but Partner interconnected manages the interface and BGP peer automatically, so can't be used for acceptance testing).

modular-magician · 2021-10-28T07:21:52Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 1 file changed, 68 insertions(+), 9 deletions(-))
Terraform Beta: Diff ( 12 files changed, 270 insertions(+), 22 deletions(-))

drebes · 2021-10-28T20:29:41Z

Added integration tests, using VPN. VPN accepts the bfd field as long as the state is set to DISABLED, so we use that.

modular-magician · 2021-10-28T20:41:17Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 3 files changed, 402 insertions(+), 9 deletions(-))
Terraform Beta: Diff ( 10 files changed, 406 insertions(+), 12 deletions(-))

modular-magician · 2021-10-28T22:33:02Z

I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccDatasourceGoogleServiceNetworkingPeeredDnsDomain_basic|TestAccCloudbuildWorkerPool_withNetwork|TestAccCloudFunctionsFunction_vpcConnector|TestAccComputeFirewallPolicyRule_update|TestAccComputeForwardingRule_update|TestAccComputeInstanceFromMachineImage_basic|TestAccComputeInstanceFromMachineImage_overrideMetadataDotStartupScript|TestAccComputeInstanceFromMachineImage_diffProject|TestAccComputeOrganizationSecurityPolicyAssociation_organizationSecurityPolicyAssociationBasicExample|TestAccComputeOrganizationSecurityPolicy_organizationSecurityPolicyUpdateExample|TestAccComputeOrganizationSecurityPolicyRule_organizationSecurityPolicyRuleUpdateExample|TestAccComputeRegionNetworkEndpointGroup_regionNetworkEndpointGroupAppengineExample|TestAccComputeRouterPeer_bfd|TestAccContainerAnalysisOccurrence_multipleSignatures|TestAccContainerNodePool_ephemeralStorageConfig|TestAccServiceNetworkingPeeredDNSDomain_basic|TestAccSqlUser_postgresIAM You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=213519

rileykarson · 2021-10-28T22:39:47Z

Hey! Just noting I've seen this and it's in my inbox as a task, I'll get to it within a couple of days.

modular-magician · 2021-10-28T23:19:11Z

Tests failed during RECORDING mode: TestAccComputeOrganizationSecurityPolicyRule_organizationSecurityPolicyRuleUpdateExample|TestAccCloudbuildWorkerPool_withNetwork|TestAccContainerAnalysisOccurrence_multipleSignatures|TestAccComputeFirewallPolicyRule_update|TestAccContainerNodePool_ephemeralStorageConfig|TestAccComputeInstanceFromMachineImage_basic|TestAccComputeInstanceFromMachineImage_overrideMetadataDotStartupScript|TestAccComputeRegionNetworkEndpointGroup_regionNetworkEndpointGroupAppengineExample|TestAccCloudFunctionsFunction_vpcConnector|TestAccComputeInstanceFromMachineImage_diffProject|TestAccSqlUser_postgresIAM Please fix these to complete your PR

rileykarson · 2021-11-02T22:52:34Z

mmv1/third_party/terraform/tests/resource_compute_router_bgp_peer_test.go

@@ -675,3 +718,100 @@ resource "google_compute_router_peer" "foobar" {
 }
 `, routerName, routerName, routerName, routerName, routerName, routerName, routerName, routerName, routerName, routerName, routerName, enable)
 }
+
+func testAccComputeRouterPeerBfd(routerName, bfdMode string) string {


What stops us from making this the example? It's a lot of config but if that's required to use the feature, I don't see a problem documenting it! See https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/compute_service_attachment#example-usage---service-attachment-basic for example

BFD doesn't make much sense for VPN right now as it can only be DISABLED. A more realistic example is to use it with interconnect but that can't be tested due to the APIs. I'm OK making this a proper example once BFD support for VPN is added, though.

SGTM, thanks!

drebes added the enhancement label Mar 17, 2020

drebes requested a review from danawillow March 17, 2020 08:07

googlebot added the cla: yes label Mar 17, 2020

danawillow suggested changes Mar 17, 2020

View reviewed changes

products/compute/api.yaml Outdated Show resolved Hide resolved

products/compute/api.yaml Outdated Show resolved Hide resolved

products/compute/api.yaml Outdated Show resolved Hide resolved

danawillow suggested changes Mar 17, 2020

View reviewed changes

drebes changed the title ~~RouterBgpPeer new attributes~~ [do_not_merge] RouterBgpPeer new attributes Mar 18, 2020

drebes mentioned this pull request Aug 9, 2021

enable/disable for BgpPeer #5063

Merged

5 tasks

This was referenced Aug 11, 2021

enable/disable for BgpPeer hashicorp/terraform-provider-google-beta#3507

Merged

enable/disable for BgpPeer hashicorp/terraform-provider-google#9776

Merged

BFD for Cloud Router

b6a93e5

drebes force-pushed the bfd branch from cfc28e0 to b6a93e5 Compare October 28, 2021 07:09

drebes changed the title ~~[do_not_merge] RouterBgpPeer new attributes~~ RouterBgpPeer BFD support Oct 28, 2021

drebes requested review from danawillow and rileykarson and removed request for danawillow October 28, 2021 07:13

Add integration test

6b426bd

rileykarson reviewed Nov 2, 2021

View reviewed changes

rileykarson approved these changes Nov 3, 2021

View reviewed changes

rileykarson merged commit 1451330 into GoogleCloudPlatform:master Nov 3, 2021

This was referenced Nov 3, 2021

RouterBgpPeer BFD support hashicorp/terraform-provider-google#10487

Merged

RouterBgpPeer BFD support hashicorp/terraform-provider-google-beta#3822

Merged

drebes mentioned this pull request Dec 3, 2021

gcloud_compute_routers BgpPeer new options hashicorp/terraform-provider-google#10206

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RouterBgpPeer BFD support #3258

RouterBgpPeer BFD support #3258

drebes commented Mar 17, 2020 •

edited

Loading

modular-magician commented Mar 17, 2020

drebes commented Mar 17, 2020

modular-magician commented Mar 17, 2020

danawillow left a comment

danawillow Mar 17, 2020

drebes Mar 18, 2020

modular-magician commented Mar 18, 2020

modular-magician commented Mar 18, 2020

drebes commented Mar 18, 2020

danawillow commented Jun 10, 2020

drebes commented Jun 10, 2020

drebes commented Oct 28, 2021

modular-magician commented Oct 28, 2021

drebes commented Oct 28, 2021

modular-magician commented Oct 28, 2021

modular-magician commented Oct 28, 2021

rileykarson commented Oct 28, 2021

modular-magician commented Oct 28, 2021

rileykarson Nov 2, 2021

drebes Nov 3, 2021

rileykarson Nov 3, 2021

RouterBgpPeer BFD support #3258

RouterBgpPeer BFD support #3258

Conversation

drebes commented Mar 17, 2020 • edited Loading

modular-magician commented Mar 17, 2020

Diff report:

drebes commented Mar 17, 2020

modular-magician commented Mar 17, 2020

Diff report:

danawillow left a comment

Choose a reason for hiding this comment

danawillow Mar 17, 2020

Choose a reason for hiding this comment

drebes Mar 18, 2020

Choose a reason for hiding this comment

modular-magician commented Mar 18, 2020

Diff report:

modular-magician commented Mar 18, 2020

Diff report:

drebes commented Mar 18, 2020

danawillow commented Jun 10, 2020

drebes commented Jun 10, 2020

drebes commented Oct 28, 2021

modular-magician commented Oct 28, 2021

Diff report:

drebes commented Oct 28, 2021

modular-magician commented Oct 28, 2021

Diff report:

modular-magician commented Oct 28, 2021

rileykarson commented Oct 28, 2021

modular-magician commented Oct 28, 2021

rileykarson Nov 2, 2021

Choose a reason for hiding this comment

drebes Nov 3, 2021

Choose a reason for hiding this comment

rileykarson Nov 3, 2021

Choose a reason for hiding this comment

drebes commented Mar 17, 2020 •

edited

Loading