Repository: linode/linode-cloud-controller-manager
Pull request #180 (2024-02-16 05:32:34)

Support for acls with >255 IPs
### General:
Adds support for firewall ACL with >255IPs, building intelligence into the ccm to break it up into chunks of ips.
Implemented an ip-split mechanism. will be adding a e2e test for the firewall feature as a whole shortly.

* [x] Have you removed all sensitive information, including but not limited to access keys and passwords?
* [x] Have you checked to ensure there aren't other open or closed [Pull Requests](../../pulls) for the same bug/feature/question?

### Pull Request Guidelines:

1. [x] Does your submission pass tests?
1. [x] Have you added tests? 
1. [x] Are you addressing a single feature in this PR? 
1. [x] Are your commits atomic, addressing one change per commit?
1. [x] Are you following the conventions of the language? 
1. [x] Have you saved your large formatting changes for a different PR, so we can focus on your work?
1. [x] Have you explained your rationale for why this feature is needed? 
1. [ ] Have you linked your PR to an [open issue](https://blog.github.com/2013-05-14-closing-issues-via-pull-requests/)




Patch:
diff --git a/.github/workflows/helm.yml b/.github/workflows/helm.yml
deleted file mode 100644
index 17df884..0000000
--- a/.github/workflows/helm.yml
+++ /dev/null
@@ -1,82 +0,0 @@
-name: Helm
-
-on:
-  pull_request:
-    paths:
-      - 'deploy/chart/**'
-  workflow_dispatch: {}
-  release: {}
-
-permissions:
-  contents: read
-
-jobs:
-  helm-test:
-    runs-on: ubuntu-latest
-    steps:
-      - name: Checkout
-        uses: actions/checkout@v4
-        with:
-          fetch-depth: 0
-
-      - name: Set Version
-        run: |
-          TAG=$(git describe --tags --abbrev=0)
-          sed -ie "s/appVersion: \"latest\"/appVersion: ${TAG#helm-}/g" deploy/chart/Chart.yaml
-          sed -ie "s/version: 0.0.0/version: ${TAG#helm-}/g" deploy/chart/Chart.yaml
-
-      - name: Set up Helm
-        uses: azure/setup-helm@v3
-
-      - uses: actions/setup-python@v4
-        with:
-          python-version: '3.10'
-          check-latest: true
-
-      - name: Set up chart-testing
-        uses: helm/chart-testing-action@v2.6.0
-
-      - name: Run chart-testing (lint)
-        run: ct lint --check-version-increment=false --chart-dirs deploy --target-branch ${{ github.event.repository.default_branch }}
-
-      # we cannot test a helm install without a valid linode
-      # - name: Create kind cluster
-      #  uses: helm/kind-action@v1.8.0
-
-      #- name: Run chart-testing (install)
-      #  run: ct install --chart-dirs helm-chart --namespace kube-system --helm-extra-set-args "--set=apiToken=test --set=region=us-east"  --target-branch ${{ github.event.repository.default_branch }}
-
-  helm-release:
-    if: github.ref == 'refs/heads/main'
-    needs: helm-test
-    permissions:
-      contents: write  # for helm/chart-releaser-action to push chart release and create a release
-    runs-on: ubuntu-latest
-    steps:
-      - name: Checkout
-        uses: actions/checkout@v4
-        with:
-          fetch-depth: 0
-
-      - name: Set Version
-        run: |
-          TAG=$(git describe --tags --abbrev=0)
-          sed -ie "s/appVersion: \"latest\"/appVersion: ${TAG#helm-}/g" deploy/chart/Chart.yaml
-          sed -ie "s/version: 0.0.0/version: ${TAG#helm-}/g" deploy/chart/Chart.yaml
-
-      - name: Configure Git
-        run: |
-          git config user.name "$GITHUB_ACTOR"
-          git config user.email "$GITHUB_ACTOR@users.noreply.github.com"
-
-      - name: Set up Helm
-        uses: azure/setup-helm@v3
-
-      - name: Run chart-releaser
-        uses: helm/chart-releaser-action@v1.6.0
-        env:
-          CR_TOKEN: "${{ secrets.GITHUB_TOKEN }}"
-          CR_RELEASE_NAME_TEMPLATE: "helm-{{ .Version }}"
-        with:
-          charts_dir: deploy
-          skip_existing: true
diff --git a/README.md b/README.md
index 2236b2b..48d65d9 100644
--- a/README.md
+++ b/README.md
@@ -255,28 +255,24 @@ This will create a file `ccm-linode.yaml` which you can use to deploy the CCM.
 Note: Your kubelets, controller-manager, and apiserver must be started with `--cloud-provider=external` as noted in the following documentation.
 
 ## Deployment Through Helm Chart
-LINODE_API_TOKEN must be a Linode APIv4 [Personal Access Token](https://cloud.linode.com/profile/tokens) with all permissions.
+Use the helm chart located under './deploy/chart'. This dir has the manifest for Linode CCM. There are two arguments required.
 
-REGION must be a Linode [region](https://api.linode.com/v4/regions).
-### Install the ccm-linode repo
-```shell
-helm repo add ccm-linode https://linode.github.io/linode-cloud-controller-manager/ 
-helm repo update ccm-linode
-```
+The first argument must be a Linode APIv4 [Personal Access Token](https://cloud.linode.com/profile/tokens) with all permissions.
 
-### To deploy ccm-linode. Run the following command:
+The second argument must be a Linode [region](https://api.linode.com/v4/regions).
 
+### To deploy CCM run the following helm command once you are in the ccm root dir:
 ```sh
 export VERSION=v0.3.22
 export LINODE_API_TOKEN=<linodeapitoken>
 export REGION=<linoderegion>
-helm install ccm-linode --set apiToken=$LINODE_API_TOKEN,region=$REGION ccm-linode/ccm-linode
+helm install linode-ccm --set apiToken=$LINODE_API_TOKEN,region=$REGION https://github.com/linode/linode-cloud-controller-manager/releases/download/$VERSION/helm-chart-$VERSION.tgz
 ```
 _See [helm install](https://helm.sh/docs/helm/helm_install/) for command documentation._
 
-### To uninstall ccm-linode from kubernetes cluster. Run the following command:
+### To uninstall linode-ccm from kubernetes cluster. Run the following command:
 ```sh
-helm uninstall ccm-linode
+helm uninstall linode-ccm
 ```
 _See [helm uninstall](https://helm.sh/docs/helm/helm_uninstall/) for command documentation._
 
@@ -286,7 +282,7 @@ export VERSION=v0.3.22
 export LINODE_API_TOKEN=<linodeapitoken>
 export REGION=<linoderegion>
 
-helm upgrade ccm-linode --install --set apiToken=$LINODE_API_TOKEN,region=$REGION ccm-linode/ccm-linode
+helm upgrade linode-ccm --install --set apiToken=$LINODE_API_TOKEN,region=$REGION https://github.com/linode/linode-cloud-controller-manager/releases/download/$VERSION/helm-chart-$VERSION.tgz
 ```
 _See [helm upgrade](https://helm.sh/docs/helm/helm_upgrade/) for command documentation._
 
diff --git a/cloud/linode/loadbalancers.go b/cloud/linode/loadbalancers.go
index 72d2269..15b3d31 100644
--- a/cloud/linode/loadbalancers.go
+++ b/cloud/linode/loadbalancers.go
@@ -28,12 +28,15 @@ import (
 
 const (
 	maxFirewallRuleLabelLen = 32
+	maxIPsPerFirewall       = 255
+	maxRulesPerFirewall     = 25
 )
 
 var (
 	errNoNodesAvailable = errors.New("No nodes available for nodebalancer")
 	errInvalidFWConfig  = errors.New("Specify either an allowList or a denyList for a firewall")
 	errTooManyFirewalls = errors.New("Too many firewalls attached to a nodebalancer")
+	errTooManyIPs = errors.New("too many IPs in this ACL, will exceed rules per firewall limit")
 )
 
 type lbNotFoundError struct {
@@ -765,8 +768,34 @@ func (l *loadbalancers) getLoadBalancerTags(_ context.Context, clusterName strin
 	return tags
 }
 
+func chunkIPs(ips []string) [][]string {
+	chunks := [][]string{}
+	ipCount := len(ips)
+
+	// If the number of IPs is less than or equal to maxIPsPerFirewall,
+	// return a single chunk containing all IPs.
+	if ipCount <= maxIPsPerFirewall {
+		return [][]string{ips}
+	}
+
+	// Otherwise, break the IPs into chunks with maxIPsPerFirewall IPs per chunk.
+	chunkCount := 0
+	for ipCount > maxIPsPerFirewall {
+		start := chunkCount * maxIPsPerFirewall
+		end := (chunkCount + 1) * maxIPsPerFirewall
+		chunks = append(chunks, ips[start:end])
+		chunkCount++
+		ipCount -= maxIPsPerFirewall
+	}
+
+	// Append the remaining IPs as a chunk.
+	chunks = append(chunks, ips[chunkCount*maxIPsPerFirewall:])
+
+	return chunks
+}
+
 // 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) {
+func processACL(fwcreateOpts *linodego.FirewallCreateOptions, aclType, label, svcName, ports string, ips linodego.NetworkAddresses) error {
 	ruleLabel := fmt.Sprintf("%s-%s", aclType, svcName)
 	if len(ruleLabel) > maxFirewallRuleLabelLen {
 		newLabel := ruleLabel[0:maxFirewallRuleLabelLen]
@@ -774,14 +803,52 @@ func processACL(fwcreateOpts *linodego.FirewallCreateOptions, aclType, label, sv
 		ruleLabel = newLabel
 	}
 
-	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),
-		Protocol:    linodego.TCP, // Nodebalancers support only TCP.
-		Ports:       ports,
-		Addresses:   ips,
-	})
+	// Linode has a limitation of firewall rules with a max of 255 IPs per rule
+	var ipv4s, ipv6s []string // doing this to avoid dereferencing a nil pointer
+	if ips.IPv6 != nil {
+		ipv6s = *ips.IPv6
+	}
+	if ips.IPv4 != nil {
+		ipv4s = *ips.IPv4
+	}
+
+	if len(ipv4s)+len(ipv6s) > maxIPsPerFirewall {
+		ipv4chunks := chunkIPs(ipv4s)
+		for i, chunk := range ipv4chunks {
+			v4chunk := chunk
+			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),
+				Protocol:    linodego.TCP, // Nodebalancers support only TCP.
+				Ports:       ports,
+				Addresses:   linodego.NetworkAddresses{IPv4: &v4chunk},
+			})
+		}
+
+		ipv6chunks := chunkIPs(ipv6s)
+		for i, chunk := range ipv6chunks {
+			v6chunk := chunk
+			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),
+				Protocol:    linodego.TCP, // Nodebalancers support only TCP.
+				Ports:       ports,
+				Addresses:   linodego.NetworkAddresses{IPv6: &v6chunk},
+			})
+		}
+	} else {
+		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),
+			Protocol:    linodego.TCP, // Nodebalancers support only TCP.
+			Ports:       ports,
+			Addresses:   ips,
+		})
+	}
+
 	fwcreateOpts.Rules.OutboundPolicy = "ACCEPT"
 	if aclType == "ACCEPT" {
 		// if an allowlist is present, we drop everything else.
@@ -790,6 +857,11 @@ func processACL(fwcreateOpts *linodego.FirewallCreateOptions, aclType, label, sv
 		// if a denylist is present, we accept everything else.
 		fwcreateOpts.Rules.InboundPolicy = "ACCEPT"
 	}
+
+	if len(fwcreateOpts.Rules.Inbound) > maxRulesPerFirewall {
+		return errTooManyIPs
+	}
+	return nil
 }
 
 type aclConfig struct {
@@ -827,7 +899,10 @@ func (l *loadbalancers) createFirewallOptsForSvc(label string, tags []string, sv
 		allowedIPs = acl.DenyList
 	}
 
-	processACL(&fwcreateOpts, aclType, label, svc.Name, portsString, *allowedIPs)
+	err = processACL(&fwcreateOpts, aclType, label, svc.Name, portsString, *allowedIPs)
+	if err != nil {
+		return nil, err
+	}
 	return &fwcreateOpts, nil
 }
 
diff --git a/deploy/chart/Chart.yaml b/deploy/chart/Chart.yaml
index 959afc2..c442b68 100644
--- a/deploy/chart/Chart.yaml
+++ b/deploy/chart/Chart.yaml
@@ -2,7 +2,5 @@ apiVersion: v2
 name: ccm-linode
 description: The Linode Cloud Controller Manager (CCM) provides a way for Kubernetes clusters to access additional Linode services. Linode's CCM will automatically provision a Linode NodeBalancer for Kubernetes Services of type "LoadBalancer".
 type: application
-version: 0.0.0
+version: 1.0.0
 appVersion: "latest"
-maintainers:
-  - name: linode
diff --git a/deploy/chart/values.yaml b/deploy/chart/values.yaml
index 2c00b6d..790458d 100644
--- a/deploy/chart/values.yaml
+++ b/deploy/chart/values.yaml
@@ -19,7 +19,7 @@ nodeSelector:
 # The pullPolicy is set to Always but can be changed when it is not required to always pull the new image
 image:
   repository: linode/linode-cloud-controller-manager
-  tag:  # only set if required, defaults to .Chart.AppVersion set during release or "latest" by default
+  tag: latest
   pullPolicy: Always
 
 # Default namespace is "kube-system" but it can be set to another namespace
@@ -48,4 +48,4 @@ tolerations:
 # LINODE_HOSTNAME_ONLY_INGRESS type bool is supported
 # env:
   # - name: EXAMPLE_ENV_VAR
-  #   value: "true"
+  #   value: "true"
\ No newline at end of file


Test patch:
diff --git a/cloud/linode/loadbalancers_test.go b/cloud/linode/loadbalancers_test.go
index 2747f85..1a44bd0 100644
--- a/cloud/linode/loadbalancers_test.go
+++ b/cloud/linode/loadbalancers_test.go
@@ -2,8 +2,12 @@ package linode
 
 import (
 	"context"
+	cryptoRand "crypto/rand"
+	"encoding/hex"
+	"encoding/json"
 	stderrors "errors"
 	"fmt"
+	"math/rand"
 	"net/http"
 	"net/http/httptest"
 	"os"
@@ -960,6 +964,7 @@ func testUpdateLoadBalancerAddNewFirewall(t *testing.T, client *linodego.Client,
 	}
 }
 
+// This will also test the firewall with >255 IPs
 func testUpdateLoadBalancerAddNewFirewallACL(t *testing.T, client *linodego.Client, fakeAPI *fakeAPI) {
 	svc := &v1.Service{
 		ObjectMeta: metav1.ObjectMeta{
@@ -1019,12 +1024,43 @@ func testUpdateLoadBalancerAddNewFirewallACL(t *testing.T, client *linodego.Clie
 		t.Fatalf("Firewalls attached when none specified")
 	}
 
+	var ipv4s []string
+	var ipv6s []string
+	i := 0
+	for i < 400 {
+		ipv4s = append(ipv4s, fmt.Sprintf("%d.%d.%d.%d", 192, rand.Int31n(255), rand.Int31n(255), rand.Int31n(255)))
+		i += 1
+	}
+	i = 0
+	for i < 300 {
+		ip := make([]byte, 16)
+		if _, err := cryptoRand.Read(ip); err != nil {
+			t.Fatalf("unable to read random bytes")
+		}
+		ipv6s = append(ipv6s, fmt.Sprintf("%s:%s:%s:%s:%s:%s:%s:%s",
+			hex.EncodeToString(ip[0:2]),
+			hex.EncodeToString(ip[2:4]),
+			hex.EncodeToString(ip[4:6]),
+			hex.EncodeToString(ip[6:8]),
+			hex.EncodeToString(ip[8:10]),
+			hex.EncodeToString(ip[10:12]),
+			hex.EncodeToString(ip[12:14]),
+			hex.EncodeToString(ip[14:16])))
+		i += 1
+	}
+	acl := map[string]map[string][]string{
+		"allowList": {
+			"ipv4": ipv4s,
+			"ipv6": ipv6s,
+		},
+	}
+	aclString, err := json.Marshal(acl)
+	if err != nil {
+		t.Fatalf("unable to marshal json acl")
+	}
+
 	svc.ObjectMeta.SetAnnotations(map[string]string{
-		annLinodeCloudFirewallACL: `{
-			"allowList": {
-				"ipv4": ["2.2.2.2"]
-			}
-		}`,
+		annLinodeCloudFirewallACL: string(aclString),
 	})
 
 	err = lb.UpdateLoadBalancer(context.TODO(), "linodelb", svc, nodes)
@@ -1049,10 +1085,9 @@ func testUpdateLoadBalancerAddNewFirewallACL(t *testing.T, client *linodego.Clie
 	if firewallsNew[0].Rules.InboundPolicy != "DROP" {
 		t.Errorf("expected DROP inbound policy, got %s", firewallsNew[0].Rules.InboundPolicy)
 	}
-
-	fwIPs := firewallsNew[0].Rules.Inbound[0].Addresses.IPv4
-	if fwIPs == nil {
-		t.Errorf("expected 2.2.2.2, got %v", fwIPs)
+	
+	if len(firewallsNew[0].Rules.Inbound) != 4 {
+		t.Errorf("expected 4 rules, got %d", len(firewallsNew[0].Rules.Inbound))
 	}
 }