Merge pull request #7151 from blackpiglet/linter_part2

Linter part2

changelogs/unreleased/7151-blackpiglet

@@ -0,0 +1 @@
+Add more linters part 2.

golangci.yaml

@@ -326,6 +326,12 @@ linters:
     - unused
     - usestdlibvars
     - whitespace
+    - dupword
+    - errchkjson
+    - ginkgolinter
+    - nilerr
+    - noctx
+    - nolintlint
   fast: false
 
 

hack/build-image/Dockerfile

@@ -56,7 +56,7 @@ RUN wget --quiet https://github.com/goreleaser/goreleaser/releases/download/v1.1
     chmod +x /usr/bin/goreleaser
 
 # get golangci-lint
-RUN curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(go env GOPATH)/bin v1.51.0
+RUN curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(go env GOPATH)/bin v1.54.2
 
 # install kubectl
 RUN curl -LO https://storage.googleapis.com/kubernetes-release/release/$(curl -s https://storage.googleapis.com/kubernetes-release/release/stable.txt)/bin/linux/amd64/kubectl

pkg/apis/velero/v1/labels_annotations.go

@@ -83,7 +83,7 @@ const (
 	// AsyncOperationIDLabel is the label key used to identify the async operation ID
 	AsyncOperationIDLabel = "velero.io/async-operation-id"
 
-	// PVCNameLabel is the label key used to identify the the PVC's namespace and name.
+	// PVCNameLabel is the label key used to identify the PVC's namespace and name.
 	// The format is <namespace>/<name>.
 	PVCNamespaceNameLabel = "velero.io/pvc-namespace-name"
 

pkg/archive/extractor.go

@@ -84,7 +84,7 @@ func (e *Extractor) readBackup(tarRdr *tar.Reader) (string, error) {
 			return "", err
 		}
 
-		target := filepath.Join(dir, header.Name) //nolint:gosec
+		target := filepath.Join(dir, header.Name) //nolint:gosec // Internal usage. No need to check.
 
 		switch header.Typeflag {
 		case tar.TypeDir:

pkg/backup/backup.go

@@ -438,8 +438,13 @@ func (kb *kubernetesBackupper) BackupWithResolvers(log logrus.FieldLogger,
 	if err := kube.PatchResource(backupRequest.Backup, updated, kb.kbClient); err != nil {
 		log.WithError(errors.WithStack((err))).Warn("Got error trying to update backup's status.progress and hook status")
 	}
-	skippedPVSummary, _ := json.Marshal(backupRequest.SkippedPVTracker.Summary())
-	log.Infof("Summary for skipped PVs: %s", skippedPVSummary)
+
+	if skippedPVSummary, err := json.Marshal(backupRequest.SkippedPVTracker.Summary()); err != nil {
+		log.WithError(errors.WithStack(err)).Warn("Fail to generate skipped PV summary.")
+	} else {
+		log.Infof("Summary for skipped PVs: %s", skippedPVSummary)
+	}
+
 	backupRequest.Status.Progress = &velerov1api.BackupProgress{TotalItems: len(backupRequest.BackedUpItems), ItemsBackedUp: len(backupRequest.BackedUpItems)}
 	log.WithField("progress", "").Infof("Backed up a total of %d items", len(backupRequest.BackedUpItems))
 

pkg/builder/server_status_request_builder.go

@@ -29,7 +29,7 @@ type ServerStatusRequestBuilder struct {
 	object *velerov1api.ServerStatusRequest
 }
 
-// ForServerStatusRequest is the constructor for for a ServerStatusRequestBuilder.
+// ForServerStatusRequest is the constructor for a ServerStatusRequestBuilder.
 func ForServerStatusRequest(ns, name, resourceVersion string) *ServerStatusRequestBuilder {
 	return &ServerStatusRequestBuilder{
 		object: &velerov1api.ServerStatusRequest{

pkg/cmd/cli/nodeagent/server.go

@@ -486,7 +486,7 @@ func (s *nodeAgentServer) getDataPathConcurrentNum(defaultNum int) int {
 	concurrentNum := math.MaxInt32
 
 	for _, rule := range configs.DataPathConcurrency.PerNodeConfig {
-		selector, err := metav1.LabelSelectorAsSelector(&rule.NodeSelector)
+		selector, err := metav1.LabelSelectorAsSelector(&(rule.NodeSelector))
 		if err != nil {
 			s.logger.WithError(err).Warnf("Failed to parse rule with label selector %s, skip it", rule.NodeSelector.String())
 			continue

pkg/cmd/server/server.go

@@ -248,7 +248,7 @@ type server struct {
 	discoveryHelper  velerodiscovery.Helper
 	dynamicClient    dynamic.Interface
 	// controller-runtime client. the difference from the controller-manager's client
-	// is that the the controller-manager's client is limited to list namespaced-scoped
+	// is that the controller-manager's client is limited to list namespaced-scoped
 	// resources in the namespace where Velero is installed, or the cluster-scoped
 	// resources. The crClient doesn't have the limitation.
 	crClient              ctrlclient.Client

pkg/cmd/util/downloadrequest/downloadrequest.go

@@ -111,7 +111,7 @@ func Stream(ctx context.Context, kbClient kbclient.Client, namespace, name strin
 	httpClient := new(http.Client)
 	httpClient.Transport = &http.Transport{
 		TLSClientConfig: &tls.Config{
-			InsecureSkipVerify: insecureSkipTLSVerify, //nolint:gosec
+			InsecureSkipVerify: insecureSkipTLSVerify, //nolint:gosec // This parameter is useful for some scenarios.
 			RootCAs:            caPool,
 		},
 		IdleConnTimeout:       timeout,
@@ -123,7 +123,7 @@ func Stream(ctx context.Context, kbClient kbclient.Client, namespace, name strin
 		ExpectContinueTimeout: defaultTransport.ExpectContinueTimeout,
 	}
 
-	httpReq, err := http.NewRequest(http.MethodGet, created.Status.DownloadURL, nil)
+	httpReq, err := http.NewRequestWithContext(ctx, http.MethodGet, created.Status.DownloadURL, nil)
 	if err != nil {
 		return err
 	}

pkg/cmd/util/output/describe.go

@@ -164,6 +164,11 @@ func (d *StructuredDescriber) JSONEncode() string {
 	encoder := json.NewEncoder(byteBuffer)
 	encoder.SetEscapeHTML(false)
 	encoder.SetIndent("", "    ")
-	_ = encoder.Encode(d.output)
+
+	err := encoder.Encode(d.output)
+	if err != nil {
+		fmt.Printf("fail to encode %s", err.Error())
+		return ""
+	}
 	return byteBuffer.String()
 }

pkg/controller/backup_controller.go

@@ -1080,7 +1080,7 @@ func generateVolumeInfoForCSIVolumeSnapshot(backup *pkgbackup.Request, csiVolume
 				SnapshotDataMoved:     false,
 				PreserveLocalSnapshot: true,
 				OperationID:           operation.Spec.OperationID,
-				StartTimestamp:        &volumeSnapshot.CreationTimestamp,
+				StartTimestamp:        &(volumeSnapshot.CreationTimestamp),
 				CSISnapshotInfo: volume.CSISnapshotInfo{
 					VSCName:        *volumeSnapshot.Status.BoundVolumeSnapshotContentName,
 					Size:           size,

pkg/controller/backup_deletion_controller.go

@@ -544,7 +544,11 @@ func (r *backupDeletionReconciler) deleteMovedSnapshots(ctx context.Context, bac
 	for i := range list.Items {
 		cm := list.Items[i]
 		snapshot := repository.SnapshotIdentifier{}
-		b, _ := json.Marshal(cm.Data)
+		b, err := json.Marshal(cm.Data)
+		if err != nil {
+			errs = append(errs, errors.Wrapf(err, "fail to marshal the snapshot info into JSON"))
+			continue
+		}
 		if err := json.Unmarshal(b, &snapshot); err != nil {
 			errs = append(errs, errors.Wrapf(err, "failed to unmarshal snapshot info"))
 			continue

pkg/controller/data_download_controller.go

@@ -151,6 +151,7 @@ func (r *DataDownloadReconciler) Reconcile(ctx context.Context, req ctrl.Request
 		log.Info("Data download starting")
 
 		if _, err := r.getTargetPVC(ctx, dd); err != nil {
+			log.WithField("error", err).Debugf("Cannot find target PVC for DataDownload yet. Retry later.")
 			return ctrl.Result{Requeue: true}, nil
 		}
 

pkg/controller/pod_volume_restore_controller.go

@@ -303,7 +303,7 @@ func (c *PodVolumeRestoreReconciler) OnDataPathCompleted(ctx context.Context, na
 	// Write a done file with name=<restore-uid> into the just-created .velero dir
 	// within the volume. The velero init container on the pod is waiting
 	// for this file to exist in each restored volume before completing.
-	if err := os.WriteFile(filepath.Join(volumePath, ".velero", string(restoreUID)), nil, 0644); err != nil { //nolint:gosec
+	if err := os.WriteFile(filepath.Join(volumePath, ".velero", string(restoreUID)), nil, 0644); err != nil { //nolint:gosec // Internal usage. No need to check.
 		_, _ = c.errorOut(ctx, &pvr, err, "error writing done file", log)
 		return
 	}

pkg/datamover/dataupload_delete_action.go

@@ -58,9 +58,14 @@ func genConfigmap(bak *velerov1.Backup, du velerov2alpha1.DataUpload) *corev1api
 		SnapshotID:            du.Status.SnapshotID,
 		RepositoryType:        GetUploaderType(du.Spec.DataMover),
 	}
-	b, _ := json.Marshal(snapshot)
+	b, err := json.Marshal(snapshot)
+	if err != nil {
+		return nil
+	}
 	data := make(map[string]string)
-	_ = json.Unmarshal(b, &data)
+	if err := json.Unmarshal(b, &data); err != nil {
+		return nil
+	}
 	return &corev1api.ConfigMap{
 		TypeMeta: metav1.TypeMeta{
 			APIVersion: corev1api.SchemeGroupVersion.String(),

pkg/metrics/metrics.go

@@ -69,7 +69,7 @@ const (
 	// data mover metrics
 	DataUploadSuccessTotal   = "data_upload_success_total"
 	DataUploadFailureTotal   = "data_upload_failure_total"
-	DataUploadCancelTotal    = "data_upload_cancel_total"
+	DataUploadCancelTotal    = "data_upload_cancel_total" //nolint:gosec // Not a hard code secret.
 	DataDownloadSuccessTotal = "data_download_success_total"
 	DataDownloadFailureTotal = "data_download_failure_total"
 	DataDownloadCancelTotal  = "data_download_cancel_total"

pkg/plugin/clientmgmt/process/client_builder.go

@@ -80,7 +80,7 @@ func (b *clientBuilder) clientConfig() *hcplugin.ClientConfig {
 			string(common.PluginKindDeleteItemAction):    framework.NewDeleteItemActionPlugin(common.ClientLogger(b.clientLogger)),
 		},
 		Logger: b.pluginLogger,
-		Cmd:    exec.Command(b.commandName, b.commandArgs...), //nolint
+		Cmd:    exec.Command(b.commandName, b.commandArgs...), //nolint:gosec // Internal call. No need to check the command line.
 	}
 }
 

pkg/podvolume/restorer.go

@@ -199,6 +199,7 @@ func (r *restorer) RestorePodVolumes(data RestoreData) []error {
 
 			err = kube.IsPodScheduled(newObj)
 			if err != nil {
+				r.log.WithField("error", err).Debugf("Pod %s/%s is not scheduled yet", newObj.GetNamespace(), newObj.GetName())
 				return false, nil
 			}
 			return true, nil

pkg/repository/config/aws.go

@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-//nolint:gosec
+//nolint:gosec // Internal usage. No need to check.
 package config
 
 import (

pkg/repository/config/gcp.go

@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-//nolint:gosec
+//nolint:gosec // Internal usage. No need to check.
 package config
 
 import "os"

pkg/repository/keys/keys.go

@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-//nolint:gosec
+//nolint:gosec // Internal call. No need to check.
 package keys
 
 import (

pkg/restic/command.go

@@ -77,7 +77,7 @@ func (c *Command) String() string {
 // Cmd returns an exec.Cmd for the command.
 func (c *Command) Cmd() *exec.Cmd {
 	parts := c.StringSlice()
-	cmd := exec.Command(parts[0], parts[1:]...) //nolint
+	cmd := exec.Command(parts[0], parts[1:]...) //nolint:gosec // Internal call. No need to check the parameter.
 	cmd.Dir = c.Dir
 
 	if len(c.Env) > 0 {

pkg/restore/restore.go

@@ -1900,7 +1900,7 @@ func shouldRenamePV(ctx *restoreContext, obj *unstructured.Unstructured, client
 // remapClaimRefNS remaps a PersistentVolume's claimRef.Namespace based on a
 // restore's NamespaceMappings, if necessary. Returns true if the namespace was
 // remapped, false if it was not required.
-func remapClaimRefNS(ctx *restoreContext, obj *unstructured.Unstructured) (bool, error) { //nolint:unparam
+func remapClaimRefNS(ctx *restoreContext, obj *unstructured.Unstructured) (bool, error) { //nolint:unparam // ignore the result 0 (bool) is never used warning.
 	if len(ctx.restore.Spec.NamespaceMapping) == 0 {
 		ctx.log.Debug("Persistent volume does not need to have the claimRef.namespace remapped because restore is not remapping any namespaces")
 		return false, nil
@@ -2315,7 +2315,7 @@ func (ctx *restoreContext) getOrderedResourceCollection(
 // getSelectedRestoreableItems applies Kubernetes selectors on individual items
 // of each resource type to create a list of items which will be actually
 // restored.
-func (ctx *restoreContext) getSelectedRestoreableItems(resource string, namespaceMapping map[string]string, originalNamespace string, items []string) (restoreableResource, results.Result, results.Result) {
+func (ctx *restoreContext) getSelectedRestoreableItems(resource string, namespaceMapping map[string]string, originalNamespace string, items []string) (restoreableResource, results.Result, results.Result) { //nolint:unparam // Ignore the warnings is always nil warning.
 	warnings, errs := results.Result{}, results.Result{}
 
 	restorable := restoreableResource{
@@ -2430,7 +2430,7 @@ func removeRestoreLabels(obj metav1.Object) {
 }
 
 // updates the backup/restore labels
-func (ctx *restoreContext) updateBackupRestoreLabels(fromCluster, fromClusterWithLabels *unstructured.Unstructured, namespace string, resourceClient client.Dynamic) (warnings, errs results.Result) {
+func (ctx *restoreContext) updateBackupRestoreLabels(fromCluster, fromClusterWithLabels *unstructured.Unstructured, namespace string, resourceClient client.Dynamic) (warnings, errs results.Result) { //nolint:unparam // Ignore the warnings is nil warning.
 	patchBytes, err := generatePatch(fromCluster, fromClusterWithLabels)
 	if err != nil {
 		ctx.log.Errorf("error generating patch for %s %s: %v", fromCluster.GroupVersionKind().Kind, kube.NamespaceAndName(fromCluster), err)

pkg/uploader/kopia/progress.go

@@ -33,7 +33,7 @@ type Throttle struct {
 
 func (t *Throttle) ShouldOutput() bool {
 	nextOutputTimeUnixNano := atomic.LoadInt64(&t.throttle)
-	if nowNano := time.Now().UnixNano(); nowNano > nextOutputTimeUnixNano { //nolint:forbidigo
+	if nowNano := time.Now().UnixNano(); nowNano > nextOutputTimeUnixNano {
 		if atomic.CompareAndSwapInt64(&t.throttle, nextOutputTimeUnixNano, nowNano+t.interval.Nanoseconds()) {
 			return true
 		}