Add Nginx as a workaround to get log streaming working again #767
Conversation
WalkthroughThis pull request introduces enhancements to the Cloudflared Home Assistant add-on, focusing on improving dependency management and adding an Nginx service configuration. The changes include a new Renovate configuration for tracking dependencies, a script to manage Nginx versioning, a detailed Nginx configuration template, and S6 overlay scripts for managing the Nginx service lifecycle. The modifications aim to provide more robust service handling and dependency tracking within the add-on. Changes
Sequence DiagramsequenceDiagram
participant Build as build.sh
participant Renovate as Renovate Config
participant Nginx as Nginx Service
participant S6 as S6 Overlay
Renovate->>Build: Track dependencies
Build->>Build: Set Nginx version
Build->>Nginx: Install Nginx
S6->>Nginx: Configure service
S6->>Nginx: Start service
Nginx->>Nginx: Process requests
S6->>Nginx: Monitor service health
The sequence diagram illustrates the workflow of dependency tracking, Nginx installation, configuration, and service management within the Cloudflared Home Assistant add-on. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
elcajon
added
new-feature
|
Thanks a lot! While testing one first thought: If we want to keep this as an optional workaround and expect a fix outside of this add-on at some point, would it not be sufficient to document a solution using the NGINX HA add-on and the tunnel with a catch-all rule on our end? |
|
I assume so, but without further Nginx configuration this would not work via the official nginx add-on. I had already tested some configurations locally with Nginx and Caddy. It's not that easy to get it to work if you're not familiar with proxies. I'm not so happy with this solution either, because I don't think it should be solved in the add-on, but I think for locally managed tunnels we could easily switch back if there is a solution upstream in the future. |
|
I tested it locally and it looks good to me. Have we identified any additional things (next to live logging) that are not working? If it is only that, I could live with the limited functionality, especially since the logs can be accessed while in the local network. On the other hand, I am also fine with integrating this workaround for anyone interested in fixing it themselves since it is not affecting anyone else. So if you want to include it, I am happy to approve the changes once the documentation is also updated with a comment and solution for the remote tunnels. |
|
I see what you mean, and I don't feel comfortable adding something new that isn't really related to the core functionality of the add-on. I also don't want to extend that functionality in the future. I don't think we have any other direct limitations apart from the live logs. But missing logs while debugging automations etc. seems to be crucial for me - also while away from home. I honestly don't know if we should even mention it in the docs at this point. Anyone who comes across the problem and reads about it in the discussions could make the change. Otherwise, I would rather make it available quietly to not confuse the average user. Another thought regarding the new live streaming from cameras (grpc) functionality, I really don't know if this will work with our current add-on configuration/solution, I don't have any devices to test this new functionality. |
|
fair point regarding the logging. Let's introduce it in a silent way and see what pops up in the issues and discussions. |
elcajon
added
bugfix
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cloudflared/rootfs/etc/s6-overlay/s6-rc.d/nginx/run (1)
16-18: Add error handling for jq command.While
set -ewill catch failures, explicit error handling would be better for debugging.Consider:
-JSON_CONF=$(jq --arg port $(bashio::core.port) \ - '({port: $port})' \ - /data/options.json) +if ! JSON_CONF=$(jq --arg port $(bashio::core.port) \ + '({port: $port})' \ + /data/options.json); then + bashio::log.error "Failed to process JSON configuration" + exit 1 +fi.github/renovate.json (1)
29-34: Consider adding documentation for the expected comment format.To help maintainers, consider adding a comment block above the regex manager configuration showing the expected format:
{ + // Matches version information from build script comments + // Expected format: + // # renovate: datasource=<source> depName=<name> versioning=<optional> ARG <name>_version="<version>" + // Example: + // # renovate: datasource=github-releases depName=nginx/nginx versioning=loose ARG nginx_version="1.26.2" "customType": "regex", "fileMatch": ["/rootfs/build.sh$"],
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/renovate.json(1 hunks)cloudflared/rootfs/build.sh(1 hunks)cloudflared/rootfs/etc/nginx/template/nginx.conf.gtpl(1 hunks)cloudflared/rootfs/etc/s6-overlay/s6-rc.d/nginx/finish(1 hunks)cloudflared/rootfs/etc/s6-overlay/s6-rc.d/nginx/run(1 hunks)cloudflared/rootfs/etc/s6-overlay/s6-rc.d/nginx/type(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- cloudflared/rootfs/etc/s6-overlay/s6-rc.d/nginx/type
🔇 Additional comments (5)
cloudflared/rootfs/build.sh (1)
30-32: LGTM! Version management looks good.Good practices observed:
- Version pinning with renovate integration
- Using --no-cache flag for installation
Let's verify if this is the latest stable version:
✅ Verification successful
✓ Nginx version 1.26.2-r4 verified as current
The specified version matches the latest stable version available in Alpine Linux repositories.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check latest nginx version in Alpine curl -s https://dl-cdn.alpinelinux.org/alpine/latest-stable/main/x86_64/APKINDEX.tar.gz | tar -xz -C /tmp && cat /tmp/APKINDEX | grep -A1 "P:nginx"Length of output: 2139
cloudflared/rootfs/etc/nginx/template/nginx.conf.gtpl (2)
23-25: Consider adding access control for the proxy.The proxy is listening on port 8321 without any access control. While this might be intended for internal use, consider adding IP-based restrictions for additional security.
Consider adding:
listen 8321; +allow 127.0.0.1; +deny all; proxy_buffering off;
36-47: Verify log streaming configuration.The configuration for log streaming looks correct with:
- Proper header handling
- Disabled caching and buffering
- Content-Type set for event-stream
Let's verify if there are any other log-related endpoints that might need similar handling:
✅ Verification successful
Log streaming configuration is properly isolated and complete
The
/api/hassio/.*/logsendpoint is the only log streaming endpoint in the configuration, and it has all the necessary settings for proper event streaming. No other endpoints require similar streaming configuration.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for log-related API endpoints in the codebase rg -l "api.*logs?" | grep -i "api"Length of output: 34
Script:
#!/bin/bash # Find all nginx configuration files echo "=== Nginx Configuration Files ===" fd -e conf -e gtpl -e template . | grep -i "nginx" # Search for location blocks in these files echo -e "\n=== Location Blocks with API Patterns ===" rg "location.*api" -A 5 # Search for hassio-related patterns echo -e "\n=== Hassio API Patterns ===" rg "/hassio/" -A 5Length of output: 1630
Script:
#!/bin/bash # View the complete nginx configuration echo "=== Complete Nginx Configuration ===" cat cloudflared/rootfs/etc/nginx/template/nginx.conf.gtpl # Find all location blocks echo -e "\n=== All Location Blocks ===" rg "^\s+location\s+" -A 5 cloudflared/rootfs/etc/nginx/template/nginx.conf.gtplLength of output: 2101
cloudflared/rootfs/etc/s6-overlay/s6-rc.d/nginx/finish (1)
7-27: LGTM! Robust error handling implementation.The script properly handles:
- Service exit codes
- Signal handling
- Container exit code management
- Appropriate logging
.github/renovate.json (1)
29-34: LGTM! Well-structured regex manager configuration.The regex configuration correctly captures version information from build script comments, with proper handling of optional versioning parameter.
|
Changing the categorization to bugfix to avoid issuing a minor version change |
Proposed Changes
This PR adds Nginx to this add-on and sets up a proxy to distinguish between HA and HA logs requests.
Thanks to @othorp (home-assistant/supervisor#5509 (comment)) for figuring out the correct proxy settings!
This PR will have no impact on currently running tunnels.
It runs an nginx proxy on port 8321 (inside the Cloudflare add-on Docker) with the correct settings to get log streaming working again.
Can be testet with remotely managed tunnels by changing the host from
homeassistant:8123tolocalhost:8321.I would like to merge this without a disruptive change to continue testing until a workaround/solution is available.
If it seems to be stable after some first testing.
We can think about changing the local managed tunnel uri to
localhost:8321in the future as well, which would have some impact on the current following setup logic:addon-cloudflared/cloudflared/rootfs/etc/s6-overlay/scripts/cloudflared-config.sh
Lines 217 to 232 in dc5aae2
For testing, clone the branch in /addons and then run the add-on locally with a tunnel token.
Related Issues
#744
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores
Infrastructure