Personal/degoswami/newflags #29154

DevanshG1 · 2026-02-09T11:17:12Z

Description

Mandatory Checklist

Please choose the target release of Azure PowerShell. (⚠️Target release is a different concept from API readiness. Please click below links for details.)
- General release
- Public preview
- Private preview
- Engineering build
- No need for a release
Check this box to confirm: I have read the Submitting Changes section of CONTRIBUTING.md and reviewed the following information:

SHOULD update ChangeLog.md file(s) appropriately
- Update src/{{SERVICE}}/{{SERVICE}}/ChangeLog.md.
  - A snippet outlining the change(s) made in the PR should be written under the ## Upcoming Release header in the past tense.
- Should not change ChangeLog.md if no new release is required, such as fixing test case only.
SHOULD regenerate markdown help files if there is cmdlet API change. Instruction
SHOULD have proper test coverage for changes in pull request.
SHOULD NOT adjust version of module manually in pull request

azure-client-tools-bot-prd · 2026-02-09T11:17:16Z

Thanks for your contribution! The pull request validation has started. Please revisit this comment for updated status.

Copilot

Pull request overview

This PR extends the Az.Sftp module to make Connect-AzSftp more configurable and diagnosable by adding new connection options and enriching debug/verbose output across SFTP cmdlets.

Changes:

Added -BufferSize (int) to control the sftp -B transfer buffer size.
Added -StorageAccountEndpoint to allow overriding the storage endpoint suffix used to build the SFTP hostname.
Expanded debug/verbose logging in Connect-AzSftp and New-AzSftpCertificate, and updated help + changelog + scenario tests accordingly.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
src/Sftp/Sftp/help/Connect-AzSftp.md	Documents new `-BufferSize` and `-StorageAccountEndpoint` parameters in syntax and parameter sections.
src/Sftp/Sftp/SftpCommands/ConnectAzSftpCommand.cs	Adds the two new parameters, passes buffer size into session, supports custom endpoint suffix, and adds detailed logging/cleanup messaging.
src/Sftp/Sftp/Models/SFTPSession.cs	Adds `BufferSizeBytes` and emits `-B` in the generated `sftp` command args (conditionally).
src/Sftp/Sftp/SftpCommands/NewAzSftpCertificateCommand.cs	Adds more structured debug/verbose logging and clearer warnings/errors around certificate generation.
src/Sftp/Sftp/CHANGELOG.md	Records the two new parameters and their intent under “Upcoming Release”.
src/Sftp/Sftp.Test/ScenarioTests/ConnectAzSftpTests.ps1	Adds new scenario test functions intended to exercise the new parameters.

Copilot · 2026-02-09T11:27:19Z

src/Sftp/Sftp.Test/ScenarioTests/ConnectAzSftpTests.ps1

+<#
+.SYNOPSIS
+Test Connect-AzSftp with BufferSize parameter
+#>
+function Test-ConnectAzSftpWithBufferSize
+{
+    $storageAccountName = Get-StorageAccountName
+    $resourceGroupName = Get-ResourceGroupName
+
+    try {
+        # Skip test in playback mode for now
+        if (IsPlayback) {
+            return
+        }
+
+        # Create test storage account
+        $storageAccount = New-TestStorageAccount -ResourceGroupName $resourceGroupName -StorageAccountName $storageAccountName
+
+        # Test connection with custom buffer size (will fail in test environment but validates parameter parsing)
+        try {
+            $result = Connect-AzSftp -StorageAccount $storageAccountName -BufferSize 524288 -SftpArg "-o", "ConnectTimeout=1"
+        }
+        catch {
+            # Expected to fail in test environment - this is acceptable
+            Write-Host "Connection failed as expected in test environment: $($_.Exception.Message)"
+        }
+
+        # Test with default buffer size (256*1024 = 262144)
+        try {
+            $result = Connect-AzSftp -StorageAccount $storageAccountName -SftpArg "-o", "ConnectTimeout=1"
+        }
+        catch {
+            Write-Host "Connection failed as expected in test environment: $($_.Exception.Message)"
+        }
+    }
+    finally {
+        # Cleanup
+        Remove-TestStorageAccount -ResourceGroupName $resourceGroupName -StorageAccountName $storageAccountName
+        Remove-AzResourceGroup -Name $resourceGroupName -Force -ErrorAction SilentlyContinue
+    }
+}
+
+<#
+.SYNOPSIS
+Test Connect-AzSftp with StorageAccountEndpoint parameter
+#>
+function Test-ConnectAzSftpWithStorageAccountEndpoint
+{
+    $storageAccountName = Get-StorageAccountName
+    $resourceGroupName = Get-ResourceGroupName
+    $customEndpoint = "blob.core.custom.endpoint.net"
+
+    try {
+        # Skip test in playback mode for now
+        if (IsPlayback) {
+            return
+        }
+
+        # Create test storage account
+        $storageAccount = New-TestStorageAccount -ResourceGroupName $resourceGroupName -StorageAccountName $storageAccountName
+
+        # Test connection with custom storage account endpoint (will fail in test environment but validates parameter parsing)
+        try {
+            $result = Connect-AzSftp -StorageAccount $storageAccountName -StorageAccountEndpoint $customEndpoint -SftpArg "-o", "ConnectTimeout=1"
+        }
+        catch {
+            # Expected to fail in test environment - this is acceptable
+            Write-Host "Connection failed as expected in test environment: $($_.Exception.Message)"
+        }
+    }
+    finally {
+        # Cleanup
+        Remove-TestStorageAccount -ResourceGroupName $resourceGroupName -StorageAccountName $storageAccountName
+        Remove-AzResourceGroup -Name $resourceGroupName -Force -ErrorAction SilentlyContinue
+    }
+}


