fix: use safe type assertion and fix path construction in createFolderIfNotExists#3079
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andyzhangx The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Pull request overview
This PR hardens Driver.createFolderIfNotExists in the Azure File CSI driver by preventing potential panics from unsafe type assertions and fixing incremental directory path construction when input paths contain empty components.
Changes:
- Replaces an unsafe
fileClient.(*azureFileDataplaneClient)assertion with a safe comma-ok assertion and clearer error handling. - Avoids repeated type assertions by caching the concrete client in a local variable.
- Fixes incremental path building to avoid incorrect leading
/when empty path components are skipped.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…rIfNotExists - Replace unsafe type assertion fileClient.(*azureFileDataplaneClient) with comma-ok pattern to prevent potential panic - Separate nil error check from type assertion to avoid dereferencing nil fileClient when newAzureFileClient returns an error - Fix currentPath construction: use currentPath != "" instead of i > 0 to correctly handle skipped empty components in paths like "a//b" - Store type assertion result in local variable to avoid repeated assertions
9f74880 to
f4e4267
Compare
… detailed messages, add unit test
There was a problem hiding this comment.
Pull request overview
This PR hardens createFolderIfNotExists in the Azure File CSI driver by removing panic-prone type assertions and fixing an edge case in recursive directory path building.
Changes:
- Replace unsafe type assertions with a safe comma-ok assertion and clearer error handling when creating the Azure File dataplane client.
- Fix incremental path construction to avoid introducing a leading
/when empty path components are skipped. - Add a unit test case covering empty
accountName.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/azurefile/azurefile.go | Makes client creation/type handling safer and corrects incremental directory path concatenation. |
| pkg/azurefile/azurefile_test.go | Adds a new test case for an empty storage account name input. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
This PR hardens Driver.createFolderIfNotExists in the Azure File driver to avoid panics and to correctly build nested directory paths when creating folders in an Azure File Share.
Changes:
- Add explicit input validation for
accountNameandfileShareName, and switch to safe (comma-ok) type assertion for the dataplane client. - Improve error handling by separating client creation errors from client-type/client-nil checks and wrapping creation errors with
%w. - Fix incremental path construction to avoid incorrect leading slashes when empty path components are skipped; add targeted test cases for the new validations.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/azurefile/azurefile.go | Adds safer client handling and fixes incremental folder path construction in createFolderIfNotExists. |
| pkg/azurefile/azurefile_test.go | Adds tests for the new early validation errors (empty account name / empty share name). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
This PR hardens Driver.createFolderIfNotExists in the Azure File driver by preventing panics from unsafe type assertions and fixing incorrect nested-path construction when empty path components are present.
Changes:
- Add early argument validation for
accountName/fileShareNameand replace an unsafe type assertion with a safe comma-ok assertion plus clearer error handling. - Fix incremental directory path construction to avoid introducing a leading
/when empty components are skipped. - Extend the existing
TestCreateFolderIfNotExiststable with coverage for emptyaccountNameand emptyfileShareName.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/azurefile/azurefile.go | Makes client creation/type handling safer and corrects nested-path building logic in createFolderIfNotExists. |
| pkg/azurefile/azurefile_test.go | Adds test cases for newly introduced early-returns on missing accountName/fileShareName. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
This PR hardens createFolderIfNotExists in the Azure File driver by avoiding unsafe type assertions and fixing incremental path building when folder paths contain empty components.
Changes:
- Add explicit input validation for
accountNameandfileShareName, and switch to safe (comma-ok) type assertion for the dataplane client. - Improve error handling by separating client creation vs client-nil checks and wrapping client creation errors with
%w. - Fix incremental path construction by using
currentPath != ""rather than the original slice index when inserting separators.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/azurefile/azurefile.go | Safer client handling + corrected incremental path building for folder creation. |
| pkg/azurefile/azurefile_test.go | Adds test cases for the new early validation errors. |
Comments suppressed due to low confidence (1)
pkg/azurefile/azurefile.go:1499
folderNameis normalized (trimmed) later for component-wise creation (strings.Trim(folderName, "/")), but the fast-path existence check uses the rawfolderNamewhen creatingfullPathClient. For inputs with leading/trailing slashes (whichisValidFolderNameallows), this can cause the pre-check to query a different/invalid path and return a non-404 error, aborting instead of proceeding with recursive creation. Consider normalizingfolderNameonce (e.g.,normalized := strings.Trim(folderName, "/")), using that forNewDirectoryClient, logging, and optionally returning early whennormalized == "".
// Performance optimization: First check if the complete directory structure already exists
// This is the most common case and avoids unnecessary recursive checking
fullPathClient := shareClient.NewDirectoryClient(folderName)
_, err = fullPathClient.GetProperties(ctx, nil)
if err == nil {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@andyzhangx: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. DetailsInstructions 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-sigs/prow repository. I understand the commands that are listed here. |
|
/cherrypick release-1.35 |
|
@andyzhangx: new pull request created: #3080 DetailsIn response to this:
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-sigs/prow repository. |
What type of PR is this?
/kind bug
What this PR does / why we need it:
Fixes several issues in
createFolderIfNotExists:Unsafe type assertion —
fileClient.(*azureFileDataplaneClient)is used without comma-ok pattern, which can panic if the type doesn't match. Changed to safe assertion with proper error handling.Nil dereference risk — When
newAzureFileClientreturns an error,fileClientcould be nil. The original code combined the nil-error check with the type assertion in a singleifcondition. While Go's short-circuit evaluation prevents this in practice, separating the checks is clearer and safer.Incorrect path construction with skipped components — The loop used
i > 0to decide whether to prepend "/" to the path. When empty components are skipped (e.g., input"a//b"),istill increments, causing a leading "/" even whencurrentPathis empty. Changed to checkcurrentPath != ""instead.Repeated type assertions — The same
fileClient.(*azureFileDataplaneClient)assertion was done multiple times. Now stored in a local variabledc.Which issue(s) this PR fixes:
None
Special notes for your reviewer:
Minimal, targeted fix. No behavioral change for the happy path.