Lua: Fix `limit_except` returning 503. #11860

ZJfans · 2024-08-24T18:33:26Z

What this PR does / why we need it:

Using limit_except GET { deny all; } together with location = / { return 403; } , When the request is POST ，results in 503, instead of 403.
This issue involves the ngx_http_core_module.c module of Nginx. When the limit_except directive is used, variables set within a location block cannot be accessed. Currently, the balancer.rewrite() in nginx.conf relies on the proxy_upstream_name variable, which leads to a failure in obtaining the balancer and results in a 503 error. However, it should actually return a 403 error.
If you need more details please let me know

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

fixes #11742

How Has This Been Tested?

kind: Ingress
metadata:
  name: foo-ingress
  namespace: default
  annotations:
    nginx.ingress.kubernetes.io/configuration-snippet: limit_except GET { deny all; }
    nginx.ingress.kubernetes.io/server-snippet: |
      location  =/ {
        return 403;
      }
spec:
  ingressClassName: nginx
  rules:
  - host:
    http:
      paths:
      - path: /foo
        pathType: Prefix
        backend:
          service:
            name: foo-service
            port:
              number: 8080

curl -X POST http://1xx.xxx.xxx.xx:32080/foo

<html>
<head><title>503 Service Temporarily Unavailable</title></head>
<body>
<center><h1>503 Service Temporarily Unavailable</h1></center>
<hr><center>nginx</center>
</body>
</html>

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.

…11742)

k8s-ci-robot · 2024-08-24T18:33:34Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ZJfans
Once this PR has been reviewed and has the lgtm label, please assign rikatz for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

linux-foundation-easycla · 2024-08-24T18:33:34Z

The committers listed above are authorized under a signed CLA.

✅ login: ZJfans / name: zhangjie (35657b3, 2a71a24)

k8s-ci-robot · 2024-08-24T18:33:34Z

Welcome @ZJfans!

It looks like this is your first PR to kubernetes/ingress-nginx 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/ingress-nginx has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2024-08-24T18:33:35Z

Hi @ZJfans. 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.

netlify · 2024-08-24T18:33:45Z

✅ Deploy Preview for kubernetes-ingress-nginx canceled.

Name	Link
🔨 Latest commit	`35657b3`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-ingress-nginx/deploys/66cf29fbeaeb1d0008fade14

Gacko · 2024-08-25T06:04:42Z

We are currently trying to refrain from snippets and/or Lua at all, so I'm not sure if it makes sense to further extend these parts of the codebase.

@strongjz @rikatz @tao12345666333 WDYT?

ZJfans · 2024-08-25T10:06:12Z

We are currently trying to refrain from snippets and/or Lua at all, so I'm not sure if it makes sense to further extend these parts of the codebase.

@strongjz @rikatz @tao12345666333 WDYT?

Sorry, I didn't realize that Lua is being removed at the moment. If this is not necessary, we can close this request.

Gacko · 2024-08-25T10:35:01Z

Lua is not being removed, but reduced and we'd like to replace it by NJS (https://nginx.org/en/docs/njs) or something similar in the long run - if even required.

Currently I can not tell if your PR is just a fix or a feature as I'm not so deep into Lua, yet. I can only tell that - what you already noticed - we'd like to reduce it and - for security reasons - get rid of snippets. Which brings me back to the "feature or fix" question as this issue only occurs in conjunction with snippets, right?

ZJfans · 2024-08-25T12:29:03Z

It's a fix, this is my analysis, not English, need translation (https://zjfans.github.io/2024/08/25/Problems%20with%20ingress-nginx%20using%20limit_except/)

Gacko · 2024-08-26T21:54:49Z

Do I understand correctly, that in case of a forbidden request method NGINX does not enter the location block and therefore does not set the proxy upstream name, but still enters the get_balancer function? Because then we should rather just do the same nil check as a few lines below and also return nil instead of answering the request with 403.

At least this Lua function does not seem to be the right place for generating the final response to me.

Gacko · 2024-08-26T21:56:24Z

/triage accepted
/kind bug
/priority backlog
/hold

Gacko · 2024-08-26T21:56:31Z

/cherry-pick release-1.10

Gacko · 2024-08-26T21:56:33Z

/cherry-pick release-1.11

k8s-infra-cherrypick-robot · 2024-08-26T21:56:33Z

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

