upgrade to golangci-lint 1.44 and fix findings

Signed-off-by: Stefan Büringer [email protected]

.github/workflows/golangci-lint.yml

@@ -17,5 +17,5 @@ jobs:
       - name: golangci-lint
         uses: golangci/golangci-lint-action@v2
         with:
-          version: v1.43.0
+          version: v1.44.0
           working-directory: ${{matrix.working-directory}}

.golangci.yml

@@ -3,6 +3,7 @@ linters:
   enable:
   - asciicheck
   - bodyclose
+  - containedctx
   - deadcode
   - depguard
   - dogsled
@@ -242,6 +243,10 @@ issues:
     - ifshort
     text:  "variable 'isDeleteNodeAllowed' is only used in the if-statement.*"
     path: ^internal/controllers/machine/machine_controller\.go$
+  - linters:
+    - ifshort
+    text: "variable 'kcpMachinesWithErrors' is only used in the if-statement.*"
+    path: ^controlplane/kubeadm/internal/workload_cluster_conditions\.go$
 
 run:
   timeout: 10m

cmd/clusterctl/client/alpha/rollout_pauser.go

@@ -36,7 +36,7 @@ func (r *rollout) ObjectPauser(proxy cluster.Proxy, ref corev1.ObjectReference)
 			return errors.Wrapf(err, "failed to fetch %v/%v", ref.Kind, ref.Name)
 		}
 		if deployment.Spec.Paused {
-			return errors.Errorf("MachineDeploymet is already paused: %v/%v\n", ref.Kind, ref.Name)
+			return errors.Errorf("MachineDeploymet is already paused: %v/%v\n", ref.Kind, ref.Name) //nolint:revive // MachineDeployment is intentionally capitalized.
 		}
 		if err := pauseMachineDeployment(proxy, ref.Name, ref.Namespace); err != nil {
 			return err

cmd/clusterctl/client/alpha/rollout_restarter.go

@@ -37,7 +37,7 @@ func (r *rollout) ObjectRestarter(proxy cluster.Proxy, ref corev1.ObjectReferenc
 			return errors.Wrapf(err, "failed to fetch %v/%v", ref.Kind, ref.Name)
 		}
 		if deployment.Spec.Paused {
-			return errors.Errorf("can't restart paused machinedeployment (run rollout resume first): %v/%v\n", ref.Kind, ref.Name)
+			return errors.Errorf("can't restart paused MachineDeployment (run rollout resume first): %v/%v", ref.Kind, ref.Name)
 		}
 		if err := setRestartedAtAnnotation(proxy, ref.Name, ref.Namespace); err != nil {
 			return err

cmd/clusterctl/client/alpha/rollout_resumer.go

@@ -36,13 +36,13 @@ func (r *rollout) ObjectResumer(proxy cluster.Proxy, ref corev1.ObjectReference)
 			return errors.Wrapf(err, "failed to fetch %v/%v", ref.Kind, ref.Name)
 		}
 		if !deployment.Spec.Paused {
-			return errors.Errorf("MachineDeployment is not currently paused: %v/%v\n", ref.Kind, ref.Name)
+			return errors.Errorf("MachineDeployment is not currently paused: %v/%v\n", ref.Kind, ref.Name) //nolint:revive // MachineDeployment is intentionally capitalized.
 		}
 		if err := resumeMachineDeployment(proxy, ref.Name, ref.Namespace); err != nil {
 			return err
 		}
 	default:
-		return errors.Errorf("Invalid resource type %q, valid values are %v", ref.Kind, validResourceTypes)
+		return errors.Errorf("invalid resource type %q, valid values are %v", ref.Kind, validResourceTypes)
 	}
 	return nil
 }

cmd/clusterctl/client/client_test.go

@@ -169,7 +169,7 @@ func newFakeClient(configClient config.Client) *fakeClient {
 		// converting the client.Kubeconfig to cluster.Kubeconfig alias
 		k := cluster.Kubeconfig(i.Kubeconfig)
 		if _, ok := fake.clusters[k]; !ok {
-			return nil, errors.Errorf("Cluster for kubeconfig %q and/or context %q does not exist.", i.Kubeconfig.Path, i.Kubeconfig.Context)
+			return nil, errors.Errorf("cluster for kubeconfig %q and/or context %q does not exist", i.Kubeconfig.Path, i.Kubeconfig.Context)
 		}
 		return fake.clusters[k], nil
 	}
