From db2261465ae8f66dca71ae83b60ef8bc1a23bb4e Mon Sep 17 00:00:00 2001
From: Alvaro Aleman <alvaroaleman@users.noreply.github.com>
Date: Tue, 25 Aug 2020 18:33:42 -0400
Subject: [PATCH] :bug: Controller: Return error when started more than once

Starting the controller more than once is always wrong. Today, it will
result in:
* Its active workqueue being re-created, resulting in a data race,
  breaking the guarantee of "there is at most one worker on a given
  queue key" and messed-up metrics
* Starting double as many workers as configured

This PR changes that to instead return an error on all subsequent Start
attempts.
---
 pkg/internal/controller/controller.go      |  4 ++++
 pkg/internal/controller/controller_test.go | 11 +++++++++++
 2 files changed, 15 insertions(+)

diff --git a/pkg/internal/controller/controller.go b/pkg/internal/controller/controller.go
index ecfc6d9560..3a22ef6467 100644
--- a/pkg/internal/controller/controller.go
+++ b/pkg/internal/controller/controller.go
@@ -18,6 +18,7 @@ package controller
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"sync"
 	"time"
@@ -126,6 +127,9 @@ func (c *Controller) Start(stop <-chan struct{}) error {
 	// use an IIFE to get proper lock handling
 	// but lock outside to get proper handling of the queue shutdown
 	c.mu.Lock()
+	if c.Started {
+		return errors.New("controller was started more than once. This is likely to be caused by being added to a manager multiple times")
+	}
 
 	c.Queue = c.MakeQueue()
 	defer c.Queue.ShutDown() // needs to be outside the iife so that we shutdown after the stop channel is closed
diff --git a/pkg/internal/controller/controller_test.go b/pkg/internal/controller/controller_test.go
index e4e0228976..c437604b64 100644
--- a/pkg/internal/controller/controller_test.go
+++ b/pkg/internal/controller/controller_test.go
@@ -162,6 +162,17 @@ var _ = Describe("controller", func() {
 			close(stopped)
 			Expect(ctrl.Start(stopped)).To(Equal(err))
 		})
+
+		It("should return an error if it gets started more than once", func() {
+			// Use a stopped channel so Start doesn't block
+			stopped := make(chan struct{})
+			close(stopped)
+			Expect(ctrl.Start(stopped)).To(BeNil())
+			err := ctrl.Start(stopped)
+			Expect(err).NotTo(BeNil())
+			Expect(err.Error()).To(Equal("controller was started more than once. This is likely to be caused by being added to a manager multiple times"))
+		})
+
 	})
 
 	Describe("Watch", func() {