Skip to content
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

Feature Request: Allow disabling custom-http-errors per ingress #8384

Open
aslafy-z opened this issue Mar 23, 2022 · 16 comments · May be fixed by #8385
Open

Feature Request: Allow disabling custom-http-errors per ingress #8384

aslafy-z opened this issue Mar 23, 2022 · 16 comments · May be fixed by #8385
Assignees
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/feature Categorizes issue or PR as related to a new feature. priority/backlog Higher priority than priority/awaiting-more-evidence. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@aslafy-z
Copy link
Contributor

aslafy-z commented Mar 23, 2022

When nginx.ingress.kubernetes.io/custom-http-errors is set to string off, overwrite the configmap value with an empty list instead of skipping it.

My current work-around is to use the 418 HTTP code (nginx.ingress.kubernetes.io/custom-http-errors: "418") because my apps aren't using it 😉

@aslafy-z aslafy-z added the kind/feature Categorizes issue or PR as related to a new feature. label Mar 23, 2022
@k8s-ci-robot k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority labels Mar 23, 2022
@strongjz
Copy link
Member

/good-first-issue
/help-wanted
/triage accepted
/priority backlog

@k8s-ci-robot
Copy link
Contributor

@strongjz:
This request has been marked as suitable for new contributors.

Guidelines

Please ensure that the issue body includes answers to the following questions:

  • Why are we solving this issue?
  • To address this issue, are there any code changes? If there are code changes, what needs to be done in the code and what places can the assignee treat as reference points?
  • Does this issue have zero to low barrier of entry?
  • How can the assignee reach out to you for help?

For more details on the requirements of such an issue, please see here and ensure that they are met.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-good-first-issue command.

In response to this:

/good-first-issue
/help-wanted
/triage accepted
/priority backlog

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-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. priority/backlog Higher priority than priority/awaiting-more-evidence. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority labels Mar 29, 2022
@k8s-triage-robot
Copy link

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

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 28, 2022
@aslafy-z
Copy link
Contributor Author

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 28, 2022
@fernandes-natanael
Copy link

Hello guys, I would like to work on this issue, can you give me more details on what exactly I need to do?

@fernandes-natanael
Copy link

/assign

@fernandes-natanael
Copy link

/unassign

@asim-reza
Copy link

/assign

@asim-reza
Copy link

/unassign

@DanielViniciusAlves
Copy link

Hi @strongjz and @aslafy-z , I am new to this repository and still trying to understand the workflow of the project, could you help me figure out what to do in this issue?

@DanielViniciusAlves
Copy link

/assign

@longwuyuan
Copy link
Contributor

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

@aslafy-z
Copy link
Contributor Author

aslafy-z commented Nov 24, 2022

Here's my use case, the platform team offers a nginx ingress controller with a company-branded default-backend and handles 500 errors by default. Some apps may want to overwrite this behavior by disabling the company default-backend all-together for their ingress (e.g.: development phases where they want to see their app outputs even if they are errors).
Allowing users to set nginx.ingress.kubernetes.io/custom-http-errors: off might do the trick.
Another way would be to add a new annotation like nginx.ingress.kubernetes.io/disable-default-backend: 'true'.

I feel like it would be a great addition but I understand your point, let's keep this issue open and see if it gains traction.

@DanielViniciusAlves
Copy link

/unassign

@en-vee
Copy link

en-vee commented Nov 7, 2023

/assign

I would like to work on this issue. As I am completely new to this workflow, can I please get some guidance on how to go about working on this issue ?

@aslafy-z
Copy link
Contributor Author

aslafy-z commented Nov 7, 2023

There's an open pull request that address this issue.

/assign

@aslafy-z aslafy-z changed the title Allow setting custom-http-errors annotation to overwrite configmap values when empty Feature Request: Allow disabling custom-http-errors per ingress Nov 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/feature Categorizes issue or PR as related to a new feature. priority/backlog Higher priority than priority/awaiting-more-evidence. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants