build(dx): Added reverse proxy using nginx for grafana #1114
Conversation
Modified docker compose and nginx config to reverse proxy grafana
Modified docker compose and nginx config to reverse proxy grafana
oindrila-b
requested review from
devesh-juspay,
junaid-shirur,
kalpadhwaryu,
shivamashtikar and
zereraz
as code owners
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds GF_SERVER_ROOT_URL to Grafana services in two docker-compose files, changes Grafana port mapping, and updates NGINX to add a Grafana upstream, exclude /grafana from static-file matching, redirect Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as Browser
participant N as NGINX
participant G as Grafana (xyne-grafana:3000)
rect rgba(235,245,255,0.6)
Note over U,N: Requesting Grafana root
U->>N: GET /grafana
N-->>U: 301 /grafana/ (redirect)
end
U->>N: GET /grafana/
alt Static asset? (excluded)
N->>G: Proxy /grafana/... (Host, X-Real-IP, X-Forwarded-For, X-Forwarded-Proto)
G-->>N: 200 OK
N-->>U: 200 OK
else WebSocket / Upgrade
N->>G: Proxy with Upgrade/Connection headers
G-->>N: Upgraded stream
N-->>U: Stream
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
Summary of ChangesHello @oindrila-b, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request implements a reverse proxy for Grafana using NGINX, allowing users to access the Grafana dashboard through a consistent and user-friendly URL ( Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds an Nginx reverse proxy for Grafana, allowing access via the /grafana subpath. The changes in the docker-compose files correctly configure Grafana's root URL, and the nginx.conf is updated to handle the proxying. My review includes a few suggestions: a high-severity recommendation to remove the directly exposed Grafana port to enforce access through Nginx, and a couple of medium-severity improvements for the Nginx configuration regarding redirect status codes, formatting consistency, and file termination. Overall, the changes are well-structured to achieve the goal. Addressing the feedback will improve the robustness and consistency of the configuration.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
deployment/portable/docker-compose.infrastructure.yml (1)
229-248: Add grafana service dependency to nginx.The nginx service should depend on the grafana service to ensure proper startup order. Without this dependency, nginx might start before Grafana is ready, causing initial connection failures.
Add the grafana dependency to the nginx service:
depends_on: - livekit + - grafana volumes:deployment/portable/docker-compose.infrastructure-cpu.yml (1)
227-246: Add grafana service dependency to nginx.The nginx service should depend on the grafana service to ensure proper startup order and avoid initial connection failures.
Add the grafana dependency:
depends_on: - livekit + - grafana volumes:
🧹 Nitpick comments (1)
deployment/portable/nginx.conf (1)
230-240: Add explicit timeout settings for Grafana queries.While the proxy configuration correctly handles WebSocket upgrades and standard headers, consider adding explicit timeout settings for long-running Grafana queries and dashboard loads.
Add timeout configurations:
location /grafana/ { proxy_pass http://grafana_backend/; proxy_http_version 1.1; proxy_set_header Host $host; proxy_set_header X-Real-IP $remote_addr; proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; proxy_set_header X-Forwarded-Proto $scheme; proxy_set_header Upgrade $http_upgrade; proxy_set_header Connection $connection_upgrade; + proxy_read_timeout 300; + proxy_connect_timeout 30; + proxy_send_timeout 300; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
deployment/portable/docker-compose.infrastructure-cpu.yml(1 hunks)deployment/portable/docker-compose.infrastructure.yml(1 hunks)deployment/portable/nginx.conf(3 hunks)
🔇 Additional comments (5)
deployment/portable/docker-compose.infrastructure.yml (1)
52-52: LGTM! Grafana root URL configuration is correct.The
GF_SERVER_ROOT_URLtemplate format is appropriate for Grafana. The template variables%(protocol)s,%(domain)s, and%(http_port)swill be dynamically resolved by Grafana at runtime to construct the proper root URL.deployment/portable/docker-compose.infrastructure-cpu.yml (1)
52-52: LGTM! Grafana root URL configuration is correct.The
GF_SERVER_ROOT_URLconfiguration is consistent with the GPU variant and properly formatted for Grafana's runtime template resolution.deployment/portable/nginx.conf (3)
83-87: LGTM! Grafana upstream configuration is correct.The upstream configuration properly references the Grafana container name and internal port, with appropriate keepalive settings.
210-223: LGTM! Static files regex correctly excludes Grafana paths.The negative lookahead
(?!grafana/)properly ensures that Grafana's static assets are not intercepted by this location block, allowing them to be handled by the Grafana proxy location instead.
225-228: LGTM! Trailing slash redirect is appropriate.The 302 redirect from
/grafanato/grafana/ensures proper path handling and is the safer choice for deployments compared to a permanent 301 redirect.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
deployment/portable/nginx.conf (3)
83-87: Remove trailing whitespace on line 85.There appears to be trailing whitespace after
3000;on line 85. This was flagged in a previous review. Please remove it for consistency.
225-228: LGTM: Permanent redirect is correctly implemented.The 301 (permanent) redirect is the appropriate choice for canonicalizing the URL by adding a trailing slash. This addresses the previous review feedback and will improve performance as browsers cache permanent redirects.
242-242: Ensure file ends with a newline.As noted in a previous review, the file should end with a newline character to comply with POSIX standards and prevent issues with text processing tools.
🧹 Nitpick comments (1)
deployment/portable/nginx.conf (1)
230-230: Consider clarifying the location ordering comment.The comment states "must come after static files to ensure proper handling," but nginx location matching follows precedence rules rather than file order. The
/grafana/prefix match will be evaluated before the~*regex match for static files, regardless of their position in the config. If the comment refers to logical organization for readability, consider clarifying that.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
deployment/portable/docker-compose.infrastructure-cpu.yml(1 hunks)deployment/portable/docker-compose.infrastructure.yml(1 hunks)deployment/portable/nginx.conf(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- deployment/portable/docker-compose.infrastructure-cpu.yml
- deployment/portable/docker-compose.infrastructure.yml
🔇 Additional comments (1)
deployment/portable/nginx.conf (1)
210-211: Static file exclusion pattern is correct.The negative lookahead
(?!grafana/)correctly excludes Grafana paths from being cached by this location block, ensuring Grafana's static assets are handled by the dedicated Grafana location block.
2 participants
Description
Added reverse proxy using NGINX for Grafana, so that users can now access Grafana using
host-address/grafanaendpoint.Testing
Tested Locally
Additional Notes
NA
Summary by CodeRabbit
New Features
Improvements