-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
NGINX: Bump to OpenResty v1.27.1.1. #12229
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for kubernetes-ingress-nginx canceled.
|
07a5d00
to
6d1e53c
Compare
6d1e53c
to
8e1b550
Compare
b4e6d09
to
e218096
Compare
/triage accepted |
need to bump the version TAG as well https://github.com/kubernetes/ingress-nginx/blob/main/images/nginx/TAG |
That's how CI will start the build https://github.com/kubernetes/ingress-nginx/blob/main/.github/workflows/zz-tmpl-images.yaml#L41 |
/kind feature |
e218096
to
19cceec
Compare
@strongjz Bumped /images/nginx/TAG from v1.0.0 to v1.0.1. Please check |
Please do not bump the tag. Cloud Build triggers on any changes. |
19cceec
to
9eefc31
Compare
@Gacko reset the TAG back to v1.0.0. Please check |
Only on merges to main? Which we want to build the image and then cherry pick into a release. |
Currently Cloud Build is configured to trigger on any changes to the images, not only the TAG, on The thing about the TAG: It's part of the release and I'd rather not let it be changed by contributors as the decision to which version to bump to is being made during the release process. Like, last time I checked the changes of each image and then bumped the TAG depending on if it's only patches/dependency updates or real changes with more impact (minor version bumped then). Also we're currently using different versions of the NGINX image across the branches as we removed Global Rate Limiting on v1.12 only, but still have it in v1.11 and v1.10. Therefore the former uses TAG v1.0.0 and the latter stay on v0.1.0. tl;dr: IMHO the TAG should be bumped during release process only.
I'd not cherry-pick these changes to any of the I hopefully find some time to review this PR this weekend. Please, if possible, do not merge before. |
We need the container built before the release.
|
I'm aware of that. But the container also builds if any file of the image changes: https://github.com/kubernetes/test-infra/blob/master/config/jobs/image-pushing/k8s-staging-ingress-nginx.yaml#L177. So there is no need to bump the TAG, which's content should be decided by the ones forging the release, in this PR here. Anyway, I hope I find some time to review it in the next days. Sorry for that! |
@longwuyuan please bump the TAG file at least to start the test build, we can figure out cloud-build after it. Once the build passes, worst case we can revert before merging. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no need to bump the TAG. Cloud Build is configured to trigger on any change in the whole directory, not just the TAG. Bumping the TAG to a proper version we then use in the NGINX_BASE is part of the release process, not part of development.
Also please do not merge this before we sorted v1.12 as we might build the NGINX base image from main
and then populate everything to the other branches. With this PR merged before the v1.12 release, the base image will include unwanted changes.
/hold
@Gacko the e2e tests runs with the new image when the TAG is bumped. I am not saying to merge, but I want to see the test passing |
This patch is very important to land, to enable the security of ingress-nginx installations, given that nginx 1.25 is EOL. What will it take to land it? |
We have spent the last few weeks debugging the nginx build in cloud build that was broken. We just got it working today. So we can start adding in PRs like this one. So soon. And most likely in 1.13. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: longwuyuan The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What this PR does / why we need it:
Types of changes
Which issue/s this PR fixes
fixes #12170
How Has This Been Tested?
make
in the /images/nginx directory, on local clone"FOCUS=GRPC make kind-e2e-test"
. It passed"make kind-e2e-test
Checklist:
cc @rikatz @tao12345666333 @strongjz @Gacko