Add feature to update / delete attached firewall #171

rammanoj · 2024-01-21T05:56:24Z

Fixes #146

Changes:

Add the below three features while updating the node balancer:
- Attach New Firewall
- Update Attached Firewall
- Delete Attached Firewall
Upgrade linodego to 1.26

General:

Have you removed all sensitive information, including but not limited to access keys and passwords?
Have you checked to ensure there aren't other open or closed Pull Requests for the same bug/feature/question?

Pull Request Guidelines:

Does your submission pass tests?
Have you added tests?
Are you addressing a single feature in this PR?
Are your commits atomic, addressing one change per commit?
Are you following the conventions of the language?
Have you saved your large formatting changes for a different PR, so we can focus on your work?
Have you explained your rationale for why this feature is needed?
Have you linked your PR to an open issue

tchinmai7 · 2024-01-22T16:01:52Z

cloud/linode/loadbalancers.go

@@ -229,6 +229,90 @@ func (l *loadbalancers) EnsureLoadBalancer(ctx context.Context, clusterName stri
 	return lbStatus, nil
 }

+func (l *loadbalancers) getNodeBalancerDeviceId(ctx context.Context, firewallID, nbID, page int) (int, bool, error) {
+	devices, err := l.client.ListFirewallDevices(ctx, firewallID, &linodego.ListOptions{PageSize: 500, PageOptions: &linodego.PageOptions{Page: page}})


I don't think you need to handle pagination manually here - linodego abstracts it from you

Though the output is []linodego.FirewallDevice. The PageOptions is passed as an input parameter. The max. page size is 500. If there are more than 500 items, this might be useful.

As discussed, for the sake of simplicity, this could be skipped with just a single call rather than recursive set of calls.

tchinmai7 · 2024-01-22T16:04:29Z

cloud/linode/loadbalancers.go

+	// get the attached firewall
+	firewalls, err := l.client.ListNodeBalancerFirewalls(ctx, nb.ID, &linodego.ListOptions{})
+	if err != nil {
+		if !ok {


can you add comments explaining this logic?

tchinmai7 · 2024-01-22T16:05:16Z

cloud/linode/loadbalancers.go

+	if existingFirewallID != newFirewallID {
+		// remove the existing firewall
+
+		if existingFirewallID != 0 {


when will existingFirewallID be 0?

The value is zero when there is no Firewall assigned. Default value of a integer variable in golang is 0. The existingFirewallID variable would only be tampered if there's a firewall attached to nodeBalancer. If not, it would be 0

ah I see the logic now. can you move the declaration for existingFirewallID to line 277 so its immediately obvious?

okokes-akamai · 2024-01-23T16:25:26Z

cloud/linode/fake_linode_test.go

@@ -186,6 +190,40 @@ func (f *fakeAPI) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 				_, _ = w.Write(rr)
 				return
 			}
+
+			rx, _ = regexp.Compile("/nodebalancers/[0-9]+/firewalls")


It's weird to ignore the error here - maybe just run MustCompile to ensure it passes.

okokes-akamai · 2024-01-23T16:31:29Z

cloud/linode/fake_linode_test.go

+							{Reason: "Not Found"},
+						},
+					}
+					rr, _ := json.Marshal(resp)


It's kinda weird that we check errors from strconv.Atoi and not these ones. They seem to be similarly likely to happen.

okokes-akamai · 2024-01-23T16:35:15Z

cloud/linode/fake_linode_test.go

@@ -24,6 +24,8 @@ type fakeAPI struct {
 	nb  map[string]*linodego.NodeBalancer
 	nbc map[string]*linodego.NodeBalancerConfig
 	nbn map[string]*linodego.NodeBalancerNode
+	fw  map[int]*linodego.Firewall
+	fwd map[int]map[int]*linodego.FirewallDevice


It's useful to add comments here to signal what the ints stand for

okokes-akamai · 2024-01-23T16:37:48Z

cloud/linode/loadbalancers.go

@@ -229,6 +229,103 @@ func (l *loadbalancers) EnsureLoadBalancer(ctx context.Context, clusterName stri
 	return lbStatus, nil
 }

+// getNodeBalancerDeviceId gets the deviceID of the nodeBalancer that is attached to the firewall.
+func (l *loadbalancers) getNodeBalancerDeviceId(ctx context.Context, firewallID, nbID, page int) (int, bool, error) {
+	devices, err := l.client.ListFirewallDevices(ctx, firewallID, &linodego.ListOptions{PageSize: 500, PageOptions: &linodego.PageOptions{Page: page}})


This is only called with Page: 1 - so when do we actually paginate?

okokes-akamai · 2024-01-23T16:40:03Z

cloud/linode/loadbalancers.go

+	firewalls, err := l.client.ListNodeBalancerFirewalls(ctx, nb.ID, &linodego.ListOptions{})
+	if err != nil {
+		if !ok {
+			if err.Error() != "[404] Not Found" {


This is a bit icky, can we maybe use the linodego.Error type assertion and its Code field?

okokes-akamai · 2024-01-23T16:40:40Z

cloud/linode/loadbalancers.go

@@ -258,6 +354,12 @@ func (l *loadbalancers) updateNodeBalancer(ctx context.Context, clusterName stri
 		}
 	}

+	// update node-balancer firewall


nit: this comment seems redundant

okokes-akamai · 2024-01-23T16:42:05Z

cloud/linode/loadbalancers_test.go

@@ -639,7 +651,7 @@ func testUpdateLoadBalancerAddTLSPort(t *testing.T, client *linodego.Client, _ *
 	}

 	if !reflect.DeepEqual(expectedPorts, observedPorts) {
-		t.Errorf("NodeBalancer ports mismatch: expected %v, got %v", expectedPorts, observedPorts)
+		t.Errorf("NodeBalancer ports mismatch: expected %v, got %v", `expectedPorts`, observedPorts)


Why are we passing a string literal to the formatter?

tchinmai7 · 2024-01-23T18:31:04Z

Merging now, will address comments in a followup PR

rpotla and others added 7 commits December 22, 2023 20:07

Firewall Update changes

5b39c2c

Updates

ab0375a

add tests

feb2f4c

Unittests

f112117

Final test cases

f017760

latest updates

f1647f8

Latest update fixes

bec346a

rammanoj marked this pull request as draft January 21, 2024 05:56

Fix linter

09e5d2e

rammanoj marked this pull request as ready for review January 21, 2024 20:02

tchinmai7 reviewed Jan 22, 2024

View reviewed changes

Address comments

5729c8a

tchinmai7 approved these changes Jan 23, 2024

View reviewed changes

okokes-akamai reviewed Jan 23, 2024

View reviewed changes

tchinmai7 merged commit 3a68362 into linode:main Jan 23, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add feature to update / delete attached firewall #171

Add feature to update / delete attached firewall #171

rammanoj commented Jan 21, 2024

tchinmai7 Jan 22, 2024

rammanoj Jan 22, 2024

tchinmai7 Jan 22, 2024

tchinmai7 Jan 22, 2024

rammanoj Jan 22, 2024

tchinmai7 Jan 22, 2024

okokes-akamai Jan 23, 2024

okokes-akamai Jan 23, 2024

okokes-akamai Jan 23, 2024

okokes-akamai Jan 23, 2024

okokes-akamai Jan 23, 2024

okokes-akamai Jan 23, 2024

okokes-akamai Jan 23, 2024

tchinmai7 commented Jan 23, 2024

Add feature to update / delete attached firewall #171

Add feature to update / delete attached firewall #171

Conversation

rammanoj commented Jan 21, 2024

General:

Pull Request Guidelines:

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

tchinmai7 commented Jan 23, 2024