Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 46 additions & 4 deletions sdk/nexent/container/k8s_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import asyncio
import logging
import socket
import re
import uuid

import kubernetes
Expand All @@ -23,6 +24,47 @@

logger = logging.getLogger("nexent.container.kubernetes")

# Kubernetes naming constraints: lowercase alphanumeric or dash, cannot start/end with dash,
# cannot have consecutive dashes, max 253 characters

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[逻辑漏洞] _sanitize_k8s_name 函数在注释中提到了 Kubernetes 命名约束(最大 253 字符),但实际代码中未对长度进行截断。如果输入名称超过 253 字符,生成的 K8s 资源名称将无效,导致创建失败。建议在函数末尾添加长度截断逻辑:sanitized = sanitized[:253].rstrip('-')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[P2] 注释写 max 253,但这个 sanitizer 没有实际 enforce 长度,而且很多使用点是 label value/名称片段,常见上限是 63。注释会让调用方误以为长度已经安全,建议把长度作为参数并在函数内截断。

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[P2] 注释写 max 253,但这个 sanitizer 没有实际 enforce 长度,而且很多使用点是 label value/名称片段,常见上限是 63。注释会让调用方误以为长度已经安全,建议把长度作为参数并在函数内截断。

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

事后审查补充:[P2] 注释写 max 253,但这个 sanitizer 没有实际 enforce 长度,而且很多使用点是 label value/名称片段,常见上限是 63。注释会让调用方误以为长度已经安全,建议把长度作为参数并在函数内截断。

影响:这个问题合入后会在对应部署、运行或权限场景中留下真实故障/安全风险,后续排查成本较高。
建议:沿着上述风险点补齐校验、配置来源、权限边界或回归测试,避免同类问题再次出现。

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

二次事后审查补充:[P2] 注释写 max 253,但这个 sanitizer 没有实际 enforce 长度,而且很多使用点是 label value/名称片段,常见上限是 63。注释会让调用方误以为长度已经安全,建议把长度作为参数并在函数内截断。

影响:该问题合入后仍可能在真实部署、运行、权限或测试场景中形成回归风险。
建议:后续按这个风险点补齐边界校验、配置来源收敛、权限约束或针对性回归测试。

K8S_NAME_PATTERN = re.compile(r"[^a-z0-9-]+")
K8S_CONSECUTIVE_DASHES = re.compile(r"-+")


def _sanitize_k8s_name(name: str) -> str:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

_sanitize_k8s_namesanitized 为空时返回 "unknown",但当输入为 None 时,虽然 if not name 的 guard 能正确拦截(返回 True),但类型注解 name: str 应改为 Optional[str] 以准确反映函数实际接受的输入类型,提高代码可读性和类型检查的准确性。

"""Convert arbitrary string to valid Kubernetes resource name.

Rules:
- Convert to lowercase

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

_sanitize_k8s_name(None) 会在 name.lower() 处抛出 AttributeError,因为 None 没有 lower() 方法。if not name 检查在 name.lower() 之后才执行,无法拦截 None 输入。测试用例 test_sanitize_none 预期返回 "unknown" 但实际会崩溃。建议将 None 检查移到最前面:if not name: return "unknown" 改为 if name is None or not name: return "unknown"

- Replace invalid characters with dash
- Collapse consecutive dashes
- Remove leading/trailing dashes
- Must start with alphanumeric

Args:
name: Input string to sanitize

Returns:
Valid Kubernetes name (lowercase alphanumeric and dashes only)
"""
if not name:
return "unknown"

# Lowercase and replace invalid chars with dash
sanitized = K8S_NAME_PATTERN.sub("-", name.lower())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[P2] name 标注为 str,但测试又传 None;如果传入 truthy 的非字符串对象,name.lower() 会抛 AttributeError。边界入口应先 str(name) 或显式拒绝非字符串。

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[P2] name 标注为 str,但测试又传 None;如果传入 truthy 的非字符串对象,name.lower() 会抛 AttributeError。边界入口应先 str(name) 或显式拒绝非字符串。

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

事后审查补充:[P2] name 标注为 str,但测试又传 None;如果传入 truthy 的非字符串对象,name.lower() 会抛 AttributeError。边界入口应先 str(name) 或显式拒绝非字符串。

影响:这个问题合入后会在对应部署、运行或权限场景中留下真实故障/安全风险,后续排查成本较高。
建议:沿着上述风险点补齐校验、配置来源、权限边界或回归测试,避免同类问题再次出现。

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

