Merge pull request kubernetes-sigs#117 from DirectXMan12/bug/glog-hunting-mark-2

glog elimination round 2

Author: k8s-ci-robot

Gopkg.lock

@@ -54,17 +54,17 @@ required = ["sigs.k8s.io/testing_frameworks/integration",
   name = "go.uber.org/zap"
   version = "1.8.0"
 
-[[constraint]]
-  name = "sigs.k8s.io/testing_frameworks"
-  revision = "f53464b8b84b4507805a0b033a8377b225163fea"
-
-[[constraint]]
-  name = "github.com/go-logr/logr"
-  revision = "9fb12b3b21c5415d16ac18dc5cd42c1cfdd40c4e"
+# these are not listed explicitly until we get version tags,
+# since dep doesn't like bare revision dependencies
 
-[[constraint]]
-  name = "github.com/golang/glog"
-  revision = "23def4e6c14b4da8ac2ed8007337bc5eb5007998"
+# [[constraint]]
+#   name = "sigs.k8s.io/testing_frameworks"
+#
+# [[constraint]]
+#   name = "github.com/go-logr/logr"
+#
+# [[constraint]]
+#   name = "github.com/go-logr/zapr"
 
 # For dependency below: Refer to issue https://github.com/golang/dep/issues/1799
 [[override]]

Gopkg.toml

@@ -21,9 +21,14 @@ import (
 	"io/ioutil"
 	"net/http"
 
-	"github.com/golang/glog"
 	"k8s.io/api/admission/v1beta1"
 	"k8s.io/apimachinery/pkg/runtime"
+	logf "sigs.k8s.io/controller-runtime/pkg/runtime/log"
+)
+
+var (
+	// TODO(directxman12): this shouldn't be a global log
+	log = logf.KBLog.WithName("admission").WithName("http-handler")
 )
 
 func (h httpHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
@@ -37,15 +42,15 @@ func (h httpHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	// verify the content type is accurate
 	contentType := r.Header.Get("Content-Type")
 	if contentType != "application/json" {
-		glog.Errorf("contentType=%s, expect application/json", contentType)
+		log.Error(nil, "invalid content type, expected application/json", "context type", contentType)
 		return
 	}
 
 	var reviewResponse *v1beta1.AdmissionResponse
 	ar := v1beta1.AdmissionReview{}
 	deserializer := codecs.UniversalDeserializer()
 	if _, _, err := deserializer.Decode(body, nil, &ar); err != nil {
-		glog.Error(err)
+		log.Error(err, "unable to decode request body")
 		reviewResponse = ErrorResponse(err)
 	} else {
 		reviewResponse = h.admit(ar)
@@ -62,10 +67,11 @@ func (h httpHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 
 	resp, err := json.Marshal(response)
 	if err != nil {
-		glog.Error(err)
+		log.Error(err, "unable to marshal response")
+		return
 	}
 	if _, err := w.Write(resp); err != nil {
-		glog.Error(err)
+		log.Error(err, "unable to write response")
 	}
 }
 

pkg/internal/admission/http.go

@@ -19,7 +19,7 @@ package recorder
 import (
 	"fmt"
 
-	"github.com/golang/glog"
+	"github.com/go-logr/logr"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/client-go/kubernetes"
@@ -34,19 +34,24 @@ type provider struct {
 	scheme *runtime.Scheme
 	// eventBroadcaster to create new recorder instance
 	eventBroadcaster record.EventBroadcaster
+	// logger is the logger to use when logging diagnostic event info
+	logger logr.Logger
 }
 
 // NewProvider create a new Provider instance.
-func NewProvider(config *rest.Config, scheme *runtime.Scheme) (recorder.Provider, error) {
+func NewProvider(config *rest.Config, scheme *runtime.Scheme, logger logr.Logger) (recorder.Provider, error) {
 	clientSet, err := kubernetes.NewForConfig(config)
 	if err != nil {
 		return nil, fmt.Errorf("failed to init clientSet: %v", err)
 	}
 
-	p := &provider{scheme: scheme}
+	p := &provider{scheme: scheme, logger: logger}
 	p.eventBroadcaster = record.NewBroadcaster()
-	p.eventBroadcaster.StartLogging(glog.Infof)
 	p.eventBroadcaster.StartRecordingToSink(&typedcorev1.EventSinkImpl{Interface: clientSet.CoreV1().Events("")})
+	p.eventBroadcaster.StartEventWatcher(
+		func(e *corev1.Event) {
+			p.logger.V(1).Info(e.Type, "object", e.InvolvedObject, "reason", e.Reason, "message", e.Message)
+		})
 
 	return p, nil
 }

pkg/internal/recorder/recorder.go

@@ -17,6 +17,7 @@ limitations under the License.
 package recorder_test
 
 import (
+	tlog "github.com/go-logr/logr/testing"
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
 	"k8s.io/client-go/kubernetes/scheme"
@@ -26,7 +27,7 @@ import (
 var _ = Describe("recorder.Provider", func() {
 	Describe("NewProvider", func() {
 		It("should return a provider instance and a nil error.", func() {
-			provider, err := recorder.NewProvider(cfg, scheme.Scheme)
+			provider, err := recorder.NewProvider(cfg, scheme.Scheme, tlog.NullLogger{})
 			Expect(provider).NotTo(BeNil())
 			Expect(err).NotTo(HaveOccurred())
 		})
@@ -35,13 +36,13 @@ var _ = Describe("recorder.Provider", func() {
 			// Invalid the config
 			cfg1 := *cfg
 			cfg1.ContentType = "invalid-type"
-			_, err := recorder.NewProvider(&cfg1, scheme.Scheme)
+			_, err := recorder.NewProvider(&cfg1, scheme.Scheme, tlog.NullLogger{})
 			Expect(err.Error()).To(ContainSubstring("failed to init clientSet"))
 		})
 	})
 	Describe("GetEventRecorder", func() {
 		It("should return a recorder instance.", func() {
-			provider, err := recorder.NewProvider(cfg, scheme.Scheme)
+			provider, err := recorder.NewProvider(cfg, scheme.Scheme, tlog.NullLogger{})
 			Expect(err).NotTo(HaveOccurred())
 
 			recorder := provider.GetEventRecorderFor("test")

pkg/internal/recorder/recorder_test.go

@@ -20,6 +20,7 @@ import (
 	"fmt"
 	"time"
 
+	"github.com/go-logr/logr"
 	"k8s.io/apimachinery/pkg/api/meta"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/client-go/kubernetes/scheme"
@@ -85,7 +86,7 @@ type Options struct {
 	// Dependency injection for testing
 	newCache            func(config *rest.Config, opts cache.Options) (cache.Cache, error)
 	newClient           func(config *rest.Config, options client.Options) (client.Client, error)
-	newRecorderProvider func(config *rest.Config, scheme *runtime.Scheme) (recorder.Provider, error)
+	newRecorderProvider func(config *rest.Config, scheme *runtime.Scheme, logger logr.Logger) (recorder.Provider, error)
 }
 
 // Runnable allows a component to be started.
@@ -132,7 +133,9 @@ func New(config *rest.Config, options Options) (Manager, error) {
 		return nil, err
 	}
 	// Create the recorder provider to inject event recorders for the components.
-	recorderProvider, err := options.newRecorderProvider(config, options.Scheme)
+	// TODO(directxman12): the log for the event provider should have a context (name, tags, etc) specific
+	// to the particular controller that it's being injected into, rather than a generic one like is here.
+	recorderProvider, err := options.newRecorderProvider(config, options.Scheme, log.WithName("events"))
 	if err != nil {
 		return nil, err
 	}

pkg/manager/manager.go

@@ -19,6 +19,7 @@ package manager
 import (
 	"fmt"
 
+	"github.com/go-logr/logr"
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
 	"k8s.io/apimachinery/pkg/api/meta"
@@ -86,7 +87,7 @@ var _ = Describe("manger.Manager", func() {
 		})
 		It("should return an error it can't create a recorder.Provider", func(done Done) {
 			m, err := New(cfg, Options{
-				newRecorderProvider: func(config *rest.Config, scheme *runtime.Scheme) (recorder.Provider, error) {
+				newRecorderProvider: func(config *rest.Config, scheme *runtime.Scheme, logger logr.Logger) (recorder.Provider, error) {
 					return nil, fmt.Errorf("expected error")
 				}})
 			Expect(m).To(BeNil())