The added PowerShell scenario test functions don’t provide meaningful coverage in CI: the ConnectAzSftpTestsRunner has all [Fact] tests skipped, and these new functions also early-return in playback mode and only swallow connection failures without asserting on behavior (e.g., that -B is included in the generated sftp command, or that the custom endpoint affects the hostname). Add deterministic unit tests in the existing non-skipped C# test suite (e.g., SftpUtilsTests/SFTPSession tests) to validate -B argument emission and host construction, or otherwise ensure these tests are actually executed.

I like work.
Life's Always got its own Earth Shadow.
Keep is safe.

Copilot · 2026-02-09T11:27:19Z

src/Sftp/Sftp/Models/SFTPSession.cs

+            if (BufferSizeBytes != 256 * 1024)
+            {
+                args.AddRange(new[] { "-B", BufferSizeBytes.ToString() });
+            }


BufferSizeBytes is treated as if 256 KB is the default, but BuildArgs() only adds the -B flag when the value differs from 256*1024. As a result, when users do not pass -BufferSize (or pass 262144), the cmdlet won’t pass -B at all and the effective buffer size will fall back to the underlying sftp client’s default—contradicting the help/CHANGELOG that state a default of 262144. Decide on one behavior and make code+docs consistent (e.g., always pass -B with the default, or make buffer size optional/nullable and only add -B when the parameter was explicitly provided).

Suggested change

if (BufferSizeBytes != 256 * 1024)

{

args.AddRange(new[] { "-B", BufferSizeBytes.ToString() });

}

args.AddRange(new[] { "-B", BufferSizeBytes.ToString() });

Copilot · 2026-02-09T11:27:20Z

src/Sftp/Sftp/SftpCommands/ConnectAzSftpCommand.cs

+                    WriteDebug($"[SFTP] Waiting for SFTP session to complete before cleanup...");
                    process.WaitForExit();
                    // Wait up to 5 minutes (300,000 ms) for the process to exit
                    bool exited = process.WaitForExit(300000);
                    if (!exited)


The process wait logic makes the 5-minute timeout ineffective: process.WaitForExit() blocks indefinitely before the timed WaitForExit(300000) is called. This can hang the cmdlet and prevent credential cleanup. Remove the unconditional wait and rely on the timed wait (or apply a single wait strategy) so the timeout actually works.

src/Sftp/Sftp/SftpCommands/ConnectAzSftpCommand.cs

amandalanise9-tech · 2026-02-09T21:59:26Z

That's a Healthy Conversation,
on This Subject. In My Opinion.
Still Dangerous.
Better one day. :\

DevanshG1 · 2026-02-10T03:28:49Z

@isra-fel Please review this PR

isra-fel · 2026-02-10T04:38:22Z

/azp run

azure-pipelines · 2026-02-10T04:38:38Z

Azure Pipelines successfully started running 3 pipeline(s).

github-actions · 2026-02-10T04:48:50Z

‼️ DO NOT MERGE THIS PR ‼️
This PR was labeled "Do Not Merge" because it contains code change that cannot be merged. Please contact the reviewer for more information.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Copilot · 2026-02-10T05:00:14Z

src/Sftp/Sftp/SftpCommands/ConnectAzSftpCommand.cs

