RHOAIENG-54696: Support Batch /v1/files and /v1/batches#709
RHOAIENG-54696: Support Batch /v1/files and /v1/batches#709openshift-merge-bot[bot] merged 11 commits intoopendatahub-io:incubatingfrom
/v1/files and /v1/batches#709Conversation
|
Skipping CI for Draft Pull Request. |
📝 WalkthroughWalkthroughThis PR adds controller E2E test infrastructure and batch-path authorization changes: a new Makefile phony target Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Key Issues
Only the issues above are flagged; each includes an actionable mitigation. 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
Replace invalid has() macro with 'key' in map operator for checking request headers in Authorino CEL expressions. The has() macro does not work with bracket-notation map access in Authorino's CEL implementation. Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
Update CEL expressions to match the template fix, add gateway.yaml reference with allowedRoutes documentation, add apiVersion/kind/metadata to the rendered AuthPolicy example, and add -vk flags to curl commands. Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
Add e2e tests verifying the AuthPolicy handles batch and inference paths correctly: authn-only for batch paths, standard SAR for inference, delegated SAR with x-maas-user header, and rejection of spoofed/unauthorized headers. Tests use port-forwarding for cluster-internal gateway access and include a Makefile target (test-e2e-controller) integrated into the CI pipeline. Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
After creating an HTTPRoute, poll its status.parents for the Kuadrant policy controller's kuadrant.io/AuthPolicyAffected condition to become True. This ensures the ext-authz filter is active before tests run, preventing false positives from unenforced authentication. Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
Add tests for /v1/files batch path (authn-only and spoofed header), unauthenticated requests (expect 401), and inference path with no RBAC (expect 403). Remove response body from all error messages to prevent leaking cluster internals in CI logs. Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
| expression: request.path.split("/")[2] | ||
| verb: | ||
| value: get | ||
| expression: "'x-maas-user' in request.headers ? 'post' : 'get'" |
There was a problem hiding this comment.
maybe we should use post-delegate to be extra explicit?
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/controller/resources/template/authpolicy_llm_isvc_userdefined.yaml`:
- Around line 32-45: The current policy allows untrusted headers to set the SAR
principal by using x-maas-user and falling back to auth.identity.* (notably in
resourceAttributes.name/resource/verb and authorizationGroups), causing
impersonation; change the logic so that the headers x-maas-user and
x-maas-groups are accepted only when the caller is the trusted batch-processor
ServiceAccount (check auth.identity.serviceAccountName or equivalent) and
require both headers to be present as an all-or-nothing pair; otherwise always
use auth.identity.user.username and auth.identity.user.groups (do not mix header
values with auth.identity.*), and ensure the resourceAttributes.resource and
verb expressions only switch to the delegated values when the caller is the
trusted service account and both headers are supplied.
- Around line 28-29: The gateway currently skips auth for paths matching the
when predicate in the rule that exempts "/v1/files" and "/v1/batches", but does
not reject or strip client-supplied tenant headers (e.g., x-maas-user,
x-maas-groups), enabling tenant spoofing; update the policy so that for the
predicate "!(request.path.startsWith('/v1/files') ||
request.path.startsWith('/v1/batches'))" (and the corresponding batch-path
handling where tenant identity is injected around the logic at lines 60-70) you
either: 1) explicitly reject requests that contain any x-maas-* headers when
request.path startsWith('/v1/files') or '/v1/batches', or 2) strip those headers
before any injection logic runs, ensuring the injection code (the tenant
identity injection referenced in the block at lines 60-70) is the only source of
truth for x-maas-* values.
In `@internal/controller/resources/template/BATCH.md`:
- Around line 52-58: The YAML examples use the wrong header key
`x-maas-username`; update those examples to `x-maas-user` so they match the
actual header injected by the template and validated by the e2e tests.
Specifically replace the `x-maas-username` mapping block with `x-maas-user`
(keeping the same when/predicate and plain/expression content) for both
occurrences reported so copied snippets configure the correct tenant propagation
header.
In `@internal/controller/test/e2e/batch_authpolicy_test_env.go`:
- Around line 113-120: Currently the listener protocol is dropped by
gatewayListenerPort(); change this by creating a helper (e.g.,
gatewayListenerSchemeAndPort or gatewayListenerURLParts) that returns both
scheme ("http" or "https") and port and have the code that builds requests use
that scheme instead of hardcoding "http://"; validate and reject non-HTTP
listeners (e.g., TCP) before attempting requests. Replace the TLS
InsecureSkipVerify usage in the http.Client Transport with loading the gateway
CA into tls.Config.RootCAs so HTTPS listeners are verified. Finally, replace any
io.ReadAll(response.Body) calls with io.ReadAll(io.LimitReader(response.Body,
<MAX_BYTES>)) (choose a sensible MAX_BYTES) to bound response reads and avoid
unbounded memory use.
- Around line 674-676: The test reads the gateway response unsafely with
io.ReadAll(resp.Body), which can OOM on large responses; fix by wrapping
resp.Body with io.LimitReader using a reasonable cap (e.g. const
maxResponseBodySize) and call io.ReadAll(io.LimitReader(resp.Body,
maxResponseBodySize)) in the code that currently uses io.ReadAll (reference
resp.Body and io.ReadAll in batch_authpolicy_test_env.go), then validate whether
the returned length equals the cap and return an error if so (indicating the
response was truncated); apply the same change to the other occurrence in
internal/controller/utils/nim.go and add the new maxResponseBodySize constant
where appropriate.
In `@internal/controller/test/e2e/batch_authpolicy_test.go`:
- Around line 22-35: The shared route provisioning in setupBatchRoutes (the
batchRoutesOnce.Do block that calls batchEnv.createNamespace,
batchEnv.deployEchoServer, batchEnv.createHTTPRoute,
batchEnv.createServiceAccount, batchEnv.requestToken and
batchEnv.waitForGatewayRoute) must be moved out of the test-scoped setup so it
isn't bound to the first test's t.Cleanup/t.Fatalf; instead perform this
one-time provisioning during suite startup (e.g. TestMain in suite_test.go or
batchEnv initialization) and perform teardown in batchEnv.close(), ensuring no
helpers called from the Do body register t.Cleanup or call t.Fatalf so that
parallel tests can't lose the ability to re-run setup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 08a84eda-afd3-41c1-837c-36e5f7fb762b
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (7)
Makefilego.modinternal/controller/resources/template/BATCH.mdinternal/controller/resources/template/authpolicy_llm_isvc_userdefined.yamlinternal/controller/test/e2e/batch_authpolicy_test.gointernal/controller/test/e2e/batch_authpolicy_test_env.gointernal/controller/test/e2e/suite_test.go
internal/controller/resources/template/authpolicy_llm_isvc_userdefined.yaml
Outdated
Show resolved
Hide resolved
Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
internal/controller/test/e2e/batch_authpolicy_test_env.go (3)
349-356: Namespace deletion int.Cleanupignores errors silently.Line 355 discards the deletion error. If deletion fails (e.g., finalizers blocking), stale namespaces accumulate. Log the error for debugging CI failures.
Suggested fix
t.Cleanup(func() { if t.Failed() { t.Logf("keeping namespace %s for debugging (test failed)", name) return } t.Logf("deleting namespace %s", name) - _ = e.clientset.CoreV1().Namespaces().Delete(context.Background(), name, metav1.DeleteOptions{}) + if err := e.clientset.CoreV1().Namespaces().Delete(context.Background(), name, metav1.DeleteOptions{}); err != nil { + t.Logf("warning: failed to delete namespace %s: %v", name, err) + } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/test/e2e/batch_authpolicy_test_env.go` around lines 349 - 356, The t.Cleanup closure currently discards the error from the namespace deletion; change it to capture the returned error from e.clientset.CoreV1().Namespaces().Delete in the t.Cleanup function and call t.Logf (or t.Errorf if you prefer) to record the error alongside the namespace name and context so CI/debugging can see why deletion failed; reference the t.Cleanup closure and the call to e.clientset.CoreV1().Namespaces().Delete(context.Background(), name, metav1.DeleteOptions{}) to locate where to add the error variable and log it.
113-120: SetMinVersion: tls.VersionTLS12on test TLS config.While
InsecureSkipVerifyis acceptable for e2e with self-signed certs, omittingMinVersionallows TLS 1.0/1.1 negotiation. CWE-327.Suggested fix
httpClient := &http.Client{ Timeout: 10 * time.Second, Transport: &http.Transport{ TLSClientConfig: &tls.Config{ InsecureSkipVerify: true, //nolint:gosec // e2e tests use self-signed certs + MinVersion: tls.VersionTLS12, }, }, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/test/e2e/batch_authpolicy_test_env.go` around lines 113 - 120, Update the TLS config used when constructing httpClient: in the TLSClientConfig (tls.Config) for the http.Transport, set MinVersion: tls.VersionTLS12 while keeping InsecureSkipVerify for the e2e self-signed certs; modify the tls.Config inside the httpClient variable initialization so it includes MinVersion: tls.VersionTLS12 to prevent negotiating TLS 1.0/1.1.
258-258: SPDY dialer'shttp.Clientlacks timeout.The
http.Client{Transport: transport}passed tospdy.NewDialerhas no timeout, risking indefinite hangs during port-forward setup if the API server is slow or unresponsive.Suggested fix
- dialer := spdy.NewDialer(upgrader, &http.Client{Transport: transport}, http.MethodPost, req.URL()) + dialer := spdy.NewDialer(upgrader, &http.Client{Transport: transport, Timeout: 30 * time.Second}, http.MethodPost, req.URL())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/test/e2e/batch_authpolicy_test_env.go` at line 258, The spdy.NewDialer call creates a dialer with an http.Client that has no timeout (dialer := spdy.NewDialer(upgrader, &http.Client{Transport: transport}, ...)); update the http.Client passed to spdy.NewDialer to include a sensible Timeout (or derive a context with timeout for the port-forward operation) so the dialer can't hang indefinitely — modify the construction of the http.Client used with spdy.NewDialer (and any related request logic) to enforce a timeout value.internal/controller/test/e2e/batch_authpolicy_test.go (2)
103-108: Header verification viastrings.Containson echo body is fragile.The echo server response format may vary, and substring matching could false-positive if the username appears elsewhere. Consider unmarshaling the JSON response and checking the specific header field.
Suggested approach
// Parse echo server response and verify header explicitly type echoResponse struct { Request struct { Headers map[string]string `json:"headers"` } `json:"request"` } var echo echoResponse if err := json.Unmarshal(body, &echo); err != nil { t.Fatalf("unmarshal echo response: %v", err) } if got := echo.Request.Headers["x-maas-user"]; got != expectedUser { t.Errorf("x-maas-user = %q, want %q", got, expectedUser) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/test/e2e/batch_authpolicy_test.go` around lines 103 - 108, Replace the fragile substring check of the echo body with explicit JSON unmarshalling and header lookup: instead of using strings.Contains on body to find expectedUser (variable expectedUser in the test), unmarshal body into a struct matching the echo response (containing Request.Headers map[string]string) and assert that echo.Request.Headers["x-maas-user"] equals expectedUser; update error handling to fail the test on unmarshal error (t.Fatalf) and report a clear mismatch if the header value differs (t.Errorf).
160-179:TestInferencePathNoRBACduplicates fixture setup; consider reusingsetupFixturewith variant.Lines 164-172 replicate the pattern in
setupFixturebut without RBAC grants. Extract a parameterized helper to reduce duplication and maintenance burden.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/test/e2e/batch_authpolicy_test.go` around lines 160 - 179, TestInferencePathNoRBAC duplicates the fixture setup logic; refactor by extracting a parameterized helper (e.g., setupFixtureVariant) that encapsulates the common sequence currently in TestInferencePathNoRBAC (calls to setupBatchRoutes, batchEnv.createNamespace, batchEnv.deployEchoServer, batchEnv.createHTTPRoute) and accepts a variant flag or callback to control RBAC creation (so the helper can either create the SA and token or skip/grant privileges). Replace the duplicated sequence in TestInferencePathNoRBAC with a call to the new helper and keep the remaining unique calls (batchEnv.createServiceAccount, batchEnv.requestToken, batchEnv.waitForGatewayRoute, gatewayGet) adjusted to use the helper’s returned namespace/token as needed.internal/controller/resources/template/BATCH.md (1)
8-15: Specify language for fenced code block.Add a language identifier (e.g.,
textorhttp) for syntax highlighting and tooling consistency.Suggested fix
-``` +```text /v1/files GET, POST🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/resources/template/BATCH.md` around lines 8 - 15, The fenced code block in internal/controller/resources/template/BATCH.md lacks a language identifier; update the opening fence from ``` to ```text (or ```http) to specify the language for syntax highlighting, for example change the block start to ```text so the listed endpoints (/v1/files, /v1/batches, etc.) are properly highlighted by tooling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/controller/test/e2e/batch_authpolicy_test_env.go`:
- Around line 269-278: The port-forward goroutine started by sending
fw.ForwardPorts() to errCh can leak if readyCh never fires or the test is
cancelled; update the select around readyCh/errCh to be context-aware and ensure
stopCh is closed on any error or timeout so the port-forward terminates.
Specifically, create a context with timeout or derive from the test context, add
a case for ctx.Done() in the select, and in both the err-from-errCh and
ctx.Done() branches close(stopCh) before returning an error; ensure
fw.ForwardPorts() continues to be called in the goroutine but that stopCh
signals it to exit on failure.
---
Nitpick comments:
In `@internal/controller/resources/template/BATCH.md`:
- Around line 8-15: The fenced code block in
internal/controller/resources/template/BATCH.md lacks a language identifier;
update the opening fence from ``` to ```text (or ```http) to specify the
language for syntax highlighting, for example change the block start to ```text
so the listed endpoints (/v1/files, /v1/batches, etc.) are properly highlighted
by tooling.
In `@internal/controller/test/e2e/batch_authpolicy_test_env.go`:
- Around line 349-356: The t.Cleanup closure currently discards the error from
the namespace deletion; change it to capture the returned error from
e.clientset.CoreV1().Namespaces().Delete in the t.Cleanup function and call
t.Logf (or t.Errorf if you prefer) to record the error alongside the namespace
name and context so CI/debugging can see why deletion failed; reference the
t.Cleanup closure and the call to
e.clientset.CoreV1().Namespaces().Delete(context.Background(), name,
metav1.DeleteOptions{}) to locate where to add the error variable and log it.
- Around line 113-120: Update the TLS config used when constructing httpClient:
in the TLSClientConfig (tls.Config) for the http.Transport, set MinVersion:
tls.VersionTLS12 while keeping InsecureSkipVerify for the e2e self-signed certs;
modify the tls.Config inside the httpClient variable initialization so it
includes MinVersion: tls.VersionTLS12 to prevent negotiating TLS 1.0/1.1.
- Line 258: The spdy.NewDialer call creates a dialer with an http.Client that
has no timeout (dialer := spdy.NewDialer(upgrader, &http.Client{Transport:
transport}, ...)); update the http.Client passed to spdy.NewDialer to include a
sensible Timeout (or derive a context with timeout for the port-forward
operation) so the dialer can't hang indefinitely — modify the construction of
the http.Client used with spdy.NewDialer (and any related request logic) to
enforce a timeout value.
In `@internal/controller/test/e2e/batch_authpolicy_test.go`:
- Around line 103-108: Replace the fragile substring check of the echo body with
explicit JSON unmarshalling and header lookup: instead of using strings.Contains
on body to find expectedUser (variable expectedUser in the test), unmarshal body
into a struct matching the echo response (containing Request.Headers
map[string]string) and assert that echo.Request.Headers["x-maas-user"] equals
expectedUser; update error handling to fail the test on unmarshal error
(t.Fatalf) and report a clear mismatch if the header value differs (t.Errorf).
- Around line 160-179: TestInferencePathNoRBAC duplicates the fixture setup
logic; refactor by extracting a parameterized helper (e.g., setupFixtureVariant)
that encapsulates the common sequence currently in TestInferencePathNoRBAC
(calls to setupBatchRoutes, batchEnv.createNamespace, batchEnv.deployEchoServer,
batchEnv.createHTTPRoute) and accepts a variant flag or callback to control RBAC
creation (so the helper can either create the SA and token or skip/grant
privileges). Replace the duplicated sequence in TestInferencePathNoRBAC with a
call to the new helper and keep the remaining unique calls
(batchEnv.createServiceAccount, batchEnv.requestToken,
batchEnv.waitForGatewayRoute, gatewayGet) adjusted to use the helper’s returned
namespace/token as needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 334a206a-e611-4415-bebd-20093f073c9a
📒 Files selected for processing (4)
internal/controller/resources/template/BATCH.mdinternal/controller/resources/template/authpolicy_llm_isvc_userdefined.yamlinternal/controller/test/e2e/batch_authpolicy_test.gointernal/controller/test/e2e/batch_authpolicy_test_env.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/controller/resources/template/authpolicy_llm_isvc_userdefined.yaml
pierDipi
changed the title
/v1/files and /v1/batches/v1/files and /v1/batches
|
I'm making some changes |
Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
|
@coderabbitai full review |
pierDipi
changed the title
/v1/files and /v1/batches/v1/files and /v1/batches
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
internal/controller/test/e2e/batch_authpolicy_test_env.go (1)
123-131:⚠️ Potential issue | 🟠 MajorPreserve the gateway listener scheme instead of forcing
http://.
gatewayListenerPort()drops protocol and even accepts TCP, thennewTestEnv()builds every URL ashttp://.... An HTTPS listener will either fail the suite or send bearer tokens over cleartext, andInsecureSkipVerifydisables certificate validation entirely (CWE-319/CWE-295). Return(scheme, port)from listener discovery, reject non-HTTP listeners, and forhttpsload the gateway CA / setServerNameinstead of usingInsecureSkipVerify.Patch sketch
-func gatewayListenerPort(gw *gatewayapiv1.Gateway) int32 { +func gatewayListenerSchemeAndPort(gw *gatewayapiv1.Gateway) (string, int32, error) { for _, l := range gw.Spec.Listeners { switch l.Protocol { - case gatewayapiv1.HTTPProtocolType, gatewayapiv1.HTTPSProtocolType, gatewayapiv1.TCPProtocolType: - return int32(l.Port) + case gatewayapiv1.HTTPProtocolType: + return "http", int32(l.Port), nil + case gatewayapiv1.HTTPSProtocolType: + return "https", int32(l.Port), nil } } - return 80 + return "", 0, fmt.Errorf("no HTTP/HTTPS listener found") }As per coding guidelines,
**/*.go: No InsecureSkipVerify in TLS configs (enables MITM attacks).Also applies to: 154-169, 382-391
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/test/e2e/batch_authpolicy_test_env.go` around lines 123 - 131, gatewayListenerPort() and newTestEnv() currently drop the listener scheme and force "http://" while tests create an http.Client with InsecureSkipVerify; change gatewayListenerPort() to return (scheme, port) and update newTestEnv() to construct URLs using the returned scheme, reject non-HTTP/HTTPS listener protocols, and remove any TLSConfig.InsecureSkipVerify usage in the HTTP client setup (e.g., in batch_authpolicy_test_env.go where the client is created). For HTTPS listeners, load the gateway CA into a tls.Config RootCAs and set ServerName to the gateway host instead of disabling verification; ensure any places that previously accepted TCP-only listeners now validate and accept only "http" or "https".internal/controller/resources/template/authpolicy_llm_isvc_userdefined.yaml (1)
28-29:⚠️ Potential issue | 🔴 CriticalDo not trust client-supplied
x-maas-*on authn-only batch paths.Batch requests skip authorization and these rules append
x-maas-user/x-maas-groupsafterward. A caller can preseed those headers, and any batch gateway that still reads the first value or concatenates tenants will run under attacker-chosen identity (CWE-290/CWE-639). The workaround is only documented ininternal/controller/resources/template/BATCH.md; the policy should strip/reject preexistingx-maas-*headers or keep batch-path support behind a compatibility gate until all consumers enforce trusted-last handling.As per coding guidelines,
**: REVIEW PRIORITIES: 1. Security vulnerabilities (provide severity, exploit scenario, and remediation code).Also applies to: 81-92
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/resources/template/authpolicy_llm_isvc_userdefined.yaml` around lines 28 - 29, The predicate that allows authn-only batch paths (the rule using predicate "!(request.path.startsWith('/v1/files') || request.path.startsWith('/v1/batches'))") must ensure client-supplied x-maas-* headers cannot be trusted; update the batch-path handling to either strip/reject any incoming x-maas-* headers (e.g., x-maas-user, x-maas-groups) before appending server-set values, or gate the compatibility behavior behind a configuration flag so batch support is disabled until gateways enforce trusted-last header processing (see BATCH.md for context); locate the rule that matches /v1/batches (and the similar block around lines 81-92) and modify it to drop or reject x-maas-* headers on those authn-only paths.
🧹 Nitpick comments (1)
Makefile (1)
157-159: Wiretest-e2e-controllerinto automation before treating it as coverage.The only workflow snippet provided still calls
make test-e2eand does not provision the Gateway/AuthPolicy prerequisites thatinternal/controller/test/e2e/batch_authpolicy_test_env.goexpects. As written, this target is manual-only and will not catch regressions in PR CI. Add a dedicated job or extend the existing e2e workflow to invoke it after the gateway stack is up.Also applies to: 194-194
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 157 - 159, Add CI automation to run the Makefile target test-e2e-controller (which runs go test -tags=e2e against ./internal/controller/test/e2e/) after the gateway and Authorino prerequisites are provisioned; update the e2e workflow (the job that currently invokes make test-e2e) to either add a new job that depends on the gateway stack or extend the existing job to run make test-e2e-controller once the gateway/AuthPolicy stack is up and healthy so internal/controller/test/e2e/batch_authpolicy_test_env.go has its prerequisites satisfied.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/controller/resources/template/authpolicy_llm_isvc_userdefined.yaml`:
- Around line 28-29: The predicate in authpolicy_llm_isvc_userdefined.yaml uses
request.path.startsWith('/v1/files') and request.path.startsWith('/v1/batches'),
which permits names like /v1/files-model to bypass auth; change these checks to
exact-or-slash-boundary checks (e.g., replace each startsWith check with a test
that the path equals the segment OR begins with the segment followed by '/' or
use a regex match like '^/v1/(files|batches)(/|$)') so only the intended full
segment routes match, update the predicate string accordingly, and add an e2e
regression that verifies a route like /v1/files-model does not get treated as
the batch/files path to prevent future regressions.
In `@internal/controller/resources/template/BATCH.md`:
- Around line 8-15: Update the fenced code blocks in BATCH.md to include a
language label (e.g., change ``` to ```text or ```yaml) for each unlabeled block
that lists endpoints (such as the /v1/files and /v1/batches blocks) to satisfy
MD040, and fix the inline code spans that have surrounding spaces (e.g., change
backticked items like ` v1/batches ` to `v1/batches`) to remove the extra spaces
and resolve MD038; apply the same changes to the other occurrences noted (around
the same endpoint snippets referenced in the comment).
---
Duplicate comments:
In `@internal/controller/resources/template/authpolicy_llm_isvc_userdefined.yaml`:
- Around line 28-29: The predicate that allows authn-only batch paths (the rule
using predicate "!(request.path.startsWith('/v1/files') ||
request.path.startsWith('/v1/batches'))") must ensure client-supplied x-maas-*
headers cannot be trusted; update the batch-path handling to either strip/reject
any incoming x-maas-* headers (e.g., x-maas-user, x-maas-groups) before
appending server-set values, or gate the compatibility behavior behind a
configuration flag so batch support is disabled until gateways enforce
trusted-last header processing (see BATCH.md for context); locate the rule that
matches /v1/batches (and the similar block around lines 81-92) and modify it to
drop or reject x-maas-* headers on those authn-only paths.
In `@internal/controller/test/e2e/batch_authpolicy_test_env.go`:
- Around line 123-131: gatewayListenerPort() and newTestEnv() currently drop the
listener scheme and force "http://" while tests create an http.Client with
InsecureSkipVerify; change gatewayListenerPort() to return (scheme, port) and
update newTestEnv() to construct URLs using the returned scheme, reject
non-HTTP/HTTPS listener protocols, and remove any TLSConfig.InsecureSkipVerify
usage in the HTTP client setup (e.g., in batch_authpolicy_test_env.go where the
client is created). For HTTPS listeners, load the gateway CA into a tls.Config
RootCAs and set ServerName to the gateway host instead of disabling
verification; ensure any places that previously accepted TCP-only listeners now
validate and accept only "http" or "https".
---
Nitpick comments:
In `@Makefile`:
- Around line 157-159: Add CI automation to run the Makefile target
test-e2e-controller (which runs go test -tags=e2e against
./internal/controller/test/e2e/) after the gateway and Authorino prerequisites
are provisioned; update the e2e workflow (the job that currently invokes make
test-e2e) to either add a new job that depends on the gateway stack or extend
the existing job to run make test-e2e-controller once the gateway/AuthPolicy
stack is up and healthy so
internal/controller/test/e2e/batch_authpolicy_test_env.go has its prerequisites
satisfied.
🪄 Autofix (Beta)
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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: d7d6e173-6efb-4b50-b2d6-d71e156f5eb5
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (7)
Makefilego.modinternal/controller/resources/template/BATCH.mdinternal/controller/resources/template/authpolicy_llm_isvc_userdefined.yamlinternal/controller/test/e2e/batch_authpolicy_test.gointernal/controller/test/e2e/batch_authpolicy_test_env.gointernal/controller/test/e2e/suite_test.go
internal/controller/resources/template/authpolicy_llm_isvc_userdefined.yaml
Outdated
Show resolved
Hide resolved
KillianGolds
left a comment
KillianGolds
left a comment
There was a problem hiding this comment.
Nice work on the BATCH.md doc, made the review a lot easier to follow. Left a couple of minor comments. Nothing blocking from my side.
pierDipi
changed the title
/v1/files and /v1/batches/v1/files and /v1/batches
|
@pierDipi: This pull request references RHOAIENG-54696 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
|
@pierDipi: This pull request references RHOAIENG-54696 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
internal/controller/resources/template/authpolicy_llm_isvc_userdefined.yaml (1)
81-92: Consider addingmetrics: falseto batch response headers for consistency.The inference headers (
x-gateway-inference-fairness-id,x-gateway-inference-objective) explicitly setmetrics: false(lines 71-72, 77-78), but the batch headers (x-maas-user,x-maas-groups) omit this field. If omitted defaults totrue, these headers may appear in Authorino metrics, potentially leaking usernames.Proposed fix
x-maas-user: when: - predicate: "request.path == '/v1/files' || request.path.startsWith('/v1/files/') || request.path == '/v1/batches' || request.path.startsWith('/v1/batches/')" + metrics: false plain: expression: auth.identity.user.username priority: 0 x-maas-groups: when: - predicate: "request.path == '/v1/files' || request.path.startsWith('/v1/files/') || request.path == '/v1/batches' || request.path.startsWith('/v1/batches/')" + metrics: false plain: expression: "auth.identity.user.groups.join(',')" priority: 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/resources/template/authpolicy_llm_isvc_userdefined.yaml` around lines 81 - 92, The x-maas-user and x-maas-groups header definitions currently lack an explicit metrics setting and may be recorded in Authorino metrics; update the x-maas-user and x-maas-groups blocks to include metrics: false (alongside their existing when/plain/priority entries) so these headers are excluded from metrics collection and usernames aren't leaked.internal/controller/resources/template/BATCH.md (3)
221-221: Fix spaces inside code span (MD038).Line 221 has
`, `which triggers MD038. Rewrite to avoid spaces at span boundaries.Proposed fix
-from request headers (split by `, ` and take the last element, or equivalent). +from request headers (split by `", "` and take the last element, or equivalent).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/resources/template/BATCH.md` at line 221, In BATCH.md update the inline code span that currently reads `, ` to remove the leading/trailing space in the code span (e.g., use backticked comma only or an escaped representation such as `','` or ```,` ``) so the span no longer starts/ends with a space and the MD038 lint error is resolved; locate the phrase "from request headers (split by `, ` and take the last element, or equivalent)" and replace the `, ` token with a code span that contains no surrounding spaces.
223-227: Missing heading marker on "Batch Gateway fixes".Line 223 should be a proper heading to maintain document structure.
Proposed fix
-Batch Gateway fixes +### Batch Gateway fixes🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/resources/template/BATCH.md` around lines 223 - 227, The line containing the text "Batch Gateway fixes" is missing a markdown heading marker; change that plain line into a proper heading (e.g., prepend "## " or the appropriate heading level used in this document) so it becomes "## Batch Gateway fixes" to restore document structure and consistency with surrounding headings.
8-15: Add language specifiers to fenced code blocks (MD040).Static analysis flags unlabeled fenced code blocks at lines 8, 253, and 616. Add
textorplaintextlanguage identifiers.Proposed fix for line 8
-``` +```text /v1/files GET, POST /v1/files/{file_id} GET, DELETEApply similar fix at lines 253 and 616.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/resources/template/BATCH.md` around lines 8 - 15, Unlabeled fenced code blocks containing API endpoint listings (e.g., the block showing "/v1/files GET, POST" and the other blocks at the later sections) should include a language specifier to satisfy MD040; update each triple-backtick fence around those plaintext API path tables to use ```text (or ```plaintext) instead of plain ``` so the blocks at the API listing with "/v1/files GET, POST" and the similar blocks at the other two locations are labeled.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/controller/resources/template/BATCH.md`:
- Around line 151-163: Update the doc reference from authorino@v0.20.0 to
authorino@v0.22.0 and replace the file:line reference with the function name
buildResponseHeaders() in the BATCH.md snippet; then open the
buildResponseHeaders function in the authorino@v0.22.0 source to verify whether
HeaderValueOption entries include the AppendAction field and, if they still omit
it, document that the AppendAction should be set (and note the correct
enum/value used in v0.22.0) or else document that the issue was fixed in v0.22.0
so no change is needed—record the exact finding in the documentation.
---
Nitpick comments:
In `@internal/controller/resources/template/authpolicy_llm_isvc_userdefined.yaml`:
- Around line 81-92: The x-maas-user and x-maas-groups header definitions
currently lack an explicit metrics setting and may be recorded in Authorino
metrics; update the x-maas-user and x-maas-groups blocks to include metrics:
false (alongside their existing when/plain/priority entries) so these headers
are excluded from metrics collection and usernames aren't leaked.
In `@internal/controller/resources/template/BATCH.md`:
- Line 221: In BATCH.md update the inline code span that currently reads `, ` to
remove the leading/trailing space in the code span (e.g., use backticked comma
only or an escaped representation such as `','` or ```,` ``) so the span no
longer starts/ends with a space and the MD038 lint error is resolved; locate the
phrase "from request headers (split by `, ` and take the last element, or
equivalent)" and replace the `, ` token with a code span that contains no
surrounding spaces.
- Around line 223-227: The line containing the text "Batch Gateway fixes" is
missing a markdown heading marker; change that plain line into a proper heading
(e.g., prepend "## " or the appropriate heading level used in this document) so
it becomes "## Batch Gateway fixes" to restore document structure and
consistency with surrounding headings.
- Around line 8-15: Unlabeled fenced code blocks containing API endpoint
listings (e.g., the block showing "/v1/files GET, POST" and the other blocks at
the later sections) should include a language specifier to satisfy MD040; update
each triple-backtick fence around those plaintext API path tables to use ```text
(or ```plaintext) instead of plain ``` so the blocks at the API listing with
"/v1/files GET, POST" and the similar blocks at the other
two locations are labeled.
🪄 Autofix (Beta)
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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 26bb8d01-cf90-4af0-93fe-70a18e89d87c
📒 Files selected for processing (2)
internal/controller/resources/template/BATCH.mdinternal/controller/resources/template/authpolicy_llm_isvc_userdefined.yaml
|
@pierDipi: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
bartoszmajsak
left a comment
bartoszmajsak
left a comment
There was a problem hiding this comment.
Few minor comments.
| priority: 0 | ||
| x-maas-user: | ||
| when: | ||
| - predicate: "request.path == '/v1/files' || request.path.startsWith('/v1/files/') || request.path == '/v1/batches' || request.path.startsWith('/v1/batches/')" |
There was a problem hiding this comment.
Probably some room for improvement on Kuadrant side to make re-usable patterns a bit more flexible.
There was a problem hiding this comment.
Is it a temporary solution and we aim for actual e2e tests when batch will become avail?
By looking at makefile we probably want to make some quality of life improvements around e2e in general:
Lines 115 to 119 in cce5945
not as part of this work, but worth planning as follow-up.
There was a problem hiding this comment.
I think these are e2e but with a mock service that helps inspecting injected headers, we won't be able to do that with a full batch service
| Authorino's response header injection currently **appends** to existing request headers instead | ||
| of overwriting them. If a client sends `x-maas-user: attacker` and Authorino injects | ||
| `x-maas-user: real-user`, the backend receives `attacker, real-user`. | ||
|
|
||
| This affects **all** response headers — both batch headers (`x-maas-user`, `x-maas-groups`) and | ||
| flow control headers (`x-gateway-inference-fairness-id`, `x-gateway-inference-objective`). |
There was a problem hiding this comment.
Are we tracking it somewhere? Maybe worth a link?
There was a problem hiding this comment.
| ### Required fix | ||
|
|
||
| Authorino must explicitly set `AppendAction: OVERWRITE_IF_EXISTS_OR_ADD` on every | ||
| `HeaderValueOption`: | ||
|
|
||
| ```go | ||
| package main | ||
|
|
||
| responseHeaders = append(responseHeaders, &envoy_core.HeaderValueOption{ | ||
| Header: &envoy_core.HeaderValue{ | ||
| Key: key, | ||
| Value: value, | ||
| }, | ||
| AppendAction: envoy_core.HeaderValueOption_OVERWRITE_IF_EXISTS_OR_ADD, | ||
| }) | ||
| ``` | ||
|
|
||
| This ensures Authorino-injected headers replace any client-supplied values with the same name, | ||
| preventing header injection attacks and multi-value concatenation. |
There was a problem hiding this comment.
on Slack they said it's fixed upstream (1.4?)
There was a problem hiding this comment.
ref https://redhat-internal.slack.com/archives/C04J77H00TD/p1772810155222059 (see full thread)
| When the `inference-access` rule is skipped (batch paths), no authorization rules remain, | ||
| so Authorino implicitly allows the request — authentication-only behavior. | ||
|
|
||
| ## Full template sketch |
There was a problem hiding this comment.
I think this needs updates - or maybe it's not full template.
|
Makes sense on both points i made, thanks. The exact path matching fix looks good too. Nothing else from me. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: KillianGolds, pierDipi The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
bot
merged commit 3265945
into
opendatahub-io:incubating
…hub-io#709) * Batch notes Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Use single rule with custom resource and verb Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Implementation Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Fix CEL expressions in AuthPolicy template for header checks Replace invalid has() macro with 'key' in map operator for checking request headers in Authorino CEL expressions. The has() macro does not work with bracket-notation map access in Authorino's CEL implementation. Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Update BATCH.md with CEL fix, gateway docs, and curl flags Update CEL expressions to match the template fix, add gateway.yaml reference with allowedRoutes documentation, add apiVersion/kind/metadata to the rendered AuthPolicy example, and add -vk flags to curl commands. Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Add e2e tests for batch AuthPolicy scenarios Add e2e tests verifying the AuthPolicy handles batch and inference paths correctly: authn-only for batch paths, standard SAR for inference, delegated SAR with x-maas-user header, and rejection of spoofed/unauthorized headers. Tests use port-forwarding for cluster-internal gateway access and include a Makefile target (test-e2e-controller) integrated into the CI pipeline. Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Wait for AuthPolicy enforcement before sending test requests After creating an HTTPRoute, poll its status.parents for the Kuadrant policy controller's kuadrant.io/AuthPolicyAffected condition to become True. This ensures the ext-authz filter is active before tests run, preventing false positives from unenforced authentication. Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Add test coverage for /v1/files, no-token, and no-RBAC scenarios Add tests for /v1/files batch path (authn-only and spoofed header), unauthenticated requests (expect 401), and inference path with no RBAC (expect 403). Remove response body from all error messages to prevent leaking cluster internals in CI logs. Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Use post-delegate verb Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Double rule to check both delegate and get access Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * fix: exact patch matching Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> --------- Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> Signed-off-by: Raghul-M <raghul.m1430@gmail.com>
4 participants
Notes on auth-aware Batch Gateway
Summary by CodeRabbit
New Features
Documentation
Tests
Chores