ingress-nginx

ingress-nginx copied to clipboard

What this PR does / why we need it:

Allows setting an empty value to the custom-http-errors in order to disable them altogether.

Types of changes

• [ ] Bug fix (non-breaking change which fixes an issue)
• [x] New feature (non-breaking change which adds functionality)
• [ ] Breaking change (fix or feature that would cause existing functionality to change)
• [ ] Documentation only

Which issue/s this PR fixes

fixes #8384 fixes #10066

How Has This Been Tested?

Checklist:

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

@aslafy-z: 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/test-infra repository.

Hi @aslafy-z. 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/test-infra repository.

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: aslafy-z To complete the pull request process, please assign strongjz after the PR has been reviewed. You can assign the PR to them by writing /assign @strongjz in a comment when ready.

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

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

/remove-lifecycle stale

@aslafy-z tht annotation is named aptly and does what it is names as.

If you are looking for a new feature to handle a case where no existing ingress rule matches a incoming http request and as a result instead of the default-backend of the project handling the response, you want your own behaviour, then I think the documented procedure is to create your own image and use that to create a backend to be configured as a default-backend.

Changing an existing annotation that is sort of not named after your desired behaviour does not seem like an improvement. There are several such changes made earlier that has led to unmaintained and unsupportable features and the project is now in a 6 month phase to clean up and stabilize the code.

This is my opinion so lets wait for other comments. But I vote for not doing this change you are proposing. I hope the project steers away from such changes that only one user benefits from and that is not deeply thought over and elaborated.

But I am not a developer so I could be wrong so lets hope there is enough info posted here about a deep dive analysis on how the change you propose is a improvement for a large number of users.

✅ Deploy Preview for kubernetes-ingress-nginx canceled.

Name	Link
🔨 Latest commit	9813ecd36b1616007094d78d8e8c74f0047d8eed
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-ingress-nginx/deploys/668bc51a4af1ec00081ebe83

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: aslafy-z Once this PR has been reviewed and has the lgtm label, please ask for approval from strongjz. 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

@longwuyuan We're delivering a kubernetes service with some mutualized bricks like the ingresses. In our default configuration, we define custom-http-errors configmap value to 502, 503. Some of our users need to disable this default handling as their app exposes this kind of errors. Today, they have to overwrite this configuration with the custom-http-errors annotation with some non used http status code like 418. This is not optimal. We would prefer them to set off as a value. Don't you think this is a correct use case? Would you prefer a dedicated annotation? Maybe an empty value is better than off? (I found that empty values is handled as an error, project wide, that's why it's not accepted as of now.)

I fixed the tests. Please have a look!

@aslafy-z can you add an e2e test for this?

/triage accepted /kind feature /priority backlog

@strongjz I added a basic e2e test, let me know if it's sufficient or not, if not, what you think is a good scenario for this test. Thank you

Having the same issue here, what is the status of this PR?

@aslafy-z the project does not set values for the configMap key tht you mentioned, by default. So this change seems like a very tailored requirement only for your use case. There is not a large user base that first sets a key:value pair in the configMap on their own and then require some exceptions to their own choice.

I am wondering if you can just avoid setting the comfigMap key:value pair for users who don't need it. That would be a improvement of your offering to your users.

I am not sure if its a improvement of the project code to tweak a annotation behaviour, when its not even enabled by default. Even the tests for this will have to first set the key:value pair in the configMap and then a second configuration step of disabling an annotation, that was never enabled out of the box.

I disagree, I have this same use case.

I use a default catchall errors but my api projects have to bypass them to being able to respond with their own error codes that overlap.

I imagine this is the same use case for many, and seems to be a pretty obvious use case.

Any project that needs to return a custom response for an overlapping error code is going to be unable to do so.

Please don't write off this feature request without understanding the desire behind it.

On Sat, Jul 6, 2024, 9:53 PM Long Wu Yuan @.***> wrote:

@aslafy-z https://github.com/aslafy-z the project does not set values for the configMap key tht you mentioned, by default. So this change seems like a very tailored requirement only for your use case. There is not a large user base that first sets a key:value pair in the configMap on their own and then require some exceptions to their own choice.

I am wondering if you can just avoid setting the comfigMap key:value pair for users who don't need it. That would be a improvement of your offering to your users.

I am not sure if its a improvement of the project code to tweak a annotation behaviour, when its not even enabled by default. Even the tests for this will have to first set the key:value pair in the configMap and then a second configuration step of disabling an annotation, that was never enabled out of the box.

— Reply to this email directly, view it on GitHub https://github.com/kubernetes/ingress-nginx/pull/8385#issuecomment-2212241245, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAD63FJNAI4DRXMDILNH3B3ZLCNQ7AVCNFSM5RNYSJ4KU5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TEMRRGIZDIMJSGQ2Q . You are receiving this because you are subscribed to this thread.Message ID: @.***>

@r2DoesInc thanks for the update. It add info so it helps.

I have no intent to write off this feature-request. I am not even a developer.

The description for this feature seems first, the clusterWide defaults are changed (while installing) because the defaults are not acceptable. And then an exception to clusterWide defaults is force configured per ingress.

Sorry, I just saw the email and misunderstood your role.

Yes the goal is to be able to globally configure this at the cluster level, but doing so precludes you from being able to send a custom response back for any code included.

Imagine you set a global 404 but you have an API project that sends a custom error message for 404. The only way to do this now is to overwrite the API project and see the unused 418 or something similar.

Setting a random unused code seems to be the accepted current solution but it's obviously hacky. Setting to 'disabled' would make the intent clear.

On Sat, Jul 6, 2024, 10:21 PM Long Wu Yuan @.***> wrote:

@r2DoesInc https://github.com/r2DoesInc thanks for the update. It add info so it helps.

I have no intent to write off this feature-request. I am not even a developer.

The description for this feature seems first, the clusterWide defaults are changed (while installing) because the defaults are not acceptable. And then an exception to clusterWide defaults is force configured per ingress.

— Reply to this email directly, view it on GitHub https://github.com/kubernetes/ingress-nginx/pull/8385#issuecomment-2212290435, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAD63FO5UJAJASCBCIBSYWDZLCQ4NAVCNFSM5RNYSJ4KU5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TEMRRGIZDSMBUGM2Q . You are receiving this because you were mentioned.Message ID: @.***>

Just to clarify, the problem is the following:

• custom-http-errors: yes at the cluster level, e.g. 404
• On ingresses for application X
  • / custom-http-errors: yes, e.g. 404, following the default cluster setting
  • /api custom-http-errors: no, because usually, HTTP statuses are transformed to render a pretty page which can break the API's expectations in where it expects 401 or 404 to act differently but always get 200 now.

Happy to provide examples of such applications.

:point_right: Currently, there is no easy way to disable custom-http-errors for /api unless setting it to a dummy HTTP status code: 418, 599, etc.

I don't know if the PR addresses the problem in the best way, there are many different ways to achieve this like with another annotation enable-custom-http-errors: true/false but it has the merit to work.

@Diaoul it seems like the fair & reasonable choice when you state the alternative of a new annotation. Precisely aiming at enabling/disabling custom-http-error-codes in one annotation and actually setting the error codes using the currently existing annotation sounds appropriate.

@aslafy-z I see that you changed this to be a new dedicated annotation. thanks.

Can you please rebase and try to run the related e2e test with env var FOCUS locally https://kubernetes.github.io/ingress-nginx/developer-guide/getting-started/

You can also try make dev-env locally, from the rootdir of the local clone of your branch to get a interactive environ for looking at your new annotation

@longwuyuan I fixed tests