Resource and data source `vcd_edgegateway` add network support for multiple subnets, sub-allocated pools and rate limits #401

Didainius · 2019-11-19T16:22:59Z

This PR closes #323, closes #397

This is a list of things it brings:

Deprecates existing external_networks list of networks in favor of new external_network block which allows to specify one or more subnets inside the network and suballocate IPs from those ranges. Also deprecates default_gateway_network field. Note: as far as I tested all external_network block features work with simple edge gateway as well
Adds additional external_network_ips computed field to get all IP addresses set on uplink gateway interfaces (in addition to default IP in default_external_network_ip field)
Adds additional test TestAccVcdEdgeGatewayParallelCreation which aims to catch a reported error when two edge gateways are being attached to the same external network at the same time (so far I did not reproduce the error)
Removed a limitation where field distributed_routing was not read due to older API version not returning it.
Adds new field fips_mode_enabled to enable FIPS mode (it must be enabled by vCD administrator in general settings to make it work, otherwise returns a human readable error). Note FIPS mode field is not being returned from API therefore it does not support read
Adds support for rate limiting per "external network"

Note: It does not yet support update.

Important. There is one problem with data sources which should be solved by this PR hashicorp/terraform-plugin-sdk#197 but is not yet done. The problem is that when a TypeSet contains only computed variables and there are multiple blocks, only one is stored. This situation is documented and tested in existing TestAccVcdEdgeGatewayExternalNetworks test

lvirbalas

First comments. This one is big ;)

CHANGELOG.md

lvirbalas · 2019-11-27T12:29:33Z

vcd/datasource_vcd_edgegateway.go

