Don't use builtin log package or glog

We should be consistenly using logr instead of the go standard library,
or glog (when in our code -- we don't have any control over code from
kubernetes core).

Most of this usage was `log.Fatal`, which we replace with calls to
`<log>.Error` and `os.Exit(1)`.

Author: DirectXMan12

pkg/admission/cert/writer/certwriter.go

@@ -23,7 +23,6 @@ import (
 	"encoding/pem"
 	"errors"
 	"fmt"
-	"log"
 	"net/url"
 	"time"
 
@@ -101,7 +100,7 @@ func handleCommon(webhook *admissionregistrationv1beta1.Webhook, ch certReadWrit
 	// Recreate the cert if it's invalid.
 	valid := validCert(certs, dnsName)
 	if !valid {
-		log.Printf("cert is invalid or expiring, regenerating a new one")
+		log.Info("cert is invalid or expiring, regenerating a new one")
 		certs, err = ch.overwrite(webhook.Name)
 		if err != nil {
 			return err

pkg/admission/cert/writer/doc.go

@@ -81,3 +81,9 @@ the desired path.
 
 */
 package writer
+
+import (
+	logf "sigs.k8s.io/controller-runtime/pkg/runtime/log"
+)
+
+var log = logf.KBLog.WithName("admission").WithName("cert").WithName("writer")

pkg/admission/cert/writer/fs.go

@@ -20,7 +20,6 @@ import (
 	"errors"
 	"fmt"
 	"io/ioutil"
-	"log"
 	"os"
 	"path"
 	"strings"
@@ -148,7 +147,7 @@ func (f *fsCertWriter) doWrite(webhookName string) (*certgenerator.Artifacts, er
 	if err != nil {
 		return nil, err
 	}
-	aw, err := atomic.NewAtomicWriter(v.path, fmt.Sprintf("processing webhook %q", webhookName))
+	aw, err := atomic.NewAtomicWriter(v.path, log.WithName("atomic-writer").WithValues("task", "processing webhook", "webhook", webhookName))
 	if err != nil {
 		return nil, err
 	}
@@ -168,14 +167,14 @@ func prepareToWrite(dir string) {
 		if os.IsNotExist(err) {
 			continue
 		} else if err != nil {
-			log.Printf("error stat file %v: %v", abspath, err)
+			log.Error(err, "unable to stat file", "file", abspath)
 		}
 		_, err = os.Readlink(abspath)
 		// if it's not a symbolic link
 		if err != nil {
 			err = os.Remove(abspath)
 			if err != nil {
-				log.Printf("failed to remove old file: %v", abspath)
+				log.Error(err, "unable to remove old file", "file", abspath)
 			}
 		}
 	}

pkg/admission/cert/writer/internal/atomic/atomic_writer.go

@@ -27,8 +27,7 @@ import (
 	"strings"
 	"time"
 
-	"github.com/golang/glog"
-
+	"github.com/go-logr/logr"
 	"k8s.io/apimachinery/pkg/util/sets"
 )
 
@@ -57,8 +56,8 @@ const (
 // inotify or fanotify to receive events when the content in the volume is
 // updated.
 type AtomicWriter struct {
-	targetDir  string
-	logContext string
+	targetDir string
+	log       logr.Logger
 }
 
 type FileProjection struct {
@@ -68,13 +67,13 @@ type FileProjection struct {
 
 // NewAtomicWriter creates a new AtomicWriter configured to write to the given
 // target directory, or returns an error if the target directory does not exist.
-func NewAtomicWriter(targetDir string, logContext string) (*AtomicWriter, error) {
+func NewAtomicWriter(targetDir string, log logr.Logger) (*AtomicWriter, error) {
 	_, err := os.Stat(targetDir)
 	if os.IsNotExist(err) {
 		return nil, err
 	}
 
-	return &AtomicWriter{targetDir: targetDir, logContext: logContext}, nil
+	return &AtomicWriter{targetDir: targetDir, log: log}, nil
 }
 
 const (
@@ -120,7 +119,7 @@ func (w *AtomicWriter) Write(payload map[string]FileProjection) error {
 	// (1)
 	cleanPayload, err := validatePayload(payload)
 	if err != nil {
-		glog.Errorf("%s: invalid payload: %v", w.logContext, err)
+		w.log.Error(err, "invalid payload")
 		return err
 	}
 
@@ -129,7 +128,7 @@ func (w *AtomicWriter) Write(payload map[string]FileProjection) error {
 	oldTsDir, err := os.Readlink(dataDirPath)
 	if err != nil {
 		if !os.IsNotExist(err) {
-			glog.Errorf("%s: error reading link for data directory: %v", w.logContext, err)
+			w.log.Error(err, "unable to read link for data directory")
 			return err
 		}
 		// although Readlink() returns "" on err, don't be fragile by relying on it (since it's not specified in docs)
@@ -144,49 +143,49 @@ func (w *AtomicWriter) Write(payload map[string]FileProjection) error {
 		// (3)
 		pathsToRemove, err = w.pathsToRemove(cleanPayload, oldTsPath)
 		if err != nil {
-			glog.Errorf("%s: error determining user-visible files to remove: %v", w.logContext, err)
+			w.log.Error(err, "unable to determine user-visible files to remove")
 			return err
 		}
 
 		// (4)
 		if should, err := shouldWritePayload(cleanPayload, oldTsPath); err != nil {
-			glog.Errorf("%s: error determining whether payload should be written to disk: %v", w.logContext, err)
+			w.log.Error(err, "unable to determine whether payload should be written to disk")
 			return err
 		} else if !should && len(pathsToRemove) == 0 {
-			glog.V(4).Infof("%s: no update required for target directory %v", w.logContext, w.targetDir)
+			w.log.V(1).Info("no update required for target directory", "directory", w.targetDir)
 			return nil
 		} else {
-			glog.V(4).Infof("%s: write required for target directory %v", w.logContext, w.targetDir)
+			w.log.V(1).Info("write required for target directory", "directory", w.targetDir)
 		}
 	}
 
 	// (5)
 	tsDir, err := w.newTimestampDir()
 	if err != nil {
-		glog.V(4).Infof("%s: error creating new ts data directory: %v", w.logContext, err)
+		w.log.Error(err, "error creating new ts data directory")
 		return err
 	}
 	tsDirName := filepath.Base(tsDir)
 
 	// (6)
 	if err = w.writePayloadToDir(cleanPayload, tsDir); err != nil {
-		glog.Errorf("%s: error writing payload to ts data directory %s: %v", w.logContext, tsDir, err)
+		w.log.Error(err, "unable to write payload to ts data directory", "ts directory", tsDir)
 		return err
 	} else {
-		glog.V(4).Infof("%s: performed write of new data to ts data directory: %s", w.logContext, tsDir)
+		w.log.V(1).Info("performed write of new data to ts data directory", "ts directory", tsDir)
 	}
 
 	// (7)
 	if err = w.createUserVisibleFiles(cleanPayload); err != nil {
-		glog.Errorf("%s: error creating visible symlinks in %s: %v", w.logContext, w.targetDir, err)
+		w.log.Error(err, "unable to create visible symlinks in target directory", "target directory", w.targetDir)
 		return err
 	}
 
 	// (8)
 	newDataDirPath := path.Join(w.targetDir, newDataDirName)
 	if err = os.Symlink(tsDirName, newDataDirPath); err != nil {
 		os.RemoveAll(tsDir)
-		glog.Errorf("%s: error creating symbolic link for atomic update: %v", w.logContext, err)
+		w.log.Error(err, "unable to create symbolic link for atomic update")
 		return err
 	}
 
@@ -201,20 +200,20 @@ func (w *AtomicWriter) Write(payload map[string]FileProjection) error {
 	if err != nil {
 		os.Remove(newDataDirPath)
 		os.RemoveAll(tsDir)
-		glog.Errorf("%s: error renaming symbolic link for data directory %s: %v", w.logContext, newDataDirPath, err)
+		w.log.Error(err, "unable to rename symbolic link for data directory", "data directory", newDataDirPath)
 		return err
 	}
 
 	// (10)
 	if err = w.removeUserVisiblePaths(pathsToRemove); err != nil {
-		glog.Errorf("%s: error removing old visible symlinks: %v", w.logContext, err)
+		w.log.Error(err, "unable to remove old visible symlinks")
 		return err
 	}
 
 	// (11)
 	if len(oldTsDir) > 0 {
 		if err = os.RemoveAll(oldTsPath); err != nil {
-			glog.Errorf("%s: error removing old data directory %s: %v", w.logContext, oldTsDir, err)
+			w.log.Error(err, "unable to remove old data directory", "data directory", oldTsDir)
 			return err
 		}
 	}
@@ -329,7 +328,7 @@ func (w *AtomicWriter) pathsToRemove(payload map[string]FileProjection, oldTsDir
 	} else if err != nil {
 		return nil, err
 	}
-	glog.V(5).Infof("%s: current paths:   %+v", w.targetDir, paths.List())
+	w.log.V(1).Info("current paths", "target directory", w.targetDir, "paths", paths.List())
 
 	newPaths := sets.NewString()
 	for file := range payload {
@@ -341,10 +340,10 @@ func (w *AtomicWriter) pathsToRemove(payload map[string]FileProjection, oldTsDir
 			subPath = strings.TrimSuffix(subPath, string(os.PathSeparator))
 		}
 	}
-	glog.V(5).Infof("%s: new paths:       %+v", w.targetDir, newPaths.List())
+	w.log.V(1).Info("new paths", "target directory", w.targetDir, "paths", newPaths.List())
 
 	result := paths.Difference(newPaths)
-	glog.V(5).Infof("%s: paths to remove: %+v", w.targetDir, result)
+	w.log.V(1).Info("paths to remove", "target directory", w.targetDir, "paths", result)
 
 	return result, nil
 }
@@ -353,7 +352,7 @@ func (w *AtomicWriter) pathsToRemove(payload map[string]FileProjection, oldTsDir
 func (w *AtomicWriter) newTimestampDir() (string, error) {
 	tsDir, err := ioutil.TempDir(w.targetDir, time.Now().UTC().Format("..2006_01_02_15_04_05."))
 	if err != nil {
-		glog.Errorf("%s: unable to create new temp directory: %v", w.logContext, err)
+		w.log.Error(err, "unable to create new temp directory")
 		return "", err
 	}
 
@@ -362,7 +361,7 @@ func (w *AtomicWriter) newTimestampDir() (string, error) {
 	// regardless of the process' umask.
 	err = os.Chmod(tsDir, 0755)
 	if err != nil {
-		glog.Errorf("%s: unable to set mode on new temp directory: %v", w.logContext, err)
+		w.log.Error(err, "unable to set mode on new temp directory")
 		return "", err
 	}
 
@@ -380,13 +379,13 @@ func (w *AtomicWriter) writePayloadToDir(payload map[string]FileProjection, dir
 
 		err := os.MkdirAll(baseDir, os.ModePerm)
 		if err != nil {
-			glog.Errorf("%s: unable to create directory %s: %v", w.logContext, baseDir, err)
+			w.log.Error(err, "unable to create directory", "directory", baseDir)
 			return err
 		}
 
 		err = ioutil.WriteFile(fullPath, content, mode)
 		if err != nil {
-			glog.Errorf("%s: unable to write file %s with mode %v: %v", w.logContext, fullPath, mode, err)
+			w.log.Error(err, "unable to write file", "file", fullPath, "mode", mode)
 			return err
 		}
 		// Chmod is needed because ioutil.WriteFile() ends up calling
@@ -395,7 +394,7 @@ func (w *AtomicWriter) writePayloadToDir(payload map[string]FileProjection, dir
 		// in the file no matter what the umask is.
 		err = os.Chmod(fullPath, mode)
 		if err != nil {
-			glog.Errorf("%s: unable to write file %s with mode %v: %v", w.logContext, fullPath, mode, err)
+			w.log.Error(err, "unable to write file", "file", fullPath, "mode", mode)
 		}
 	}
 
@@ -445,7 +444,7 @@ func (w *AtomicWriter) removeUserVisiblePaths(paths sets.String) error {
 			continue
 		}
 		if err := os.Remove(path.Join(w.targetDir, p)); err != nil {
-			glog.Errorf("%s: error pruning old user-visible path %s: %v", w.logContext, p, err)
+			w.log.Error(err, "unable to prune old user-visible path", "path", p)
 			lasterr = err
 		}
 	}

pkg/admission/cert/writer/internal/atomic/atomic_writer_test.go

@@ -30,6 +30,7 @@ import (
 
 	"k8s.io/apimachinery/pkg/util/sets"
 	utiltesting "k8s.io/client-go/util/testing"
+	log "github.com/go-logr/logr/testing"
 )
 
 func TestNewAtomicWriter(t *testing.T) {
@@ -39,7 +40,7 @@ func TestNewAtomicWriter(t *testing.T) {
 	}
 	defer os.RemoveAll(targetDir)
 
-	_, err = NewAtomicWriter(targetDir, "-test-")
+	_, err = NewAtomicWriter(targetDir, log.TestLogger{T: t})
 	if err != nil {
 		t.Fatalf("unexpected error creating writer for existing target dir: %v", err)
 	}
@@ -53,7 +54,7 @@ func TestNewAtomicWriter(t *testing.T) {
 		t.Fatalf("unexpected error ensuring dir %v does not exist: %v", nonExistentDir, err)
 	}
 
-	_, err = NewAtomicWriter(nonExistentDir, "-test-")
+	_, err = NewAtomicWriter(nonExistentDir, log.TestLogger{T: t})
 	if err == nil {
 		t.Fatalf("unexpected success creating writer for nonexistent target dir: %v", err)
 	}
@@ -228,7 +229,7 @@ func TestPathsToRemove(t *testing.T) {
 		}
 		defer os.RemoveAll(targetDir)
 
-		writer := &AtomicWriter{targetDir: targetDir, logContext: "-test-"}
+		writer := &AtomicWriter{targetDir: targetDir, log: log.TestLogger{T: t}}
 		err = writer.Write(tc.payload1)
 		if err != nil {
 			t.Errorf("%v: unexpected error writing: %v", tc.name, err)
@@ -396,7 +397,7 @@ IAAAAAAAsDyZDwU=`
 		}
 		defer os.RemoveAll(targetDir)
 
-		writer := &AtomicWriter{targetDir: targetDir, logContext: "-test-"}
+		writer := &AtomicWriter{targetDir: targetDir, log: log.TestLogger{T: t}}
 		err = writer.Write(tc.payload)
 		if err != nil && tc.success {
 			t.Errorf("%v: unexpected error writing payload: %v", tc.name, err)
@@ -572,7 +573,7 @@ func TestUpdate(t *testing.T) {
 		}
 		defer os.RemoveAll(targetDir)
 
-		writer := &AtomicWriter{targetDir: targetDir, logContext: "-test-"}
+		writer := &AtomicWriter{targetDir: targetDir, log: log.TestLogger{T: t}}
 
 		err = writer.Write(tc.first)
 		if err != nil {
@@ -740,7 +741,7 @@ func TestMultipleUpdates(t *testing.T) {
 		}
 		defer os.RemoveAll(targetDir)
 
-		writer := &AtomicWriter{targetDir: targetDir, logContext: "-test-"}
+		writer := &AtomicWriter{targetDir: targetDir, log: log.TestLogger{T: t}}
 
 		for _, payload := range tc.payloads {
 			writer.Write(payload)
@@ -959,7 +960,7 @@ func TestCreateUserVisibleFiles(t *testing.T) {
 			t.Fatalf("%v: unexpected error creating data path: %v", tc.name, err)
 		}
 
-		writer := &AtomicWriter{targetDir: targetDir, logContext: "-test-"}
+		writer := &AtomicWriter{targetDir: targetDir, log: log.TestLogger{T: t}}
 		payload, err := validatePayload(tc.payload)
 		if err != nil {
 			t.Fatalf("%v: unexpected error validating payload: %v", tc.name, err)

pkg/builder/example_test.go

@@ -19,16 +19,20 @@ package builder_test
 import (
 	"context"
 	"fmt"
-	"log"
+	"os"
 
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
 	"sigs.k8s.io/controller-runtime/pkg/builder"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
+	logf "sigs.k8s.io/controller-runtime/pkg/runtime/log"
 	"sigs.k8s.io/controller-runtime/pkg/runtime/signals"
 )
 
+// NB: don't call SetLogger in init(), or else you'll mess up logging in the main suite.
+var log = logf.Log.WithName("builder-examples")
+
 // This example creates a simple application Controller that is configured for ReplicaSets and Pods.
 //
 // * Create a new application for ReplicaSets that manages Pods owned by the ReplicaSet and calls into
@@ -41,10 +45,14 @@ func ExampleBuilder() {
 		Owns(&corev1.Pod{}).           // ReplicaSet owns Pods created by it
 		Build(&ReplicaSetReconciler{}) // Build
 	if err != nil {
-		log.Fatal(err)
+		log.Error(err, "Unable to build controller")
+		os.Exit(1)
 	}
 
-	log.Fatal(rs.Start(signals.SetupSignalHandler()))
+	if err := rs.Start(signals.SetupSignalHandler()); err != nil {
+		log.Error(err, "Unable to run controller")
+		os.Exit(1)
+	}
 }
 
 // ReplicaSetReconciler is a simple Controller example implementation.