Repository: kubernetes-sigs/azuredisk-csi-driver
Pull request #693 (2021-01-22 08:15:37)

fix: throw error when unsupported resources are requested on Azure Stack
<!--  Thanks for sending a pull request!  Here are some tips for you:

1. If this is your first time, please read our contributor guidelines: https://git.k8s.io/community/contributors/guide#your-first-contribution and developer guide https://git.k8s.io/community/contributors/devel/development.md#development-guide
2. Please label this pull request according to what type of issue you are addressing, especially if this is a release targeted pull request. For reference on required PR/issue labels, read here:
https://github.com/kubernetes/community/blob/master/contributors/devel/sig-release/release.md#issue-kind-label
3. Ensure you have added or ran the appropriate tests for your PR: https://github.com/kubernetes/community/blob/master/contributors/devel/sig-testing/testing.md
4. If you want *faster* PR reviews, read how: https://git.k8s.io/community/contributors/guide/pull-requests.md#best-practices-for-faster-reviews
5. Follow the instructions for writing a release note: https://git.k8s.io/community/contributors/guide/release-notes.md
6. If the PR is unfinished, see how to mark it: https://git.k8s.io/community/contributors/guide/pull-requests.md#marking-unfinished-pull-requests
-->

**What type of PR is this?**

<!--
Add one of the following kinds:
/kind bug
/kind test
/kind cleanup
/kind documentation
/kind feature
/kind design

Optionally add one or more of the following kinds if applicable:
/kind api-change
/kind deprecation
/kind failing-test
/kind flake
/kind regression
-->
/kind bug
**What this PR does / why we need it**:
Throw error when unsupported resources are requested on Azure Stack.
Change the test logic accordingly.
Add limitations for Azure Stack.

