feat(.golangci.yml): enable unparam linter and remediate

.golangci.yml

@@ -23,6 +23,7 @@ linters:
     - nolintlint
     - stylecheck
     - unconvert
+    - unparam
 issues:
   exclude-rules:
     # Excluding single digits from magic number detector because it produces too many obvious results (like klog)

pkg/controllers/netpol/network_policy_controller.go

@@ -262,11 +262,7 @@ func (npc *NetworkPolicyController) fullPolicySync() {
 		return
 	}
 
-	activePodFwChains, err := npc.syncPodFirewallChains(networkPoliciesInfo, syncVersion)
-	if err != nil {
-		klog.Errorf("Aborting sync. Failed to sync pod firewalls: %v", err.Error())
-		return
-	}
+	activePodFwChains := npc.syncPodFirewallChains(networkPoliciesInfo, syncVersion)
 
 	// Makes sure that the ACCEPT rules for packets marked with "0x20000" are added to the end of each of kube-router's
 	// top level chains

pkg/controllers/netpol/network_policy_controller_test.go

@@ -135,7 +135,7 @@ func tNewPodNamespaceMapFromTC(target map[string]string) tPodNamespaceMap {
 
 // tCreateFakePods creates the Pods and Namespaces that will be affected by the network policies
 //	returns a map like map[Namespace]map[PodName]bool
-func tCreateFakePods(t *testing.T, podInformer cache.SharedIndexInformer, nsInformer cache.SharedIndexInformer) tPodNamespaceMap {
+func tCreateFakePods(t *testing.T, podInformer cache.SharedIndexInformer, nsInformer cache.SharedIndexInformer) {
 	podNamespaceMap := make(tPodNamespaceMap)
 	pods := []podInfo{
 		{name: "Aa", labels: labels.Set{"app": "a"}, namespace: "nsA", ip: "1.1"},
@@ -169,7 +169,6 @@ func tCreateFakePods(t *testing.T, podInformer cache.SharedIndexInformer, nsInfo
 	for _, ns := range namespaces {
 		tAddToInformerStore(t, nsInformer, &v1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: ns.name, Labels: ns.labels}})
 	}
-	return podNamespaceMap
 }
 
 // newFakeNode is a helper function for creating Nodes for testing.
@@ -188,7 +187,7 @@ func newFakeNode(name string, addr string) *v1.Node {
 
 // newUneventfulNetworkPolicyController returns new NetworkPolicyController object without any event handler
 func newUneventfulNetworkPolicyController(podInformer cache.SharedIndexInformer,
-	npInformer cache.SharedIndexInformer, nsInformer cache.SharedIndexInformer) (*NetworkPolicyController, error) {
+	npInformer cache.SharedIndexInformer, nsInformer cache.SharedIndexInformer) *NetworkPolicyController {
 
 	npc := NetworkPolicyController{}
 	npc.syncPeriod = time.Hour
@@ -199,7 +198,7 @@ func newUneventfulNetworkPolicyController(podInformer cache.SharedIndexInformer,
 	npc.nsLister = nsInformer.GetIndexer()
 	npc.npLister = npInformer.GetIndexer()
 
-	return &npc, nil
+	return &npc
 }
 
 // tNetpolTestCase helper struct to define the inputs to the test case (netpols) and
@@ -376,7 +375,7 @@ func TestNewNetworkPolicySelectors(t *testing.T) {
 	defer cancel()
 	informerFactory.Start(ctx.Done())
 	cache.WaitForCacheSync(ctx.Done(), podInformer.HasSynced)
-	krNetPol, _ := newUneventfulNetworkPolicyController(podInformer, netpolInformer, nsInformer)
+	krNetPol := newUneventfulNetworkPolicyController(podInformer, netpolInformer, nsInformer)
 	tCreateFakePods(t, podInformer, nsInformer)
 	for _, test := range testCases {
 		test.netpol.createFakeNetpol(t, netpolInformer)
@@ -532,7 +531,7 @@ func TestNetworkPolicyBuilder(t *testing.T) {
 	defer cancel()
 	informerFactory.Start(ctx.Done())
 	cache.WaitForCacheSync(ctx.Done(), podInformer.HasSynced)
-	krNetPol, _ := newUneventfulNetworkPolicyController(podInformer, netpolInformer, nsInformer)
+	krNetPol := newUneventfulNetworkPolicyController(podInformer, netpolInformer, nsInformer)
 	tCreateFakePods(t, podInformer, nsInformer)
 	for _, test := range testCases {
 		test.netpol.createFakeNetpol(t, netpolInformer)

pkg/controllers/netpol/pod.go

@@ -74,11 +74,11 @@ func (npc *NetworkPolicyController) handlePodDelete(obj interface{}) {
 }
 
 func (npc *NetworkPolicyController) syncPodFirewallChains(networkPoliciesInfo []networkPolicyInfo,
-	version string) (map[string]bool, error) {
+	version string) map[string]bool {
 
 	activePodFwChains := make(map[string]bool)
 
-	dropUnmarkedTrafficRules := func(podName, podNamespace, podFwChainName string) error {
+	dropUnmarkedTrafficRules := func(podName, podNamespace, podFwChainName string) {
 		// add rule to log the packets that will be dropped due to network policy enforcement
 		comment := "\"rule to log dropped traffic POD name:" + podName + " namespace: " + podNamespace + "\""
 		args := []string{"-A", podFwChainName, "-m", "comment", "--comment", comment,
@@ -87,7 +87,7 @@ func (npc *NetworkPolicyController) syncPodFirewallChains(networkPoliciesInfo []
 		// This used to be AppendUnique when we were using iptables directly, this checks to make sure we didn't drop
 		// unmarked for this chain already
 		if strings.Contains(npc.filterTableRules.String(), strings.Join(args, " ")) {
-			return nil
+			return
 		}
 		npc.filterTableRules.WriteString(strings.Join(args, " "))
 
@@ -100,15 +100,10 @@ func (npc *NetworkPolicyController) syncPodFirewallChains(networkPoliciesInfo []
 		// reset mark to let traffic pass through rest of the chains
 		args = []string{"-A", podFwChainName, "-j", "MARK", "--set-mark", "0/0x10000", "\n"}
 		npc.filterTableRules.WriteString(strings.Join(args, " "))
-
-		return nil
 	}
 
 	// loop through the pods running on the node
-	allLocalPods, err := npc.getLocalPods(npc.nodeIP.String())
-	if err != nil {
-		return nil, err
-	}
+	allLocalPods := npc.getLocalPods(npc.nodeIP.String())
 	for _, pod := range *allLocalPods {
 
 		// ensure pod specific firewall chain exist for all the pods that need ingress firewall
@@ -126,10 +121,7 @@ func (npc *NetworkPolicyController) syncPodFirewallChains(networkPoliciesInfo []
 		// setup rules to intercept inbound traffic to the pods
 		npc.interceptPodOutboundTraffic(pod, podFwChainName)
 
-		err = dropUnmarkedTrafficRules(pod.name, pod.namespace, podFwChainName)
-		if err != nil {
-			return nil, err
-		}
+		dropUnmarkedTrafficRules(pod.name, pod.namespace, podFwChainName)
 
 		// set mark to indicate traffic from/to the pod passed network policies.
 		// Mark will be checked to explicitly ACCEPT the traffic
@@ -139,7 +131,7 @@ func (npc *NetworkPolicyController) syncPodFirewallChains(networkPoliciesInfo []
 		npc.filterTableRules.WriteString(strings.Join(args, " "))
 	}
 
-	return activePodFwChains, nil
+	return activePodFwChains
 }
 
 // setup rules to jump to applicable network policy chains for the traffic from/to the pod
@@ -256,7 +248,7 @@ func (npc *NetworkPolicyController) interceptPodOutboundTraffic(pod podInfo, pod
 	npc.filterTableRules.WriteString(strings.Join(args, " "))
 }
 
-func (npc *NetworkPolicyController) getLocalPods(nodeIP string) (*map[string]podInfo, error) {
+func (npc *NetworkPolicyController) getLocalPods(nodeIP string) *map[string]podInfo {
 	localPods := make(map[string]podInfo)
 	for _, obj := range npc.podLister.List() {
 		pod := obj.(*api.Pod)
@@ -269,7 +261,7 @@ func (npc *NetworkPolicyController) getLocalPods(nodeIP string) (*map[string]pod
 			namespace: pod.ObjectMeta.Namespace,
 			labels:    pod.ObjectMeta.Labels}
 	}
-	return &localPods, nil
+	return &localPods
 }
 
 func podFirewallChainName(namespace, podName string, version string) string {

pkg/controllers/routing/network_routes_controller_test.go

@@ -1954,6 +1954,7 @@ func startInformersForRoutes(nrc *NetworkRoutingController, clientset kubernetes
 	nrc.nodeLister = nodeInformer.GetIndexer()
 }
 
+// nolint:unparam // it doesn't hurt anything to leave timeout here, and increases future flexibility for testing
 func waitForListerWithTimeout(lister cache.Indexer, timeout time.Duration, t *testing.T) {
 	tick := time.Tick(100 * time.Millisecond)
 	timeoutCh := time.After(timeout)

pkg/controllers/routing/utils.go

@@ -20,6 +20,7 @@ import (
 
 // Used for processing Annotations that may contain multiple items
 // Pass this the string and the delimiter
+// nolint:unparam // while delimiter is always "," for now it provides flexibility to leave the function this way
 func stringToSlice(s, d string) []string {
 	ss := make([]string, 0)
 	if strings.Contains(s, d) {

pkg/utils/ipset.go

@@ -199,7 +199,7 @@ func (ipset *IPSet) run(args ...string) (string, error) {
 }
 
 // Used to run ipset binary with arg and inject stdin buffer and return stdout.
-func (ipset *IPSet) runWithStdin(stdin *bytes.Buffer, args ...string) (string, error) {
+func (ipset *IPSet) runWithStdin(stdin *bytes.Buffer, args ...string) error {
 	var stderr bytes.Buffer
 	var stdout bytes.Buffer
 	cmd := exec.Cmd{
@@ -211,10 +211,10 @@ func (ipset *IPSet) runWithStdin(stdin *bytes.Buffer, args ...string) (string, e
 	}
 
 	if err := cmd.Run(); err != nil {
-		return "", errors.New(stderr.String())
+		return errors.New(stderr.String())
 	}
 
-	return stdout.String(), nil
+	return nil
 }
 
 // NewIPSet create a new IPSet with ipSetPath initialized.
@@ -347,7 +347,7 @@ func (set *Set) BatchAdd(addOptions [][]string) error {
 	restoreContents := builder.String()
 
 	// Invoke the command
-	_, err := set.Parent.runWithStdin(bytes.NewBufferString(restoreContents), "restore")
+	err := set.Parent.runWithStdin(bytes.NewBufferString(restoreContents), "restore")
 	if err != nil {
 		return err
 	}
@@ -548,7 +548,7 @@ func (ipset *IPSet) Save() error {
 // Send formatted ipset.sets into stdin of "ipset restore" command.
 func (ipset *IPSet) Restore() error {
 	stdin := bytes.NewBufferString(buildIPSetRestore(ipset))
-	_, err := ipset.runWithStdin(stdin, "restore", "-exist")
+	err := ipset.runWithStdin(stdin, "restore", "-exist")
 	if err != nil {
 		return err
 	}