Skip to content

test: Allow Storm to accept any env vars, not just proxy vars #132

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.

Sign up for GitHub

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

Merged
merged 29 commits into from
Oct 27, 2025
Merged

test: Allow Storm to accept any env vars, not just proxy vars #132

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
e049086
fix proxy in testing
ayaegashi Sep 11, 2025
df6b131
no noproxy
ayaegashi Sep 11, 2025
75411ad
revert target config
ayaegashi Sep 11, 2025
d10c8f8
remove noproxy
ayaegashi Sep 12, 2025
7e2c325
try as array of strings
ayaegashi Sep 12, 2025
0a11b46
test only misc
ayaegashi Sep 12, 2025
c55cc0a
revert target configs
ayaegashi Sep 12, 2025
41fa134
change variable name to env_var_args
ayaegashi Sep 12, 2025
cf07c74
rename to env-var-args in e2e-test-run
ayaegashi Sep 12, 2025
73143fe
go back to https proxy
ayaegashi Sep 18, 2025
12c5193
rename ot PROXYARG
ayaegashi Sep 18, 2025
1de9f14
rename contd
ayaegashi Sep 18, 2025
f1ce724
add test env vars
ayaegashi Sep 18, 2025
6fe7aa9
add log
ayaegashi Sep 18, 2025
e410774
add --env for each var
ayaegashi Sep 18, 2025
5c92b73
test again
ayaegashi Sep 18, 2025
910492f
fixed typo
ayaegashi Sep 18, 2025
faf8e97
revert test env vars
ayaegashi Sep 18, 2025
c644cdb
update help string
ayaegashi Sep 18, 2025
e66110d
nit
ayaegashi Sep 18, 2025
e3b8a44
use strings cut
ayaegashi Sep 18, 2025
6b28ac2
remove unnecessary log
ayaegashi Sep 18, 2025
1338975
use single quotes
ayaegashi Sep 19, 2025
34282b8
preserve env unnecessary
ayaegashi Sep 19, 2025
b5db98f
only run misc for validation
ayaegashi Sep 19, 2025
36eb1d3
revert target config
ayaegashi Sep 19, 2025
f0e8bbe
remove test env var
ayaegashi Sep 19, 2025
0fb3f2d
remove code declaring vars before sudo
ayaegashi Sep 19, 2025
449617b
Merge remote-tracking branch 'origin/main' into ayaegashi/proxy-update
ayaegashi Oct 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions .pipelines/templates/stages/testing_baremetal/baremetal-testing.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,8 @@ stages:
# Alternate HTTP(S) proxy which can be used with AEP02 is "http://192.168.229.51:3128"
- name: BAREMETAL_HTTPS_PROXY
value: "http://172.16.1.10:3128"
- name: BAREMETAL_ENV_VARS
Copy link
Contributor

Choose a reason for hiding this comment

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

So, these BAREMETAL_ENV_VARS will include other var-s, and HTTPS_PROXY will only be one of them? Just curious why this change is needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, that's correct. This PR is in response to this comment on my previous PR a couple weeks back: #86 (comment) I didn't see this comment until recently haha

Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest that everything in the pipeline stays mostly the same, otherwise there are many edge cases not being considered, and only when we get to invoking the storm binary we transform it into the proxy, ie

storm-bin ... --env HTTPS_PROXY=${{ parameters.HTTPS_PROXY }} ...

value: "HTTPS_PROXY=${{ variables.BAREMETAL_HTTPS_PROXY }}"

# Sourced from the matrix
- name: TRIDENT_CONFIGURATION_NAME
Expand Down Expand Up @@ -226,11 +228,11 @@ stages:
SERVICE_OVERRIDE=""
if [ "${{ parameters.runtimeEnv }}" == "host" ]; then
SERVICE_OVERRIDE="[Service]
Environment=\"HTTPS_PROXY=${{ variables.BAREMETAL_HTTPS_PROXY }}\""
Environment=\"${{ variables.BAREMETAL_ENV_VARS }}\""
else
# When running containerized Trident, we must directly embed the proxy in the docker command
SERVICE_OVERRIDE="[Service]
Environment=\"HTTPS_PROXY=${{ variables.BAREMETAL_HTTPS_PROXY }}\"
Environment=\"${{ variables.BAREMETAL_ENV_VARS }}\"
ExecStart=
ExecStart=docker run \\
--env HTTPS_PROXY \\
Expand Down Expand Up @@ -391,7 +393,7 @@ stages:
netlistenPort: ${{ variables.NETLAUNCH_PORT }}
runtimeEnv: ${{ parameters.runtimeEnv }}
netlistenConfigFile: $(TRIDENT_SOURCE_DIR)/baremetal-netlisten.yaml
httpsProxy: ${{ variables.BAREMETAL_HTTPS_PROXY }}
envVars: ${{ variables.BAREMETAL_ENV_VARS }}

- template: ../common_tasks/remove-from-acr.yml
parameters:
Expand Down
10 changes: 5 additions & 5 deletions .pipelines/templates/stages/testing_common/e2e-ab-update-stage-finalize-test-run.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ parameters:
type: string
default: ""