@@ -179,7 +179,7 @@ func newFakeClient(configClient config.Client) *fakeClient {
 		InjectClusterClientFactory(clusterClientFactory),
 		InjectRepositoryFactory(func(input RepositoryClientFactoryInput) (repository.Client, error) {
 			if _, ok := fake.repositories[input.Provider.ManifestLabel()]; !ok {
-				return nil, errors.Errorf("Repository for kubeconfig %q does not exist.", input.Provider.ManifestLabel())
+				return nil, errors.Errorf("repository for kubeconfig %q does not exist", input.Provider.ManifestLabel())
 			}
 			return fake.repositories[input.Provider.ManifestLabel()], nil
 		}),
@@ -222,7 +222,7 @@ func newFakeCluster(kubeconfig cluster.Kubeconfig, configClient config.Client) *
 		cluster.InjectPollImmediateWaiter(pollImmediateWaiter),
 		cluster.InjectRepositoryFactory(func(provider config.Provider, configClient config.Client, options ...repository.Option) (repository.Client, error) {
 			if _, ok := fake.repositories[provider.Name()]; !ok {
-				return nil, errors.Errorf("Repository for kubeconfig %q does not exists.", provider.Name())
+				return nil, errors.Errorf("repository for kubeconfig %q does not exist", provider.Name())
 			}
 			return fake.repositories[provider.Name()], nil
 		}),

cmd/clusterctl/client/cluster/mover_test.go

@@ -929,7 +929,7 @@ func Test_objectMover_filesToObjs(t *testing.T) {
 
 			for _, fileName := range tt.files {
 				path := filepath.Join(dir, fileName)
-				file, err := os.Create(path)
+				file, err := os.Create(path) //nolint:gosec // No security issue: unit test.
 				if err != nil {
 					return
 				}

cmd/clusterctl/client/common.go

@@ -87,16 +87,14 @@ func parseProviderName(provider string) (name string, version string, err error)
 }
 
 func validateDNS1123Label(label string) error {
-	errs := validation.IsDNS1123Label(label)
-	if len(errs) != 0 {
+	if errs := validation.IsDNS1123Label(label); len(errs) != 0 {
 		return errors.New(strings.Join(errs, "; "))
 	}
 	return nil
 }
 
 func validateDNS1123Domanin(subdomain string) error {
-	errs := validation.IsDNS1123Subdomain(subdomain)
-	if len(errs) != 0 {
+	if errs := validation.IsDNS1123Subdomain(subdomain); len(errs) != 0 {
 		return errors.New(strings.Join(errs, "; "))
 	}
 	return nil

cmd/clusterctl/client/config/providers_client.go

@@ -298,8 +298,7 @@ func validateProvider(r Provider) error {
 		return errors.Errorf("name %s must be used with the %s type (name: %s, type: %s)", ClusterAPIProviderName, clusterctlv1.CoreProviderType, r.Name(), r.Type())
 	}
 
-	errMsgs := validation.IsDNS1123Subdomain(r.Name())
-	if len(errMsgs) != 0 {
+	if errMsgs := validation.IsDNS1123Subdomain(r.Name()); len(errMsgs) != 0 {
 		return errors.Errorf("invalid provider name: %s", strings.Join(errMsgs, "; "))
 	}
 	if r.URL() == "" {

cmd/clusterctl/client/config/reader_viper.go

@@ -137,7 +137,7 @@ func downloadFile(url string, filepath string) error {
 	ctx := context.TODO()
 
 	// Create the file
-	out, err := os.Create(filepath)
+	out, err := os.Create(filepath) //nolint:gosec // No security issue: filepath is safe.
 	if err != nil {
 		return errors.Wrapf(err, "failed to create the clusterctl config file %s", filepath)
 	}

cmd/clusterctl/client/config_test.go

@@ -823,7 +823,7 @@ func newFakeClientWithoutCluster(configClient config.Client) *fakeClient {
 		InjectConfig(fake.configClient),
 		InjectRepositoryFactory(func(input RepositoryClientFactoryInput) (repository.Client, error) {
 			if _, ok := fake.repositories[input.Provider.ManifestLabel()]; !ok {
-				return nil, errors.Errorf("Repository for kubeconfig %q does not exist.", input.Provider.ManifestLabel())
+				return nil, errors.Errorf("repository for kubeconfig %q does not exist", input.Provider.ManifestLabel())
 			}
 			return fake.repositories[input.Provider.ManifestLabel()], nil
 		}),

cmd/clusterctl/client/delete.go

@@ -116,7 +116,7 @@ func (c *clusterctlClient) Delete(options DeleteOptions) error {
 				return err
 			}
 			if provider.Namespace == "" {
-				return errors.Errorf("Failed to identify the namespace for the %q provider.", provider.ProviderName)
+				return errors.Errorf("failed to identify the namespace for the %q provider", provider.ProviderName)
 			}
 
 			if provider.Version != "" {
@@ -125,7 +125,7 @@ func (c *clusterctlClient) Delete(options DeleteOptions) error {
 					return err
 				}
 				if provider.Version != version {
-					return errors.Errorf("Failed to identity the provider %q with version %q.", provider.ProviderName, provider.Version)
+					return errors.Errorf("failed to identify the provider %q with version %q", provider.ProviderName, provider.Version)
 				}
 			}
 

cmd/clusterctl/cmd/config_repositories.go

@@ -78,7 +78,7 @@ func init() {
 
 func runGetRepositories(cfgFile string, out io.Writer) error {
 	if cro.output != RepositoriesOutputText && cro.output != RepositoriesOutputYaml {
-		return errors.Errorf("Invalid output format %q. Valid values: %v.", cro.output, RepositoriesOutputs)
+		return errors.Errorf("invalid output format %q, valid values: %v", cro.output, RepositoriesOutputs)
 	}
 
 	if out == nil {

controlplane/kubeadm/internal/cluster_test.go

@@ -188,7 +188,7 @@ func TestGetWorkloadCluster(t *testing.T) {
 
 			for _, o := range tt.objs {
 				g.Expect(env.Client.Create(ctx, o)).To(Succeed())
-				defer func(do client.Object) {
+				defer func(do client.Object) { //nolint:gocritic // No resource leak.
 					g.Expect(env.Cleanup(ctx, do)).To(Succeed())
 				}(o)
 			}

controlplane/kubeadm/internal/workload_cluster_conditions.go

@@ -108,7 +108,7 @@ func (w *Workload) updateManagedEtcdConditions(ctx context.Context, controlPlane
 			conditions.MarkUnknown(machine, controlplanev1.MachineEtcdMemberHealthyCondition, controlplanev1.EtcdMemberInspectionFailedReason, "Failed to connect to the etcd pod on the %s node: %s", node.Name, err)
 			continue
 		}
-		defer etcdClient.Close()
+		defer etcdClient.Close() //nolint:gocritic // etcdClient is closed correctly, no resource leak.
 
 		// While creating a new client, forFirstAvailableNode retrieves the status for the endpoint; check if the endpoint has errors.
 		if len(etcdClient.Errors) > 0 {

controlplane/kubeadm/internal/workload_cluster_etcd.go

@@ -45,7 +45,7 @@ func (w *Workload) ReconcileEtcdMembers(ctx context.Context, nodeNames []string,
 		if err != nil {
 			continue
 		}
-		defer etcdClient.Close()
+		defer etcdClient.Close() //nolint:gocritic // etcdClient is closed correctly, no resource leak.
 
 		members, err := etcdClient.Members(ctx)
 		if err != nil {

controlplane/kubeadm/internal/workload_cluster_etcd_test.go

@@ -668,7 +668,7 @@ func TestReconcileEtcdMembers(t *testing.T) {
 
 			for _, o := range tt.objs {
 				g.Expect(env.CreateAndWait(ctx, o)).To(Succeed())
-				defer func(do client.Object) {
+				defer func(do client.Object) { //nolint:gocritic // No resource leak.
 					g.Expect(env.CleanupAndWait(ctx, do)).To(Succeed())
 				}(o)
 			}

internal/controllers/machinehealthcheck/machinehealthcheck_controller_test.go

@@ -1864,7 +1864,7 @@ func TestClusterToMachineHealthCheck(t *testing.T) {
 			for _, obj := range tc.toCreate {
 				o := obj
 				gs.Expect(r.Client.Create(ctx, &o)).To(Succeed())
-				defer func() {
+				defer func() { //nolint:gocritic // No resource leak.
 					gs.Expect(r.Client.Delete(ctx, &o)).To(Succeed())
 				}()
 				// Check the cache is populated
@@ -1939,7 +1939,7 @@ func TestMachineToMachineHealthCheck(t *testing.T) {
 			for _, obj := range tc.toCreate {
 				o := obj
 				gs.Expect(r.Client.Create(ctx, &o)).To(Succeed())
-				defer func() {
+				defer func() { //nolint:gocritic // No resource leak.
 					gs.Expect(r.Client.Delete(ctx, &o)).To(Succeed())
 				}()
 				// Check the cache is populated
@@ -2041,7 +2041,7 @@ func TestNodeToMachineHealthCheck(t *testing.T) {
 			for _, obj := range tc.mhcToCreate {
 				o := obj
 				gs.Expect(r.Client.Create(ctx, &o)).To(Succeed())
-				defer func() {
+				defer func() { //nolint:gocritic // No resource leak.
 					gs.Expect(r.Client.Delete(ctx, &o)).To(Succeed())
 				}()
 				// Check the cache is populated
@@ -2054,7 +2054,7 @@ func TestNodeToMachineHealthCheck(t *testing.T) {
 			for _, obj := range tc.mToCreate {
 				o := obj
 				gs.Expect(r.Client.Create(ctx, &o)).To(Succeed())
-				defer func() {
+				defer func() { //nolint:gocritic // No resource leak.
 					gs.Expect(r.Client.Delete(ctx, &o)).To(Succeed())
 				}()
 				// Ensure the status is set (required for matching node to machine)

internal/webhooks/cluster.go

@@ -81,7 +81,7 @@ func (webhook *Cluster) Default(ctx context.Context, obj runtime.Object) error {
 		clusterClass, err := webhook.getClusterClassForCluster(ctx, cluster)
 		if err != nil {
 			// Return early with errors if the ClusterClass can't be retrieved.
-			return apierrors.NewInternalError(errors.Wrapf(err, "Cluster %s can't be validated. ClusterClass %s can not be retrieved.", cluster.Name, cluster.Spec.Topology.Class))
+			return apierrors.NewInternalError(errors.Wrapf(err, "Cluster %s can't be validated. ClusterClass %s can not be retrieved", cluster.Name, cluster.Spec.Topology.Class))
 		}
 
 		// We gather all defaulting errors and return them together.

internal/webhooks/cluster_test.go

@@ -46,7 +46,10 @@ func TestClusterDefaultNamespaces(t *testing.T) {
 		},
 	}
 	webhook := &Cluster{}
-	t.Run("for Cluster", customDefaultValidateTest(ctx, c, webhook))
+	// TODO(sbueringer) We are storing the func in testFunc temporarily to work around
+	// an issue in thelper: https://github.com/kulti/thelper/issues/31
+	testFunc := customDefaultValidateTest(ctx, c, webhook)
+	t.Run("for Cluster", testFunc)
 	g.Expect(webhook.Default(ctx, c)).To(Succeed())
 
 	g.Expect(c.Spec.InfrastructureRef.Namespace).To(Equal(c.Namespace))
@@ -348,8 +351,10 @@ func TestClusterDefaultTopologyVersion(t *testing.T) {
 
 	// Create the webhook and add the fakeClient as its client.
 	webhook := &Cluster{Client: fakeClient}
-
-	t.Run("for Cluster", customDefaultValidateTest(ctx, c, webhook))
+	// TODO(sbueringer) We are storing the func in testFunc temporarily to work around
+	// an issue in thelper: https://github.com/kulti/thelper/issues/31
+	testFunc := customDefaultValidateTest(ctx, c, webhook)
+	t.Run("for Cluster", testFunc)
 	g.Expect(webhook.Default(ctx, c)).To(Succeed())
 
 	g.Expect(c.Spec.Topology.Version).To(HavePrefix("v"))

internal/webhooks/clusterclass_test.go

@@ -76,7 +76,10 @@ func TestClusterClassDefaultNamespaces(t *testing.T) {
 
 	// Create the webhook and add the fakeClient as its client.
 	webhook := &ClusterClass{Client: fakeClient}
-	t.Run("for ClusterClass", customDefaultValidateTest(ctx, in, webhook))
+	// TODO(sbueringer) We are storing the func in testFunc temporarily to work around
+	// an issue in thelper: https://github.com/kulti/thelper/issues/31
+	testFunc := customDefaultValidateTest(ctx, in, webhook)
+	t.Run("for ClusterClass", testFunc)
 
 	g := NewWithT(t)
 	g.Expect(webhook.Default(ctx, in)).To(Succeed())

test/framework/clusterctl/logger/log_file.go

@@ -36,7 +36,7 @@ func CreateLogFile(input CreateLogFileInput) *LogFile {
 	filePath := filepath.Join(input.LogFolder, input.Name)
 	Expect(os.MkdirAll(filepath.Dir(filePath), 0750)).To(Succeed(), "Failed to create log folder %s", filepath.Dir(filePath))
 
-	f, err := os.Create(filePath)
+	f, err := os.Create(filePath) //nolint:gosec // No security issue: filepath is safe.
 	Expect(err).ToNot(HaveOccurred(), "Failed to create log file %s", filePath)
 
 	return &LogFile{

test/framework/docker_logcollector.go

@@ -163,5 +163,5 @@ func fileOnHost(path string) (*os.File, error) {
 	if err := os.MkdirAll(filepath.Dir(path), os.ModePerm); err != nil {
 		return nil, err
 	}
-	return os.Create(path)
+	return os.Create(path) //nolint:gosec // No security issue: path is safe.
 }

test/framework/ginkgoextensions/output.go

@@ -58,7 +58,7 @@ func EnableFileLogging(path string) (io.WriteCloser, error) {
 }
 
 func newFileWriter(path string) (io.WriteCloser, error) {
-	f, err := os.Create(path)
+	f, err := os.Create(path) //nolint:gosec // No security issue: path is safe.
 	if err != nil {
 		return nil, errors.Wrap(err, "failed to create file")
 	}

test/framework/kubetest/setup.go

@@ -31,7 +31,7 @@ func copyFile(srcFilePath, destFilePath string) error {
 	if err != nil {
 		return err
 	}
-	destFile, err := os.Create(destFilePath)
+	destFile, err := os.Create(destFilePath) //nolint:gosec // No security issue: destFilePath is safe.
 	if err != nil {
 		return err
 	}

test/infrastructure/container/docker.go

@@ -78,7 +78,7 @@ func (d *dockerRuntime) SaveContainerImage(ctx context.Context, image, dest stri
 	}
 	defer reader.Close()
 
-	tar, err := os.Create(dest)
+	tar, err := os.Create(dest) //nolint:gosec // No security issue: dest is safe.
 	if err != nil {
 		return fmt.Errorf("failed to create destination file %q: %v", dest, err)
 	}

test/infrastructure/docker/api/v1beta1/dockercluster_webhook.go

@@ -45,8 +45,7 @@ var _ webhook.Validator = &DockerCluster{}
 
 // ValidateCreate implements webhook.Validator so a webhook will be registered for the type.
 func (c *DockerCluster) ValidateCreate() error {
-	allErrs := validateDockerClusterSpec(c.Spec)
-	if len(allErrs) > 0 {
+	if allErrs := validateDockerClusterSpec(c.Spec); len(allErrs) > 0 {
 		return apierrors.NewInvalid(GroupVersion.WithKind("DockerCluster").GroupKind(), c.Name, allErrs)
 	}
 	return nil

test/infrastructure/docker/internal/docker/machine.go

@@ -308,7 +308,7 @@ func (m *Machine) PreloadLoadImages(ctx context.Context, images []string) error
 		if err != nil {
 			return errors.Wrap(err, "failed to open image")
 		}
-		defer f.Close()
+		defer f.Close() //nolint:gocritic // No resource leak.
 
 		ps := m.container.Commander.Command("ctr", "--namespace=k8s.io", "images", "import", "-")
 		ps.SetStdin(f)