Kong / kong

🦍 The Cloud-Native API Gateway and AI Gateway.
https://konghq.com/install/
Apache License 2.0
39.4k stars 4.83k forks source link

kong 2.1.0 rc, 301 bug #6132

Closed RyouZhang closed 4 years ago

RyouZhang commented 4 years ago

kong 2.1.0 rc

whehn http response 301, kong changed the response header Location

for example: request url: https://xxxxx/api/user/profile

upstream header Status code: 301 Location: /api/user/profile/

kong response header Status code:301 Location: /api/user/profile/api/user/profile/

carnei-ro commented 4 years ago

This is happening here too, although I could not reproduce with a simple app as you describe. This is happening when I try to call an Apache Airflow eg. I have create this repo to show the error: https://github.com/carnei-ro/kong-bug-location-header

We are using Kong 2.1.0 GA - Databaseless

gszr commented 4 years ago

@RyouZhang, could you please share more info on your setup - eg, routes/services/upstreams?

carnei-ro commented 4 years ago

@gszr using simple service and route it can be reproduced (but I think it depends on the Upstream App too)

Here my conf:

_format_version: "1.1"
routes:
  - name: root
    service: airflow
    regex_priority: 1
    hosts: []
    methods: []
    preserve_host: true
    strip_path: false
    paths:
      - /
services:
  - name: airflow
    protocol: http
    host: 10.120.41.69
    port: 8080
    path: /

I used this very same service+route in Kong 2.0.5 and Kong 2.1.0.

carnei-ro commented 4 years ago

Is it possible to be relationed to the X-Forwarded-Prefix header?

gszr commented 4 years ago

@carnei-ro @RyouZhang Here's what I'm trying:

Kong Config:

_format_version: "1.1"
routes:
  - name: root
    service: s1
    regex_priority: 1
    hosts: []
    methods: []
    preserve_host: true
    strip_path: false
    paths:
      - /
services:
  - name: s1
    protocol: http
    host: 127.0.0.1
    port: 8210
    path: /

and the mock upstream service listening on port 8210:

worker_processes 1;

events{}

http {
    server {
        listen 8210;
        location /redirect {
            content_by_lua_block {
                ngx.status=301
                ngx.header["Location"] = "http://localhost/foo/bar"
            }
        }
    }
}

Request/Response:

kong $ http :8000/redirect                  
HTTP/1.1 301 Moved Permanently
Connection: keep-alive
Content-Type: text/plain
Date: Tue, 21 Jul 2020 18:35:28 GMT
Location: http://localhost/foo/bar
Server: openresty/1.15.8.3
Transfer-Encoding: chunked
Via: kong/2.1.0
X-Kong-Proxy-Latency: 0
X-Kong-Upstream-Latency: 0
carnei-ro commented 4 years ago

Yeah, I've made a simple ruby app to try it too. I dig to the X-Forwarded-Prefix, and it seams to be it that is causing the behavior, not Kong itself, but how the app deal with this header. I built a Kong container like this:

FROM kong/kong:2.1.0

COPY kong.yml /etc/kong/kong.yml

ENV KONG_ADMIN_LISTEN='0.0.0.0:9210' \
    KONG_PROXY_LISTEN='0.0.0.0:8210' \
    KONG_PROXY_ACCESS_LOG='/dev/stdout' \
    KONG_PROXY_ERROR_LOG='/dev/stderr' \
    KONG_DATABASE='off' \
    KONG_DECLARATIVE_CONFIG='/etc/kong/kong.yml' \
    KONG_PLUGINS=bundled

USER root
RUN sed 's/^.*X-Forwarded-Prefix.*/# &/g' -i /usr/local/share/lua/5.1/kong/templates/nginx_kong.lua
USER kong

Note I am commenting the X-Forwarded-Prefix from the template (please, help me out to deal with in a better way), then I have fired a request to my AirFlow and it worked as expected!

RyouZhang commented 4 years ago

@gszr my upstream app is django,I think the key point is my Location header don't have host.

carnei-ro commented 4 years ago

@RyouZhang I suggest you check how your App handles the redirect. The only change I've found comparing Kong 2.0 with 2.1 was the presence of the X-Forwarded-Prefix and some frameworks may consider this header when building the string for the Location header.

gszr commented 4 years ago

