fix(agfs): fix s3 compatible for bytedance tos#742
Conversation
qin-ptr
left a comment
qin-ptr
left a comment
There was a problem hiding this comment.
Review Summary
这个 PR 修复了 AGFS 对字节云 TOS 的兼容性问题。核心方案合理，但存在测试代码中的 bug 需要修复。
核心变更
- 添加
disable_content_sha256配置项，禁用x-amz-content-sha256header - 使用 AWS SDK v2 的标准 middleware 实现，符合最佳实践
- 配置的完整生命周期管理做得很好
主要问题
- Blocking:
test_fs_s3.py:58存在 AttributeError 风险 - Blocking: Windows build 失败
- 测试代码的错误处理变更与 PR 主题无关，建议分离
详细评论见下方各文件。
🤖 I am a bot owned by @qin-ctx.
| ) | ||
| AGFS_CONF.mode = "http-client" | ||
|
|
||
|
|
There was a problem hiding this comment.
[Bug] (blocking)
如果 AGFS_CONF 是 None，这行代码会在模块加载时抛出 AttributeError: 'NoneType' object has no attribute 'mode'。
原代码有条件检查：
if AGFS_CONF is not None:
AGFS_CONF.mode = "http-client"pytestmark 只是标记测试跳过，不会阻止模块加载时执行顶层代码。
修复建议：
pytestmark = pytest.mark.skipif(
AGFS_CONF is None or AGFS_CONF.backend != "s3",
reason="AGFS S3 configuration not found in ov.conf",
)
if AGFS_CONF is not None:
AGFS_CONF.mode = "http-client"| except Exception: | ||
| return None | ||
| except Exception as e: | ||
| raise Exception("config load error:") from e |
There was a problem hiding this comment.
[Design] (non-blocking)
这个错误处理变更（从 return None 改为 raise Exception）会改变测试行为：配置加载失败时现在会抛出异常，而不是跳过测试。
这个变更与 PR 的主要目的（修复 TOS 兼容性）无关。建议：
- 将此变更分离到单独的 commit，说明原因（例如："让配置错误更早暴露"）
- 改进错误消息，提供更多上下文：
raise Exception(f"Failed to load AGFS config from {CONFIG_FILE}: {e}") from e
如果这是调试时的临时修改，考虑是否应该保留在最终版本中。
| return AGFSConfig(**agfs_data) | ||
| except Exception: | ||
| return None | ||
| except Exception as e: |
There was a problem hiding this comment.
[Design] (non-blocking)
同 test_fs_binding_s3.py 的评论，这个错误处理变更应该：
- 与主要功能分离（单独 commit 或 PR）
- 在 PR description 中说明原因
- 提供更详细的错误消息
| region_name=s3_conf.region, | ||
| endpoint_url=s3_conf.endpoint, | ||
| use_ssl=s3_conf.use_ssl, | ||
| # use_path_style=s3_conf.use_path_style, |
There was a problem hiding this comment.
[Suggestion] (non-blocking)
被注释的代码应该直接删除，而不是保留注释。如果将来需要，可以从 git 历史中恢复。
# 删除这行注释死代码会降低可读性，让后续维护者困惑。
|
|
||
| disable_content_sha256: bool = Field( | ||
| default=False, | ||
| description="Disable x-amz-content-sha256 header. Required for some S3-compatible services like TOS.", |
There was a problem hiding this comment.
[Suggestion] (non-blocking)
配置项的文档可以更详细，帮助用户理解何时需要启用此选项：
disable_content_sha256: bool = Field(
default=False,
description="Disable x-amz-content-sha256 header for S3-compatible services that don't support it "
"(e.g., ByteDance TOS, Huawei OBS). Keep disabled (False) for AWS S3 and MinIO.",
)| clientOpts = append(clientOpts, func(o *s3.Options) { | ||
| o.APIOptions = append(o.APIOptions, func(s *middleware.Stack) error { | ||
| return v4.SwapComputePayloadSHA256ForUnsignedPayloadMiddleware(s) | ||
| }) |
There was a problem hiding this comment.
[Good] (informational)
使用 SwapComputePayloadSHA256ForUnsignedPayloadMiddleware 是处理不支持 content-sha256 header 的 S3 兼容服务的标准做法。实现正确。✅
3 participants
修复对字节云tos的适配
Type of Change
Testing
描述如何测试这些更改：
Related Issues
Checklist