- name: httpsProxy
- name: envVars
Copy link
Contributor

@alejandro-microsoft alejandro-microsoft Sep 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think this could be a little misleading. The Go code was modified to receive the list of env variables, and I think each is received with the same flag, but here the parameter is just for the httpsProxy and it will use the env-vars flag. Changing the name here, would imply these are all the env variables, but more can be added by using the same flag too. This parameter only carries the httpsProxy value, but maybe the idea would be to store the rest here too (if so, I think you may need to handle the input in Go in a different way).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's true, although I was thinking that any env vars used for clean install would be the same ones used during a/b updates as well so it makes sense to set them once in baremetal-testing.yml and then just receive and use them here.
I think if we needed to add another env var in this template, we can also just append to the parameter env var and pass the whole list.

Copy link
Contributor

@alejandro-microsoft alejandro-microsoft Sep 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

If we needed to add another env var in this template, we can append to the parameter env var and pass the whole list.

I definitely, agree with this approach, but I'm unsure if with the current code the whole list of env vars can be passed in one string with just the flag at the beginning.

For example, can you pass just the flag and then the list of env variables (space separated or comma separated):
-env-vars HTTPS_PROXY=http://0.0.0.0 env-var2 env-var3
or do you need to precede each variable with the flag:
-env-vars HTTPS_PROXY=http://0.0.0.0 --env-vars env-var2 --env-vars env-var3
?

If it is the former, I see no problem. For the latter, it may be a good idea to explicitly add each env variable, change how the flags are added, or simply add a comment for clarification.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Either format should work, so both --env-vars VAR_ONE="var_one",VAR_TWO="var_two" and --env-vars VAR_ONE="var_one" --env-vars VAR_TWO="var_two should lead to the equivalent outcome.

I think though as you said it may be confusing, so I switched the parameter back to httpsProxy :)

type: string
default: ""

Expand All @@ -74,8 +74,8 @@ steps:
-s "${{ parameters.artifactsDirectory }}" > ./stage-ab-update-deployment.log 2>&1 &

PROXY_ARG=""
if [ -n "${{ parameters.httpsProxy }}" ]; then
PROXY_ARG="--proxy HTTPS_PROXY=${{ parameters.httpsProxy }}"
if [ -n "${{ parameters.envVars }}" ]; then
PROXY_ARG="--env-vars ${{ parameters.envVars }}"
fi

echo "Running script to stage A/B update..."
Expand Down Expand Up @@ -142,8 +142,8 @@ steps:
-s "${{ parameters.artifactsDirectory }}" > ./finalize-ab-update.log 2>&1 &

PROXY_ARG=""
if [ -n "${{ parameters.httpsProxy }}" ]; then
PROXY_ARG="--proxy HTTPS_PROXY=${{ parameters.httpsProxy }}"
if [ -n "${{ parameters.envVars }}" ]; then
PROXY_ARG="--env-vars ${{ parameters.envVars }}"
fi

echo "Running script to finalize A/B update..."
Expand Down
12 changes: 6 additions & 6 deletions .pipelines/templates/stages/testing_common/e2e-test-run.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ parameters:
type: string
default: ""

- name: httpsProxy
- name: envVars
type: string
default: ""

Expand Down Expand Up @@ -133,8 +133,8 @@ steps:
-p ${{ parameters.netlistenPort }} > ./stage-finalize-ab-update-runtime-os-B.log 2>&1 &

PROXY_ARG=""
if [ -n "${{ parameters.httpsProxy }}" ]; then
PROXY_ARG="--proxy HTTPS_PROXY=${{ parameters.httpsProxy }}"
if [ -n "${{ parameters.envVars }}" ]; then
PROXY_ARG="--env-vars ${{ parameters.envVars }}"
fi

echo "Running script to stage and finalize A/B update..."
Expand Down Expand Up @@ -242,8 +242,8 @@ steps:
-p ${{ parameters.netlistenPort }} > ./stage-finalize-ab-update-runtime-os-A.log 2>&1 &

PROXY_ARG=""
if [ -n "${{ parameters.httpsProxy }}" ]; then
PROXY_ARG="--proxy HTTPS_PROXY=${{ parameters.httpsProxy }}"
if [ -n "${{ parameters.envVars }}" ]; then
PROXY_ARG="--env-vars ${{ parameters.envVars }}"
fi

echo "Running script to stage and finalize A/B update..."
Expand Down Expand Up @@ -353,7 +353,7 @@ steps:
deploymentEnvironment: ${{ parameters.deploymentEnvironment }}
netlistenPort: ${{ parameters.netlistenPort }}
netlistenConfigFile: ${{ parameters.netlistenConfigFile }}
httpsProxy: ${{ parameters.httpsProxy }}
envVars: ${{ parameters.envVars }}