+				Description: "If true, default gateway will be used for the edge gateways' default routing and DNS forwarding.(False by default)",
+			},
+			"external_network": {
+				Type:        schema.TypeSet,


Please confirm whether using TypeSet leads to the issue where changing (updating) value of any field in HCL shows the whole external_network block as being deleted and re-created during a terraform plan.

Yes, this will show it. Something like that:

A remotely related issue at HashiCorp for reference:
hashicorp/terraform#21901

lvirbalas · 2019-11-27T12:30:11Z

vcd/datasource_vcd_edgegateway.go

+						"incoming_rate_limit": {
+							Computed:    true,
+							Type:        schema.TypeFloat,
+							Description: "Incoming rate limit (Mbps)",


Please confirm that Mbps is what API is using (just want to make sure we avoid nasty conversion issues).

Yes. And funny enough it works properly with float values (included in tests as well):

We have case where API and UI don't match Types.

Yep, but these numbers in screenshot were created using provider, not entered manually.

vcd/datasource_vcd_edgegateway.go

lvirbalas

Documentation requests.

website/docs/r/edgegateway.html.markdown

lvirbalas · 2019-11-28T09:09:21Z

website/docs/r/edgegateway.html.markdown

 * `configuration` - (Required) Configuration of the vShield edge VM for this gateway. One of: `compact`, `full` ("Large"), `x-large`, `full4` ("Quad Large").
-* `default_gateway_network` - (Optional) Name of the external network to be used as default gateway. It must be included in the
+* `default_gateway_network` - (Deprecated, Optional) Name of the external network to be used as default gateway. It must be included in the


Which field replaces this deprecated one?
In general, it feels we should add a sentence to all deprecated fields saying "Please use ... instead" to avoid putting the user into detective clothes.

Added pointers - please check if it makes sense to have them at the end of description or I should move the new field descriptions to the front.

dataclouder

LGTM

vbauzys

Overall complicated PR. Can't do good review do that code is complicated and have to know structures, changes, overall understand inside things. Also SDK upgrade should be as separate PR.

vbauzys · 2019-11-28T14:25:28Z

vcd/datasource_vcd_edgegateway.go

+						"incoming_rate_limit": {
+							Computed:    true,
+							Type:        schema.TypeFloat,
+							Description: "Incoming rate limit (Mbps)",


We have case where API and UI don't match Types.

vbauzys · 2019-11-28T14:31:35Z

vcd/resource_vcd_edgegateway.go

+// 	<OutRateLimit>100</OutRateLimit>
+// 	<UseForDefaultRoute>true</UseForDefaultRoute>
+// </GatewayInterface>
+func getGatewayInterfacesType(vcdClient *VCDClient, externalInterfaceSet *schema.Set) ([]*types.GatewayInterface, error) {


Can we divide this function with meaningful names to make self documented code?

I split it, but IMHO the names are already meaningful as they can be with this API structure for interfaces. To me now it is even harder to follow it, but you can check if it is more readable for fresh eyes.

lvirbalas

One last doc polish in-line and LGTM. Thanks for a big addition!

lvirbalas · 2019-12-03T10:50:35Z

website/docs/r/edgegateway.html.markdown

+  default gateway. It must be included in the list of `external_networks`. Providing an empty string
+  or omitting the argument will create the edge gateway without a default gateway. **Please use**
+  the  [external network](#external-network) block structure and `use_for_default_route` to specify
+  a subnet should be used as default route.


Suggested change

a subnet should be used as default route.

a subnet which should be used as a default route.

vbauzys · 2019-12-03T11:32:07Z

Don't forget to solve conflicts

Didainius added 18 commits November 13, 2019 15:45

WIP

9295238

WIP

96af345

Tests getting close

0e8fe0d

All tests pass

0c4c32d

empty tests pass

e59e656

merge master

4419cee

fix govcd bump

3b801ab

rename function

9d364a0

IP_ADDRES does not work

fa09729

Export all IPs in computed 'external_network_ips' field

e0f0cc1

docs and tests

9214164

Merge master

0fd8f72

Tests pass, github issue marked

0303937

include govcd pr

29b7aa8

Added changelog notes, parallel test to catch failures

f356ad8

Upgrade SDK to 1.3.0, pull in later govcd, add more tests

dc58e9e

Remove legacy syntax checks

2a8ee46

cleanup tests and comments

30ad647

ghost added size/XXL dependencies documentation labels Nov 19, 2019

Didainius added 2 commits November 19, 2019 18:28

go mod vendor && tidy

861469b

Add correct github issue to changelog

31716df

Didainius mentioned this pull request Nov 19, 2019

Additional edge gateway external network features: multiple external networks, manual IP assignment, IP pool sub-allocation #323

Closed

Didainius self-assigned this Nov 19, 2019

Didainius changed the title ~~Resource and data source vcd_edgegateway add network support for multiple subnets and sub-allocated pools~~ Resource and data source vcd_edgegateway add network support for multiple subnets, sub-allocated pools and rate limits Nov 20, 2019

Remove old syntax fox tf configs

d96f123

Didainius marked this pull request as ready for review November 20, 2019 13:44

Didainius requested review from dataclouder and lvirbalas November 20, 2019 13:46

Didainius requested a review from vbauzys November 20, 2019 13:47

Didainius added 4 commits November 20, 2019 17:14

Add deprecation info to fields in data source

45c8a33

Do not enable distributed routing during tests

dc2b6e2

Merge master

9b46264

go mod tidy

baaba7f

lvirbalas requested changes Nov 27, 2019

View reviewed changes

Address changelog notes

271bc3f

lvirbalas requested changes Nov 28, 2019

View reviewed changes

dataclouder approved these changes Nov 28, 2019

View reviewed changes

vbauzys suggested changes Nov 28, 2019

View reviewed changes

Didainius added 4 commits November 29, 2019 08:23

merge master

6ffac7a

bump govcd

4ade9d5

Address documentation review

dced79d

Split functions

cee3fd8

lvirbalas approved these changes Dec 3, 2019

View reviewed changes

vbauzys approved these changes Dec 3, 2019

View reviewed changes

Didainius added 2 commits December 4, 2019 07:07

Merge master

8819b76

Address comment in docs

e64db20

Didainius merged commit 519ca43 into vmware:master Dec 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resource and data source `vcd_edgegateway` add network support for multiple subnets, sub-allocated pools and rate limits #401

Resource and data source `vcd_edgegateway` add network support for multiple subnets, sub-allocated pools and rate limits #401

Didainius commented Nov 19, 2019 •

edited

Loading

lvirbalas left a comment

lvirbalas Nov 27, 2019

Didainius Nov 27, 2019

lvirbalas Nov 28, 2019

lvirbalas Nov 27, 2019

Didainius Nov 27, 2019

vbauzys Nov 28, 2019

Didainius Nov 29, 2019

lvirbalas left a comment

lvirbalas Nov 28, 2019

Didainius Nov 29, 2019

dataclouder left a comment

vbauzys left a comment

vbauzys Nov 28, 2019

vbauzys Nov 28, 2019

Didainius Nov 29, 2019

lvirbalas left a comment

lvirbalas Dec 3, 2019

vbauzys commented Dec 3, 2019

	a subnet should be used as default route.
	a subnet which should be used as a default route.

Resource and data source vcd_edgegateway add network support for multiple subnets, sub-allocated pools and rate limits #401

Resource and data source vcd_edgegateway add network support for multiple subnets, sub-allocated pools and rate limits #401

Conversation

Didainius commented Nov 19, 2019 • edited Loading

lvirbalas 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

Choose a reason for hiding this comment

lvirbalas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dataclouder left a comment

Choose a reason for hiding this comment

vbauzys 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

lvirbalas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbauzys commented Dec 3, 2019

Resource and data source `vcd_edgegateway` add network support for multiple subnets, sub-allocated pools and rate limits #401

Resource and data source `vcd_edgegateway` add network support for multiple subnets, sub-allocated pools and rate limits #401

Didainius commented Nov 19, 2019 •

edited

Loading