From f051ecaee97f40e5c66b8349ac6dec37b4968abd Mon Sep 17 00:00:00 2001 From: Xun Jiang Date: Thu, 27 Apr 2023 20:39:53 +0800 Subject: [PATCH] Enable errcheck linter and resolve found issues. Signed-off-by: Xun Jiang --- changelogs/unreleased/6208-blackpiglet | 1 + golangci.yaml | 1 + internal/delete/delete_item_action_handler.go | 7 +++- pkg/backup/backup.go | 6 ++- pkg/client/factory.go | 16 ++++++-- pkg/cmd/cli/bug/bug.go | 4 +- pkg/cmd/cli/completion/completion.go | 22 ++++++++--- pkg/cmd/cli/nodeagent/server.go | 15 ++++++-- pkg/cmd/server/server.go | 15 ++++++-- pkg/cmd/util/output/output.go | 4 +- pkg/controller/backup_controller.go | 3 +- .../backup_repository_controller.go | 4 +- pkg/controller/backup_sync_controller.go | 6 ++- pkg/controller/restore_controller.go | 5 +-- pkg/install/resources.go | 37 ++++++++++++++----- pkg/plugin/framework/object_store_client.go | 4 +- pkg/plugin/framework/server.go | 5 ++- pkg/restore/restore.go | 6 ++- pkg/test/fake_file_system.go | 6 +-- 19 files changed, 125 insertions(+), 42 deletions(-) create mode 100644 changelogs/unreleased/6208-blackpiglet diff --git a/changelogs/unreleased/6208-blackpiglet b/changelogs/unreleased/6208-blackpiglet new file mode 100644 index 0000000000..2829fcaa55 --- /dev/null +++ b/changelogs/unreleased/6208-blackpiglet @@ -0,0 +1 @@ +Enable errcheck linter and resolve found issues \ No newline at end of file diff --git a/golangci.yaml b/golangci.yaml index 997c709eb0..d8d3633672 100644 --- a/golangci.yaml +++ b/golangci.yaml @@ -302,6 +302,7 @@ linters: - bodyclose - dogsled - durationcheck + - errcheck - exportloopref - goconst - gofmt diff --git a/internal/delete/delete_item_action_handler.go b/internal/delete/delete_item_action_handler.go index f91127d7d8..d85df8a6a1 100644 --- a/internal/delete/delete_item_action_handler.go +++ b/internal/delete/delete_item_action_handler.go @@ -63,7 +63,12 @@ func InvokeDeleteActions(ctx *Context) error { if err != nil { return errors.Wrapf(err, "error extracting backup") } - defer ctx.Filesystem.RemoveAll(dir) + defer func() { + if err := ctx.Filesystem.RemoveAll(dir); err != nil { + ctx.Log.Errorf("error removing temporary directory %s: %s", dir, err.Error()) + } + }() + ctx.Log.Debugf("Downloaded and extracted the backup file to: %s", dir) backupResources, err := archive.NewParser(ctx.Log, ctx.Filesystem).Parse(dir) diff --git a/pkg/backup/backup.go b/pkg/backup/backup.go index 34cb42baad..4067712ddf 100644 --- a/pkg/backup/backup.go +++ b/pkg/backup/backup.go @@ -635,7 +635,11 @@ func (kb *kubernetesBackupper) FinalizeBackup(log logrus.FieldLogger, } // write new tar archive replacing files in original with content updateFiles for matches - buildFinalTarball(tr, tw, updateFiles) + if err := buildFinalTarball(tr, tw, updateFiles); err != nil { + log.Errorf("Error building final tarball: %s", err.Error()) + return err + } + log.WithField("progress", "").Infof("Updated a total of %d items", len(backupRequest.BackedUpItems)) return nil diff --git a/pkg/client/factory.go b/pkg/client/factory.go index aa5119c262..58ea5994bd 100644 --- a/pkg/client/factory.go +++ b/pkg/client/factory.go @@ -153,10 +153,18 @@ func (f *factory) KubebuilderClient() (kbclient.Client, error) { } scheme := runtime.NewScheme() - velerov1api.AddToScheme(scheme) - k8scheme.AddToScheme(scheme) - apiextv1beta1.AddToScheme(scheme) - apiextv1.AddToScheme(scheme) + if err := velerov1api.AddToScheme(scheme); err != nil { + return nil, err + } + if err := k8scheme.AddToScheme(scheme); err != nil { + return nil, err + } + if err := apiextv1beta1.AddToScheme(scheme); err != nil { + return nil, err + } + if err := apiextv1.AddToScheme(scheme); err != nil { + return nil, err + } kubebuilderClient, err := kbclient.New(clientConfig, kbclient.Options{ Scheme: scheme, }) diff --git a/pkg/cmd/cli/bug/bug.go b/pkg/cmd/cli/bug/bug.go index a8237e1c3b..9f87e3f5ab 100644 --- a/pkg/cmd/cli/bug/bug.go +++ b/pkg/cmd/cli/bug/bug.go @@ -162,7 +162,9 @@ func getKubectlVersion() (string, error) { case <-time.After(kubectlTimeout): // we don't care about the possible error returned from Kill() here, // just return an empty string - kubectlCmd.Process.Kill() + if err := kubectlCmd.Process.Kill(); err != nil { + return "", fmt.Errorf("error killing kubectl process: %w", err) + } return "", errors.New("timeout waiting for kubectl version") case err := <-done: diff --git a/pkg/cmd/cli/completion/completion.go b/pkg/cmd/cli/completion/completion.go index ee1c285093..9aff563d4b 100644 --- a/pkg/cmd/cli/completion/completion.go +++ b/pkg/cmd/cli/completion/completion.go @@ -64,7 +64,10 @@ $ velero completion fish > ~/.config/fish/completions/velero.fish shell := args[0] switch shell { case "bash": - cmd.Root().GenBashCompletion(os.Stdout) + if err := cmd.Root().GenBashCompletion(os.Stdout); err != nil { + fmt.Println("fail to generate bash completion script", err) + os.Exit(1) + } case "zsh": // # fix #4912 // cobra does not support zsh completion ouptput used by source command @@ -72,11 +75,20 @@ $ velero completion fish > ~/.config/fish/completions/velero.fish // Need to append compdef manually to do that. zshHead := "#compdef velero\ncompdef _velero velero\n" out := os.Stdout - out.Write([]byte(zshHead)) - - cmd.Root().GenZshCompletion(out) + if _, err := out.Write([]byte(zshHead)); err != nil { + fmt.Println("fail to append compdef command into zsh completion script: ", err) + os.Exit(1) + } + + if err := cmd.Root().GenZshCompletion(out); err != nil { + fmt.Println("fail to generate zsh completion script: ", err) + os.Exit(1) + } case "fish": - cmd.Root().GenFishCompletion(os.Stdout, true) + if err := cmd.Root().GenFishCompletion(os.Stdout, true); err != nil { + fmt.Println("fail to generate fish completion script: ", err) + os.Exit(1) + } default: fmt.Println("Invalid shell specified, specify bash, zsh, or fish") os.Exit(1) diff --git a/pkg/cmd/cli/nodeagent/server.go b/pkg/cmd/cli/nodeagent/server.go index 6b3eed3b03..81183751d0 100644 --- a/pkg/cmd/cli/nodeagent/server.go +++ b/pkg/cmd/cli/nodeagent/server.go @@ -121,9 +121,18 @@ func newNodeAgentServer(logger logrus.FieldLogger, factory client.Factory, metri ctrl.SetLogger(zap.New(zap.UseDevMode(true))) - velerov1api.AddToScheme(scheme) - v1.AddToScheme(scheme) - storagev1api.AddToScheme(scheme) + if err := velerov1api.AddToScheme(scheme); err != nil { + cancelFunc() + return nil, err + } + if err := v1.AddToScheme(scheme); err != nil { + cancelFunc() + return nil, err + } + if err := storagev1api.AddToScheme(scheme); err != nil { + cancelFunc() + return nil, err + } nodeName := os.Getenv("NODE_NAME") diff --git a/pkg/cmd/server/server.go b/pkg/cmd/server/server.go index 0242da7269..904dee663b 100644 --- a/pkg/cmd/server/server.go +++ b/pkg/cmd/server/server.go @@ -313,9 +313,18 @@ func newServer(f client.Factory, config serverConfig, logger *logrus.Logger) (*s } scheme := runtime.NewScheme() - velerov1api.AddToScheme(scheme) - corev1api.AddToScheme(scheme) - snapshotv1api.AddToScheme(scheme) + if err := velerov1api.AddToScheme(scheme); err != nil { + cancelFunc() + return nil, err + } + if err := corev1api.AddToScheme(scheme); err != nil { + cancelFunc() + return nil, err + } + if err := snapshotv1api.AddToScheme(scheme); err != nil { + cancelFunc() + return nil, err + } ctrl.SetLogger(logrusr.New(logger)) diff --git a/pkg/cmd/util/output/output.go b/pkg/cmd/util/output/output.go index e964d698b2..5b3095cd69 100644 --- a/pkg/cmd/util/output/output.go +++ b/pkg/cmd/util/output/output.go @@ -61,7 +61,9 @@ func ClearOutputFlagDefault(cmd *cobra.Command) { return } f.DefValue = "" - f.Value.Set("") + if err := f.Value.Set(""); err != nil { + fmt.Printf("error clear the default value of output flag: %s\n", err.Error()) + } } // GetOutputFlagValue returns the value of the "output" flag diff --git a/pkg/controller/backup_controller.go b/pkg/controller/backup_controller.go index d485562f85..90462a60f7 100644 --- a/pkg/controller/backup_controller.go +++ b/pkg/controller/backup_controller.go @@ -561,8 +561,7 @@ func (b *backupReconciler) validateAndGetSnapshotLocations(backup *velerov1api.B continue } location := &velerov1api.VolumeSnapshotLocation{} - b.kbClient.Get(context.Background(), kbclient.ObjectKey{Namespace: backup.Namespace, Name: defaultLocation}, location) - if err != nil { + if err := b.kbClient.Get(context.Background(), kbclient.ObjectKey{Namespace: backup.Namespace, Name: defaultLocation}, location); err != nil { errors = append(errors, fmt.Sprintf("error getting volume snapshot location named %s: %v", defaultLocation, err)) continue } diff --git a/pkg/controller/backup_repository_controller.go b/pkg/controller/backup_repository_controller.go index 34e4b23e2f..12d2930517 100644 --- a/pkg/controller/backup_repository_controller.go +++ b/pkg/controller/backup_repository_controller.go @@ -96,7 +96,9 @@ func (r *BackupRepoReconciler) invalidateBackupReposForBSL(bslObj client.Object) for i := range list.Items { r.logger.WithField("BSL", bsl.Name).Infof("Invalidating Backup Repository %s", list.Items[i].Name) - r.patchBackupRepository(context.Background(), &list.Items[i], repoNotReady("re-establish on BSL change")) + if err := r.patchBackupRepository(context.Background(), &list.Items[i], repoNotReady("re-establish on BSL change")); err != nil { + r.logger.WithField("BSL", bsl.Name).WithError(err).Errorf("fail to patch BackupRepository %s", list.Items[i].Name) + } } return []reconcile.Request{} diff --git a/pkg/controller/backup_sync_controller.go b/pkg/controller/backup_sync_controller.go index 606077fa12..0ffc0f9777 100644 --- a/pkg/controller/backup_sync_controller.go +++ b/pkg/controller/backup_sync_controller.go @@ -18,6 +18,7 @@ package controller import ( "context" + "fmt" "time" snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v4/apis/volumesnapshot/v1" @@ -397,7 +398,10 @@ func backupSyncSourceOrderFunc(objList client.ObjectList) client.ObjectList { cpBsl := bsl bslArray = append(bslArray, &cpBsl) } - meta.SetList(resultBSLList, bslArray) + if err := meta.SetList(resultBSLList, bslArray); err != nil { + fmt.Printf("fail to sort BSL list: %s", err.Error()) + return &velerov1api.BackupStorageLocationList{} + } return resultBSLList } diff --git a/pkg/controller/restore_controller.go b/pkg/controller/restore_controller.go index 501893f7db..c63de4fa5f 100644 --- a/pkg/controller/restore_controller.go +++ b/pkg/controller/restore_controller.go @@ -305,10 +305,7 @@ func (r *restoreReconciler) validateAndComplete(restore *api.Restore) backupInfo })) backupList := &api.BackupList{} - r.kbClient.List(context.Background(), backupList, &client.ListOptions{ - LabelSelector: selector, - }) - if err != nil { + if err := r.kbClient.List(context.Background(), backupList, &client.ListOptions{LabelSelector: selector}); err != nil { restore.Status.ValidationErrors = append(restore.Status.ValidationErrors, "Unable to list backups for schedule") return backupInfo{} } diff --git a/pkg/install/resources.go b/pkg/install/resources.go index a511374005..0d60e9743a 100644 --- a/pkg/install/resources.go +++ b/pkg/install/resources.go @@ -17,6 +17,7 @@ limitations under the License. package install import ( + "fmt" "time" corev1 "k8s.io/api/core/v1" @@ -250,7 +251,9 @@ func AllCRDs() *unstructured.UnstructuredList { for _, crd := range v1crds.CRDs { crd.SetLabels(Labels()) - appendUnstructured(resources, crd) + if err := appendUnstructured(resources, crd); err != nil { + fmt.Printf("error appending CRD %s: %s\n", crd.GetName(), err.Error()) + } } return resources @@ -262,32 +265,44 @@ func AllResources(o *VeleroOptions) *unstructured.UnstructuredList { resources := AllCRDs() ns := Namespace(o.Namespace) - appendUnstructured(resources, ns) + if err := appendUnstructured(resources, ns); err != nil { + fmt.Printf("error appending Namespace %s: %s\n", ns.GetName(), err.Error()) + } serviceAccountName := defaultServiceAccountName if o.ServiceAccountName == "" { crb := ClusterRoleBinding(o.Namespace) - appendUnstructured(resources, crb) + if err := appendUnstructured(resources, crb); err != nil { + fmt.Printf("error appending ClusterRoleBinding %s: %s\n", crb.GetName(), err.Error()) + } sa := ServiceAccount(o.Namespace, o.ServiceAccountAnnotations) - appendUnstructured(resources, sa) + if err := appendUnstructured(resources, sa); err != nil { + fmt.Printf("error appending ServiceAccount %s: %s\n", sa.GetName(), err.Error()) + } } else { serviceAccountName = o.ServiceAccountName } if o.SecretData != nil { sec := Secret(o.Namespace, o.SecretData) - appendUnstructured(resources, sec) + if err := appendUnstructured(resources, sec); err != nil { + fmt.Printf("error appending Secret %s: %s\n", sec.GetName(), err.Error()) + } } if !o.NoDefaultBackupLocation { bsl := BackupStorageLocation(o.Namespace, o.ProviderName, o.Bucket, o.Prefix, o.BSLConfig, o.CACertData) - appendUnstructured(resources, bsl) + if err := appendUnstructured(resources, bsl); err != nil { + fmt.Printf("error appending BackupStorageLocation %s: %s\n", bsl.GetName(), err.Error()) + } } // A snapshot location may not be desirable for users relying on pod volume backup/restore if o.UseVolumeSnapshots { vsl := VolumeSnapshotLocation(o.Namespace, o.ProviderName, o.VSLConfig) - appendUnstructured(resources, vsl) + if err := appendUnstructured(resources, vsl); err != nil { + fmt.Printf("error appending VolumeSnapshotLocation %s: %s\n", vsl.GetName(), err.Error()) + } } secretPresent := o.SecretData != nil @@ -322,7 +337,9 @@ func AllResources(o *VeleroOptions) *unstructured.UnstructuredList { deploy := Deployment(o.Namespace, deployOpts...) - appendUnstructured(resources, deploy) + if err := appendUnstructured(resources, deploy); err != nil { + fmt.Printf("error appending Deployment %s: %s\n", deploy.GetName(), err.Error()) + } if o.UseNodeAgent { dsOpts := []podTemplateOption{ @@ -337,7 +354,9 @@ func AllResources(o *VeleroOptions) *unstructured.UnstructuredList { dsOpts = append(dsOpts, WithFeatures(o.Features)) } ds := DaemonSet(o.Namespace, dsOpts...) - appendUnstructured(resources, ds) + if err := appendUnstructured(resources, ds); err != nil { + fmt.Printf("error appending DaemonSet %s: %s\n", ds.GetName(), err.Error()) + } } return resources diff --git a/pkg/plugin/framework/object_store_client.go b/pkg/plugin/framework/object_store_client.go index 7f40921c2a..3cc5cd6e99 100644 --- a/pkg/plugin/framework/object_store_client.go +++ b/pkg/plugin/framework/object_store_client.go @@ -87,7 +87,9 @@ func (c *ObjectStoreGRPCClient) PutObject(bucket, key string, body io.Reader) er return nil } if err != nil { - stream.CloseSend() + if err := stream.CloseSend(); err != nil { + return common.FromGRPCError(err) + } return errors.WithStack(err) } diff --git a/pkg/plugin/framework/server.go b/pkg/plugin/framework/server.go index 4fdae230a6..3c3871fac8 100644 --- a/pkg/plugin/framework/server.go +++ b/pkg/plugin/framework/server.go @@ -232,7 +232,10 @@ func getNames(command string, kind common.PluginKind, plugin Interface) []Plugin func (s *server) Serve() { if s.flagSet != nil && !s.flagSet.Parsed() { s.log.Debugf("Parsing flags") - s.flagSet.Parse(os.Args[1:]) + if err := s.flagSet.Parse(os.Args[1:]); err != nil { + s.log.Errorf("fail to parse the flags: %s", err.Error()) + return + } } s.log.Level = s.logLevelFlag.Parse() diff --git a/pkg/restore/restore.go b/pkg/restore/restore.go index 8783da19aa..ad48a94255 100644 --- a/pkg/restore/restore.go +++ b/pkg/restore/restore.go @@ -401,7 +401,11 @@ func (ctx *restoreContext) execute() (results.Result, results.Result) { errs.AddVeleroError(err) return warnings, errs } - defer ctx.fileSystem.RemoveAll(dir) + defer func() { + if err := ctx.fileSystem.RemoveAll(dir); err != nil { + ctx.log.Errorf("error removing temporary directory %s: %s", dir, err.Error()) + } + }() // Need to set this for additionalItems to be restored. ctx.restoreDir = dir diff --git a/pkg/test/fake_file_system.go b/pkg/test/fake_file_system.go index 89dc036f8a..7a744647b4 100644 --- a/pkg/test/fake_file_system.go +++ b/pkg/test/fake_file_system.go @@ -81,7 +81,7 @@ func (fs *FakeFileSystem) Stat(path string) (os.FileInfo, error) { func (fs *FakeFileSystem) WithFile(path string, data []byte) *FakeFileSystem { file, _ := fs.fs.Create(path) - file.Write(data) + _, _ = file.Write(data) file.Close() return fs @@ -89,14 +89,14 @@ func (fs *FakeFileSystem) WithFile(path string, data []byte) *FakeFileSystem { func (fs *FakeFileSystem) WithFileAndMode(path string, data []byte, mode os.FileMode) *FakeFileSystem { file, _ := fs.fs.OpenFile(path, os.O_CREATE|os.O_RDWR, mode) - file.Write(data) + _, _ = file.Write(data) file.Close() return fs } func (fs *FakeFileSystem) WithDirectory(path string) *FakeFileSystem { - fs.fs.MkdirAll(path, 0755) + _ = fs.fs.MkdirAll(path, 0755) return fs }