Fix nits on own review.

cmd/metadata_prefetch/main.go

@@ -61,7 +61,7 @@ func main() {
 	err := cmd.Start()
 	if err == nil {
 		mountPaths, err := getDirectoryNames(mountPathsLocation)
-		if err != nil {
+		if err == nil {
 			klog.Infof("Running ls on mountPath(s): %s", strings.Join(mountPaths, ", "))
 		} else {
 			klog.Warningf("failed to get mountPaths: %v", err)
@@ -77,14 +77,14 @@ func main() {
 		klog.Errorf("Error starting ls command: %v.", err)
 	}
 
-	klog.Info("going to sleep...")
+	klog.Info("Going to sleep...")
 
 	// Keep the process running.
 	select {}
 }
 
-// getDirectoryNames returns a list of strings with the names of all the directories
-// present within the provided directory path.
+// getDirectoryNames returns a list of strings representing the names of
+// the directories within the provided path.
 func getDirectoryNames(dirPath string) ([]string, error) {
 	directories := []string{}
 	items, err := os.ReadDir(dirPath)

cmd/webhook/main.go

@@ -52,7 +52,7 @@ var (
 	ephemeralStorageRequest = flag.String("sidecar-ephemeral-storage-request", "5Gi", "The default ephemeral storage request for gcsfuse sidecar container.")
 	ephemeralStorageLimit   = flag.String("sidecar-ephemeral-storage-limit", "5Gi", "The default ephemeral storage limit for gcsfuse sidecar container.")
 	sidecarImage            = flag.String("sidecar-image", "", "The gcsfuse sidecar container image.")
-	metadataSidecarImage    = flag.String("metadata-sidecar-image", "", "The gcsfuse sidecar container image.")
+	metadataSidecarImage    = flag.String("metadata-sidecar-image", "", "The metadata prefetch sidecar container image.")
 
 	// These are set at compile time.
 	webhookVersion = "unknown"

pkg/webhook/client.go

@@ -19,45 +19,57 @@ package webhook
 
 import (
 	"errors"
+	"fmt"
 
 	corev1 "k8s.io/api/core/v1"
 )
 
 // IsPreprovisionCSIVolume checks whether the volume is a pre-provisioned volume for the desired csiDriver.
-func (si *SidecarInjector) IsPreprovisionCSIVolume(csiDriver string, pvc *corev1.PersistentVolumeClaim) (bool, *corev1.PersistentVolume, error) {
+func (si *SidecarInjector) IsPreprovisionCSIVolume(csiDriver string, pvc *corev1.PersistentVolumeClaim) (bool, error) {
+	pv, err := si.GetPreprovisionCSIVolume(csiDriver, pvc)
+	if pv != nil {
+		return true, err
+	}
+
+	return false, err
+}
+
+// GetPreprovisionCSIVolume gets the pre-provisioned persistentVolume when backed by the desired csiDriver.
+func (si *SidecarInjector) GetPreprovisionCSIVolume(csiDriver string, pvc *corev1.PersistentVolumeClaim) (*corev1.PersistentVolume, error) {
 	if csiDriver == "" {
-		return false, nil, errors.New("failed to check IsPreprovisionCSIVolume, csiDriver is empty")
+		return nil, errors.New("csiDriver is empty, cannot verify storage type")
 	}
 
 	if pvc == nil {
-		return false, nil, errors.New("failed to check IsPreprovisionCSIVolume, pvc is nil")
+		return nil, errors.New("pvc is nil, cannot get pv")
 	}
 
 	if pvc.Spec.VolumeName == "" {
-		return false, nil, nil
+		return nil, errors.New("pvc.spec.volumeName is not set, cannot find pv")
 	}
 
 	// GetPV returns an error if pvc.Spec.VolumeName is not empty and the associated PV object is not found in the API server.
 	pv, err := si.GetPV(pvc.Spec.VolumeName)
 	if err != nil {
-		return false, nil, err // no additional context needed for error.
+		return nil, err // no additional context needed for error.
 	}
 
 	if pv.Spec.CSI == nil {
-		return false, nil, nil
+		return nil, fmt.Errorf("perisistentVolume is not backed by %s", csiDriver)
 	}
 
-	// PV is using dynamic mounting. See details: https://cloud.google.com/storage/docs/gcsfuse-mount#dynamic-mount
-	if pv.Spec.CSI.VolumeHandle == "_" {
-		return false, nil, nil
+	// PV is using dynamic mounting, which does not count as a preprovisioned volume.
+	// See details: https://cloud.google.com/storage/docs/gcsfuse-mount#dynamic-mount
+	if csiDriver == gcsFuseCsiDriverName && pv.Spec.CSI.VolumeHandle == "_" {
+		return nil, errors.New("persistentVolume is setup using dynamic mounting")
 	}
 
-	if pv.Spec.CSI.Driver == csiDriver {
-		return true, pv, nil
+	// Returns false when PV - PVC pair was created for a different csi driver or different storage type.
+	if pv.Spec.CSI.Driver != csiDriver {
+		return nil, fmt.Errorf("persistentVolume is backed by %s CSI Driver instead of the expected %s CSI driver", pv.Spec.CSI.Driver, csiDriver)
 	}
 
-	// Returns false when PV - PVC pair was created for a different csi driver or different storage type.
-	return false, nil, nil
+	return pv, nil
 }
 
 func (si *SidecarInjector) GetPV(name string) (*corev1.PersistentVolume, error) {

pkg/webhook/client_test.go

@@ -48,14 +48,21 @@ func TestIsPreprovisionCSIVolume(t *testing.T) {
 			csiDriverName:    "fake-csi-driver",
 			pvc:              nil,
 			expectedResponse: false,
-			expectedError:    errors.New(`failed to check IsPreprovisionCSIVolume, pvc is nil`),
+			expectedError:    errors.New(`pvc is nil, cannot get pv`),
+		},
+		{
+			testName:         "given blank csiDriver name",
+			csiDriverName:    "",
+			pvc:              &corev1.PersistentVolumeClaim{},
+			expectedResponse: false,
+			expectedError:    errors.New("csiDriver is empty, cannot verify storage type"),
 		},
 		{
 			testName:         "not preprovisioned pvc",
 			csiDriverName:    "fake-csi-driver",
 			pvc:              &corev1.PersistentVolumeClaim{},
 			expectedResponse: false,
-			expectedError:    nil,
+			expectedError:    errors.New("pvc.spec.volumeName is not set, cannot find pv"),
 		},
 		{
 			testName:      "preprovisioned pvc volume not found",
@@ -103,7 +110,7 @@ func TestIsPreprovisionCSIVolume(t *testing.T) {
 				},
 			},
 			expectedResponse: false,
-			expectedError:    nil,
+			expectedError:    errors.New("persistentVolume is backed by other-csi-driver CSI Driver instead of the expected fake-csi-driver CSI driver"),
 		},
 		{
 			testName:      "preprovisioned pvc for different csi driver",
@@ -161,44 +168,48 @@ func TestIsPreprovisionCSIVolume(t *testing.T) {
 				},
 			},
 			expectedResponse: false,
-			expectedError:    nil,
+			expectedError:    errors.New("perisistentVolume is not backed by fake-csi-driver"),
 		},
 	}
 
 	for _, testcase := range testcases {
-		ctx, cancel := context.WithCancel(context.Background())
-		defer cancel()
-		fakeClient := fake.NewSimpleClientset()
-
-		for _, pvInK8s := range testcase.pvsInK8s {
-			_, err := fakeClient.CoreV1().PersistentVolumes().Create(context.TODO(), &pvInK8s, metav1.CreateOptions{})
-			if err != nil {
-				klog.Errorf("test setup failed: %v", err)
+		t.Run(testcase.testName, func(t *testing.T) {
+			t.Parallel()
+
+			ctx, cancel := context.WithCancel(context.Background())
+			defer cancel()
+			fakeClient := fake.NewSimpleClientset()
+
+			for _, pvInK8s := range testcase.pvsInK8s {
+				_, err := fakeClient.CoreV1().PersistentVolumes().Create(context.TODO(), &pvInK8s, metav1.CreateOptions{})
+				if err != nil {
+					klog.Errorf("test setup failed: %v", err)
+				}
 			}
-		}
 
-		csiGroupClient := SidecarInjector{}
+			csiGroupClient := SidecarInjector{}
 
-		informer := informers.NewSharedInformerFactoryWithOptions(fakeClient, resyncDuration, informers.WithNamespace(metav1.NamespaceAll))
-		csiGroupClient.PvLister = informer.Core().V1().PersistentVolumes().Lister()
-		csiGroupClient.PvcLister = informer.Core().V1().PersistentVolumeClaims().Lister()
+			informer := informers.NewSharedInformerFactoryWithOptions(fakeClient, resyncDuration, informers.WithNamespace(metav1.NamespaceAll))
+			csiGroupClient.PvLister = informer.Core().V1().PersistentVolumes().Lister()
+			csiGroupClient.PvcLister = informer.Core().V1().PersistentVolumeClaims().Lister()
 
-		informer.Start(ctx.Done())
-		informer.WaitForCacheSync(ctx.Done())
+			informer.Start(ctx.Done())
+			informer.WaitForCacheSync(ctx.Done())
 
-		response, _, err := csiGroupClient.IsPreprovisionCSIVolume(testcase.csiDriverName, testcase.pvc)
-		if err != nil && testcase.expectedError != nil {
-			if err.Error() != testcase.expectedError.Error() {
-				t.Error("for test: ", testcase.testName, ", want: ", testcase.expectedError.Error(), " but got: ", err.Error())
+			response, err := csiGroupClient.IsPreprovisionCSIVolume(testcase.csiDriverName, testcase.pvc)
+			if err != nil && testcase.expectedError != nil {
+				if err.Error() != testcase.expectedError.Error() {
+					t.Error("for test: ", testcase.testName, ", want: ", testcase.expectedError.Error(), " but got: ", err.Error())
+				}
+			} else if err != nil || testcase.expectedError != nil {
+				// if one of them is nil, both must be nil to pass
+				t.Error("for test: ", testcase.testName, ", want: ", testcase.expectedError, " but got: ", err)
 			}
-		} else if err != nil || testcase.expectedError != nil {
-			// if one of them is nil, both must be nil to pass
-			t.Error("for test: ", testcase.testName, ", want: ", testcase.expectedError, " but got: ", err)
-		}
 
-		if response != testcase.expectedResponse {
-			t.Error("for test: ", testcase.testName, ", want: ", testcase.expectedResponse, " but got: ", response)
-		}
+			if response != testcase.expectedResponse {
+				t.Error("for test: ", testcase.testName, ", want: ", testcase.expectedResponse, " but got: ", response)
+			}
+		})
 	}
 }
 

pkg/webhook/injection.go

@@ -41,6 +41,10 @@ func (si *SidecarInjector) injectAsNativeSidecar(pod *corev1.Pod) (bool, error)
 			klog.Errorf("failed to parse enableNativeSidecar annotation: %v", err)
 		} else {
 			nativeSidecarEnabled = parsedAnnotation
+			// Warn the user if they used annotation incorrectly.
+			if nativeSidecarEnabled && !supportsNativeSidecar {
+				klog.Errorf("attempting to enable native sidecar on a cluster that does not support it, this is not allowed")
+			}
 		}
 	}
 
@@ -98,6 +102,8 @@ func (si *SidecarInjector) injectMetadataPrefetchSidecarContainer(pod *corev1.Po
 	userProvidedMetadataPrefetchSidecarImage, err := ExtractContainerImageAndDeleteContainer(&pod.Spec, MetadataPrefetchSidecarName)
 	if err != nil {
 		klog.Errorf("failed to get user provided metadata prefetch image... skipping injection.")
+
+		return
 	}
 
 	if userProvidedMetadataPrefetchSidecarImage != "" {