Extract firewall management into separate controller #403

rramkumar1 · 2018-07-17T16:30:30Z

This PR attempts to move the management of the L7 firewall rule into a separate controller.

Summary of changes:

Add controller.go to pkg/firewalls. This new controller also creates and interacts with the firewall pool, rather than the ClusterManager. Currently, the controller watches ingresses and services and updates the firewall rule as necessary. In the future, we can extend this to support the firewall rules for expose NEG.
Remove all firewall related stuff from ClusterManager and pkg/controller/controller.go
Move IsHealthy from ClusterManager to the controller the health check is meant for. It really has notplace in ClusterManager.

This leads well into a followup PR which can remove the ClusterManager entirely. Open to discussion on some of the design decisions in the PR. /assign @nicksardo

rramkumar1 · 2018-07-17T16:31:34Z

/assign @nicksardo

rramkumar1 · 2018-07-17T16:39:35Z

Note: This does not compile because of an import cycle. Trying to fix that now, but should not affect review.

nicksardo · 2018-07-17T16:51:59Z

cmd/glbc/main.go

@@ -193,6 +200,9 @@ func runControllers(ctx *context.ControllerContext) {
 	ctx.Start(stopCh)
 	lbc.Run()


I believe this blocks...

Oops..moved start of fwc above lbc.Run()

nicksardo · 2018-07-17T21:41:10Z

pkg/utils/join.go

+// IngressesForObject gets Ingresses that are associated with the
+// passed in object. It is a wrapper around functions which do the actual
+// work of getting Ingresses based on a typed object.
+func (j *Joiner) IngressesForObject(obj interface{}) []*extensions.Ingress {


I'd suggest ditching the wrapper and exposing the ...ForService and ...ForBackendConfig funcs.

Ditched the wrapper

nicksardo · 2018-07-17T21:52:10Z

pkg/utils/join.go

+// NewJoiner returns a Joiner.
+func NewJoiner(
+	ingressInformer cache.SharedIndexInformer,
+	svcInformer cache.SharedIndexInformer,


Suggest passing in the listers instead of the full informers.

nicksardo · 2018-07-17T21:55:28Z

pkg/utils/join.go

+	svcInformer cache.SharedIndexInformer,
+	defaultBackendSvcPortID ServicePortID) *Joiner {
+	ingLister := StoreToIngressLister{Store: ingressInformer.GetStore()}
+	return &Joiner{ingLister, svcInformer, defaultBackendSvcPortID}


Actually, I'm not seeing a strong reason why Joiner needs to exist as a struct. Seems more flexible to make the last two funcs static and pass in the appropriate lister. Then a consuming controller isn't forced to provide all listers even if it only uses one.

So after trying the alternative, I think its much easier to read the code when Joiner exists as a struct. The reason is because the initalization of the struct will take care of storing all necessary things needed (listers, default backend service port ID) and then the function calls become nice and compact. The alternative is that each function call becomes really bloated with all the necessary arguments.

To alleviate the problem of sometimes passing in stuff you don't need, maybe the Joiner can just take a ControllerContext? That contains everything it needs.

nicksardo · 2018-07-17T21:59:16Z

pkg/utils/taskqueue.go

@@ -29,6 +29,7 @@ import (
 type TaskQueue interface {
 	Run(period time.Duration, stopCh <-chan struct{})
 	Enqueue(obj interface{})
+	EnqueueAll(objs []interface{})


Variadic func?

Enqueue(obj ...interface{})

nicksardo · 2018-07-17T22:29:16Z

pkg/firewalls/controller.go

+	ctx.ServiceInformer.AddEventHandler(cache.ResourceEventHandlerFuncs{
+		AddFunc: func(obj interface{}) {
+			ings := joiner.IngressesForObject(obj)
+			lbc.ingQueue.EnqueueAll(ings)


rramkumar1 · 2018-07-18T23:00:26Z

Realized there are some things I haven't thought about that I need to add to my implementation. Marking as "WIP" for now.

nicksardo · 2018-07-20T21:10:49Z

pkg/firewalls/controller.go

+func (fwc *FirewallController) Run(stopCh chan struct{}) {
+	defer fwc.shutdown()
+	go fwc.queue.Run(time.Second, stopCh)
+	<-stopCh


If Run(...) is supposed to block, is there any need to start the fwc.queue.Run(...) in a goroutine? Also, fwc.shutdown() calls firewallPool.Shutdown() which deletes the firewall. That should only be called when we have no ingresses/NEGs.

func (fwc *FirewallController) Run(stopCh chan struct{}) { fwc.queue.Run(time.Second, stopCh)

Removed firewallPool.Shutdown from the fwc.shutdown() call.

Now I think the behavior for all these shutdowns is correct. We only call shutdown on the firewall pool if there are no ingresses and we only shutdown the queue when the process itself is terminated

nicksardo · 2018-07-20T21:12:00Z

pkg/firewalls/controller.go

+	<-stopCh
+}
+
+func (fwc *FirewallController) shutdown() error {


Add a comment that this should only be called when we no longer need a firewall rule.

So actually the shutdown() on the controller should only consist of the queue being shutdown. Also, we should only call shutdown() on the controller when the overall process is being terminated right?

nicksardo · 2018-07-20T21:28:42Z

pkg/utils/join.go

+}
+
+// IngressesForBackendConfig gets all Ingresses that reference (indirectly) a BackendConfig.
+func (j *Joiner) IngressesForBackendConfig(beConfig *backendconfigv1beta1.BackendConfig) (ingList []*extensions.Ingress) {


Maybe add a low priority TODO note for optimizing the joining here.

nicksardo · 2018-07-20T22:14:56Z

/lgtm
Needs testing and then someone else can merge.

rramkumar1 · 2018-07-24T18:09:49Z

Made some small changes:

Added a util function to convert a list of ingresses into a list of interfaces{}. This ensures that we can use the now variadic Enqueue function
Made queueKey a pointer.

I tested this end-to-end and it works so it should be good to merge.

rramkumar1 · 2018-07-24T18:09:55Z

/assign @MrHohn

MrHohn · 2018-07-24T18:24:49Z

/lgtm

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 17, 2018

k8s-ci-robot assigned nicksardo Jul 17, 2018

rramkumar1 force-pushed the global-state-manager branch from af1b4d5 to bbbf955 Compare July 17, 2018 16:40

nicksardo reviewed Jul 17, 2018

View reviewed changes

rramkumar1 force-pushed the global-state-manager branch from bbbf955 to b9d5a35 Compare July 17, 2018 17:05

nicksardo reviewed Jul 17, 2018

View reviewed changes

rramkumar1 force-pushed the global-state-manager branch 3 times, most recently from 0b8359f to 63bd9a7 Compare July 18, 2018 21:48

rramkumar1 changed the title ~~Extract firewall management into separate controller~~ Extract firewall management into separate controller [WIP Jul 18, 2018

rramkumar1 changed the title ~~Extract firewall management into separate controller [WIP~~ Extract firewall management into separate controller [WIP] Jul 18, 2018

rramkumar1 force-pushed the global-state-manager branch from 63bd9a7 to d52021e Compare July 19, 2018 15:09

rramkumar1 changed the title ~~Extract firewall management into separate controller [WIP]~~ Extract firewall management into separate controller Jul 19, 2018

rramkumar1 force-pushed the global-state-manager branch 7 times, most recently from 2dcca64 to dc8f7bd Compare July 19, 2018 16:58

nicksardo reviewed Jul 20, 2018

View reviewed changes

rramkumar1 force-pushed the global-state-manager branch 2 times, most recently from 01eec3f to d17223f Compare July 20, 2018 22:00

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 20, 2018

Extract firewall management into separate controller

ebaf600

rramkumar1 force-pushed the global-state-manager branch from d17223f to ebaf600 Compare July 24, 2018 18:08

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 24, 2018

k8s-ci-robot assigned MrHohn Jul 24, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 24, 2018

MrHohn merged commit b70b7a2 into kubernetes:master Jul 24, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract firewall management into separate controller #403

Extract firewall management into separate controller #403

rramkumar1 commented Jul 17, 2018 •

edited

Loading

rramkumar1 commented Jul 17, 2018

rramkumar1 commented Jul 17, 2018

nicksardo Jul 17, 2018

rramkumar1 Jul 17, 2018 •

edited

Loading

nicksardo Jul 17, 2018

rramkumar1 Jul 18, 2018

nicksardo Jul 17, 2018

rramkumar1 Jul 18, 2018

nicksardo Jul 17, 2018

rramkumar1 Jul 18, 2018

rramkumar1 Jul 18, 2018

nicksardo Jul 17, 2018

rramkumar1 Jul 18, 2018

nicksardo Jul 17, 2018

rramkumar1 Jul 18, 2018

rramkumar1 commented Jul 18, 2018

nicksardo Jul 20, 2018

rramkumar1 Jul 20, 2018

rramkumar1 Jul 20, 2018

nicksardo Jul 20, 2018

rramkumar1 Jul 20, 2018

nicksardo Jul 20, 2018

rramkumar1 Jul 20, 2018

nicksardo commented Jul 20, 2018

rramkumar1 commented Jul 24, 2018

rramkumar1 commented Jul 24, 2018

MrHohn commented Jul 24, 2018

		@@ -193,6 +200,9 @@ func runControllers(ctx *context.ControllerContext) {
		ctx.Start(stopCh)
		lbc.Run()

Extract firewall management into separate controller #403

Extract firewall management into separate controller #403

Conversation

rramkumar1 commented Jul 17, 2018 • edited Loading

rramkumar1 commented Jul 17, 2018

rramkumar1 commented Jul 17, 2018

Choose a reason for hiding this comment

rramkumar1 Jul 17, 2018 • edited Loading

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

rramkumar1 commented Jul 18, 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

nicksardo commented Jul 20, 2018

rramkumar1 commented Jul 24, 2018

rramkumar1 commented Jul 24, 2018

MrHohn commented Jul 24, 2018

rramkumar1 commented Jul 17, 2018 •

edited

Loading

rramkumar1 Jul 17, 2018 •

edited

Loading