**Which issue(s) this PR fixes**:
<!-- 
*Automatically closes linked issue when PR is merged.
Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`.
_If PR is about `failing-tests or flakes`, please post the related issues/tests in a comment and do not use `Fixes`_*
-->
Fixes #

**Requirements**:
- [ ] uses [conventional commit messages](https://www.conventionalcommits.org/)
  <!-- Common commit types:
        build: Build 🏭
        chore: Maintenance 🔧
        ci: Continuous Integration 💜
        docs: Documentation 📘
        feat: Features 🌈
        fix: Bug Fixes 🐞
        perf: Performance Improvements 🚀
        refactor: Code Refactoring 💎
        revert: Revert Change ◀️
        style: Code Style 🎶
        security: Security Fix 🛡️
        test: Testing 💚 -->
- [ ] includes documentation
- [ ] adds unit tests
- [ ] tested upgrade from previous version

**Special notes for your reviewer**:


**Release note**:
```
none
```



Patch:
diff --git a/README.md b/README.md
index 2d629956f..09f00b9ec 100644
--- a/README.md
+++ b/README.md
@@ -50,6 +50,9 @@ Please refer to [`disk.csi.azure.com` driver parameters](./docs/driver-parameter
 ### Support
  - Please see our [support policy][support-policy]
 
+### Limitations
+ - Please refer to [Azure Disk CSI Driver Limitations](./docs/limitations.md)
+
 ## Kubernetes Development
  - Please refer to [development guide](./docs/csi-dev.md)
 
diff --git a/deploy/example/snapshot/storageclass-azuredisk-snapshot.yaml b/deploy/example/snapshot/storageclass-azuredisk-snapshot.yaml
index 7dcb9cb30..e89e55531 100644
--- a/deploy/example/snapshot/storageclass-azuredisk-snapshot.yaml
+++ b/deploy/example/snapshot/storageclass-azuredisk-snapshot.yaml
@@ -6,4 +6,4 @@ metadata:
 driver: disk.csi.azure.com
 deletionPolicy: Delete
 parameters:
-  incremental: "true"  # available values: "true", "false" ("true" by default)
+  incremental: "true"  # available values: "true", "false" ("true" by default for Azure Public Cloud, and "false" by default for Azure Stack Cloud)
diff --git a/deploy/example/storageclass-azuredisk-csi-azurestack.yaml b/deploy/example/storageclass-azuredisk-csi-azurestack.yaml
new file mode 100644
index 000000000..e8590389a
--- /dev/null
+++ b/deploy/example/storageclass-azuredisk-csi-azurestack.yaml
@@ -0,0 +1,11 @@
+---
+apiVersion: storage.k8s.io/v1
+kind: StorageClass
+metadata:
+  name: managed-csi
+provisioner: disk.csi.azure.com
+parameters:
+  skuname: Standard_LRS  # alias: storageaccounttype, available values: Standard_LRS, Premium_LRS
+reclaimPolicy: Delete
+volumeBindingMode: Immediate
+allowVolumeExpansion: true
diff --git a/docs/limitations.md b/docs/limitations.md
new file mode 100644
index 000000000..21d820ff5
--- /dev/null
+++ b/docs/limitations.md
@@ -0,0 +1,8 @@
+# Azure Disk CSI Driver Limitations
+
+## Limitations on Azure Stack
+ - Azure Disk IOPS is capped at 2300, please read this [documentation](https://docs.microsoft.com/en-us/azure-stack/user/azure-stack-vm-sizes?view=azs-2008) for details.
+ - Azure Stack does not support shared disk, so parameter `maxShares` larger than 1 is not valid in a `StorageClass`.
+ - Azure Stack only supports Standard Locally-redundant (Standard_LRS) and Premium Locally-redundant (Premium_LRS) Storage Account types, so only `Standard_LRS` and `Premium_LRS` are valid for parameter `skuName` in a `StorageClass`.
+ - Azure Stack does not support incremental disk Snapshot, so only `false` is valid for parameter `incremental` in a `VolumeSnapshotClass`.
+ - For Windows agent nodes, you will need to install Windows CSI Proxy, please refer to [Windows CSI Proxy](https://github.com/kubernetes-csi/csi-proxy). To enable the proxy via AKS Engine API model, please refer to [CSI Proxy for Windows](https://github.com/Azure/aks-engine/blob/master/docs/topics/csi-proxy-windows.md).
\ No newline at end of file
diff --git a/hack/verify-examples.sh b/hack/verify-examples.sh
index 56d4f1578..264bcc348 100755
--- a/hack/verify-examples.sh
+++ b/hack/verify-examples.sh
@@ -17,7 +17,11 @@ set -euo pipefail
 
 echo "begin to create deployment examples ..."
 
-kubectl apply -f deploy/example/storageclass-azuredisk-csi.yaml
+if [[ "$2" == "azurestackcloud" ]]; then
+	kubectl apply -f deploy/example/storageclass-azuredisk-csi-azurestack.yaml
+else
+    kubectl apply -f deploy/example/storageclass-azuredisk-csi.yaml
+fi
 
 if [[ "$1" == "linux" ]]; then
     kubectl apply -f deploy/example/deployment.yaml
diff --git a/pkg/azuredisk/azure.go b/pkg/azuredisk/azure.go
index 5bc4a00c3..1fe8bec15 100644
--- a/pkg/azuredisk/azure.go
+++ b/pkg/azuredisk/azure.go
@@ -37,8 +37,8 @@ var (
 )
 
 // IsAzureStackCloud decides whether the driver is running on Azure Stack Cloud.
-func IsAzureStackCloud(cloud *azure.Cloud) bool {
-	return strings.EqualFold(cloud.Config.Cloud, "AZURESTACKCLOUD")
+func IsAzureStackCloud(cloud string) bool {
+	return strings.EqualFold(cloud, "AZURESTACKCLOUD")
 }
 
 // GetCloudProvider get Azure Cloud Provider
diff --git a/pkg/azuredisk/azure_common.go b/pkg/azuredisk/azure_common.go
index d6fdbf605..fe3621f31 100644
--- a/pkg/azuredisk/azure_common.go
+++ b/pkg/azuredisk/azure_common.go
@@ -32,8 +32,9 @@ import (
 )
 
 const (
-	defaultStorageAccountType       = compute.StandardSSDLRS
-	defaultAzureDataDiskCachingMode = v1.AzureDataDiskCachingReadOnly
+	azurePublicCloudDefaultStorageAccountType = compute.StandardSSDLRS
+	azureStackCloudDefaultStorageAccountType  = compute.StandardLRS
+	defaultAzureDataDiskCachingMode           = v1.AzureDataDiskCachingReadOnly
 )
 
 var (
@@ -45,13 +46,19 @@ var (
 	lunPathRE = regexp.MustCompile(`/dev(?:.*)/disk/azure/scsi(?:.*)/lun(.+)`)
 )
 
-func normalizeStorageAccountType(storageAccountType string) (compute.DiskStorageAccountTypes, error) {
+func normalizeStorageAccountType(storageAccountType, cloud string) (compute.DiskStorageAccountTypes, error) {
 	if storageAccountType == "" {
-		return defaultStorageAccountType, nil
+		if IsAzureStackCloud(cloud) {
+			return azureStackCloudDefaultStorageAccountType, nil
+		}
+		return azurePublicCloudDefaultStorageAccountType, nil
 	}
 
 	sku := compute.DiskStorageAccountTypes(storageAccountType)
 	supportedSkuNames := compute.PossibleDiskStorageAccountTypesValues()
+	if IsAzureStackCloud(cloud) {
+		supportedSkuNames = []compute.DiskStorageAccountTypes{compute.StandardLRS, compute.PremiumLRS}
+	}
 	for _, s := range supportedSkuNames {
 		if sku == s {
 			return sku, nil
diff --git a/pkg/azuredisk/controllerserver.go b/pkg/azuredisk/controllerserver.go
index 7af7d4efd..88e35290e 100644
--- a/pkg/azuredisk/controllerserver.go
+++ b/pkg/azuredisk/controllerserver.go
@@ -165,7 +165,7 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest)
 	}
 	parameters[resizeRequired] = strconv.FormatBool(false)
 
-	if IsAzureStackCloud(d.cloud) {
+	if IsAzureStackCloud(d.cloud.Config.Cloud) {
 		if maxShares > 1 {
 			return nil, status.Error(codes.InvalidArgument, fmt.Sprintf("Invalid maxShares value: %d as Azure Stack does not support shared disk.", maxShares))
 		}
@@ -191,18 +191,11 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest)
 	}
 
 	// normalize values
-	skuName, err := normalizeStorageAccountType(storageAccountType)
+	skuName, err := normalizeStorageAccountType(storageAccountType, d.cloud.Config.Cloud)
 	if err != nil {
 		return nil, err
 	}
 
-	if IsAzureStackCloud(d.cloud) {
-		if skuName != compute.StandardLRS && skuName != compute.PremiumLRS {
-			klog.Warningf("Use sku %s instead as Azure Stack does not support %s.", compute.StandardLRS, skuName)
-			skuName = compute.StandardLRS
-		}
-	}
-
 	if _, err = normalizeCachingMode(cachingMode); err != nil {
 		return nil, err
 	}
@@ -668,7 +661,7 @@ func (d *Driver) CreateSnapshot(ctx context.Context, req *csi.CreateSnapshotRequ
 		}
 	}
 
-	if IsAzureStackCloud(d.cloud) {
+	if IsAzureStackCloud(d.cloud.Config.Cloud) {
 		klog.V(2).Info("Use full snapshot instead as Azure Stack does not incremental snapshot.")
 		incremental = false
 	}


Test patch:
diff --git a/pkg/azuredisk/azure_common_test.go b/pkg/azuredisk/azure_common_test.go
index 90c2bc51a..17102613f 100644
--- a/pkg/azuredisk/azure_common_test.go
+++ b/pkg/azuredisk/azure_common_test.go
@@ -139,44 +139,63 @@ func TestIoHandler(t *testing.T) {
 
 func TestNormalizeStorageAccountType(t *testing.T) {
 	tests := []struct {
+		cloud               string
 		storageAccountType  string
 		expectedAccountType compute.DiskStorageAccountTypes
 		expectError         bool
 	}{
 		{
+			cloud:               "AZUREPUBLICCLOUD",
 			storageAccountType:  "",
 			expectedAccountType: compute.StandardSSDLRS,
 			expectError:         false,
 		},
 		{
+			cloud:               "AZURESTACKCLOUD",
+			storageAccountType:  "",
+			expectedAccountType: compute.StandardLRS,
+			expectError:         false,
+		},
+		{
+			cloud:               "AZUREPUBLICCLOUD",
 			storageAccountType:  "NOT_EXISTING",
 			expectedAccountType: "",
 			expectError:         true,
 		},
 		{
+			cloud:               "AZUREPUBLICCLOUD",
 			storageAccountType:  "Standard_LRS",
 			expectedAccountType: compute.StandardLRS,
 			expectError:         false,
 		},
 		{
+			cloud:               "AZUREPUBLICCLOUD",
 			storageAccountType:  "Premium_LRS",
 			expectedAccountType: compute.PremiumLRS,
 			expectError:         false,
 		},
 		{
+			cloud:               "AZUREPUBLICCLOUD",
 			storageAccountType:  "StandardSSD_LRS",
 			expectedAccountType: compute.StandardSSDLRS,
 			expectError:         false,
 		},
 		{
+			cloud:               "AZUREPUBLICCLOUD",
 			storageAccountType:  "UltraSSD_LRS",
 			expectedAccountType: compute.UltraSSDLRS,
 			expectError:         false,
 		},
+		{
+			cloud:               "AZURESTACKCLOUD",
+			storageAccountType:  "UltraSSD_LRS",
+			expectedAccountType: "",
+			expectError:         true,
+		},
 	}
 
 	for _, test := range tests {
-		result, err := normalizeStorageAccountType(test.storageAccountType)
+		result, err := normalizeStorageAccountType(test.storageAccountType, test.cloud)
 		assert.Equal(t, result, test.expectedAccountType)
 		assert.Equal(t, err != nil, test.expectError, fmt.Sprintf("error msg: %v", err))
 	}
diff --git a/test/e2e/dynamic_provisioning_test.go b/test/e2e/dynamic_provisioning_test.go
index 7e628e67c..cad78d8fa 100644
--- a/test/e2e/dynamic_provisioning_test.go
+++ b/test/e2e/dynamic_provisioning_test.go
@@ -165,6 +165,9 @@ func (t *dynamicProvisioningTestSuite) defineTests(isMultiZone bool) {
 			Pods:                   pods,
 			StorageClassParameters: map[string]string{"skuName": "StandardSSD_LRS"},
 		}
+		if isAzureStackCloud {
+			test.StorageClassParameters = map[string]string{"skuName": "Standard_LRS"}
+		}
 		test.Run(cs, ns)
 	})
 
@@ -218,6 +221,9 @@ func (t *dynamicProvisioningTestSuite) defineTests(isMultiZone bool) {
 			Pods:                   pods,
 			StorageClassParameters: map[string]string{"skuName": "StandardSSD_LRS"},
 		}
+		if isAzureStackCloud {
+			test.StorageClassParameters = map[string]string{"skuName": "Standard_LRS"}
+		}
 		test.Run(cs, ns)
 	})
 
@@ -458,6 +464,9 @@ func (t *dynamicProvisioningTestSuite) defineTests(isMultiZone bool) {
 			Pods:                   pods,
 			StorageClassParameters: map[string]string{"skuName": "StandardSSD_LRS"},
 		}
+		if isAzureStackCloud {
+			test.StorageClassParameters = map[string]string{"skuName": "Standard_LRS"}
+		}
 		test.Run(cs, ns)
 	})
 
@@ -524,6 +533,9 @@ func (t *dynamicProvisioningTestSuite) defineTests(isMultiZone bool) {
 			PodWithSnapshot:        podWithSnapshot,
 			StorageClassParameters: map[string]string{"skuName": "StandardSSD_LRS"},
 		}
+		if isAzureStackCloud {
+			test.StorageClassParameters = map[string]string{"skuName": "Standard_LRS"}
+		}
 		test.Run(cs, snapshotrcs, ns)
 	})
 
@@ -561,6 +573,9 @@ func (t *dynamicProvisioningTestSuite) defineTests(isMultiZone bool) {
 			PodWithSnapshot:        podWithSnapshot,
 			StorageClassParameters: map[string]string{"skuName": "StandardSSD_LRS"},
 		}
+		if isAzureStackCloud {
+			test.StorageClassParameters = map[string]string{"skuName": "Standard_LRS"}
+		}
 		test.Run(cs, snapshotrcs, ns)
 	})
 
diff --git a/test/e2e/suite_test.go b/test/e2e/suite_test.go
index 74f510c44..73ed8e08f 100644
--- a/test/e2e/suite_test.go
+++ b/test/e2e/suite_test.go
@@ -134,12 +134,16 @@ var _ = ginkgo.AfterSuite(func() {
 		execTestCmd([]testCmd{checkPodsRestart})
 
 		os := "linux"
+		cloud := "azurepubliccloud"
 		if isWindowsCluster {
 			os = "windows"
 		}
+		if isAzureStackCloud {
+			cloud = "azurestackcloud"
+		}
 		createExampleDeployment := testCmd{
 			command:  "bash",
-			args:     []string{"hack/verify-examples.sh", os},
+			args:     []string{"hack/verify-examples.sh", os, cloud},
 			startLog: "create example deployments",
 			endLog:   "example deployments created",
 		}
diff --git a/test/e2e/testsuites/specs.go b/test/e2e/testsuites/specs.go
index 03d27aa2d..2de80c998 100644
--- a/test/e2e/testsuites/specs.go
+++ b/test/e2e/testsuites/specs.go
@@ -18,6 +18,8 @@ package testsuites
 
 import (
 	"fmt"
+	"os"
+	"strings"
 
 	"github.com/onsi/ginkgo"
 
@@ -71,8 +73,10 @@ const (
 )
 
 var (
-	SnapshotAPIGroup             = "snapshot.storage.k8s.io"
-	supportedStorageAccountTypes = []string{"Standard_LRS", "Premium_LRS", "StandardSSD_LRS"}
+	SnapshotAPIGroup                             = "snapshot.storage.k8s.io"
+	isAzureStackCloud                            = strings.EqualFold(os.Getenv("AZURE_CLOUD_NAME"), "AZURESTACKCLOUD")
+	azurePublicCloudSupportedStorageAccountTypes = []string{"Standard_LRS", "Premium_LRS", "StandardSSD_LRS"}
+	azureStackCloudSupportedStorageAccountTypes  = []string{"Standard_LRS", "Premium_LRS"}
 )
 
 type VolumeMountDetails struct {
@@ -111,6 +115,10 @@ func (pod *PodDetails) SetupWithDynamicVolumes(client clientset.Interface, names
 func (pod *PodDetails) SetupWithDynamicMultipleVolumes(client clientset.Interface, namespace *v1.Namespace, csiDriver driver.DynamicPVTestDriver) (*TestPod, []func()) {
 	tpod := NewTestPod(client, namespace, pod.Cmd, pod.IsWindows)
 	cleanupFuncs := make([]func(), 0)
+	supportedStorageAccountTypes := azurePublicCloudSupportedStorageAccountTypes
+	if isAzureStackCloud {
+		supportedStorageAccountTypes = azureStackCloudSupportedStorageAccountTypes
+	}
 	accountTypeCount := len(supportedStorageAccountTypes)
 	for n, v := range pod.Volumes {
 		storageClassParameters := map[string]string{"skuName": supportedStorageAccountTypes[n%accountTypeCount]}