Skip to content

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

Open
aviruthen wants to merge 1 commit into
aws:masterfrom
aviruthen:fix-bedrock-tar-member-keys
Open

aviruthen wants to merge 1 commit 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.

This branch was successfully deployed

1 active deployment
auto-approve — f8541614 Deployed Oct 28, 2026 by aviruthen via wait-for-approval #770
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