Fix golangci-lint errors (#10196)

* Fix golangci-lint errors

Signed-off-by: z1cheng <imchench@gmail.com>

* Fix dupl errors

Signed-off-by: z1cheng <imchench@gmail.com>

* Fix comments

Signed-off-by: z1cheng <imchench@gmail.com>

* Fix errcheck lint errors

Signed-off-by: z1cheng <imchench@gmail.com>

* Fix assert in e2e test

Signed-off-by: z1cheng <imchench@gmail.com>

* Not interrupt the waitForPodsReady

Signed-off-by: z1cheng <imchench@gmail.com>

* Replace string with constant

Signed-off-by: z1cheng <imchench@gmail.com>

* Fix comments

Signed-off-by: z1cheng <imchench@gmail.com>

* Revert write file permision

Signed-off-by: z1cheng <imchench@gmail.com>

---------

Signed-off-by: z1cheng <imchench@gmail.com>

pkg/apis/ingress/sslcert.go

@@ -66,12 +66,12 @@ type SSLCert struct {
 }
 
 // GetObjectKind implements the ObjectKind interface as a noop
-func (s SSLCert) GetObjectKind() schema.ObjectKind {
+func (s *SSLCert) GetObjectKind() schema.ObjectKind {
 	return schema.EmptyObjectKind
 }
 
 // Identifier returns a the couple issuer / serial number if they both exist, an empty string otherwise
-func (s SSLCert) Identifier() string {
+func (s *SSLCert) Identifier() string {
 	if s.Certificate != nil {
 		if s.Certificate.SerialNumber != nil {
 			return fmt.Sprintf("%s-%s", s.Certificate.Issuer.SerialNumber, s.Certificate.SerialNumber.String())
@@ -81,7 +81,7 @@ func (s SSLCert) Identifier() string {
 }
 
 // HashInclude defines if a field should be used or not to calculate the hash
-func (s SSLCert) HashInclude(field string, v interface{}) (bool, error) {
+func (s *SSLCert) HashInclude(field string, _ interface{}) (bool, error) {
 	switch field {
 	case "PemSHA", "CASHA", "ExpireTime":
 		return true, nil

pkg/apis/ingress/types.go

@@ -73,7 +73,7 @@ type Configuration struct {
 
 	DefaultSSLCertificate *SSLCert `json:"-"`
 
-	StreamSnippets []string
+	StreamSnippets []string `json:"StreamSnippets"`
 }
 
 // Backend describes one or more remote server/s (endpoints) associated with a service
@@ -129,7 +129,7 @@ type TrafficShapingPolicy struct {
 }
 
 // HashInclude defines if a field should be used or not to calculate the hash
-func (s Backend) HashInclude(field string, v interface{}) (bool, error) {
+func (b *Backend) HashInclude(field string, _ interface{}) (bool, error) {
 	switch field {
 	case "Endpoints":
 		return false, nil
@@ -410,5 +410,4 @@ type Ingress struct {
 }
 
 // GeneralConfig holds the definition of lua general configuration data
-type GeneralConfig struct {
-}
+type GeneralConfig struct{}

pkg/apis/ingress/types_equals.go

@@ -80,58 +80,58 @@ func (c1 *Configuration) Equal(c2 *Configuration) bool {
 }
 
 // Equal tests for equality between two Backend types
-func (b1 *Backend) Equal(b2 *Backend) bool {
-	if b1 == b2 {
+func (b *Backend) Equal(newB *Backend) bool {
+	if b == newB {
 		return true
 	}
-	if b1 == nil || b2 == nil {
+	if b == nil || newB == nil {
 		return false
 	}
-	if b1.Name != b2.Name {
+	if b.Name != newB.Name {
 		return false
 	}
-	if b1.NoServer != b2.NoServer {
+	if b.NoServer != newB.NoServer {
 		return false
 	}
 
-	if b1.Service != b2.Service {
-		if b1.Service == nil || b2.Service == nil {
+	if b.Service != newB.Service {
+		if b.Service == nil || newB.Service == nil {
 			return false
 		}
-		if b1.Service.GetNamespace() != b2.Service.GetNamespace() {
+		if b.Service.GetNamespace() != newB.Service.GetNamespace() {
 			return false
 		}
-		if b1.Service.GetName() != b2.Service.GetName() {
+		if b.Service.GetName() != newB.Service.GetName() {
 			return false
 		}
 	}
 
-	if b1.Port != b2.Port {
+	if b.Port != newB.Port {
 		return false
 	}
-	if b1.SSLPassthrough != b2.SSLPassthrough {
+	if b.SSLPassthrough != newB.SSLPassthrough {
 		return false
 	}
-	if !(&b1.SessionAffinity).Equal(&b2.SessionAffinity) {
+	if !(&b.SessionAffinity).Equal(&newB.SessionAffinity) {
 		return false
 	}
-	if b1.UpstreamHashBy != b2.UpstreamHashBy {
+	if b.UpstreamHashBy != newB.UpstreamHashBy {
 		return false
 	}
-	if b1.LoadBalancing != b2.LoadBalancing {
+	if b.LoadBalancing != newB.LoadBalancing {
 		return false
 	}
 
-	match := compareEndpoints(b1.Endpoints, b2.Endpoints)
+	match := compareEndpoints(b.Endpoints, newB.Endpoints)
 	if !match {
 		return false
 	}
 
-	if !b1.TrafficShapingPolicy.Equal(b2.TrafficShapingPolicy) {
+	if !b.TrafficShapingPolicy.Equal(&newB.TrafficShapingPolicy) {
 		return false
 	}
 
-	return sets.StringElementsMatch(b1.AlternativeBackends, b2.AlternativeBackends)
+	return sets.StringElementsMatch(b.AlternativeBackends, newB.AlternativeBackends)
 }
 
 // Equal tests for equality between two SessionAffinityConfig types
@@ -243,7 +243,7 @@ func (e1 *Endpoint) Equal(e2 *Endpoint) bool {
 }
 
 // Equal checks for equality between two TrafficShapingPolicies
-func (tsp1 TrafficShapingPolicy) Equal(tsp2 TrafficShapingPolicy) bool {
+func (tsp1 *TrafficShapingPolicy) Equal(tsp2 *TrafficShapingPolicy) bool {
 	if tsp1.Weight != tsp2.Weight {
 		return false
 	}
@@ -335,6 +335,8 @@ func (s1 *Server) Equal(s2 *Server) bool {
 }
 
 // Equal tests for equality between two Location types
+//
+//nolint:gocyclo // Ignore function complexity error
 func (l1 *Location) Equal(l2 *Location) bool {
 	if l1 == l2 {
 		return true
@@ -550,39 +552,39 @@ func (l4b1 *L4Backend) Equal(l4b2 *L4Backend) bool {
 }
 
 // Equal tests for equality between two SSLCert types
-func (s1 *SSLCert) Equal(s2 *SSLCert) bool {
-	if s1 == s2 {
+func (s *SSLCert) Equal(newS *SSLCert) bool {
+	if s == newS {
 		return true
 	}
-	if s1 == nil || s2 == nil {
+	if s == nil || newS == nil {
 		return false
 	}
-	if s1.CASHA != s2.CASHA {
+	if s.CASHA != newS.CASHA {
 		return false
 	}
-	if s1.CRLSHA != s2.CRLSHA {
+	if s.CRLSHA != newS.CRLSHA {
 		return false
 	}
-	if s1.PemSHA != s2.PemSHA {
+	if s.PemSHA != newS.PemSHA {
 		return false
 	}
-	if s1.CAFileName != s2.CAFileName {
+	if s.CAFileName != newS.CAFileName {
 		return false
 	}
-	if s1.CRLFileName != s2.CRLFileName {
+	if s.CRLFileName != newS.CRLFileName {
 		return false
 	}
-	if !s1.ExpireTime.Equal(s2.ExpireTime) {
+	if !s.ExpireTime.Equal(newS.ExpireTime) {
 		return false
 	}
-	if s1.PemCertKey != s2.PemCertKey {
+	if s.PemCertKey != newS.PemCertKey {
 		return false
 	}
-	if s1.UID != s2.UID {
+	if s.UID != newS.UID {
 		return false
 	}
 
-	return sets.StringElementsMatch(s1.CN, s2.CN)
+	return sets.StringElementsMatch(s.CN, newS.CN)
 }
 
 var compareEndpointsFunc = func(e1, e2 interface{}) bool {

pkg/apis/ingress/types_equals_test.go

@@ -25,19 +25,29 @@ import (
 )
 
 func TestEqualConfiguration(t *testing.T) {
-	ap, _ := filepath.Abs("../../../test/manifests/configuration-a.json")
+	ap, err := filepath.Abs("../../../test/manifests/configuration-a.json")
+	if err != nil {
+		t.Errorf("unexpected error: %v", err)
+	}
 	a, err := readJSON(ap)
 	if err != nil {
 		t.Errorf("unexpected error reading JSON file: %v", err)
 	}
 
-	bp, _ := filepath.Abs("../../../test/manifests/configuration-b.json")
+	bp, err := filepath.Abs("../../../test/manifests/configuration-b.json")
+	if err != nil {
+		t.Errorf("unexpected error: %v", err)
+	}
+
 	b, err := readJSON(bp)
 	if err != nil {
 		t.Errorf("unexpected error reading JSON file: %v", err)
 	}
 
-	cp, _ := filepath.Abs("../../../test/manifests/configuration-c.json")
+	cp, err := filepath.Abs("../../../test/manifests/configuration-c.json")
+	if err != nil {
+		t.Errorf("unexpected error: %v", err)
+	}
 	c, err := readJSON(cp)
 	if err != nil {
 		t.Errorf("unexpected error reading JSON file: %v", err)
@@ -84,15 +94,18 @@ func TestL4ServiceElementsMatch(t *testing.T) {
 		{[]L4Service{{Port: 80}}, []L4Service{{Port: 80}}, true},
 		{
 			[]L4Service{
-				{Port: 80, Endpoints: []Endpoint{{Address: "1.1.1.1"}}}},
+				{Port: 80, Endpoints: []Endpoint{{Address: "1.1.1.1"}}},
+			},
 			[]L4Service{{Port: 80}},
 			false,
 		},
 		{
 			[]L4Service{
-				{Port: 80, Endpoints: []Endpoint{{Address: "1.1.1.1"}, {Address: "1.1.1.2"}}}},
+				{Port: 80, Endpoints: []Endpoint{{Address: "1.1.1.1"}, {Address: "1.1.1.2"}}},
+			},
 			[]L4Service{
-				{Port: 80, Endpoints: []Endpoint{{Address: "1.1.1.2"}, {Address: "1.1.1.1"}}}},
+				{Port: 80, Endpoints: []Endpoint{{Address: "1.1.1.2"}, {Address: "1.1.1.1"}}},
+			},
 			true,
 		},
 		{

pkg/flags/flags.go

@@ -298,12 +298,12 @@ https://blog.maxmind.com/2019/12/18/significant-changes-to-accessing-and-using-g
 		nginx.HealthCheckTimeout = time.Duration(*defHealthCheckTimeout) * time.Second
 	}
 
-	if len(*watchNamespace) != 0 && len(*watchNamespaceSelector) != 0 {
+	if *watchNamespace != "" && *watchNamespaceSelector != "" {
 		return false, nil, fmt.Errorf("flags --watch-namespace and --watch-namespace-selector are mutually exclusive")
 	}
 
 	var namespaceSelector labels.Selector
-	if len(*watchNamespaceSelector) != 0 {
+	if *watchNamespaceSelector != "" {
 		var err error
 		namespaceSelector, err = labels.Parse(*watchNamespaceSelector)
 		if err != nil {
@@ -311,7 +311,7 @@ https://blog.maxmind.com/2019/12/18/significant-changes-to-accessing-and-using-g
 		}
 	}
 
-	var histogramBuckets = &collectors.HistogramBuckets{
+	histogramBuckets := &collectors.HistogramBuckets{
 		TimeBuckets:   *timeBuckets,
 		LengthBuckets: *lengthBuckets,
 		SizeBuckets:   *sizeBuckets,
@@ -360,7 +360,7 @@ https://blog.maxmind.com/2019/12/18/significant-changes-to-accessing-and-using-g
 			HTTPS:    *httpsPort,
 			SSLProxy: *sslProxyPort,
 		},
-		IngressClassConfiguration: &ingressclass.IngressClassConfiguration{
+		IngressClassConfiguration: &ingressclass.Configuration{
 			Controller:         *ingressClassController,
 			AnnotationValue:    *ingressClassAnnotation,
 			WatchWithoutClass:  *watchWithoutClass,
@@ -380,7 +380,7 @@ https://blog.maxmind.com/2019/12/18/significant-changes-to-accessing-and-using-g
 
 	var err error
 	if nginx.MaxmindEditionIDs != "" {
-		if err = nginx.ValidateGeoLite2DBEditions(); err != nil {
+		if err := nginx.ValidateGeoLite2DBEditions(); err != nil {
 			return false, nil, err
 		}
 		if nginx.MaxmindLicenseKey != "" || nginx.MaxmindMirror != "" {

pkg/flags/flags_test.go

@@ -33,7 +33,8 @@ func TestDefaults(t *testing.T) {
 
 	oldArgs := os.Args
 	defer func() { os.Args = oldArgs }()
-	os.Args = []string{"cmd",
+	os.Args = []string{
+		"cmd",
 		"--default-backend-service", "namespace/test",
 		"--http-port", "0",
 		"--https-port", "0",
@@ -53,8 +54,8 @@ func TestDefaults(t *testing.T) {
 	}
 }
 
-func TestSetupSSLProxy(t *testing.T) {
-	// TODO
+func TestSetupSSLProxy(_ *testing.T) {
+	// TODO TestSetupSSLProxy
 }
 
 func TestFlagConflict(t *testing.T) {

pkg/metrics/handler.go

@@ -29,7 +29,6 @@ import (
 )
 
 func RegisterHealthz(healthPath string, mux *http.ServeMux, checks ...healthz.HealthChecker) {
-
 	healthCheck := []healthz.HealthChecker{healthz.PingHealthz}
 	if len(checks) > 0 {
 		healthCheck = append(healthCheck, checks...)
@@ -67,7 +66,7 @@ func RegisterProfiler(host string, port int) {
 
 	server := &http.Server{
 		Addr: fmt.Sprintf("%s:%d", host, port),
-		//G112 (CWE-400): Potential Slowloris Attack
+		// G112 (CWE-400): Potential Slowloris Attack
 		ReadHeaderTimeout: 10 * time.Second,
 		Handler:           mux,
 	}

pkg/tcpproxy/tcp.go

@@ -69,7 +69,7 @@ func (p *TCPProxy) Handle(conn net.Conn) {
 	}
 
 	proxy := p.Default
-	hostname, err := parser.GetHostname(data[:])
+	hostname, err := parser.GetHostname(data)
 	if err == nil {
 		klog.V(4).InfoS("TLS Client Hello", "host", hostname)
 		proxy = p.Get(hostname)
@@ -91,8 +91,14 @@ func (p *TCPProxy) Handle(conn net.Conn) {
 
 	if proxy.ProxyProtocol {
 		// write out the Proxy Protocol header
-		localAddr := conn.LocalAddr().(*net.TCPAddr)
-		remoteAddr := conn.RemoteAddr().(*net.TCPAddr)
+		localAddr, ok := conn.LocalAddr().(*net.TCPAddr)
+		if !ok {
+			klog.Errorf("unexpected type: %T", conn.LocalAddr())
+		}
+		remoteAddr, ok := conn.RemoteAddr().(*net.TCPAddr)
+		if !ok {
+			klog.Errorf("unexpected type: %T", conn.RemoteAddr())
+		}
 		protocol := "UNKNOWN"
 		if remoteAddr.IP.To4() != nil {
 			protocol = "TCP4"

pkg/util/file/file_watcher.go

@@ -26,8 +26,8 @@ import (
 	"github.com/fsnotify/fsnotify"
 )
 
-// FileWatcher is an interface we use to watch changes in files
-type FileWatcher interface {
+// Watcher is an interface we use to watch changes in files
+type Watcher interface {
 	Close() error
 }
 
@@ -40,7 +40,7 @@ type OSFileWatcher struct {
 }
 
 // NewFileWatcher creates a new FileWatcher
-func NewFileWatcher(file string, onEvent func()) (FileWatcher, error) {
+func NewFileWatcher(file string, onEvent func()) (Watcher, error) {
 	fw := OSFileWatcher{
 		file:    file,
 		onEvent: onEvent,

pkg/util/file/filesystem.go

@@ -17,4 +17,4 @@ limitations under the License.
 package file
 
 // ReadWriteByUser defines linux permission to read and write files for the owner user
-const ReadWriteByUser = 0700
+const ReadWriteByUser = 0o700

pkg/util/file/structure.go

@@ -33,12 +33,10 @@ const (
 	DefaultSSLDirectory = "/etc/ingress-controller/ssl"
 )
 
-var (
-	directories = []string{
-		DefaultSSLDirectory,
-		AuthDirectory,
-	}
-)
+var directories = []string{
+	DefaultSSLDirectory,
+	AuthDirectory,
+}
 
 // CreateRequiredDirectories verifies if the required directories to
 // start the ingress controller exist and creates the missing ones.

pkg/util/ingress/ingress.go

@@ -114,7 +114,7 @@ func GetRemovedIngresses(rucfg, newcfg *ingress.Configuration) []string {
 
 // IsDynamicConfigurationEnough returns whether a Configuration can be
 // dynamically applied, without reloading the backend.
-func IsDynamicConfigurationEnough(newcfg *ingress.Configuration, oldcfg *ingress.Configuration) bool {
+func IsDynamicConfigurationEnough(newcfg, oldcfg *ingress.Configuration) bool {
 	copyOfRunningConfig := *oldcfg
 	copyOfPcfg := *newcfg
 
@@ -133,21 +133,21 @@ func IsDynamicConfigurationEnough(newcfg *ingress.Configuration, oldcfg *ingress
 // clearL4serviceEndpoints is a helper function to clear endpoints from the ingress configuration since they should be ignored when
 // checking if the new configuration changes can be applied dynamically.
 func clearL4serviceEndpoints(config *ingress.Configuration) {
-	var clearedTCPL4Services []ingress.L4Service
-	var clearedUDPL4Services []ingress.L4Service
-	for _, service := range config.TCPEndpoints {
+	clearedTCPL4Services := make([]ingress.L4Service, 0, len(config.TCPEndpoints))
+	clearedUDPL4Services := make([]ingress.L4Service, 0, len(config.UDPEndpoints))
+	for i := range config.TCPEndpoints {
 		copyofService := ingress.L4Service{
-			Port:      service.Port,
-			Backend:   service.Backend,
+			Port:      config.TCPEndpoints[i].Port,
+			Backend:   config.TCPEndpoints[i].Backend,
 			Endpoints: []ingress.Endpoint{},
 			Service:   nil,
 		}
 		clearedTCPL4Services = append(clearedTCPL4Services, copyofService)
 	}
-	for _, service := range config.UDPEndpoints {
+	for i := range config.UDPEndpoints {
 		copyofService := ingress.L4Service{
-			Port:      service.Port,
-			Backend:   service.Backend,
+			Port:      config.UDPEndpoints[i].Port,
+			Backend:   config.UDPEndpoints[i].Backend,
 			Endpoints: []ingress.Endpoint{},
 			Service:   nil,
 		}
@@ -160,7 +160,7 @@ func clearL4serviceEndpoints(config *ingress.Configuration) {
 // clearCertificates is a helper function to clear Certificates from the ingress configuration since they should be ignored when
 // checking if the new configuration changes can be applied dynamically if dynamic certificates is on
 func clearCertificates(config *ingress.Configuration) {
-	var clearedServers []*ingress.Server
+	clearedServers := make([]*ingress.Server, 0, len(config.Servers))
 	for _, server := range config.Servers {
 		copyOfServer := *server
 		copyOfServer.SSLCert = nil
@@ -169,16 +169,16 @@ func clearCertificates(config *ingress.Configuration) {
 	config.Servers = clearedServers
 }
 
-type redirect struct {
+type Redirect struct {
 	From    string
 	To      string
 	SSLCert *ingress.SSLCert
 }
 
 // BuildRedirects build the redirects of servers based on configurations and certificates
-func BuildRedirects(servers []*ingress.Server) []*redirect {
+func BuildRedirects(servers []*ingress.Server) []*Redirect {
 	names := sets.Set[string]{}
-	redirectServers := make([]*redirect, 0)
+	redirectServers := make([]*Redirect, 0)
 
 	for _, srv := range servers {
 		if !srv.RedirectFromToWWW {
@@ -212,7 +212,7 @@ func BuildRedirects(servers []*ingress.Server) []*redirect {
 			continue
 		}
 
-		r := &redirect{
+		r := &Redirect{
 			From: from,
 			To:   to,
 		}

pkg/util/process/controller.go

@@ -16,9 +16,9 @@ limitations under the License.
 
 package process
 
-// ProcessController defines a common interface for a process to be controlled,
+// Controller defines a common interface for a process to be controlled,
 // like the configurer, the webhook or the proper ingress controller
-type ProcessController interface {
+type Controller interface {
 	Start()
 	Stop() error
 }

pkg/util/process/sigterm.go

@@ -29,7 +29,7 @@ type exiter func(code int)
 
 // HandleSigterm receives a ProcessController interface and deals with
 // the graceful shutdown
-func HandleSigterm(ngx ProcessController, delay int, exit exiter) {
+func HandleSigterm(ngx Controller, delay int, exit exiter) {
 	signalChan := make(chan os.Signal, 1)
 	signal.Notify(signalChan, syscall.SIGTERM)
 	<-signalChan

pkg/util/process/sigterm_test.go

@@ -43,7 +43,7 @@ func (f *FakeProcess) exiterFunc(code int) {
 }
 
 func sendDelayedSignal(delay time.Duration) error {
-	time.Sleep(delay * time.Second)
+	time.Sleep(delay)
 	return syscall.Kill(syscall.Getpid(), syscall.SIGTERM)
 }
 
@@ -67,7 +67,7 @@ func TestHandleSigterm(t *testing.T) {
 		process := &FakeProcess{shouldError: tt.shouldError}
 		t.Run(tt.name, func(t *testing.T) {
 			go func() {
-				err := sendDelayedSignal(2) // Send a signal after 2 seconds
+				err := sendDelayedSignal(2 * time.Second) // Send a signal after 2 seconds
 				if err != nil {
 					t.Errorf("error sending delayed signal: %v", err)
 				}

pkg/util/runtime/cpu_notlinux.go

@@ -23,7 +23,7 @@ import (
 	"runtime"
 )
 
-// NumCPU ...
+// NumCPU returns the number of logical CPUs usable by the current process.
 func NumCPU() int {
 	return runtime.NumCPU()
 }

pkg/util/sets/match_test.go

@@ -20,23 +20,20 @@ import (
 	"testing"
 )
 
-var (
-	testCasesElementMatch = []struct {
-		listA    []string
-		listB    []string
-		expected bool
-	}{
-		{nil, nil, true},
-		{[]string{"1"}, nil, false},
-		{[]string{"1"}, []string{"1"}, true},
-		{[]string{"1", "2", "1"}, []string{"1", "1", "2"}, true},
-		{[]string{"1", "3", "1"}, []string{"1", "1", "2"}, false},
-		{[]string{"1", "1"}, []string{"1", "2"}, false},
-	}
-)
+var testCasesElementMatch = []struct {
+	listA    []string
+	listB    []string
+	expected bool
+}{
+	{nil, nil, true},
+	{[]string{"1"}, nil, false},
+	{[]string{"1"}, []string{"1"}, true},
+	{[]string{"1", "2", "1"}, []string{"1", "1", "2"}, true},
+	{[]string{"1", "3", "1"}, []string{"1", "1", "2"}, false},
+	{[]string{"1", "1"}, []string{"1", "2"}, false},
+}
 
 func TestElementsMatch(t *testing.T) {
-
 	for _, testCase := range testCasesElementMatch {
 		result := StringElementsMatch(testCase.listA, testCase.listB)
 		if result != testCase.expected {