fix(tasks): keep proxy and CA variables for ignore_env commands

[berendt]

run_command(ignore_env=True) passed the caller's env straight to
subprocess.Popen. Every caller on that path hands it an empty dict
(osism/tasks/openstack.py, four call sites), so openstack-image-manager,
openstack-flavor-manager, openstack-project-manager and everything going
through run_openstack_command_with_cloud() started with a completely empty
environment.

That dropped HTTP_PROXY, HTTPS_PROXY and NO_PROXY, so on a manager
without direct outbound connectivity the image manager could not reach the
image sources at all, which is the blocker reported in osism/issues#1432. The
aria2c subprocess that openstack-image-manager spawns inherited the same
empty environment. REQUESTS_CA_BUNDLE, CURL_CA_BUNDLE and SSL_CERT_FILE
were lost with them, and so were PATH, HOME, LANG and TZ.

The commit

fix(tasks): keep proxy and CA variables for ignore_env commands builds the
subprocess environment from an allowlist instead of from an empty dict.
osism/tasks/__init__.py gains ISOLATED_ENV_NAMES, ISOLATED_ENV_PREFIXES
and build_isolated_env(), and the ignore_env branch of run_command()
calls the helper. The allowlist carries PATH, HOME, LANG, LC_*, TZ,
SSL_CERT_FILE, SSL_CERT_DIR, REQUESTS_CA_BUNDLE, CURL_CA_BUNDLE and
HTTP_PROXY, HTTPS_PROXY, NO_PROXY, ALL_PROXY in both spellings, because
aria2 reads only the lowercase names. The caller's env is overlaid on top, so
an explicit value still wins.

The OS_* variables of openstack.env stay out, which is what the blanking
was for: they must not override the --cloud selection taken from
clouds.yaml.

No call site changes. The fix sits at the single seam so that every
ignore_env=True caller gets it.

tests/unit/tasks/test_init.py: test_run_command_ignore_env_passes_env_verbatim
asserted the old contract, that the env object reaches Popen by identity,
and is replaced by tests over the new one. The expected allowlist is written
out as a literal in the test file, independent of ISOLATED_ENV_NAMES, and one
parametrized case per name pins that it is inherited; a second parametrized
case covers the LC_ prefix rule. OS_AUTH_URL / OS_PASSWORD / OS_CLOUD,
an unrelated variable and a name that merely contains LC_ do not arrive, and
the caller's env wins without being mutated, including a key outside the
allowlist. The ignore_env=False test is untouched.

Verification

Not run locally, per the author's standing preference for this repository. The
unit tests above are the verification and run in the Zuul check job
python-osism-unit-tests. Locally only ruff format --check and
python -m py_compile were run over the two changed files, both clean.

Closes #2682

🤖 Generated with Claude Code

[sourcery-ai]

Hey - I've found 1 issue

Prompt for AI Agents

Please address the comments from this code review:

## Individual Comments

### Comment 1
<location path="osism/tasks/__init__.py" line_range="462-464" />
<code_context>
+def build_isolated_env(env):
+    """Build the environment for a subprocess started with ignore_env=True.
+
+    Returns the allowlisted variables of the current process, overlaid with
+    env. Everything outside the allowlist is dropped, the OS_* credentials of
+    openstack.env in particular.
+    """
+    command_env = {
</code_context>
<issue_to_address>
**nitpick:** The docstring says that everything outside the allowlist is dropped, but `command_env.update(env)` reintroduces every key supplied by the caller, including variables outside the allowlist. The documented isolation guarantee is therefore false for explicit caller values.

**Suggested fix:** Clarify that the allowlist applies to variables inherited from `os.environ`, while all explicitly supplied `env` entries are overlaid afterward.

```suggestion
    Returns the allowlisted variables inherited from os.environ, with all
    explicitly supplied env entries overlaid afterward. Variables outside the
    allowlist supplied through env are retained.
```
</issue_to_address>

Sourcery assessment

Needs a human reviewer. If the allowlist or inherited proxy/CA values are wrong, commands may fail, trust the wrong certificate bundle, or send requests through an unintended proxy; requests made before a revert could have external effects. Reverting restores the previous empty-environment behavior, but it cannot undo any requests already made.

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

[ideaship]

The commit message says nothing about the test change, while the PR description documents it properly, it names test_run_command_ignore_env_passes_env_verbatim, states the contract it asserted, and explains the three tests replacing it. Please carry one sentence of that into the commit message, since that is what survives in git log once the PR page is cold:

"Replaces test_run_command_ignore_env_passes_env_verbatim, which asserted that the caller's dict reaches Popen by identity, with three tests covering passthrough, OS_* exclusion and caller precedence."

[ideaship]

Please reword this. The first sentence scopes the allowlist correctly to the current process, and then "Everything outside the allowlist is dropped" drops that scope, so it also reads as a claim about the result, where it does not hold: command_env.update(env) reintroduces every caller-supplied key, allowlisted or not. The allowlist governs what is inherited; the overlay is unconditional, and was in the previous implementation too.

Worth being exact about because the loose reading invites the wrong repair, narrowing the overlay to match the prose. The allowlist is there for the ambient OS_* values of openstack.env, not for what a call site asks for deliberately, and ignore_env=False does not filter env either.

    Returns the allowlisted variables inherited from os.environ, overlaid with
    env. Variables outside the allowlist are not inherited; entries passed
    explicitly through env are always kept, including the OS_* credentials of
    openstack.env if a caller ever passes them.

[ideaship]

Please extend this to cover the whole allowlist. It currently pins five of the sixteen names in ISOLATED_ENV_NAMES (HTTPS_PROXY, https_proxy, NO_PROXY, REQUESTS_CA_BUNDLE, PATH) plus LC_ALL, which exercises the separate ISOLATED_ENV_PREFIXES rule rather than the set. Untested: HOME, LANG, TZ, SSL_CERT_FILE, SSL_CERT_DIR, CURL_CA_BUNDLE, HTTP_PROXY, ALL_PROXY, http_proxy, no_proxy, all_proxy. The allowlist is the whole deliverable of this change, so it should be the thing the test pins.

Please write the expected names out in the test file and parameterize over that literal, rather than iterating tasks.ISOLATED_ENV_NAMES, since deriving the expectation from the artifact under test passes whatever the artifact says. With an independent list, a misspelt SSL_CERT_FIEL in the frozenset means SSL_CERT_FILE stops being inherited and that case fails; a deleted name fails the same way. Please cover ISOLATED_ENV_PREFIXES too, LC_ALL is the only thing exercising it today and it is a separate mechanism from the set.

No unit test can confirm that these are the spellings the consuming libraries actually read; this buys divergence detection and a reviewable inventory.

[ideaship]

Please add one assertion here. The removed test_run_command_ignore_env_passes_env_verbatim was the only coverage that a caller key outside the allowlist reaches Popen, and this test only covers a caller overriding an allowlisted name. Pass {"HTTPS_PROXY": "http://explicit:3128", "TOOL_SPECIFIC": "x"}, add assert passed["TOOL_SPECIFIC"] == "x", and keep the caller-dict immutability assertion against the expanded dict.

All four call sites pass {} today (osism/tasks/openstack.py:783, 846, 928, 975), so this is coverage rather than a live bug, but it pins the half of the documented contract that a later "tighten the isolation" edit would quietly remove, and it pairs with the docstring correction above.