From 7e957dcc5db5c20a3293a628727af6771f7f8bda Mon Sep 17 00:00:00 2001 From: Quan Tian Date: Tue, 16 Apr 2024 16:12:45 +0800 Subject: [PATCH] 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 --- .golangci.yml | 1 + .../cmd/multicluster-controller/clusterset_webhook.go | 2 +- .../multicluster/commonarea/remote_common_area_test.go | 9 ++++----- .../multicluster/member/resourceimport_controller.go | 2 +- .../multicluster/member/serviceexport_controller.go | 4 ++-- .../controller/networkpolicy/networkpolicy_controller.go | 4 ++-- pkg/agent/flowexporter/connections/l7_listener.go | 2 +- pkg/agent/secondarynetwork/podwatch/controller.go | 9 ++++----- 8 files changed, 16 insertions(+), 17 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 3418a789249..d903a5a074a 100644 --- 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 index bded7de9d3b..798d70258db 100644 --- 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 index 28029df0630..6f09fcf8d8c 100644 --- 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 index 99373ffa9ff..c5bfb480408 100644 --- 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 index 557ad9faeb7..2232f3a1056 100644 --- 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 index c4f22e5d8a7..63f1be42f4a 100644 --- 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 index c3167aec6d3..0b34825b9e2 100644 --- 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 index 4fa884b9373..663b7d92e1e 100644 --- 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) } } }()