test(discovery): kind-based real-cluster e2e for multi-port discovery - #2063
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds opt-in Kind-based end-to-end tests for Kubernetes service discovery. The change defines mock workloads, local and in-cluster gateway fixtures, lifecycle coverage, fleet-mode coverage, and a manual GitHub Actions workflow. ChangesKind discovery E2E
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant GitHub Actions
participant Kind
participant Kubernetes
participant smg
participant MockEngine
GitHub Actions->>Kind: Create cluster and apply manifests
Kind->>Kubernetes: Start gateway and engine workloads
GitHub Actions->>smg: Start discovery gateway
smg->>Kubernetes: Watch annotated engine Pods
smg->>MockEngine: Query worker endpoints
smg-->>GitHub Actions: Route requests to discovered workers
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
👋 The PR description doesn't fully follow
Please update the PR description so reviewers have the context they need. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| curl -Lo kind "https://kind.sigs.k8s.io/dl/v0.29.0/kind-linux-amd64" | ||
| chmod +x kind && sudo mv kind /usr/local/bin/ | ||
| curl -Lo kubectl "https://dl.k8s.io/release/v1.33.0/bin/linux/amd64/kubectl" | ||
| chmod +x kubectl && sudo mv kubectl /usr/local/bin/ |
There was a problem hiding this comment.
🟡 Nit: Other workflows in this repo (e.g. nightly-triage.yml, engine-version-watch.yml) pin downloaded binaries with SHA256 checksums. These curl -Lo calls for kind and kubectl skip verification, which is inconsistent and leaves a supply-chain gap — a compromised CDN or DNS hijack could inject a tampered binary.
Consider adding checksum verification to match the existing repo pattern:
| curl -Lo kind "https://kind.sigs.k8s.io/dl/v0.29.0/kind-linux-amd64" | |
| chmod +x kind && sudo mv kind /usr/local/bin/ | |
| curl -Lo kubectl "https://dl.k8s.io/release/v1.33.0/bin/linux/amd64/kubectl" | |
| chmod +x kubectl && sudo mv kubectl /usr/local/bin/ | |
| curl -Lo kind "https://kind.sigs.k8s.io/dl/v0.29.0/kind-linux-amd64" | |
| echo "<sha256> kind" | sha256sum --check --quiet | |
| chmod +x kind && sudo mv kind /usr/local/bin/ | |
| curl -Lo kubectl "https://dl.k8s.io/release/v1.33.0/bin/linux/amd64/kubectl" | |
| echo "<sha256> kubectl" | sha256sum --check --quiet | |
| chmod +x kubectl && sudo mv kubectl /usr/local/bin/ |
There was a problem hiding this comment.
Clean, well-structured e2e test. The kind cluster setup, mock engine server, hostNetwork approach, and assertion logic all look solid. Good cleanup trap with diagnostics on failure.
0 🔴 Important · 1 🟡 Nit · 0 🟣 Pre-existing
The one nit is about adding SHA256 checksum verification for the kind/kubectl downloads to match the pattern used in other workflows.
f58fe15 to
3096662
Compare
|
Validated end-to-end on a real cluster: temporarily added a branch push trigger (since workflow_dispatch only registers from the default branch), ran on GitHub-hosted ubuntu-latest, then removed the trigger and force-pushed the branch back to its single clean commit. Run: https://github.com/smg-project/smg/actions/runs/31047416285 — success. Timeline from the log: kind cluster up in ~45s → both pods Ready → smg discovered 5 workers in ~4s (4 annotated ports + fallback) → ownership-label assertions passed → graceful delete converged in ~6s → force delete converged in ~2s → PASS. Total e2e step ~1 minute; whole job ~16 minutes (dominated by the Rust build). |
9747100 to
1a898a3
Compare
3096662 to
a353d62
Compare
1a898a3 to
763101b
Compare
a353d62 to
3e9e386
Compare
763101b to
596fde7
Compare
3e9e386 to
fb746af
Compare
fb746af to
fc26a6c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/e2e-kind-discovery.yml:
- Around line 6-9: Remove the push trigger and its test/discovery-kind-e2e
branch filter from the workflow, leaving only the workflow_dispatch trigger for
manual execution.
In `@e2e_test/kind_discovery/run.sh`:
- Around line 28-29: Update workers() to propagate curl failures instead of
converting them to an empty JSON object. In wait_for_count, capture the
worker_count result, detect a failed command, and reject immediately before
comparing the count; preserve normal count-based waiting when the query
succeeds.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ccbb9469-5ea5-4123-a531-792a9e884949
📒 Files selected for processing (4)
.github/workflows/e2e-kind-discovery.ymle2e_test/kind_discovery/burst.yamle2e_test/kind_discovery/manifests.yamle2e_test/kind_discovery/run.sh
| workers() { curl -sf "http://127.0.0.1:${SMG_PORT}/workers" || echo '{}'; } | ||
| worker_count() { workers | jq '[(.workers // .)[]?] | length'; } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🔴 Important Propagate /workers query failures.
Line 28 converts every HTTP failure into an empty worker set. A live smg process can still have no reachable /workers endpoint. In that case, wait_for_count 0 succeeds and the forced-deletion check reports a false pass.
Make workers fail on a curl error. In wait_for_count, capture and reject a failed worker_count before comparing the count.
Proposed fix
-workers() { curl -sf "http://127.0.0.1:${SMG_PORT}/workers" || echo '{}'; }
+workers() { curl -fsS "http://127.0.0.1:${SMG_PORT}/workers"; }
wait_for_count() {
local expect=$1 what=$2 deadline=$((SECONDS + 120))
while ((SECONDS < deadline)); do
+ local actual
# A dead gateway must never satisfy an expected-zero count.
if [[ -n ${SMG_PID} ]] && ! kill -0 "${SMG_PID}" 2>/dev/null; then
echo "smg process died while waiting for ${what}"
return 1
fi
- if [[ "$(worker_count)" == "${expect}" ]]; then return 0; fi
+ if ! actual="$(worker_count)"; then
+ echo "could not query workers while waiting for ${what}"
+ return 1
+ fi
+ if [[ "${actual}" == "${expect}" ]]; then return 0; fiAs per coding guidelines, run the silent-failure-hunter agent to detect “swallowed errors, inappropriate fallbacks, and missing error propagation.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@e2e_test/kind_discovery/run.sh` around lines 28 - 29, Update workers() to
propagate curl failures instead of converting them to an empty JSON object. In
wait_for_count, capture the worker_count result, detect a failed command, and
reject immediately before comparing the count; preserve normal count-based
waiting when the query succeeds.
Source: Coding guidelines
| def __init__(self, binary: Path): | ||
| self.binary = binary | ||
| self.base_url = f"http://127.0.0.1:{SMG_PORT}" | ||
| self.log_path = Path(tempfile.mkstemp(prefix="smg-kind-e2e-", suffix=".log")[1]) |
There was a problem hiding this comment.
🟡 Nit: mkstemp returns (fd, name) — discarding [0] leaks the open file descriptor. Either close it explicitly or use NamedTemporaryFile(delete=False):
| self.log_path = Path(tempfile.mkstemp(prefix="smg-kind-e2e-", suffix=".log")[1]) | |
| fd, path = tempfile.mkstemp(prefix="smg-kind-e2e-", suffix=".log") | |
| os.close(fd) | |
| self.log_path = Path(path) |
| self.proc: subprocess.Popen | None = None | ||
|
|
||
| def start(self) -> None: | ||
| log = open(self.log_path, "a") |
There was a problem hiding this comment.
🟡 Nit: The file handle opened here is never closed. After restart() (called by test_gateway_restart_rebuilds_registry), a second handle is opened to the same file while the first remains alive. Consider storing it on self and closing it in stop():
def start(self) -> None:
self._log_fh = open(self.log_path, "a")
self.proc = subprocess.Popen(..., stdout=self._log_fh, ...)
def stop(self) -> None:
...
if hasattr(self, "_log_fh") and self._log_fh:
self._log_fh.close()
self._log_fh = None
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/e2e-kind-discovery.yml (2)
37-42: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winFail immediately on HTTP download errors.
curl -Losaves the response body and exits successfully for server errors unless--failis set. Add--failto both download commands so this step fails immediately instead of using a malformed binary or error page.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/e2e-kind-discovery.yml around lines 37 - 42, Add the curl HTTP failure flag to both download commands in the “Install kind and kubectl” step, ensuring kind and kubectl downloads fail immediately on server-error responses while preserving the existing destinations and permissions.Source: Coding guidelines
37-42: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSecurity Misconfiguration (CWE-494): Download of Code Without Integrity Check
Reachability: External
Verify the downloaded Kind and kubectl binaries before installation.
Lines 39 and 41 install executables from external releases without a checksum or signature check. A compromised release asset can run on the workflow runner when the Kind E2E tests invoke
kindorkubectl. Add the official SHA-256 or signature validation forkind-linux-amd64andkubectlbeforechmod/sudo mv, and fail before installation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/e2e-kind-discovery.yml around lines 37 - 42, Update the “Install kind and kubectl” workflow step to verify both downloaded binaries against their official SHA-256 checksums or signatures before making them executable or moving them into /usr/local/bin. Fetch the expected verification data for the pinned releases, validate each file, and ensure any mismatch fails the step before installation.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e_test/kind_discovery/conftest.py`:
- Around line 77-82: Update workers() and the wait_for_count flow so request
failures and non-2xx /workers responses are not converted into an empty worker
list. Preserve retries for transient failures, but only evaluate the expected
count—including zero—after a successful response whose payload has the valid
worker-list schema; otherwise continue waiting or propagate the failure so
forced deletion cannot pass without confirming worker removal.
---
Outside diff comments:
In @.github/workflows/e2e-kind-discovery.yml:
- Around line 37-42: Add the curl HTTP failure flag to both download commands in
the “Install kind and kubectl” step, ensuring kind and kubectl downloads fail
immediately on server-error responses while preserving the existing destinations
and permissions.
- Around line 37-42: Update the “Install kind and kubectl” workflow step to
verify both downloaded binaries against their official SHA-256 checksums or
signatures before making them executable or moving them into /usr/local/bin.
Fetch the expected verification data for the pinned releases, validate each
file, and ensure any mismatch fails the step before installation.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 982658b2-0ab6-47d2-9137-b75a9141e8cf
📒 Files selected for processing (6)
.github/workflows/e2e-kind-discovery.ymle2e_test/kind_discovery/__init__.pye2e_test/kind_discovery/conftest.pye2e_test/kind_discovery/manifests.yamle2e_test/kind_discovery/test_kind_discovery.pye2e_test/pyproject.toml
| def workers(self) -> list[dict]: | ||
| try: | ||
| payload = requests.get(f"{self.base_url}/workers", timeout=5).json() | ||
| except requests.RequestException: | ||
| return [] | ||
| return payload.get("workers", payload if isinstance(payload, list) else []) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🔴 Important Do not treat an unavailable /workers endpoint as an empty worker set.
workers() returns [] for request failures and does not reject non-2xx responses. wait_for_count(0, ...) can then pass while the smg process is alive but /workers is unavailable or failing. This allows the forced-deletion test to pass without confirming worker removal.
Keep retrying transient request failures in wait_for_count, but require a successful response with a valid worker-list schema before accepting an expected count of zero. As per coding guidelines, prioritize missing error handling and inappropriate fallbacks.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 78-78: Request-controlled URL passed to requests; validate against an allowlist to prevent SSRF.
Context: requests.get(f"{self.base_url}/workers", timeout=5)
Note: [CWE-918] Server-Side Request Forgery (SSRF).
(ssrf-requests)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@e2e_test/kind_discovery/conftest.py` around lines 77 - 82, Update workers()
and the wait_for_count flow so request failures and non-2xx /workers responses
are not converted into an empty worker list. Preserve retries for transient
failures, but only evaluate the expected count—including zero—after a successful
response whose payload has the valid worker-list schema; otherwise continue
waiting or propagate the failure so forced deletion cannot pass without
confirming worker removal.
Source: Coding guidelines
| docker build -t smg-gateway:e2e \ | ||
| -f e2e_test/kind_discovery/Dockerfile.smg target/debug | ||
| # Fail here (not as CrashLoopBackOff later) on any linkage problem. | ||
| docker run --rm smg-mock-worker:e2e --help >/dev/null 2>&1 || true |
There was a problem hiding this comment.
🟡 Nit: || true silently swallows failures, contradicting the comment on the line above ("Fail here … on any linkage problem"). A missing shared library would exit non-zero and be masked. The gateway check on the next line does fail properly — if mock-worker also supports --help, drop the || true; if it doesn't, the exit code from an unsupported flag is still distinct from a linkage crash (signal vs exit code), so consider checking the exit code explicitly instead.
Three pytest suites against a real kind cluster (no shell assertion logic; --confcutdir isolates them from the GPU e2e conftest so only pytest+requests are needed). Host-gateway suites: multi-port registration with ownership labels; batch pod bursts; namespace scoping via a decoy pod; kubectl-annotate port-set edits; kubelet readiness flips keeping workers; gateway restart rebuild; pod recreation rotating the ownership uid; completions fanning out across discovered engines; graceful vs force deletion. mock-worker fleet suites: one pod running forty engines from a single worker-ports annotation; gRPC engines registering with grpc connection mode; PD prefill/decode mix with per-engine aligned bootstrap ports. In-cluster operator journey: smg deployed as a Deployment with in-cluster ServiceAccount auth and the minimal RBAC from in_cluster.yaml (reference manifests), discovering a plain multi-port engine Deployment over the pod network, driven with kubectl scale and rollout restart; a misconfigured annotation falls back to one worker and is diagnosable from gateway logs. Each step records convergence timings into a printed cluster report (rollout capacity floor stayed at 12/12). Mock engines return 404 for unknown paths: the gateway's metadata fetch relies on it to fall back between endpoint variants. The workflow is workflow_dispatch-only with a runner input defaulting to ubuntu-latest until Docker availability on the self-hosted runner pool is confirmed; smg and mock-worker images are built in-job (pr-test-rust publishes no server binary artifact and its runner image is not glibc-matched). Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
6f3b2ec to
88ec5e9
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Expanded per review discussion and validated end-to-end on real clusters (temporary branch trigger, since workflow_dispatch only registers from the default branch; trigger removed and the branch squashed back to one commit). Final validated run: https://github.com/smg-project/smg/actions/runs/31075739665 — 16/16 passed in 3m42s of test time (~20 min wall incl. Rust + image builds). Earlier iterations also caught and fixed a mock-fidelity bug worth noting: the ConfigMap engine answered What this suite now coversHost-gateway lifecycle (9): multi-port registration + ownership labels, batch bursts, namespace scoping (decoy pod in a foreign namespace), mock-worker fleets (3): one pod running forty engines from a single In-cluster operator journey (4): smg deployed as a Deployment with in-cluster ServiceAccount auth and the minimal RBAC in Cluster report from the validated runRegistration converges in ~1s; removals in ~6s (5s drain settle + one pass); a rolling restart of the engine fleet held full capacity throughout (surge pods registered before old ones drained). |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
e2e_test/kind_discovery/conftest.py (1)
78-104: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win🟣 Pre-existing Reject failed or invalid
/workersresponses.Both helpers convert request failures, non-success JSON responses, and missing
workersfields into an empty worker set. This letsKindGateway.wait_for_count(0, ...)pass without confirming forced deletion.
e2e_test/kind_discovery/conftest.py#L78-L104: retry transient failures, but only compare counts after a successful response with a valid worker-list schema.e2e_test/kind_discovery/test_in_cluster_journey.py#L46-L66: apply the same response-status and schema validation so both harnesses have the same contract.As per coding guidelines, “Do not silently fall back to None or a default when configuration validation should fail loudly.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e_test/kind_discovery/conftest.py` around lines 78 - 104, Update KindGateway.workers and wait_for_count in e2e_test/kind_discovery/conftest.py (lines 78-104) to retry transient request failures, require a successful response, and validate that the response contains a worker list before comparing counts; invalid or failed responses must not become an empty set. Apply the same response-status and worker-schema validation in e2e_test/kind_discovery/test_in_cluster_journey.py (lines 46-66) so both harnesses share this contract.Source: Coding guidelines
🧹 Nitpick comments (4)
e2e_test/kind_discovery/test_kind_discovery.py (4)
304-305: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value🟡 Nit:
urlsholds ports, not URLs.The set comprehension extracts the port suffix. Rename the variable to
portsso the assertion reads correctly.♻️ Proposed change
- urls = {w["url"].rsplit(":", 1)[-1] for w in gateway.workers()} - assert urls == {str(p) for p in self.SCALE_PORTS} + ports = {w["url"].rsplit(":", 1)[-1] for w in gateway.workers()} + assert ports == {str(p) for p in self.SCALE_PORTS}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e_test/kind_discovery/test_kind_discovery.py` around lines 304 - 305, Rename the set comprehension variable from urls to ports in the gateway worker assertion, keeping its extracted port values and comparison unchanged.
283-307: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win🔴 Important: The fleet tests depend on
TestKindDiscoveryhaving emptied the cluster.
test_forty_engine_pod_registers_and_unwindsopens withwait_for_count(0, "clean slate before fleet scenarios"). That precondition holds only whentest_graceful_then_force_deleteran first in the same session. If a user selects the fleet tests alone (for example with-k Fleetor-k PD), the baseline is 5 workers and this line times out after 120 seconds with a misleading message.Make the clean slate explicit instead of inherited. Delete the baseline pods in a class-scoped autouse fixture, then wait for zero.
♻️ Proposed change
class TestKindMockWorkerFleet: """Scale and protocol-mix scenarios on the real mock-worker binary.""" SCALE_PORTS = [29000 + i for i in range(40)] + `@pytest.fixture`(autouse=True, scope="class") + def _clean_slate(self, gateway): + subprocess.run( + ["kubectl", "delete", "-f", str(HERE / "manifests.yaml"), + "--ignore-not-found", "--grace-period=0", "--force"], + check=True, capture_output=True, text=True, + ) + gateway.wait_for_count(0, "clean slate before fleet scenarios") + def test_forty_engine_pod_registers_and_unwinds(self, gateway): - gateway.wait_for_count(0, "clean slate before fleet scenarios") apply_manifest(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e_test/kind_discovery/test_kind_discovery.py` around lines 283 - 307, Update TestKindDiscovery with a class-scoped autouse fixture that deletes the baseline pods before fleet scenarios run, then waits for the gateway worker count to reach zero. Ensure test_forty_engine_pod_registers_and_unwinds and other fleet tests no longer rely on test_graceful_then_force_delete ordering, while preserving the existing zero-count verification.
83-104: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value🟡 Nit:
kubectl create namespace decoyis not idempotent.The test never deletes the
decoynamespace. A second run of this test against the same cluster fails withAlreadyExistsbecausekubectlruns withcheck=True. Usekubectl applyfor the namespace, or delete the namespace at the end of the test.♻️ Proposed change
- kubectl("create", "namespace", "decoy") + apply_manifest("apiVersion: v1\nkind: Namespace\nmetadata:\n name: decoy\n")Note:
apply_manifestis defined later in the module. Move it aboveTestKindDiscoverybefore using it here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e_test/kind_discovery/test_kind_discovery.py` around lines 83 - 104, Make namespace setup in test_namespace_scoping_ignores_foreign_pod idempotent by replacing the failing kubectl create operation with the module’s apply-based namespace setup, moving apply_manifest above TestKindDiscovery if needed before use. Preserve the existing decoy pod application and discovery assertions.
309-333: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win🔴 Important: Each fleet test leaks its pod on failure, so later tests cascade.
test_grpc_workers_register_with_grpc_modeassumes the worker count is 0 on entry, andtest_pd_mix_with_aligned_bootstrap_portsassumes the gRPC pod was removed. Each test deletes its pod only on the success path. Ifwait_for_countraises, or if an assertion in the loop at Line 330 fails, the pod stays in the cluster. Every following test then observes an unexpected worker count and fails for an unrelated reason. Add a fixture ortry/finallythat force-deletes the pod for each test.♻️ Proposed change
+@pytest.fixture +def owned_pods(): + names: list[str] = [] + yield names + for name in names: + subprocess.run( + ["kubectl", "delete", "pod", name, "--ignore-not-found", + "--grace-period=0", "--force"], + check=False, capture_output=True, text=True, + )Then register each pod name (
fleet-0,grpc-0,prefill-0,decode-0) inowned_podsright afterapply_manifest, and drop the trailingkubectl deletecalls.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e_test/kind_discovery/test_kind_discovery.py` around lines 309 - 333, Ensure every fleet test, including test_grpc_workers_register_with_grpc_mode and the tests creating fleet-0, prefill-0, and decode-0, cleans up its pod on both success and failure. Register each pod name in the existing owned_pods mechanism immediately after apply_manifest, then remove the trailing success-only kubectl delete calls so fixture teardown force-deletes all owned pods.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/e2e-kind-discovery.yml:
- Around line 52-53: Remove the `|| true` fallback from the `docker run --rm
smg-mock-worker:e2e --help` smoke test so non-zero exits propagate immediately
as workflow failures. Preserve the existing silent output redirection and run
the silent-failure-hunter agent on the changed workflow file.
- Around line 22-27: Declare workflow-level GITHUB_TOKEN permissions for the
kind-discovery workflow by adding contents: read before jobs. Keep the existing
checkout and job configuration unchanged while ensuring repository-controlled
steps receive no write permissions.
- Around line 13-24: Restrict the workflow_dispatch runner input to approved
labels by changing the runner input definition to a choice containing only
trusted values such as ubuntu-latest and k8s-runner-cpu. Update the
kind-discovery job’s runs-on expression to use that validated input while
preserving ubuntu-latest as the default.
In `@e2e_test/kind_discovery/test_in_cluster_journey.py`:
- Around line 159-181: Update the rollout test around the floor tracking and
reporting to assert that the recorded minimum worker count is at least 12 before
post-rollout convergence. Keep the worker-count floor separate from elapsed-time
tracking, and report the floor only after enforcing this assertion; use the
existing rollout flow and symbols such as floor, start, and incluster.report.
In `@e2e_test/kind_discovery/test_kind_discovery.py`:
- Around line 166-202: Update test_completions_route_to_discovered_workers so
non-200 responses during fan-out are retried until successful, using a bounded
deadline and short delay rather than asserting on the first attempt. Continue
collecting served-by values only from successful responses, preserve the content
validation, and retain the final distinct-engine assertion.
---
Duplicate comments:
In `@e2e_test/kind_discovery/conftest.py`:
- Around line 78-104: Update KindGateway.workers and wait_for_count in
e2e_test/kind_discovery/conftest.py (lines 78-104) to retry transient request
failures, require a successful response, and validate that the response contains
a worker list before comparing counts; invalid or failed responses must not
become an empty set. Apply the same response-status and worker-schema validation
in e2e_test/kind_discovery/test_in_cluster_journey.py (lines 46-66) so both
harnesses share this contract.
---
Nitpick comments:
In `@e2e_test/kind_discovery/test_kind_discovery.py`:
- Around line 304-305: Rename the set comprehension variable from urls to ports
in the gateway worker assertion, keeping its extracted port values and
comparison unchanged.
- Around line 283-307: Update TestKindDiscovery with a class-scoped autouse
fixture that deletes the baseline pods before fleet scenarios run, then waits
for the gateway worker count to reach zero. Ensure
test_forty_engine_pod_registers_and_unwinds and other fleet tests no longer rely
on test_graceful_then_force_delete ordering, while preserving the existing
zero-count verification.
- Around line 83-104: Make namespace setup in
test_namespace_scoping_ignores_foreign_pod idempotent by replacing the failing
kubectl create operation with the module’s apply-based namespace setup, moving
apply_manifest above TestKindDiscovery if needed before use. Preserve the
existing decoy pod application and discovery assertions.
- Around line 309-333: Ensure every fleet test, including
test_grpc_workers_register_with_grpc_mode and the tests creating fleet-0,
prefill-0, and decode-0, cleans up its pod on both success and failure. Register
each pod name in the existing owned_pods mechanism immediately after
apply_manifest, then remove the trailing success-only kubectl delete calls so
fixture teardown force-deletes all owned pods.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d341ce11-e3df-4fdd-9764-af1eddff7c07
📒 Files selected for processing (11)
.github/workflows/e2e-kind-discovery.ymle2e_test/kind_discovery/Dockerfile.mock-workere2e_test/kind_discovery/Dockerfile.smge2e_test/kind_discovery/__init__.pye2e_test/kind_discovery/burst.yamle2e_test/kind_discovery/conftest.pye2e_test/kind_discovery/in_cluster.yamle2e_test/kind_discovery/manifests.yamle2e_test/kind_discovery/test_in_cluster_journey.pye2e_test/kind_discovery/test_kind_discovery.pye2e_test/pyproject.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e_test/pyproject.toml
| runner: | ||
| description: Runner label (needs Docker for the kind node container) | ||
| required: false | ||
| default: ubuntu-latest | ||
|
|
||
| env: | ||
| RUSTC_WRAPPER: sccache | ||
| SCCACHE_GHA_ENABLED: "true" | ||
|
|
||
| jobs: | ||
| kind-discovery: | ||
| runs-on: ${{ inputs.runner || 'ubuntu-latest' }} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow file =="
if [ -f .github/workflows/e2e-kind-discovery.yml ]; then
cat -n .github/workflows/e2e-kind-discovery.yml
else
echo "missing .github/workflows/e2e-kind-discovery.yml"
fi
echo
echo "== workflow files mentioning inputs.runner, dispatch, or this workflow =="
rg -n "e2e-kind-discovery|inputs\.runner|workflow_dispatch|workflow_call|runs-on|permissions:|persist-credentials|actions/checkout" .github/workflows || true
echo
echo "== repo-level permissions/default branch refs (if available) =="
git ls-files .github/workflows | sed -n '1,120p'Repository: smg-project/smg
Length of output: 20830
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== setup-rust action =="
if [ -f .github/actions/setup-rust/action.yml ]; then
cat -n .github/actions/setup-rust/action.yml
else
echo "missing .github/actions/setup-rust/action.yml"
# find and print matching action files
fd -a 'action\.(ya?ml|yaml)$' .github/actions | sed -n '1,80p'
fi
echo
echo "== Python context files =="
ls -la e2e_test/kind_discovery 2>/dev/null || true
for f in e2e_test/kind_discovery/conftest.py e2e_test/kind_discovery/test_in_cluster_journey.py e2e_test/kind_discovery/Dockerfile.mock-worker e2e_test/kind_discovery/Dockerfile.smg; do
if [ -f "$f" ]; then
echo "--- $f ---"
sed -n '1,180p' "$f"
fi
doneRepository: smg-project/smg
Length of output: 15543
Security Misconfiguration (CWE-269): Improper Privilege Management
Reachability: External
Restrict inputs.runner to approved labels.
inputs.runner is a free-form manual workflow input and is passed directly into runs-on, so an actor who can dispatch this workflow can choose any available runner label if an ubuntu-latest runner is unavailable. Use a choice input that only includes trusted labels such as ubuntu-latest and k8s-runner-cpu, or keep runs-on fixed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/e2e-kind-discovery.yml around lines 13 - 24, Restrict the
workflow_dispatch runner input to approved labels by changing the runner input
definition to a choice containing only trusted values such as ubuntu-latest and
k8s-runner-cpu. Update the kind-discovery job’s runs-on expression to use that
validated input while preserving ubuntu-latest as the default.
Source: Coding guidelines
| jobs: | ||
| kind-discovery: | ||
| runs-on: ${{ inputs.runner || 'ubuntu-latest' }} | ||
| timeout-minutes: 45 | ||
| steps: | ||
| - uses: actions/checkout@v7 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow file =="
cat -n .github/workflows/e2e-kind-discovery.yml | head -n 220
echo
echo "== workflow permission references in related files =="
rg -n "permissions:|GITHUB_TOKEN|actions/checkout|contents:" .github/workflows .github 2>/dev/null || true
echo
echo "== diff stat/name-only =="
git diff --stat HEAD~1..HEAD 2>/dev/null || git diff --stat 2>/dev/null || true
git diff --name-only HEAD~1..HEAD 2>/dev/null || git diff --name-only 2>/dev/null || true
echo
echo "== parse workflow top-level/job permissions with small python parser =="
python3 - <<'PY'
from pathlib import Path
p = Path('.github/workflows/e2e-kind-discovery.yml')
if not p.exists():
print("missing .github/workflows/e2e-kind-discovery.yml")
raise SystemExit
text = p.read_text()
print("has_top_level_permissions=", any(line.startswith("permissions:") for line in text.splitlines()))
for i,line in enumerate(text.splitlines(), 1):
if line.startswith("permissions:"):
print(f"permission_block_line={i}: {line!r}")
PYRepository: smg-project/smg
Length of output: 30124
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect any referenced workflow step(s) without executing repository code.
echo "== workflow step context =="
sed -n '22,160p' .github/workflows/e2e-kind-discovery.ymlRepository: smg-project/smg
Length of output: 1799
Security Misconfiguration (CWE-732): Incorrect Permission Assignment for Critical Resource
Declare explicit least-privilege GITHUB_TOKEN permissions.
This manual workflow_dispatch workflow runs repository-controlled steps after checkout, including a local GitHub Action and e2e tests. Set permissions: contents: read at the workflow scope so the GITHUB_TOKEN cannot be reused for repository mutations.
🧰 Tools
🪛 zizmor (1.29.0)
[warning] 27-27: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/e2e-kind-discovery.yml around lines 22 - 27, Declare
workflow-level GITHUB_TOKEN permissions for the kind-discovery workflow by
adding contents: read before jobs. Keep the existing checkout and job
configuration unchanged while ensuring repository-controlled steps receive no
write permissions.
Source: Coding guidelines
| # Fail here (not as CrashLoopBackOff later) on any linkage problem. | ||
| docker run --rm smg-mock-worker:e2e --help >/dev/null 2>&1 || true |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🟡 Nit Propagate the mock-worker smoke-test failure.
The comment at Line 52 says this command must fail on linkage problems, but Line 53 ends with || true. This converts every non-zero exit into success and delays the error until the cluster tests. Remove || true.
Proposed failure propagation fix
- docker run --rm smg-mock-worker:e2e --help >/dev/null 2>&1 || true
+ docker run --rm smg-mock-worker:e2e --help >/dev/null 2>&1As per coding guidelines, run the silent-failure-hunter agent on changed files to detect swallowed errors, inappropriate fallbacks, and missing error propagation.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Fail here (not as CrashLoopBackOff later) on any linkage problem. | |
| docker run --rm smg-mock-worker:e2e --help >/dev/null 2>&1 || true | |
| # Fail here (not as CrashLoopBackOff later) on any linkage problem. | |
| docker run --rm smg-mock-worker:e2e --help >/dev/null 2>&1 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/e2e-kind-discovery.yml around lines 52 - 53, Remove the
`|| true` fallback from the `docker run --rm smg-mock-worker:e2e --help` smoke
test so non-zero exits propagate immediately as workflow failures. Preserve the
existing silent output redirection and run the silent-failure-hunter agent on
the changed workflow file.
Source: Coding guidelines
| start = time.monotonic() | ||
| floor = 12 | ||
| kubectl("rollout", "restart", "deployment/engines") | ||
| while True: | ||
| floor = min(floor, len(incluster.workers())) | ||
| status = subprocess.run( | ||
| ["kubectl", "rollout", "status", "deployment/engines", "--timeout=1s"], | ||
| capture_output=True, | ||
| text=True, | ||
| ) | ||
| if status.returncode == 0: | ||
| break | ||
| assert time.monotonic() - start < 300, "rollout never completed" | ||
|
|
||
| incluster.wait_for_count(12, "post-rollout convergence") | ||
| new_ips = engine_pod_ips() | ||
| assert incluster.worker_urls() == expected_urls(new_ips) | ||
| assert not (expected_urls(old_ips - new_ips) & incluster.worker_urls()), ( | ||
| "stale workers survived the rollout" | ||
| ) | ||
| incluster.report.append( | ||
| ("rollout restart capacity floor", float(floor), "min workers during roll (of 12)") | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🔴 Important Assert the rollout capacity floor.
Line 163 records a worker count, but the test only prints floor. A rollout can drop below 12 workers and still pass after final convergence.
Assert floor >= 12. Record elapsed time separately from the worker count.
Proposed fix
incluster.wait_for_count(12, "post-rollout convergence")
new_ips = engine_pod_ips()
assert incluster.worker_urls() == expected_urls(new_ips)
+ assert floor >= 12, f"rollout dropped below required capacity: {floor}/12 workers"
assert not (expected_urls(old_ips - new_ips) & incluster.worker_urls()), (
"stale workers survived the rollout"
)
incluster.report.append(
- ("rollout restart capacity floor", float(floor), "min workers during roll (of 12)")
+ (
+ "rollout restart capacity floor",
+ time.monotonic() - start,
+ f"minimum workers during roll: {floor}/12",
+ )
)As per coding guidelines, “Run the pr-test-analyzer agent to verify that tests adequately cover new or changed functionality.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| start = time.monotonic() | |
| floor = 12 | |
| kubectl("rollout", "restart", "deployment/engines") | |
| while True: | |
| floor = min(floor, len(incluster.workers())) | |
| status = subprocess.run( | |
| ["kubectl", "rollout", "status", "deployment/engines", "--timeout=1s"], | |
| capture_output=True, | |
| text=True, | |
| ) | |
| if status.returncode == 0: | |
| break | |
| assert time.monotonic() - start < 300, "rollout never completed" | |
| incluster.wait_for_count(12, "post-rollout convergence") | |
| new_ips = engine_pod_ips() | |
| assert incluster.worker_urls() == expected_urls(new_ips) | |
| assert not (expected_urls(old_ips - new_ips) & incluster.worker_urls()), ( | |
| "stale workers survived the rollout" | |
| ) | |
| incluster.report.append( | |
| ("rollout restart capacity floor", float(floor), "min workers during roll (of 12)") | |
| ) | |
| start = time.monotonic() | |
| floor = 12 | |
| kubectl("rollout", "restart", "deployment/engines") | |
| while True: | |
| floor = min(floor, len(incluster.workers())) | |
| status = subprocess.run( | |
| ["kubectl", "rollout", "status", "deployment/engines", "--timeout=1s"], | |
| capture_output=True, | |
| text=True, | |
| ) | |
| if status.returncode == 0: | |
| break | |
| assert time.monotonic() - start < 300, "rollout never completed" | |
| incluster.wait_for_count(12, "post-rollout convergence") | |
| new_ips = engine_pod_ips() | |
| assert incluster.worker_urls() == expected_urls(new_ips) | |
| assert floor >= 12, f"rollout dropped below required capacity: {floor}/12 workers" | |
| assert not (expected_urls(old_ips - new_ips) & incluster.worker_urls()), ( | |
| "stale workers survived the rollout" | |
| ) | |
| incluster.report.append( | |
| ( | |
| "rollout restart capacity floor", | |
| time.monotonic() - start, | |
| f"minimum workers during roll: {floor}/12", | |
| ) | |
| ) |
🧰 Tools
🪛 ast-grep (0.45.0)
[error] 163-167: Command coming from incoming request
Context: subprocess.run(
["kubectl", "rollout", "status", "deployment/engines", "--timeout=1s"],
capture_output=True,
text=True,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@e2e_test/kind_discovery/test_in_cluster_journey.py` around lines 159 - 181,
Update the rollout test around the floor tracking and reporting to assert that
the recorded minimum worker count is at least 12 before post-rollout
convergence. Keep the worker-count floor separate from elapsed-time tracking,
and report the floor only after enforcing this assertion; use the existing
rollout flow and symbols such as floor, start, and incluster.report.
Source: Coding guidelines
| def test_completions_route_to_discovered_workers(self, gateway): | ||
| import requests | ||
|
|
||
| gateway.wait_for_count(5, "baseline before data-plane check") | ||
| # Health promotion lags registration; wait for the first 200 before | ||
| # asserting fan-out. | ||
| deadline = time.monotonic() + 60 | ||
| while True: | ||
| probe = requests.post( | ||
| f"{gateway.base_url}/v1/chat/completions", | ||
| json={ | ||
| "model": "kind-e2e-model", | ||
| "messages": [{"role": "user", "content": "warmup"}], | ||
| }, | ||
| timeout=10, | ||
| ) | ||
| if probe.status_code == 200: | ||
| break | ||
| assert time.monotonic() < deadline, f"no worker became routable: {probe.text}" | ||
| time.sleep(1) | ||
| served_by: set[str] = set() | ||
| for i in range(10): | ||
| response = requests.post( | ||
| f"{gateway.base_url}/v1/chat/completions", | ||
| json={ | ||
| "model": "kind-e2e-model", | ||
| "messages": [{"role": "user", "content": f"probe {i}"}], | ||
| }, | ||
| timeout=10, | ||
| ) | ||
| assert response.status_code == 200, response.text | ||
| content = response.json()["choices"][0]["message"]["content"] | ||
| assert content.startswith("served-by-"), content | ||
| served_by.add(content) | ||
| # Round-robin over the discovered fleet: several distinct engines | ||
| # (pod ports) must actually serve traffic. | ||
| assert len(served_by) >= 2, f"traffic pinned to one engine: {served_by}" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🔴 Important: The fan-out loop can flake while remaining engines are still being health-promoted.
The warmup loop only proves that one worker became routable. The following 10 requests then assert status_code == 200 on every attempt. If a second engine is registered but not yet health-promoted, the gateway can route to it and return a non-200 response. The test then fails for a timing reason, not a routing defect. Retry non-200 responses inside the fan-out loop instead of asserting on the first attempt.
🐛 Proposed fix
served_by: set[str] = set()
- for i in range(10):
- response = requests.post(
- f"{gateway.base_url}/v1/chat/completions",
- json={
- "model": "kind-e2e-model",
- "messages": [{"role": "user", "content": f"probe {i}"}],
- },
- timeout=10,
- )
- assert response.status_code == 200, response.text
- content = response.json()["choices"][0]["message"]["content"]
- assert content.startswith("served-by-"), content
- served_by.add(content)
+ deadline = time.monotonic() + 60
+ attempts = 0
+ while len(served_by) < 2 and time.monotonic() < deadline:
+ response = requests.post(
+ f"{gateway.base_url}/v1/chat/completions",
+ json={
+ "model": "kind-e2e-model",
+ "messages": [{"role": "user", "content": f"probe {attempts}"}],
+ },
+ timeout=10,
+ )
+ attempts += 1
+ if response.status_code != 200:
+ time.sleep(1)
+ continue
+ content = response.json()["choices"][0]["message"]["content"]
+ assert content.startswith("served-by-"), content
+ served_by.add(content)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_completions_route_to_discovered_workers(self, gateway): | |
| import requests | |
| gateway.wait_for_count(5, "baseline before data-plane check") | |
| # Health promotion lags registration; wait for the first 200 before | |
| # asserting fan-out. | |
| deadline = time.monotonic() + 60 | |
| while True: | |
| probe = requests.post( | |
| f"{gateway.base_url}/v1/chat/completions", | |
| json={ | |
| "model": "kind-e2e-model", | |
| "messages": [{"role": "user", "content": "warmup"}], | |
| }, | |
| timeout=10, | |
| ) | |
| if probe.status_code == 200: | |
| break | |
| assert time.monotonic() < deadline, f"no worker became routable: {probe.text}" | |
| time.sleep(1) | |
| served_by: set[str] = set() | |
| for i in range(10): | |
| response = requests.post( | |
| f"{gateway.base_url}/v1/chat/completions", | |
| json={ | |
| "model": "kind-e2e-model", | |
| "messages": [{"role": "user", "content": f"probe {i}"}], | |
| }, | |
| timeout=10, | |
| ) | |
| assert response.status_code == 200, response.text | |
| content = response.json()["choices"][0]["message"]["content"] | |
| assert content.startswith("served-by-"), content | |
| served_by.add(content) | |
| # Round-robin over the discovered fleet: several distinct engines | |
| # (pod ports) must actually serve traffic. | |
| assert len(served_by) >= 2, f"traffic pinned to one engine: {served_by}" | |
| def test_completions_route_to_discovered_workers(self, gateway): | |
| import requests | |
| gateway.wait_for_count(5, "baseline before data-plane check") | |
| # Health promotion lags registration; wait for the first 200 before | |
| # asserting fan-out. | |
| deadline = time.monotonic() + 60 | |
| while True: | |
| probe = requests.post( | |
| f"{gateway.base_url}/v1/chat/completions", | |
| json={ | |
| "model": "kind-e2e-model", | |
| "messages": [{"role": "user", "content": "warmup"}], | |
| }, | |
| timeout=10, | |
| ) | |
| if probe.status_code == 200: | |
| break | |
| assert time.monotonic() < deadline, f"no worker became routable: {probe.text}" | |
| time.sleep(1) | |
| served_by: set[str] = set() | |
| deadline = time.monotonic() + 60 | |
| attempts = 0 | |
| while len(served_by) < 2 and time.monotonic() < deadline: | |
| response = requests.post( | |
| f"{gateway.base_url}/v1/chat/completions", | |
| json={ | |
| "model": "kind-e2e-model", | |
| "messages": [{"role": "user", "content": f"probe {attempts}"}], | |
| }, | |
| timeout=10, | |
| ) | |
| attempts += 1 | |
| if response.status_code != 200: | |
| time.sleep(1) | |
| continue | |
| content = response.json()["choices"][0]["message"]["content"] | |
| assert content.startswith("served-by-"), content | |
| served_by.add(content) | |
| # Round-robin over the discovered fleet: several distinct engines | |
| # (pod ports) must actually serve traffic. | |
| assert len(served_by) >= 2, f"traffic pinned to one engine: {served_by}" |
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 173-180: Request-controlled URL passed to requests; validate against an allowlist to prevent SSRF.
Context: requests.post(
f"{gateway.base_url}/v1/chat/completions",
json={
"model": "kind-e2e-model",
"messages": [{"role": "user", "content": "warmup"}],
},
timeout=10,
)
Note: [CWE-918] Server-Side Request Forgery (SSRF).
(ssrf-requests)
[warning] 187-194: Request-controlled URL passed to requests; validate against an allowlist to prevent SSRF.
Context: requests.post(
f"{gateway.base_url}/v1/chat/completions",
json={
"model": "kind-e2e-model",
"messages": [{"role": "user", "content": f"probe {i}"}],
},
timeout=10,
)
Note: [CWE-918] Server-Side Request Forgery (SSRF).
(ssrf-requests)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@e2e_test/kind_discovery/test_kind_discovery.py` around lines 166 - 202,
Update test_completions_route_to_discovered_workers so non-200 responses during
fan-out are retried until successful, using a bounded deadline and short delay
rather than asserting on the first attempt. Continue collecting served-by values
only from successful responses, preserve the content validation, and retain the
final distinct-engine assertion.
1 participant
Stacked on #2062 (which stacks on #2061).
Description
Real-cluster coverage for the layers the fake-API-server tests can't reach: RBAC/kubeconfig auth, real watch bookmarks and relists, kubelet readiness transitions, and actual graceful/force deletion semantics.
e2e_test/kind_discovery/manifests.yaml— two hostNetwork pods on a kind node:multi-engine-0runs four HTTP engine servers from one container (the exact multiple-servers-per-pod deployment shape) listed insmg.ai/worker-ports: "28080,28081,28082,28083";single-engine-0has no annotation and exercises the--service-discovery-portfallback. The mock engine is a stdlib-only python script shipped via ConfigMap — no image builds.e2e_test/kind_discovery/run.sh— creates the cluster, waits for readiness, starts thesmgbinary on the runner host against the kind kubeconfig, then asserts viaGET /workers: 5 workers (4 + 1 fallback), all carryingsmg.ai/pod-uid, exactly 4 labeledsmg.ai/pod-name=multi-engine-0; gracefulkubectl delete→ 4 workers;--grace-period=0 --force→ 0. Dumps the smg log tail and pod state on failure; always tears the cluster down..github/workflows/e2e-kind-discovery.yml— workflow_dispatch only with arunnerinput defaulting toubuntu-latest(GitHub-hosted has Docker, so this runs today); flip the input to the self-hosted pool once dind availability there is confirmed. Not a merge gate.hostNetwork is what makes the host-side smg able to probe worker ports at the pod IP (= node container IP, routable on Linux runners) — and it also mirrors the real multi-engine GPU-pod deployment pattern.
Validation
Runtime validation needs a Linux Docker host, so this is dispatch-validated on CI rather than locally (local Docker daemon unavailable, and macOS cannot route to kind node IPs regardless). Locally verified:
bash -non the script, workflow + manifest YAML parse, and the embedded multi-port engine server executed standalone — all four ports served/healthand/get_model_infocorrectly. The full discovery pipeline itself is already covered end-to-end by the deterministic fake-API-server tests in #2062; this PR adds the real-cluster plumbing on top.Suggested first run:
gh workflow run e2e-kind-discovery.ymlonce this merges (or dispatch from this branch).