Repository: digitalocean/doctl
Pull request #1496 (2024-01-02 19:33:29)

LBAAS-2899: doctl not removing LB firewall rules on update
Currently, when you pass an empty value to the the load balancer allow/deny rules flag we are not updating the LB firewall rules. To match the behavior of our public API we should allow an empty flag allow/deny rules flag to remove the firewall rules from the LB.

At current passing an empty flag does not populates the LB firewall field. We should be separating the behavior of the allow/deny rules flag not being specified and the flag being specified but empty

This change allows an empty value in the fw rules flags to update the LB firewall to empty.
When not specifying the fw rules flags the LB firewall will not be updated.

guptado: can you share how are we testing it out?
jvasilevsky: 
Add allow firewall rule
```
doctl compute load-balancer update a5faf024-bb6d-4b68-80c9-c987d504783d --name="nyc3-load-balancer-01" --vpc-uuid="32e26fa6-86b7-45d8-9719-1bc4b7edbed4" --size="lb-small" --size-unit="1" --region="nyc3" --forwarding-rules="entry_protocol:http,entry_port:80,target_protocol:http,target_port:80 entry_protocol:https,entry_port:443,target_protocol:https,target_port:443,tls_passthrough:true" --allow-list="cidr:1.2.0.0/16"
```

![image](https://github.com/digitalocean/doctl/assets/95491250/1c62b406-b5cf-464b-846e-5c414d996b95)


No firewall flag specified: No action
```
doctl compute load-balancer update a5faf024-bb6d-4b68-80c9-c987d504783d --name="nyc3-load-balancer-01" --vpc-uuid="32e26fa6-86b7-45d8-9719-1bc4b7edbed4" --size="lb-small" --size-unit="1" --region="nyc3" --forwarding-rules="entry_protocol:http,entry_port:80,target_protocol:http,target_port:80 entry_protocol:https,entry_port:443,target_protocol:https,target_port:443,tls_passthrough:true"
```

![image](https://github.com/digitalocean/doctl/assets/95491250/564a7857-967f-42a9-be50-459d2274d40e)

Empty allow flag: remove firewall rules
```
doctl compute load-balancer update a5faf024-bb6d-4b68-80c9-c987d504783d --name="nyc3-load-balancer-01" --vpc-uuid="32e26fa6-86b7-45d8-9719-1bc4b7edbed4" --size="lb-small" --size-unit="1" --region="nyc3" --forwarding-rules="entry_protocol:http,entry_port:80,target_protocol:http,target_port:80 entry_protocol:https,entry_port:443,target_protocol:https,target_port:443,tls_passthrough:true" --allow-list=""
```
![image](https://github.com/digitalocean/doctl/assets/95491250/d6232897-ce90-4488-939b-e6889656cf2c)




Patch:
diff --git a/commands/load_balancers.go b/commands/load_balancers.go
index 30494030..845ab546 100644
--- a/commands/load_balancers.go
+++ b/commands/load_balancers.go
@@ -125,9 +125,9 @@ With the load-balancer command, you can list, create, or delete load balancers,
 		"disable automatic DNS record creation for Let's Encrypt certificates that are added to the load balancer")
 	AddStringFlag(cmdRecordUpdate, doctl.ArgProjectID, "", "",
 		"Indicates which project to associate the Load Balancer with. If not specified, the Load Balancer will be placed in your default project.")
-	AddStringSliceFlag(cmdRecordUpdate, doctl.ArgAllowList, "", []string{},
+	AddStringSliceFlag(cmdRecordUpdate, doctl.ArgAllowList, "", nil,
 		"A comma-separated list of ALLOW rules for the load balancer, e.g.: `ip:1.2.3.4,cidr:1.2.0.0/16`")
-	AddStringSliceFlag(cmdRecordUpdate, doctl.ArgDenyList, "", []string{},
+	AddStringSliceFlag(cmdRecordUpdate, doctl.ArgDenyList, "", nil,
 		"A comma-separated list of DENY rules for the load balancer, e.g.: `ip:1.2.3.4,cidr:1.2.0.0/16`")
 
 	CmdBuilder(cmd, RunLoadBalancerList, "list", "List load balancers", "Use this command to get a list of the load balancers on your account, including the following information for each:\n\n"+lbDetail, Writer,
@@ -557,17 +557,17 @@ func buildRequestFromArgs(c *CmdConfig, r *godo.LoadBalancerRequest) error {
 		r.HTTPIdleTimeoutSeconds = &t
 	}
 
-	allowRules, err := c.Doit.GetStringSlice(c.NS, doctl.ArgAllowList)
+	allowRules, allowflagSet, err := c.Doit.GetStringSliceIsFlagSet(c.NS, doctl.ArgAllowList)
 	if err != nil {
 		return err
 	}
 
-	denyRules, err := c.Doit.GetStringSlice(c.NS, doctl.ArgDenyList)
+	denyRules, denyFlagSet, err := c.Doit.GetStringSliceIsFlagSet(c.NS, doctl.ArgDenyList)
 	if err != nil {
 		return err
 	}
 
-	if len(allowRules) > 0 || len(denyRules) > 0 {
+	if allowflagSet || denyFlagSet {
 		firewall := new(godo.LBFirewall)
 		firewall.Allow = allowRules
 		firewall.Deny = denyRules
diff --git a/doit.go b/doit.go
index bcb62d8b..818ff72c 100644
--- a/doit.go
+++ b/doit.go
@@ -219,6 +219,7 @@ type Config interface {
 	GetInt(ns, key string) (int, error)
 	GetIntPtr(ns, key string) (*int, error)
 	GetStringSlice(ns, key string) ([]string, error)
+	GetStringSliceIsFlagSet(ns, key string) ([]string, bool, error)
 	GetStringMapString(ns, key string) (map[string]string, error)
 	GetDuration(ns, key string) (time.Duration, error)
 }
@@ -426,6 +427,15 @@ func (c *LiveConfig) GetStringSlice(ns, key string) ([]string, error) {
 	return out, nil
 }
 
+// GetStringSliceIsFlagSet returns a config value as a string slice and a bool representing the existence of the flag.
+func (c *LiveConfig) GetStringSliceIsFlagSet(ns, key string) ([]string, bool, error) {
+	if !c.IsSet(key) {
+		return nil, false, nil
+	}
+	strSlice, err := c.GetStringSlice(ns, key)
+	return strSlice, true, err
+}
+
 // GetStringMapString returns a config value as a string to string map.
 func (c *LiveConfig) GetStringMapString(ns, key string) (map[string]string, error) {
 	nskey := nskey(ns, key)
@@ -562,6 +572,17 @@ func (c *TestConfig) GetStringSlice(ns, key string) ([]string, error) {
 	return c.v.GetStringSlice(nskey), nil
 }
 
+// GetStringSliceIsFlagSet returns the string slice value for the key in the given
+// namespace and a bool representing the existence of the flag. Because this is a mock implementation,
+// and error will never be returned.
+func (c *TestConfig) GetStringSliceIsFlagSet(ns, key string) ([]string, bool, error) {
+	nskey := nskey(ns, key)
+	if !c.v.IsSet(nskey) {
+		return nil, false, nil
+	}
+	return c.v.GetStringSlice(nskey), true, nil
+}
+
 // GetStringMapString returns the string-to-string value for the key in the
 // given namespace. Because this is a mock implementation, and error will never
 // be returned.


Test patch:
diff --git a/commands/load_balancers_test.go b/commands/load_balancers_test.go
index b8975218..9d763274 100644
--- a/commands/load_balancers_test.go
+++ b/commands/load_balancers_test.go
@@ -209,6 +209,123 @@ func TestLoadBalancerUpdateNoID(t *testing.T) {
 	})
 }
 
+func TestLoadBalancerUpdateNoFirewallFlags(t *testing.T) {
+	withTestClient(t, func(config *CmdConfig, tm *tcMocks) {
+		lbID := "cde2c0d6-41e3-479e-ba60-ad971227232c"
+		timeout := uint64(120)
+		r := godo.LoadBalancerRequest{
+			Name:       "lb-name",
+			Region:     "nyc1",
+			DropletIDs: []int{1, 2},
+			SizeUnit:   4,
+			StickySessions: &godo.StickySessions{
+				Type:             "cookies",
+				CookieName:       "DO-LB",
+				CookieTtlSeconds: 5,
+			},
+			HealthCheck: &godo.HealthCheck{
+				Protocol:               "http",
+				Port:                   80,
+				CheckIntervalSeconds:   4,
+				ResponseTimeoutSeconds: 23,
+				HealthyThreshold:       5,
+				UnhealthyThreshold:     10,
+			},
+			ForwardingRules: []godo.ForwardingRule{
+				{
+					EntryProtocol:  "http",
+					EntryPort:      80,
+					TargetProtocol: "http",
+					TargetPort:     80,
+				},
+			},
+			ProjectID:              "project-id-uuid",
+			HTTPIdleTimeoutSeconds: &timeout,
+			Firewall:               nil,
+		}
+		disableLetsEncryptDNSRecords := true
+		r.DisableLetsEncryptDNSRecords = &disableLetsEncryptDNSRecords
+		tm.loadBalancers.EXPECT().Update(lbID, &r).Return(&testLoadBalancer, nil)
+
+		config.Args = append(config.Args, lbID)
+		config.Doit.Set(config.NS, doctl.ArgRegionSlug, "nyc1")
+		config.Doit.Set(config.NS, doctl.ArgSizeSlug, "")
+		config.Doit.Set(config.NS, doctl.ArgSizeUnit, 4)
+		config.Doit.Set(config.NS, doctl.ArgLoadBalancerName, "lb-name")
+		config.Doit.Set(config.NS, doctl.ArgDropletIDs, []string{"1", "2"})
+		config.Doit.Set(config.NS, doctl.ArgStickySessions, "type:cookies,cookie_name:DO-LB,cookie_ttl_seconds:5")
+		config.Doit.Set(config.NS, doctl.ArgHealthCheck, "protocol:http,port:80,check_interval_seconds:4,response_timeout_seconds:23,healthy_threshold:5,unhealthy_threshold:10")
+		config.Doit.Set(config.NS, doctl.ArgForwardingRules, "entry_protocol:http,entry_port:80,target_protocol:http,target_port:80")
+		config.Doit.Set(config.NS, doctl.ArgDisableLetsEncryptDNSRecords, true)
+		config.Doit.Set(config.NS, doctl.ArgProjectID, "project-id-uuid")
+		config.Doit.Set(config.NS, doctl.ArgHTTPIdleTimeoutSeconds, 120)
+
+		err := RunLoadBalancerUpdate(config)
+		assert.NoError(t, err)
+	})
+}
+
+func TestLoadBalancerUpdateEmptyFirewallFlags(t *testing.T) {
+	withTestClient(t, func(config *CmdConfig, tm *tcMocks) {
+		lbID := "cde2c0d6-41e3-479e-ba60-ad971227232c"
+		timeout := uint64(120)
+		r := godo.LoadBalancerRequest{
+			Name:       "lb-name",
+			Region:     "nyc1",
+			DropletIDs: []int{1, 2},
+			SizeUnit:   4,
+			StickySessions: &godo.StickySessions{
+				Type:             "cookies",
+				CookieName:       "DO-LB",
+				CookieTtlSeconds: 5,
+			},
+			HealthCheck: &godo.HealthCheck{
+				Protocol:               "http",
+				Port:                   80,
+				CheckIntervalSeconds:   4,
+				ResponseTimeoutSeconds: 23,
+				HealthyThreshold:       5,
+				UnhealthyThreshold:     10,
+			},
+			ForwardingRules: []godo.ForwardingRule{
+				{
+					EntryProtocol:  "http",
+					EntryPort:      80,
+					TargetProtocol: "http",
+					TargetPort:     80,
+				},
+			},
+			ProjectID:              "project-id-uuid",
+			HTTPIdleTimeoutSeconds: &timeout,
+			Firewall: &godo.LBFirewall{
+				Deny:  []string{},
+				Allow: []string{},
+			},
+		}
+		disableLetsEncryptDNSRecords := true
+		r.DisableLetsEncryptDNSRecords = &disableLetsEncryptDNSRecords
+		tm.loadBalancers.EXPECT().Update(lbID, &r).Return(&testLoadBalancer, nil)
+
+		config.Args = append(config.Args, lbID)
+		config.Doit.Set(config.NS, doctl.ArgRegionSlug, "nyc1")
+		config.Doit.Set(config.NS, doctl.ArgSizeSlug, "")
+		config.Doit.Set(config.NS, doctl.ArgSizeUnit, 4)
+		config.Doit.Set(config.NS, doctl.ArgLoadBalancerName, "lb-name")
+		config.Doit.Set(config.NS, doctl.ArgDropletIDs, []string{"1", "2"})
+		config.Doit.Set(config.NS, doctl.ArgStickySessions, "type:cookies,cookie_name:DO-LB,cookie_ttl_seconds:5")
+		config.Doit.Set(config.NS, doctl.ArgHealthCheck, "protocol:http,port:80,check_interval_seconds:4,response_timeout_seconds:23,healthy_threshold:5,unhealthy_threshold:10")
+		config.Doit.Set(config.NS, doctl.ArgForwardingRules, "entry_protocol:http,entry_port:80,target_protocol:http,target_port:80")
+		config.Doit.Set(config.NS, doctl.ArgDisableLetsEncryptDNSRecords, true)
+		config.Doit.Set(config.NS, doctl.ArgProjectID, "project-id-uuid")
+		config.Doit.Set(config.NS, doctl.ArgHTTPIdleTimeoutSeconds, 120)
+		config.Doit.Set(config.NS, doctl.ArgDenyList, []string{})
+		config.Doit.Set(config.NS, doctl.ArgAllowList, []string{})
+
+		err := RunLoadBalancerUpdate(config)
+		assert.NoError(t, err)
+	})
+}
+
 func TestLoadBalancerDelete(t *testing.T) {
 	withTestClient(t, func(config *CmdConfig, tm *tcMocks) {
 		lbID := "cde2c0d6-41e3-479e-ba60-ad971227232c"