Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade NGINX version and all the Openresty modules we bake in the base NGINX image #9198

Closed
ElvinEfendi opened this issue Oct 21, 2022 · 4 comments
Labels
needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one.

Comments

@ElvinEfendi
Copy link
Member

ElvinEfendi commented Oct 21, 2022

ingress-nginx currently uses NGINX 1.19.10 while the latest stable NGINX version is 1.22 as per http://nginx.org/en/download.html.

You can see in http://nginx.org/en/CHANGES that there are a lot of improvements since 1.19.10, notably:

    *) Bugfix: nginx returned the "Connection: keep-alive" header line in
       responses during graceful shutdown of old worker processes.

Introduced in Changes with nginx 1.21.6 25 Jan 2022. I have not tested, but this could potentially fix #8094 too.

Since we rely on Openresty ecosystem, we should try to follow what they use as base NGINX version, which is 1.21.4. Looking at NGINX changelog, there does not seem to be huge diff between 1.21.4 and 1.21.6, so I think we should be able to safely use NGINX 1.21.6 and apply these Openresty patches.

Why not directly go to the latest NGINX version 1.23.2?
We can probably do this too, I see that Openresty adopted the patches for it too. The only reservation would be that is not the latest stable release, but as long as our tests passes maybe that's fine?

@ElvinEfendi ElvinEfendi added the kind/feature Categorizes issue or PR as related to a new feature. label Oct 21, 2022
@k8s-ci-robot k8s-ci-robot removed the kind/feature Categorizes issue or PR as related to a new feature. label Oct 21, 2022
@k8s-ci-robot
Copy link
Contributor

@ElvinEfendi: The label(s) kind/maintenance cannot be applied, because the repository doesn't have them.

In response to this:

ingress-nginx currently uses NGINX 1.19.10 while the latest stable NGINX version is 1.22 as per http://nginx.org/en/download.html.

You can see in http://nginx.org/en/CHANGES that there are a lot of improvements since 1.19.10, notably:

   *) Bugfix: nginx returned the "Connection: keep-alive" header line in
      responses during graceful shutdown of old worker processes.

Introduced in Changes with nginx 1.21.6 25 Jan 2022. I have not tested, but this could potentially fix #8094 too.

Since we rely on Openresty ecosystem, we should try to follow what they use as base NGINX version, which is 1.21.4. Looking at NGINX changelog, there does not seem to be huge diff between 1.21.4 and 1.21.6, so I think we should be able to safely use NGINX 1.21.6 and apply these Openresty patches.

Why not directly go to the latest NGINX version 1.23.2?
We can probably do this too, I see that Openresty adopted the patches for it too. The only reservation would be that is not the latest stable release, but as long as our tests passes maybe that's fine?

/kind maintenance
/remove-kind feature

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the needs-kind Indicates a PR lacks a `kind/foo` label and requires one. label Oct 21, 2022
@k8s-ci-robot
Copy link
Contributor

@ElvinEfendi: This issue is currently awaiting triage.

If Ingress contributors determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority labels Oct 21, 2022
@tao12345666333
Copy link
Member

yes. I’m working on it. #8889 (comment)

@ElvinEfendi
Copy link
Member Author

Closing this in favour of #8799.

Talked to @tao12345666333 in Slack, @tao12345666333 is going to test with both 1.21.6 and 1.23.2 and use the latest working one in #8889 (comment).

If none of those work because of some conflicts with Openresty ecosystem, then we can go with 1.21.4 and at the least apply this patch nginx/nginx@22d4ff0 to resolve the graceful shutdown issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one.
Projects
None yet
Development

No branches or pull requests

3 participants