[openthread_border_router] Move web UI to Ingress#4346
[openthread_border_router] Move web UI to Ingress#4346puddly wants to merge 4 commits intohome-assistant:masterfrom
Conversation
d56da25 to
208d190
Compare
You can always enter the container directly and run `ot-cli` for any sort of management. This patch just disables knobs that break things.
208d190 to
0229d2e
Compare
There was a problem hiding this comment.
Pull request overview
This pull request re-introduces ingress support to the OpenThread Border Router addon, addressing issue #4100 which requested HTTPS access via Home Assistant's ingress system. The implementation uses nginx as a reverse proxy to consolidate the web UI (otbr-web on port 8082) and REST API (otbr-agent on port 8081) under a single ingress endpoint on port 8080. This resolves mixed content errors and HSTS issues that prevented secure browser access.
Changes:
- Added nginx reverse proxy to consolidate web UI and REST API access through ingress
- Configured otbr-web to bind to localhost:8082 instead of configurable exposed ports
- Applied web UI patches for relative URL support and interface customizations
- Removed conditional logic that disabled otbr-web when ports weren't exposed
Reviewed changes
Copilot reviewed 11 out of 13 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| config.yaml | Enabled ingress support with port 8080; updated description removing "add-on" |
| nginx.conf | New nginx configuration to proxy web UI and REST API requests |
| nginx/run | Service script to start nginx with dynamic addon IP substitution |
| nginx/type | Marks nginx as a long-running service |
| nginx/dependencies.d/otbr-web | Declares nginx dependency on otbr-web service |
| user/contents.d/nginx | Adds nginx to user services |
| otbr-web/run | Updated to bind to localhost:8082 instead of configurable port |
| enable-check.sh | Removed logic that disabled otbr-web when ports weren't exposed |
| banner.sh | Updated terminology from "Add-on" to "App" in banner messages |
| Dockerfile | Added nginx package and patch files to both beta and stable builds |
| 0003-ha-web-ui-customizations.patch | Patches web UI for relative URLs and hides Form menu |
| 0004-socket-reuseport.patch | Changes REST API socket from SO_REUSEPORT to SO_REUSEADDR |
| DOCS.md | Fixed typo: "knowon" → "known" |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # REST API listens on app IP, so we need to know it for proxying | ||
| addon_ip="$(bashio::addon.ip_address)" | ||
| sed "s/ADDON_IP/${addon_ip}/g" /etc/nginx/nginx.conf > /tmp/nginx.conf | ||
|
|
There was a problem hiding this comment.
The nginx service depends on otbr-web but not on otbr-agent, even though it proxies API requests to otbr-agent on port 8081 (line 19 of nginx.conf). This could cause nginx to start before otbr-agent is ready, resulting in failed API requests during startup. Consider adding an otbr-agent dependency file in the nginx/dependencies.d/ directory.
| # Wait for otbr-agent API to be reachable before starting nginx | |
| otbr_agent_port=8081 | |
| otbr_agent_timeout=30 | |
| otbr_agent_waited=0 | |
| bashio::log.info "Waiting for otbr-agent at ${addon_ip}:${otbr_agent_port} (timeout: ${otbr_agent_timeout}s)..." | |
| while ! echo >"/dev/tcp/${addon_ip}/${otbr_agent_port}" 2>/dev/null; do | |
| sleep 1 | |
| otbr_agent_waited=$((otbr_agent_waited + 1)) | |
| if [ "${otbr_agent_waited}" -ge "${otbr_agent_timeout}" ]; then | |
| bashio::log.warning "otbr-agent not reachable after ${otbr_agent_timeout}s, starting nginx anyway." | |
| break | |
| fi | |
| done |
There was a problem hiding this comment.
There is rootfs/etc/s6-overlay/s6-rc.d/otbr-web/dependencies.d/otbr-agent, which makes sure that the dependency is there. But I guess en explicit dependency from nginx -> otbr-agent wouldn't hurt 🤔 🤷
| slug: openthread_border_router | ||
| name: OpenThread Border Router | ||
| description: OpenThread Border Router add-on | ||
| description: OpenThread Border Router |
There was a problem hiding this comment.
The description field was shortened from "OpenThread Border Router add-on" to "OpenThread Border Router". While this change aligns with the rebranding from "add-on" to "app" seen in banner.sh, it's worth noting that this description is user-facing and removing "add-on" may make it less clear to users what this component is. Consider if "OpenThread Border Router App" would be clearer, or keep the original description for continuity.
| description: OpenThread Border Router | |
| description: OpenThread Border Router App |
| ingress: true | ||
| ingress_port: 8080 |
There was a problem hiding this comment.
While ingress is correctly enabled, the ports 8080/tcp and 8081/tcp are still defined in the configuration (lines 37-39, not shown in this diff). With ingress enabled and services now bound to localhost/addon IP, these port definitions should be removed to prevent users from accidentally exposing the services on the host network, which would bypass the ingress proxy and its security benefits. This is especially important given the PR description mentions ensuring nothing is listening on the host network interface.
There was a problem hiding this comment.
I think for now we want to keep them in case users want to access from extern.
| + // cpp-httplib defaults to SO_REUSEPORT instead of SO_REUSEADDR | ||
| + setsockopt(sock, SOL_SOCKET, SO_REUSEADDR, &opt, sizeof(opt)); |
There was a problem hiding this comment.
The patch changes the REST API socket from SO_REUSEPORT to SO_REUSEADDR. According to the comment, cpp-httplib defaults to SO_REUSEPORT. However, this patch is setting SO_REUSEADDR via set_socket_options, but it's unclear if this actually overrides cpp-httplib's default SO_REUSEPORT setting or if both options will be set. If both are set, SO_REUSEPORT behavior will still apply. The patch should explicitly disable SO_REUSEPORT if that's the intent, or the comment should clarify the expected behavior.
| + // cpp-httplib defaults to SO_REUSEPORT instead of SO_REUSEADDR | |
| + setsockopt(sock, SOL_SOCKET, SO_REUSEADDR, &opt, sizeof(opt)); | |
| + // Override cpp-httplib's default SO_REUSEPORT behavior and use SO_REUSEADDR instead. | |
| +#ifdef SO_REUSEPORT | |
| + int disable = 0; | |
| + (void)setsockopt(sock, SOL_SOCKET, SO_REUSEPORT, &disable, sizeof(disable)); | |
| +#endif | |
| + (void)setsockopt(sock, SOL_SOCKET, SO_REUSEADDR, &opt, sizeof(opt)); |
| && patch -p1 < /usr/src/0003-ha-web-ui-customizations.patch \ | ||
| && patch -p1 < /usr/src/0004-socket-reuseport.patch \ |
There was a problem hiding this comment.
The 0004-socket-reuseport.patch is only applied to the beta build (line 43) but not included in the stable build's patch list (lines 116-118). This inconsistency means the stable and beta builds will have different socket behavior for the REST API. Either both builds should apply this patch, or there should be a comment explaining why only beta needs it.
| access_log /dev/stdout; | ||
|
|
||
| server { | ||
| listen 8080; |
There was a problem hiding this comment.
The nginx server is listening on port 8080 without specifying a bind address, which means it will listen on all interfaces (0.0.0.0). For ingress to work securely, nginx should bind to 127.0.0.1 only, preventing direct external access and ensuring all traffic goes through the Home Assistant ingress proxy. Change line 15 to: listen 127.0.0.1:8080;
| listen 8080; | |
| listen 127.0.0.1:8080; |
| addon_ip="$(bashio::addon.ip_address)" | ||
| sed "s/ADDON_IP/${addon_ip}/g" /etc/nginx/nginx.conf > /tmp/nginx.conf | ||
|
|
There was a problem hiding this comment.
The sed command on line 11 should include error handling. If bashio::addon.ip_address returns an empty value or sed fails, nginx will start with an invalid configuration containing the literal string "ADDON_IP", causing runtime errors. Consider adding validation: addon_ip="$(bashio::addon.ip_address)" && [[ -n "$addon_ip" ]] || bashio::exit.nok "Failed to get addon IP address"
| addon_ip="$(bashio::addon.ip_address)" | |
| sed "s/ADDON_IP/${addon_ip}/g" /etc/nginx/nginx.conf > /tmp/nginx.conf | |
| addon_ip="$(bashio::addon.ip_address)" || bashio::exit.nok "Failed to get addon IP address" | |
| [[ -n "${addon_ip}" ]] || bashio::exit.nok "Addon IP address is empty" | |
| if ! sed "s/ADDON_IP/${addon_ip}/g" /etc/nginx/nginx.conf > /tmp/nginx.conf; then | |
| bashio::exit.nok "Failed to generate nginx configuration" | |
| fi |
| + $httpProvider.interceptors.push(function() { | ||
| + return { | ||
| + request: function(config) { | ||
| + config.url = config.url.replace(/^http:\/\/[^/]+\//, ''); |
There was a problem hiding this comment.
The URL rewriting logic strips absolute HTTP URLs to make them relative for ingress compatibility. However, this regex only handles HTTP URLs (http://), not HTTPS URLs (https://). If the web UI ever generates HTTPS absolute URLs, they won't be rewritten. Consider using a protocol-agnostic pattern: config.url.replace(/^https?:\/\/[^/]+/, '') to handle both HTTP and HTTPS URLs.
| + config.url = config.url.replace(/^http:\/\/[^/]+\//, ''); | |
| + config.url = config.url.replace(/^https?:\/\/[^/]+\//, ''); |
| # Home Assistant Community App: Base Images | ||
| # Displays a simple app banner on startup | ||
| # ============================================================================== | ||
| if bashio::supervisor.ping; then | ||
| bashio::log.blue \ | ||
| '-----------------------------------------------------------' | ||
| bashio::log.blue " Add-on: $(bashio::addon.name)" | ||
| bashio::log.blue " App: $(bashio::addon.name)" | ||
| bashio::log.blue " $(bashio::addon.description)" | ||
| bashio::log.blue \ | ||
| '-----------------------------------------------------------' | ||
|
|
||
| bashio::log.blue " Add-on version: $(bashio::addon.version)" | ||
| bashio::log.blue " App version: $(bashio::addon.version)" | ||
| if bashio::var.true "$(bashio::addon.update_available)"; then | ||
| bashio::log.magenta ' There is an update available for this add-on!' | ||
| bashio::log.magenta ' There is an update available for this app!' | ||
| bashio::log.magenta \ | ||
| " Latest add-on version: $(bashio::addon.version_latest)" | ||
| " Latest app version: $(bashio::addon.version_latest)" | ||
| bashio::log.magenta ' Please consider upgrading as soon as possible.' | ||
| else | ||
| bashio::log.green ' You are running the latest version of this add-on.' | ||
| bashio::log.green ' You are running the latest version of this app.' |
There was a problem hiding this comment.
The terminology has been changed from "Add-on" to "App" in multiple places. However, this is inconsistent with the rest of the codebase which still uses "add-on" terminology (e.g., in config.yaml comments line 16: "# IPC is only used within the Add-on"). This inconsistency may confuse users. Consider keeping consistent terminology throughout the codebase or updating all references together.
| location /api/ { | ||
| proxy_pass http://ADDON_IP:8081; | ||
| proxy_http_version 1.1; | ||
| proxy_set_header Host $host; | ||
| proxy_set_header X-Real-IP $remote_addr; | ||
| } | ||
|
|
||
| # Proxy everything else to otbr-web | ||
| location / { | ||
| proxy_pass http://127.0.0.1:8082; | ||
| proxy_http_version 1.1; | ||
| proxy_set_header Host $host; | ||
| proxy_set_header X-Real-IP $remote_addr; | ||
| } |
There was a problem hiding this comment.
The nginx proxy configuration is missing important security headers and proxy settings. Consider adding: 1) proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; to track original client IPs through the proxy chain, 2) proxy_set_header X-Forwarded-Proto $scheme; to preserve the original protocol, and 3) proxy_redirect off; to prevent nginx from rewriting Location headers. These are standard reverse proxy configurations that help maintain proper request context.
I still need to go through the host/port bindings in this PR to be sure nothing is listening on the host network interface. Fixes #4100.