-
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
Convert isValidClientBodyBufferSize to something more generic #3409
Conversation
Cc @ElvinEfendi |
I'm OK with this change but I think the more important issue is how we handle when getting annotations for given ingress fails:
There's always going to be race since listing ingresses and getting their annotations is not an atomic operation. That being said we can make the situation better by introducing another function that returns both the list of ingresses as well as their annotations in a map:
|
/lgtm |
… it for client_max_body_size
84a0c01
to
0f3e2b9
Compare
@ElvinEfendi reading the travis-ci e2e log I think the reason for the invalid configuration happens when there is no endpoints available or an error getting information about the service. |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aledbf, ElvinEfendi, wayt 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 |
What this PR does / why we need it:
This re-use the isValidClientBodyBufferSize for client_max_body_size configuration.
I'm make isValidClientBodyBufferSize more generic for that.
Recently, we've had nginx configuration issue because of
client_max_body_size
being set to an empty value.As mentioned in #2494 this is caused by a race condition somewhere, making https://github.com/kubernetes/ingress-nginx/blob/master/internal/ingress/controller/store/store.go#L675 return an empty
annotations.Ingress
.Which issue this PR fixes
Special notes for your reviewer:
This does not solve the underlying race condition but fixes the reload issue it causes.