From 58222239581789e8989952e011bf35ca328d3367 Mon Sep 17 00:00:00 2001 From: hime Date: Tue, 5 Nov 2024 19:00:18 +0000 Subject: [PATCH] Fix nits on own review. --- cmd/metadata_prefetch/main.go | 8 +- cmd/webhook/main.go | 2 +- pkg/webhook/client.go | 38 ++++-- pkg/webhook/client_test.go | 71 ++++++---- pkg/webhook/injection.go | 6 + pkg/webhook/injection_test.go | 250 ++++++++++++++++++++++++++++++++++ pkg/webhook/parsers.go | 12 +- pkg/webhook/parsers_test.go | 42 +++--- pkg/webhook/volumes.go | 4 +- 9 files changed, 354 insertions(+), 79 deletions(-) diff --git a/cmd/metadata_prefetch/main.go b/cmd/metadata_prefetch/main.go index e556f95a..14be08dd 100644 --- a/cmd/metadata_prefetch/main.go +++ b/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) diff --git a/cmd/webhook/main.go b/cmd/webhook/main.go index 01134799..1728d4ce 100644 --- a/cmd/webhook/main.go +++ b/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" diff --git a/pkg/webhook/client.go b/pkg/webhook/client.go index 63745d61..63fede20 100644 --- a/pkg/webhook/client.go +++ b/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) { diff --git a/pkg/webhook/client_test.go b/pkg/webhook/client_test.go index fa9e567e..434d81de 100644 --- a/pkg/webhook/client_test.go +++ b/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) + } + }) } } diff --git a/pkg/webhook/injection.go b/pkg/webhook/injection.go index 42797720..05daeed6 100644 --- a/pkg/webhook/injection.go +++ b/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 != "" { diff --git a/pkg/webhook/injection_test.go b/pkg/webhook/injection_test.go index 97d283db..fb07abd9 100644 --- a/pkg/webhook/injection_test.go +++ b/pkg/webhook/injection_test.go @@ -34,6 +34,171 @@ import ( var UnsupportedVersion = version.MustParseGeneric("1.28.0") +func TestInjectAsNativeSidecar(t *testing.T) { + t.Parallel() + + testCases := []struct { + testName string + cpVersion *version.Version + nodes []corev1.Node + pod *corev1.Pod + expect bool + expectedError error + }{ + { + testName: "test should allow native sidecar", + pod: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + GcsFuseNativeSidecarEnableAnnotation: "true", + }, + }, + }, + cpVersion: minimumSupportedVersion, + nodes: nativeSupportNodes(), + expect: true, + }, + { + testName: "test should not native sidecar by user request", + pod: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + GcsFuseNativeSidecarEnableAnnotation: "false", + }, + }, + }, + cpVersion: minimumSupportedVersion, + nodes: nativeSupportNodes(), + expect: false, + }, + { + testName: "test should be native sidecar, user sent malformed request", + pod: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + GcsFuseNativeSidecarEnableAnnotation: "maybe", + }, + }, + }, + cpVersion: minimumSupportedVersion, + nodes: nativeSupportNodes(), + expect: true, + }, + { + testName: "test should not allow native sidecar, skew", + pod: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + GcsFuseNativeSidecarEnableAnnotation: "true", + }, + }, + }, + cpVersion: minimumSupportedVersion, + nodes: skewVersionNodes(), + expect: false, + }, + { + testName: "test should not allow native sidecar, all under 1.29", + cpVersion: minimumSupportedVersion, + pod: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + GcsFuseNativeSidecarEnableAnnotation: "true", + }, + }, + }, + nodes: regularSidecarSupportNodes(), + expect: false, + }, + { + testName: "test should not allow native sidecar, all nodes are 1.29, cp is 1.28", + cpVersion: UnsupportedVersion, + pod: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + GcsFuseNativeSidecarEnableAnnotation: "true", + }, + }, + }, + nodes: nativeSupportNodes(), + expect: false, + }, + { + testName: "test no nodes present, native sidecar support false", + cpVersion: UnsupportedVersion, + pod: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + GcsFuseNativeSidecarEnableAnnotation: "true", + }, + }, + }, + nodes: []corev1.Node{}, + expect: false, + }, + { + testName: "test no nodes present, allow native sidecar support true", + cpVersion: minimumSupportedVersion, + pod: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + GcsFuseNativeSidecarEnableAnnotation: "true", + }, + }, + }, + nodes: []corev1.Node{}, + expect: true, + }, + { + testName: "test no nodes present, allow native sidecar support false", + cpVersion: minimumSupportedVersion, + pod: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + GcsFuseNativeSidecarEnableAnnotation: "false", + }, + }, + }, + nodes: []corev1.Node{}, + expect: false, + }, + } + for _, tc := range testCases { + t.Run(tc.testName, func(t *testing.T) { + t.Parallel() + + fakeClient := fake.NewSimpleClientset() + // Create the nodes. + for _, node := range tc.nodes { + n := node + _, err := fakeClient.CoreV1().Nodes().Create(context.Background(), &n, metav1.CreateOptions{}) + if err != nil { + t.Error("failed to setup/create nodes") + } + } + + informerFactory := informers.NewSharedInformerFactoryWithOptions(fakeClient, time.Second*1, informers.WithNamespace(metav1.NamespaceAll)) + lister := informerFactory.Core().V1().Nodes().Lister() + si := &SidecarInjector{ + NodeLister: lister, + ServerVersion: tc.cpVersion, + } + + stopCh := make(<-chan struct{}) + informerFactory.Start(stopCh) + informerFactory.WaitForCacheSync(stopCh) + + result, err := si.injectAsNativeSidecar(tc.pod) + if result != tc.expect { + t.Errorf("\nfor %s, got native sidecar support to be: %t, but want: %t", tc.testName, result, tc.expect) + if err != nil { + t.Errorf("error returned from method: %v", err) + } + } + }) + } +} + func TestSupportsNativeSidecar(t *testing.T) { t.Parallel() @@ -1040,6 +1205,91 @@ func TestInjectMetadataPrefetchSidecar(t *testing.T) { }, }, }, + { + testName: "fuse sidecar present & using privately hosted image, injection fail", + pod: &corev1.Pod{ + Spec: corev1.PodSpec{ + InitContainers: []corev1.Container{ + { + Name: GcsFuseSidecarName, + }, + { + Name: "two", + }, + { + Name: "three", + }, + }, + Containers: []corev1.Container{ + { + Name: MetadataPrefetchSidecarName, + Image: "a:a:a:a", + }, + { + Name: "workload-one", + }, + { + Name: "workload-two", + }, + { + Name: "workload-three", + }, + }, + Volumes: []corev1.Volume{ + { + Name: "my-volume", + VolumeSource: corev1.VolumeSource{ + CSI: &corev1.CSIVolumeSource{ + Driver: gcsFuseCsiDriverName, + VolumeAttributes: map[string]string{ + gcsFuseMetadataPrefetchOnMountVolumeAttribute: "true", + }, + }, + }, + }, + }, + }, + }, + expectedPod: &corev1.Pod{ + Spec: corev1.PodSpec{ + InitContainers: []corev1.Container{ + { + Name: GcsFuseSidecarName, + }, + { + Name: "two", + }, + { + Name: "three", + }, + }, + Containers: []corev1.Container{ + { + Name: "workload-one", + }, + { + Name: "workload-two", + }, + { + Name: "workload-three", + }, + }, + Volumes: []corev1.Volume{ + { + Name: "my-volume", + VolumeSource: corev1.VolumeSource{ + CSI: &corev1.CSIVolumeSource{ + Driver: gcsFuseCsiDriverName, + VolumeAttributes: map[string]string{ + gcsFuseMetadataPrefetchOnMountVolumeAttribute: "true", + }, + }, + }, + }, + }, + }, + }, + }, } for _, tc := range testCases { t.Run(tc.testName, func(t *testing.T) { diff --git a/pkg/webhook/parsers.go b/pkg/webhook/parsers.go index 30995273..3253a50b 100644 --- a/pkg/webhook/parsers.go +++ b/pkg/webhook/parsers.go @@ -57,10 +57,8 @@ func ExtractContainerImageAndDeleteContainer(podSpec *corev1.PodSpec, containerN image = podSpec.Containers[index].Image // The next webhook step is to reinject the sidecar, removing user declaration to prevent dual injection creation failures. - if image != "" { - copy(podSpec.Containers[index:], podSpec.Containers[index+1:]) - podSpec.Containers = podSpec.Containers[:len(podSpec.Containers)-1] - } + copy(podSpec.Containers[index:], podSpec.Containers[index+1:]) + podSpec.Containers = podSpec.Containers[:len(podSpec.Containers)-1] if _, _, _, err := parsers.ParseImageName(image); err != nil { return "", fmt.Errorf("could not parse input image: %q, error: %w", image, err) @@ -72,10 +70,8 @@ func ExtractContainerImageAndDeleteContainer(podSpec *corev1.PodSpec, containerN image = podSpec.InitContainers[index].Image // The next webhook step is to reinject the sidecar, removing user declaration to prevent dual injection creation failures. - if image != "" { - copy(podSpec.InitContainers[index:], podSpec.InitContainers[index+1:]) - podSpec.InitContainers = podSpec.InitContainers[:len(podSpec.InitContainers)-1] - } + copy(podSpec.InitContainers[index:], podSpec.InitContainers[index+1:]) + podSpec.InitContainers = podSpec.InitContainers[:len(podSpec.InitContainers)-1] if _, _, _, err := parsers.ParseImageName(image); err != nil { return "", fmt.Errorf("could not parse input image: %q, error: %w", image, err) diff --git a/pkg/webhook/parsers_test.go b/pkg/webhook/parsers_test.go index 4520bd29..48d3ed47 100644 --- a/pkg/webhook/parsers_test.go +++ b/pkg/webhook/parsers_test.go @@ -319,11 +319,7 @@ func TestExtractContainerImageAndDeleteContainer(t *testing.T) { }, expectedPod: corev1.Pod{ Spec: corev1.PodSpec{ - Containers: []corev1.Container{ - { - Name: GcsFuseSidecarName, - }, - }, + Containers: []corev1.Container{}, }, }, expectedImage: "", @@ -331,24 +327,28 @@ func TestExtractContainerImageAndDeleteContainer(t *testing.T) { }, } for _, tc := range testCases { - pod := tc.pod + t.Run(tc.testName, func(t *testing.T) { + t.Parallel() + + pod := tc.pod - image, err := ExtractContainerImageAndDeleteContainer(&pod.Spec, GcsFuseSidecarName) - if image != tc.expectedImage { - t.Errorf(`unexpected image: want: "%s" but got: "%s"`, tc.expectedImage, image) - } - if err != nil && tc.expectedError != nil { - if err.Error() != tc.expectedError.Error() { - t.Error("for test: ", tc.testName, ", want: ", tc.expectedError.Error(), " but got: ", err.Error()) + image, err := ExtractContainerImageAndDeleteContainer(&pod.Spec, GcsFuseSidecarName) + if image != tc.expectedImage { + t.Errorf(`unexpected image: want: "%s" but got: "%s"`, tc.expectedImage, image) + } + if err != nil && tc.expectedError != nil { + if err.Error() != tc.expectedError.Error() { + t.Error("for test: ", tc.testName, ", want: ", tc.expectedError.Error(), " but got: ", err.Error()) + } + } else if err != nil || tc.expectedError != nil { + // if one of them is nil, both must be nil to pass + t.Error("for test: ", tc.testName, ", want: ", tc.expectedError, " but got: ", err) } - } else if err != nil || tc.expectedError != nil { - // if one of them is nil, both must be nil to pass - t.Error("for test: ", tc.testName, ", want: ", tc.expectedError, " but got: ", err) - } - // verifyPod - if diff := cmp.Diff(pod, tc.expectedPod); diff != "" { - t.Errorf(`unexpected pod: diff "%s" want: "%v" but got: "%v"`, diff, tc.expectedPod, pod) - } + // verifyPod + if diff := cmp.Diff(pod, tc.expectedPod); diff != "" { + t.Errorf(`unexpected pod: diff "%s" want: "%v" but got: "%v"`, diff, tc.expectedPod, pod) + } + }) } } diff --git a/pkg/webhook/volumes.go b/pkg/webhook/volumes.go index b718c00a..65954158 100644 --- a/pkg/webhook/volumes.go +++ b/pkg/webhook/volumes.go @@ -48,13 +48,13 @@ func (si *SidecarInjector) isGcsFuseCSIVolume(volume corev1.Volume, namespace st } // Check if the PVC is a preprovisioned parallelstore volume. - isPreprovisionGcsFuseCsi, pv, err := si.IsPreprovisionCSIVolume(gcsFuseCsiDriverName, pvcObj) + pv, err := si.GetPreprovisionCSIVolume(gcsFuseCsiDriverName, pvcObj) if err != nil { klog.Warningf("unable to determine if PVC %s/%s is a pre-provisioned gcsfuse volume: %v", namespace, pvcName, err) return false, nil, nil } - if isPreprovisionGcsFuseCsi { + if pv != nil { return true, pv.Spec.CSI.VolumeAttributes, nil }