kubernetes / ingress-nginx

Ingress-NGINX Controller for Kubernetes
https://kubernetes.github.io/ingress-nginx/
Apache License 2.0
17.29k stars 8.21k forks source link

disable-proxy-intercept-errors is not working when defining custom-http-errors #10832

Open Abbas-b-b opened 8 months ago

Abbas-b-b commented 8 months ago

What happened:

The new configuration disable-proxy-intercept-errors is not working as expected. The following configuration still renders proxy_intercept_errors on in the nginx configuration and the upstream error response is not passed to the client.

controller:
  config:
    custom-http-errors: 404
    disable-proxy-intercept-errors: "true"

What you expected to happen:

This should not render proxy_intercept_errors on; entry or render proxy_intercept_errors off; innginx.conf file in controller pod.

NGINX Ingress controller version (exec into the pod and run nginx-ingress-controller --version.):

ControllerVersion: 1.9.5 HelmChartVersion: 4.9.0

Kubernetes version (use kubectl version):

1.27.2

How to reproduce this issue:

Deploy nginx-ingress using the mentioned configuration.

k8s-ci-robot commented 8 months ago

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](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
ahsannaseem01 commented 8 months ago

Hi @Abbas-b-b I tried to reproduce this with mentioned config and here are the initial findings. Setting

controller:
  config:
    custom-http-errors: 404
    disable-proxy-intercept-errors: "true"

sets the config this way


location @custom_upstream-default-backend_404 {
                        internal;

                        # Ensure that modsecurity will not run on custom error pages o
r they might be blocked

                        proxy_intercept_errors off;

                        proxy_set_header       X-Code             404;
                        proxy_set_header       X-Format           $http_accept;
                        proxy_set_header       X-Original-URI     $request_uri;
                        proxy_set_header       X-Namespace        $namespace;
                        proxy_set_header       X-Ingress-Name     $ingress_name;
                        proxy_set_header       X-Service-Name     $service_name;
                        proxy_set_header       X-Service-Port     $service_port;
                        proxy_set_header       X-Request-ID       $req_id;
                        proxy_set_header       X-Forwarded-For    $remote_addr;
                        proxy_set_header       Host               $best_http_host;

                        set $proxy_upstream_name "upstream-default-backend";

                        rewrite                (.*) / break;

                        proxy_pass            http://upstream_balancer;
                        log_by_lua_block {

                                monitor.call()

                        }
                }

        }

which blocks the error and send generic server exception

Screenshot 2024-01-10 at 7 26 36 PM

However when we dont pass the config it shows the actual error to the client which is 404 in this case.

Screenshot 2024-01-10 at 7 24 26 PM

Abbas-b-b commented 8 months ago

@ahsannaseem01 Thanks for your try. But the configuration section you posted belongs to the @custom_upstream-default-backend_404 section which is OK (I have the same). you need to look at the http section in the nginx.conf that includes the proxy_intercept_errors on; entry.

BTW, what I get from your screenshots is that you faced the same problem and you expected 404 in two scenarios. right?

Abbas-b-b commented 7 months ago

The disable-proxy-intercept-errors configuration is removed in 4.9.1

https://github.com/kubernetes/ingress-nginx/commit/4e97379b4e4a5d82546ad706e405045ccad0c834

Abbas-b-b commented 7 months ago

The workaround we came up with is setting "proxy_intercept_errors off; in server-snippet for the Ingress:

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: nginx-test
  annotations:
    nginx.ingress.kubernetes.io/server-snippet: "proxy_intercept_errors off;"
spec:
  ingressClassName: nginx
...

and also setting custom-errors and allow-snippet-annotations: "true".

# helm values
controller:
  config:
    allow-snippet-annotations: "true"
    custom-http-errors: 400,401,402,403

defaultBackend:
  enabled: true
  image:
    registry: registry.k8s.io
    image: ingress-nginx/nginx-errors

  extraConfigMaps:
    - name: custom-error
      data:
        error: "my custom error"

  extraVolumeMounts:
    - name: custom-error
      mountPath: /www

  extraVolumes:
   - name: custom-error
     configMap:
       name: custom-error
       items:
         - key: "error"
           path: "404.html"
         - key: "error"
           path: "404.json"
         - key: "error"
           path: "4xx.html"
chriss-de commented 7 months ago

@strongjz can you explain why that patch that took almost a year to get reviewed has been removed without a mention on why? Will it be added back? We need this feature and reading the PR a few other too.

strongjz commented 7 months ago

We discussed what PR's needed to be merged in our community before we released 1.9.6, also again in slack before it was released.

It was missed in the PR list for merging in the release-1.9.

It will be added to the release for 1.9.7.

It took a year since we have 3 maintainers, one for core, one for lua and one for releases, so we have a lot to wade through and sometimes things get missed. We discuss a lot in ingress-dev slack channel and in our communities about the priorities for ingress-nginx.

It was noted that a new contributor was able to replicate this issue in slack, but we were not able to move further into getting it fixed.

sboardwell commented 5 months ago

Hi everyone, it is a shame that this didn't make it, although I can understand the workload involved with so few maintainers.

Is there any link to track or would this be the ticket?

Thanks to all for the work on this. It would be excellent to finally get it over the line in 1.9.7

chriss-de commented 5 months ago

Use controller version 1.10+ - the patch to disable proxy_intercept_errors is available there