Feature subnet #156

minsikl · 2017-06-20T02:46:24Z

softlayer_subnet resource supports portable subnet and static subnet management.
softlayer_subnet.md, resource_softlayer_subnet.go, and resource_softlayer_subnet_test.go files are added to implement softlayer_subnet resource. This PR will solve the issue #147

renier · 2017-06-22T20:02:42Z

docs/resources/softlayer_subnet.md

+```hcl
+# Create a new portable subnet
+resource "softlayer_subnet" "portable_subnet" {
+  type = "Portable"


if there can only be two types, portable or static, make it a boolean then portable = true or static = true to avoid possible spelling errors. If it is not portable, then it is static and vice-versa.

SoftLayer internally uses some subnet types: PRIMARY, ADDITIONAL_PRIMARY, SECONDARY, ROUTED_TO_VLAN, SECONDARY_ON_VLAN, STORAGE_NETWORK, and STATIC_IP_ROUTED. From an end user perspective, the types can be simplified to: Primary, Static, Portable, and Global. We can ignore Global type because softlayer_global_ip is already provided. As of now, softlayer_subnet only supports Static and Portable because Primary subnet cannot be ordered manually. However, I think that Primary subnet ordering will be supported in the future. Then, we can extend this resource without schema update.

softlayer_subnet data source also can be added later. Unlike softlayer_subnet resource, the data source may supports Primary, Portable, Static. In this case, we can use the same attribute name type.

@minsikl ok, in that case, why not use the same type strings as the SL API? What would static map to? STATIC_IP_ROUTED? What about portable?

The point is, if we can't make it fool-proof with booleans because there could be more than two types, then let's go with the raw API type names and then we can point there for the list of allowed types and spelling.

@renier The initial code used ROUTED_TO_VLAN for portable subnets and STATIC_IP_ROUTED for static subnets. The subnet type can be retrieved using SoftLayer_Network_Subnet ::subnetType. However, I realized that SL API returns additional types such as SECONDARY_ON_VLAN, SUBNET_ON_VLAN, and STATIC_IP_ROUTED. After IPv6 feature is added, I got additional types such as STATIC_IP6_ROUTED. The raw subnet type is not an input parameter of a create function and I'm not able to guess which type will be returned from SL API. In end-user perspective, these raw types are meaningless and SL Portal uses terminology Portable, Static.

Do we go with the portal or with the API? is the question.

If we go with the portal names, it would be good to at least document what that means in API types.

The raw subnet type is not an input parameter of a create function and I'm not able to guess which type will be returned from SL API.

What kinds of subnets can you create then? and how do you specify the kind upon creation? (over the API)

However, the user could just provide the required resource attribute without specifying the type,

I explained that primary subnet and portable subnet has the same attributes. How will you distinguish the primary subnet if you don't have type attribute?

primary = true/false

primary = true is okay. But, what is primary = false? Is that mean that the subnet will be global, secondary, third, or portable? Will you add an attribute per types such as static = true and secondary = true? primary is one of subnet types. If you use type attribute, you don't have to add additional true/false attributes to support different types. Why do you use true/false for type names?

However, the user could just provide the required resource attribute without specifying the type,

I explained that primary subnet and portable subnet has the same attributes. How will you distinguish the primary subnet if you don't have type attribute?

Can a user order a primary subnet today? If not, then I stand by that we should not add a required pseudo-attribute based on something that has not happened in the API yet. We don't have enough information on how to handle until it actually does. Does this make sense?

Is it that we know that orderable primary subnets is in the works and we expect that to be available any moment now? If so, do we know exactly how the ordering of a primary subnet will look like?

@renier softlayer_vlan already provides a function to create the first primary subnet. When you create a new VLAN using terraform, you should define the size of the first primary subnet. Then, placeOrder in softlayer_vlan resource sends priceId of VLAN and priceId of the primary subnet and creates a new VLAN with a primary subnet. So, primary subnets ordering requires same information with portable subnets ordering.

Subnet types are not pseudo-attributes. price item key names contain them, SL portal, automatic tickets, and knowledgelayer use them. Only SL API provides it's internal subnet types.

Boolean attributes should be only used when SL API provides boolean properties or the attributes are optional values such as advanced monitering or redundant power supply. Or they will be pseudo-attributes, and it's hard to understand the meaning.

I defined a general softlayer_subnet resource and it is a pseudo resource. That's why the type attribute is important in softlayer_subnet resource. type is added to define a real resource type of subnets. In the later, if a new attribute is added to describe a new subnet type such as primary=true/false, primary and type will provide duplicated information, and I don't think that it is a good design. Current softlayer_subnet only supports parts of SL subnet types. At least, we need to provide extensible schema if we want to use softlayer_subnet.

If you don't want to use type attribute, let's just define separate resources softlayer_portable_subnet and softlayer_static_subnet. There is no advantage to using softlayer_subnet and if softlayer_subnet is provided without type, it will just make confusion.

softlayer_vlan already provides a function to create the first primary subnet.

That's good to know and understand. I had not made the link between the vlan and the primary subnet. Why were you thinking that primary could be an additional type to softlayer_subnet, when it is already being provided by softlayer_vlan?

Subnet types are not pseudo-attributes. price item key names contain them, SL portal, automatic tickets, and knowledgelayer use them. Only SL API provides it's internal subnet types.
Boolean attributes should be only used when SL API provides boolean properties or the attributes are optional values such as advanced monitering or redundant power supply. Or they will be pseudo-attributes, and it's hard to understand the meaning.

In this case, the type is an attribute that is not synced with the API and is only managed locally. Thus, it is a pseudo-attribute of the resource by definition. Just as any boolean flags in other resources that are also only managed locally. The most common pseudo-attributes are booleans, because you usually don't need to make those kinds of attributes more complex and should not if possible.

I defined a general softlayer_subnet resource and it is a pseudo resource.

Based on my definition above, a pseudo-resource would be something that is not sync-ed with the cloud and it would be managed completely locally. For example, the random provider in terraform only has pseudo-resources. It follows that softlayer_subnet is not a pseudo-resource.

If you don't want to use type attribute, let's just define separate resources softlayer_portable_subnet and softlayer_static_subnet. There is no advantage to using softlayer_subnet and if softlayer_subnet is provided without type, it will just make confusion.

It doesn't have to be confusing if we document that portable subnets are created when you fill out the vlan_id, otherwise if you provide the endpoint_ip, it is static. Even less if you provide a documented computed attribute to the resource computing this fact for them as a word (portable or static).

Creating multiple resources, one per subnet type, is also an option, as long as 99% of the code is shared among them.

That's good to know and understand.

I gave you the primary subnet provisioning example as an answer to your question: Can a user order a primary subnet today? If not, then I stand by that we should not add a required pseudo-attribute based on something that has not happened in the API yet. We don't have enough information on how to handle until it actually does.. You can see that primary subnet requires same attributes with portable subnet when you create a new primary subnet.

Why were you thinking that primary could be an additional type to softlayer_subnet, when it is already being provided by softlayer_vlan?

If you get a chance to deploy middle/large size systems or some cluster solutions on SoftLayer, you will find several primary subnet requirements which are not provided in SoftLayer. You can only create the first primary subnet as I explained above. You are not able to create 2nd/3rd primary subnets with a specific CIDR. Some cluster solutions require specific CIDR and some users want to reserve primary subnets in their VLAN for their firewall rules or applications.

Based on my definition above, a pseudo-resource would be something that is not sync-ed with the cloud and it would be managed completely locally. For example, the random provider in terraform only has pseudo-resources. It follows that softlayer_subnet is not a pseudo-resource.

primary subnet, portable subnet, static subnet, and global subnet are combinations of interface and routing commands on physical routers and switches. subnet is a general network terminology which provides IP range and CIDR information. But, primary subnet, portable subnet, static subnet, and global subnet are not general terminologies and only used in SoftLayer. They are offering names and you can create and manage them on SoftLayer. subnet is used to refer to these soft layer offerings, not the actual resource name.

It doesn't have to be confusing if we document that portable subnets are created when you fill out the vlan_id, otherwise if you provide the endpoint_ip, it is static. Even less if you provide a documented computed attribute to the resource computing this fact for them as a word (portable or static).

Documents must, of course, be provided. Nevertheless, if there is a better way to define a resource, that method should be used.
Indirect resource definition methods also affect tf file readability. In a tf file where dozens of subnets are defined, people have to extract the subnet type by identifying the manual, deducing the subnet type, or creating a separate script.

Creating multiple resources, one per subnet type, is also an option, as long as 99% of the code is shared among them.

I'll create create two different subnets softlayer_subnet_portable and softlayer_subnet_static.

renier · 2017-06-22T20:03:40Z

docs/resources/softlayer_subnet.md

+# Create a new portable subnet
+resource "softlayer_subnet" "portable_subnet" {
+  type = "Portable"
+  network = "PRIVATE"


same with the type of network. if there can only be two types, public and private, then make it a boolean private = true.

renier · 2017-06-22T20:06:55Z

softlayer/resource_softlayer_subnet.go

+			},
+
+			// IP version 4 or IP version 6
+			"ip_version": {


could we have a default of 4 for the ip version?

renier · 2017-06-22T20:09:10Z

softlayer/resource_softlayer_subnet.go

+			},
+
+			// Provides IP address/netmask format (ex. 10.10.10.10/28)
+			"subnet": &schema.Schema{


Do we have to validate the format here? Also, the resource is called subnet, so having a property also called the same can mislead the user. Can we calle it cidr or cidr_mask?

subnet is a computed attribute. So, validation is not necessary. In SoftLayer Portal, that column name is subnet and VLAN resource uses subnet as an attribute name of IP address/cidr format. I can change the attribute name to more user-friendly name. I think that cidr can be used as a name of this attribute. But, SoftLayer API uses this field name to represent cidr block(ex. /25). How about ip_cidr or ip_address_cidr?

I see. If it is computed and SoftLayer portal calls it subnet, how about subnet_cidr?

virtual_guest, bare_metal, lb_vpx, and vlan uses the name subnet or public/private_sunbet for this cidr notation. For consistency, I would like to use the name subnet. However subnet_cidr is also good.

Yes, it should somehow say it is a cidr in the property. Explaining it as "a subnet resource has a subnet property" is going to be a problem. For the other resources, that problem does not exist. Consistency with the other resources does not seem to be necessary for this.

renier · 2017-06-22T20:10:36Z

softlayer/resource_softlayer_subnet.go

+	d.Set("subnet", *subnet.NetworkIdentifier+"/"+strconv.Itoa(*subnet.Cidr))
+	if subnet.Note != nil {
+		d.Set("notes", *subnet.Note)
+	}


Not necessary, but these blocks could be refactored with d.Set("notes", sl.Get(subnet.Note, nil))

renier · 2017-06-26T13:50:14Z

softlayer/resource_softlayer_subnet.go

+		d.Set("type", "Static")
+	} else if strings.Contains(*subnet.SubnetType, "VLAN") {
+		d.Set("type", "Portable")
+	}


@minsikl There are SubnetTypes that would not contain STATIC or VLAN. In that case "type" would be unset. Is that intentional?

@renier Portable subnets contain "VLAN" and Static subnets contain "STATIC". In the future, additional types can be added. For example, Primary subnets contain "PRIMARY".

@minsikl Since there are some types that do not contain either word, technically, would it would be possible to get that from the API? If so, programmatically, it would be safer to add an else clause to handle that situation. For example, to throw an error in that case, since we don't support any other type of subnet.

@renier Users may try to import different type of subnets. The current code will not generate an error. If you want to check the type strictly, you can throw an exception.

Updated the code to generate an error when an invalid subnet type is detected.

minsikl added 8 commits June 15, 2017 12:23

Initial implementation of subnet resource.

86b1cc0

Updated attribute names for subnet.

936a6fc

Changed network to optional attribute.

2cd5637

Added subnet_test.

d954fbb

Added doc for subnet resource.

54ea415

Added IPv6 support for subnet resource.

0c5ff3c

Fixed capacity information for ipv6 on subnet resource.

cfc94e7

Checked activeTransaction for subnet provisioning.

894f6b9

minsikl added the feature label Jun 20, 2017

minsikl requested a review from renier June 20, 2017 02:46

renier requested changes Jun 22, 2017

View reviewed changes

minsikl added 2 commits June 24, 2017 19:07

Updated attributes for softlayer_subnet.

c470697

Updated the doc for subnet.

fb09e1e

renier reviewed Jun 26, 2017

View reviewed changes

minsikl added 2 commits June 27, 2017 10:59

Updated the name of subnet_cidr attribute.

da3490a

Checked the subnet type strictly.

bcb7e55

minsikl closed this Jul 17, 2017

minsikl deleted the feature-subnet branch July 17, 2017 18:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature subnet #156

Feature subnet #156

minsikl commented Jun 20, 2017

renier Jun 22, 2017

minsikl Jun 22, 2017

renier Jun 23, 2017

minsikl Jun 23, 2017

renier Jun 23, 2017 •

edited

Loading

minsikl Jun 27, 2017

renier Jun 29, 2017 •

edited

Loading

minsikl Jun 30, 2017

renier Jun 30, 2017 •

edited

Loading

minsikl Jul 5, 2017

renier Jun 22, 2017

minsikl Jun 24, 2017

renier Jun 22, 2017

minsikl Jun 24, 2017

renier Jun 22, 2017

minsikl Jun 22, 2017

renier Jun 23, 2017

minsikl Jun 23, 2017

renier Jun 23, 2017

minsikl Jun 25, 2017

renier Jun 22, 2017

minsikl Jun 24, 2017

renier Jun 26, 2017

minsikl Jun 26, 2017

renier Jun 26, 2017

minsikl Jun 27, 2017

minsikl Jun 27, 2017

Feature subnet #156

Feature subnet #156

Conversation

minsikl commented Jun 20, 2017

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

renier Jun 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

renier Jun 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

renier Jun 30, 2017 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

renier Jun 23, 2017 •

edited

Loading

renier Jun 29, 2017 •

edited

Loading

renier Jun 30, 2017 •

edited

Loading