[fix] : limit fw-rule description to 100 chars (#321)

* limit fw-rule description to 100 chars

* address review comment

Author: rahulait

cloud/linode/firewall/firewalls.go

@@ -20,6 +20,7 @@ import (
 
 const (
 	maxFirewallRuleLabelLen = 32
+	maxFirewallRuleDescLen  = 100
 	maxIPsPerFirewall       = 255
 	maxRulesPerFirewall     = 25
 )
@@ -183,6 +184,16 @@ func chunkIPs(ips []string) [][]string {
 	return chunks
 }
 
+// truncateFWRuleDesc truncates the description to maxFirewallRuleDescLen if it exceeds the limit.
+func truncateFWRuleDesc(desc string) string {
+	if len(desc) > maxFirewallRuleDescLen {
+		newDesc := desc[0:maxFirewallRuleDescLen-3] + "..."
+		klog.Infof("Firewall rule description '%s' is too long. Stripping it to '%s'", desc, newDesc)
+		desc = newDesc
+	}
+	return desc
+}
+
 // processACL takes the IPs, aclType, label etc and formats them into the passed linodego.FirewallCreateOptions pointer.
 func processACL(fwcreateOpts *linodego.FirewallCreateOptions, aclType, label, svcName, ports string, ips linodego.NetworkAddresses) error {
 	ruleLabel := fmt.Sprintf("%s-%s", aclType, svcName)
@@ -205,10 +216,11 @@ func processACL(fwcreateOpts *linodego.FirewallCreateOptions, aclType, label, sv
 		ipv4chunks := chunkIPs(ipv4s)
 		for i, chunk := range ipv4chunks {
 			v4chunk := chunk
+			desc := fmt.Sprintf("Rule %d, Created by linode-ccm: %s, for %s", i, label, svcName)
 			fwcreateOpts.Rules.Inbound = append(fwcreateOpts.Rules.Inbound, linodego.FirewallRule{
 				Action:      aclType,
 				Label:       ruleLabel,
-				Description: fmt.Sprintf("Rule %d, Created by linode-ccm: %s, for %s", i, label, svcName),
+				Description: truncateFWRuleDesc(desc),
 				Protocol:    linodego.TCP, // Nodebalancers support only TCP.
 				Ports:       ports,
 				Addresses:   linodego.NetworkAddresses{IPv4: &v4chunk},
@@ -218,20 +230,22 @@ func processACL(fwcreateOpts *linodego.FirewallCreateOptions, aclType, label, sv
 		ipv6chunks := chunkIPs(ipv6s)
 		for i, chunk := range ipv6chunks {
 			v6chunk := chunk
+			desc := fmt.Sprintf("Rule %d, Created by linode-ccm: %s, for %s", i, label, svcName)
 			fwcreateOpts.Rules.Inbound = append(fwcreateOpts.Rules.Inbound, linodego.FirewallRule{
 				Action:      aclType,
 				Label:       ruleLabel,
-				Description: fmt.Sprintf("Rule %d, Created by linode-ccm: %s, for %s", i, label, svcName),
+				Description: truncateFWRuleDesc(desc),
 				Protocol:    linodego.TCP, // Nodebalancers support only TCP.
 				Ports:       ports,
 				Addresses:   linodego.NetworkAddresses{IPv6: &v6chunk},
 			})
 		}
 	} else {
+		desc := fmt.Sprintf("Created by linode-ccm: %s, for %s", label, svcName)
 		fwcreateOpts.Rules.Inbound = append(fwcreateOpts.Rules.Inbound, linodego.FirewallRule{
 			Action:      aclType,
 			Label:       ruleLabel,
-			Description: fmt.Sprintf("Created by linode-ccm: %s, for %s", label, svcName),
+			Description: truncateFWRuleDesc(desc),
 			Protocol:    linodego.TCP, // Nodebalancers support only TCP.
 			Ports:       ports,
 			Addresses:   ips,
@@ -453,7 +467,7 @@ func (l *LinodeClient) updateNodeBalancerFirewallWithACL(
 				return nil
 			}
 
-			fwCreateOpts, err := CreateFirewallOptsForSvc(service.Name, []string{""}, service)
+			fwCreateOpts, err := CreateFirewallOptsForSvc(firewalls[0].Label, []string{""}, service)
 			if err != nil {
 				return err
 			}

cloud/linode/loadbalancers_test.go

@@ -258,6 +258,10 @@ func TestCCMLoadBalancers(t *testing.T) {
 			name: "Update Load Balancer - No Nodes",
 			f:    testUpdateLoadBalancerNoNodes,
 		},
+		{
+			name: "Create Load Balancer - Very long Service name",
+			f:    testVeryLongServiceName,
+		},
 	}
 
 	for _, tc := range testCases {
@@ -816,6 +820,89 @@ func testUpdateLoadBalancerAddPortAnnotation(t *testing.T, client *linodego.Clie
 	}
 }
 
+func toJSON(v interface{}) string {
+	data, _ := json.Marshal(v)
+	return string(data)
+}
+
+func testVeryLongServiceName(t *testing.T, client *linodego.Client, _ *fakeAPI) {
+	ipv4DenyList := make([]string, 130)
+	ipv6DenyList := make([]string, 130)
+
+	for i := 0; i < len(ipv4DenyList); i++ {
+		ipv4DenyList[i] = fmt.Sprintf("192.168.1.%d/32", i)
+		ipv6DenyList[i] = fmt.Sprintf("2001:db8::%x/128", i)
+	}
+	denyListJSON := fmt.Sprintf(`{
+		"denyList": {
+			"ipv4": %s,
+			"ipv6": %s
+		}
+	}`, toJSON(ipv4DenyList), toJSON(ipv6DenyList))
+
+	svc := &v1.Service{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: strings.Repeat(randString(), 6),
+			UID:  "foobar123",
+			Annotations: map[string]string{
+				annotations.AnnLinodeCloudFirewallACL: denyListJSON,
+			},
+		},
+		Spec: v1.ServiceSpec{
+			Ports: []v1.ServicePort{
+				{
+					Name:     randString(),
+					Protocol: "TCP",
+					Port:     int32(80),
+					NodePort: int32(30000),
+				},
+			},
+		},
+	}
+
+	nodes := []*v1.Node{
+		{
+			Status: v1.NodeStatus{
+				Addresses: []v1.NodeAddress{
+					{
+						Type:    v1.NodeInternalIP,
+						Address: "127.0.0.1",
+					},
+				},
+			},
+		},
+	}
+
+	lb := newLoadbalancers(client, "us-west").(*loadbalancers)
+	fakeClientset := fake.NewSimpleClientset()
+	lb.kubeClient = fakeClientset
+
+	defer func() {
+		_ = lb.EnsureLoadBalancerDeleted(context.TODO(), "linodelb", svc)
+	}()
+
+	lbStatus, err := lb.EnsureLoadBalancer(context.TODO(), "linodelb", svc, nodes)
+	if err != nil {
+		t.Errorf("EnsureLoadBalancer returned an error: %s", err)
+	}
+	svc.Status.LoadBalancer = *lbStatus
+	stubService(fakeClientset, svc)
+
+	svc.ObjectMeta.SetAnnotations(map[string]string{
+		annotations.AnnLinodeCloudFirewallACL: `{
+			"denyList": {
+				"ipv4": ["192.168.1.0/32"],
+				"ipv6": ["2001:db8::/128"]
+			}
+		}`,
+	})
+
+	err = lb.UpdateLoadBalancer(context.TODO(), "linodelb", svc, nodes)
+	if err != nil {
+		t.Fatalf("UpdateLoadBalancer returned an error with updated annotations: %s", err)
+	}
+}
+
 func testUpdateLoadBalancerAddTags(t *testing.T, client *linodego.Client, _ *fakeAPI) {
 	svc := &v1.Service{
 		ObjectMeta: metav1.ObjectMeta{