二次事后审查补充:[P2] name 标注为 str,但测试又传 None;如果传入 truthy 的非字符串对象,name.lower() 会抛 AttributeError。边界入口应先 str(name) 或显式拒绝非字符串。

影响:该问题合入后仍可能在真实部署、运行、权限或测试场景中形成回归风险。
建议:后续按这个风险点补齐边界校验、配置来源收敛、权限约束或针对性回归测试。


# Collapse consecutive dashes
sanitized = K8S_CONSECUTIVE_DASHES.sub("-", sanitized)

# Remove leading/trailing dashes
sanitized = sanitized.strip("-")

# Ensure it starts with alphanumeric
if sanitized and not sanitized[0].isalnum():
sanitized = "x" + sanitized

# Fallback if empty
return sanitized if sanitized else "unknown"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

_sanitize_k8s_name 函数没有对输出长度进行限制。Kubernetes DNS label 名称最长 63 个字符(RFC 1123),而 pod 名称由 mcp-{safe_name}-{tenant}-{user}-{uuid} 拼接而成。如果 service_name 很长,最终 pod 名称可能超过 253 字符限制导致 K8s API 报错。建议在函数末尾添加截断逻辑,或在 _generate_pod_name 中对各段长度做约束。

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[P1] 返回值没有任何长度限制,长 service_name 经过清洗后仍然可以非常长,最终 pod name 会超过 Kubernetes 限制。这里应该在 sanitizer 或 _generate_pod_name 中截断并附稳定 hash。

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[P1] 返回值没有任何长度限制,长 service_name 经过清洗后仍然可以非常长,最终 pod name 会超过 Kubernetes 限制。这里应该在 sanitizer 或 _generate_pod_name 中截断并附稳定 hash。

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

事后审查补充:[P1] 返回值没有任何长度限制,长 service_name 经过清洗后仍然可以非常长,最终 pod name 会超过 Kubernetes 限制。这里应该在 sanitizer 或 _generate_pod_name 中截断并附稳定 hash。

影响:这个问题合入后会在对应部署、运行或权限场景中留下真实故障/安全风险,后续排查成本较高。
建议:沿着上述风险点补齐校验、配置来源、权限边界或回归测试,避免同类问题再次出现。

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

二次事后审查补充:[P1] 返回值没有任何长度限制,长 service_name 经过清洗后仍然可以非常长,最终 pod name 会超过 Kubernetes 限制。这里应该在 sanitizer 或 _generate_pod_name 中截断并附稳定 hash。

影响:该问题合入后仍可能在真实部署、运行、权限或测试场景中形成回归风险。
建议:后续按这个风险点补齐边界校验、配置来源收敛、权限约束或针对性回归测试。



class ContainerError(Exception):
"""Raised when container operation fails"""
Expand Down Expand Up @@ -77,9 +119,9 @@ def __init__(self, config: KubernetesContainerConfig):

def _generate_pod_name(self, service_name: str, tenant_id: str, user_id: str) -> str:
"""Generate unique pod name with service, tenant, and user segments."""
safe_name = "".join(c if c.isalnum() or c == "-" else "-" for c in service_name)
tenant_part = (tenant_id or "")[:8]
user_part = (user_id or "")[:8]
safe_name = _sanitize_k8s_name(service_name)
tenant_part = _sanitize_k8s_name(tenant_id)[:8]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[P2] tenant_part/user_part 只取前 8 位,tenantabcdef 和 tenantabcxyz 会生成同样前缀,排障和清理时容易误判。建议用前缀+短 hash,而不是纯截断。

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[P2] tenant_part/user_part 只取前 8 位,tenantabcdef 和 tenantabcxyz 会生成同样前缀,排障和清理时容易误判。建议用前缀+短 hash,而不是纯截断。

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

事后审查补充:[P2] tenant_part/user_part 只取前 8 位,tenantabcdef 和 tenantabcxyz 会生成同样前缀,排障和清理时容易误判。建议用前缀+短 hash,而不是纯截断。

影响:这个问题合入后会在对应部署、运行或权限场景中留下真实故障/安全风险,后续排查成本较高。
建议:沿着上述风险点补齐校验、配置来源、权限边界或回归测试,避免同类问题再次出现。

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

