add controller name interface to all controllers (#85)

pkg/controller/common/cleaner.go

@@ -7,6 +7,7 @@ import (
 	"math"
 	"time"
 
+	"github.com/Azure/aks-app-routing-operator/pkg/controller/controllername"
 	"github.com/Azure/aks-app-routing-operator/pkg/controller/metrics"
 	"github.com/go-logr/logr"
 	"github.com/hashicorp/go-multierror"
@@ -23,7 +24,7 @@ import (
 )
 
 type cleaner struct {
-	name       string
+	name       controllername.ControllerNamer
 	mapper     meta.RESTMapper
 	clientset  kubernetes.Interface
 	dynamic    dynamic.Interface
@@ -33,7 +34,7 @@ type cleaner struct {
 }
 
 // NewCleaner creates a cleaner that attempts to delete resources with the labels specified and of the types returned by CleanTypeRetriever
-func NewCleaner(manager ctrl.Manager, name string, gvrRetriever CleanTypeRetriever) error {
+func NewCleaner(manager ctrl.Manager, name controllername.ControllerNamer, gvrRetriever CleanTypeRetriever) error {
 	d, err := dynamic.NewForConfig(manager.GetConfig())
 	if err != nil {
 		return fmt.Errorf("creating dynamic client: %w", err)
@@ -53,13 +54,13 @@ func NewCleaner(manager ctrl.Manager, name string, gvrRetriever CleanTypeRetriev
 		name:       name,
 		mapper:     mapper,
 		dynamic:    d,
-		logger:     manager.GetLogger().WithName(name),
+		logger:     name.AddToLogger(manager.GetLogger()),
 		clientset:  cs,
 		retriever:  gvrRetriever,
 		maxRetries: 2,
 	}
 
-	metrics.InitControllerMetrics(c.controllerName())
+	metrics.InitControllerMetrics(name)
 	return manager.Add(c)
 }
 
@@ -96,7 +97,7 @@ func (c *cleaner) Clean(ctx context.Context) error {
 		//this makes sure they have the proper value
 		//just calling defer metrics.HandleControllerReconcileMetrics(controllerName, result, err) would bind
 		//the values of result and err to their zero values, since they were just instantiated
-		metrics.HandleControllerReconcileMetrics(c.controllerName(), ctrl.Result{}, err)
+		metrics.HandleControllerReconcileMetrics(c.name, ctrl.Result{}, err)
 	}()
 	if c.retriever == nil {
 		err = errors.New("retriever is nil")
@@ -183,10 +184,6 @@ func (c *cleaner) NeedLeaderElection() bool {
 	return true
 }
 
-func (c *cleaner) controllerName() string {
-	return c.name
-}
-
 func isNamespaced(clientset kubernetes.Interface, gvr schema.GroupVersionResource) (bool, error) {
 	res, err := clientset.Discovery().ServerResourcesForGroupVersion(gvr.GroupVersion().String())
 	if err != nil {

pkg/controller/common/cleaner_test.go

@@ -5,6 +5,7 @@ import (
 	"reflect"
 	"testing"
 
+	"github.com/Azure/aks-app-routing-operator/pkg/controller/controllername"
 	"github.com/go-logr/logr"
 	"github.com/stretchr/testify/require"
 	"k8s.io/apimachinery/pkg/api/errors"
@@ -188,7 +189,6 @@ func TestCleanType(t *testing.T) {
 		}
 
 		c := &cleaner{
-			name:      "test-name",
 			dynamic:   d,
 			clientset: fakeClientset(),
 			logger:    logr.Discard(),
@@ -242,7 +242,7 @@ func AssertAction(t *testing.T, got []k8stesting.Action, expected k8stesting.Act
 func TestNewCleaner(t *testing.T) {
 	m, err := manager.New(restConfig, manager.Options{MetricsBindAddress: "0"})
 	require.NoError(t, err)
-	err = NewCleaner(m, "test-new-cleaner", RetrieverEmpty())
+	err = NewCleaner(m, controllername.New("test"), RetrieverEmpty())
 	require.NoError(t, err)
 }
 
@@ -266,7 +266,7 @@ func TestClean(t *testing.T) {
 		{
 			name: "nil retriver",
 			c: &cleaner{
-				name:      "test-name",
+				name:      controllername.New("test"),
 				dynamic:   d,
 				clientset: fakeClientset(),
 				logger:    logr.Discard(),
@@ -276,7 +276,7 @@ func TestClean(t *testing.T) {
 		{
 			name: "runs clean without err",
 			c: &cleaner{
-				name:      "test-name",
+				name:      controllername.New("test"),
 				dynamic:   d,
 				clientset: fakeClientset(),
 				logger:    logr.Discard(),
@@ -315,7 +315,7 @@ func TestCleanerStart(t *testing.T) {
 		{
 			name: "failing to start",
 			c: &cleaner{
-				name:       "test-name",
+				name:       controllername.New("test"),
 				dynamic:    d,
 				clientset:  fakeClientset(),
 				logger:     logr.Discard(),
@@ -326,7 +326,7 @@ func TestCleanerStart(t *testing.T) {
 		{
 			name: "starts cleaner",
 			c: &cleaner{
-				name:       "test-name",
+				name:       controllername.New("test"),
 				dynamic:    d,
 				clientset:  fakeClientset(),
 				logger:     logr.Discard(),

pkg/controller/common/resource_reconciler.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"time"
 
+	"github.com/Azure/aks-app-routing-operator/pkg/controller/controllername"
 	"github.com/Azure/aks-app-routing-operator/pkg/controller/metrics"
 	"github.com/Azure/aks-app-routing-operator/pkg/util"
 	"github.com/go-logr/logr"
@@ -13,20 +14,20 @@ import (
 )
 
 type resourceReconciler struct {
-	name                    string
+	name                    controllername.ControllerNamer
 	client                  client.Client
 	logger                  logr.Logger
 	interval, retryInterval time.Duration
 	resources               []client.Object
 }
 
 // NewResourceReconciler creates a reconciler that continuously ensures that the provided resources are provisioned
-func NewResourceReconciler(manager ctrl.Manager, name string, resources []client.Object, reconcileInterval time.Duration) error {
+func NewResourceReconciler(manager ctrl.Manager, name controllername.ControllerNamer, resources []client.Object, reconcileInterval time.Duration) error {
 	metrics.InitControllerMetrics(name)
 	rr := &resourceReconciler{
 		name:          name,
 		client:        manager.GetClient(),
-		logger:        manager.GetLogger().WithName(name),
+		logger:        name.AddToLogger(manager.GetLogger()),
 		interval:      reconcileInterval,
 		retryInterval: time.Second,
 		resources:     resources,

pkg/controller/common/resource_reconciler_test.go

@@ -5,6 +5,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/Azure/aks-app-routing-operator/pkg/controller/controllername"
 	"github.com/Azure/aks-app-routing-operator/pkg/controller/metrics"
 	"github.com/Azure/aks-app-routing-operator/pkg/controller/testutils"
 	"github.com/go-logr/logr"
@@ -21,7 +22,7 @@ func TestResourceReconcilerEmpty(t *testing.T) {
 	c := fake.NewClientBuilder().Build()
 
 	rr := &resourceReconciler{
-		name:      "test-name",
+		name:      controllername.New("test", "resource", "reconciler"),
 		client:    c,
 		logger:    logr.Discard(),
 		resources: []client.Object{},
@@ -48,7 +49,7 @@ func TestResourceReconcilerIntegration(t *testing.T) {
 	}
 
 	rr := &resourceReconciler{
-		name:      "test-name",
+		name:      controllername.New("test"),
 		client:    c,
 		logger:    logr.Discard(),
 		resources: []client.Object{obj},
@@ -97,7 +98,7 @@ func TestResourceReconcilerLeaderElection(t *testing.T) {
 func TestNewResourceReconciler(t *testing.T) {
 	m, err := manager.New(restConfig, manager.Options{MetricsBindAddress: "0"})
 	require.NoError(t, err)
-	err = NewResourceReconciler(m, "test-rr", nil, 1*time.Nanosecond)
+	err = NewResourceReconciler(m, controllername.New("test"), nil, 1*time.Nanosecond)
 	require.NoError(t, err)
 }
 
@@ -118,7 +119,7 @@ func TestResourceReconciler_DeletionTimestamp(t *testing.T) {
 	c := fake.NewClientBuilder().WithObjects(obj).Build()
 
 	rr := &resourceReconciler{
-		name:      "test-name",
+		name:      controllername.New("test", "name"),
 		client:    c,
 		logger:    logr.Discard(),
 		resources: []client.Object{obj},
@@ -155,7 +156,7 @@ func TestResourceReconciler_Start(t *testing.T) {
 	c := fake.NewClientBuilder().WithObjects(obj).Build()
 
 	rr := &resourceReconciler{
-		name:      "test-name",
+		name:      controllername.New("test", "name"),
 		client:    c,
 		logger:    logr.Discard(),
 		resources: []client.Object{obj},

pkg/controller/controllername/controllername.go

@@ -3,37 +3,49 @@ package controllername
 import (
 	"strings"
 	"unicode"
+
+	"github.com/go-logr/logr"
 )
 
 const (
 	metricsNameDelimiter = "_"
 	loggerNameDelimiter  = "-"
 )
 
+// ControllerNamer is an interface that returns the name of the controller in all necessary forms
 type ControllerNamer interface {
+	// String returns the name of the controller in a human-readable form
+	String() string
+	// MetricsName returns the name of the controller in a form that can be used for Prometheus metrics, specifically as a Prometheus label https://prometheus.io/docs/practices/naming/#labels
 	MetricsName() string
+	// LoggerName returns the name of the controller in a form that can be used for logr logger naming
 	LoggerName() string
+	// AddToLogger adds controller name fields to the logger then returns the logger with the added fields
+	AddToLogger(l logr.Logger) logr.Logger
 }
 
 // controllerName ex. {"My","Controller", "Name"} ->  MyControllerName
 type controllerName []string
 
-func NewControllerName(name []string) controllerName {
-	cn := make(controllerName, len(name))
-
-	for i, w := range name {
-		cn[i] = strip(strings.ToLower(w))
-
+// New returns a new controllerName after taking each word of the controller name as a separate argument
+func New(firstWord string, additionalWords ...string) controllerName { // using a non-variadic for the first word makes it impossible to accidentally pass no arguments in. Accepting variadic versus slices also helps with not accepting empty slices and is easier to use
+	cn := make(controllerName, 0, len(additionalWords)+1)
+	for _, w := range append([]string{firstWord}, additionalWords...) {
+		cleaned := clean(w)
+		if cleaned != "" {
+			cn = append(cn, cleaned)
+		}
 	}
+
 	return cn
 }
 
-// Strip removes spaces and non letters
-func strip(s string) string {
+// clean removes spaces and non letters and lowercases
+func clean(s string) string {
 	rr := make([]rune, 0, len(s))
 	for _, r := range s {
 		if unicode.IsLetter(r) {
-			rr = append(rr, r)
+			rr = append(rr, unicode.ToLower(r))
 		}
 	}
 	return string(rr)
@@ -46,3 +58,14 @@ func (c controllerName) MetricsName() string {
 func (c controllerName) LoggerName() string {
 	return strings.Join(c, loggerNameDelimiter)
 }
+
+func (c controllerName) String() string {
+	return strings.Join(c, " ")
+}
+
+func (c controllerName) AddToLogger(l logr.Logger) logr.Logger {
+	return l.
+		WithName(c.LoggerName()).
+		WithValues("controller", c.String()).
+		WithValues("controllerMetricsName", c.MetricsName()) // include metrics name, so we can automate creating queries that check Logs based on alerts
+}

pkg/controller/controllername/controllername_test.go

@@ -1,19 +1,20 @@
 package controllername
 
 import (
-	"github.com/stretchr/testify/require"
 	"regexp"
 	"testing"
+
+	"github.com/stretchr/testify/require"
 )
 
 func TestMetricsName(t *testing.T) {
 
-	cn1 := NewControllerName([]string{"SomeFakeControllerName"})
-	cn2 := NewControllerName([]string{"Some", "Controller", "Name"})
-	cn3 := NewControllerName([]string{" SomeName", "Entered  ", "poorly"})
-	cn4 := NewControllerName([]string{"Some Spaces"})
-	cn5 := NewControllerName([]string{"Too  Many       Spaces"})
-	cn6 := NewControllerName([]string{"special!@characters"})
+	cn1 := New("SomeFakeControllerName")
+	cn2 := New("Some", "Controller", "Name")
+	cn3 := New(" SomeName", "Entered  ", "poorly")
+	cn4 := New("Some Spaces")
+	cn5 := New("Too  Many       Spaces")
+	cn6 := New("special!@characters")
 
 	metricName1 := cn1.MetricsName()
 	metricName2 := cn2.MetricsName()
@@ -32,12 +33,12 @@ func TestMetricsName(t *testing.T) {
 }
 
 func TestLoggerName(t *testing.T) {
-	cn1 := NewControllerName([]string{"SomeFakeControllerName"})
-	cn2 := NewControllerName([]string{"Some", "Controller", "Name"})
-	cn3 := NewControllerName([]string{" SomeName", "Entered  ", "poorly"})
-	cn4 := NewControllerName([]string{"Some Spaces"})
-	cn5 := NewControllerName([]string{"Too  Many       Spaces"})
-	cn6 := NewControllerName([]string{"special!@characters"})
+	cn1 := New("SomeFakeControllerName")
+	cn2 := New("Some", "Controller", "Name")
+	cn3 := New(" SomeName", "Entered  ", "poorly")
+	cn4 := New("Some Spaces")
+	cn5 := New("Too  Many       Spaces")
+	cn6 := New("special!@characters")
 
 	loggerName1 := cn1.LoggerName()
 	loggerName2 := cn2.LoggerName()
@@ -52,25 +53,60 @@ func TestLoggerName(t *testing.T) {
 	require.True(t, isBestPracticeLoggerName(loggerName4))
 	require.True(t, isBestPracticeLoggerName(loggerName5))
 	require.True(t, isBestPracticeLoggerName(loggerName6))
+}
 
+func TestControllerNameString(t *testing.T) {
+	tests := []struct {
+		name     string
+		input    []string
+		expected string
+	}{
+		{
+			name:     "single word",
+			input:    []string{"SomeFakeControllerName"},
+			expected: "somefakecontrollername",
+		},
+		{
+			name:     "multiple words",
+			input:    []string{"Some", "Controller", "Name"},
+			expected: "some controller name",
+		},
+		{
+			name:     "multiple words with spaces",
+			input:    []string{" SomeName", "Entered  ", "poorly"},
+			expected: "somename entered poorly",
+		},
+		{
+			name:     "multiple words with characters",
+			input:    []string{"special!@characters", "and", "numbers123"},
+			expected: "specialcharacters and numbers",
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			cn := New(test.input[0], test.input[1:]...)
+			require.Equal(t, test.expected, cn.String())
+		})
+	}
 }
 
 func TestStrip(t *testing.T) {
 	str := "a *&b   c "
-	striped := strip(str)
+	striped := clean(str)
 
 	require.Equal(t, striped, "abc")
 }
 
-// IsPrometheusBestPracticeName - function returns true if the name given matches best practices for prometheus name, i.e. snake_case
+// isPrometheusBestPracticeName - function returns true if the name given matches best practices for prometheus name, i.e. snake_case
 func isPrometheusBestPracticeName(controllerName string) bool {
 	pattern := "^[a-z]+(_[a-z]+)*$"
 	match, _ := regexp.MatchString(pattern, controllerName)
 
 	return match
 }
 
-// IsBestPracticeLoggerName - function returns true if the name given matches best practices for prometheus name, i.e. kebab-case
+// isBestPracticeLoggerName - function returns true if the name given matches best practices for prometheus name, i.e. kebab-case
 func isBestPracticeLoggerName(controllerName string) bool {
 	pattern := "^[a-z]+(-[a-z]+)*$"
 	match, _ := regexp.MatchString(pattern, controllerName)