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

VirtualServerRoute: action.return headers #2420

Closed
andrew-s opened this issue Feb 11, 2022 · 3 comments · Fixed by #5204
Closed

VirtualServerRoute: action.return headers #2420

andrew-s opened this issue Feb 11, 2022 · 3 comments · Fixed by #5204
Assignees
Labels
backlog Pull requests/issues that are backlog items proposal An issue that proposes a feature request
Milestone

Comments

@andrew-s
Copy link
Contributor

andrew-s commented Feb 11, 2022

Is your feature request related to a problem? Please describe.
There are situations when using VirtualServerRoute's that you want to return a header when you use action.return, an example of this is shifting OPTIONS method requests into Nginx instead of the application (e.g. if you're pre-building ingresses via a helm chart) and you need to return the headers in an action.return - currently this is unsupported

Describe the solution you'd like
Strangely, headers can be returned in the ErrorPage.return ( https://docs.nginx.com/nginx-ingress-controller/configuration/virtualserver-and-virtualserverroute-resources/#errorpagereturnheader ) - essentially this same mechanism can be used for action.return and then makes these consistent since they're almost identical. I also don't believe this an nginx.conf limitation.

An example and use case of what this might look like;

       - conditions:
         - variable: $request_method
           value: "OPTIONS"
         action:
           return:
            code: 200
            body: "OK"
             headers:
             - name: Access-Control-Allow-Origin
               value: "*"

Describe alternatives you've considered
In theory, it looks like you'd be able to do this with the location snippet support but it seems like that goes against the conditional match patterns that VirtualServer brings to the table, e.g. you could return in the http snippet before the conditional match happens and there are some situations where you may want to do this (such as conditionals on other variables).

Additional context
I'd be happy to open a PR and start working on this if it's something that would be accept into the spec

Update: It looks like there's a bug in the location snippet whereby it doesn't get sent to the default routes so this doesn't work, I hadn't realised that default routes are sent through the ErrorPages struct with a default 418 code which then gets remapped, this looks to be causing issues with using location snippets here. The true workaround here (and is not great) is to create a service in the cluster that always returns "OK" and proxy pass that upstream until headers can be returned in action.return. So to be clear, snippets do not work here.

Aha! Link: https://nginx.aha.io/features/IC-413

@andrew-s andrew-s added the proposal An issue that proposes a feature request label Feb 11, 2022
@github-actions
Copy link

Hi @andrew-s thanks for reporting!

Be sure to check out the docs while you wait for a human to take a look at this 🙂

Cheers!

@brianehlert
Copy link
Collaborator

Thanks @andrew-s .
I have seen this request before and see the value in it.
If you are willing to begin working on this, that is great.

It would need to extend both the VirtualServer and the VirtualServerRoute objects to remain consistent.

VirtualServer Example:

  - path: /v1/add
    matches:
    - conditions:
      - argument: psk
        value: superSecretKey
      action:
        proxy:
          upstream: apiv1
          responseHeaders:
            add:
            - name: Access-Control-Allow-Origin
              value: "*"
    action:
      return:
        code: 200
        type: application/json
        headers:
          - name: Access-Control-Allow-Origin
            value: "*"
        body: |
          {\"result\": \"ERROR\", \"details\": \"Please provide the Super Secret Key in the psk field\"}

@lucacome lucacome added the backlog candidate Pull requests/issues that are candidates to be backlog items label Mar 17, 2023
@lucacome lucacome removed this from the v3.1.0 milestone Mar 17, 2023
@andrew-s
Copy link
Contributor Author

Submitted the corresponding PR for this.

@vepatel vepatel added backlog Pull requests/issues that are backlog items and removed backlog candidate Pull requests/issues that are candidates to be backlog items labels Apr 4, 2024
@vepatel vepatel added this to the v3.6.0 milestone Apr 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog Pull requests/issues that are backlog items proposal An issue that proposes a feature request
Projects
Status: Done 🚀
Development

Successfully merging a pull request may close this issue.

5 participants