Act as a local network reverse proxy #843
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis change removes the Nginx proxy workaround and all related configuration, scripts, and templates, replacing it with a built-in Caddy proxy implementation. The update introduces new configuration options, documentation, and runtime logic for enabling the Caddy proxy, and refactors build and workflow files to support the new architecture. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AddonConfig
participant PrepareScript
participant Caddy
participant Cloudflared
User->>AddonConfig: Set use_builtin_proxy (true/false)
AddonConfig->>PrepareScript: Pass config options
PrepareScript->>PrepareScript: If use_builtin_proxy true
alt use_builtin_proxy true
PrepareScript->>Caddy: Generate and validate Caddyfile
PrepareScript->>Caddy: Start Caddy service
PrepareScript->>Cloudflared: Configure ingress to Caddy
else use_builtin_proxy false
PrepareScript->>PrepareScript: Skip Caddy, mark no_built_in_proxy
PrepareScript->>Cloudflared: Configure ingress to Home Assistant/additional hosts directly
end
Cloudflared->>User: Tunnel connections as configured
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing touches🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
brenner-tobias
added
new-feature
felipecrs
changed the title
felipecrs
changed the title
|
@brenner-tobias, @elcajon, this PR is ready to review. I would like to suggest the following approach:
|
felipecrs
requested review from
brenner-tobias and
elcajon
and removed request for
elcajon
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
cloudflared/rootfs/etc/s6-overlay/s6-rc.d/caddy/run (1)
11-11: Consider adding Caddyfile validation before starting Caddy.While the current implementation is functional, consider adding a validation step to ensure the Caddyfile exists and is valid before starting Caddy.
+# Validate Caddyfile exists +if [[ ! -f /etc/caddy/Caddyfile ]]; then + bashio::log.error "Caddyfile not found at /etc/caddy/Caddyfile" + exit 1 +fi + bashio::log.info "Starting Caddy..." exec env XDG_DATA_HOME=/data caddy run --config /etc/caddy/Caddyfilecloudflared/DOCS.md (3)
132-134: Fix grammar and enhance clarity.The static analysis tools identified grammar issues with "an unified access" - it should be "a unified access" since "unified" starts with a consonant sound.
-When `use_builtin_proxy` is enabled, you can also add `internalOnly: true` to a -hostname to only allow access to it from within your local network. When this -service is accessed from Cloudflare, it will return a 403 Forbidden. +When `use_builtin_proxy` is enabled, you can also add `internalOnly: true` to a +hostname to only allow access to it from within your local network. When this +service is accessed via Cloudflare, it will return a 403 Forbidden.
227-230: Fix grammar issues and improve readability.Multiple grammar issues need correction based on static analysis:
- "an unified access" should be "a unified access"
- Verb tense inconsistency with "you saved"
-If enabled, the connection to Home Assistant and additional hosts will be made -through the built-in Caddy proxy. This works around issues with live logs ([#744](https://github.com/brenner-tobias/addon-cloudflared/discussions/744)) -and allows an unified access to Home Assistant and additional hosts even within -your local network. +If enabled, the connection to Home Assistant and additional hosts will be made +through the built-in Caddy proxy. This works around issues with live logs ([#744](https://github.com/brenner-tobias/addon-cloudflared/discussions/744)) +and allows a unified access to Home Assistant and additional hosts even within +your local network.🧰 Tools
🪛 LanguageTool
[grammar] ~229-~229: Uncountable nouns are usually not used with an indefinite article. Use simply “unified access”.
Context: ...loudflared/discussions/744)) and allows an unified access to Home Assistant and additional hosts ...(A_UNCOUNTABLE)
271-276: Fix verb tense and improve flow.The phrase "you saved a lot of time" should use future tense to match the context of benefits the user will receive.
-Congratulations! You are now using the built-in proxy to access your Home -Assistant instance and additional hosts locally, through an unified URL without -having to go swap between internal and external URLs. Also, you saved a lot of -time by not having to set up a reverse proxy like Nginx Proxy Manager yourself, -including handling the HTTPS certificates and HTTP to HTTPS redirection. +Congratulations! You are now using the built-in proxy to access your Home +Assistant instance and additional hosts locally, through a unified URL without +having to swap between internal and external URLs. Also, you will save a lot of +time by not having to set up a reverse proxy like Nginx Proxy Manager yourself, +including handling the HTTPS certificates and HTTP to HTTPS redirection.🧰 Tools
🪛 LanguageTool
[misspelling] ~272-~272: Use “a” instead of ‘an’ if the following word doesn’t start with a vowel sound, e.g. ‘a sentence’, ‘a university’.
Context: ...e and additional hosts locally, through an unified URL without having to go swap b...(EN_A_VS_AN)
[uncategorized] ~273-~273: This verb may not be in the correct tense. Consider changing the tense to fit the context better.
Context: ...n internal and external URLs. Also, you saved a lot of time by not having to set up a...(AI_EN_LECTOR_REPLACEMENT_VERB_TENSE)
cloudflared/rootfs/etc/s6-overlay/s6-rc.d/caddy/finish (1)
24-25: Fix spacing inconsistency in redirection.There's inconsistent spacing around the redirection operator. The codebase seems to prefer no space before
>.- echo $((128 + exit_code_signal)) >/run/s6-linux-init-container-results/exitcode + echo $((128 + exit_code_signal)) > /run/s6-linux-init-container-results/exitcode
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
.github/renovate.json(1 hunks).github/workflows/ci.yaml(0 hunks).github/workflows/deploy.yaml(0 hunks)cloudflared/DOCS.md(2 hunks)cloudflared/Dockerfile(3 hunks)cloudflared/build.yaml(0 hunks)cloudflared/config.yaml(1 hunks)cloudflared/rootfs/build.sh(2 hunks)cloudflared/rootfs/etc/caddy/Caddyfile.gtpl(1 hunks)cloudflared/rootfs/etc/nginx/includes/mime.types(0 hunks)cloudflared/rootfs/etc/nginx/includes/proxy_params.conf(0 hunks)cloudflared/rootfs/etc/nginx/includes/server_params.conf(0 hunks)cloudflared/rootfs/etc/nginx/nginx.conf(0 hunks)cloudflared/rootfs/etc/nginx/template/homeassistant.gtpl(0 hunks)cloudflared/rootfs/etc/s6-overlay/s6-rc.d/caddy/finish(1 hunks)cloudflared/rootfs/etc/s6-overlay/s6-rc.d/caddy/run(1 hunks)cloudflared/rootfs/etc/s6-overlay/s6-rc.d/cloudflared/finish(0 hunks)cloudflared/rootfs/etc/s6-overlay/s6-rc.d/init-cloudflared-config/up(0 hunks)cloudflared/rootfs/etc/s6-overlay/s6-rc.d/nginx/run(0 hunks)cloudflared/rootfs/etc/s6-overlay/s6-rc.d/nginx/type(0 hunks)cloudflared/rootfs/etc/s6-overlay/s6-rc.d/prepare/run.sh(12 hunks)cloudflared/rootfs/etc/s6-overlay/s6-rc.d/prepare/up(1 hunks)cloudflared/rootfs/run.sh(4 hunks)cloudflared/translations/en.yaml(1 hunks)
💤 Files with no reviewable changes (12)
- cloudflared/rootfs/etc/s6-overlay/s6-rc.d/init-cloudflared-config/up
- cloudflared/rootfs/etc/nginx/nginx.conf
- .github/workflows/ci.yaml
- .github/workflows/deploy.yaml
- cloudflared/rootfs/etc/s6-overlay/s6-rc.d/nginx/type
- cloudflared/rootfs/etc/s6-overlay/s6-rc.d/nginx/run
- cloudflared/rootfs/etc/s6-overlay/s6-rc.d/cloudflared/finish
- cloudflared/rootfs/etc/nginx/includes/server_params.conf
- cloudflared/rootfs/etc/nginx/includes/proxy_params.conf
- cloudflared/rootfs/etc/nginx/template/homeassistant.gtpl
- cloudflared/rootfs/etc/nginx/includes/mime.types
- cloudflared/build.yaml
🧰 Additional context used
🧠 Learnings (1)
cloudflared/translations/en.yaml (1)
Learnt from: elcajon
PR: brenner-tobias/addon-cloudflared#729
File: cloudflared/config.yaml:22-25
Timestamp: 2024-10-28T07:10:00.103Z
Learning: In 'cloudflared/config.yaml', use translations for port descriptions to maintain consistency with other configuration descriptions, as per Home Assistant's developer documentation.
🪛 LanguageTool
cloudflared/DOCS.md
[grammar] ~229-~229: Uncountable nouns are usually not used with an indefinite article. Use simply “unified access”.
Context: ...loudflared/discussions/744)) and allows an unified access to Home Assistant and additional hosts ...
(A_UNCOUNTABLE)
[misspelling] ~272-~272: Use “a” instead of ‘an’ if the following word doesn’t start with a vowel sound, e.g. ‘a sentence’, ‘a university’.
Context: ...e and additional hosts locally, through an unified URL without having to go swap b...
(EN_A_VS_AN)
[uncategorized] ~273-~273: This verb may not be in the correct tense. Consider changing the tense to fit the context better.
Context: ...n internal and external URLs. Also, you saved a lot of time by not having to set up a...
(AI_EN_LECTOR_REPLACEMENT_VERB_TENSE)
🔇 Additional comments (29)
cloudflared/rootfs/run.sh (3)
2-2: Good addition of shellcheck directive.Adding the shellcheck directive enables proper bash syntax checking, which improves code quality and helps catch potential issues during development.
16-16: LGTM: Improved formatting consistency.The formatting improvements enhance readability while maintaining the same logical behavior.
Also applies to: 22-22, 31-31
42-43: Correct use of array expansion for different purposes.Good practice using
"${options[*]}"for logging (joins elements) and"${options[@]}"for command execution (preserves argument boundaries). This ensures proper argument handling while providing readable debug output.cloudflared/rootfs/etc/s6-overlay/s6-rc.d/caddy/run (1)
4-8: Good use of marker file pattern for conditional service execution.The logic correctly uses a marker file in
/dev/shmto conditionally disable the service and properly instructs s6-overlay not to restart usings6-svc -O .. This is a clean pattern for optional services..github/renovate.json (1)
9-9: Good simplification using predefined Renovate preset.Replacing custom regex managers with the
customManagers:dockerfileVersionspreset is a positive change that:
- Reduces maintenance overhead
- Leverages community-maintained patterns
- Aligns with the new explicit version build arguments mentioned in the broader changes
- More likely to stay current with Renovate updates
cloudflared/translations/en.yaml (2)
43-48: LGTM! Clear and informative description update.The translation correctly reflects the change from Nginx to Caddy proxy and provides clear benefits explanation for users. The description effectively communicates the purpose and advantages of the built-in proxy feature.
56-59: Good use of translations for port descriptions.The port descriptions are well-structured and follow Home Assistant's best practices for configuration translations. The removal of the "TCP port" suffix from the metrics interface makes the naming more consistent.
cloudflared/rootfs/build.sh (4)
8-8: Excellent addition of strict error handling.Adding
set -euximproves the build script's robustness by:
-e: Exit on any command failure-u: Exit on undefined variables-x: Print commands for debuggingThis will help catch build issues early and make troubleshooting easier.
10-24: Well-implemented architecture mapping and binary download.The architecture mapping correctly handles the differences between Home Assistant and Caddy naming conventions. The download implementation is secure and efficient:
- Uses HTTPS URLs from official repositories
- Pipes directly to tar extraction
- Extracts only the needed binary file
42-42: Good improvement with quiet download flag.Adding the
-qflag to wget reduces verbose output during builds, making the logs cleaner while maintaining functionality.
51-53: Good defensive programming withrm -f.Using
rm -finstead ofrmprevents build failures if the legacy service files don't exist, making the build more robust across different base image versions.cloudflared/rootfs/etc/caddy/Caddyfile.gtpl (4)
1-5: Conditional local certificates configuration looks correct.The template properly handles the case where automatic HTTPS is disabled by enabling local certificates. This is appropriate for scenarios where port 443 isn't exposed externally.
17-20: Internal TLS endpoint configuration is appropriate.The localhost endpoint with internal TLS and 407 response code correctly indicates proxy authentication is required. This is a standard HTTP status code for this scenario.
44-50: Catch-all configuration is well-structured.The port 80 catch-all properly handles both configured and unconfigured scenarios with appropriate HTTP status codes (proxy vs 404).
6-16:❓ Verification inconclusive
Consider the security implications of TLS skip verification.
The configuration uses
tls_insecure_skip_verifyfor HTTPS backend connections. While this may be necessary for internal services with self-signed certificates, it poses security risks.Please verify that this is the intended behavior and document the security implications. Consider:
- Whether certificate validation can be enabled for specific backends
- If users should be warned about the security trade-offs
- Whether this should be configurable per host
🏁 Script executed:
#!/bin/bash # Search for documentation about TLS skip verification in the codebase rg -i "tls.*skip.*verify|insecure.*tls" --type mdLength of output: 51
I couldn’t find any existing docs mentioning
tls_insecure_skip_verify, so let’s confirm where it’s used:
🏁 Script executed:
#!/bin/bash # Locate all occurrences of tls_insecure_skip_verify in the repo rg "tls_insecure_skip_verify"Length of output: 173
Document and Review TLS Insecure Skip Verification
I searched the entire repo and found only two occurrences of
tls_insecure_skip_verifyincloudflared/rootfs/etc/caddy/Caddyfile.gtpl. There’s no existing documentation for this setting. Please confirm that disabling certificate validation is intentional and consider:
- Adding documentation on the security implications (e.g., in SECURITY.md or README).
- Enabling full certificate verification for production backends.
- Exposing a per‐backend or environment flag to control
tls_insecure_skip_verify.- Warning users about the risks of skipping TLS certificate validation.
If this behavior is required (e.g., for internal self-signed certificates), please include a comment in the template explaining the decision.
cloudflared/config.yaml (3)
21-21: Good default value for built-in proxy option.Setting
use_builtin_proxy: trueas the default makes sense since this feature provides better functionality and resolves live log issues mentioned in the PR objectives.
36-36: Good addition of internalOnly field to schema.The
internalOnlyboolean field provides users with fine-grained control over which services should be accessible externally via Cloudflare. This aligns well with the security-focused design.
40-40: Schema entry for use_builtin_proxy is correctly defined.The schema properly defines the new option as an optional boolean, which matches the implementation in the options section.
cloudflared/DOCS.md (1)
234-276: Excellent comprehensive setup guide.The detailed step-by-step setup instructions for the built-in proxy are well-structured and provide clear guidance for users. The examples with specific IP addresses and domain names make it easy to understand. The inclusion of OpenWRT-specific instructions is particularly helpful.
🧰 Tools
🪛 LanguageTool
[misspelling] ~272-~272: Use “a” instead of ‘an’ if the following word doesn’t start with a vowel sound, e.g. ‘a sentence’, ‘a university’.
Context: ...e and additional hosts locally, through an unified URL without having to go swap b...(EN_A_VS_AN)
[uncategorized] ~273-~273: This verb may not be in the correct tense. Consider changing the tense to fit the context better.
Context: ...n internal and external URLs. Also, you saved a lot of time by not having to set up a...(AI_EN_LECTOR_REPLACEMENT_VERB_TENSE)
cloudflared/rootfs/etc/s6-overlay/s6-rc.d/caddy/finish (2)
8-10: Good proxy bypass mechanism.The check for
/dev/shm/no_built_in_proxyprovides a clean way to bypass Caddy service management when the built-in proxy is disabled. This aligns well with the flag set in the preparation script.
22-28:✅ Verification successful
Verify the exit code logic for signal handling.
The logic handles exit code 256 (indicating the service was killed by a signal) but the condition
if [[ "${exit_code_signal}" -eq 15 ]]specifically handles SIGTERM. This seems correct, but let's verify this is the intended behavior.
🏁 Script executed:
#!/bin/bash # Description: Verify s6 service signal handling patterns in similar finish scripts # Search for similar signal handling patterns in other s6 finish scripts fd -t f -n finish . | xargs grep -l "exit_code_signal" | head -5 | xargs catLength of output: 307
🏁 Script executed:
#!/bin/bash # Search for exit_code_signal usage in all s6 finish scripts under the s6-overlay directory grep -R "exit_code_signal" -n cloudflared/rootfs/etc/s6-overlayLength of output: 502
SIGTERM-only shutdown handling is correct
I searched all
s6-rc.d/*/finishscripts incloudflared/rootfs/etc/s6-overlayand only this one usesexit_code_signal. It explicitly checks for signal 15 (SIGTERM) to invokehalt, which matches the intended graceful-shutdown behavior. No changes are needed here.cloudflared/Dockerfile (3)
9-12: Excellent version management with renovate integration.The use of renovate annotations for automatic dependency updates is a best practice. The version pinning for both Caddy and Cloudflared ensures reproducible builds while allowing automated updates.
17-18: Good build process simplification.The change from
RUN ["/build.sh", "${BUILD_ARCH}"]toRUN ["/build.sh"]simplifies the build process. The build script now likely uses environment variables for architecture detection, which is cleaner.
1-1:❓ Verification inconclusive
Verify the fixed base image version is appropriate.
The Dockerfile now uses a fixed base image version instead of the dynamic
BUILD_FROM. While this simplifies the build process, ensure this base image version is compatible with all target architectures.
🏁 Script executed:
#!/bin/bash # Description: Check if the fixed base image supports all required architectures # Check the manifest for the base image to see supported architectures curl -s https://ghcr.io/v2/hassio-addons/base/manifests/17.2.5 | jq '.manifests[].platform'Length of output: 151
🏁 Script executed:
#!/bin/bash # Description: Retry fetching the manifest list with the proper Accept header curl -s \ -H "Accept: application/vnd.docker.distribution.manifest.list.v2+json" \ https://ghcr.io/v2/hassio-addons/base/manifests/17.2.5 \ | jq '.manifests[].platform'Length of output: 222
Verify fixed base image supports all required architectures
The Dockerfile now pins to
ghcr.io/hassio-addons/base:17.2.5. Please confirm this tag provides all the platforms you target:
- Run locally (or via your CI)
docker manifest inspect ghcr.io/hassio-addons/base:17.2.5- Verify the returned “Platform” entries cover every architecture you need (e.g., amd64, arm64, armv7, etc.)
cloudflared/rootfs/etc/s6-overlay/s6-rc.d/prepare/run.sh (5)
236-240: Correct ingress routing logic for built-in proxy.The conditional routing logic properly directs traffic through the Caddy proxy when enabled, using localhost domains that correspond to the host entries added later in the script.
255-259: Good validation for internalOnly flag.The validation ensures that
internalOnlycan only be used with the built-in proxy, which makes sense since this feature requires Caddy's conditional access control.
349-402: Excellent Caddy configuration implementation.The
configureCaddyfunction is well-structured with:
- Proper port exposure detection via supervisor API
- Conditional HTTPS based on port configuration
- Template-based Caddyfile generation with validation
- Host entry management for internal communication
The use of the supervisor API to detect port exposure is particularly smart for automatic HTTPS determination.
388-393: Robust configuration validation process.The use of both
caddy fmtandcaddy validateensures the generated Caddyfile is both properly formatted and syntactically correct before deployment.
352-365:❓ Verification inconclusive
Validate the supervisor API port detection logic.
The logic checks if port 443 is exposed to determine auto HTTPS, but let's verify this API endpoint structure and ensure proper error handling.
🏁 Script executed:
#!/bin/bash # Description: Verify the supervisor API endpoint structure for addon info # Check the supervisor API documentation or examples for the expected response structure rg -A 10 -B 5 "addons/self/info" . || echo "No local examples found" # Also search for similar supervisor API usage patterns rg -A 5 "supervisor/addons" .Length of output: 2223
Verify Supervisor API network port detection and add error handling
We couldn’t find any local examples for
GET http://supervisor/addons/self/info, so please:
- Confirm that the JSON response indeed includes
.data.network["443/tcp"]set to the host port (e.g."443").- Ensure the
curl | jq | greppipeline handles:
- HTTP errors or timeouts (consider adding
--max-timeand checking HTTP status).- Missing or malformed
.data.networkentries.- JSON parsing failures—log a warning or error if detection fails.
- Default
auto_https=falseon any error or unexpected response to avoid enabling HTTPS erroneously.File: cloudflared/rootfs/etc/s6-overlay/s6-rc.d/prepare/run.sh (L352-365)
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
cloudflared/DOCS.md (1)
132-147: 🛠️ Refactor suggestionAdd an example for the new
internalOnlyflag in theadditional_hostssectionThe docs describe how to use
internalOnly: true(lines 132–134) but the example at lines 139–147 doesn’t include it. Adding a concrete snippet will help users understand and adopt this new flag.--- a/cloudflared/DOCS.md +++ a/cloudflared/DOCS.md @@ additional_hosts example (around line 140) - hostname: website.example.com service: http://192.168.1.3:8080 disableChunkedEncoding: true + - hostname: internal-service.example.local + service: http://192.168.1.100 + internalOnly: true
🧹 Nitpick comments (2)
cloudflared/DOCS.md (2)
227-230: Refine the wording for theuse_builtin_proxyoptionLines 227–230 could be more concise and fix the article usage on “unified access.” Consider rephrasing to active voice and dropping “a”:
- If enabled, the connection to Home Assistant and additional hosts will be made - through the built-in Caddy proxy. This works around issues with live logs ([#744]) - and allows a unified access to Home Assistant and additional hosts even within - your local network. + If enabled, connections to Home Assistant and additional hosts are routed through + the built-in Caddy proxy. This works around live log issues ([#744]) and + provides unified access (internally and externally) to your services.🧰 Tools
🪛 LanguageTool
[grammar] ~229-~229: Uncountable nouns are usually not used with an indefinite article. Use simply “unified access”.
Context: ...loudflared/discussions/744)) and allows a unified access to Home Assistant and additional hosts ...(A_UNCOUNTABLE)
271-276: Polish the concluding sentence for tense and clarityThe closing paragraph (lines 271–276) should use present tense and tighten phrasing:
- Congratulations! You are now using the built-in proxy to access your Home - Assistant instance and additional hosts locally, through a unified URL without - having to swap between internal and external URLs. Also, you saved a lot of - time by not having to set up a reverse proxy like Nginx Proxy Manager yourself, - including handling the HTTPS certificates and HTTP to HTTPS redirection. + Congratulations! You are now using the built-in proxy to access your Home + Assistant instance and additional hosts locally through a unified URL without + having to swap between internal and external URLs. You also save a lot of + time by not having to set up a reverse proxy like Nginx Proxy Manager yourself, + including managing HTTPS certificates and HTTP-to-HTTPS redirection.🧰 Tools
🪛 LanguageTool
[uncategorized] ~273-~273: This verb may not be in the correct tense. Consider changing the tense to fit the context better.
Context: ...n internal and external URLs. Also, you saved a lot of time by not having to set up a...(AI_EN_LECTOR_REPLACEMENT_VERB_TENSE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cloudflared/DOCS.md(2 hunks)cloudflared/rootfs/etc/caddy/Caddyfile.gtpl(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cloudflared/rootfs/etc/caddy/Caddyfile.gtpl
🧰 Additional context used
🪛 LanguageTool
cloudflared/DOCS.md
[grammar] ~229-~229: Uncountable nouns are usually not used with an indefinite article. Use simply “unified access”.
Context: ...loudflared/discussions/744)) and allows a unified access to Home Assistant and additional hosts ...
(A_UNCOUNTABLE)
[uncategorized] ~273-~273: This verb may not be in the correct tense. Consider changing the tense to fit the context better.
Context: ...n internal and external URLs. Also, you saved a lot of time by not having to set up a...
(AI_EN_LECTOR_REPLACEMENT_VERB_TENSE)
🔇 Additional comments (5)
cloudflared/DOCS.md (5)
234-242: Port exposure instructions look goodThe steps to expose ports 80/tcp, 443/tcp, and 443/udp (lines 234–242) are clear and correct for enabling HTTP, HTTPS, and HTTP/3.
243-250: Local DNS configuration steps are accurateThe guidance on setting up your DNS server to resolve hostnames to the Home Assistant IP (lines 243–250) is detailed and actionable.
252-258: DNS resolution verification is solidThe instructions for using
nslookupto confirm local resolution (lines 252–258) are clear and necessary.
259-263: Accessing services via browser is well explainedLines 259–263 correctly describe how to access your services once DNS is configured.
265-270: Reiteration of theinternalOnlyflag is helpfulRepeating the internal-only guidance in the local access steps (lines 265–270) reinforces the new feature—this duplication is beneficial for clarity.
brenner-tobias
left a comment
brenner-tobias
left a comment
There was a problem hiding this comment.
Thanks a lot, this is great work.
I am only able to review the docs for now, so please find my first comments here
|
Looking at the discussion here and in #908, I just switched the service of my HA tunnel back to using Cloudflare without NGINX ("homeassistant.local:8123") and checked the live logs. They seem to work fine, which indicates for me that there might have been some changes within HA or Cloudflare that potentially fixed our issues. How are your observations on this @felipecrs, @elcajon, @Christalsoldier? If the live log works without any additional reverse proxy, we should discuss if keeping one (NGINX or caddy) within this add-on still makes sense, so happy to hear your thoughts. |
I also checked this today and found that the live logs work without a proxy. - also not sure why This PR introduces functionality using Caddy, beyond the current NGINX implementation. Despite the fact that I don't have a use case for the enhanced functionality, I have no objection to continue the development. I think we should just make sure that there is no breaking change. |
|
Generally, I think we should keep the add-on as simple as possible and therefor easy to maintain. So my initial thought is to remove all additional proxy services if they are not needed for "base" functionality and potentially create a sort of "sister" Caddy add-on in our HA repository, which includes the additional benefits. Having that said, it is certainly beneficial to have everything in one place, if the benefits are "worth it", not sure if they are though to be honest. I suggest @felipecrs to summarise all the reasons for Caddy to be directly included in this add-on. Also, let's think about a separate add-on and the down and up-sides of such an approach. |
Yes, that's in my todos now. |
…lared into caddy-internal-proxy
…-cloudflared into caddy-internal-proxy
…lared into caddy-internal-proxy
Proposed Changes
This is extremely work in progress and should not be reviewed. I just wanted to save my working before going to bed. But it's working!
The caddy server listens to port 80 and 443, is configured with all the same hosts as cloudflared itself and handles automatic HTTPs through HTTP challenge.
Without requiring any extra user configuration like tokens, email, and whatnot. User just have to choose to expose the add-on's 80/443, and then configure their internal DNS server to serve Home Assistant IP for the domains proxied.
With this, when user is inside home:
When outside home:
Without needing to learn anything new or having to use Nginx Proxy Manager.
Related Issues
Refs #292 (reply in thread)
TODOs
use_builtin_proxyonly works for locally managed tunnel (because otherwise there is not enough data to build the Caddyfile)/.well-known/acme-challenge/must not be blocked in security rulesinternalOnlySummary by CodeRabbit
New Features
Improvements
Removals