Fix misformatted klog and add a linter for loggers (#6227)

It also fixes a bug in secondary podController that the code would panic due to nil pointer dereference when there is a DeletedFinalStateUnknown received from the informer. Signed-off-by: Quan Tian <[email protected]>
antrea-io · Apr 16, 2024 · 7e957dc · 7e957dc
1 parent 6793694
commit 7e957dc
Show file tree

Hide file tree

Showing 8 changed files with 16 additions and 17 deletions.
diff --git a/.golangci.yml b/.golangci.yml
@@ -32,3 +32,4 @@ linters:
     - goimports
     - vet
     - revive
+    - loggercheck
diff --git a/multicluster/cmd/multicluster-controller/clusterset_webhook.go b/multicluster/cmd/multicluster-controller/clusterset_webhook.go
@@ -92,7 +92,7 @@ func (v *clusterSetValidator) Handle(ctx context.Context, req admission.Request)
 
 	if len(clusterSetList.Items) > 0 {
 		err := fmt.Errorf("multiple ClusterSets in a Namespace are not allowed")
-		klog.ErrorS(err, "ClusterSet", klog.KObj(clusterSet), "Namespace", v.namespace)
+		klog.ErrorS(err, "Found existing ClusterSets when handling new ClusterSet", "newClusterSet", klog.KObj(clusterSet), "Namespace", v.namespace)
 		return admission.Errored(http.StatusPreconditionFailed, err)
 	}
 	return admission.Allowed("")

diff --git a/multicluster/controllers/multicluster/commonarea/remote_common_area_test.go b/multicluster/controllers/multicluster/commonarea/remote_common_area_test.go
@@ -26,7 +26,6 @@ import (
 	"go.uber.org/mock/gomock"
 	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/klog/v2"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 
@@ -68,12 +67,12 @@ func TestMemberAnnounce(t *testing.T) {
 
 			err := fakeRemoteClient.List(ctx, memberAnnounceList, client.InNamespace("cluster-a-ns"))
 			if err != nil {
-				klog.InfoS("member announce not written to remote cluster %v yet", err)
+				t.Logf("member announce not written to remote cluster yet: %v", err)
 				continue
 			}
 
 			if !remoteCommonAreaUnderTest.IsConnected() {
-				klog.InfoS("Remote cluster not marked as connected yet")
+				t.Log("Remote cluster not marked as connected yet")
 				continue
 			}
 			done <- true
@@ -133,12 +132,12 @@ func TestMemberAnnounceWithExistingMemberAnnounce(t *testing.T) {
 
 			err := fakeRemoteClient.List(ctx, memberAnnounceList, client.InNamespace("cluster-a-ns"))
 			if err != nil {
-				klog.InfoS("member announce not written to remote cluster %v yet", err)
+				t.Logf("member announce not written to remote cluster yet: %v", err)
 				continue
 			}
 
 			if !remoteCommonAreaUnderTest.IsConnected() {
-				klog.InfoS("Remote cluster not marked as connected yet")
+				t.Log("Remote cluster not marked as connected yet")
 				continue
 			}
 			done <- true

diff --git a/multicluster/controllers/multicluster/member/resourceimport_controller.go b/multicluster/controllers/multicluster/member/resourceimport_controller.go
@@ -286,7 +286,7 @@ func (r *ResourceImportReconciler) handleResImpUpdateForEndpoints(ctx context.Co
 	if epNotFound {
 		err := r.localClusterClient.Create(ctx, mcsEpObj, &client.CreateOptions{})
 		if err != nil {
-			klog.ErrorS(err, "Failed to create MCS Endpoints", "endpoints", klog.KObj(mcsEpObj), err)
+			klog.ErrorS(err, "Failed to create MCS Endpoints", "endpoints", klog.KObj(mcsEpObj))
 			return ctrl.Result{}, err
 		}
 		r.installedResImports.Add(*resImp)

diff --git a/multicluster/controllers/multicluster/member/serviceexport_controller.go b/multicluster/controllers/multicluster/member/serviceexport_controller.go
@@ -214,7 +214,7 @@ func (r *ServiceExportReconciler) Reconcile(ctx context.Context, req ctrl.Reques
 			}
 			return ctrl.Result{}, nil
 		}
-		klog.ErrorS(err, "Failed to get Service", req.String())
+		klog.ErrorS(err, "Failed to get Service", "service", req.String())
 		return ctrl.Result{}, err
 	}
 
@@ -259,7 +259,7 @@ func (r *ServiceExportReconciler) Reconcile(ctx context.Context, req ctrl.Reques
 	} else {
 		newSubsets, hasReadyEndpoints, err = r.checkSubsetsFromEndpoint(ctx, req, eps)
 		if err != nil {
-			klog.ErrorS(err, "Failed to get Endpoints", req.String())
+			klog.ErrorS(err, "Failed to get Endpoints", "endpoints", req.String())
 			return ctrl.Result{}, err
 		}
 	}

diff --git a/pkg/agent/controller/networkpolicy/networkpolicy_controller.go b/pkg/agent/controller/networkpolicy/networkpolicy_controller.go
@@ -934,10 +934,10 @@ func (w *watcher) fallback() {
 	if w.fullSynced {
 		return
 	}
-	klog.InfoS("Getting init events for %s from fallback", w.objectType)
+	klog.InfoS("Getting init events from fallback", "objectType", w.objectType)
 	objects, err := w.FallbackFunc()
 	if err != nil {
-		klog.ErrorS(err, "Failed to get init events for %s from fallback", w.objectType)
+		klog.ErrorS(err, "Failed to get init events from fallback", "objectType", w.objectType)
 		return
 	}
 	if err := w.ReplaceFunc(objects); err != nil {

diff --git a/pkg/agent/flowexporter/connections/l7_listener.go b/pkg/agent/flowexporter/connections/l7_listener.go
@@ -104,7 +104,7 @@ func (l *L7Listener) listenAndAcceptConn() {
 		return
 	}
 	if err := os.MkdirAll(filepath.Dir(l.suricataEventSocketPath), 0750); err != nil {
-		klog.ErrorS(err, "Failed to create directory %s", filepath.Dir(l.suricataEventSocketPath))
+		klog.ErrorS(err, "Failed to create directory", "dir", filepath.Dir(l.suricataEventSocketPath))
 		return
 	}
 	listener, err := net.Listen("unix", l.suricataEventSocketPath)

diff --git a/pkg/agent/secondarynetwork/podwatch/controller.go b/pkg/agent/secondarynetwork/podwatch/controller.go
@@ -157,17 +157,16 @@ func allocatePodSecondaryIfaceName(usedIFNames sets.Set[string]) (string, error)
 }
 
 func (pc *podController) enqueuePod(obj interface{}) {
-	var err error
 	pod, isPod := obj.(*corev1.Pod)
 	if !isPod {
 		podDeletedState, ok := obj.(cache.DeletedFinalStateUnknown)
 		if !ok {
-			klog.ErrorS(err, "Unexpected object received:", obj)
+			klog.ErrorS(nil, "Received unexpected object", "obj", obj)
 			return
 		}
-		pod, ok := podDeletedState.Obj.(*corev1.Pod)
+		pod, ok = podDeletedState.Obj.(*corev1.Pod)
 		if !ok {
-			klog.ErrorS(err, "DeletedFinalStateUnknown object is not of type Pod: ", podDeletedState.Obj, pod)
+			klog.ErrorS(nil, "DeletedFinalStateUnknown object is not of type Pod", "obj", podDeletedState.Obj)
 			return
 		}
 	}
@@ -346,7 +345,7 @@ func (pc *podController) configureSecondaryInterface(
 			if ifConfigErr != nil {
 				// Interface creation failed. Free allocated IP address
 				if err := pc.ipamAllocator.SecondaryNetworkRelease(podOwner); err != nil {
-					klog.ErrorS(err, "IPAM de-allocation failed: ", err)
+					klog.ErrorS(err, "IPAM de-allocation failed", "podOwner", podOwner)
 				}
 			}
 		}()