style: add more linters check and update related code (#777)

* Add more linters check and update related code

* address comments and add more items

* address comments

Author: mallow111

.golangci.yml

@@ -10,7 +10,9 @@ linters:
     - deadcode
     - durationcheck
     - exportloopref
+    - errcheck
     - exhaustive
+    - goconst
     - gofmt
     - goimports
     - gosimple
@@ -19,8 +21,10 @@ linters:
     - ineffassign
     - makezero
     - misspell
+    - nilerr
     - revive
     - structcheck
+    - stylecheck
     - typecheck
     - unused
     - varcheck

controllers/secretproviderclasspodstatus_controller.go

@@ -30,10 +30,8 @@ import (
 	"sigs.k8s.io/secrets-store-csi-driver/pkg/util/secretutil"
 
 	corev1 "k8s.io/api/core/v1"
-	v1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/runtime"
 	apiruntime "k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/wait"
@@ -131,7 +129,7 @@ func (r *SecretProviderClassPodStatusReconciler) Patcher(ctx context.Context) er
 			spcMap[namespace+"/"+spcName] = *spc
 		}
 		// get the pod and check if the pod has a owner reference
-		pod := &v1.Pod{}
+		pod := &corev1.Pod{}
 		err = r.reader.Get(ctx, client.ObjectKey{Namespace: namespace, Name: spcPodStatuses[i].Status.PodName}, pod)
 		if err != nil {
 			return fmt.Errorf("failed to fetch pod during patching, err: %+v", err)
@@ -234,7 +232,7 @@ func (r *SecretProviderClassPodStatusReconciler) Reconcile(ctx context.Context,
 
 	// Obtain the full pod metadata. An object reference is needed for sending
 	// events and the UID is helpful for validating the SPCPS TargetPath.
-	pod := &v1.Pod{}
+	pod := &corev1.Pod{}
 	if err := r.reader.Get(ctx, client.ObjectKey{Namespace: req.Namespace, Name: spcPodStatus.Status.PodName}, pod); err != nil {
 		klog.ErrorS(err, "failed to get pod", "pod", klog.ObjectRef{Namespace: req.Namespace, Name: spcPodStatus.Status.PodName})
 		if apierrors.IsNotFound(err) {
@@ -245,7 +243,7 @@ func (r *SecretProviderClassPodStatusReconciler) Reconcile(ctx context.Context,
 	// skip reconcile if the pod is being terminated
 	// or the pod is in succeeded state (for jobs that complete aren't gc yet)
 	// or the pod is in a failed state (all containers get terminated)
-	if !pod.GetDeletionTimestamp().IsZero() || pod.Status.Phase == v1.PodSucceeded || pod.Status.Phase == v1.PodFailed {
+	if !pod.GetDeletionTimestamp().IsZero() || pod.Status.Phase == corev1.PodSucceeded || pod.Status.Phase == corev1.PodFailed {
 		klog.V(5).InfoS("pod is being terminated, skipping reconcile", "pod", klog.KObj(pod))
 		return ctrl.Result{}, nil
 	}
@@ -478,7 +476,7 @@ func (r *SecretProviderClassPodStatusReconciler) patchSecretWithOwnerRef(ctx con
 
 // secretExists checks if the secret with name and namespace already exists
 func (r *SecretProviderClassPodStatusReconciler) secretExists(ctx context.Context, name, namespace string) (bool, error) {
-	o := &v1.Secret{}
+	o := &corev1.Secret{}
 	secretKey := types.NamespacedName{
 		Namespace: namespace,
 		Name:      name,
@@ -494,7 +492,7 @@ func (r *SecretProviderClassPodStatusReconciler) secretExists(ctx context.Contex
 }
 
 // generateEvent generates an event
-func (r *SecretProviderClassPodStatusReconciler) generateEvent(obj runtime.Object, eventType, reason, message string) {
+func (r *SecretProviderClassPodStatusReconciler) generateEvent(obj apiruntime.Object, eventType, reason, message string) {
 	if obj != nil {
 		r.eventRecorder.Eventf(obj, eventType, reason, message)
 	}

pkg/rotation/reconciler.go

@@ -26,7 +26,6 @@ import (
 
 	secretsstorev1 "sigs.k8s.io/secrets-store-csi-driver/apis/v1"
 	"sigs.k8s.io/secrets-store-csi-driver/controllers"
-	"sigs.k8s.io/secrets-store-csi-driver/pkg/client/clientset/versioned"
 	secretsStoreClient "sigs.k8s.io/secrets-store-csi-driver/pkg/client/clientset/versioned"
 	internalerrors "sigs.k8s.io/secrets-store-csi-driver/pkg/errors"
 	"sigs.k8s.io/secrets-store-csi-driver/pkg/k8s"
@@ -80,7 +79,7 @@ type Reconciler struct {
 	reporter             StatsReporter
 	eventRecorder        record.EventRecorder
 	kubeClient           kubernetes.Interface
-	crdClient            versioned.Interface
+	crdClient            secretsStoreClient.Interface
 	// cache contains v1.Pod, secretsstorev1.SecretProviderClassPodStatus (both filtered on *nodeID),
 	// v1.Secret (filtered on secrets-store.csi.k8s.io/managed=true)
 	cache client.Reader
@@ -410,11 +409,12 @@ func (r *Reconciler) reconcile(ctx context.Context, spcps *secretsstorev1.Secret
 
 		updateFn := func() (bool, error) {
 			err = r.updateSecretProviderClassPodStatus(ctx, spcps)
+			updated := true
 			if err != nil {
 				klog.ErrorS(err, "failed to update latest versions in spc pod status", "spcps", klog.KObj(spcps), "controller", "rotation")
-				return false, nil
+				updated = false
 			}
-			return true, nil
+			return updated, nil
 		}
 
 		if err := wait.ExponentialBackoff(wait.Backoff{

pkg/rotation/reconciler_test.go

@@ -460,7 +460,8 @@ func TestReconcileError(t *testing.T) {
 			server, err := providerfake.NewMocKCSIProviderServer(serverEndpoint)
 			g.Expect(err).NotTo(HaveOccurred())
 			server.SetObjects(test.expectedObjectVersions)
-			server.Start()
+			err = server.Start()
+			g.Expect(err).NotTo(HaveOccurred())
 
 			err = testReconciler.reconcile(context.TODO(), test.secretProviderClassPodStatusToProcess)
 			g.Expect(err).To(HaveOccurred())
@@ -601,7 +602,8 @@ func TestReconcileNoError(t *testing.T) {
 		server, err := providerfake.NewMocKCSIProviderServer(serverEndpoint)
 		g.Expect(err).NotTo(HaveOccurred())
 		server.SetObjects(expectedObjectVersions)
-		server.Start()
+		err = server.Start()
+		g.Expect(err).NotTo(HaveOccurred())
 
 		err = os.WriteFile(secretProviderClassPodStatusToProcess.Status.TargetPath+"/object1", []byte("newdata"), permission)
 		g.Expect(err).NotTo(HaveOccurred())

pkg/secrets-store/nodeserver.go

@@ -56,6 +56,7 @@ const (
 	csipodsa                 = "csi.storage.k8s.io/serviceAccount.name"
 	csipodsatokens           = "csi.storage.k8s.io/serviceAccount.tokens" //nolint
 	secretProviderClassField = "secretProviderClass"
+	osWindows                = "windows"
 )
 
 func (ns *nodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePublishVolumeRequest) (npvr *csi.NodePublishVolumeResponse, err error) {
@@ -73,7 +74,9 @@ func (ns *nodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePublis
 			// again for mount, entire node publish volume is retried
 			if targetPath != "" && mounted {
 				klog.InfoS("unmounting target path as node publish volume failed", "targetPath", targetPath, "pod", klog.ObjectRef{Namespace: podNamespace, Name: podName})
-				ns.mounter.Unmount(targetPath)
+				if err = ns.mounter.Unmount(targetPath); err != nil {
+					klog.ErrorS(err, "failed to unmounting target path")
+				}
 			}
 			ns.reporter.ReportNodePublishErrorCtMetric(providerName, errorReason)
 			return
@@ -245,7 +248,7 @@ func (ns *nodeServer) NodeUnpublishVolume(ctx context.Context, req *csi.NodeUnpu
 
 	// for windows as the target path is not a mount point, we need to explicitly remove the contents from the
 	// dir to be able to cleanup the target path.
-	if runtime.GOOS == "windows" {
+	if runtime.GOOS == osWindows {
 		files, err := filepath.Glob(filepath.Join(targetPath, "*"))
 		if err != nil {
 			klog.ErrorS(err, "failed to get files from target path", "targetPath", targetPath)

pkg/secrets-store/provider_client_test.go

@@ -187,7 +187,9 @@ func TestMountContent(t *testing.T) {
 			server.SetReturnError(test.providerError)
 			server.SetObjects(test.objectVersions)
 			server.SetFiles(test.files)
-			server.Start()
+			if err := server.Start(); err != nil {
+				t.Fatalf("unable to start server :%s", err)
+			}
 
 			client, err := pool.Get(context.Background(), "provider1")
 			if err != nil {
@@ -242,7 +244,9 @@ func TestMountContent_TooLarge(t *testing.T) {
 			Contents: []byte("foo"),
 		},
 	})
-	server.Start()
+	if err := server.Start(); err != nil {
+		t.Fatalf("expected err to be nil, got: %+v", err)
+	}
 
 	client, err := pool.Get(context.Background(), "provider1")
 	if err != nil {
@@ -335,7 +339,9 @@ func TestMountContentError(t *testing.T) {
 			server.SetReturnError(test.providerError)
 			server.SetObjects(test.expectedObjectVersion)
 			server.SetProviderErrorCode(test.expectedErrorCode)
-			server.Start()
+			if err := server.Start(); err != nil {
+				t.Fatalf("expected err to be nil, got: %+v", err)
+			}
 
 			client, err := pool.Get(context.Background(), providerName)
 			if err != nil {
@@ -365,7 +371,9 @@ func TestPluginClientBuilder(t *testing.T) {
 	for i := 0; i < 10; i++ {
 		server, cleanup := fakeServer(t, path, fmt.Sprintf("server-%d", i))
 		defer cleanup()
-		server.Start()
+		if err := server.Start(); err != nil {
+			t.Fatalf("expected err to be nil, got: %+v", err)
+		}
 	}
 
 	var wg sync.WaitGroup
@@ -393,7 +401,9 @@ func TestPluginClientBuilder_ConcurrentGet(t *testing.T) {
 	provider := "server"
 	server, cleanup := fakeServer(t, path, provider)
 	defer cleanup()
-	server.Start()
+	if err := server.Start(); err != nil {
+		t.Fatalf("expected err to be nil, got: %+v", err)
+	}
 
 	var wg sync.WaitGroup
 
@@ -423,7 +433,9 @@ func TestPluginClientBuilderErrorNotFound(t *testing.T) {
 	// check that the provider is found once server is started
 	server, cleanup := fakeServer(t, path, "notfoundprovider")
 	defer cleanup()
-	server.Start()
+	if err := server.Start(); err != nil {
+		t.Fatalf("expected err to be nil, got: %+v", err)
+	}
 
 	if _, err := cb.Get(ctx, "notfoundprovider"); err != nil {
 		t.Errorf("Get(%s) = %v, want nil", "notfoundprovider", err)
@@ -462,7 +474,9 @@ func TestVersion(t *testing.T) {
 			server, cleanup := fakeServer(t, socketPath, "provider1")
 			defer cleanup()
 
-			server.Start()
+			if err := server.Start(); err != nil {
+				t.Fatalf("expected err to be nil, got: %+v", err)
+			}
 
 			client, err := pool.Get(context.Background(), "provider1")
 			if err != nil {
@@ -492,7 +506,9 @@ func TestPluginClientBuilder_HealthCheck(t *testing.T) {
 	provider := "server"
 	server, cleanup := fakeServer(t, path, provider)
 	defer cleanup()
-	server.Start()
+	if err := server.Start(); err != nil {
+		t.Fatalf("expected err to be nil, got: %+v", err)
+	}
 
 	// run the provider healthcheck
 	go cb.HealthCheck(ctx, healthCheckInterval)

pkg/util/fileutil/atomic_writer.go

@@ -131,30 +131,30 @@ func (w *AtomicWriter) Write(payload map[string]FileProjection) error {
 
 	// (2)
 	dataDirPath := filepath.Join(w.targetDir, dataDirName)
-	oldTsDir, err := os.Readlink(dataDirPath)
+	oldTSDir, err := os.Readlink(dataDirPath)
 	if err != nil {
 		if !os.IsNotExist(err) {
 			klog.ErrorS(err, "error reading link for data directory", "logContext", w.logContext)
 			return err
 		}
 		// although Readlink() returns "" on err, don't be fragile by relying on it (since it's not specified in docs)
-		// empty oldTsDir indicates that it didn't exist
-		oldTsDir = ""
+		// empty oldTSDir indicates that it didn't exist
+		oldTSDir = ""
 	}
-	oldTsPath := filepath.Join(w.targetDir, oldTsDir)
+	oldTSPath := filepath.Join(w.targetDir, oldTSDir)
 
 	var pathsToRemove sets.String
 	// if there was no old version, there's nothing to remove
-	if len(oldTsDir) != 0 {
+	if len(oldTSDir) != 0 {
 		// (3)
-		pathsToRemove, err = w.pathsToRemove(cleanPayload, oldTsPath)
+		pathsToRemove, err = w.pathsToRemove(cleanPayload, oldTSPath)
 		if err != nil {
 			klog.ErrorS(err, "error determining user-visible files to remove", "logContext", w.logContext)
 			return err
 		}
 
 		// (4)
-		if should, err := shouldWritePayload(cleanPayload, oldTsPath); err != nil {
+		if should, err := shouldWritePayload(cleanPayload, oldTSPath); err != nil {
 			klog.ErrorS(err, "error determining whether payload should be written to disk", "logContext", w.logContext)
 			return err
 		} else if !should && len(pathsToRemove) == 0 {
@@ -216,9 +216,9 @@ func (w *AtomicWriter) Write(payload map[string]FileProjection) error {
 	}
 
 	// (11)
-	if len(oldTsDir) > 0 {
-		if err = os.RemoveAll(oldTsPath); err != nil {
-			klog.ErrorS(err, "error removing old data directory", "logContext", w.logContext, "oldTsDir", oldTsDir)
+	if len(oldTSDir) > 0 {
+		if err = os.RemoveAll(oldTSPath); err != nil {
+			klog.ErrorS(err, "error removing old data directory", "logContext", w.logContext, "oldTSDir", oldTSDir)
 			return err
 		}
 	}
@@ -281,9 +281,9 @@ func validatePath(targetPath string) error {
 }
 
 // shouldWritePayload returns whether the payload should be written to disk.
-func shouldWritePayload(payload map[string]FileProjection, oldTsDir string) (bool, error) {
+func shouldWritePayload(payload map[string]FileProjection, oldTSDir string) (bool, error) {
 	for userVisiblePath, fileProjection := range payload {
-		shouldWrite, err := shouldWriteFile(filepath.Join(oldTsDir, userVisiblePath), fileProjection.Data)
+		shouldWrite, err := shouldWriteFile(filepath.Join(oldTSDir, userVisiblePath), fileProjection.Data)
 		if err != nil {
 			return false, err
 		}
@@ -314,10 +314,10 @@ func shouldWriteFile(path string, content []byte) (bool, error) {
 // pathsToRemove walks the current version of the data directory and
 // determines which paths should be removed (if any) after the payload is
 // written to the target directory.
-func (w *AtomicWriter) pathsToRemove(payload map[string]FileProjection, oldTsDir string) (sets.String, error) {
+func (w *AtomicWriter) pathsToRemove(payload map[string]FileProjection, oldTSDir string) (sets.String, error) {
 	paths := sets.NewString()
 	visitor := func(path string, info os.FileInfo, err error) error {
-		relativePath := strings.TrimPrefix(path, oldTsDir)
+		relativePath := strings.TrimPrefix(path, oldTSDir)
 		relativePath = strings.TrimPrefix(relativePath, string(os.PathSeparator))
 		if relativePath == "" {
 			return nil
@@ -327,7 +327,7 @@ func (w *AtomicWriter) pathsToRemove(payload map[string]FileProjection, oldTsDir
 		return nil
 	}
 
-	err := filepath.Walk(oldTsDir, visitor)
+	err := filepath.Walk(oldTSDir, visitor)
 	if os.IsNotExist(err) {
 		return nil, nil
 	} else if err != nil {

pkg/util/fileutil/atomic_writer_test.go

@@ -240,7 +240,7 @@ func TestPathsToRemove(t *testing.T) {
 		}
 
 		dataDirPath := filepath.Join(targetDir, dataDirName)
-		oldTsDir, err := os.Readlink(dataDirPath)
+		oldTSDir, err := os.Readlink(dataDirPath)
 		if err != nil && os.IsNotExist(err) {
 			t.Errorf("Data symlink does not exist: %v", dataDirPath)
 			continue
@@ -249,7 +249,7 @@ func TestPathsToRemove(t *testing.T) {
 			continue
 		}
 
-		actual, err := writer.pathsToRemove(tc.payload2, filepath.Join(targetDir, oldTsDir))
+		actual, err := writer.pathsToRemove(tc.payload2, filepath.Join(targetDir, oldTSDir))
 		if err != nil {
 			t.Errorf("%v: unexpected error determining paths to remove: %v", tc.name, err)
 			continue
@@ -747,7 +747,10 @@ func TestMultipleUpdates(t *testing.T) {
 		writer := &AtomicWriter{targetDir: targetDir, logContext: "-test-"}
 
 		for _, payload := range tc.payloads {
-			writer.Write(payload)
+			if err := writer.Write(payload); err != nil {
+				t.Errorf("unexpected error writing payload: %v, err:%v", payload, err)
+				continue
+			}
 
 			checkVolumeContents(targetDir, tc.name, payload, t)
 		}

pkg/util/secretutil/secret_test.go

@@ -337,9 +337,15 @@ func createTestFile(tmpDir, fileName string) (string, error) {
 	if fileName != "" {
 		filePath := fmt.Sprintf("%s/%s", tmpDir, fileName)
 		f, err := os.Create(filePath)
-		f.Write([]byte("test"))
 		defer f.Close()
-		return filePath, err
+		if err != nil {
+			return filePath, err
+		}
+		_, err = f.Write([]byte("test"))
+		if err != nil {
+			return filePath, err
+		}
+		return filePath, nil
 	}
 	return "", nil
 }

provider/fake/fake_server.go

@@ -87,7 +87,11 @@ func (m *MockCSIProviderServer) Start() error {
 	if err != nil {
 		return err
 	}
-	go m.grpcServer.Serve(m.listener)
+	go func() {
+		if err = m.grpcServer.Serve(m.listener); err != nil {
+			return
+		}
+	}()
 	return nil
 }