Skip to content

Conversation

@pando85
Copy link
Contributor

@pando85 pando85 commented Jul 14, 2025

  • Fix backward compatibility for the proxy-busy-buffers-size annotation.
  • Now, the proxy_busy_buffers_size directive is only rendered in the nginx template if the annotation is explicitly set.
  • If the annotation is not defined, the directive is omitted from the generated config (restoring pre-d1dc3e8 behavior).
  • Updated annotation parsing logic to avoid setting a default value when the annotation is missing.
  • Added and updated unit tests to verify both presence and absence scenarios.

What this PR does / why we need it:

#13598

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • CVE Report (Scanner found CVE and adding report)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation only

Which issue/s this PR fixes

How Has This Been Tested?

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I've read the CONTRIBUTION guide
  • I have added unit and/or e2e tests to cover my changes.
  • All new and existing tests passed.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 14, 2025
@k8s-ci-robot
Copy link
Contributor

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-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added 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. labels Jul 14, 2025
@k8s-ci-robot k8s-ci-robot requested a review from Gacko July 14, 2025 10:51
@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority labels Jul 14, 2025
@k8s-ci-robot
Copy link
Contributor

Hi @pando85. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 14, 2025
@netlify
Copy link

netlify bot commented Jul 14, 2025

Deploy Preview for kubernetes-ingress-nginx ready!

Name Link
🔨 Latest commit 30718c2
🔍 Latest deploy log https://app.netlify.com/projects/kubernetes-ingress-nginx/deploys/687a6f84b258360008bef861
😎 Deploy Preview https://deploy-preview-13610--kubernetes-ingress-nginx.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@pando85 pando85 force-pushed the fix/make-proxy-busy-buffers-size-annotation-backward-compatible branch 2 times, most recently from 995f3d9 to 45ef3b8 Compare July 14, 2025 11:15
@tao12345666333
Copy link
Member

Thank you for submitting this fix. Let me take a look

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 16, 2025
@tao12345666333 tao12345666333 requested a review from Copilot July 16, 2025 01:04
@tao12345666333
Copy link
Member

/assign

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Restores backward compatibility by only rendering the proxy_busy_buffers_size directive when the corresponding annotation is explicitly set, updates parser logic to avoid defaulting when the annotation is absent, and adjusts unit tests accordingly.

  • Wrap the proxy_busy_buffers_size directive in the nginx template behind a presence check.
  • Change annotation parsing to clear BusyBuffersSize instead of defaulting when missing.
  • Update existing test to expect an empty value and add a test for when the annotation is present.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
rootfs/etc/nginx/template/nginx.tmpl Conditionally render proxy_busy_buffers_size only if annotation is set
internal/ingress/annotations/proxy/main.go Remove default fallback for BusyBuffersSize and clear it when missing
internal/ingress/annotations/proxy/main_test.go Update tests for absence; add test for presence of BusyBuffersSize
Comments suppressed due to low confidence (4)

internal/ingress/annotations/proxy/main.go:306

  • Differentiate between a missing annotation and a parse error: only swallow the ‘annotation not found’ case and propagate or log other errors to avoid silently dropping invalid values.
	if err != nil {

internal/ingress/annotations/proxy/main_test.go:310

  • Include the actual error in the failure message (e.g., t.Fatalf("unexpected error: %v", err)) to aid debugging when the test fails.
		t.Fatalf("unexpected error parsing a valid")

internal/ingress/annotations/proxy/main_test.go:302

  • Consider adding a test for an invalid or malformed proxy-busy-buffers-size value to verify that parse errors are handled as expected.
// Add a test for when annotation is set

rootfs/etc/nginx/template/nginx.tmpl:1079

  • [nitpick] Adjust indentation of the if/end block to align with surrounding directives for consistent readability in the template.
            {{ if $location.Proxy.BusyBuffersSize }}

@tao12345666333
Copy link
Member

Adding this link for others to reference

#12433

@tao12345666333
Copy link
Member

Syntax: proxy_busy_buffers_size size;
Default:
proxy_busy_buffers_size 8k|16k;
Context: http, server, location
When buffering of responses from the proxied server is enabled, limits the total size of buffers that can be busy sending a response to the client while the response is not yet fully read. In the meantime, the rest of the buffers can be used for reading the response and, if needed, buffering part of the response to a temporary file. By default, size is limited by the size of two buffers set by the proxy_buffer_size and proxy_buffers directives.

http://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_busy_buffers_size

We can leave it blank by default when the user does not set the annotation, so that Nginx will use the default value

Copy link
Member

@tao12345666333 tao12345666333 left a comment

Choose a reason for hiding this comment

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

Thanks for submitting this fix, it looks great and will fix the issue

@Gacko
Copy link
Member

Gacko commented Jul 16, 2025

I am already working on this, @tao12345666333. Wanted to get it merged today.

@pando85
Copy link
Contributor Author

pando85 commented Jul 16, 2025

If you have any other comments, I will be happy to address them.

@Gacko
Copy link
Member

Gacko commented Jul 16, 2025

Not yet, but I wanted to do some maintenance work, like bumping Go, Kubernetes and stuff and deprecating v1.11, first before merging user provided PRs after the release.

@tao12345666333 tao12345666333 added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jul 16, 2025
@Gacko Gacko changed the title Config/Annotations: Fix proxy-busy-buffers-size backward compatibility Config/Annotations: Fix proxy-busy-buffers-size. Jul 16, 2025
@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority labels Jul 16, 2025
@Gacko Gacko force-pushed the fix/make-proxy-busy-buffers-size-annotation-backward-compatible branch from ea7fa26 to 4cb49bc Compare July 16, 2025 15:42
@Gacko
Copy link
Member

Gacko commented Jul 16, 2025

/cherry-pick release-1.13

@k8s-infra-cherrypick-robot
Copy link
Contributor

@Gacko: once the present PR merges, I will cherry-pick it on top of release-1.13 in a new PR and assign it to you.

In response to this:

/cherry-pick release-1.13

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-sigs/prow repository.

@Gacko Gacko force-pushed the fix/make-proxy-busy-buffers-size-annotation-backward-compatible branch from 4cb49bc to 30718c2 Compare July 18, 2025 16:00
Copy link
Member

@Gacko Gacko left a comment

Choose a reason for hiding this comment

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

/unhold
/lgtm

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 18, 2025
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 18, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Gacko, pando85

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 18, 2025
@k8s-ci-robot k8s-ci-robot merged commit 75a5907 into kubernetes:main Jul 18, 2025
27 checks passed
@k8s-infra-cherrypick-robot
Copy link
Contributor

@Gacko: new pull request created: #13638

In response to this:

/cherry-pick release-1.13

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-sigs/prow repository.

@HaveFun83
Copy link

any ETA for the patch release?

@mmclane
Copy link

mmclane commented Aug 5, 2025

Still waiting on what to expect for a patch release.

@Gacko
Copy link
Member

Gacko commented Aug 5, 2025

I don't wanna disappoint you, but rather kindly remind you we're all doing this in our free time. To me this sounds close to demanding, but nevertheless, I managed to kick off the release process besides my day to day job. 🙂

@mmclane
Copy link

mmclane commented Aug 6, 2025

I don't wanna disappoint you, but rather kindly remind you we're all doing this in our free time. To me this sounds close to demanding, but nevertheless, I managed to kick off the release process besides my day to day job. 🙂

Perhaps it was a bit demanding. From my perspective, it had been weeks since the fix had been merged and the comments from the team suggested that it would be a quick process. I have been waiting weeks to get ingress-nginx sorted because there was another issue that kept me from upgrading that you all decided to just not fix. So once I sorted that out on my end I upgraded only to find that the pods wont start. I completely understand that you are doing this in your free time. I wasn't asking for anything other then an expectation to be set.

Not to be too greedy or demanding but I still don't know what to expect for when that fix might be available.

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. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. size/M Denotes a PR that changes 30-99 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.

7 participants