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: In lua http filter add ability to set the contents of the body #5998

Closed
DevoKun opened this issue Feb 19, 2019 · 11 comments · Fixed by #13172
Closed

Feature Request: In lua http filter add ability to set the contents of the body #5998

DevoKun opened this issue Feb 19, 2019 · 11 comments · Fixed by #13172
Labels
enhancement Feature requests. Not bugs or questions. help wanted Needs help!

Comments

@DevoKun
Copy link

DevoKun commented Feb 19, 2019

Feature Request: In lua http filter add ability to set the contents of the body

The idea is to be able to set the contents of the response body from lua.
This feature is helpful for providing a custom response in the event of an upstream failure.
For example, if a circuit breaker fails for an API that is expected to be JSON, a response of an empty json array could [] be returned instead of the message upstream connect error or disconnect/reset before headers. reset reason: connection failure which would break an app expecting JSON.

filter_chains:
- filters:
  - name: envoy.http_connection_manager
    config:
      http_filters:
        - name: envoy.lua
          config:
            inline_code: |
              function envoy_on_response(response_handle)
                if response_handle:headers():get(":status") != "200" then
                  response_handle:body():set('[]')
                end
              end
@mattklein123 mattklein123 added the enhancement Feature requests. Not bugs or questions. label Feb 19, 2019
@stale
Copy link

stale bot commented Mar 21, 2019

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or other activity occurs. Thank you for your contributions.

@stale stale bot added the stale stalebot believes this issue/PR has not been touched recently label Mar 21, 2019
@DevoKun
Copy link
Author

DevoKun commented Mar 25, 2019

Please do not close this feature request. The request is still relevant.

@stale stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Mar 25, 2019
@dio dio added the help wanted Needs help! label Mar 25, 2019
@dio
Copy link
Member

dio commented Mar 25, 2019

@DevoKun gotcha!

@nmnellis
Copy link

I was just trying to implement this in order to fix the issue with grpc-json transcoder not placing error messages and codes in the body. #3383

@steveyang95
Copy link

Hi, are there any updates to this issue? I found a use case for this feature as well and would love to know what the status is for this feature request

@marrrcin
Copy link

marrrcin commented Jan 3, 2020

+1
What is the current status?

@bluetechy
Copy link
Contributor

bluetechy commented Jan 13, 2020

The use case that I need this feature for is: User specifies the HTTP header Accept-Charset to cause the API to return back the response in the specified encoding (and update HTTP response header Content-Length, accordingly).

Similarly, I would like to update the request body so that I can allow users to submit requests using different encodings and handle them all as UTF-8 internally.

@dio
Copy link
Member

dio commented Sep 9, 2020

One way to fix this is to make BufferWrapper to be mutable (or probably introduce another class that allows mutation to the buffered data). A quick sketch on making wrapped buffer to be mutable: https://github.com/envoyproxy/envoy/compare/master...dio:mutable-buffer?expand=1#diff-dc19b516207df88d31bc3ee355d58ecaR41

Hence we can do:

          - name: envoy.filters.http.lua
            typed_config:
              "@type": type.googleapis.com/envoy.config.filter.http.lua.v2.Lua
              inline_code: |
                function envoy_on_request(request_handle)
                end
                function envoy_on_response(response_handle)
                  response_handle:headers():replace("content-length", 6)
                  response_handle:body():set("hello\n")
                end

Notice that we need to know the content-length in advance since when calling response_handle:body() the headers is already continued. One might expect to have the following API instead:

content_length = response_handle:body():set("hello\n")
response_handle:headers():replace("content-length", content_length)

But it is not possible for now.

Another way is to implement full-blown handle:respond() but this requires checking on some internal states in the filter.

cc. @ceastman-ibm

@dio
Copy link
Member

dio commented Sep 11, 2020

@mattklein123 do you think it is OK to let BufferWrapper to wrap mutable buffer (as exhibited in #5998 (comment))? Or probably you have other suggestions? Thanks!

@mattklein123
Copy link
Member

From a quick glance it seems reasonable. Let's discuss in code review? re: content-length, you should also be able to clear it and switch to chunk encoding if necessary. I would definitely have documentation/example about that.

@dio
Copy link
Member

dio commented Sep 18, 2020

I have submitted a PR (#13172) for this. Not entirely sure, probably we can repurpose the respond API as a wrapper for the low-level codes exhibited in the PR.

dio added a commit that referenced this issue Sep 22, 2020
This patch adds `setBytes()` API to the buffer wrapper by making the wrapped buffer to be mutable. This allows rewriting upstream data in the response path.

Risk Level: Low
Testing: Added unit and integration tests.
Docs Changes: Added
Release Notes: Added
Fixes #5998

Signed-off-by: Dhi Aurrahman <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature requests. Not bugs or questions. help wanted Needs help!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants