chore: Switch to vanilla Caddy, when rate limiting is disabled #33387
Conversation
pratapaprasanna
force-pushed
the
make/xcaddy/configurable
branch
from
b660a02
to
750ab73
Compare
pratapaprasanna
force-pushed
the
make/xcaddy/configurable
branch
from
750ab73
to
11f4b19
Compare
pratapaprasanna
changed the title
WalkthroughWalkthroughThe changes primarily focus on refining the configuration and setup processes for the Caddy server within the Appsmith deployment scripts. Key updates include handling the Changes
Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Additional Context UsedShellCheck (8)
Additional comments not posted (1)
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![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
Out of diff range and nitpick comments (2)
deploy/docker/fs/opt/appsmith/caddy-reconfigure.mjs (1)
Line range hint
266-284: Ensure proper error handling when writing to the Caddyfile.try { fs.mkdirSync(dirname(CaddyfilePath), { recursive: true }); fs.writeFileSync(CaddyfilePath, parts.join("\n")); spawnSync("/opt/caddy/caddy", ["fmt", "--overwrite", CaddyfilePath]); spawnSync("/opt/caddy/caddy", ["reload", "--config", CaddyfilePath]); } catch (error) { console.error("Error configuring Caddy:", error); }deploy/docker/fs/opt/appsmith/entrypoint.sh (1)
Line range hint
136-136: Use-ninstead of! -zfor better readability.if [[ -n ${maximum_heap} ]]; then export APPSMITH_JAVA_HEAP_ARG="-Xmx${maximum_heap}m" fi
deploy/docker/base.dockerfile
Outdated
| RUN set -o xtrace \ | ||
| && mkdir -p /opt/basecaddy \ | ||
| && version="$(curl --write-out '%{redirect_url}' 'https://github.com/caddyserver/caddy/releases/latest' | sed 's,.*/v,,')" \ | ||
| && curl --location "https://github.com/caddyserver/caddy/releases/download/v$version/caddy_${version}_linux_$(uname -m | sed 's/x86_64/amd64/; s/aarch64/arm64/').tar.gz" \ | ||
| | tar -xz -C /opt/basecaddy |
There was a problem hiding this comment.
Avoid masking return values by separating declaration and assignment.
RUN set -o xtrace \
&& mkdir -p /opt/basecaddy \
- && version="$(curl --write-out '%{redirect_url}' 'https://github.com/caddyserver/caddy/releases/latest' | sed 's,.*/v,,')" \
+ && curl --write-out '%{redirect_url}' 'https://github.com/caddyserver/caddy/releases/latest' | sed 's,.*/v,,') \
+ && version="${version}" \
&& curl --location "https://github.com/caddyserver/caddy/releases/download/v$version/caddy_${version}_linux_$(uname -m | sed 's/x86_64/amd64/; s/aarch64/arm64/').tar.gz" \
| tar -xz -C /opt/basecaddyCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| RUN set -o xtrace \ | |
| && mkdir -p /opt/basecaddy \ | |
| && version="$(curl --write-out '%{redirect_url}' 'https://github.com/caddyserver/caddy/releases/latest' | sed 's,.*/v,,')" \ | |
| && curl --location "https://github.com/caddyserver/caddy/releases/download/v$version/caddy_${version}_linux_$(uname -m | sed 's/x86_64/amd64/; s/aarch64/arm64/').tar.gz" \ | |
| | tar -xz -C /opt/basecaddy | |
| RUN set -o xtrace \ | |
| && mkdir -p /opt/basecaddy \ | |
| && curl --write-out '%{redirect_url}' 'https://github.com/caddyserver/caddy/releases/latest' | sed 's,.*/v,,') \ | |
| && version="${version}" \ | |
| && curl --location "https://github.com/caddyserver/caddy/releases/download/v$version/caddy_${version}_linux_$(uname -m | sed 's/x86_64/amd64/; s/aarch64/arm64/').tar.gz" \ | |
| | tar -xz -C /opt/basecaddy |
| const RATE_LIMIT_ENV = (process.env._APPSMITH_RATE_LIMIT) | ||
|
|
||
| if (RATE_LIMIT_ENV == "disabled") { | ||
| var RATE_LIMIT = undefined | ||
| } | ||
| else { | ||
| // Rate limit, numeric value defining the requests-per-second allowed. | ||
| var RATE_LIMIT = parseInt(process.env._APPSMITH_RATE_LIMIT || 100, 10) | ||
| } |
There was a problem hiding this comment.
Use const or let instead of var for block-scoped variables.
const RATE_LIMIT_ENV = (process.env._APPSMITH_RATE_LIMIT);
if (RATE_LIMIT_ENV == "disabled") {
- var RATE_LIMIT = undefined;
+ let RATE_LIMIT = undefined;
} else {
// Rate limit, numeric value defining the requests-per-second allowed.
- var RATE_LIMIT = parseInt(process.env._APPSMITH_RATE_LIMIT || 100, 10);
+ let RATE_LIMIT = parseInt(process.env._APPSMITH_RATE_LIMIT || 100, 10);
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| const RATE_LIMIT_ENV = (process.env._APPSMITH_RATE_LIMIT) | |
| if (RATE_LIMIT_ENV == "disabled") { | |
| var RATE_LIMIT = undefined | |
| } | |
| else { | |
| // Rate limit, numeric value defining the requests-per-second allowed. | |
| var RATE_LIMIT = parseInt(process.env._APPSMITH_RATE_LIMIT || 100, 10) | |
| } | |
| const RATE_LIMIT_ENV = (process.env._APPSMITH_RATE_LIMIT); | |
| if (RATE_LIMIT_ENV == "disabled") { | |
| let RATE_LIMIT = undefined; | |
| } else { | |
| // Rate limit, numeric value defining the requests-per-second allowed. | |
| let RATE_LIMIT = parseInt(process.env._APPSMITH_RATE_LIMIT || 100, 10); | |
| } |
| { | ||
| debug | ||
| admin 127.0.0.1:2019 | ||
| persist_config off | ||
| acme_ca_root /etc/ssl/certs/ca-certificates.crt | ||
| servers { | ||
| trusted_proxies static 0.0.0.0/0 | ||
| } | ||
| } | ||
|
|
||
| (file_server) { | ||
| file_server { | ||
| precompressed br gzip | ||
| disable_canonical_uris | ||
| } | ||
| } | ||
|
|
||
| (reverse_proxy) { | ||
| reverse_proxy { | ||
| to 127.0.0.1:{args[0]} | ||
| header_up -Forwarded | ||
| header_up X-Appsmith-Request-Id {http.request.uuid} | ||
| } | ||
| } | ||
|
|
||
| (all-config) { | ||
| log { | ||
| output stdout | ||
| } | ||
| skip_log /api/v1/health | ||
|
|
||
| # The internal request ID header should never be accepted from an incoming request. | ||
| request_header -X-Appsmith-Request-Id | ||
|
|
||
| # Ref: https://stackoverflow.com/a/38191078/151048 | ||
| # We're only accepting v4 UUIDs today, in order to not make it too lax unless needed. | ||
| @valid-request-id expression {header.X-Request-Id}.matches("(?i)^[0-9A-F]{8}-[0-9A-F]{4}-[4][0-9A-F]{3}-[89AB][0-9A-F]{3}-[0-9A-F]{12}$") | ||
| header @valid-request-id X-Request-Id {header.X-Request-Id} | ||
| @invalid-request-id expression !{header.X-Request-Id}.matches("(?i)^[0-9A-F]{8}-[0-9A-F]{4}-[4][0-9A-F]{3}-[89AB][0-9A-F]{3}-[0-9A-F]{12}$") | ||
| header @invalid-request-id X-Request-Id invalid_request_id | ||
| request_header @invalid-request-id X-Request-Id invalid_request_id | ||
|
|
||
| header { | ||
| -Server | ||
| Content-Security-Policy "frame-ancestors ${frameAncestorsPolicy}" | ||
| X-Content-Type-Options "nosniff" | ||
| X-Appsmith-Request-Id {http.request.uuid} | ||
| } | ||
|
|
||
| request_body { | ||
| max_size ${process.env.APPSMITH_CODEC_SIZE || 150}MB | ||
| } | ||
|
|
||
| handle { | ||
| root * {$WWW_PATH} | ||
| try_files /loading.html /index.html | ||
| import file_server | ||
| } | ||
| root * /opt/appsmith/editor | ||
| @file file | ||
| handle @file { | ||
| import file_server | ||
| skip_log | ||
| } | ||
|
|
||
| handle /static/* { | ||
| error 404 | ||
| } | ||
|
|
||
| handle /info { | ||
| root * /opt/appsmith | ||
| rewrite * /info.json | ||
| import file_server | ||
| } | ||
|
|
||
| @backend path /api/* /oauth2/* /login/* | ||
| handle @backend { | ||
| import reverse_proxy 8080 | ||
| } | ||
|
|
||
| handle /rts/* { | ||
| import reverse_proxy 8091 | ||
| } | ||
|
|
||
| redir /supervisor /supervisor/ | ||
| handle_path /supervisor/* { | ||
| import reverse_proxy 9001 | ||
| } | ||
|
|
||
| handle_errors { | ||
| respond "{err.status_code} {err.status_text}" {err.status_code} | ||
| header -Server | ||
| } | ||
| } | ||
|
|
||
| # We bind to http on 80, so that localhost requests don't get redirected to https. | ||
| :80 { | ||
| import all-config | ||
| } | ||
| `) |
There was a problem hiding this comment.
Consider refactoring the repeated Caddyfile configuration blocks to avoid duplication.
if (RATE_LIMIT == undefined) {
parts.push(`
{
debug
admin 127.0.0.1:2019
persist_config off
acme_ca_root /etc/ssl/certs/ca-certificates.crt
servers {
trusted_proxies static 0.0.0.0/0
}
}
...
`);
} else {
parts.push(`
{
debug
admin 127.0.0.1:2019
persist_config off
acme_ca_root /etc/ssl/certs/ca-certificates.crt
servers {
trusted_proxies static 0.0.0.0/0
}
order rate_limit before basicauth
}
...
`);
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if (RATE_LIMIT == undefined ) { | |
| parts.push(` | |
| { | |
| debug | |
| admin 127.0.0.1:2019 | |
| persist_config off | |
| acme_ca_root /etc/ssl/certs/ca-certificates.crt | |
| servers { | |
| trusted_proxies static 0.0.0.0/0 | |
| } | |
| } | |
| (file_server) { | |
| file_server { | |
| precompressed br gzip | |
| disable_canonical_uris | |
| } | |
| } | |
| (reverse_proxy) { | |
| reverse_proxy { | |
| to 127.0.0.1:{args[0]} | |
| header_up -Forwarded | |
| header_up X-Appsmith-Request-Id {http.request.uuid} | |
| } | |
| } | |
| (all-config) { | |
| log { | |
| output stdout | |
| } | |
| skip_log /api/v1/health | |
| # The internal request ID header should never be accepted from an incoming request. | |
| request_header -X-Appsmith-Request-Id | |
| # Ref: https://stackoverflow.com/a/38191078/151048 | |
| # We're only accepting v4 UUIDs today, in order to not make it too lax unless needed. | |
| @valid-request-id expression {header.X-Request-Id}.matches("(?i)^[0-9A-F]{8}-[0-9A-F]{4}-[4][0-9A-F]{3}-[89AB][0-9A-F]{3}-[0-9A-F]{12}$") | |
| header @valid-request-id X-Request-Id {header.X-Request-Id} | |
| @invalid-request-id expression !{header.X-Request-Id}.matches("(?i)^[0-9A-F]{8}-[0-9A-F]{4}-[4][0-9A-F]{3}-[89AB][0-9A-F]{3}-[0-9A-F]{12}$") | |
| header @invalid-request-id X-Request-Id invalid_request_id | |
| request_header @invalid-request-id X-Request-Id invalid_request_id | |
| header { | |
| -Server | |
| Content-Security-Policy "frame-ancestors ${frameAncestorsPolicy}" | |
| X-Content-Type-Options "nosniff" | |
| X-Appsmith-Request-Id {http.request.uuid} | |
| } | |
| request_body { | |
| max_size ${process.env.APPSMITH_CODEC_SIZE || 150}MB | |
| } | |
| handle { | |
| root * {$WWW_PATH} | |
| try_files /loading.html /index.html | |
| import file_server | |
| } | |
| root * /opt/appsmith/editor | |
| @file file | |
| handle @file { | |
| import file_server | |
| skip_log | |
| } | |
| handle /static/* { | |
| error 404 | |
| } | |
| handle /info { | |
| root * /opt/appsmith | |
| rewrite * /info.json | |
| import file_server | |
| } | |
| @backend path /api/* /oauth2/* /login/* | |
| handle @backend { | |
| import reverse_proxy 8080 | |
| } | |
| handle /rts/* { | |
| import reverse_proxy 8091 | |
| } | |
| redir /supervisor /supervisor/ | |
| handle_path /supervisor/* { | |
| import reverse_proxy 9001 | |
| } | |
| handle_errors { | |
| respond "{err.status_code} {err.status_text}" {err.status_code} | |
| header -Server | |
| } | |
| } | |
| # We bind to http on 80, so that localhost requests don't get redirected to https. | |
| :80 { | |
| import all-config | |
| } | |
| `) | |
| if (RATE_LIMIT == undefined) { | |
| parts.push(` | |
| { | |
| debug | |
| admin 127.0.0.1:2019 | |
| persist_config off | |
| acme_ca_root /etc/ssl/certs/ca-certificates.crt | |
| servers { | |
| trusted_proxies static 0.0.0.0/0 | |
| } | |
| ... | |
| `); | |
| } else { | |
| parts.push(` | |
| { | |
| debug | |
| admin 127.0.0.1:2019 | |
| persist_config off | |
| acme_ca_root /etc/ssl/certs/ca-certificates.crt | |
| servers { | |
| trusted_proxies static 0.0.0.0/0 | |
| } | |
| order rate_limit before basicauth | |
| ... | |
| `); | |
| } |
| setup_caddy() { | ||
| if [[ $_APPSMITH_RATE_LIMIT == "disabled" ]]; then | ||
| echo "rate limiting is disabled" | ||
| mv /opt/basecaddy/caddy /opt/caddy/caddy | ||
| fi | ||
| } |
There was a problem hiding this comment.
Double quote to prevent globbing and word splitting.
setup_caddy() {
if [[ "$_APPSMITH_RATE_LIMIT" == "disabled" ]]; then
echo "rate limiting is disabled"
mv /opt/basecaddy/caddy /opt/caddy/caddy
fi
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| setup_caddy() { | |
| if [[ $_APPSMITH_RATE_LIMIT == "disabled" ]]; then | |
| echo "rate limiting is disabled" | |
| mv /opt/basecaddy/caddy /opt/caddy/caddy | |
| fi | |
| } | |
| setup_caddy() { | |
| if [[ "$_APPSMITH_RATE_LIMIT" == "disabled" ]]; then | |
| echo "rate limiting is disabled" | |
| mv /opt/basecaddy/caddy /opt/caddy/caddy | |
| fi | |
| } |
…ake/xcaddy/configurable # Conflicts: # deploy/docker/fs/opt/appsmith/caddy-reconfigure.mjs
| exec /opt/caddy/caddy run --config "$TMP/Caddyfile" | ||
| exec "$_APPSMITH_CADDY" run --config "$TMP/Caddyfile" |
There was a problem hiding this comment.
Small nitpick, let's not lose the final newline. A final newline character at the end of files is usually recommended since some old tools have problem dealing with files that don't have a final newline character.
Ref:
There was a problem hiding this comment.
It is because of the vi editor I use I guess. I dont do that deliberately Let me take care going forward
| @@ -16,6 +16,6 @@ gzip --keep --force "$(basename "$WWW_PATH/index.html")" | |||
| popd | |||
|
|
|||
| # Caddy may already be running for the loading page. | |||
| /opt/caddy/caddy stop --config "$TMP/Caddyfile" || true | |||
| $_APPSMITH_CADDY stop --config "$TMP/Caddyfile" || true | |||
There was a problem hiding this comment.
| $_APPSMITH_CADDY stop --config "$TMP/Caddyfile" || true | |
| "$_APPSMITH_CADDY" stop --config "$TMP/Caddyfile" || true |
| @@ -217,4 +217,4 @@ function isCertExpired(path) { | |||
| const cert = new X509Certificate(fs.readFileSync(path, "utf-8")) | |||
| console.log(path, cert) | |||
| return new Date(cert.validTo) < new Date() | |||
| } | |||
| } | |||
There was a problem hiding this comment.
Let's not remove the final newline.
| } | |
| } | |
| const CaddyfilePath = process.env.TMP + "/Caddyfile" | ||
|
|
||
| // Rate limit environment. | ||
| const isRateLimitingDisabled = process.env.APPSMITH_RATE_LIMIT === "disabled" |
There was a problem hiding this comment.
Looks like I've unfortunately applied this condition in reverse. It's named isRateLimitingDisabled, but is actually used in later code like it's isRateLimitingEnabled. Let's rename it and change === to !==.
| init_loading_pages(){ | ||
| export XDG_DATA_HOME=/appsmith-stacks/data # so that caddy saves tls certs and other data under stacks/data/caddy | ||
| export XDG_CONFIG_HOME=/appsmith-stacks/configuration | ||
| mkdir -p "$XDG_DATA_HOME" "$XDG_CONFIG_HOME" | ||
| cp templates/loading.html "$WWW_PATH" | ||
| node caddy-reconfigure.mjs | ||
| /opt/caddy/caddy start --config "$TMP/Caddyfile" | ||
| $_APPSMITH_CADDY start --config "$TMP/Caddyfile" |
There was a problem hiding this comment.
| $_APPSMITH_CADDY start --config "$TMP/Caddyfile" | |
| "$_APPSMITH_CADDY" start --config "$TMP/Caddyfile" |
| @@ -447,13 +447,21 @@ runEmbeddedPostgres=1 | |||
| init_postgres || runEmbeddedPostgres=0 | |||
| } | |||
|
|
|||
| setup_caddy() { | |||
| if [[ "$APPSMITH_RATE_LIMIT" == "disabled" ]]; then | |||
There was a problem hiding this comment.
Quote the variable to prevent globbing and word splitting.
- if [[ "$APPSMITH_RATE_LIMIT" == "disabled" ]]; then
+ if [[ "${APPSMITH_RATE_LIMIT}" == "disabled" ]]; thenCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if [[ "$APPSMITH_RATE_LIMIT" == "disabled" ]]; then | |
| if [[ "${APPSMITH_RATE_LIMIT}" == "disabled" ]]; then |
| init_loading_pages(){ | ||
| export XDG_DATA_HOME=/appsmith-stacks/data # so that caddy saves tls certs and other data under stacks/data/caddy | ||
| export XDG_CONFIG_HOME=/appsmith-stacks/configuration | ||
| mkdir -p "$XDG_DATA_HOME" "$XDG_CONFIG_HOME" | ||
| cp templates/loading.html "$WWW_PATH" | ||
| node caddy-reconfigure.mjs | ||
| /opt/caddy/caddy start --config "$TMP/Caddyfile" | ||
| "$_APPSMITH_CADDY" start --config "$TMP/Caddyfile" |
There was a problem hiding this comment.
Quote the command to prevent globbing and word splitting.
- "$_APPSMITH_CADDY" start --config "$TMP/Caddyfile"
+ "${_APPSMITH_CADDY}" start --config "$TMP/Caddyfile"Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| "$_APPSMITH_CADDY" start --config "$TMP/Caddyfile" | |
| "${_APPSMITH_CADDY}" start --config "$TMP/Caddyfile" |
There was a problem hiding this comment.
Line 190 and 191 are still using hard-coded Caddy. Can we change them to use the env variable please?
Can we also test with if the container built with changes in this PR, starts when all Docker capabilities are dropped please?
There was a problem hiding this comment.
Yea my bad. Sorry
|
@pratapaprasanna, also, can we ensure the "Semantic PR" check passes on PRs please? It checks that PR titles should start with I'm fixing the title for this PR, please feel free to change if you disagree. Thanks! |
sharat87
changed the title
sharat87
changed the title
github-actions
bot
added
the
skip-changelog
| @@ -474,6 +482,7 @@ function capture_infra_details(){ | |||
|
|
|||
| # Main Section | |||
| print_appsmith_info | |||
| setup_caddy | |||
There was a problem hiding this comment.
Ensure setup_caddy is called before init_loading_pages to properly configure Caddy before starting it.
setup_caddy
init_loading_pages
print_appsmith_info
unset_unused_variablesThis sequence ensures that the Caddy server is configured before any pages are loaded, which is crucial for proper application behavior.
| setup_caddy() { | ||
| if [[ "$APPSMITH_RATE_LIMIT" == "disabled" ]]; then | ||
| export _APPSMITH_CADDY="/opt/caddy/caddy_vanilla" | ||
| else | ||
| export _APPSMITH_CADDY="/opt/caddy/caddy" | ||
| fi | ||
| } |
There was a problem hiding this comment.
Ensure consistent environment variable naming.
- if [[ "$APPSMITH_RATE_LIMIT" == "disabled" ]]; then
+ if [[ "${APPSMITH_RATE_LIMIT}" == "disabled" ]]; thenThis change ensures that the variable is properly quoted to prevent globbing and word splitting, aligning with best practices in shell scripting.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| setup_caddy() { | |
| if [[ "$APPSMITH_RATE_LIMIT" == "disabled" ]]; then | |
| export _APPSMITH_CADDY="/opt/caddy/caddy_vanilla" | |
| else | |
| export _APPSMITH_CADDY="/opt/caddy/caddy" | |
| fi | |
| } | |
| setup_caddy() { | |
| if [[ "${APPSMITH_RATE_LIMIT}" == "disabled" ]]; then | |
| export _APPSMITH_CADDY="/opt/caddy/caddy_vanilla" | |
| else | |
| export _APPSMITH_CADDY="/opt/caddy/caddy" | |
| fi | |
| } |
github-actions
bot
added
Bug
Depends-on: 33591
Fixes: #31997