You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(aks-diagnostic): move SAS token off argv to an environment variable - #9883
The SAS token used to authenticate blob uploads was a required positional
CLI argument, exposing it to any local process able to read this
process's command line (e.g. via /proc/<pid>/cmdline), rather than only
to whoever invoked the script.
Add support for passing it via the AKS_DIAGNOSTIC_SAS_TOKEN environment
variable (preferred, takes precedence when both are set), which does not
appear in argv. The positional CLI argument is kept as a deprecated
fallback for backward compatibility with any existing external callers -
this script is baked onto the VHD standalone (no in-repo caller invokes
it; vhdbuilder/packer/packer_source.sh only copies the file), so there is
no way to enumerate or update every caller in this change. Using it now
prints a warning to stderr pointing at the env var instead.
To make the new optional sas_token positional unambiguous against the
pre-existing optional file_path positional, file_path is now the
--file-path flag rather than a second positional argument (two optional
trailing positionals cannot otherwise be told apart by argparse).
Also removed the unused handleArgs() function: dead code (zero call
sites, confirmed via repo-wide search) that carried the exact same
argv-based SAS token exposure as a legacy positional-args fallback.
No change to the actual upload behavior/HTTP logic.
Verified locally:
- python3 -m py_compile and pyflakes both pass with no warnings/errors.
- Manually exercised all argument-resolution paths: missing token (both
env var and positional absent) now errors with a clear message;
env-var-only invocation succeeds and the token does not appear in the
process's argv (checked via ps/cmdline); positional-only invocation
still works and emits the deprecation warning; when both are set, the
env var wins and no warning is printed. --help output reviewed for
correctness.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 350c2577-7031-4d0d-adc1-e99d053fc981
This PR adds an environment-variable option for diagnostic upload credentials, reducing command-line exposure of SAS tokens in the standalone VHD script.
Changes:
Prefer AKS_DIAGNOSTIC_SAS_TOKEN, retaining a deprecated positional token fallback.
Replace the positional custom file path with --file-path and update usage documentation.
Remove the unused handleArgs() function without changing upload HTTP logic.
The previous commit converted file_path from a positional argument to
a --file-path flag to resolve an argparse ambiguity with the new
optional sas_token positional. That broke the pre-existing 4-argument
invocation style (container url sas_token file_path), which is a real
regression for any external caller still using it.
Fix properly: capture the trailing sas_token/file_path positionals
together via nargs='*' and resolve them by count after parsing, so
every pre-existing invocation shape - (), (token,), (token, file_path)
- behaves exactly as before. --file-path remains available as the
preferred way to set a custom path when the token itself comes from
the AKS_DIAGNOSTIC_SAS_TOKEN env var (no token positional at all).
Verified locally: re-ran all argument-resolution paths, specifically
including the previously-broken old 4-positional-argument form
(container url token file_path) with a real file on disk, confirming
it resolves file_path correctly and reaches the upload call. Also
re-verified the new env-var + --file-path combination, the "too many
arguments" error path, and that the SAS token still does not appear in
the process's argv when sourced from the environment variable.
python3 -m py_compile and pyflakes both pass with no warnings.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 350c2577-7031-4d0d-adc1-e99d053fc981
Run both scripts with identical inputs and compare upload requests, file reads, exits, retries and console output. Cover known RP calls and 200 Hypothesis examples, allowing only the existing deprecation warning. Run the compatibility suite in CI on Python 3.9 and 3.12.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 350c2577-7031-4d0d-adc1-e99d053fc981
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
An explicitly empty file path now falls back to the default archive instead of failing validation, breaking backward compatibility and risking an unintended upload.
No open issues 🎉
1 previously missed issue
In code that hasn't changed since last review
Empty file path silently uploads the default archive
🟡 Medium Risk — 🔄 Backward Compatibility: An explicitly empty file path (for example, a quoted unset path variable in an existing caller) is now replaced with the default archive. Previously, validation rejected that empty path; where the default logs.zip exists, the same invocation now uploads it instead. Check for None rather than truthiness so an explicitly invalid path still fails instead of silently uploading a different file.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
Positional-token invocations omit the promised deprecation warning, leaving existing callers without notice to move credentials from argv to AKS_DIAGNOSTIC_SAS_TOKEN.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it
AKS_DIAGNOSTIC_SAS_TOKENas the preferred way to provide the diagnostic SAS token, avoiding exposure in the process argv.file_path, including the existing default behavior.--file-pathsupports env-only custom paths and explicitly overrides positionalfile_path.handleArgsis not removed.Which issue(s) this PR fixes
Validation