Expand cloudstack_loadbalancer_rule to work without vpcs

When using load balancer rules on an IP associated with a network
instead of a vpc, the network field can be omitted and inferred from the
IP. Filling this into state on read causes a spurious diff.

The openfirewall flag defaults to true when used on a network IP.
Implicit resource creation doesn't fit the terraform model, so we
disable it.

Also added a test which shows arguments that can be changed without
creating a new resource.

builtin/providers/cloudstack/resource_cloudstack_loadbalancer.go

@@ -79,6 +79,9 @@ func resourceCloudStackLoadBalancerRuleCreate(d *schema.ResourceData, meta inter
 		d.Get("public_port").(int),
 	)
 
+	// Don't autocreate a firewall rule, use a resource if needed
+	p.SetOpenfirewall(false)
+
 	// Set the description
 	if description, ok := d.GetOk("description"); ok {
 		p.SetDescription(description.(string))
@@ -160,14 +163,16 @@ func resourceCloudStackLoadBalancerRuleRead(d *schema.ResourceData, meta interfa
 	d.Set("public_port", lb.Publicport)
 	d.Set("private_port", lb.Privateport)
 
-	// Get the network details
+	setValueOrUUID(d, "ipaddress", lb.Publicip, lb.Publicipid)
+
+	// Only set network if user specified it to avoid spurious diffs
+	if _, ok := d.GetOk("network"); ok {
 		network, _, err := cs.Network.GetNetworkByID(lb.Networkid)
 		if err != nil {
 			return err
 		}
-
-	setValueOrUUID(d, "ipaddress", lb.Publicip, lb.Publicipid)
 		setValueOrUUID(d, "network", network.Name, lb.Networkid)
+	}
 
 	return nil
 }

builtin/providers/cloudstack/resource_cloudstack_loadbalancer_test.go

@@ -35,6 +35,64 @@ func TestAccCloudStackLoadBalancerRule_basic(t *testing.T) {
 }
 
 func TestAccCloudStackLoadBalancerRule_update(t *testing.T) {
+	id := ""
+	resource.Test(t, resource.TestCase{
+		PreCheck:     func() { testAccPreCheck(t) },
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckCloudStackLoadBalancerRuleDestroy,
+		Steps: []resource.TestStep{
+			resource.TestStep{
+				Config: testAccCloudStackLoadBalancerRule_basic,
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckCloudStackLoadBalancerRuleExist("cloudstack_loadbalancer_rule.foo"),
+					func(s *terraform.State) error {
+						rs, ok := s.RootModule().Resources["cloudstack_loadbalancer_rule.foo"]
+						if !ok {
+							return fmt.Errorf("Not found: cloudstack_loadbalancer_rule.foo")
+						}
+						id = rs.Primary.ID
+						return nil
+					},
+					resource.TestCheckResourceAttr(
+						"cloudstack_loadbalancer_rule.foo", "name", "terraform-lb"),
+					resource.TestCheckResourceAttr(
+						"cloudstack_loadbalancer_rule.foo", "algorithm", "roundrobin"),
+					resource.TestCheckResourceAttr(
+						"cloudstack_loadbalancer_rule.foo", "public_port", "80"),
+					resource.TestCheckResourceAttr(
+						"cloudstack_loadbalancer_rule.foo", "private_port", "80"),
+				),
+			},
+
+			resource.TestStep{
+				Config: testAccCloudStackLoadBalancerRule_update,
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckCloudStackLoadBalancerRuleExist("cloudstack_loadbalancer_rule.foo"),
+					func(s *terraform.State) error {
+						rs, ok := s.RootModule().Resources["cloudstack_loadbalancer_rule.foo"]
+						if !ok {
+							return fmt.Errorf("Not found: cloudstack_loadbalancer_rule.foo")
+						}
+						if id != rs.Primary.ID {
+							return fmt.Errorf("Resource has changed!")
+						}
+						return nil
+					},
+					resource.TestCheckResourceAttr(
+						"cloudstack_loadbalancer_rule.foo", "name", "terraform-lb-update"),
+					resource.TestCheckResourceAttr(
+						"cloudstack_loadbalancer_rule.foo", "algorithm", "leastconn"),
+					resource.TestCheckResourceAttr(
+						"cloudstack_loadbalancer_rule.foo", "public_port", "80"),
+					resource.TestCheckResourceAttr(
+						"cloudstack_loadbalancer_rule.foo", "private_port", "80"),
+				),
+			},
+		},
+	})
+}
+
+func TestAccCloudStackLoadBalancerRule_forcenew(t *testing.T) {
 	resource.Test(t, resource.TestCase{
 		PreCheck:     func() { testAccPreCheck(t) },
 		Providers:    testAccProviders,
@@ -56,7 +114,70 @@ func TestAccCloudStackLoadBalancerRule_update(t *testing.T) {
 			},
 
 			resource.TestStep{
-				Config: testAccCloudStackLoadBalancerRule_update,
+				Config: testAccCloudStackLoadBalancerRule_forcenew,
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckCloudStackLoadBalancerRuleExist("cloudstack_loadbalancer_rule.foo"),
+					resource.TestCheckResourceAttr(
+						"cloudstack_loadbalancer_rule.foo", "name", "terraform-lb-update"),
+					resource.TestCheckResourceAttr(
+						"cloudstack_loadbalancer_rule.foo", "algorithm", "leastconn"),
+					resource.TestCheckResourceAttr(
+						"cloudstack_loadbalancer_rule.foo", "public_port", "443"),
+					resource.TestCheckResourceAttr(
+						"cloudstack_loadbalancer_rule.foo", "private_port", "443"),
+				),
+			},
+		},
+	})
+}
+
+func TestAccCloudStackLoadBalancerRule_vpc(t *testing.T) {
+	resource.Test(t, resource.TestCase{
+		PreCheck:     func() { testAccPreCheck(t) },
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckCloudStackLoadBalancerRuleDestroy,
+		Steps: []resource.TestStep{
+			resource.TestStep{
+				Config: testAccCloudStackLoadBalancerRule_vpc,
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckCloudStackLoadBalancerRuleExist("cloudstack_loadbalancer_rule.foo"),
+					resource.TestCheckResourceAttr(
+						"cloudstack_loadbalancer_rule.foo", "name", "terraform-lb"),
+					resource.TestCheckResourceAttr(
+						"cloudstack_loadbalancer_rule.foo", "algorithm", "roundrobin"),
+					resource.TestCheckResourceAttr(
+						"cloudstack_loadbalancer_rule.foo", "public_port", "80"),
+					resource.TestCheckResourceAttr(
+						"cloudstack_loadbalancer_rule.foo", "private_port", "80"),
+				),
+			},
+		},
+	})
+}
+
+func TestAccCloudStackLoadBalancerRule_vpc_update(t *testing.T) {
+	resource.Test(t, resource.TestCase{
+		PreCheck:     func() { testAccPreCheck(t) },
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckCloudStackLoadBalancerRuleDestroy,
+		Steps: []resource.TestStep{
+			resource.TestStep{
+				Config: testAccCloudStackLoadBalancerRule_vpc,
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckCloudStackLoadBalancerRuleExist("cloudstack_loadbalancer_rule.foo"),
+					resource.TestCheckResourceAttr(
+						"cloudstack_loadbalancer_rule.foo", "name", "terraform-lb"),
+					resource.TestCheckResourceAttr(
+						"cloudstack_loadbalancer_rule.foo", "algorithm", "roundrobin"),
+					resource.TestCheckResourceAttr(
+						"cloudstack_loadbalancer_rule.foo", "public_port", "80"),
+					resource.TestCheckResourceAttr(
+						"cloudstack_loadbalancer_rule.foo", "private_port", "80"),
+				),
+			},
+
+			resource.TestStep{
+				Config: testAccCloudStackLoadBalancerRule_vpc_update,
 				Check: resource.ComposeTestCheckFunc(
 					testAccCheckCloudStackLoadBalancerRuleExist("cloudstack_loadbalancer_rule.foo"),
 					resource.TestCheckResourceAttr(
@@ -133,6 +254,120 @@ func testAccCheckCloudStackLoadBalancerRuleDestroy(s *terraform.State) error {
 }
 
 var testAccCloudStackLoadBalancerRule_basic = fmt.Sprintf(`
+resource "cloudstack_instance" "foobar1" {
+  name = "terraform-server1"
+  display_name = "terraform"
+  service_offering= "%s"
+  network = "%s"
+  template = "%s"
+  zone = "%s"
+  expunge = true
+}
+
+resource "cloudstack_loadbalancer_rule" "foo" {
+  name = "terraform-lb"
+  ipaddress = "%s"
+  # network omitted, inferred from IP
+  algorithm = "roundrobin"
+  public_port = 80
+  private_port = 80
+  members = ["${cloudstack_instance.foobar1.id}"]
+}
+
+# attempt to create dependent firewall rule
+# this will clash if cloudstack creates the implicit rule as it does by default
+resource "cloudstack_firewall" "foo" {
+  ipaddress = "${cloudstack_loadbalancer_rule.foo.ipaddress}"
+  rule {
+    source_cidr = "0.0.0.0/0"
+    protocol = "tcp"
+    ports = ["${cloudstack_loadbalancer_rule.foo.public_port}"]
+  }
+}
+`,
+	CLOUDSTACK_SERVICE_OFFERING_1,
+	CLOUDSTACK_NETWORK_1,
+	CLOUDSTACK_TEMPLATE,
+	CLOUDSTACK_ZONE,
+	CLOUDSTACK_PUBLIC_IPADDRESS)
+
+var testAccCloudStackLoadBalancerRule_update = fmt.Sprintf(`
+resource "cloudstack_instance" "foobar1" {
+  name = "terraform-server1"
+  display_name = "terraform"
+  service_offering= "%s"
+  network = "%s"
+  template = "%s"
+  zone = "%s"
+  expunge = true
+}
+
+resource "cloudstack_loadbalancer_rule" "foo" {
+  name = "terraform-lb-update"
+  ipaddress = "%s"
+  # network omitted, inferred from IP
+  algorithm = "leastconn"
+  public_port = 80
+  private_port = 80
+  members = ["${cloudstack_instance.foobar1.id}"]
+}
+
+# attempt to create dependent firewall rule
+# this will clash if cloudstack creates the implicit rule as it does by default
+resource "cloudstack_firewall" "foo" {
+  ipaddress = "${cloudstack_loadbalancer_rule.foo.ipaddress}"
+  rule {
+    source_cidr = "0.0.0.0/0"
+    protocol = "tcp"
+    ports = ["${cloudstack_loadbalancer_rule.foo.public_port}"]
+  }
+}
+`,
+	CLOUDSTACK_SERVICE_OFFERING_1,
+	CLOUDSTACK_NETWORK_1,
+	CLOUDSTACK_TEMPLATE,
+	CLOUDSTACK_ZONE,
+	CLOUDSTACK_PUBLIC_IPADDRESS)
+
+var testAccCloudStackLoadBalancerRule_forcenew = fmt.Sprintf(`
+resource "cloudstack_instance" "foobar1" {
+  name = "terraform-server1"
+  display_name = "terraform"
+  service_offering= "%s"
+  network = "%s"
+  template = "%s"
+  zone = "%s"
+  expunge = true
+}
+
+resource "cloudstack_loadbalancer_rule" "foo" {
+  name = "terraform-lb-update"
+  ipaddress = "%s"
+  # network omitted, inferred from IP
+  algorithm = "leastconn"
+  public_port = 443
+  private_port = 443
+  members = ["${cloudstack_instance.foobar1.id}"]
+}
+
+# attempt to create dependent firewall rule
+# this will clash if cloudstack creates the implicit rule as it does by default
+resource "cloudstack_firewall" "foo" {
+  ipaddress = "${cloudstack_loadbalancer_rule.foo.ipaddress}"
+  rule {
+    source_cidr = "0.0.0.0/0"
+    protocol = "tcp"
+    ports = ["${cloudstack_loadbalancer_rule.foo.public_port}"]
+  }
+}
+`,
+	CLOUDSTACK_SERVICE_OFFERING_1,
+	CLOUDSTACK_NETWORK_1,
+	CLOUDSTACK_TEMPLATE,
+	CLOUDSTACK_ZONE,
+	CLOUDSTACK_PUBLIC_IPADDRESS)
+
+var testAccCloudStackLoadBalancerRule_vpc = fmt.Sprintf(`
 resource "cloudstack_vpc" "foobar" {
 	name = "terraform-vpc"
 	cidr = "%s"
@@ -180,7 +415,7 @@ resource "cloudstack_loadbalancer_rule" "foo" {
 	CLOUDSTACK_SERVICE_OFFERING_1,
 	CLOUDSTACK_TEMPLATE)
 
-var testAccCloudStackLoadBalancerRule_update = fmt.Sprintf(`
+var testAccCloudStackLoadBalancerRule_vpc_update = fmt.Sprintf(`
 resource "cloudstack_vpc" "foobar" {
     name = "terraform-vpc"
     cidr = "%s"