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

Update base nginx #7552

Merged
merged 3 commits into from
Aug 27, 2021
Merged

Update base nginx #7552

merged 3 commits into from
Aug 27, 2021

Conversation

ElvinEfendi
Copy link
Member

maybe will help with #7080

the premise of this PR is to make our nginx base image to look more like https://github.com/openresty/openresty which is presumably better tested

the patches are exact copies from https://github.com/openresty/openresty

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 27, 2021
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 27, 2021
@@ -18,7 +18,7 @@ set -o errexit
set -o nounset
set -o pipefail

export NGINX_VERSION=1.20.1
export NGINX_VERSION=1.19.9
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if we want to downgrade the version of NGINX

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd not want to downgrade either, but then we would have to adjust the Openresty patches for 1.20.1 which I personally don't feel capable of doing in a reasonable time.

Copy link
Member

Choose a reason for hiding this comment

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

Talked with @ElvinEfendi in slack, we're going to downgrade to 1.19.9 nginx and patch openrusty to test to see if it fixes the coredump issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

added the 1.19.9 related security patch too from openresty: e0cea12

@longwuyuan
Copy link
Contributor

longwuyuan commented Aug 27, 2021 via email

@ElvinEfendi
Copy link
Member Author

Only s390x uses will benefit

@longwuyuan why?

@longwuyuan
Copy link
Contributor

longwuyuan commented Aug 27, 2021 via email

@strongjz
Copy link
Member

strongjz commented Aug 27, 2021

/kind bug
/triage accepted
/priority critical-urgent

This is related to #6896

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. triage/accepted Indicates an issue or PR is ready to be actively worked on. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority labels Aug 27, 2021
@strongjz
Copy link
Member

/lgtm
/approve

Let's see if this addresses the core dump issues. If not we can always back out these changes.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 27, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ElvinEfendi, strongjz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit c6bc987 into kubernetes:main Aug 27, 2021
@ElvinEfendi ElvinEfendi deleted the update-base-nginx branch August 27, 2021 14:39
@longwuyuan
Copy link
Contributor

Saw a related note. #7552 (comment)

No segfault in 2 days on 0.49.0 with high number of ingress objects.

k8s-ci-robot pushed a commit that referenced this pull request Aug 28, 2021
* Revert "Update base nginx (#7552)"

This reverts commit c6bc987.

* keep alpine bump
@BloodyIron
Copy link

Considering this is a downgrade from NGINX base 1.20.x to 1.19.x, I don't know why this is being framed as upgrade, since 0.47 implemented 1.20.1 to mitigate CVE-2021-23017. So... is ingress-nginx 1.0.1/onwards now vulnerable to CVE-2021-23017? This whole history around NGINX base version is so muddy I can't reliably tell. 0.49.1/onward (0.x branch) sure looks to be CVE-2021-23017 vulnerable too... was any of this considered for any of these NGINX base changes??? #7164 is relevant.

rchshld pushed a commit to joomcode/ingress-nginx that referenced this pull request May 19, 2023
* upgrade alpine

* use nginx 1.19.9 and corresponding patches from openresty

* include openresty CVE-2021-23017 patch too
rchshld pushed a commit to joomcode/ingress-nginx that referenced this pull request May 19, 2023
* Revert "Update base nginx (kubernetes#7552)"

This reverts commit c6bc987.

* keep alpine bump
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants