feat(auth): reuse existing docker login (OAT) instead of requiring credential.json - #168
Conversation
…al.json
Reads ${DOCKER_CONFIG:-~/.docker}/config.json the same way the Docker
CLI does, so a machine already authenticated via `docker login`
(including an org access token) is not forced to duplicate credentials
into credential.json, and blank placeholder credentials never override
or break a working login. Also distinguishes insufficient_scope
(authorization) from unauthorized (authentication) in base-image pull
failures so the error message points at the right fix, and logs in
before `docker build --pull` only when there's no existing login to
reuse.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…s via PATH Honor $ROCM_PATH (falling back to /opt/rocm) instead of hardcoding /opt/rocm in amd_smi_utils.py, rocm_smi_utils.py, and gpu_info_profiler.py, and detect nvidia-smi/rocm-smi/amd-smi via `command -v` in gpu_info_pre.sh instead of a fixed binary path, so detection works when ROCm is installed elsewhere or GPU tools are only on PATH. Also make the rpd tracer's LD_LIBRARY_PATH ROCm-path-aware, and fall back to saving the raw trace.rpd when rpd2tracing.py fails instead of losing the trace. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR updates madengine’s Docker registry authentication flow to reuse existing ambient docker login state (including OATs) by reading ${DOCKER_CONFIG:-~/.docker}/config.json, avoiding duplicated credentials in credential.json, and improves diagnostics for base-image pull denials. It also includes ROCm/GPU tooling path detection improvements and a trace post-processing fallback.
Changes:
- Add ambient Docker auth detection and login precedence/escape-hatch behavior (
MAD_SKIP_DOCKER_LOGIN=1), plus clearer “insufficient_scope” vs “unauthorized” denial messaging. - Update build flow to avoid re-authenticating when an ambient login already exists and emit a targeted hint on registry pull denial.
- Make ROCm/GPU tooling honor
$ROCM_PATH, detect GPU tools via PATH in shell, and preserve raw trace output if conversion fails.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_auth.py | Adds unit coverage for ambient auth detection, blank credential handling, denial explanation, and skip-login behavior. |
| src/madengine/core/auth.py | Implements ambient Docker auth detection, credential usability checks, denial explanation, and updated login precedence/skip behavior. |
| src/madengine/execution/docker_builder.py | Logs in to base-image registry only when needed and prints actionable hints on base-image pull denial. |
| src/madengine/core/console.py | Ensures captured command output is surfaced on failure for better diagnostics. |
| src/madengine/orchestration/build_orchestrator.py | Allows ambient docker login to satisfy push auth requirements when explicit creds are absent. |
| docs/configuration.md | Documents ambient Docker auth reuse, precedence table, and new environment variables/denial semantics. |
| src/madengine/scripts/common/tools/rocm_smi_utils.py | Uses $ROCM_PATH to locate ROCm SMI Python bindings. |
| src/madengine/scripts/common/tools/amd_smi_utils.py | Uses $ROCM_PATH to locate AMD SMI Python bindings. |
| src/madengine/scripts/common/tools/gpu_info_profiler.py | Updates ROCm path usage in GPU vendor detection and AMD SMI binding discovery. |
| src/madengine/scripts/common/tools.json | Adjusts RPD tool invocation to incorporate $ROCM_PATH into LD_LIBRARY_PATH. |
| src/madengine/scripts/common/pre_scripts/gpu_info_pre.sh | Switches GPU tool detection to PATH-based checks. |
| src/madengine/scripts/common/post_scripts/trace.sh | Falls back to saving raw trace.rpd if conversion fails. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/madengine/scripts/common/tools/gpu_info_profiler.py:106
- detect_gpu_vendor() still relies on hardcoded filesystem paths (e.g. /usr/bin/nvidia-smi) instead of using PATH discovery ("command -v" / shutil.which) as described in the PR summary. This can fail to detect GPU tooling when the binaries are on PATH but not in those exact locations.
rocm_path = os.environ.get("ROCM_PATH", "/opt/rocm")
if os.path.exists("/usr/bin/nvidia-smi"):
return True, False
elif os.path.exists(f"{rocm_path}/bin/rocm-smi") or check_amd_smi_available():
return False, True
src/madengine/scripts/common/tools/gpu_info_profiler.py:111
- This error message hardcodes tool locations (e.g. "/usr/bin/nvidia-smi") even though the detection logic is moving toward PATH-based discovery and ROCM_PATH. The message can mislead users on systems where the tools exist on PATH but not at those exact paths.
"Unable to detect GPU vendor. No GPU management tools found.\n"
"For NVIDIA: /usr/bin/nvidia-smi not found\n"
f"For AMD: {rocm_path}/bin/rocm-smi and amd-smi not found\n\n"
…a PATH rocminfo may be absent even when rocm-smi/amd-smi are present; call it only when available instead of failing the pre-script. Also prefer PATH lookups over hardcoded /usr/bin and $ROCM_PATH/bin paths when detecting GPU vendor. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
explain_registry_denial() always suggested Docker Hub credentials even when the failing image referenced another registry (e.g. ghcr.io). Extract the registry host from the image reference and tailor the docker login / credential.json suggestions to it, falling back to the existing Docker Hub guidance when the image has no registry host. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/madengine/execution/docker_builder.py:276
- If the base image cannot be resolved (base_docker == ""), _registry_of() currently falls back to "docker.io", which can trigger an unnecessary/incorrect docker-login attempt (and even a misleading "missing dockerhub creds" message) despite not knowing what registry is actually needed. Skip the ambient-auth check/login when base_docker is empty.
base_docker = self._resolve_base_docker(dockerfile)
base_registry = self._registry_of(base_docker)
if credentials and not has_ambient_docker_auth(base_registry):
src/madengine/core/auth.py:114
- has_ambient_docker_auth() only checks non-DockerHub registries under the bare host key (e.g. "ghcr.io"). Docker config.json commonly stores auth keys with scheme/prefix variants (similar to the Docker Hub https://index.docker.io/v1/ entry). If the host is stored as https:///v1/, this will incorrectly report "no ambient auth" and cause redundant login attempts.
# Downstream code derives the registry host the same way (docker login <host>).
host = registry.split("/")[0]
if host.lower() in _DOCKERHUB_ALIASES:
return _DOCKERHUB_CONFIG_KEYS
return (host,)
Summary
docker login— including an organization access token (OAT) — by reading${DOCKER_CONFIG:-~/.docker}/config.jsonthe same way the Docker CLI does, so credentialsdon't need to be duplicated into
credential.json. Blank/placeholder credential entries are treated as "not configured" and never override or break a working login.insufficient_scope(authorization) fromunauthorized/pull access denied(authentication) in base-image pull failures, so the error message points at the actual fixinstead of telling users to re-run
docker loginwhen the real problem is token scope.docker build --pullonly when there's no existing login to reuse, so an already-authenticated node isn't re-authenticated.MAD_SKIP_DOCKER_LOGIN=1escape hatch to always defer to ambient credentials.amd_smi_utils.py,rocm_smi_utils.py,gpu_info_profiler.py,gpu_info_pre.sh) honor$ROCM_PATHand detectnvidia-smi/rocm-smi/amd-smiviacommand -vinstead of hardcoded/opt/rocmpaths, plus a fallback intrace.shto save the rawtrace.rpdifrpd2tracing.pyfails.Test plan
pytest tests/unit/test_auth.py -vdocker buildon a node with only an ambientdocker login(nocredential.jsonentry) succeeds without prompting for credentialsMAD_SKIP_DOCKER_LOGIN=1bypasses login attempts even whencredential.jsonhas valid credentials/opt/rocmvia$ROCM_PATH