二次事后审查补充:[P2] tenant_part/user_part 只取前 8 位,tenantabcdef 和 tenantabcxyz 会生成同样前缀,排障和清理时容易误判。建议用前缀+短 hash,而不是纯截断。

影响:该问题合入后仍可能在真实部署、运行、权限或测试场景中形成回归风险。
建议:后续按这个风险点补齐边界校验、配置来源收敛、权限约束或针对性回归测试。

user_part = _sanitize_k8s_name(user_id)[:8]
uuid_part = uuid.uuid4().hex[:8]
return f"mcp-{safe_name}-{tenant_part}-{user_part}-{uuid_part}"

Expand Down Expand Up @@ -486,7 +528,7 @@ def list_containers(

# Filter by service_name if provided
if service_name:
safe_name = "".join(c if c.isalnum() or c == "-" else "-" for c in service_name)
safe_name = _sanitize_k8s_name(service_name)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[P2] list_containers 用 sanitize 后的 safe_name 再做子串匹配,service=api 会匹配到 my-api-v2 等不相关 pod。应和创建时的 component label 做精确匹配,或保存完整 sanitized service_name label。

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[P2] list_containers 用 sanitize 后的 safe_name 再做子串匹配,service=api 会匹配到 my-api-v2 等不相关 pod。应和创建时的 component label 做精确匹配,或保存完整 sanitized service_name label。

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

事后审查补充:[P2] list_containers 用 sanitize 后的 safe_name 再做子串匹配,service=api 会匹配到 my-api-v2 等不相关 pod。应和创建时的 component label 做精确匹配,或保存完整 sanitized service_name label。

影响:这个问题合入后会在对应部署、运行或权限场景中留下真实故障/安全风险,后续排查成本较高。
建议:沿着上述风险点补齐校验、配置来源、权限边界或回归测试,避免同类问题再次出现。

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

二次事后审查补充:[P2] list_containers 用 sanitize 后的 safe_name 再做子串匹配,service=api 会匹配到 my-api-v2 等不相关 pod。应和创建时的 component label 做精确匹配,或保存完整 sanitized service_name label。

影响:该问题合入后仍可能在真实部署、运行、权限或测试场景中形成回归风险。
建议:后续按这个风险点补齐边界校验、配置来源收敛、权限约束或针对性回归测试。

pod_component = labels.get(self.LABEL_COMPONENT, "")
if safe_name not in pod_component:
continue
Expand Down
168 changes: 164 additions & 4 deletions test/sdk/container/test_k8s_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
KubernetesContainerClient,
ContainerError,
ContainerConnectionError,
_sanitize_k8s_name,
)
from nexent.container.k8s_config import KubernetesContainerConfig

Expand Down Expand Up @@ -90,6 +91,79 @@ def mock_pod():
return pod


# ---------------------------------------------------------------------------
# Test _sanitize_k8s_name
# ---------------------------------------------------------------------------


class TestSanitizeK8sName:
"""Test _sanitize_k8s_name helper function"""

def test_sanitize_basic_alphanumeric(self):
"""Test basic alphanumeric string passes through"""
assert _sanitize_k8s_name("test-service") == "test-service"
assert _sanitize_k8s_name("abc123") == "abc123"

def test_sanitize_lowercase_conversion(self):
"""Test uppercase letters are converted to lowercase"""
assert _sanitize_k8s_name("TestService") == "testservice"
assert _sanitize_k8s_name("UPPERCASE") == "uppercase"

def test_sanitize_special_characters_replaced(self):
"""Test special characters are replaced with dash"""
assert _sanitize_k8s_name("test@service") == "test-service"
assert _sanitize_k8s_name("foo#bar") == "foo-bar"
assert _sanitize_k8s_name("test$123") == "test-123"

def test_sanitize_consecutive_special_chars(self):
"""Test consecutive special characters are collapsed to single dash"""
assert _sanitize_k8s_name("foo@@bar") == "foo-bar"
assert _sanitize_k8s_name("test@#$service") == "test-service"
assert _sanitize_k8s_name("a!!b") == "a-b"

def test_sanitize_leading_special_chars(self):
"""Test leading special characters are removed"""
assert _sanitize_k8s_name("@test") == "test"
assert _sanitize_k8s_name("#foo") == "foo"
assert _sanitize_k8s_name("!test@service") == "test-service"

def test_sanitize_trailing_special_chars(self):
"""Test trailing special characters are removed"""
assert _sanitize_k8s_name("test@") == "test"
assert _sanitize_k8s_name("test-service!") == "test-service"

