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

request response buffering options #1016

Merged
merged 3 commits into from
Jan 25, 2021

Conversation

shaneutt
Copy link
Contributor

What this PR does / why we need it:
Add options to KongIngress and ingress annotations for request_buffering and response_buffering options.

Which issue this PR fixes
fixes #924

@shaneutt shaneutt added area/feature New feature or request area/kong Issue with Kong proxy behavior, rather than the controller priority/medium area/tests labels Jan 25, 2021
@shaneutt shaneutt self-assigned this Jan 25, 2021
@codecov
Copy link

codecov bot commented Jan 25, 2021

Codecov Report

Merging #1016 (c2fdebb) into next (5090eea) will increase coverage by 0.22%.
The diff coverage is 76.92%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next    #1016      +/-   ##
==========================================
+ Coverage   49.56%   49.78%   +0.22%     
==========================================
  Files          32       32              
  Lines        3198     3224      +26     
==========================================
+ Hits         1585     1605      +20     
- Misses       1483     1487       +4     
- Partials      130      132       +2     
Impacted Files Coverage Δ
...ernal/ingress/controller/parser/kongstate/route.go 88.13% <72.72%> (-2.19%) ⬇️
internal/ingress/annotations/annotations.go 89.65% <100.00%> (+0.76%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5090eea...c2fdebb. Read the comment docs.

Copy link
Contributor

@rainest rainest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another case like https://github.com/Kong/kubernetes-ingress-controller/blob/1.1.0/internal/ingress/controller/parser/kongstate/route_test.go#L175-L192 (items are original route, KongIngress, expected route after override) will handle testing the new KongIngress logic. Otherwise LGTM

@shaneutt shaneutt force-pushed the req_resp_route_buffering branch from 75a5e47 to a42e61b Compare January 25, 2021 22:18
@shaneutt shaneutt requested a review from rainest January 25, 2021 22:19
rainest
rainest previously approved these changes Jan 25, 2021
@rainest rainest changed the base branch from main to next January 25, 2021 22:30
@rainest rainest dismissed their stale review January 25, 2021 22:30

The base branch was changed.

@shaneutt shaneutt requested a review from rainest January 25, 2021 22:32
@shaneutt shaneutt force-pushed the req_resp_route_buffering branch from 26ca6a6 to a42e61b Compare January 25, 2021 22:33
@shaneutt shaneutt force-pushed the req_resp_route_buffering branch from a42e61b to c2fdebb Compare January 25, 2021 22:36
@shaneutt shaneutt merged commit d2442de into Kong:next Jan 25, 2021
@shaneutt shaneutt deleted the req_resp_route_buffering branch January 25, 2021 22:41
@hbagdi
Copy link
Member

hbagdi commented Jan 28, 2021

@Kong/team-k8s This actually wouldn't work in traditional/hybrid mode of kong: Kong/deck#260
Can we start tracking this again?

@shaneutt
Copy link
Contributor Author

@Kong/team-k8s This actually wouldn't work in traditional/hybrid mode of kong: Kong/deck#260
Can we start tracking this again?

I've assigned that one to me and I'll dig in.

@sahil-sharma
Copy link

sahil-sharma commented Sep 13, 2021

@shaneutt Is there any update on this as I am facing the same issue?
image
I am having versions as mentioned below:

Kong version: 2.5.0-ubuntu (docker image)
Kong-Ingress-Controller: 1.3.2
Host OS: Ubuntu-20.04
K8s version: v1.20.1

I have installed a custom plugin (to my Kong docker image) that injects a Cookie response header to the client in every response (on a specific route).
I am able to receive a response back with HTTP 20X but there is no such header to the client while I added a Correlation-ID plugin and that I am able to get.
There is no error in Kong-Ingress-Controller Pod but, in Kong-Proxy pod I am seeing the above error.
I have added this annotations to my Ingress YAML file: taken from here

konghq.com/response-buffering: "true"

The flow of request/response is like this:
Request: Client ---> Kong ----> Backend service
Response: Backend service ----> Kong (here kong injects the header) ----> Client
Is this correct or should I add something else? Any hints on this.

@shaneutt
Copy link
Contributor Author

@shaneutt Is there any update on this as I am facing the same issue?
image
I am having versions as mentioned below:

Kong version: 2.5.0-ubuntu (docker image)
Kong-Ingress-Controller: 1.3.2
Host OS: Ubuntu-20.04
K8s version: v1.20.1

I have installed a custom plugin (to my Kong docker image) that injects a Cookie response header to the client in every response (on a specific route).
I am able to receive a response back with HTTP 20X but there is no such header to the client while I added a Correlation-ID plugin and that I am able to get.
There is no error in Kong-Ingress-Controller Pod but, in Kong-Proxy pod I am seeing the above error.
Any hints on what kind of annotations I should add to Kong to enable response buffering.

If you use the annotation route (rather than the CRD) then the annotations should be konghq.com/request-buffering and konghq.com/response-buffering.

The output you showed above is coming from the Kong Proxy (e.g. https://github.com/kong/kong as opposed to this ingress controller) on this line:

https://github.com/Kong/kong/blob/master/kong/init.lua#L840

This is telling you that v2 HTTP requests are involved, but not compatible with this feature.

As per the original PR for request_buffering:

Kong/kong#6057

This is specific to HTTP v1.1 chunked encoding.

Going forward I don't think communicating via the comments in this old PR is ideal: If you need further guidance on this I would recommend opening a discussion thread for assistance either in kong/kong, or here in kong/kubernetes-ingress-controller depending on which component you feel you specifically need help with:

Let me know if the information above is helpful to you, and do feel free to open a thread if you need further assistance.

@sahil-sharma
Copy link

@shaneutt Make sense. Thanks for the inputs. I will open a new thread kong/kong. 🙏

@shaneutt
Copy link
Contributor Author

@shaneutt Make sense. Thanks for the inputs. I will open a new thread kong/kong. pray

Sounds good. If you end up needing any help with the Kubernetes components too let us know 🖖

@sahil-sharma
Copy link

Thanks for this nice gesture, @shaneutt. 🙏
BTW, I have created a discussion thread.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/feature New feature or request area/kong Issue with Kong proxy behavior, rather than the controller area/tests priority/medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Kong 2.2 request/response route buffering
4 participants