fix(automl,autorag): extend dynamic port-forwarding to S3 and LlamaStack paths#7310
Conversation
…ack paths The initial port-forwarding implementation only covered the DSPA middleware path. Three additional code paths also use in-cluster URLs that need rewriting for local development: - S3 resolveS3Client (both packages): when secretName is provided, the DSPA middleware is skipped and the endpoint URL from the secret is used directly. Add port-forward rewrite before S3 client creation. - LlamaStack AttachLlamaStackClientFromSecret (autorag): the base URL from the secret is used directly. Add port-forward rewrite before client creation. - LlamaStack ListProviders (autorag): auth token was stripped on HTTP requests. After port-forwarding rewrites to http://localhost, the token must be sent. Allow auth on localhost connections. All changes are guarded by portForwardManager being non-nil (requires DevMode=true) or by localhost-only scope. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Skipping CI for Draft Pull Request. |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 48 minutes and 13 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis change adds port-forwarding capability for development environments across multiple service handlers. Three files conditionally rewrite endpoint URLs (S3 and llama_stack_client_base_url) when a Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Security IssuesCWE-522 (Insufficiently Protected Credentials) / CWE-295 (Improper Certificate Validation): CWE-200 (Exposure of Sensitive Information): CWE-1025 (Comparison Using Wrong Factors): 🚥 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 |
|
@jefho-rh @nickmazzi @GAUNSD I hit some issues today when trying to test the execution of the latest air-gap cluster compatible pipeline yaml files for #7307 against cluster https://console-openshift-console.apps.ods-dis-rhoai-test.aws.rh-ods.com. The earlier port-forwarding changes fixed issues connecting to managed minio as part of retrieving pipeline results, but missed a couple other local development scenarios:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7310 +/- ##
==========================================
+ Coverage 63.92% 64.97% +1.05%
==========================================
Files 2502 2447 -55
Lines 77696 76159 -1537
Branches 19756 19216 -540
==========================================
- Hits 49664 49488 -176
+ Misses 28032 26671 -1361 see 73 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/automl/bff/internal/api/s3_handler.go (1)
149-155: Same silent-pfErr pattern — please log and fall through.Matches the finding in
autorag/.../middleware.go. The siblingAttachPipelineServerClientblock already uses thelogger.Warn+ fallback pattern; this S3 endpoint rewrite should too, or dev-mode failures will surface as opaque S3 connection errors.🔧 Proposed diff
if app.portForwardManager != nil && creds.EndpointURL != "" { - if rewritten, pfErr := app.portForwardManager.ForwardURL(ctx, creds.EndpointURL); pfErr == nil { + if rewritten, pfErr := app.portForwardManager.ForwardURL(ctx, creds.EndpointURL); pfErr != nil { + app.logger.Warn("dynamic port-forward failed for S3 endpoint, using original URL", + "error", pfErr, "url", creds.EndpointURL) + } else { creds.EndpointURL = rewritten } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/automl/bff/internal/api/s3_handler.go` around lines 149 - 155, The S3 endpoint rewrite currently swallows port-forward errors; update the block that calls app.portForwardManager.ForwardURL so that when pfErr != nil it logs the error (use the same logger.Warn pattern as AttachPipelineServerClient) and then falls through without modifying creds.EndpointURL, and only assign creds.EndpointURL = rewritten when pfErr == nil; reference app.portForwardManager, creds.EndpointURL, and ForwardURL to locate and change the code.packages/autorag/bff/internal/api/middleware.go (1)
387-393: Port-forward error is silently swallowed — inconsistent with the sibling block 15 lines down.
AttachPipelineServerClient(lines 506-513) logs a warning onpfErrand falls back to the original URL, but this new LlamaStack block drops the error on the floor. In dev, that turns "port-forward failed" into a confusing DNS error later with no breadcrumb. Mirror the existing pattern for consistency and debuggability.🔧 Proposed diff
if app.portForwardManager != nil { - if rewritten, pfErr := app.portForwardManager.ForwardURL(ctx, baseURL); pfErr == nil { + if rewritten, pfErr := app.portForwardManager.ForwardURL(ctx, baseURL); pfErr != nil { + logger.Warn("dynamic port-forward failed for LlamaStack base URL, using original URL", + "error", pfErr, "url", baseURL) + } else { baseURL = rewritten } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/autorag/bff/internal/api/middleware.go` around lines 387 - 393, The LlamaStack dev-only port-forward block is silently dropping errors from app.portForwardManager.ForwardURL which hides why requests later fail; update the block that calls app.portForwardManager.ForwardURL(ctx, baseURL) to mirror the sibling AttachPipelineServerClient behavior: if ForwardURL returns an error, log a warning including the error and original baseURL (e.g. via app.logger.Warnf or the same logger used in AttachPipelineServerClient) and then continue using the original baseURL as the fallback. Ensure you reference the same logging call/signature used in the sibling block so the behavior and message format remain consistent.packages/autorag/bff/internal/api/s3_handler.go (1)
147-153: Duplicate of the silent-pfErr issue flagged in automl'ss3_handler.goand autorag'smiddleware.go.Same fix pattern — log a warning on
pfErrand fall through to the original endpoint so failed port-forwards are diagnosable rather than masquerading as S3 connect errors.🔧 Proposed diff
if app.portForwardManager != nil && creds.EndpointURL != "" { - if rewritten, pfErr := app.portForwardManager.ForwardURL(ctx, creds.EndpointURL); pfErr == nil { + if rewritten, pfErr := app.portForwardManager.ForwardURL(ctx, creds.EndpointURL); pfErr != nil { + app.logger.Warn("dynamic port-forward failed for S3 endpoint, using original URL", + "error", pfErr, "url", creds.EndpointURL) + } else { creds.EndpointURL = rewritten } }Also note: the three call sites (LlamaStack base URL, two S3 endpoints) plus the two existing ones in
AttachPipelineServerClientare now five near-identicalportForwardManager.ForwardURLwrappers across autorag/automl. A small shared helper likeapp.rewriteURLWithPortForward(ctx, logger, "S3 endpoint", url)would DRY this up and guarantee uniform logging going forward.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/autorag/bff/internal/api/s3_handler.go` around lines 147 - 153, The port-forward failure is currently ignored in the app.portForwardManager.ForwardURL call, so update the block around app.portForwardManager and creds.EndpointURL to log a warning when pfErr != nil and then continue using the original creds.EndpointURL (do not overwrite it on error); specifically, change the ForwardURL handling for app.portForwardManager.ForwardURL(ctx, creds.EndpointURL) to check pfErr and call the logger (e.g., app.logger.Warn or similar) with context like "port-forward failed for S3 endpoint" plus pfErr, only assign creds.EndpointURL = rewritten when pfErr == nil, and consider extracting this pattern into a shared helper (e.g., app.rewriteURLWithPortForward(ctx, logger, "S3 endpoint", url)) to reuse for other ForwardURL call sites.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/automl/bff/internal/api/s3_handler.go`:
- Around line 149-155: The S3 endpoint rewrite currently swallows port-forward
errors; update the block that calls app.portForwardManager.ForwardURL so that
when pfErr != nil it logs the error (use the same logger.Warn pattern as
AttachPipelineServerClient) and then falls through without modifying
creds.EndpointURL, and only assign creds.EndpointURL = rewritten when pfErr ==
nil; reference app.portForwardManager, creds.EndpointURL, and ForwardURL to
locate and change the code.
In `@packages/autorag/bff/internal/api/middleware.go`:
- Around line 387-393: The LlamaStack dev-only port-forward block is silently
dropping errors from app.portForwardManager.ForwardURL which hides why requests
later fail; update the block that calls app.portForwardManager.ForwardURL(ctx,
baseURL) to mirror the sibling AttachPipelineServerClient behavior: if
ForwardURL returns an error, log a warning including the error and original
baseURL (e.g. via app.logger.Warnf or the same logger used in
AttachPipelineServerClient) and then continue using the original baseURL as the
fallback. Ensure you reference the same logging call/signature used in the
sibling block so the behavior and message format remain consistent.
In `@packages/autorag/bff/internal/api/s3_handler.go`:
- Around line 147-153: The port-forward failure is currently ignored in the
app.portForwardManager.ForwardURL call, so update the block around
app.portForwardManager and creds.EndpointURL to log a warning when pfErr != nil
and then continue using the original creds.EndpointURL (do not overwrite it on
error); specifically, change the ForwardURL handling for
app.portForwardManager.ForwardURL(ctx, creds.EndpointURL) to check pfErr and
call the logger (e.g., app.logger.Warn or similar) with context like
"port-forward failed for S3 endpoint" plus pfErr, only assign creds.EndpointURL
= rewritten when pfErr == nil, and consider extracting this pattern into a
shared helper (e.g., app.rewriteURLWithPortForward(ctx, logger, "S3 endpoint",
url)) to reuse for other ForwardURL call sites.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 6a6aac3c-a4e9-4397-8e37-89d9c11cf7e6
📒 Files selected for processing (4)
packages/automl/bff/internal/api/s3_handler.gopackages/autorag/bff/internal/api/middleware.gopackages/autorag/bff/internal/api/s3_handler.gopackages/autorag/bff/internal/integrations/llamastack/llamastack_client.go
…ping Address review feedback: ForwardURL errors in the S3 handler and LlamaStack middleware were silently swallowed. Now log warnings consistent with the pattern used in AttachPipelineServerClient. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Thanks for finding and fixing the in-cluster service URL flows @chrjones-rh! No critical issues from AI-Assisted review ✅ Manual testing AutoRAG S3 with external service URL 
✅ Manual testing AutoRAG S3 with internal service URL 
✅ Manual testing AutoRAG LS with external service URL 
✅ Manual testing AutoRAG LS with internal service URL 
✅ Manual testing AutoML S3 with external service URL 
✅ Manual testing AutoML S3 with internal service URL 
✅ All tests passing
/lgtm |
|
/approve AI assisted reviewOverall AssessmentApprove — This is a clean, well-scoped follow-up that correctly extends an established pattern. The changes are minimal, consistent, and properly guarded for dev-only use. SuggestionsThese are non-blocking observations:
Test ValidationautoragFrontend ✅BFF ✅Contract ✅automlFrontend ✅BFF ✅Contract ✅ |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nickmazzi 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 794845f
into
opendatahub-io:main
…ack paths (opendatahub-io#7310) * fix(automl,autorag): extend dynamic port-forwarding to S3 and LlamaStack paths The initial port-forwarding implementation only covered the DSPA middleware path. Three additional code paths also use in-cluster URLs that need rewriting for local development: - S3 resolveS3Client (both packages): when secretName is provided, the DSPA middleware is skipped and the endpoint URL from the secret is used directly. Add port-forward rewrite before S3 client creation. - LlamaStack AttachLlamaStackClientFromSecret (autorag): the base URL from the secret is used directly. Add port-forward rewrite before client creation. - LlamaStack ListProviders (autorag): auth token was stripped on HTTP requests. After port-forwarding rewrites to http://localhost, the token must be sent. Allow auth on localhost connections. All changes are guarded by portForwardManager being non-nil (requires DevMode=true) or by localhost-only scope. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(automl,autorag): log port-forward errors instead of silently dropping Address review feedback: ForwardURL errors in the S3 handler and LlamaStack middleware were silently swallowed. Now log warnings consistent with the pattern used in AttachPipelineServerClient. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
3 participants
https://issues.redhat.com/browse/RHOAIENG-58535
Description
Follow-up to #7281. The initial dynamic port-forwarding implementation only covered the DSPA middleware path in
AttachPipelineServerClient. Three additional code paths also use in-cluster URLs that need rewriting for local development:S3
resolveS3Client(both automl and autorag)When the caller provides an explicit
secretNamequery parameter, theattachPipelineClientIfNeededshim skipsAttachPipelineServerCliententirely — so the DSPA storage discovery and its port-forward rewriting never run. The S3 endpoint URL from the secret (e.g.http://minio-dspa.namespace.svc.cluster.local:9000) was used as-is, failing withno such hostlocally.Fix: Add port-forward rewrite in
resolveS3Clientbefore S3 client creation, covering both the DSPA and explicitsecretNamepaths.LlamaStack
AttachLlamaStackClientFromSecret(autorag)The LlamaStack base URL is read from a K8s secret and used directly to create the client. When the secret contains an in-cluster URL (e.g.
http://llama-stack-service.llama-stack.svc.cluster.local:8321), it's unreachable locally.Fix: Add port-forward rewrite before LlamaStack client creation.
LlamaStack
ListProvidersauth token (autorag)The
ListProvidersmethod intentionally strips theAuthorization: Bearerheader on HTTP requests to prevent token leakage over cleartext. After port-forwarding rewrites the URL tohttp://localhost:<port>, the scheme is HTTP and the token is dropped — causing the LlamaStack server to reject the request.Fix: Allow the auth token to be sent over HTTP when the destination is localhost (traffic stays on loopback, never leaves the machine).
Safety
app.portForwardManager != nil(requiresDevMode=true, never set in production)localhostor127.0.0.1— secrets in production always contain in-cluster or external URLsHow Has This Been Tested?
secretNamefor managed MinIOTest Impact
No tests added — changes are dev-only port-forward rewriting following the same pattern established in #7281.
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
mainSummary by CodeRabbit