def test_sanitize_mixed_case_with_specials(self):
"""Test mixed case with special characters"""
assert _sanitize_k8s_name("Foo@Bar!Test") == "foo-bar-test"

def test_sanitize_empty_string(self):
"""Test empty string returns 'unknown'"""
assert _sanitize_k8s_name("") == "unknown"

def test_sanitize_only_special_chars(self):
"""Test string with only special characters returns 'unknown'"""
assert _sanitize_k8s_name("@@@") == "unknown"
assert _sanitize_k8s_name("!@#") == "unknown"

def test_sanitize_none(self):
"""Test None returns 'unknown'"""
assert _sanitize_k8s_name(None) == "unknown"

def test_sanitize_with_dots(self):
"""Test dots are converted to dashes"""
assert _sanitize_k8s_name("foo.bar") == "foo-bar"
assert _sanitize_k8s_name("foo...bar") == "foo-bar"

def test_sanitize_underscore_replaced(self):
"""Test underscores are replaced with dash"""
assert _sanitize_k8s_name("foo_bar") == "foo-bar"

def test_sanitize_spaces_replaced(self):
"""Test spaces are replaced with dash"""
assert _sanitize_k8s_name("foo bar") == "foo-bar"
assert _sanitize_k8s_name("foo bar") == "foo-bar"


# ---------------------------------------------------------------------------
# Test KubernetesContainerClient.__init__
# ---------------------------------------------------------------------------
Expand Down Expand Up @@ -192,6 +266,72 @@ def test_generate_pod_name_with_special_chars(self, k8s_container_client):
assert "@" not in name
assert "#" not in name

def test_generate_pod_name_consecutive_special_chars(self, k8s_container_client):
"""Test pod name generation with consecutive special characters"""
with patch("nexent.container.k8s_client.uuid.uuid4") as mock_uuid:
mock_uuid.return_value.hex = "a1b2c3d4"
name = k8s_container_client._generate_pod_name(
"foo@@bar", "tenant123", "user12345")
assert name == "mcp-foo-bar-tenant12-user1234-a1b2c3d4"
assert "--" not in name

def test_generate_pod_name_leading_special_chars(self, k8s_container_client):
"""Test pod name generation with leading special characters"""
with patch("nexent.container.k8s_client.uuid.uuid4") as mock_uuid:
mock_uuid.return_value.hex = "a1b2c3d4"
name = k8s_container_client._generate_pod_name(
"@test-service", "tenant123", "user12345")
# "@test-service" -> "test-service" (leading @ stripped)
assert name.startswith("mcp-test")
assert not name.startswith("mcp-@")

def test_generate_pod_name_trailing_special_chars(self, k8s_container_client):
"""Test pod name generation with trailing special characters"""
with patch("nexent.container.k8s_client.uuid.uuid4") as mock_uuid:
mock_uuid.return_value.hex = "a1b2c3d4"
name = k8s_container_client._generate_pod_name(
"test-service@", "tenant123", "user12345")
assert name == "mcp-test-service-tenant12-user1234-a1b2c3d4"
assert name.endswith("-a1b2c3d4")

def test_generate_pod_name_uppercase(self, k8s_container_client):
"""Test pod name generation with uppercase letters"""
with patch("nexent.container.k8s_client.uuid.uuid4") as mock_uuid:
mock_uuid.return_value.hex = "a1b2c3d4"
name = k8s_container_client._generate_pod_name(
"TestService", "tenant123", "user12345")
assert name == "mcp-testservice-tenant12-user1234-a1b2c3d4"

def test_generate_pod_name_underscores(self, k8s_container_client):
"""Test pod name generation with underscores"""
with patch("nexent.container.k8s_client.uuid.uuid4") as mock_uuid:
mock_uuid.return_value.hex = "a1b2c3d4"
name = k8s_container_client._generate_pod_name(
"test_service", "tenant_123", "user_12345")
# tenant_123 -> tenant-123 (9 chars), truncated to 8 -> tenant-1
# user_12345 -> user-12345 (10 chars), truncated to 8 -> user-123
assert name == "mcp-test-service-tenant-1-user-123-a1b2c3d4"