In response to this:

/cherry-pick release-1.10

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-infra-cherrypick-robot · 2024-08-26T21:56:34Z

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

In response to this:

/cherry-pick release-1.11

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 · 2024-08-26T21:57:03Z

/ok-to-test

Gacko

Please also add appropriate tests for what you're trying to fix.

rikatz · 2024-08-26T23:48:18Z

I would like as well an e2e test for this situation returning the right behavior.

ZJfans · 2024-08-27T02:04:23Z

Do I understand correctly, that in case of a forbidden request method NGINX does not enter the location block and therefore does not set the proxy upstream name, but still enters the get_balancer function? Because then we should rather just do the same nil check as a few lines below and also return nil instead of answering the request with 403.

At least this Lua function does not seem to be the right place for generating the final response to me.

Exactly, for the reasons you mentioned. I will add tests, but I'm not very familiar with this, so it will take me some time.

ZJfans · 2024-08-30T13:43:41Z

@Gacko
I have a question, e2e does not allow post, but I need to test get and post, can you give me some advice

f.HTTPTestClient().
		    POST("/").
		    WithHeader("Host", host).
		    Expect().
		    Status(http.StatusForbidden)

run golangci-lint
  Running [/home/runner/golangci-lint-1.56.2-linux-amd64/golangci-lint run] in [/home/runner/work/ingress-nginx/ingress-nginx] ...
  test/e2e/annotations/affinity.go:1: : # k8s.io/ingress-nginx/test/e2e/annotations
  Error: test/e2e/annotations/limitexcept.go:60:7: f.HTTPTestClient().POST undefined (type *httpexpect.HTTPRequest has no field or method POST)
  Error: test/e2e/annotations/limitexcept.go:72:7: f.HTTPTestClient().POST undefined (type *httpexpect.HTTPRequest has no field or method POST) (typecheck)
  /*
  Error: test/e2e/e2e.go:[33](https://github.com/kubernetes/ingress-nginx/actions/runs/10597984395/job/29369438735?pr=11860#step:5:35):4: could not import k8s.io/ingress-nginx/test/e2e/annotations (-: # k8s.io/ingress-nginx/test/e2e/annotations
  Error: test/e2e/annotations/limitexcept.go:60:7: f.HTTPTestClient().POST undefined (type *httpexpect.HTTPRequest has no field or method POST)
  Error: test/e2e/annotations/limitexcept.go:72:7: f.HTTPTestClient().POST undefined (type *httpexpect.HTTPRequest has no field or method POST)) (typecheck)
  	_ "k8s.io/ingress-nginx/test/e2e/annotations"
  	  ^
  
  Error: issues found
  Ran golangci-lint in 86874ms

Gacko · 2024-08-31T07:48:31Z

We had a chat about this PR in our Community Meeting and came to the conclusion that this is not the ideal place to add this kind of error handling.

The function you're adding it to is meant for determining the upstream to use and not for making a final decision about the HTTP status code, especially not a 403.

I cannot tell if there is a more proper way to handle this as, from the top of my head, I don't know if this issue is more about the order NGINX is executing Lua and/or plain directives in the nginx.conf or could maybe solved by just not returning a 503 here, or at least not if the backend name is undefined / set to - from the very beginning.

Gacko · 2024-08-31T07:48:46Z

/cc @strongjz WDYT?

ZJfans · 2024-08-31T11:11:51Z

It's great that you discussed this issue. I also think this fix is not good enough. But I can't think of a better solution. The good thing is that this error has a small impact, just the expected 403 becomes 503.

I think the order is, 1. If a request is forbidden, nginx updates the configuration of the location block to the limit_except block (excluding the location configuration) 2. Lua gets the variable set in the location block (error occurs) 3. Execute the limit_except logic.

I think it's better to merge the location block configuration instead of simply replacing it. Maybe nginx thinks that the request is forbidden and should not get the original location block configuration again, because it will return 403 anyway.

Maybe my analysis is wrong, this is interesting, and I look forward to a good solution. And more importantly, limit_except should be used with caution in the future to prevent interference with other location directives.

fix: limit_except returns 503 instead of the correct 403 (kubernetes#…

2a71a24

…11742)

k8s-ci-robot requested a review from puerco August 24, 2024 18:33

k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. area/lua Issues or PRs related to lua code labels Aug 24, 2024

k8s-ci-robot requested a review from tao12345666333 August 24, 2024 18:33

k8s-ci-robot added needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 24, 2024

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 24, 2024

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Aug 24, 2024

ZJfans marked this pull request as draft August 25, 2024 05:37

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 25, 2024

ZJfans marked this pull request as ready for review August 25, 2024 05:56

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 25, 2024

k8s-ci-robot requested review from Gacko and strongjz August 25, 2024 05:56

Gacko changed the title ~~fix: limit_except returns 503 instead of the correct 403 (#11742)~~ Lua: Check backend name to be non-nil. Aug 26, 2024

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 Aug 26, 2024

Gacko changed the title ~~Lua: Check backend name to be non-nil.~~ Lua: Fix limit_except returning 503. Aug 26, 2024

Gacko requested changes Aug 26, 2024

View reviewed changes

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 28, 2024

Add tests for limit_except

35657b3

ZJfans force-pushed the fix_limit_except_503 branch from 952020f to 35657b3 Compare August 28, 2024 13:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lua: Fix `limit_except` returning 503. #11860

Lua: Fix `limit_except` returning 503. #11860

ZJfans commented Aug 24, 2024 •

edited

Loading

k8s-ci-robot commented Aug 24, 2024

linux-foundation-easycla bot commented Aug 24, 2024 •

edited

Loading

k8s-ci-robot commented Aug 24, 2024

k8s-ci-robot commented Aug 24, 2024

netlify bot commented Aug 24, 2024 •

edited

Loading

Gacko commented Aug 25, 2024

ZJfans commented Aug 25, 2024

Gacko commented Aug 25, 2024

ZJfans commented Aug 25, 2024

Gacko commented Aug 26, 2024

Gacko commented Aug 26, 2024

Gacko commented Aug 26, 2024

Gacko commented Aug 26, 2024

k8s-infra-cherrypick-robot commented Aug 26, 2024

k8s-infra-cherrypick-robot commented Aug 26, 2024

Gacko commented Aug 26, 2024

Gacko left a comment

rikatz commented Aug 26, 2024

ZJfans commented Aug 27, 2024

ZJfans commented Aug 30, 2024 •

edited

Loading

Gacko commented Aug 31, 2024

Gacko commented Aug 31, 2024

ZJfans commented Aug 31, 2024

Lua: Fix limit_except returning 503. #11860

Are you sure you want to change the base?

Lua: Fix limit_except returning 503. #11860

Conversation

ZJfans commented Aug 24, 2024 • edited Loading

What this PR does / why we need it:

Types of changes

Which issue/s this PR fixes

How Has This Been Tested?

Checklist:

k8s-ci-robot commented Aug 24, 2024

linux-foundation-easycla bot commented Aug 24, 2024 • edited Loading

k8s-ci-robot commented Aug 24, 2024

k8s-ci-robot commented Aug 24, 2024

netlify bot commented Aug 24, 2024 • edited Loading

✅ Deploy Preview for kubernetes-ingress-nginx canceled.

Gacko commented Aug 25, 2024

ZJfans commented Aug 25, 2024

Gacko commented Aug 25, 2024

ZJfans commented Aug 25, 2024

Gacko commented Aug 26, 2024

Gacko commented Aug 26, 2024

Gacko commented Aug 26, 2024

Gacko commented Aug 26, 2024

k8s-infra-cherrypick-robot commented Aug 26, 2024

k8s-infra-cherrypick-robot commented Aug 26, 2024

Gacko commented Aug 26, 2024

Gacko left a comment

Choose a reason for hiding this comment

rikatz commented Aug 26, 2024

ZJfans commented Aug 27, 2024

ZJfans commented Aug 30, 2024 • edited Loading

Gacko commented Aug 31, 2024

Gacko commented Aug 31, 2024

ZJfans commented Aug 31, 2024

Lua: Fix `limit_except` returning 503. #11860

Lua: Fix `limit_except` returning 503. #11860

ZJfans commented Aug 24, 2024 •

edited

Loading

linux-foundation-easycla bot commented Aug 24, 2024 •

edited

Loading

netlify bot commented Aug 24, 2024 •

edited

Loading

ZJfans commented Aug 30, 2024 •

edited

Loading