copr / copr

#2335 Disable nginx port in redirection for backend httpd

Merged 2 years ago by praiskup. Opened 2 years ago by tommylike.

copr/ tommylike/copr feature/bug-fix into main

Disable nginx port in redirection for backend httpd

TommyLike • 2 years ago

52a3ce4

docker/backend_httpd/files/etc/nginx/conf.d/copr-be.conf

file modified

		`@@ -9,6 +9,7 @@`
		`root /var/lib/copr/public_html/;`

		`location / {`
		`+ port_in_redirect off;`
praiskup commented 2 years ago Would you mind better documenting why this is needed? A concrete case, otherwise LGTM, thanks.
		`autoindex on;`
		`}`
		`}`

tommylike commented 2 years ago

When backend httpd is served behind another proxying nginx, it's
a common case that these two are configured with different ports,
disable port in redirection will prevent client get incorrect port
number if '301 moved permanently' happened.

tommylike commented 2 years ago

@praiskup @frostyx Thanks.

zuul commented 2 years ago

Build succeeded.

csdiff-pylint : SUCCESS in 57s

praiskup commented on line 6 of docker/keygen-httpd/files/entrypoint 2 years ago

This seems wrong. The command should take the name of the package, not user.

praiskup commented on line 5 of docker/backend_httpd/files/etc/nginx/conf.d/copr-be.conf 2 years ago

Would you mind better documenting why this is needed? A concrete case, otherwise LGTM, thanks.

rebased onto 2b768ec00458be9e8528ff388bd03478f8256e0f

2 years ago

tommylike commented 2 years ago

This seems wrong. The command should take the name of the package, not user.

Thanks, you are correct!

zuul commented 2 years ago

Build succeeded.

csdiff-pylint : SUCCESS in 1m 56s

tommylike commented 2 years ago

Would you mind better documenting why this is needed? A concrete case, otherwise LGTM, thanks.

Nginx autoindex module will return "301 Moved" with configured port in header‘s location when visiting folder without "/" at the end of the url。
Something like this below:

< HTTP/2 301
< date: Sun, 09 Oct 2022 00:58:14 GMT
< content-type: text/html
< content-length: 162
< location: http://repo.openeuler.org/openEuler-20.03-LTS/
< strict-transport-security: max-age=15724800; includeSubDomains
< x-using-nginx-controller: true
<
<html>
<head><title>301 Moved Permanently</title></head>
<body>
<center>

301 Moved Permanently

</center>

<center>nginx</center>
</body>
</html>

Edited 2 years ago by tommylike

praiskup commented 2 years ago

I mean this in the commit message:

Fix bug in configuration files
1. disable port in redirection for backend http nginx

We need more "why?" than "how? and what?".
https://cbea.ms/git-commit/#why-not-how

Shouldn't the proxy understand the location headers, and translate ports? Please give us your background story to protect your fix in the future! :-)

tommylike commented 2 years ago

I mean this in the commit message:
Fix bug in configuration files 1. disable port in redirection for backend http nginx

We need more "why?" than "how? and what?".
https://cbea.ms/git-commit/#why-not-how

Shouldn't the proxy understand the location headers, and translate ports? Please give us your background story to protect your fix in the future! :-)

@praiskup Sure, I explain here first, If the backend nginx are servered behind another proxying nginx, it's a common case they are configured with different ports. Therefore, if nginx autoindex module return 301 with inner port, the client will send request within incorrect port number.

rebased onto 52a3ce4

2 years ago

zuul commented 2 years ago

Build succeeded.