Make CORS origins configurable via environment variable in the engine - #152
Make CORS origins configurable via environment variable in the engine#152vikasgoswami2 wants to merge 1 commit into
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe pull request adds configurable CORS origins support to the Skyflo engine. A new ChangesCORS Configuration & Wiring
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@charts/skyflo/templates/engine-configmap.yaml`:
- Line 20: The CORS_ORIGINS env line should use Helm's default filter so an
unset value doesn't render as an empty string and override the Python default;
replace the current use of .Values.engine.config.corsOrigins with the Helm
default form (e.g., default nil .Values.engine.config.corsOrigins) when
rendering the CORS_ORIGINS environment variable (the CORS_ORIGINS key and
.Values.engine.config.corsOrigins reference) so the template either preserves
the app's pydantic default or only injects the env var when a value is provided.
In `@charts/skyflo/values.yaml`:
- Line 42: Uncomment and set a default for the engine.config.corsOrigins value
so the rendered CORS_ORIGINS env var is not an empty string; specifically,
restore/correct the corsOrigins entry in values.yaml to the pydantic-settings
default "http://localhost:8080,http://127.0.0.1:8080" so that the template
interpolation (.Values.engine.config.corsOrigins) used to populate CORS_ORIGINS
in the engine ConfigMap yields a non-empty value and does not override the
Python default to an empty string.
In `@engine/src/api/middleware/__init__.py`:
- Around line 13-16: Check whether cors_origins (computed from
settings.CORS_ORIGINS in the middleware init) is empty before calling
app.add_middleware; if it resolves to an empty list, either log a clear warning
via the application's logger or raise an exception so startup fails fast, and
include the offending settings.CORS_ORIGINS value in the message. Ensure you
update the code path around cors_origins, CORSMiddleware, and app.add_middleware
so operators see a warning/error when no origins are configured.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 482c565a-0c78-4b70-92c7-8c61135f6a2a
📒 Files selected for processing (6)
charts/skyflo/templates/engine-configmap.yamlcharts/skyflo/values.yamldeployment/config/engine-configmap.yamlengine/.env.exampleengine/src/api/config/settings.pyengine/src/api/middleware/__init__.py
|
@KaranJagtiani Can you please check and let me know if I need to work on these coderabbit suggestions ? |
KaranJagtiani
left a comment
KaranJagtiani
left a comment
There was a problem hiding this comment.
@vikasgoswami2 please resolve coderabbit comments as well
| default=None, env="LLM_THINKING_BUDGET_TOKENS" | ||
| ) | ||
| AGENT_TYPE: str = "assistant" | ||
| CORS_ORIGINS: str = "http://localhost:8080,http://127.0.0.1:8080" |
There was a problem hiding this comment.
The default port is wrong. The UI runs on port 3000, not 8080. This breaks local development out of the box. The issue spec explicitly states the default must be http://localhost:3000,http://127.0.0.1:3000 -- that is what was hardcoded before this PR and what the acceptance criteria requires. Port 8080 is nowhere in this codebase as a UI port.
There was a problem hiding this comment.
@KaranJagtiani Are you sure it is running on 3000 port because everywhere for the engine I can see 8080 port. Even in the helm chart engine deployment I can see 8080 port is exposed. It's Dockerfile also expose 8080 port and even though in the ReadMe it is mentioned that engine is running on port 8080. Please find few links where I've seen this :-
skyflo/deployment/engine/Dockerfile
Line 62 in 8104550
Please let me know and based on your response I'll make the necessary changes.
There was a problem hiding this comment.
@vikasgoswami2 8080 is the engine port, not the UI origin. CORS controls which browser origins can call the engine, so it must match where the UI is loaded from, not where the engine listens.
The UI runs on 3000 (yarn dev, Next.js containerPort in the UI deployment). In cluster installs the nginx proxy handles /api/v1 on the same origin, so CORS mostly does not matter there. It does matter for local dev when the browser talks to the engine directly, and that origin is localhost:3000.
Please change the default to http://localhost:3000,http://127.0.0.1:3000 across settings, values, and the configmaps.
48baa20 to
b621ee6
Compare
feat (engine): make CORS origins configurable via environment variable
b621ee6 to
a89c8e5
Compare
KaranJagtiani
left a comment
KaranJagtiani
left a comment
There was a problem hiding this comment.
We can drop the engine scope from the commit msg since it touches deployment files too:
feat: make CORS origins configurable via environment variable
Also, please make the necessary changes wherever the port is incorrect.
| default=None, env="LLM_THINKING_BUDGET_TOKENS" | ||
| ) | ||
| AGENT_TYPE: str = "assistant" | ||
| CORS_ORIGINS: str = "http://localhost:8080,http://127.0.0.1:8080" |
There was a problem hiding this comment.
@vikasgoswami2 8080 is the engine port, not the UI origin. CORS controls which browser origins can call the engine, so it must match where the UI is loaded from, not where the engine listens.
The UI runs on 3000 (yarn dev, Next.js containerPort in the UI deployment). In cluster installs the nginx proxy handles /api/v1 on the same origin, so CORS mostly does not matter there. It does matter for local dev when the browser talks to the engine directly, and that origin is localhost:3000.
Please change the default to http://localhost:3000,http://127.0.0.1:3000 across settings, values, and the configmaps.
2 participants
Description
Please include a summary of the changes and the motivation behind them.
Related Issue(s)
#95
Type of Change
Testing
Please describe the tests you've added/performed to verify your changes.
Checklist
Before Requesting Review
Code Quality
printstatements orconsole.logcallspackage-lock.json(we useyarnonly for the UI)Screenshots (if applicable)
Additional Notes