def test_generate_pod_name_dots(self, k8s_container_client):
"""Test pod name generation with dots"""
with patch("nexent.container.k8s_client.uuid.uuid4") as mock_uuid:
mock_uuid.return_value.hex = "a1b2c3d4"
name = k8s_container_client._generate_pod_name(
"test.service", "tenant.123", "user.12345")
# tenant.123 -> tenant.123 (9 chars), truncated to 8 -> tenant.1
# user.12345 -> user.12345 (10 chars), truncated to 8 -> user.123
assert name == "mcp-test-service-tenant-1-user-123-a1b2c3d4"

def test_generate_pod_name_spaces(self, k8s_container_client):
"""Test pod name generation with spaces"""
with patch("nexent.container.k8s_client.uuid.uuid4") as mock_uuid:
mock_uuid.return_value.hex = "a1b2c3d4"
name = k8s_container_client._generate_pod_name(
"test service", "tenant 123", "user 12345")
# tenant 123 -> tenant 123 (9 chars), truncated to 8 -> tenant 1
# user 12345 -> user 12345 (10 chars), truncated to 8 -> user 123
assert name == "mcp-test-service-tenant-1-user-123-a1b2c3d4"

def test_generate_pod_name_long_user_id(self, k8s_container_client):
"""Test pod name generation with long user ID"""
long_user_id = "a" * 20
Expand All @@ -216,31 +356,31 @@ def test_generate_pod_name_empty_tenant(self, k8s_container_client):
mock_uuid.return_value.hex = "a1b2c3d4"
name = k8s_container_client._generate_pod_name(
"test-service", "", "user12345")
assert name == "mcp-test-service--user1234-a1b2c3d4"
assert name == "mcp-test-service-unknown-user1234-a1b2c3d4"

def test_generate_pod_name_empty_user(self, k8s_container_client):
"""Test pod name generation with empty user_id"""
with patch("nexent.container.k8s_client.uuid.uuid4") as mock_uuid:
mock_uuid.return_value.hex = "a1b2c3d4"
name = k8s_container_client._generate_pod_name(
"test-service", "tenant123", "")
assert name == "mcp-test-service-tenant12--a1b2c3d4"
assert name == "mcp-test-service-tenant12-unknown-a1b2c3d4"

def test_generate_pod_name_none_tenant(self, k8s_container_client):
"""Test pod name generation with None tenant_id"""
with patch("nexent.container.k8s_client.uuid.uuid4") as mock_uuid:
mock_uuid.return_value.hex = "a1b2c3d4"
name = k8s_container_client._generate_pod_name(
"test-service", None, "user12345")
assert name == "mcp-test-service--user1234-a1b2c3d4"
assert name == "mcp-test-service-unknown-user1234-a1b2c3d4"

def test_generate_pod_name_none_user(self, k8s_container_client):
"""Test pod name generation with None user_id"""
with patch("nexent.container.k8s_client.uuid.uuid4") as mock_uuid:
mock_uuid.return_value.hex = "a1b2c3d4"
name = k8s_container_client._generate_pod_name(
"test-service", "tenant123", None)
assert name == "mcp-test-service-tenant12--a1b2c3d4"
assert name == "mcp-test-service-tenant12-unknown-a1b2c3d4"


# ---------------------------------------------------------------------------
Expand Down Expand Up @@ -1265,6 +1405,26 @@ def test_list_containers_service_filter_special_chars(self, k8s_container_client

assert len(result) == 0

def test_list_containers_service_filter_consecutive_special_chars(self, k8s_container_client, mock_pod):
"""Test listing containers with service filter containing consecutive special characters"""
k8s_container_client.core_v1.list_namespaced_pod.return_value = MagicMock(items=[mock_pod])

# The sanitized version of "test@@service" is "test-service"
# Since mock_pod's component is "test-service", it should match
result = k8s_container_client.list_containers(service_name="test@@service")

assert len(result) == 1

def test_list_containers_service_filter_leading_special_chars(self, k8s_container_client, mock_pod):
"""Test listing containers with service filter containing leading special characters"""
k8s_container_client.core_v1.list_namespaced_pod.return_value = MagicMock(items=[mock_pod])

# The sanitized version of "@test-service" is "test-service" (leading @ stripped)
# Since mock_pod's component is "test-service", it should match
result = k8s_container_client.list_containers(service_name="@test-service")

assert len(result) == 1

def test_list_containers_pod_no_ports(self, k8s_container_client):
"""Test listing containers when pod has no ports configured"""
mock_pod_no_ports = MagicMock()
Expand Down
Loading