Skip to content

fix: normalize tar member paths when extracting Bedrock model artifacts - #6543

Open
aviruthen wants to merge 3 commits into
aws:masterfrom
aviruthen:fix-bedrock-tar-member-keys
Open

aviruthen wants to merge 3 commits into
aws:masterfrom
aviruthen:fix-bedrock-tar-member-keys

Conversation

@aviruthen

Copy link
Copy Markdown
Collaborator

Summary

When BedrockModelBuilder.deploy() imports an OSS model whose artifacts are a model.tar.gz, _extract_tar_gz_to_s3 uploads each archive member to a sibling extracted/ prefix. Destination keys were built by appending the raw tar member name. Archives created with tar -C <dir> . therefore produced keys like extracted/./config.json, which don't line up with the config.json / checkpoints/hf*/ lookups done right after extraction.

This change normalizes member names into clean relative keys before upload.

Changes

  • sagemaker-serve/src/sagemaker/serve/bedrock_model_builder.py:
    • Add _tar_member_relative_key(), which strips leading ./ and empty segments (for example, ./checkpoints//hf/config.json becomes checkpoints/hf/config.json).
    • Members whose names can't be represented as a relative key under the extraction prefix (absolute paths, drive-letter paths, or .. segments) are skipped with a warning. This matches how custom_extractall_tarfile filters members elsewhere in the SDK.
    • Existing behavior is preserved: non-file members are skipped, the existing-prefix short-circuit still applies, and RuntimeError is still raised if nothing is extracted.

Testing

  • Added TestTarMemberRelativeKey and TestExtractTarGzToS3 to sagemaker-serve/tests/unit/test_bedrock_model_builder.py. These use real in-memory .tar.gz archives with a mocked S3 client to check:
    • normalized keys for plain, nested, and ./-prefixed members
    • unsupported member paths are skipped
    • the empty-result error
    • the existing-prefix short-circuit
  • tests/unit/test_bedrock_model_builder.py passes (123 tests).
  • black and flake8 are clean on the changed files.

@aviruthen
aviruthen deployed to auto-approve October 28, 2026 22:49 — with GitHub Actions Active
@aviruthen
aviruthen deployed to auto-approve October 28, 2026 22:49 — with GitHub Actions Active
@aviruthen
aviruthen deployed to auto-approve October 28, 2026 22:50 — with GitHub Actions Active

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🤖 AI code review

This PR fixes BedrockModelBuilder._extract_tar_gz_to_s3 so tar member names (e.g. from tar -C <dir> . producing ./config.json) are normalized into clean relative S3 keys, and rejects absolute/drive-letter/.. member paths. The new _tar_member_relative_key helper and its behavior are consistent with the SDK's existing tar-safety conventions (PR #6195), and the added unit tests cover normalization, rejection of unsafe paths, the empty-result error, and the existing-prefix short-circuit. The change looks correct and well-tested; I found no blocking issues.

Reviewed commit f854161. Automated review; verify before acting.

@aviruthen
aviruthen deployed to auto-approve October 29, 2026 23:44 — with GitHub Actions Active
@aviruthen
aviruthen deployed to auto-approve October 29, 2026 23:45 — with GitHub Actions Active
@aviruthen
aviruthen deployed to auto-approve October 29, 2026 23:45 — with GitHub Actions Active

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🤖 AI code review

This PR fixes a real bug in BedrockModelBuilder._extract_tar_gz_to_s3 where tar member names like ./config.json produced S3 keys (extracted/./config.json) that didn't match the post-extraction config.json/checkpoints/ lookups. The new _tar_member_relative_key helper normalizes member names and rejects absolute/../drive-letter paths, consistent with the SDK's existing tar-extraction hardening (PR #6195). The change is well-scoped and covered by thorough unit tests using real in-memory archives. No blocking issues found.

Reviewed commit 1ad6824. Automated review; verify before acting.

@aviruthen
aviruthen deployed to auto-approve October 30, 2026 21:16 — with GitHub Actions Active
@aviruthen
aviruthen deployed to auto-approve October 30, 2026 21:16 — with GitHub Actions Active
@aviruthen
aviruthen deployed to auto-approve October 30, 2026 21:16 — with GitHub Actions Active

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🤖 AI code review

This PR fixes _extract_tar_gz_to_s3 in bedrock_model_builder.py so tar member names are normalized into clean relative keys (stripping ./ and empty segments) before upload, fixing a mismatch where archives created with tar -C <dir> . produced keys like extracted/./config.json. It also rejects absolute/drive-letter/.. paths with a warning, consistent with existing tar-extraction safety work in the SDK (Source: #6195). The change is well-scoped, preserves existing behavior, and ships thorough unit tests covering normalization, directory skipping, unsupported-path skipping, the empty-result error, and the existing-prefix short-circuit. I found no issues worth blocking on.

Reviewed commit 06e042d. Automated review; verify before acting.

This branch was successfully deployed

1 active deployment
auto-approve — 06e042d4 Deployed Oct 30, 2026 by aviruthen via wait-for-approval #1880
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant