Skip to content

feat(modelharness): support browser CORS - #220

Open
tnsimon wants to merge 3 commits into
kaito-project:mainfrom
tnsimon:add-cors-preflight
Open

tnsimon wants to merge 3 commits into
kaito-project:mainfrom
tnsimon:add-cors-preflight

Conversation

@tnsimon

@tnsimon tnsimon commented Sep 17, 2026

Copy link
Copy Markdown
Collaborator

Summary

Add opt-in browser CORS support to the modelharness Gateway while preserving the existing authentication boundary.

The Gateway now:

  • answers valid /v1 browser preflights locally before authentication;
  • validates requested origins, methods, and headers;
  • supports exact origins and non-credentialed wildcard origins;
  • adds CORS headers to upstream responses, authentication failures, and direct/local Envoy responses;
  • keeps every actual model request behind API-key or Entra authentication.

CORS remains disabled by default.

Configuration

Exact-origin mode:

cors:
  enabled: true
  allowedOrigins:
    - https://app.example.com
    - http://localhost:3000

Exact-origin mode reflects only configured origins, supports credentials, and emits Vary: Origin.

Wildcard mode:

cors:
  enabled: true
  allowedOrigins:
    - "*"
  allowCredentials: false

Wildcard mode requires * to be the sole origin and credentials to be explicitly disabled. It emits Access-Control-Allow-Origin: *, never emits Access-Control-Allow-Credentials, accepts every non-empty incoming origin (including Origin: null), and omits Origin from Vary.

Partial wildcards, configured literal null, and wildcard/exact-origin combinations are rejected.

Behavior

  • Complete, allowed preflights receive a local 204.
  • Disallowed preflight origins, methods, or headers receive a local 403.
  • Origin-bearing actual requests with disallowed origins or methods receive a local 403.
  • Bare or incomplete OPTIONS requests are not preflights and remain subject to authentication.
  • Requests without Origin, and paths outside /v1, are unchanged.
  • Actual browser requests continue through authentication and model routing.
  • CORS headers are added to successful upstream responses, authentication failures, and unknown-model direct 404 responses.

OPTIONS is intentionally absent from cors.allowedMethods: that setting describes the eventual application method in Access-Control-Request-Method (such as GET or POST), not the preflight transport method.

Implementation

  • Add a namespace-scoped gateway-cors EnvoyFilter with kaito.cors inserted first in the Gateway HTTP filter chain.
  • Use per-stream metadata to decorate actual responses, including local replies.
  • Add Helm schema, template, and Go validation for exact and wildcard configurations.
  • Replace the complete origins list during Helm reconciliation using --set-json.
  • Add ordered exact-to-wildcard E2E reconciliation on the same Gateway.
  • Document configuration, wildcard restrictions, and the authentication boundary.

The Lua filter is used instead of native Envoy CORS because BBR recomputes routes and the initial inference route may be a direct_response; native Envoy CORS explicitly bypasses direct-response routes.

Validation

Completed local verification:

git diff --check
jq empty charts/modelharness/values.schema.json
E2E_PROVIDER=upstream go test -race -count=1 ./test/e2e/deploy/... ./test/e2e/utils/...
E2E_PROVIDER=upstream go test -run '^' ./test/e2e/...
E2E_PROVIDER=upstream go vet ./test/e2e/...
make test
make lint
make verify-boilerplate

Chart verification covered default, exact-origin, wildcard, and invalid configurations; schema-bypassed template validation; YAML parsing; and istioctl validate.

Live Gateway E2E validation passed:

24 Passed | 0 Failed

The live tests covered preflight 204, local denial 403, authentication failure 401, upstream 200, and direct/local unknown-model 404 response paths.

@zhuangqh

Copy link
Copy Markdown
Contributor

The change is high quality and closely matches surrounding idioms (extensive doc comments, table-driven tests, Ginkgo labels, --set-json for arrays). No hard idiom violations. All findings are
judgement-call smells.

  • test/e2e/apikey_auth_spec.go — Duplicated Code (strongest finding). The two Context blocks ("Browser CORS" and "Wildcard browser CORS") each redeclare a byte-identical sendPreflight closure
    (WithoutAuth + Origin + Access-Control-Request-Method, optional Access-Control-Request-Headers, then SendGatewayRequest(... http.MethodOptions ...)). Both also carry near-identical
    expectActualCORSHeaders closures (differ only in exact vs wildcard expectations) and repeat the same "bare OPTIONS" / "incomplete preflight" It blocks verbatim. → Extract a package-level
    sendPreflight helper and fold the two bypass tests into a shared table.
  • envoyfilter-cors.yaml (Lua) — Duplicated Code (judgement call). add_vary and remove_vary each re-implement the "split on comma, trim, case-fold, compare token" loop that also appears in
    requested_headers_are_allowed (and in the Go test's commaSeparatedHeaderTokens). A small each_vary_token iterator would remove the triplication. Minor — the file is self-contained.
  • Cross-boundary duplicated validation — Shotgun Surgery risk (judgement call). The origin regex, the :80/:443 default-port rejection, the wildcard-sole-origin rule, and duplicate-detection all exist
    three times: Helm fail guards, values.schema.json, and Go deploy/values.go. The regex is copied character-for-character. Changing the accepted origin grammar forces edits in three files with no
    shared source. Inherent to the Helm/Go split, but worth a cross-linking comment so the coupling is discoverable.
  • Naming (judgement call). metadata.name: gateway-cors vs internal filter name kaito.cors is a mild inconsistency — not blocking.
  • helm.go has one over-long wrapped comment line — cosmetic, gofmt doesn't touch prose.

Lua correctness idioms were checked and are sound: envoy_on_response re-validates the metadata origin against allowed_origins (defends against stale/forged dynamic metadata), per-stream metadata is
used instead of a Lua global, and trim/is_api_path handle nil and query strings. No Speculative Generality — ReconcileModelHarnessCORS and the *bool AllowCredentials (nil = inherit) are both
exercised by the wildcard test path.

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.

2 participants