Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: check storage account type if parameter is missing #1391

Merged
merged 3 commits into from
Oct 7, 2023

Conversation

RomanBednar
Copy link
Contributor

@RomanBednar RomanBednar commented Aug 21, 2023

What type of PR is this?
/kind bug

What this PR does / why we need it:

While using StorageClass with samba protocol without explicitly specifying strorage account type users get a misleading message and volume creation fails:

Status=400 Code="InvalidHeaderValue" Message="The value for one of the HTTP headers is not in the correct format.

This happens in a scenario when creating a volume with size < 100Gi and storage account name (premium type) was provided as a parameter in StorageClass while skuName was omitted. Premium requirement is that a fileshare must have at least 100Gi in size.

Driver can lookup storage account type via storage account client and use that instead of relying on theskuName parameter that users otherwise need to set manually.

Which issue(s) this PR fixes:

Fixes #1390

Requirements:

Special notes for your reviewer:

Release note:

`skuName` storage class parameter is now optional when using premium storage account with volume sizes below 100Gi

@k8s-ci-robot k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Aug 21, 2023
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 21, 2023
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 23, 2023
@RomanBednar RomanBednar changed the title improve error message when smb is requested without account type check storage account type if parameter is missing Aug 23, 2023
@RomanBednar RomanBednar force-pushed the smb-errmsg branch 2 times, most recently from 682bf44 to f51d55b Compare August 23, 2023 13:57
@@ -312,6 +312,16 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest)
return nil, status.Errorf(codes.InvalidArgument, "fsType(%s) is not supported with protocol(%s)", fsType, protocol)
}

if account != "" && sku == "" {
Copy link
Member

@andyzhangx andyzhangx Aug 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also add fileShareSize < minimumPremiumShareSize check, that could save one StorageAccountClient.GetProperties call: sku is only needed when fileShareSize < minimumPremiumShareSize

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added, along with one more unit test to verify StorageAccountClient.GetProperties is not called in this case.

@andyzhangx andyzhangx changed the title check storage account type if parameter is missing fix: check storage account type if parameter is missing Aug 24, 2023
@RomanBednar
Copy link
Contributor Author

/hold

I get an error while obtaining storage account properties, need to investigate.

E0824 12:33:51.882227 1 utils.go:81] GRPC error: rpc error: code = Internal desc = failed to get properties on storage account account(rbednarplrs) rg(), error: &{false 400 0001-01-01 00:00:00 +0000 UTC {"error":{"code":"InvalidApiVersionParameter","message":"The api-version '2021-02-01' is invalid. The supported versions are '2023-07-01,2023-07-01-preview,2023-03-01-preview,2022-12-01,2022-11-01-preview,2022-09-01,2022-06-01,2022-05-01,2022-03-01-preview,2022-01-01,2021-04-01,2021-01-01,2020-10-01,2020-09-01,2020-08-01,2020-07-01,2020-06-01,2020-05-01,2020-01-01,2019-11-01,2019-10-01,2019-09-01,2019-08-01,2019-07-01,2019-06-01,2019-05-10,2019-05-01,2019-03-01,2018-11-01,2018-09-01,2018-08-01,2018-07-01,2018-06-01,2018-05-01,2018-02-01,2018-01-01,2017-12-01,2017-08-01,2017-06-01,2017-05-10,2017-05-01,2017-03-01,2016-09-01,2016-07-01,2016-06-01,2016-02-01,2015-11-01,2015-01-01,2014-04-01-preview,2014-04-01,2014-01-01,2013-03-01,2014-02-26,2014-04'."}}}

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 24, 2023
@RomanBednar
Copy link
Contributor Author

The only working combination I could find so far was when I set network APIVersion to 2023-07-01 and set resourceGroup parameter manually:

allowVolumeExpansion: true
apiVersion: storage.k8s.io/v1
kind: StorageClass
metadata:
  name: azurefile-premium
parameters:
  protocol: smb
  storageAccount: premiumacct
  resourceGroup: rbednar-mycluster-01-4c6xn-rg
provisioner: file.csi.azure.com
reclaimPolicy: Delete
volumeBindingMode: Immediate

@andyzhangx Any idea how this should be handled? Seems like GetProperties call requires resource group name, but if that's the case this patch would not help much.

@RomanBednar RomanBednar force-pushed the smb-errmsg branch 2 times, most recently from 1b9131b to d629463 Compare October 3, 2023 13:36
@RomanBednar
Copy link
Contributor Author

I've found a similar issue with CLI where the workaround was to add a resource group name, not sure if it might be related or if the same fix is planned for Azure SDK.

Anyway, GetProperties currently requires the resource group name, so what we could do is to load the name from cloud config earlier and add a check (resourceGroup != "") so we don't call GetProperties if it's not found.

Testing it with the same storage class I've posted in previous comment (but without resourceGroup paramenter) works fine for me.

@RomanBednar
Copy link
Contributor Author

/unhold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 3, 2023
@andyzhangx
Copy link
Member

/retest

1 similar comment
@andyzhangx
Copy link
Member

/retest

Copy link
Member

@andyzhangx andyzhangx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/retest

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 6, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andyzhangx, RomanBednar

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 6, 2023
@andyzhangx
Copy link
Member

/retest

@k8s-ci-robot k8s-ci-robot merged commit b20bd12 into kubernetes-sigs:master Oct 7, 2023
10 checks passed
@andyzhangx
Copy link
Member

/cherrypick release-1.29

@k8s-infra-cherrypick-robot

@andyzhangx: failed to push cherry-picked changes in GitHub: pushToNamedFork is not implemented in the v2 client

In response to this:

/cherrypick release-1.29

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@andyzhangx
Copy link
Member

/cherrypick release-1.29

@k8s-infra-cherrypick-robot

@andyzhangx: failed to push cherry-picked changes in GitHub: pushToNamedFork is not implemented in the v2 client

In response to this:

/cherrypick release-1.29

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@andyzhangx
Copy link
Member

/cherrypick release-1.29

@k8s-infra-cherrypick-robot

@andyzhangx: failed to push cherry-picked changes in GitHub: pushToNamedFork is not implemented in the v2 client

In response to this:

/cherrypick release-1.29

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@andyzhangx
Copy link
Member

/cherrypick release-1.29

@k8s-infra-cherrypick-robot

@andyzhangx: new pull request created: #1479

In response to this:

/cherrypick release-1.29

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Volume provisioning fails with "InvalidHeaderValue" when using "smb" protocol and no skuName
4 participants