+                    throw new PSArgumentException(
+                        "StorageAccountEndpoint must be a DNS suffix such as 'blob.core.windows.net' and must not contain a scheme or path.");
+                }
+
+                if (string.IsNullOrEmpty(normalizedSuffix))
+                {
+                    throw new PSArgumentException("StorageAccountEndpoint cannot be empty.");


The PSArgumentException thrown for invalid StorageAccountEndpoint doesn’t specify the parameter name, which makes PowerShell error output less actionable (it won’t highlight which argument is invalid). Consider using the overload that includes paramName (or an AzPSArgumentException with nameof(StorageAccountEndpoint)) for clearer diagnostics.

Suggested change

throw new PSArgumentException(

"StorageAccountEndpoint must be a DNS suffix such as 'blob.core.windows.net' and must not contain a scheme or path.");

}

if (string.IsNullOrEmpty(normalizedSuffix))

{

throw new PSArgumentException("StorageAccountEndpoint cannot be empty.");

throw new AzPSArgumentException(

"StorageAccountEndpoint must be a DNS suffix such as 'blob.core.windows.net' and must not contain a scheme or path.",

nameof(StorageAccountEndpoint));

}

if (string.IsNullOrEmpty(normalizedSuffix))

{

throw new AzPSArgumentException("StorageAccountEndpoint cannot be empty.", nameof(StorageAccountEndpoint));

Copilot · 2026-02-10T05:00:15Z

src/Sftp/Sftp/help/Connect-AzSftp.md

+### -BufferSize
+Buffer size in bytes for SFTP file transfers. Default: 262144 (256 KB).
+


The help text claims a default BufferSize of 262144 (256 KB), but the current implementation only adds the sftp -B flag when a non-default value is set. Unless -B 262144 is always emitted, the effective default will be whatever the sftp client uses. Please align the documentation with actual behavior (or update the code so the stated default is enforced).

Suggest naming it -BufferSizeInBytes for usability

Copilot · 2026-02-10T05:00:15Z

src/Sftp/Sftp/CHANGELOG.md

 ## Upcoming Release
+* Added `BufferSize` parameter to `Connect-AzSftp` cmdlet
+    - Allows users to specify buffer size in bytes for SFTP file transfers using the sftp -B flag
+    - Default value is 262144 (256 KB)


CHANGELOG notes a BufferSize default of 262144 (256 KB), but the current sftp -B argument is only emitted when a non-default value is used. Either ensure the default is actually applied in the command invocation, or adjust the changelog entry to avoid stating an incorrect default behavior.

Suggested change

- Default value is 262144 (256 KB)

- When specified, a value of 262144 (256 KB) is recommended for optimal performance

isra-fel · 2026-02-10T05:08:38Z

/azp run

azure-pipelines · 2026-02-10T05:08:55Z

Azure Pipelines successfully started running 3 pipeline(s).

DevanshG1 · 2026-02-10T05:54:28Z

@isra-fel please run the checks

isra-fel

Please see the inline comments

isra-fel · 2026-02-10T06:17:37Z

src/Sftp/Sftp/help/Connect-AzSftp.md

+### -BufferSize
+Buffer size in bytes for SFTP file transfers. Default: 262144 (256 KB).
+


Suggest naming it -BufferSizeInBytes for usability

src/Sftp/Sftp/Models/SFTPSession.cs

isra-fel · 2026-02-10T06:27:07Z

/azp run

azure-pipelines · 2026-02-10T06:27:25Z

Azure Pipelines successfully started running 3 pipeline(s).

github-actions · 2026-02-10T06:27:31Z

This PR was labeled "needs-revision" because it has unresolved review comments or CI failures.
Please resolve all open review comments and make sure all CI checks are green. Refer to our guide to troubleshoot common CI failures.

Copilot

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.

Copilot · 2026-02-10T07:01:46Z

src/Sftp/Sftp.Test/ScenarioTests/SftpUtilsTests.cs

+            // Arrange - default buffer size is 256 * 1024 = 262144
+            var session = new SFTPSession(
+                storageAccount: "testaccount",
+                username: "testuser",
+                host: "testaccount.blob.core.windows.net",
+                port: 22,
+                publicKeyFile: null,
+                privateKeyFile: null,
+                certFile: null,
+                sftpArgs: null,
+                sshClientFolder: null,
+                sshProxyFolder: null,
+                credentialsFolder: null,
+                yesWithoutPrompt: false,
+                bufferSizeBytes: 256 * 1024  // Default value
+            );


These tests hardcode the default buffer size as 256 * 1024 / 262144. Since the production default is defined in SftpConstants.DefaultBufferSizeBytes, using the constant here would keep tests aligned if the default ever changes and avoid duplicating magic numbers.

Copilot · 2026-02-10T07:01:47Z

src/Sftp/Sftp/SftpCommands/ConnectAzSftpCommand.cs

                        PublicKeyFile, PrivateKeyFile, credentialsFolder, SshClientFolder);
                    PublicKeyFile = pubKey;
                    PrivateKeyFile = privKey;



