bug: enable_websocket be set to true in upstream_id config does not work

[sasakiyori]

Current Behavior

I register a route by script like below:

#!/bin/bash
curl http://127.0.0.1:9080/apisix/admin/upstreams/ws -H "X-API-KEY: edd1c9f034335f136f87ad84b625c8f1" -X PUT -d '
{
    "name": "ups_ws",
    "type": "roundrobin",
    "enable_websocket": true,
    "nodes": {
        "node1:12345": 1
    }
}'

curl http://127.0.0.1:9080/apisix/admin/routes/ws -H "X-API-KEY: edd1c9f034335f136f87ad84b625c8f1" -X PUT -d '
{
    "name": "websocket",
    "method": "POST",
    "uris": [
        "/api/websocket*"
    ],
    "plugins": {
        "proxy-rewrite": {
            "regex_uri": ["/api/", "/svc/"]
        }
    },
    "upstream_id": "ws"
}'

When I call the request, I found in upstream nodes, Connection and Upgrade was not in the request headers, enable_websocket did not work.

Expected Behavior

I expects enable_websocket will work, and the request headers should contain Connection and Upgrade.

Actually when I use upstream directly instead of using upstream_id, enable_websocket works normally.
When I set enable_websocket in routes setting, it works too.

Error Logs

I found that when I use upstream_id setting, the code does not concern about the value of enable_websocket, which will set Connection and Upgrade headers later. But upstream does condition checks for it, route and route.service_id checks as well.

https://github.com/apache/apisix/blob/master/apisix/init.lua#L463

    if up_id then
        local upstream = apisix_upstream.get_by_id(up_id)
        if not upstream then
            if is_http then
                return core.response.exit(502)
            end

            return ngx_exit(1)
        end

        api_ctx.matched_upstream = upstream

    else
        if route.has_domain then
            local err
            route, err = parse_domain_in_route(route)
            if err then
                core.log.error("failed to get resolved route: ", err)
                return core.response.exit(500)
            end

            api_ctx.conf_version = route.modifiedIndex
            api_ctx.matched_route = route
        end

        local route_val = route.value
        -- if upstream_id does not exists, it will check upstream.enable_websocket
        if route_val.upstream and route_val.upstream.enable_websocket then
            enable_websocket = true
        end

        api_ctx.matched_upstream = (route.dns_value and
                                    route.dns_value.upstream)
                                   or route_val.upstream
    end

    if enable_websocket then
       -- this sets the headers I need
        api_ctx.var.upstream_upgrade    = api_ctx.var.http_upgrade
        api_ctx.var.upstream_connection = api_ctx.var.http_connection
        core.log.info("enabled websocket for route: ", route.value.id)
    end

Steps to Reproduce

1. Run apisix via docker image
2. run script:

#!/bin/bash
curl http://127.0.0.1:9080/apisix/admin/upstreams/ws -H "X-API-KEY: edd1c9f034335f136f87ad84b625c8f1" -X PUT -d '
{
    "name": "ups_ws",
    "type": "roundrobin",
    "enable_websocket": true,
    "nodes": {
        "node1:12345": 1
    }
}'

# this route does not work for enable websocket
curl http://127.0.0.1:9080/apisix/admin/routes/wsNotWork -H "X-API-KEY: edd1c9f034335f136f87ad84b625c8f1" -X PUT -d '
{
    "name": "websocket_not_work",
    "method": "POST",
    "uris": [
        "/api/websocket1*"
    ],
    "plugins": {
        "proxy-rewrite": {
            "regex_uri": ["/api/", "/svc/"]
        }
    },
    "upstream_id": "ws"
}'

# this routes works for enable websocket
curl http://127.0.0.1:9080/apisix/admin/routes/wsWorks -H "X-API-KEY: edd1c9f034335f136f87ad84b625c8f1" -X PUT -d '
{
    "name": "websocket_work",
    "method": "POST",
    "uris": [
        "/api/websocket2*"
    ],
    "plugins": {
        "proxy-rewrite": {
            "regex_uri": ["/api/", "/svc/"]
        }
    },
    "upstream": {
        "type": "roundrobin",
        "enable_websocket": true,
        "nodes": {
            "node1:12345": 1
        }
    }
}'

Environment

• APISIX version (run apisix version): 2.13.0
• Operating system (run uname -a): Linux 0c06ebd3850b 5.10.76-linuxkit # 1 SMP Mon Nov 8 10:21:19 UTC 2021 x86_64 Linux
• OpenResty / Nginx version (run openresty -V or nginx -V): openresty/1.19.3.2
• etcd version, if relevant (run curl http://127.0.0.1:9090/v1/server_info):
• APISIX Dashboard version, if relevant:
• Plugin runner version, for issues related to plugin runners:
• LuaRocks version, for installation issues (run luarocks --version):

[spacewander]

Actually when I use upstream directly instead of using upstream_id, enable_websocket works normally.

enable_websocket should be configured in the route. As you can find out that it's missing in the upstream's doc. We keep the old code just for the compatible but it doesn't mean it can work with upstream_id.

I am curious why you will set it in the upstream as this field should already be removed from the doc?

[sasakiyori]

I have already forgotten where I read the usage of setting it in the upstream.😂
When I found upstream can work but upstream_id can't, I read the source code and thought this might be a bug.
Since enable_websocket no longer work for upstream_id, I will configure it in the route :)

[spacewander]

@sasakiyori
Would you like to remove the enable_websocket in the upstream completely? As it is considered deprecated in 2020.

[sasakiyori]

@spacewander Do you mean completely remove the logic about enable_websocket in the upstream from the code? If so, I want to work on this.

[spacewander]

Yes, PR is welcome!