- template: ../testing_common/trident-rebuild.yml
parameters:
Expand Down
14 changes: 7 additions & 7 deletions tools/storm/helpers/ab_update.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,11 @@ type AbUpdateHelper struct {
args struct {
utils.SshCliSettings `embed:""`
utils.EnvCliSettings `embed:""`
TridentConfig string `short:"c" required:"" help:"File name of the custom read-write Trident config on the host to point Trident to."`
Version string `short:"v" required:"" help:"Version of the Trident image to use for the A/B update."`
StageAbUpdate bool `short:"s" help:"Controls whether A/B update should be staged."`
FinalizeAbUpdate bool `short:"f" help:"Controls whether A/B update should be finalized."`
Proxy string `help:"Proxy address. Input should include the env var name, i.e. HTTPS_PROXY=http://0.0.0.0."`
TridentConfig string `short:"c" required:"" help:"File name of the custom read-write Trident config on the host to point Trident to."`
Version string `short:"v" required:"" help:"Version of the Trident image to use for the A/B update."`
StageAbUpdate bool `short:"s" help:"Controls whether A/B update should be staged."`
FinalizeAbUpdate bool `short:"f" help:"Controls whether A/B update should be finalized."`
EnvVars []string `short:"e" help:"Environment variables, as a list of strings. Each var should include the env var name, i.e. HTTPS_PROXY=http://0.0.0.0."`
}

client *ssh.Client
Expand Down Expand Up @@ -65,7 +65,7 @@ func (h *AbUpdateHelper) getHostConfig(tc storm.TestCase) error {
}
})

out, err := utils.InvokeTrident(h.args.Env, h.client, h.args.Proxy, "get configuration")
out, err := utils.InvokeTrident(h.args.Env, h.client, h.args.EnvVars, "get configuration")
if err != nil {
return fmt.Errorf("failed to invoke Trident: %w", err)
}
Expand Down Expand Up @@ -230,7 +230,7 @@ func (h *AbUpdateHelper) triggerTridentUpdate(tc storm.TestCase) error {
for i := 1; ; i++ {
logrus.Infof("Invoking Trident attempt #%d with args: %s", i, args)

out, err := utils.InvokeTrident(h.args.Env, h.client, h.args.Proxy, args)
out, err := utils.InvokeTrident(h.args.Env, h.client, h.args.EnvVars, args)
if err != nil {
if err, ok := err.(*ssh.ExitMissingError); ok && strings.Contains(out.Stderr, "Rebooting system") {
// The connection closed without an exit code, and the output contains "Rebooting system".
Expand Down
22 changes: 14 additions & 8 deletions tools/storm/utils/trident.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,10 @@ const (
DOCKER_IMAGE_PATH = "/var/lib/trident/trident-container.tar.gz"
)

func BuildTridentContainerCommand(env string) string {
func BuildTridentContainerCommand(envVars []string) string {
cmd := DOCKER_COMMAND_BASE
if env != "" {
cmd += fmt.Sprintf("--env %s ", env)
if len(envVars) != 0 {
cmd += fmt.Sprintf("--env %s ", strings.Join(envVars, " "))
}
cmd += TRIDENT_CONTAINER
return cmd
Expand All @@ -38,23 +38,29 @@ func BuildTridentContainerCommand(env string) string {
// - The SSH session cannot be created
// - There was an error starting the command.
// - Some IO error occurred while reading stdout or stderr.
func InvokeTrident(env TridentEnvironment, client *ssh.Client, proxy string, arguments string) (*SshCmdOutput, error) {
func InvokeTrident(env TridentEnvironment, client *ssh.Client, envVars []string, arguments string) (*SshCmdOutput, error) {
var cmd string
switch env {
case TridentEnvironmentHost:
cmd = TRIDENT_BINARY
case TridentEnvironmentContainer:
cmd = BuildTridentContainerCommand(proxy)
cmd = BuildTridentContainerCommand(envVars)
case TridentEnvironmentNone:
return nil, fmt.Errorf("trident service is not running")
default:
return nil, fmt.Errorf("invalid environment: %s", env)
}

var cmdPrefix string
if proxy != "" {
envVar := strings.Split(proxy, "=")[0]
cmdPrefix = fmt.Sprintf("%s sudo --preserve-env=%s", proxy, envVar)
if len(envVars) != 0 {
var envKeys []string
for _, v := range envVars {
if key := strings.Split(v, "=")[0]; key != "" {
envKeys = append(envKeys, key)
Copy link

Copilot AI Sep 18, 2025

Choose a reason for hiding this comment

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

This code doesn't handle environment variables without an equals sign (e.g., just 'PATH'). strings.Split(v, "=")[0] will return the entire string if no '=' is found, but the check key != "" will always pass for non-empty strings, potentially causing issues with malformed env vars.

Suggested change
if key := strings.Split(v, "=")[0]; key != "" {
envKeys = append(envKeys, key)
if strings.Contains(v, "=") {
if key := strings.SplitN(v, "=", 2)[0]; key != "" {
envKeys = append(envKeys, key)
}

Copilot uses AI. Check for mistakes.
}
}
preservedEnvs := strings.Join(envKeys, ",")
cmdPrefix = fmt.Sprintf("%s sudo --preserve-env=%s", strings.Join(envVars, " "), preservedEnvs)
} else {
cmdPrefix = "sudo"
}
Expand Down
Loading