FileUtils.CheckOrCreatePublicPrivateFiles returns a deleteKeys flag (delKeys here) indicating whether key material was generated and should be cleaned up, but the value is never applied to the cmdlet-level deleteKeys. This can leave newly generated keys on disk unexpectedly (and they contain sensitive material). Propagate delKeys into deleteKeys (e.g., deleteKeys |= delKeys) so cleanup behavior matches what FileUtils reports.

Suggested change

deleteKeys |= delKeys;

Copilot · 2026-02-10T07:01:47Z

src/Sftp/Sftp/SftpCommands/ConnectAzSftpCommand.cs

+            // Use custom endpoint suffix if provided
+            if (!string.IsNullOrWhiteSpace(StorageAccountEndpoint))
+            {
+                var normalizedSuffix = StorageAccountEndpoint.Trim();
+
+                // Remove any leading dots so that values like ".blob.core.windows.net" are accepted
+                normalizedSuffix = normalizedSuffix.TrimStart('.');
+
+                // Reject values that look like full URLs or contain paths, since we only expect a DNS suffix
+                if (normalizedSuffix.IndexOf("://", StringComparison.Ordinal) >= 0 ||
+                    normalizedSuffix.IndexOf("/", StringComparison.Ordinal) >= 0)
+                {
+                    throw new PSArgumentException(
+                        "StorageAccountEndpoint must be a DNS suffix such as 'blob.core.windows.net' and must not contain a scheme or path.");
+                }
+
+                if (string.IsNullOrEmpty(normalizedSuffix))
+                {
+                    throw new PSArgumentException("StorageAccountEndpoint cannot be empty.");
+                }
+
+                return normalizedSuffix;
+            }


StorageAccountEndpoint introduces new parsing/validation behavior (trimming leading dots and rejecting schemes/paths), but there are no unit tests covering valid/invalid inputs or verifying the computed hostname uses the custom suffix. Add deterministic C# tests for GetStorageEndpointSuffix behavior (or refactor it into a testable helper) so regressions are caught without relying on skipped E2E scripts.

Copilot · 2026-02-10T07:01:47Z

src/Sftp/Sftp.Test/ScenarioTests/CmdletParameterCompatibilityTests.cs

+            // Test that BufferSizeInBytes has correct default value (256 * 1024 = 262144)
+            var connectCommand = new ConnectAzSftpCommand();
+
+            Assert.Equal(256 * 1024, connectCommand.BufferSizeInBytes);
+            Assert.Equal(262144, connectCommand.BufferSizeInBytes);


The default-value test duplicates the production constant as 256 * 1024 and 262144. Prefer asserting against SftpConstants.DefaultBufferSizeBytes so tests remain correct if the default is ever adjusted, and you only have one source of truth.

Suggested change

// Test that BufferSizeInBytes has correct default value (256 * 1024 = 262144)

var connectCommand = new ConnectAzSftpCommand();

Assert.Equal(256 * 1024, connectCommand.BufferSizeInBytes);

Assert.Equal(262144, connectCommand.BufferSizeInBytes);

// Test that BufferSizeInBytes has correct default value

var connectCommand = new ConnectAzSftpCommand();

Assert.Equal(SftpConstants.DefaultBufferSizeBytes, connectCommand.BufferSizeInBytes);

Copilot · 2026-02-10T07:01:48Z

src/Sftp/Sftp/SftpCommands/ConnectAzSftpCommand.cs

+                return normalizedSuffix;
+            }
+
            string cloudName = DefaultContext?.Environment?.Name?.ToLower() ?? "azurecloud";


DefaultContext?.Environment?.Name?.ToLower() uses current-culture casing rules. For environment name comparisons, use ToLowerInvariant() to avoid culture-specific edge cases (e.g., Turkish-I).

Suggested change

string cloudName = DefaultContext?.Environment?.Name?.ToLower() ?? "azurecloud";

string cloudName = DefaultContext?.Environment?.Name?.ToLowerInvariant() ?? "azurecloud";

Copilot · 2026-02-10T07:01:48Z

src/Sftp/Sftp/SftpCommands/ConnectAzSftpCommand.cs

