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.

Sign up for GitHub

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

Jump to bottom

Dynamic custom error pages for mimetypes do not work if multiple mimetypes were given in the Accept header #10974

Open
ngoeddel-openi opened this issue Feb 8, 2024 · 3 comments
Open

Dynamic custom error pages for mimetypes do not work if multiple mimetypes were given in the Accept header #10974

ngoeddel-openi opened this issue Feb 8, 2024 · 3 comments
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. needs-priority needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one.

Comments

@ngoeddel-openi
Copy link

We implemented custom error pages for requests that expect a JSON response.
This however only works if the Accept: header consists of only one mimetype, in this case application/json.

If you add additional mimetypes in your request what seems to be quite common the errorHandler can not resolve the mimetype to an extension anymore and just returns the default custom error pages which are usually a HTML representation.

The following examples show what the current state is:
Works as expected:

$ curl -D- -H 'Accept: application/json' https://preprod.example.com/api/accounts
HTTP/1.1 403 Forbidden
Date: Tue, 06 Feb 2024 17:12:10 GMT
Content-Type: application/json
Content-Length: 41
Connection: keep-alive

{ "code": "403", "message": "Forbidden" }

Works as expected:

$ curl -D- https://preprod.example.com/api/accounts
HTTP/1.1 403 Forbidden
Date: Tue, 06 Feb 2024 17:12:14 GMT
Content-Type: */*
Content-Length: 210
Connection: keep-alive

<!DOCTYPE html>
    <html>
      <head>
        <title>403 Forbidden</title>
      </head>
      <body>
        <center>
          <h1>403 Forbidden</h1>
        </center>
        <hr>
      </body>
    </html>

Does not work as expected: (should also return a custom JSON error page 403.json)

$ curl -D- -H 'Accept: application/json, text/plain, */*' https://preprod.example.com/api/accounts
HTTP/1.1 403 Forbidden
Date: Thu, 08 Feb 2024 11:57:43 GMT
Content-Type: text/html
Content-Length: 210
Connection: keep-alive

<!DOCTYPE html>
    <html>
      <head>
        <title>403 Forbidden</title>
      </head>
      <body>
        <center>
          <h1>403 Forbidden</h1>
        </center>
        <hr>
      </body>
    </html>

The reason for that is simple. The Go function mime.ExtensionsByType and mime.ParseMediaType do not like multiple mimetypes and just return an empty list. Here's a small example and its output:

package main

import (
	"fmt"
	"mime"
)

func main() {
	fmt.Println("A single mimetypes:")
	defaultsExts, _ := mime.ExtensionsByType("application/json")
	fmt.Println(defaultsExts)
	mediatype, params, _ := mime.ParseMediaType("application/json")
	fmt.Println(mediatype)
	fmt.Println(params)
	fmt.Println("\nMultiple mimetypes:")
	defaultsExts, _ = mime.ExtensionsByType("application/json, text/plain, */*")
	fmt.Println(defaultsExts)
	mediatype, params, _ = mime.ParseMediaType("application/json, text/plain, */*")
	fmt.Println(mediatype)
	fmt.Println(params)
}

Output:

A single mimetypes:
[.json]
application/json
map[]

Multiple mimetypes:
[]

map[]

Possible solution

Preparse the Accept header and only take the first mimetype that's mentioned there. This should be quite easy to do. Just split the string by comma and take the first element before handing it over to the errorHandler. Or just do that in the errorHandler itself.

I by myself am not a Go programmer unfortunately so it would be nice if someone else could implement that.
@ngoeddel-openi ngoeddel-openi added the kind/feature Categorizes issue or PR as related to a new feature. label Feb 8, 2024
@k8s-ci-robot
Copy link
Contributor

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.

@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 Feb 8, 2024
@ngoeddel-openi
Copy link
Author

I found a workaround for the issue but I am not sure if it has bad side effects. Just add this annotation to your Ingress resource:

metadata:
  annotations:
    nginx.ingress.kubernetes.io/server-snippet: |
      if ($http_accept ~* "([^,]+),.*") {
        set $http_accept $1;
      }

It drops all but the firstly mentioned mimetype in the Accept header before it gets proxied to any backend.

However this is just for your information or anybody else who runs into that issue.
A fix or at least a discussion is still much appreciated.

@github-actions GitHub Actions
Copy link

This is stale, but we won't close it automatically, just bare in mind the maintainers may be busy with other tasks and will reach your issue ASAP. If you have any question or request to prioritize this, please reach #ingress-nginx-dev on Kubernetes Slack.

@github-actions github-actions bot added the lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. label Mar 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. needs-priority needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one.
Projects
[SIG Network] Ingress NGINX
Status: No status
Milestone
No milestone
Development

No branches or pull requests
2 participants
@k8s-ci-robot @ngoeddel-openi