feat(annotations): introduce enable-custom-http-errors annotation

[aslafy-z]

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)
• 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:

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

[k8s-ci-robot]

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.

[k8s-triage-robot]

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

[aslafy-z]

/remove-lifecycle stale

[longwuyuan]

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

[Diaoul]

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.

👉 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.

[longwuyuan]

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

[longwuyuan]

@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/

[longwuyuan]

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

[aslafy-z]

@longwuyuan I fixed tests

[HaveFun83]

any news here? We have a similar issue

[strongjz]

@aslafy-z can you rebase to get the latest changes and squash the commits into 1.

/ok-to-test
/triage accepted

[aslafy-z]

@strongjz Commits have been squashed, the branch has been rebased, and tests are passing! 🚀

[rikatz]

/uncc

[rikatz]

/uncc