Skip to content

Add Brotli Support for NGINX in ESP#784

Merged
qiwzhang merged 12 commits intocloudendpoints:masterfrom
ilyaGurevich:master
Apr 8, 2020
Merged

Add Brotli Support for NGINX in ESP#784
qiwzhang merged 12 commits intocloudendpoints:masterfrom
ilyaGurevich:master

Conversation

@ilyaGurevich
Copy link
Copy Markdown
Contributor

@ilyaGurevich ilyaGurevich commented Apr 7, 2020

Brotli compression is not currently supported in ESP, however the latest version of NGINX found here indicates that adding a brotli flag to the nginx_repositories function will allow the associated brotli dependencies to be added to ESP. The bazel build of nginx already supports Brotli, all we have to do is just add the flag on the ESP-side.

@qiwzhang
Copy link
Copy Markdown
Contributor

qiwzhang commented Apr 7, 2020

Are you sure the code is included in the ESP nginx binary?

It seems that you just define the bazel dependency and library target. you may have to add the lib to here
and here

@qiwzhang
Copy link
Copy Markdown
Contributor

qiwzhang commented Apr 7, 2020

Please rebase your code

@qiwzhang
Copy link
Copy Markdown
Contributor

qiwzhang commented Apr 7, 2020

What is your plan of adding Brotli compression related config to ESP nginx config? Do you plan to add some start_esp flags for them? Or you plan to use your own custom nginx config?

@qiwzhang
Copy link
Copy Markdown
Contributor

qiwzhang commented Apr 7, 2020

We have integration tests written in perl. They are all in src/nginx/t folder. Could you also add an integration test for this feature.

You can follow this test
as example, its "response2" skip service control, you just need to compress the response data and verify it

Comment thread src/nginx/BUILD
"@nginx//:core",
"@nginx//:http",
"@ngx_brotli//:http_brotli_filter",
"@ngx_brotli//:http_brotli_static",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need static? Here is what the doc say

ngx_brotli is a set of two nginx modules:

ngx_brotli filter module - used to compress responses on-the-fly,
ngx_brotli static module - used to serve pre-compressed files.

Do we need o support serve static compressed files. ESP is just a proxy.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I can see your point, but that I was assuming the functionality here is similar to gzip and gzip_static. The latter which appears to be enabled in the nginx_esp binary.. (--with-http_gzip_static_module)

@ilyaGurevich
Copy link
Copy Markdown
Contributor Author

What is your plan of adding Brotli compression related config to ESP nginx config? Do you plan to add some start_esp flags for them? Or you plan to use your own custom nginx config?

We will be using our own nginx.conf that includes the brotli directives.

@qiwzhang
Copy link
Copy Markdown
Contributor

qiwzhang commented Apr 8, 2020

LGTM. I will trigger a presubmit test.

@ilyaGurevich ilyaGurevich requested a review from qiwzhang April 8, 2020 18:04
@qiwzhang qiwzhang merged commit eaffe12 into cloudendpoints:master Apr 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants