Handle firewall rule creation for load balancers #70

ghubcoder · 2018-04-01T11:21:22Z

This adds the ability for digitalocean-cloud-controller-manager to inspect the droplets which are part of a newly created load balancer and check whether they have an attached firewall.

If a firewall is present, it then checks to see if a firewall rule is required to allow the load balancer to communicate with the droplet on the target port.

Upon deletion of the load balancer, the reverse happens and the firewall rules are removed.

I've tested this and it does appear to work. I think perhaps the firewall function mocks need pulling out into their own file and more tests need adding.

There also needs to be some cleanup of the error handling/logging messages. Would be good to see if this works for anyone else.

cagedmantis · 2018-04-12T21:04:12Z

Hey @ghubcoder apologies for the delay, I've been getting ramped up on the controller code. Taking a look now.

cagedmantis · 2018-04-16T14:25:30Z

do/loadbalancers.go

+	var ruleExists bool
+	var rulesToAdd []firewallRequest
+	var alreadyAdded bool
+	//var completedFirewalls []string


Remove unused variable

cagedmantis · 2018-04-16T15:48:11Z

do/loadbalancers.go

+// the load balancer.
+//
+// EnsureFirewall will not modify service or nodes.
+func (l *loadbalancers) EnsureFirewall(lb *godo.LoadBalancer) error {


The function name isn't really describing what this function does. It's ensuring that the firewall rules allow a load balancer. Not that a firewall exists.

cagedmantis · 2018-04-16T15:48:40Z

do/loadbalancers.go

+	var alreadyAdded bool
+	//var completedFirewalls []string
+
+	for _, dropletid := range lb.DropletIDs {


Variable should be dropletID

cagedmantis · 2018-04-16T15:57:54Z

do/loadbalancers.go

+			return fmt.Errorf("Error listing firewalls for droplet %d", dropletid)
+		}
+
+		if len(firewalls) > 0 {


if len(firewalls) == 0 { continue } would reduce nesting.

cagedmantis · 2018-04-16T16:01:21Z

do/loadbalancers.go

@@ -179,6 +184,126 @@ func (l *loadbalancers) EnsureLoadBalancer(clusterName string, service *v1.Servi

 }

+func checkIfPortInRange(port int, fwPortRange string) bool {
+
+	fwPortList := strings.Split(fwPortRange, "-")


Create one function that convert the port range into two integers and use the two integers as arguments for these helper functions.

I had a look at this, the only issue is that the first value can sometimes be a string containing "all", rather than just two integers. Need to think about how to clean this up.

kind : Service
apiVersion : v1
metadata :
name : bonjour
annotations :
service.beta.kubernetes.io/do-loadbalancer-certificate-id : " 1234-5678-9012-3456 "
service.beta.kubernetes.io/do-loadbalancer- protocole : " https "
# Décommentez une fois que hello.example.com pointe vers l'adresse IP externe de l'équilibreur de charge DO.
# service.beta.kubernetes.io/do-loadbalancer-hostname: "hello.example.com"
spec :
type : LoadBalancer
sélecteur :
app : hello
ports :
- Nom : https
protocole : TCP
Port : 443
targetPort : 80

apiVersion : apps / v1
kind : Métadonnées de déploiement
:
nom : bonjour spec :
répliques : 3 sélecteur :
matchLabels :
app : bonjour modèle :
metadata :
labels :
app : bonjour spec :
conteneurs :

- nom : bonjour image : ports snormore / bonjour : - protocole containerPort : 80 : TCP

cagedmantis · 2018-04-16T16:05:22Z

do/loadbalancers.go

 	_, err = l.client.LoadBalancers.Delete(ctx, lb.ID)
 	return err
 }

+func (l *loadbalancers) EnsureFirewallDeleted(lb *godo.LoadBalancer) error {


Function name is misleading. Perhaps FirewallRuleDeleted.

cagedmantis · 2018-04-16T16:12:32Z

do/loadbalancers.go

+				//check each firewall to see if rule already exists
+				for _, firewall := range firewalls {
+					for _, fwInboundRule := range firewall.InboundRules {
+						for _, fwSourcelbs := range fwInboundRule.Sources.LoadBalancerUIDs {


This logic can be broken up into functions. Checking if the rule exists, deleting the rule, creating the rule.

cagedmantis · 2018-04-16T16:12:48Z

do/loadbalancers.go

+			return fmt.Errorf("Error listing firewalls for droplet %d", dropletid)
+		}
+
+		if len(firewalls) > 0 {


Similar comments to the ones noted above.

cagedmantis · 2018-04-16T16:14:17Z

It would make things cleaner if they were moved to a firewall.go file.

cagedmantis · 2018-04-16T16:14:48Z

Thanks for the submission. I'm excited to use this.

ghubcoder · 2018-04-18T16:27:57Z

@cagedmantis thanks for this, I'll take a look at making the changes.

ghubcoder · 2018-04-19T08:47:31Z

do/firewalls.go

+
+// Check if a single port rule exists for a load balancer
+// forwarding rule ( will not remove ranges defined by ports or ALL )
+func lbfwRuleRequiresDeletion(lbID string, lbRule godo.ForwardingRule, firewalls []godo.Firewall) bool {


This function is pretty much the same as lbfwRuleExists, only difference is it makes a call to checkIfPortRequiresDeletion, rather than checkIfPortInRange. These are slightly different as we only delete a port if it's not included in a range of ports. Whereas we only add a port if it's not contained within a list of ranges.

These two functions (lbfwRuleExists, lbfwRuleRequiresDeletion) could be replaced by one function, and the checking function required could be passed in as a parameter. Although I'm not sure if that is good practice.

ghubcoder · 2018-04-19T08:50:52Z

I've tested it once more, seems to work ok after the changes. Need to think about adding some tests now for the firewalls.go code.

timoreimann · 2018-11-30T23:05:56Z

I wonder what the status of this PR is? @ghubcoder, seems like you added the remaining tests, is that correct?

pritpal-sabharwal · 2019-02-26T22:41:04Z

For what it is worth, I have merged this PR locally and am running it on a K8S cluster. It seems to work. Creating a Service creates a LB and updates the Firewalls for the relevant droplets. Upon deletion of the service the LB is deleted and the Firewall rules are removed too.

Not sure if there is anything else left to do here to get this over the line.

ghubcoder · 2019-03-30T17:15:08Z

@pritpal-sabharwal thanks for giving this a try, very much appreciated !

timoreimann · 2020-07-26T08:24:19Z

I'm going to close this PR. Apart from the question of whether there's still interest, we have pending PR #332 to take a different approach to opening up and closing down ports dynamically through a dedicated, managed firewall. While the PR targets NodePorts, theoretically it can be extended for LB ports as well.

ghubcoder added 3 commits April 1, 2018 09:41

Initial commit

9f2cdbb

Delete firewall rules before lb

3105856

Formatting

61372ec

ghubcoder changed the title ~~Handle firelwall rule creation for load balancers~~ Handle firewall rule creation for load balancers Apr 1, 2018

Resolve vet hints, remove log statements

db9eb38

ghubcoder mentioned this pull request Apr 1, 2018

Feature request - Hooking into firewalls when creating load balancers #68

Closed

Simulate listing and adding rule

251a856

cagedmantis reviewed Apr 16, 2018

View reviewed changes

Refactor, move code into firewalls file, rename function names

3a8ba7b

ghubcoder commented Apr 19, 2018

View reviewed changes

ghubcoder added 3 commits April 19, 2018 22:43

Remove duplicate code, pass check func in as paramter

2fce66a

Adding initial set of tests

6500c2e

Add test for requestExists

bf50035

lxfontes added the sprint-next label Sep 10, 2018

timoreimann mentioned this pull request Nov 30, 2018

Firewall Controller #86

Closed

timoreimann removed the sprint-next label Feb 13, 2019

timoreimann mentioned this pull request Jun 9, 2019

Preserve client IP address on both HTTP and HTTPS #144

Closed

timoreimann closed this Jul 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle firewall rule creation for load balancers #70

Handle firewall rule creation for load balancers #70

ghubcoder commented Apr 1, 2018

cagedmantis commented Apr 12, 2018

cagedmantis Apr 16, 2018

cagedmantis Apr 16, 2018

cagedmantis Apr 16, 2018

cagedmantis Apr 16, 2018

cagedmantis Apr 16, 2018

ghubcoder Apr 19, 2018

Sandradeb5 Dec 26, 2019

cagedmantis Apr 16, 2018

cagedmantis Apr 16, 2018

cagedmantis Apr 16, 2018

cagedmantis commented Apr 16, 2018

cagedmantis commented Apr 16, 2018

ghubcoder commented Apr 18, 2018

ghubcoder Apr 19, 2018 •

edited

ghubcoder commented Apr 19, 2018

timoreimann commented Nov 30, 2018

pritpal-sabharwal commented Feb 26, 2019

ghubcoder commented Mar 30, 2019

timoreimann commented Jul 26, 2020

Handle firewall rule creation for load balancers #70

Handle firewall rule creation for load balancers #70

Conversation

ghubcoder commented Apr 1, 2018

cagedmantis commented Apr 12, 2018

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

cagedmantis commented Apr 16, 2018

cagedmantis commented Apr 16, 2018

ghubcoder commented Apr 18, 2018

ghubcoder Apr 19, 2018 • edited

Choose a reason for hiding this comment

ghubcoder commented Apr 19, 2018

timoreimann commented Nov 30, 2018

pritpal-sabharwal commented Feb 26, 2019

ghubcoder commented Mar 30, 2019

timoreimann commented Jul 26, 2020

ghubcoder Apr 19, 2018 •

edited