From d9c2f3f936ccbf536ca8a6672a14b0b59d2f661d Mon Sep 17 00:00:00 2001 From: Igor Shishkin Date: Wed, 10 Jul 2024 22:19:12 -0700 Subject: [PATCH 1/4] feat: add allowSharedKeyAccess parameter --- docs/driver-parameters.md | 1 + pkg/blob/blob.go | 1 + pkg/blob/controllerserver.go | 11 +++++++++++ 3 files changed, 13 insertions(+) diff --git a/docs/driver-parameters.md b/docs/driver-parameters.md index 25f29b9ff..bd7e54db2 100644 --- a/docs/driver-parameters.md +++ b/docs/driver-parameters.md @@ -31,6 +31,7 @@ containerNamePrefix | specify Azure storage directory prefix created by driver | server | specify Azure storage account server address | existing server address, e.g. `accountname.privatelink.blob.core.windows.net` | No | if empty, driver will use default `accountname.blob.core.windows.net` or other sovereign cloud account address accessTier | [Access tier for storage account](https://learn.microsoft.com/en-us/azure/storage/blobs/access-tiers-overview) | Standard account can choose `Hot` or `Cool`, and Premium account can only choose `Premium` | No | empty(use default setting for different storage account types) allowBlobPublicAccess | Allow or disallow public access to all blobs or containers for storage account created by driver | `true`,`false` | No | `false` +allowSharedKeyAccess | Allow or disallow shared key access for storage account created by driver | `true`,`false` | No | `true` requireInfraEncryption | specify whether or not the service applies a secondary layer of encryption with platform managed keys for data at rest for storage account created by driver | `true`,`false` | No | `false` storageEndpointSuffix | specify Azure storage endpoint suffix | `core.windows.net`, `core.chinacloudapi.cn`, etc | No | if empty, driver will use default storage endpoint suffix according to cloud environment tags | [tags](https://docs.microsoft.com/en-us/azure/azure-resource-manager/management/tag-resources) would be created in newly created storage account | tag format: 'foo=aaa,bar=bbb' | No | "" diff --git a/pkg/blob/blob.go b/pkg/blob/blob.go index 82a00d41c..0ae551961 100644 --- a/pkg/blob/blob.go +++ b/pkg/blob/blob.go @@ -94,6 +94,7 @@ const ( keyVaultSecretVersionField = "keyvaultsecretversion" storageAccountNameField = "storageaccountname" allowBlobPublicAccessField = "allowblobpublicaccess" + allowSharedKeyAccessField = "allowsharedkeyaccess" requireInfraEncryptionField = "requireinfraencryption" ephemeralField = "csi.storage.k8s.io/ephemeral" podNamespaceField = "csi.storage.k8s.io/pod.namespace" diff --git a/pkg/blob/controllerserver.go b/pkg/blob/controllerserver.go index 44e4d31dc..b0554860f 100644 --- a/pkg/blob/controllerserver.go +++ b/pkg/blob/controllerserver.go @@ -105,6 +105,8 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest) var err error // set allowBlobPublicAccess as false by default allowBlobPublicAccess := pointer.Bool(false) + // set allowBlobPublicAccess as true by default + allowSharedKeyAccess := pointer.Bool(true) containerNameReplaceMap := map[string]string{} @@ -171,6 +173,10 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest) if strings.EqualFold(v, trueValue) { allowBlobPublicAccess = pointer.Bool(true) } + case allowSharedKeyAccessField: + if strings.EqualFold(v, falseValue) { + allowSharedKeyAccess = pointer.Bool(false) + } case requireInfraEncryptionField: if strings.EqualFold(v, trueValue) { requireInfraEncryption = pointer.Bool(true) @@ -310,6 +316,10 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest) storageEndpointSuffix = d.getStorageEndPointSuffix() } + if storeAccountKey && !pointer.BoolDeref(allowSharedKeyAccess, false) { + return nil, status.Errorf(codes.InvalidArgument, "storeAccountKey is not supported for account with shared access key disabled") + } + accountOptions := &azure.AccountOptions{ Name: account, Type: storageAccountType, @@ -324,6 +334,7 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest) IsHnsEnabled: isHnsEnabled, EnableNfsV3: enableNfsV3, AllowBlobPublicAccess: allowBlobPublicAccess, + AllowSharedKeyAccess: allowSharedKeyAccess, RequireInfrastructureEncryption: requireInfraEncryption, VNetResourceGroup: vnetResourceGroup, VNetName: vnetName, From 405834ee180ad6d8dc8a1d74a1cc376b742b6571 Mon Sep 17 00:00:00 2001 From: Igor Shishkin Date: Thu, 11 Jul 2024 10:08:27 -0700 Subject: [PATCH 2/4] Add basic test --- test/e2e/dynamic_provisioning_test.go | 34 +++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/test/e2e/dynamic_provisioning_test.go b/test/e2e/dynamic_provisioning_test.go index 05a09a659..0112bc11d 100644 --- a/test/e2e/dynamic_provisioning_test.go +++ b/test/e2e/dynamic_provisioning_test.go @@ -92,6 +92,40 @@ var _ = ginkgo.Describe("[blob-csi-e2e] Dynamic Provisioning", func() { test.Run(ctx, cs, ns) }) + ginkgo.It("should create a volume on demand with storage account having shared access key disabled", func(ctx ginkgo.SpecContext) { + pods := []testsuites.PodDetails{ + { + Cmd: "echo 'hello world' > /mnt/test-1/data && grep 'hello world' /mnt/test-1/data", + Volumes: []testsuites.VolumeDetails{ + { + ClaimSize: "10Gi", + MountOptions: []string{ + "-o allow_other", + "--file-cache-timeout-in-seconds=120", + "--cancel-list-on-mount-seconds=0", + }, + VolumeMount: testsuites.VolumeMountDetails{ + NameGenerate: "test-volume-", + MountPathGenerate: "/mnt/test-", + }, + }, + }, + }, + } + test := testsuites.DynamicallyProvisionedCmdVolumeTest{ + CSIDriver: testDriver, + Pods: pods, + StorageClassParameters: map[string]string{ + "skuName": "Standard_GRS", + "storeAccountKey": "false", + "allowSharedKeyAccess": "false", + "AzureStorageAuthType": "msi", + "AzureStorageIdentityClientID": "dummy", + }, + } + test.Run(ctx, cs, ns) + }) + ginkgo.It("should create a volume on demand with mount options", func(ctx ginkgo.SpecContext) { pods := []testsuites.PodDetails{ { From e4985f9b41ef612be0dcb71813ad0a237ba3509e Mon Sep 17 00:00:00 2001 From: Igor Shishkin Date: Fri, 12 Jul 2024 12:24:08 -0700 Subject: [PATCH 3/4] PR feedback --- pkg/blob/controllerserver.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/pkg/blob/controllerserver.go b/pkg/blob/controllerserver.go index b0554860f..8879b4185 100644 --- a/pkg/blob/controllerserver.go +++ b/pkg/blob/controllerserver.go @@ -97,7 +97,7 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest) parameters = make(map[string]string) } var storageAccountType, subsID, resourceGroup, location, account, containerName, containerNamePrefix, protocol, customTags, secretName, secretNamespace, pvcNamespace, tagValueDelimiter string - var isHnsEnabled, requireInfraEncryption, enableBlobVersioning, createPrivateEndpoint, enableNfsV3 *bool + var isHnsEnabled, requireInfraEncryption, enableBlobVersioning, createPrivateEndpoint, enableNfsV3, allowSharedKeyAccess *bool var vnetResourceGroup, vnetName, subnetName, accessTier, networkEndpointType, storageEndpointSuffix, fsGroupChangePolicy string var matchTags, useDataPlaneAPI, getLatestAccountKey bool var softDeleteBlobs, softDeleteContainers int32 @@ -105,8 +105,6 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest) var err error // set allowBlobPublicAccess as false by default allowBlobPublicAccess := pointer.Bool(false) - // set allowBlobPublicAccess as true by default - allowSharedKeyAccess := pointer.Bool(true) containerNameReplaceMap := map[string]string{} @@ -174,9 +172,11 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest) allowBlobPublicAccess = pointer.Bool(true) } case allowSharedKeyAccessField: - if strings.EqualFold(v, falseValue) { - allowSharedKeyAccess = pointer.Bool(false) + var boolValue bool + if boolValue, err = strconv.ParseBool(v); err != nil { + return nil, status.Errorf(codes.InvalidArgument, "invalid %s: %s in volume context", allowSharedKeyAccessField, v) } + allowSharedKeyAccess = pointer.Bool(boolValue) case requireInfraEncryptionField: if strings.EqualFold(v, trueValue) { requireInfraEncryption = pointer.Bool(true) @@ -316,7 +316,7 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest) storageEndpointSuffix = d.getStorageEndPointSuffix() } - if storeAccountKey && !pointer.BoolDeref(allowSharedKeyAccess, false) { + if storeAccountKey && !pointer.BoolDeref(allowSharedKeyAccess, true) { return nil, status.Errorf(codes.InvalidArgument, "storeAccountKey is not supported for account with shared access key disabled") } From 99289274ff984a27bf323f98ff245a4b5321bf6f Mon Sep 17 00:00:00 2001 From: andyzhangx Date: Sun, 14 Jul 2024 03:01:55 +0000 Subject: [PATCH 4/4] test: fix e2e test fix --- test/e2e/dynamic_provisioning_test.go | 41 +++------------------------ 1 file changed, 4 insertions(+), 37 deletions(-) diff --git a/test/e2e/dynamic_provisioning_test.go b/test/e2e/dynamic_provisioning_test.go index 0112bc11d..02dfb3b03 100644 --- a/test/e2e/dynamic_provisioning_test.go +++ b/test/e2e/dynamic_provisioning_test.go @@ -92,40 +92,6 @@ var _ = ginkgo.Describe("[blob-csi-e2e] Dynamic Provisioning", func() { test.Run(ctx, cs, ns) }) - ginkgo.It("should create a volume on demand with storage account having shared access key disabled", func(ctx ginkgo.SpecContext) { - pods := []testsuites.PodDetails{ - { - Cmd: "echo 'hello world' > /mnt/test-1/data && grep 'hello world' /mnt/test-1/data", - Volumes: []testsuites.VolumeDetails{ - { - ClaimSize: "10Gi", - MountOptions: []string{ - "-o allow_other", - "--file-cache-timeout-in-seconds=120", - "--cancel-list-on-mount-seconds=0", - }, - VolumeMount: testsuites.VolumeMountDetails{ - NameGenerate: "test-volume-", - MountPathGenerate: "/mnt/test-", - }, - }, - }, - }, - } - test := testsuites.DynamicallyProvisionedCmdVolumeTest{ - CSIDriver: testDriver, - Pods: pods, - StorageClassParameters: map[string]string{ - "skuName": "Standard_GRS", - "storeAccountKey": "false", - "allowSharedKeyAccess": "false", - "AzureStorageAuthType": "msi", - "AzureStorageIdentityClientID": "dummy", - }, - } - test.Run(ctx, cs, ns) - }) - ginkgo.It("should create a volume on demand with mount options", func(ctx ginkgo.SpecContext) { pods := []testsuites.PodDetails{ { @@ -619,9 +585,10 @@ var _ = ginkgo.Describe("[blob-csi-e2e] Dynamic Provisioning", func() { CSIDriver: testDriver, Pods: pods, StorageClassParameters: map[string]string{ - "skuName": "Premium_LRS", - "protocol": "nfs", - "mountPermissions": "0", + "skuName": "Premium_LRS", + "protocol": "nfs", + "mountPermissions": "0", + "allowSharedKeyAccess": "false", }, } test.Run(ctx, cs, ns)