gzip not working after adding br to Accept-Encoding

[minac]

Context
In my organization, we have projects that have some statically compressed assets (with gzip and brotli versions), and some not compressed. We recently decided to add Brotli configuration to our nginx to serve those statically compressed assets for clients that ask for them. At this point, we do not want dynamic Brotli compression until we understand the CPU cost associated with it.

Statically compressed files work well
Testing nginx with the ngx_brotli module statically compiled and a basic Brotli configuration I can see that calling the assets with uncompressed, gzipped, and brotli'ed versions on disk and the header Accept-Encoding: gzip,deflate,br works as expected.

Dynamic Brotli configuration woks as expected
If I enable Brotli configuration to dynamically compress assets, it also works as expected.

Uncompressed files do not get gzipped if brotli is in the Accept-Encoding header
Assets that do NOT have compressed versions on disk are the problem. When called with the header Accept-Encoding: gzip,deflate, the assets come gzipped. But with the header Accept-Encoding: gzip,deflate,br they come completely uncompressed. It's as if the Brotli header disables gzip.

I created a little repository with a docker image that includes nginx and this module and added the most basic configuration and a random set of assets to test this: https://github.com/minac/nginx_brotli_test.

Questions

1. Is this expected behavior?
2. Is this a problem with my configuration?
3. If this issue is not for this project, I would appreciate it if you could point me in the right direction.

Thank you.

[adamburgess]

Removing the two gzip lines from here fixes it for me:

ngx_brotli/static/ngx_http_brotli_static_module.c

Lines 135 to 142 in 9aec15e

	/* Test if this request is allowed to have the brotli response. */
	static ngx_int_t check_eligility(ngx_http_request_t* req) {
	if (req != req->main) return NGX_DECLINED;
	if (check_accept_encoding(req) != NGX_OK) return NGX_DECLINED;
	req->gzip_tested = 1;
	req->gzip_ok = 0;
	return NGX_OK;
	}

As far as I understand it:

1. Accept-Encoding is checked, and if it doesn't contain brotli, continue (nginx moves on to gzip etc)
2. If it does, then gzip checks are disabled (line 139/140)
3. Only then is the .br file checked. If it doesn't exist, continue.
4. Nginx attempts to check for gzip, but it has been disabled! So no gzipping happens.

@eustas can the static module be changed to move those two lines down to somewhere like here?

ngx_brotli/static/ngx_http_brotli_static_module.c

Line 268 in 9aec15e

/* Setup response body. */

At that stage the .br file has been checked to exist, has been read, and the handler is ready to respond.

[adamburgess]

I tested with your repo and confirmed this should fix it.

Just for reference, how the gzip_ok/tested flags are used in nginx:
https://github.com/nginx/nginx/blob/ca9bf16f09ef2b0755bfe880c68dc71b9c46f879/src/http/modules/ngx_http_gzip_filter_module.c#L256-L263
If not tested, run ngx_http_gzip_ok (this checks for A-E: gzip, and then sets tested/ok). If already tested, check if gzip is ok. If false, skip. So ngx_brotli sets gzip_tested to 1 (skipping the gzip check) and gzip_ok to 0 (preventing gzip from running).

Interestingly -- gzip_static does not use gzip_tested/ok. I see you have a static .gz file in your repo. If you gzip_static on; and remove the brotli file, the static gz file will be served even without this brotli patch.
https://github.com/nginx/nginx/blob/ca9bf16f09ef2b0755bfe880c68dc71b9c46f879/src/http/modules/ngx_http_gzip_static_module.c#L112-L113
It does the header check only.

[minac]

Hi @adamburgess! Thanks for the deep dive here. I agree with everything you wrote.
To confirm, the problem is not with static gzipped files, but with dynamic gzipping.
I hope your fix can go into the module so we can start using it! Thanks 🙏

[rgzn-aiyun]

It is strange that the problem described above does not exist after testing multiple times.

[adamburgess]

I just re-ran the repro at https://github.com/minac/nginx_brotli_test and the problem definitely still exists...