feat: live AKS contract test environment + fix silently swallowed HTTP errors (#109)

[hellices]

Closes #109

What

A live-cluster contract test suite (tests/contract/, 21 tests) that proves the k8s layer against a real AKS API server, plus the post-merge workflow that runs it — and a real client bug the suite caught on its first run.

The bug the suite caught (first commit)

kubernetes_asyncio's rest layer only raises ApiException when it preloads the response body. Every raw (_preload_content=False) call — all writes, dry-run previews, get_object, list_namespaces, SSAR, log streams — received error responses as if they had succeeded:

• a refused write (409 uid precondition, 429 PDB denial) silently looked like a success
• a 404 GET returned the error Status JSON as if it were the object

Unit tests never saw it because their fakes raise ApiException directly. Fixed with _raise_for_status at every raw-response consumption point; RED unit tests added first (tests/k8s/test_client.py::*non_2xx*), existing fakes now carry a real HTTP status.

Contract suite (second commit)

• Gated on KORVID_CONTRACT_RUN_ID — without it every test skips at collection, so the fast PR gate is untouched.
• Contract pattern: preview → read back from the API server → prove zero persistent mutation (uid/resourceVersion/generation unchanged); execute → prove exactly-once (one generation bump, 404/409 on replay).
• Coverage: delete, create/replace, scale, rollout-restart (exact previewed stamp), in-place pod resize (1.35 pods/resize), helm install/upgrade/rollback/uninstall incl. dry-runs (state read from release Secrets, not helm output), cordon/uncordon/drain-plan/eviction-vs-PDB (disposable workload node only — tests fail rather than touch a system node), TokenRequest-based RBAC allow/403/SSAR, discovery + watch.
• tests/contract/janitor.py: label-keyed idempotent cleanup of stale korvid-contract-* namespaces and leftover cordons.

Workflow

.github/workflows/k8s-contract.yml: push to main / protected manual dispatch only, aks-contract-test environment (main-only branch policy), OIDC federation — no stored kubeconfig or secret. Starts the stopped cluster, runs janitor then the suite, uploads diagnostics on failure, and always stops the cluster, emitting an actionable alert if the stop fails.

Infrastructure is provisioned as code kept outside the repo; the provisioning record (Bicep + deployment details) is on #109.

Validation

• Full live run against aks-korvid-contract-test: 21/21 passed (after the client fix; before it, 9 failed — evidence the contract model works).
• make check green: 2791 passed, ruff/mypy/tach clean.
• Cluster stopped after validation.

[Copilot]

Pull request overview

Adds live AKS contract testing and fixes raw Kubernetes HTTP errors being silently accepted.

Changes:

• Raises ApiStatusError for non-2xx raw responses.
• Adds 21 live-cluster contract tests and cleanup tooling.
• Adds a protected post-merge AKS workflow and documentation.

Reviewed changes

Copilot reviewed 20 out of 20 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
.github/workflows/k8s-contract.yml	Runs the AKS contract suite.
src/korvid/k8s/client.py	Validates raw HTTP statuses.
tests/k8s/test_client.py	Tests non-2xx handling.
tests/k8s/test_logs.py	Updates log response fakes.
tests/k8s/test_client_resize.py	Updates resize response fakes.
tests/k8s/test_client_previews.py	Updates preview response fakes.
tests/k8s/test_client_node_ops.py	Updates node-operation fakes.
tests/contract/__init__.py	Documents the contract package.
tests/contract/conftest.py	Provides gating, fixtures, and polling.
tests/contract/janitor.py	Cleans stale cluster state.
tests/contract/test_create_replace_contract.py	Tests create, replace, discovery, and watch.
tests/contract/test_delete_contract.py	Tests delete preview and execution.
tests/contract/test_helm_contract.py	Tests Helm lifecycle operations.
tests/contract/test_node_ops_contract.py	Tests disposable-node operations.
tests/contract/test_rbac_contract.py	Tests short-lived RBAC identities.
tests/contract/test_resize_contract.py	Tests in-place pod resize.
tests/contract/test_scale_rollout_contract.py	Tests scale and rollout restart.
pyproject.toml	Registers the contract marker.
docs/dev/README.md	Links contract documentation.
docs/dev/contract-tests.md	Documents infrastructure and usage.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[my-reviewer-agent]

라이브 AKS contract 테스트 스위트 + raw-path HTTP 에러 무시 버그 수정 리뷰입니다.

핵심 버그 수정 검증: kubernetes_asyncio는 _preload_content=False일 때 non-2xx에서 ApiException을 던지지 않는다는 진단이 정확하고, _raise_for_status가 raw 응답 소비 지점 4곳(_request_write, _request_json, _to_dict, stream_logs)에 모두 배치되었습니다. RED 단위 테스트 5종(409 delete/create, 404 get, 403 list, 400 logs)이 회귀를 고정하고, 기존 fake들에 status=200이 일관되게 추가되어 있습니다.

Contract 스위트: preview→read-back→무변이 증명 / execute→정확히-1회(generation bump, 404/409 replay) 패턴이 delete/create/replace/scale/rollout/resize/helm/node-ops/RBAC 전반에 일관 적용됨. KORVID_CONTRACT_RUN_ID 게이트로 PR 게이트 무영향, 라벨 키 기반 janitor의 멱등 정리, disposable 노드 강제(시스템 노드 폴백 대신 fail) 모두 확인.

워크플로 안전장치: 클러스터 identity 이중 가드(이름 + purpose/production-use 태그), OIDC 페더레이션(저장 자격증명 없음), 별도 stop-cluster job으로 contract job timeout 시에도 정지 보장, 정지 실패 시 actionable alert — 잘 설계되었습니다.

인라인은 사소한 Suggestion 2건뿐입니다. APPROVE.

[Copilot]

Pull request overview

Copilot reviewed 20 out of 20 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (4)

.github/workflows/k8s-contract.yml:126

• Even if the collection step is made cancellation-aware, this condition still skips artifact upload when the run is cancelled. Include cancelled() so cancellation diagnostics are preserved.

        if: failure()

.github/workflows/k8s-contract.yml:117

• failure() is false when the job is cancelled, so diagnostics are not collected on cancellation even though issue #109 requires collection after cancellation as well as test failure. Include cancelled() in this condition.

This issue also appears on line 126 of the same file.

        if: failure()

src/korvid/k8s/client.py:1431

• The new status check still does not cover raw watch requests: _make_raw_watch_callable returns the _preload_content=False response directly. In kubernetes_asyncio 36.1, Watch never inspects resp.status; an empty non-2xx body is retried indefinitely and a non-JSON error body reaches callers as malformed event data instead of ApiStatusError. Check the response status in the watch callable before returning it and add a non-2xx watch test.

def _raise_for_status(resp: Any, body: bytes) -> None:
    """Raise ApiStatusError for a non-2xx raw (``_preload_content=False``)
    response. kubernetes_asyncio's rest layer only raises ApiException when
    it preloads the body, so raw-path callers must check the status
    themselves — otherwise refused writes (409 uid precondition, 429 PDB

.github/workflows/k8s-contract.yml:123

• The failure-handling requirements explicitly call for pod descriptions, but this bundle only captures the pod list. Add kubectl describe pods -A output so scheduling, probe, mount, and container-state failures remain diagnosable after cleanup.

          kubectl get events -A --sort-by=.lastTimestamp > diag/events.txt || true
          kubectl get pods -A -o wide > diag/pods.txt || true
          kubectl get nodes -o wide > diag/nodes.txt || true
          kubectl get ns -l app.kubernetes.io/managed-by=korvid-contract -o yaml > diag/namespaces.yaml || true

[Copilot]

Pull request overview

Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

tests/contract/test_helm_contract.py:26

• Local run IDs are explicitly allowed to contain _ or ., but this value is used directly as a Helm release name; for example, the documented local-$USER command fails Helm's release-name validation when the username contains an underscore. Normalize RUN_ID to lowercase DNS characters here just as the namespace fixture does.

RELEASE = f"korvid-contract-{RUN_ID}"[:53].rstrip("-").lower()

.github/workflows/k8s-contract.yml:143

• The timeout rationale also applies to diagnostics and fixture cleanup: when the contract job reaches its timeout, GitHub kills its later diagnostics, upload, kubeconfig deletion, and pytest teardown, while this recovery job only stops AKS. That leaves no failure artifact and leaves namespaces/cordons until a future run, contrary to issue #109's requirement that diagnostics and cleanup run after cancellation. Have the recovery job obtain short-lived Kubernetes credentials while the cluster is still running, collect/upload diagnostics and run the janitor when needs.contract.result is not successful, then stop the cluster.

  # Separate job so the stop runs even when the contract job hits its
  # timeout-minutes (GitHub kills the job before later always() steps,
  # which would leave the stopped-at-rest cluster running and billing).

[hellices]

Round 3 (suppressed low-confidence findings) disposition:

1. Helm release name vs raw RUN_ID — fixed in 9228b5c. The DNS-1123 normalization moved to conftest.SAFE_RUN_ID, shared by the namespace fixture and the helm RELEASE name, so local-$USER run ids with _/. work everywhere.

2. Recovery job should also collect diagnostics + run the janitor — declined as designed-for:

   • Leftover fixtures from an interrupted run are the pre-run janitor's job: it sweeps app.kubernetes.io/managed-by=korvid-contract namespaces and uncordons labeled nodes at the start of every run (and can be run manually). Cleanup does not depend on the dying job.
   • Diagnostics loss at job timeout is already mitigated by the timeout layering: the pytest step has its own timeout-minutes: 30 inside a 45-minute job, leaving 15 minutes of headroom for the failure() || cancelled() diagnostics/upload steps to run — the job-level kill only triggers if diagnostics themselves hang.
   • Giving the stop job cluster credentials would widen it from a pure control-plane action (az aks stop) to data-plane access, which we prefer to keep out of the always()-recovery path.

[Copilot]

Pull request overview

Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (5)

tests/contract/test_resize_contract.py:47

• This execute contract is also skipped based on the implementation under test. A false-negative capability result therefore removes all live execute coverage for resize while the workflow remains green; assert the dedicated cluster contract instead.

    if not await client.supports_pod_resize():
        pytest.skip("cluster does not expose the pods/resize subresource")

tests/contract/test_node_ops_contract.py:115

• This assertion accepts every HTTP 429, so API Priority and Fairness throttling would make the test pass even if the PDB did not block the eviction. The client now preserves the response body specifically to distinguish those cases; assert that this 429 identifies a disruption-budget denial.

    with pytest.raises(ApiStatusError, match="429"):
        await client.evict_pod(namespace, "evict-subject", uid=None)

tests/contract/janitor.py:36

• The managed-by label identifies ownership, not staleness. Every active local contract run uses this same label, while the workflow concurrency group serializes only GitHub runs, so this startup sweep can delete another currently running local suite. Add cluster-wide serialization (for example, a Lease) or a reliable stale-age/active-run check before deleting; the cordon sweep needs the same protection.

    for summary in await client.list_objects(NAMESPACE, None):
        if dict(summary.labels).get(MANAGED_BY_LABEL) != MANAGED_BY_VALUE:
            continue

tests/contract/test_create_replace_contract.py:91

• Cancelling a task only requests cancellation; it does not wait for the task to finish. If the watch times out or exits with an error while create_object is still pending, the creator can race namespace teardown or survive until the event loop closes. Await the cancelled task so cleanup is deterministic.

    finally:
        creator.cancel()

tests/contract/test_resize_contract.py:26

• This uses the product's own capability probe to skip the contract that should validate it. If discovery regresses and incorrectly reports no pods/resize, both resize contracts disappear from an otherwise green run. Because the dedicated AKS target is expected to expose this subresource, fail explicitly instead of skipping.

This issue also appears on line 46 of the same file.

    if not await client.supports_pod_resize():
        pytest.skip("cluster does not expose the pods/resize subresource")

[my-reviewer-agent]

재리뷰 (신규 커밋 2건, d549552 → 9228b5c) — 승인합니다.

검토한 델타:

1. raw watch 경로 non-2xx 검사 (client.py::_make_raw_watch_callable): kubernetes_asyncio의 Watch가 resp.status를 전혀 확인하지 않아 non-2xx watch 응답이 무한 재시도되거나 malformed event로 파싱되던 문제를 Watch에 넘기기 전에 _raise_for_status로 차단. try/finally로 resp.close() 보장, getattr(resp, "status", 0) or 0 fail-closed 처리 모두 적절합니다. RED 테스트(test_raw_watch_callable_raises_on_non_2xx_raw_response)가 403+Status body로 실제 경로를 검증하고, 기존 watch 테스트들의 mock도 status를 갖도록 일관되게 갱신됨.
2. test_stream_logs_raises_on_non_2xx_raw_response에 resp.close.assert_called_once() 추가 — 에러 경로에서의 연결 누수 회귀 방지, 이전 라운드 코멘트 해소.
3. SAFE_RUN_ID DNS-1123 정규화 (conftest.py): re.sub(r"[^a-z0-9-]", "-", RUN_ID.lower())를 namespace fixture와 helm RELEASE 이름이 공유 — 로컬 run id(local-$USER)의 ./_로 인한 422/helm 이름 검증 실패를 한 곳에서 해결. 중복 정규화 로직 제거도 깔끔합니다.
4. 워크플로 진단: failure() || cancelled() + kubectl describe pods -A 추가 — 타임아웃 취소 시에도 아티팩트 확보, 합리적.

지적 사항 없음. APPROVE

[hellices]

Round 4 (suppressed low-confidence findings — second consecutive advisory-only round, review limit reached) disposition:

Fixed in 0c8ba2a (validated live, 4/4 pass):

• resize contracts now assert supports_pod_resize() instead of skipping — a negative probe on the dedicated k8s ≥ 1.33 cluster is a discovery regression, and skipping silently dropped all live resize coverage.
• the blocked-eviction 429 asserts is_pdb_denial(), so APF throttling can't masquerade as a PDB denial.
• the watch test awaits its cancelled creator task (contextlib.suppress(CancelledError)) so cleanup is deterministic.

Declined — janitor startup sweep vs concurrent local runs: accepted as a documented trade-off. The cluster is single-purpose and effectively single-operator; GitHub runs are serialized by the concurrency group, and concurrent local+CI runs are already unsupported (they'd contend for the same cordonable node and PDB targets regardless of the janitor). A cluster-wide Lease would add coordination machinery to protect a scenario the environment doesn't support. The janitor only touches app.kubernetes.io/managed-by=korvid-contract objects, so the blast radius of a mid-run sweep is another contract run, never real workloads.