@carnei-ro That is correct. 8773aab2c introduced support for X-Forwarded-Prefix, which upstream services may or may not comply with when building the Location header. I will leave this open for now while we give some more thought to it.

carnei-ro commented 4 years ago

@gszr I wish I could disable sending this header in some routes. For me, it is easier to change the API GW than a legacy App. Is there any possibility to accomplish that? (as I said, I commented the nginx template for some Kong clusters - not a good idea.) I have tried add the "request-transformer" plugin but it does not have effect since the template uses $upstream_x_forwarded_prefix variable, and to use a Kong serverless function I, personally, consider it risky.

gszr commented 4 years ago

@carnei-ro An alternative is to set the trusted_ips property; then, if the client passes in X-Forwarded-Prefix as /, Kong will trust and forward that value to the upstream service.

RyouZhang commented 4 years ago

@gszr in kong/runloop/handler.lua the var. http_x_forwarded_prefix is nil?? image

gszr commented 4 years ago

@gszr in kong/runloop/handler.lua the var. http_x_forwarded_prefix is nil?? image

@RyouZhang That code path does what I described above: if client IP is in trusted_ips, it proxies whatever values the client passed in X-Forward-* headers (headers are exposed by Nginx with http_ variables). Otherwise, Kong inserts its own values for those headers.

RyouZhang commented 4 years ago

this is my questions,why the forwarded_prefix = var.request_uri ? I think “/” is default value

bungle commented 4 years ago

@RyouZhang, isn't the request_uri the expected one when no trusted x-forwarded-prefix header is present in request? That follows similar pattern as with other X-Forwarded-*, that is the Kong will add it if they are missing so that next proxy or upstream can still get the original value.

bungle commented 4 years ago

@RyouZhang, or perhaps I implemented it wrong... should it only have the stripped path perhaps?

bungle commented 4 years ago

What I mean, perhaps what I implemented is what is called X-Forwarded-Path, and not the X-Forwarded-Prefix. Where the -Path is the original path and the -Prefix should instead be the one the part of the path that we stripped?

RyouZhang commented 4 years ago

What I mean, perhaps what I implemented is what is called X-Forwarded-Path, and not the X-Forwarded-Prefix. Where the -Path is the original path and the -Prefix should instead be the one the part of the path that we stripped?

I think X-Forwarded-Prefix is a custom params for upstream, maybe a config for service or router. default X-Forwarded-Prefix is empty value

bungle commented 4 years ago

What I mean, perhaps what I implemented is what is called X-Forwarded-Path, and not the X-Forwarded-Prefix. Where the -Path is the original path and the -Prefix should instead be the one the part of the path that we stripped?

I think X-Forwarded-Prefix is a custom params for upstream, maybe a config for service or router. default X-Forwarded-Prefix is empty value

Where do you get that? Usually what I see is that it is ”/” by default.

RyouZhang commented 4 years ago

What I mean, perhaps what I implemented is what is called X-Forwarded-Path, and not the X-Forwarded-Prefix. Where the -Path is the original path and the -Prefix should instead be the one the part of the path that we stripped?

I think X-Forwarded-Prefix is a custom params for upstream, maybe a config for service or router. default X-Forwarded-Prefix is empty value

Where do you get that? Usually what I see is that it is ”/” by default.

for example: nginx proxy -> upstream /blog/index.html -> /index.html then upstream server return 301 and location is /index_new.html it failed 404 will get, because the real 301 locatuon is /index_new.html not /blog/index_new.html

in this case, the route need a x-forward-prefix set /blog

bungle commented 4 years ago

Yes, e.g. if kong strips prefix, that prefix need to be set as X-Forwarded-Prefix, otherwise /?

RyouZhang commented 4 years ago

Yes, e.g. if kong strips prefix, that prefix need to be set as X-Forwarded-Prefix, otherwise /?

Yes,service set X-Forwarded-Prefix otherwise / is enough

bungle commented 4 years ago

@RyouZhang and @carnei-ro,

There is now a PR #6201 to fix this. Can you please try it out and report any issues / give feedback, thank you!

gszr commented 4 years ago

Hi all, we have merged the fix for this issue https://github.com/Kong/kong/pull/6222; it will be part of the upcoming 2.1.3 release. Please give it a try! Closing this. The X-Forwarded-Path addition will be merged into next and released in the upcoming 2.2 release.