feat(openresty): activate brotli when the module is enabled, and fix gzip defaults - #13639
Open
Snrat wants to merge 8 commits into
Open
feat(openresty): activate brotli when the module is enabled, and fix gzip defaults#13639Snrat wants to merge 8 commits into
Snrat wants to merge 8 commits into
Conversation
Add a managed-file mechanism for http-context nginx directives, mirroring the existing one for conf/modules-enabled. A separate directory is required because load_module is a main-context directive, so modules-enabled is included at the top level of nginx.conf and cannot host http-context directives. Files carry a 1panel-http- prefix; anything else in the directory is left untouched. Writes are atomic via a temporary file plus rename, and the directory is snapshotted so a failed nginx -t can be rolled back. The mechanism is inert when conf/http.d does not exist, which is the case for OpenResty installations predating the directory.
Bring the embedded gzip template in line with how sites are actually served. It was previously dead code: nothing referenced gzip.conf, so the values never reached an installation. It is now embedded and used by the migration that follows. gzip_types was missing application/json, so JSON API responses were served uncompressed. Also add ld+json, text/xml, xhtml+xml, rss+xml, atom+xml, wasm, svg+xml and ttf/otf. Already compressed formats (images, woff2, archives) stay out on purpose. gzip_comp_level 6 -> 5, at the cost/ratio knee for gzip. gzip_proxied any, so that proxied responses are compressed regardless of their Cache-Control semantics. gzip_static is intentionally not enabled: nginx does not verify that a .gz file is newer than its source, so a stale artifact would be served indefinitely with no error.
Enabling ngx_brotli only emitted load_module, leaving the module loaded but inert: no response was ever brotli-encoded until the user added `brotli on` and `brotli_types` to nginx.conf by hand. The module is prebuilt into the OpenResty image and listed in the catalog, so the only missing step was the runtime configuration. Enabling the module now also writes its http-context directives to conf/http.d, and disabling or deleting it removes them. Removal matters: leaving `brotli on` behind after the .so is unloaded makes nginx fail to start on an unknown directive. Both directory sets are written before nginx -t runs, so nginx only ever observes a consistent state, and a failed check rolls back load_module files and runtime directives together. Runtime defaults are declared per module in a table, so other modules needing http-context configuration can be added without touching the reconcile logic. brotli_types matches gzip_types so both encoders cover the same content. brotli_comp_level is 5 rather than the nginx default of 6: level 5 reaches roughly gzip level 9 ratio at a fraction of the cost, while 6 is tuned for static assets and is too expensive for dynamic responses. brotli_static is deliberately omitted, for the same reason gzip_static is: nginx does not verify that a precompressed artifact is newer than its source, so a stale file would be served indefinitely with no error. Installations without conf/http.d keep the previous behaviour instead of failing.
Upgrades deliberately preserve the user's nginx.conf, so corrected gzip defaults shipped with a new OpenResty version never reach existing installations. Rewrite the values in place during upgrade, but only when the block is provably untouched. The rewrite requires every gzip directive to match the factory values byte for byte, with none missing, none added and none duplicated. Any deviation means the user tuned compression, and their configuration is left alone. gzip stays in the http block of nginx.conf rather than moving to an included file: nginx rejects a duplicate gzip directive across contexts, and the compression settings page reads and writes these same keys in nginx.conf, so a relocated block would be reintroduced on the next save and break nginx -t. The config parser is not used either. Its dumper regenerates the whole file, drops standalone comments and reorders proxy includes, which would be destructive on a user's main config. Lines are edited individually so everything outside the gzip block stays byte-identical. The rewrite is idempotent, and a failed nginx -t restores the previous file. A failure is logged as a warning instead of failing the upgrade.
The form stripped the unit suffix when reading a directive and then always appended a fixed one when saving, so the unit was silently reinterpreted. A config carrying `gzip_min_length 512;`, meaning 512 bytes, was read as 512 and written back as `512k`, inflating the threshold by 1024 and effectively disabling compression for every response under 512 KB. The same applied to client_header_buffer_size and client_max_body_size, where the value grew by a factor of 1024 in the opposite, riskier direction. Remember the unit that was read and write it back unchanged, defaulting to the previous suffix only when the directive carries no unit information. The input suffix now shows the unit actually in use instead of a hardcoded label. Also fix the value parsing itself: `Number(value.match(/\d+/g))` coerces a multi-number match to NaN, so a directive such as `gzip_buffers 4 16k` would blank the field. Take the first captured number instead.
Brotli could be enabled as a module but never configured from the panel, so its behaviour was invisible and unchangeable without editing nginx.conf by hand. The section appears only once the module is enabled and built, since the directives are rejected by nginx while the module is not loaded. Values are read from and written to the panel-managed http.d file rather than nginx.conf, so they are removed together with the module. brotli_types stays out of the form on purpose: it is kept aligned with gzip_types so both encoders cover the same content, and exposing it would invite the two lists to drift apart. Saving reuses the existing scope endpoint with a dedicated brotli scope, which keeps the managed file as the single source of truth instead of duplicating the values into nginx.conf.
Manual builds and upgrades disagreed on when a full OpenResty image rebuild is required. `executeNginxModuleBuild` used `staticNginxBuildRequired`, which also treated a non-empty `RESTY_CONFIG_OPTIONS_MORE` in .env as a reason to rebuild, while `buildNginx` looked only at the module list. The env value is derived state, not an input: `configureStaticNginxModules` rewrites it from the current module list, and every build path calls that function before building. With no static module enabled it writes an empty string, so the rebuild the latch triggered ran with an empty option list and could only reproduce the image it started from — up to 120 minutes of build time to arrive back where it began. Decide on the module list alone, which is what the upgrade path already did. An install that genuinely has an enabled static module is unaffected: both predicates already agreed in that case. Leftover values are still cleared, by `configureStaticNginxModules` on the next build or upgrade.
Module state written before build modes existed carries no buildMode. validateNginxModuleBuildMode rejects the empty value, which fails loadNginxModules and with it every module operation and the upgrade itself — the whole module subsystem, not just the static feature. Infer the missing value from what the install can actually do instead: dynamic when the builder and catalog are present, static when the compose file still has a build section and build/Dockerfile to recompile the image. Builds follow the same principle. Asking a pre-dynamic install to build a module used to return "the installed OpenResty version does not support dynamic module builds", which is a dead end: these versions produce modules by compiling them into the image, and they still can. Such a build is now retargeted to the static path, with --add-dynamic-module rewritten back to --add-module and =dynamic switches reduced to their plain form. The error is kept only for installs that reference a prebuilt image and genuinely cannot compile anything, and it now says so and points at the upgrade. The retarget applies to a copy that drives one build and is never persisted, so the catalog stays authoritative and modules return to dynamic once the install gains a builder. Verified end to end against 1.27.1.2-5-1-focal, which ships no Dockerfile.modules and no module.catalog.json: ngx_brotli compiles into the image, nginx -t accepts the brotli directives with no load_module present, and the server responds with Content-Encoding: br.
wanghe-fit2cloud
marked this pull request as draft
August 26, 2026 06:31
Snrat
marked this pull request as ready for review
August 26, 2026 08:32
Member
|
如果用户已经有 |
Contributor
Author
感谢指出,目前这个场景确实没有进行覆盖。 appstore 的 upgrade.sh 会在 nginx.conf 中幂等插入 http.d 的 include 并创建空目录,但升级过程不产生任何托管文件,且 ngx_brotli 会针对新版本重新编译并自测——用户的手写配置在升级后照常生效。 如果用户先前手动执行过相关配置,说明用户已经成功启用了 br 模块,否则 nginx 会直接抛出报错。在这种情况下用户执行 Openrestry 升级时,用户不会遇到报错信息。
目前考虑针对此场景在压缩配置页面内增加配置文件扫描,识别用户是否已经手动启用 brotli 功能,并且要求用户执行相关操作以规避潜在的错误。 或者请问是否有其他的思路来解决此问题?此场景的影响面较小,因此不会影响到绝大部分用户。 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Enabling
ngx_brotlionly ever emittedload_module. The module was loadedbut inert: no response was brotli-encoded until the user added
brotli onandbrotli_typesto nginx.conf by hand. The module is prebuilt into the OpenRestyimage and listed in the catalog, so the only missing step was the runtime
configuration.
Depends on 1Panel-dev/appstore#9230, which adds the
conf/http.dinclude.Merge that first. This PR probes for the directory and keeps the previous
behaviour when it is absent, so merging out of order degrades silently rather
than breaking.
Why a separate directory
load_moduleis main-context;brotli onis http-context. They cannot share afile, and
conf/modules-enabledis included at the top level of nginx.conf.Runtime directives therefore go to
conf/http.d, managed with the samesnapshot/rollback discipline as the existing module files.
Files carry a
1panel-http-prefix; anything else in the directory is leftalone. Writes are atomic (temp file + rename).
Runtime defaults are declared per module in a table, so another module needing
http-context configuration can be added without touching the reconcile logic.
Ordering guarantee
Both directory sets are written before
nginx -truns, so nginx only everobserves a consistent state, and a failed check rolls back
load_modulefilesand runtime directives together.
This matters: leaving
brotli onbehind after the.sois unloaded makesnginx fail to start with
unknown directive "brotli". The reverse order isequally fatal.
gzip
The embedded
gzip.conftemplate was dead code — nothing referenced it, so itsvalues never reached an installation. It is now embedded and used.
Upgrades deliberately preserve the user's nginx.conf, so corrected defaults
shipped with a new OpenResty version would never reach existing installations.
The upgrade now rewrites the gzip values in place, but only when the block
is provably untouched: every directive must match the factory values byte for
byte, with none missing, added or duplicated. Any deviation means the user
tuned compression and their configuration is left alone.
gzip stays in nginx.conf rather than moving to
http.d: nginx rejects aduplicate
gzipdirective across contexts, and the compression settings pagereads and writes these same keys in nginx.conf, so a relocated block would be
reintroduced on the next save and break
nginx -t.The config parser is not used for this. Its dumper regenerates the whole file,
drops standalone comments and reorders proxy includes, which would be
destructive on a user's main config. Lines are edited individually so
everything outside the gzip block stays byte-identical.
Module builds on older installs
Two defects surfaced while testing the above against pre-dynamic versions.
Module state written before build modes existed carries no
buildMode.validateNginxModuleBuildModerejects the empty value, which failsloadNginxModulesand with itGetModules,UpdateModule,Buildand theupgrade itself — the whole module subsystem, not just the static feature. The
missing value is now inferred from what the install can actually do: dynamic
when the builder and catalog are present, static when the compose file still
has a build section and
build/Dockerfileto recompile the image.Asking such an install to build a module used to return "the installed
OpenResty version does not support dynamic module builds", which is a dead
end: these versions produce modules by compiling them into the image, and they
still can. The build is now retargeted to the static path, with
--add-dynamic-modulerewritten back to--add-moduleand=dynamicswitches reduced to their plain form. The retarget applies to a copy that
drives one build and is never persisted, so the catalog stays authoritative
and modules return to dynamic once the install gains a builder. The error is
kept only for installs that reference a prebuilt image and genuinely cannot
compile anything, and it now says so and points at the upgrade.
Separately, manual builds and upgrades disagreed on when a full image rebuild
is required:
executeNginxModuleBuildalso treated a non-emptyRESTY_CONFIG_OPTIONS_MOREin .env as a reason to rebuild. That value isderived state —
configureStaticNginxModulesrewrites it from the currentmodule list before every build, writing an empty string when no static module
is enabled. The rebuild it triggered therefore ran with an empty option list
and could only reproduce the image it started from, up to 120 minutes to
arrive back where it began. Both paths now decide on the module list alone.
Also fixed: size units in the performance page
The form stripped the unit suffix when reading a directive and then always
appended a fixed one when saving, silently reinterpreting the unit.
A config carrying
gzip_min_length 512;— 512 bytes — was read as512andwritten back as
512k, inflating the threshold by 1024 and effectivelydisabling compression for every response under 512 KB. The same applied to
client_header_buffer_sizeandclient_max_body_size, where the value grewby 1024× in the riskier direction.
Additionally
Number(value.match(/\d+/g))coerces a multi-number match toNaN, so a directive such asgzip_buffers 4 16kwould blank the field.UI
The brotli section appears only once the module is enabled and built, since
nginx rejects the directives while the module is not loaded. Values are read
from and written to the managed file rather than nginx.conf, so they are
removed together with the module.
brotli_typesis deliberately not exposed: it is kept aligned withgzip_typesso both encoders cover the same content, and exposing it wouldinvite the two lists to drift apart.
Commits
f61fcb9conf/http.d0db2a9c23484fe77bb04deef44389ba509bed75c0e98e17f3Testing
Verified against the real
1panel/openresty:1.31.1.1-2-4-nobleimage with areal
ngx_brotli.so, compiled using the appstore's ownDockerfile.modules. 73 integration assertions plus 15 Go tests, allpassing.
Static checks:
go build,go vet,gofmtandgo teston linux/amd64 withgo1.26.1; frontend
type-check(no new errors against a 267-error baseline),prettier --check, andvite build.brotli enable —
Content-Encoding: brconfirmed;.soverified mappedinto the worker via
/proc/*/maps;nginx -tand live reload both pass.brotli disable —
nginx -tpasses, live reload passes, and a fullcontainer restart succeeds. The restart is the real risk and it is covered.
Negative tests — directives without the module are rejected
(
unknown directive "brotli"), and unloading the module before removing itsdirectives breaks nginx. Both confirm why the ordering above is required.
gzip rewrite — driven by a harness compiled from the shipped source file,
so tested logic cannot drift from shipped logic. A stock config is rewritten
and
nginx -tpasses; configs with a tuned comp level, gzip switched off, abyte-valued
min_length, or an extra directive are all left byte-identical(md5 unchanged); the rewrite is idempotent; non-gzip lines diff to zero.
Older installs — verified end to end against
1.27.1.2-5-1-focal, whichships no
Dockerfile.modulesand nomodule.catalog.json: itsmodule.jsonhas no
buildMode,ngx_brotlicompiles into the image,nginx -tacceptsthe brotli directives with no
load_modulepresent, and the server respondswith
Content-Encoding: br(5226 B → 48 B).Full journey — old install → upgrade → enable brotli → disable → restart.
Measured on a 16 KB JSON response:
gzip_typeshad noapplication/jsonHTML page: raw 9042 B → gzip 146 B → brotli 83 B.
Not covered
reconcileDynamicNginxModuleConfigwas not driven end to end; it needs afull agent plus database plus install record. The managed files were
constructed to match its output format exactly and validated against real
nginx behaviour.
zstdis out of scope. Unlike brotli it is not prebuilt into the image andnot in the catalog, so adding it means introducing a new third-party source
rather than finishing an existing integration. The runtime-defaults table
makes it a small change once that decision is made.