🐛 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.

Author: alvaroaleman

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

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() {