Upgrade Go to 1.23 (#6647)

* Update some for loops in response to the following change introduced
  in Go 1.22: each iteration of the loop creates new variables, to avoid
  accidental sharing bugs. As a result of this change, a few existing
  loop variables had become heap-allocated, so we updated the loop to
  avoid the extra allocation. Other loops that were affected by the
  change (compiling differently), but for which the loop variable was
  stack-allocated, were reviewed individually and when it made sense,
  the loop was refactored to avoid the extra variable copy. The `go test
  -gcflags=-d=loopvar=2` command was used to find affected loops.
* The antrea/codegen image was updated to use Go 1.23. No changes in
  generated files.
* golangci-lint was updated to the latest available version (v1.60.3)

    - govet reported some invalid usages of printf, which was addressed;
      this lead to some refactoring of the `antctl check` code
    - a new gosec rule (G115), which looks for potential integer
      overflows when converting between integer types, caused a lot of
      errors to be reported in the codebase. There were too many errors
      to be addressed as part of this PR, so we only handled a few of
      them for which the fix was straightforward. When the rule
      implementation is improved (so that it reports fewer false
      positives), we should review all errors and address them as needed.

Fixes #6644

Signed-off-by: Antonin Bas <antonin.bas@broadcom.com>

.golangci.yml

@@ -2,13 +2,23 @@
 run:
   tests: true
   timeout: 15m
-  skip-files:
-    - ".*\\.pb\\.go"
-  skip-dirs-use-default: true
 
 linters-settings:
   goimports:
     local-prefixes: antrea.io/antrea
+  gosec:
+    excludes:
+      # At the time of writing this, the G115 rule is not even part of an
+      # official release of gosec. This rule causes a lot of errors to be
+      # reported in the codebase. While some of the reported errors should be
+      # addressed, a lot can also be ignored and there are also some clear false
+      # positives that should not be flagged by gosec in the first place (see
+      # https://github.com/securego/gosec/issues/1187). We will re-enable this
+      # rule in the future when it becomes more accurate.
+      - G115 # Potential integer overflow when converting between integer types
+  misspell:
+    ignore-words:
+      - creater
   revive:
     ignore-generated-header: false
     severity: warning
@@ -30,6 +40,6 @@ linters:
     - staticcheck
     - gosec
     - goimports
-    - vet
+    - govet
     - revive
     - loggercheck

Makefile

@@ -23,7 +23,7 @@ GIT_HOOKS          := $(shell find hack/git_client_side_hooks -type f -print)
 DOCKER_NETWORK     ?= default
 TRIVY_TARGET_IMAGE ?=
 
-GOLANGCI_LINT_VERSION := v1.54.0
+GOLANGCI_LINT_VERSION := v1.60.3
 GOLANGCI_LINT_BINDIR  := $(CURDIR)/.golangci-bin
 GOLANGCI_LINT_BIN     := $(GOLANGCI_LINT_BINDIR)/$(GOLANGCI_LINT_VERSION)/golangci-lint
 

build/images/codegen/README.md

@@ -22,6 +22,7 @@ Here is the table of codegen images that have been uploaded:
 
 | Tag                       | Change                                                                        |
 | :------------------------ | ----------------------------------------------------------------------------- |
+| kubernetes-1.29.2-build.3 | Upgraded Go to v1.23                                                          |
 | kubernetes-1.29.2-build.2 | Upgraded go.uber.org/mock/mockgen to v0.4.0                                   |
 | kubernetes-1.29.2-build.1 | Upgraded controller-gen to v0.14.0                                            |
 | kubernetes-1.29.2-build.0 | Upgraded protoc (v26.0), protoc-gen-go (v1.33.0), protoc-gen-go-grpc (v1.3.0) |

build/images/deps/go-version

@@ -1 +1 @@
-1.21
+1.23

docs/prometheus-integration.md

@@ -345,13 +345,14 @@ could not otherwise find a use for. This should be subtracted from the
 total GC CPU time to obtain a measure of compulsory GC CPU time. This
 metric is an overestimate, and not directly comparable to system CPU time
 measurements. Compare only with other /cpu/classes metrics.
-- **go_cpu_classes_gc_pause_cpu_seconds_total:** Estimated total CPU time spent
-with the application paused by the GC. Even if only one thread is running
-during the pause, this is computed as GOMAXPROCS times the pause latency
-because nothing else can be executing. This is the exact sum of samples in
-/gc/pause:seconds if each sample is multiplied by GOMAXPROCS at the time
-it is taken. This metric is an overestimate, and not directly comparable to
-system CPU time measurements. Compare only with other /cpu/classes metrics.
+- **go_cpu_classes_gc_pause_cpu_seconds_total:** Estimated total CPU time
+spent with the application paused by the GC. Even if only one thread is
+running during the pause, this is computed as GOMAXPROCS times the pause
+latency because nothing else can be executing. This is the exact sum of
+samples in /sched/pauses/total/gc:seconds if each sample is multiplied by
+GOMAXPROCS at the time it is taken. This metric is an overestimate, and not
+directly comparable to system CPU time measurements. Compare only with other
+/cpu/classes metrics.
 - **go_cpu_classes_gc_total_cpu_seconds_total:** Estimated total CPU
 time spent performing GC tasks. This metric is an overestimate, and not
 directly comparable to system CPU time measurements. Compare only with other
@@ -434,8 +435,8 @@ of an out-of-memory error, because the limiter trades memory for CPU time
 when the GC's CPU time gets too high. This is most likely to occur with use
 of SetMemoryLimit. The first GC cycle is cycle 1, so a value of 0 indicates
 that it was never enabled.
-- **go_gc_pauses_seconds:** Distribution of individual GC-related
-stop-the-world pause latencies. Bucket counts increase monotonically.
+- **go_gc_pauses_seconds:** Deprecated. Prefer the identical
+/sched/pauses/total/gc:seconds.
 - **go_gc_scan_globals_bytes:** The total amount of global variable space
 that is scannable.
 - **go_gc_scan_heap_bytes:** The total amount of heap space that is scannable.
@@ -444,6 +445,9 @@ last GC cycle.
 - **go_gc_scan_total_bytes:** The total amount space that is scannable. Sum
 of all metrics in /gc/scan.
 - **go_gc_stack_starting_size_bytes:** The stack size of new goroutines.
+- **go_godebug_non_default_behavior_asynctimerchan_events_total:** The number
+of non-default behaviors executed by the time package due to a non-default
+GODEBUG=asynctimerchan=... setting.
 - **go_godebug_non_default_behavior_execerrdot_events_total:** The number of
 non-default behaviors executed by the os/exec package due to a non-default
 GODEBUG=execerrdot=... setting.
@@ -456,18 +460,27 @@ GODEBUG=gocachetest=... setting.
 - **go_godebug_non_default_behavior_gocacheverify_events_total:** The number
 of non-default behaviors executed by the cmd/go package due to a non-default
 GODEBUG=gocacheverify=... setting.
+- **go_godebug_non_default_behavior_gotypesalias_events_total:** The number
+of non-default behaviors executed by the go/types package due to a non-default
+GODEBUG=gotypesalias=... setting.
 - **go_godebug_non_default_behavior_http2client_events_total:** The number of
 non-default behaviors executed by the net/http package due to a non-default
 GODEBUG=http2client=... setting.
 - **go_godebug_non_default_behavior_http2server_events_total:** The number of
 non-default behaviors executed by the net/http package due to a non-default
 GODEBUG=http2server=... setting.
+- **go_godebug_non_default_behavior_httplaxcontentlength_events_total:**
+The number of non-default behaviors executed by the net/http package due to
+a non-default GODEBUG=httplaxcontentlength=... setting.
+- **go_godebug_non_default_behavior_httpmuxgo121_events_total:** The number
+of non-default behaviors executed by the net/http package due to a non-default
+GODEBUG=httpmuxgo121=... setting.
+- **go_godebug_non_default_behavior_httpservecontentkeepheaders_events_total:**
+The number of non-default behaviors executed by the net/http package due to
+a non-default GODEBUG=httpservecontentkeepheaders=... setting.
 - **go_godebug_non_default_behavior_installgoroot_events_total:** The number
 of non-default behaviors executed by the go/build package due to a non-default
 GODEBUG=installgoroot=... setting.
-- **go_godebug_non_default_behavior_jstmpllitinterp_events_total:** The
-number of non-default behaviors executed by the html/template package due
-to a non-default GODEBUG=jstmpllitinterp=... setting.
 - **go_godebug_non_default_behavior_multipartmaxheaders_events_total:**
 The number of non-default behaviors executed by the mime/multipart package
 due to a non-default GODEBUG=multipartmaxheaders=... setting.
@@ -489,15 +502,42 @@ GODEBUG=randautoseed=... setting.
 - **go_godebug_non_default_behavior_tarinsecurepath_events_total:** The
 number of non-default behaviors executed by the archive/tar package due to
 a non-default GODEBUG=tarinsecurepath=... setting.
+- **go_godebug_non_default_behavior_tls10server_events_total:** The number of
+non-default behaviors executed by the crypto/tls package due to a non-default
+GODEBUG=tls10server=... setting.
+- **go_godebug_non_default_behavior_tls3des_events_total:** The number of
+non-default behaviors executed by the crypto/tls package due to a non-default
+GODEBUG=tls3des=... setting.
 - **go_godebug_non_default_behavior_tlsmaxrsasize_events_total:** The
 number of non-default behaviors executed by the crypto/tls package due to
 a non-default GODEBUG=tlsmaxrsasize=... setting.
+- **go_godebug_non_default_behavior_tlsrsakex_events_total:** The number of
+non-default behaviors executed by the crypto/tls package due to a non-default
+GODEBUG=tlsrsakex=... setting.
+- **go_godebug_non_default_behavior_tlsunsafeekm_events_total:** The number of
+non-default behaviors executed by the crypto/tls package due to a non-default
+GODEBUG=tlsunsafeekm=... setting.
+- **go_godebug_non_default_behavior_winreadlinkvolume_events_total:**
+The number of non-default behaviors executed by the os package due to a
+non-default GODEBUG=winreadlinkvolume=... setting.
+- **go_godebug_non_default_behavior_winsymlink_events_total:** The number
+of non-default behaviors executed by the os package due to a non-default
+GODEBUG=winsymlink=... setting.
+- **go_godebug_non_default_behavior_x509keypairleaf_events_total:** The
+number of non-default behaviors executed by the crypto/tls package due to
+a non-default GODEBUG=x509keypairleaf=... setting.
+- **go_godebug_non_default_behavior_x509negativeserial_events_total:** The
+number of non-default behaviors executed by the crypto/x509 package due to
+a non-default GODEBUG=x509negativeserial=... setting.
 - **go_godebug_non_default_behavior_x509sha1_events_total:** The number of
 non-default behaviors executed by the crypto/x509 package due to a non-default
 GODEBUG=x509sha1=... setting.
 - **go_godebug_non_default_behavior_x509usefallbackroots_events_total:**
 The number of non-default behaviors executed by the crypto/x509 package due
 to a non-default GODEBUG=x509usefallbackroots=... setting.
+- **go_godebug_non_default_behavior_x509usepolicies_events_total:** The
+number of non-default behaviors executed by the crypto/x509 package due to
+a non-default GODEBUG=x509usepolicies=... setting.
 - **go_godebug_non_default_behavior_zipinsecurepath_events_total:** The
 number of non-default behaviors executed by the archive/zip package due to
 a non-default GODEBUG=zipinsecurepath=... setting.
@@ -588,11 +628,33 @@ code simultaneously.
 - **go_sched_latencies_seconds:** Distribution of the time goroutines have
 spent in the scheduler in a runnable state before actually running. Bucket
 counts increase monotonically.
-- **go_sync_mutex_wait_total_seconds_total:** Approximate cumulative time
-goroutines have spent blocked on a sync.Mutex or sync.RWMutex. This metric
-is useful for identifying global changes in lock contention. Collect a
-mutex or block profile using the runtime/pprof package for more detailed
-contention data.
+- **go_sched_pauses_stopping_gc_seconds:** Distribution of individual
+GC-related stop-the-world stopping latencies. This is the time it takes from
+deciding to stop the world until all Ps are stopped. This is a subset of the
+total GC-related stop-the-world time (/sched/pauses/total/gc:seconds). During
+this time, some threads may be executing. Bucket counts increase monotonically.
+- **go_sched_pauses_stopping_other_seconds:** Distribution of
+individual non-GC-related stop-the-world stopping latencies. This
+is the time it takes from deciding to stop the world until all Ps are
+stopped. This is a subset of the total non-GC-related stop-the-world time
+(/sched/pauses/total/other:seconds). During this time, some threads may be
+executing. Bucket counts increase monotonically.
+- **go_sched_pauses_total_gc_seconds:** Distribution of individual
+GC-related stop-the-world pause latencies. This is the time from deciding
+to stop the world until the world is started again. Some of this time
+is spent getting all threads to stop (this is measured directly in
+/sched/pauses/stopping/gc:seconds), during which some threads may still be
+running. Bucket counts increase monotonically.
+- **go_sched_pauses_total_other_seconds:** Distribution of individual
+non-GC-related stop-the-world pause latencies. This is the time from
+deciding to stop the world until the world is started again. Some of
+this time is spent getting all threads to stop (measured directly in
+/sched/pauses/stopping/other:seconds). Bucket counts increase monotonically.
+- **go_sync_mutex_wait_total_seconds_total:** Approximate cumulative
+time goroutines have spent blocked on a sync.Mutex, sync.RWMutex, or
+runtime-internal lock. This metric is useful for identifying global changes
+in lock contention. Collect a mutex or block profile using the runtime/pprof
+package for more detailed contention data.
 - **go_threads:** Number of OS threads created.
 
 #### Hidden Metrics

go.mod

@@ -1,6 +1,6 @@
 module antrea.io/antrea
 
-go 1.21.0
+go 1.23.0
 
 require (
 	antrea.io/libOpenflow v0.14.0

hack/.spelling_failures

@@ -9,3 +9,4 @@ generated_expansion
 generated.pb.go
 generated.proto
 zz_generated
+.golangci.yml

hack/update-codegen.sh

@@ -22,7 +22,7 @@ function echoerr {
 }
 
 ANTREA_ROOT="$( cd "$( dirname "${BASH_SOURCE[0]}" )/../" && pwd )"
-IMAGE_NAME="antrea/codegen:kubernetes-1.29.2-build.2"
+IMAGE_NAME="antrea/codegen:kubernetes-1.29.2-build.3"
 
 # We will use git clone to make a working copy of the repository into a
 # temporary directory. This requires that all changes have been committed

multicluster/controllers/multicluster/leader/resourceexport_controller.go

@@ -352,7 +352,7 @@ func (r *ResourceExportReconciler) refreshEndpointsResourceImport(
 		svcResExport := &mcsv1alpha1.ResourceExport{}
 		err := r.Client.Get(context.Background(), svcResExportName, svcResExport)
 		if err != nil && apierrors.IsNotFound(err) {
-			return newResImport, false, fmt.Errorf("failed to get corresponding Service type of ResourceExport: " + svcResExportName.String())
+			return newResImport, false, fmt.Errorf("failed to get ResourceExport %s: %w", svcResExportName.String(), err)
 		}
 		if len(svcResExport.Status.Conditions) > 0 {
 			if svcResExport.Status.Conditions[0].Status != corev1.ConditionTrue {

multicluster/controllers/multicluster/member/labelidentity_controller.go

@@ -158,12 +158,13 @@ func (r *LabelIdentityReconciler) clusterSetMapFunc(ctx context.Context, a clien
 			podList := &v1.PodList{}
 			r.Client.List(ctx, podList)
 			requests = make([]reconcile.Request, len(podList.Items))
-			for i, pod := range podList.Items {
+			for idx := range podList.Items {
+				pod := &podList.Items[idx]
 				podNamespacedName := types.NamespacedName{
 					Name:      pod.GetName(),
 					Namespace: pod.GetNamespace(),
 				}
-				requests[i] = reconcile.Request{
+				requests[idx] = reconcile.Request{
 					NamespacedName: podNamespacedName,
 				}
 			}
@@ -183,8 +184,9 @@ func (r *LabelIdentityReconciler) namespaceMapFunc(ctx context.Context, ns clien
 	podList := &v1.PodList{}
 	r.Client.List(context.TODO(), podList, client.InNamespace(ns.GetName()))
 	requests := make([]reconcile.Request, len(podList.Items))
-	for i, pod := range podList.Items {
-		requests[i] = reconcile.Request{
+	for idx := range podList.Items {
+		pod := &podList.Items[idx]
+		requests[idx] = reconcile.Request{
 			NamespacedName: types.NamespacedName{
 				Name:      pod.GetName(),
 				Namespace: pod.GetNamespace(),

multicluster/controllers/multicluster/member/node_controller.go

@@ -392,7 +392,8 @@ func (r *NodeReconciler) clusterSetMapFunc(ctx context.Context, a client.Object)
 		if len(clusterSet.Status.Conditions) > 0 && clusterSet.Status.Conditions[0].Status == corev1.ConditionTrue {
 			nodeList := &corev1.NodeList{}
 			r.Client.List(ctx, nodeList)
-			for _, n := range nodeList.Items {
+			for idx := range nodeList.Items {
+				n := &nodeList.Items[idx]
 				if _, ok := n.Annotations[common.GatewayAnnotation]; ok {
 					requests = append(requests, reconcile.Request{
 						NamespacedName: types.NamespacedName{

multicluster/controllers/multicluster/member/resourceimport_controller_test.go

@@ -18,8 +18,8 @@ package member
 
 import (
 	"context"
+	"fmt"
 	"reflect"
-	"strconv"
 	"strings"
 	"testing"
 	"time"
@@ -562,17 +562,17 @@ func TestStaleControllerNoRaceWithResourceImportReconciler(t *testing.T) {
 	stopCh := make(chan struct{})
 	defer close(stopCh)
 	q := workqueue.NewRateLimitingQueue(workqueue.DefaultItemBasedRateLimiter())
-	numInitialResImp := 50
-	for i := 1; i <= numInitialResImp; i++ {
+	const numInitialResImp = 50
+	for i := uint32(1); i <= numInitialResImp; i++ {
 		resImp := &mcv1alpha1.ResourceImport{
 			ObjectMeta: metav1.ObjectMeta{
-				Name:      "label-identity-" + strconv.Itoa(i),
+				Name:      fmt.Sprintf("label-identity-%d", i),
 				Namespace: "antrea-mcs",
 			},
 			Spec: mcv1alpha1.ResourceImportSpec{
 				LabelIdentity: &mcv1alpha1.LabelIdentitySpec{
-					Label: "ns:kubernetes.io/metadata.name=ns&pod:seq=" + strconv.Itoa(i),
-					ID:    uint32(i),
+					Label: fmt.Sprintf("ns:kubernetes.io/metadata.name=ns&pod:seq=%d", i),
+					ID:    i,
 				},
 			},
 		}

multicluster/controllers/multicluster/member/serviceexport_controller.go

@@ -540,7 +540,8 @@ func (r *ServiceExportReconciler) clusterSetMapFunc(ctx context.Context, a clien
 		if len(clusterSet.Status.Conditions) > 0 && clusterSet.Status.Conditions[0].Status == corev1.ConditionTrue {
 			svcExports := &k8smcsv1alpha1.ServiceExportList{}
 			r.Client.List(ctx, svcExports)
-			for _, svcExport := range svcExports.Items {
+			for idx := range svcExports.Items {
+				svcExport := &svcExports.Items[idx]
 				namespacedName := types.NamespacedName{
 					Name:      svcExport.GetName(),
 					Namespace: svcExport.GetNamespace(),

multicluster/hack/update-codegen.sh

@@ -22,7 +22,7 @@ function echoerr {
 }
 
 ANTREA_ROOT="$( cd "$( dirname "${BASH_SOURCE[0]}" )/../../" && pwd )"
-IMAGE_NAME="antrea/codegen:kubernetes-1.29.2-build.2"
+IMAGE_NAME="antrea/codegen:kubernetes-1.29.2-build.3"
 
 # We will use git clone to make a working copy of the repository into a
 # temporary directory. This requires that all changes have been committed

multicluster/test/e2e/antreapolicy_test.go

@@ -70,7 +70,7 @@ func initializeForPolicyTest(t *testing.T, data *MCTestData) {
 	}
 	for clusterName := range data.clusterTestDataMap {
 		d := data.clusterTestDataMap[clusterName]
-		k8sUtils, err := antreae2e.NewKubernetesUtils(&d)
+		k8sUtils, err := antreae2e.NewKubernetesUtils(d)
 		failOnError(err, t)
 		if clusterName != leaderCluster {
 			_, err = k8sUtils.Bootstrap(perClusterNamespaces, perNamespacePods, true, nil, nil)

multicluster/test/e2e/framework.go

@@ -69,7 +69,7 @@ var testOptions TestOptions
 
 type MCTestData struct {
 	clusters            []string
-	clusterTestDataMap  map[string]antreae2e.TestData
+	clusterTestDataMap  map[string]*antreae2e.TestData
 	controlPlaneNames   map[string]string
 	logsDirForTestCase  string
 	clusterGateways     map[string]string
@@ -87,9 +87,9 @@ func (data *MCTestData) createClients() error {
 	data.clusters = []string{
 		leaderCluster, eastCluster, westCluster,
 	}
-	data.clusterTestDataMap = map[string]antreae2e.TestData{}
+	data.clusterTestDataMap = make(map[string]*antreae2e.TestData)
 	for i, cluster := range data.clusters {
-		testData := antreae2e.TestData{ClusterName: cluster}
+		testData := &antreae2e.TestData{ClusterName: cluster}
 		if err := testData.CreateClient(kubeConfigPaths[i]); err != nil {
 			return fmt.Errorf("error initializing clients for cluster %s: %v", cluster, err)
 		}
@@ -193,7 +193,7 @@ func (data *MCTestData) createPod(clusterName, name, nodeName, namespace, ctrNam
 			WithCommand(command).WithArgs(args).
 			WithEnv(env).WithPorts(ports).WithHostNetwork(hostNetwork).
 			WithMutateFunc(mutateFunc).
-			Create(&d)
+			Create(d)
 	}
 	return fmt.Errorf("clusterName %s not found", clusterName)
 }

pkg/agent/controller/networkpolicy/audit_logging.go

@@ -110,7 +110,7 @@ func (l *AuditLogger) terminateLogKey(logMsg string) {
 	defer l.logDeduplication.logMutex.Unlock()
 	logRecord := l.logDeduplication.logMap[logMsg]
 	if logRecord.count == 1 {
-		l.npLogger.Printf(logMsg)
+		l.npLogger.Print(logMsg)
 	} else {
 		l.npLogger.Printf("%s [%d packets in %s]", logMsg, logRecord.count, time.Since(logRecord.initTime))
 	}
@@ -155,7 +155,7 @@ func (l *AuditLogger) LogDedupPacket(ob *logInfo) {
 	// Deduplicate non-Allow packet log.
 	logMsg := buildLogMsg(ob)
 	if ob.disposition == openflow.DispositionToString[openflow.DispositionAllow] {
-		l.npLogger.Printf(logMsg)
+		l.npLogger.Print(logMsg)
 	} else {
 		// Increase count if duplicated within 1 sec, create buffer otherwise.
 		exists := l.updateLogKey(logMsg, l.bufferLength)