Support for network types per bond, and for the new hybrid-bonded type

[t0mk]

Fixes #231

@displague will probably not be happy about the format of this PR, but I think this is the best way to add support for network type per bond (like it's in the UI), and for the hybrid-bonded network type.

I initally wanted to create a separate netwokr type transitions for the hybrid-bonded, but I realized it's very hard to do well. The hybrid-mixed type is reported when we assign a VLAN to a layer3 bond port of a device (of certain plans). The assigned VLAN is the only difference between layer3 and hybrid-bonded types. I think it's best to merge those two types, and leave the VLAN assignment up to the user.

If we assume a VLAN ID as a part of the network type hybrid-bonded, we would need to keep track of the assigned VLAN, and remove it once it's converted to some other network type. Problem is, that user can assign more vlans to the bond port later on. This is not hard to sanitize in the Golang SDK, but it would be disturbing for new terraform resource device_bond_network_type. If a user will assign VLANs explicitly in other resources, the device_bond_network_type will not be importable, because we can't say which was the VLAN that put it to the hybrid-bonded type. We will also need to store the inital VLAN id locally.

If we (sort of) merge the l3 and h-bonded types, we can achieve all that is possible in the UI, and we have a good chance to make the bond network state controllable in Terraform.

I put the new code intentionaly to new files, so that it can be reviewed easier.

Putting a device to hybrid-mixed network type is illustrated in TestAccPortBondNetworkStateHybridBonded. You should run the test in some faciltiy where c3.small.x86 is available, e.g.

PACKNGO_TEST_FACILITY=ny5 PACKNGO_DEBUG=1 PACKNGO_TEST_ACTUAL_API=1 go test -v -timeout=20m -run=TestAccPortBondNetworkStateHybridBonded

[t0mk]

To illustrate how we could do hybrid-bonded in TF:

// substitute "packet" for "metal"
locals {
    project_id = "<uuid>"
}

resource "packet_device" "test" {
  hostname    = "test"
  plan             = "c1.small.x86"
  facilities       = ["ny5"]
  operating_system = "ubuntu_16_04"
  billing_cycle    = "hourly"
  project_id       = local.project_id
}

resource "packet_vlan" "test" {
  facility    = "ny5"
  project_id  = local.project_id
}

resource "packet_port_vlan_attachment" "test" {
  count     = local.device_count
  device_id = packet_device_network_type.test.id
  port_name = "bond0"
  vlan_vnid = packet_vlan.test.vxlan
}

resource "packet_device_bond_network_type" "test" {
  depends_on = [packet_port_vlan_attachment.test]
  device_id = packet_device.test.id
  bond_port = "bond0"
  type           = "hybrid-bonded"  
}

.. there would be a diffSuppress making type hybrid-bonded and layer3 the same. The TF configuration would be a bit different from the web console, but enough documentation would clear it up I think.

[displague]

same type transitions should not trigger an error

[t0mk]

OK!

[displague]

This function builds to the list of helpers in #230

[displague]

I expect we would refactor ConvertDevice to call this function for each Bond.

[t0mk]

So you would prefer to still convert network type per device? Instead of to convert bonds?

I just noticed that it's not poss to put bond1 (in n2.xlarge) to L3 and hybrid type, and it reminded of a message you sent me a week ago:

I don’t think we are able to do L3 for bond1. And we were discussing to limit bond1 capabilities to only L2 bonded and unbonded.
Only bond0 should be used for L3 capabilities.

Since the configuration for n2.xlarge is so inflexible (or special), we could treat it as a special case and create a separate function for it. Also we could create a separate tf resource.

If we treat n2 as a special case, maybe we can rollback to use network type for a device (instead of for a bond as this PR suggests). I'm sorry I pushed for the network-type per bond, I thought it's the most versatile way to control the network type.

I don't understand why the bonds are now displayed and controlled separately in the web console, when the 2 bonds in n2.xlarge are so inflexible. Anyway, I figured what network types are possible for bonds of n2.xlarge.

• bond0: L3, L2-bonded, L2-unbonded (no hybrid or hybrid-bonded)
• bond1: L2-bonded, L2-unbonded (no L3, hybrid, hybrid-bonded)

Considering all this, I think we should define possible (useful) configurations for the n2.xlarge, and just treat it differently. I don't think that we should map the n2.xlarge confs to the one-bond types.

[displague]

So you would prefer to still convert network type per device? Instead of to convert bonds?

The thought here is that users (Terraform) will prefer ConvertDeviceBond. For backward compatibility, ConvertDevice can leverage ConvertDeviceBond to do the work. We can deprecate it independently of the refactor.

If we keep ConvertDevice it would have to result in the bonds and ports being in the same state they used to.
It may be easier to deprecate it (or remove it now).

What do you think?

Since the configuration for n2.xlarge is so inflexible (or special), we could treat it as a special case and create a separate function for it. Also we could create a separate tf resource.

I don't think users should have to choose different resource type based on device plan. I do think that they should consult documentation and use/choose attribute values that apply to the bond that they are configuring.

We run into problems when we try to bake logic into Terraform and Packngo to do more than what the API strictly offers.

If we remove the magic of device port name lookups and network type translation, a user could have a more powerful interface that shouldn't need much revisiting in the future.

resource "metal_port" "server-bond0" {
  port_id = metal_device.foo.network_ports.bond0.id
  bonded = true
  layer2 = false // this is default and represents either layer2 or layer3, leads to /port/id/convert-layer-X call on diff
  // network_type = "hybrid-bonded" // computed, read-only
  virtual_networks = [metal_vlan.foo.id]
  native_virtual_network = metal_vlan.native.id
  ip_address {
    type = "private_ipv4"
    cidr = 30
    // reservation_ids
    // cidr_notation from metal_ip_attachment
  }
}

resource "metal_port" "server-bond1" {
  port_id = metal_device.foo.network_ports.bond1.id
  bonded = true
  layer2 = true // representing the desired /port/id/convert-layer-X state
  // network_type = "layer2-bonded" // computed, read-only
  virtual_networks = [metal_vlan.foo.id]
}

The default of layer2=false would mean that layer3 is applied, which is the API default and is the most permissive (allowing for both IP addresses and VLANs in IBX facilities, thanks to hybrid-bonded).

The API endpoint has a hyphen (.../convert/layer-2, .../convert/layer-3), the facility Layer2 feature uses underscore (layer_2), and the network_type returned by bond devices uses no hyphens (layer3). I think we can follow the network_type naming pattern for this field without much confusion.

If the user sets invalid combinations, they will receive an API error and can adjust accordingly. Terraform doesn't have to validate the user input and does not need to make assumptions about what values can be used.

We can try adding validation, but I fear we will run into more problems than we avoid by doing so.

Considering all this, I think we should define possible (useful) configurations for the n2.xlarge, and just treat it differently. I don't think that we should map the n2.xlarge confs to the one-bond types.

I can add that in the future we may have 8 port resources, for example, that are neither "metal_device" nor "metal_hardware_reservation" resources. We'll benefit from treating ports as independent resources.

[t0mk]

The thought here is that users (Terraform) will prefer ConvertDeviceBond. For backward compatibility, ConvertDevice can leverage ConvertDeviceBond to do the work. We can deprecate it independently of the refactor.

sound good

If we remove the magic of device port name lookups and network type translation, a user could have a more powerful interface that shouldn't need much revisiting in the future.

powerful indeed, but not simple. A lot of docs will be needed to map it to the states from the UI. But it's probably a way to go if there are more resources with ports coming.

The API endpoint has a hyphen (.../convert/layer-2, .../convert/layer-3), the facility Layer2 feature uses underscore (layer_2), and the network_type returned by bond devices uses no hyphens (layer3). I think we can follow the network_type naming pattern for this field without much confusion.

Yes, "layer{2,3}" for naming is ok. Just it's not straightforward to find out the current l2/l3 state (for the sake of the /convert/ calls) from the network_type attr, but it can be done. I think it's

• layer-3: layer3, hybrid, hybrid-bonded
• layer-2: network type can be layer2-individual, layer2-bonded

This is sth that's not in the UI.