+                // Reject values that look like full URLs or contain paths, since we only expect a DNS suffix
+                if (normalizedSuffix.IndexOf("://", StringComparison.Ordinal) >= 0 ||
+                    normalizedSuffix.IndexOf("/", StringComparison.Ordinal) >= 0)
+                {
+                    throw new PSArgumentException(
+                        "StorageAccountEndpoint must be a DNS suffix such as 'blob.core.windows.net' and must not contain a scheme or path.");
+                }


StorageAccountEndpoint is treated as a DNS suffix, but the current validation allows characters like :. In OpenSSH sftp, a destination of the form host:dir is parsed as an initial remote path, so allowing : in the suffix can change semantics unexpectedly. Consider rejecting : (and other non-DNS characters/whitespace) so the parameter is constrained to a true hostname suffix.

isra-fel · 2026-02-10T07:08:14Z

/azp run

azure-pipelines · 2026-02-10T07:08:31Z

Azure Pipelines successfully started running 3 pipeline(s).

isra-fel

LGTM!

Devansh Goswami and others added 4 commits January 29, 2026 01:20

Adding a Bytes and custom storage account endpoint flag

e995bde

improving logging and resolving comments

69571a4

Renaming BufferSizeBytes to BufferSize to fix the errors

7a7436f

Merge branch 'Azure:main' into main

2249f33

Copilot AI review requested due to automatic review settings February 9, 2026 11:17

Copilot started reviewing on behalf of DevanshG1 February 9, 2026 11:19 View session

Copilot AI reviewed Feb 9, 2026

View reviewed changes

Merge branch 'Azure:main' into personal/degoswami/newflags

60ecbfc

DevanshG1 added the Do Not Merge 🚫 label Feb 10, 2026

Update src/Sftp/Sftp/SftpCommands/ConnectAzSftpCommand.cs

91842de

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings February 10, 2026 04:50

Copilot started reviewing on behalf of DevanshG1 February 10, 2026 04:52 View session

Copilot AI reviewed Feb 10, 2026

View reviewed changes

Adding new test cases for the flags

0573b48

DevanshG1 added ready to review and removed Do Not Merge 🚫 labels Feb 10, 2026

isra-fel requested changes Feb 10, 2026

View reviewed changes

isra-fel added needs-revision and removed ready to review labels Feb 10, 2026

isra-fel assigned DevanshG1 Feb 10, 2026

Resolving comments

e9bb810

Copilot AI review requested due to automatic review settings February 10, 2026 06:51

Copilot started reviewing on behalf of DevanshG1 February 10, 2026 06:53 View session

DevanshG1 requested a review from isra-fel February 10, 2026 07:00

Copilot AI reviewed Feb 10, 2026

View reviewed changes

isra-fel approved these changes Feb 11, 2026

View reviewed changes

isra-fel merged commit f8f5ee6 into Azure:main Feb 11, 2026
16 of 18 checks passed

		### -BufferSize
		Buffer size in bytes for SFTP file transfers. Default: 262144 (256 KB).

	- Default value is 262144 (256 KB)
	- When specified, a value of 262144 (256 KB) is recommended for optimal performance

	string cloudName = DefaultContext?.Environment?.Name?.ToLower() ?? "azurecloud";
	string cloudName = DefaultContext?.Environment?.Name?.ToLowerInvariant() ?? "azurecloud";

Personal/degoswami/newflags #29154

Personal/degoswami/newflags #29154

Uh oh!

Conversation

DevanshG1 commented Feb 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Mandatory Checklist

Uh oh!

azure-client-tools-bot-prd bot commented Feb 9, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

amandalanise9-tech Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

amandalanise9-tech Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

amandalanise9-tech commented Feb 9, 2026

Uh oh!

DevanshG1 commented Feb 10, 2026

Uh oh!

isra-fel commented Feb 10, 2026

Uh oh!

azure-pipelines bot commented Feb 10, 2026

Uh oh!

github-actions bot commented Feb 10, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

isra-fel Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

isra-fel commented Feb 10, 2026

Uh oh!

azure-pipelines bot commented Feb 10, 2026

Uh oh!

DevanshG1 commented Feb 10, 2026

Uh oh!

isra-fel left a comment

Choose a reason for hiding this comment

Uh oh!

isra-fel Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

isra-fel commented Feb 10, 2026

Uh oh!

azure-pipelines bot commented Feb 10, 2026

Uh oh!

github-actions bot commented Feb 10, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

DevanshG1 commented Feb 9, 2026 •

edited

Loading