centrifugal / centrifugo

Scalable real-time messaging server in a language-agnostic way. Self-hosted alternative to Pubnub, Pusher, Ably. Set up once and forever.
https://centrifugal.dev
Apache License 2.0
8.38k stars 594 forks source link

[bug] proxy connect with SubscribeOptions not working v5.4.4 #873

Closed morya closed 2 months ago

morya commented 2 months ago

Describe the bug.

SubscribeOptions field from connect proxy response, is not working.

version: v5.4.4

Centrifugo version isv5.4.4 Client library used is github.com/centrifugal/centrifuge v0.33.1-0.20240801054640-96afa7258679 Operating system is macos

Steps to Reproduce How can the bug be triggered?

with config.yaml for centrifugo like this:

proxy_connect_endpoint: http://192.168.1.1:8080/centrifugo-connect  # this is the python fastapi address
proxy_connect_timeout:  "2s"

Expected behavior What output or behaviour were you expecting instead?

join/leave events been emitted.

Code Snippets A minimum viable code snippet can be useful.

I use fastapi to be centrifugo proxy-handler.

# coding:utf8

import secrets

from pydantic import BaseModel
from fastapi import FastAPI

app = FastAPI()

class ReqCentConnect(BaseModel):
    client: str
    transport: str
    protocol: str
    name: str
    data: object

@app.post("/centrifugo-connect")
async def on_connect(req: ReqCentConnect):
    # uid = secrets.token_hex(4)
    uid = req.client.split("-")[0]
    return {
        "result": {
            "user": uid,
            "data": {
                    "logo": "abc.png",
            },
            "channels": ["room"],
            "subs": {
                "room": {
                    "override": {
                        "presence": {
                            "value": True,
                        },
                        "join_leave": {
                            "value": True,
                        },
                        "force_push_join_leave": {
                            "value": True,
                        },
                    },
                },
            }
        }
    }
morya commented 2 months ago

yes, a workaround method,

just split subscribe to another API command ( I think)

morya commented 2 months ago

I think, the related code lines are:

https://github.com/centrifugal/centrifugo/blob/60d3746d42bc95398d765eec8ffb9c36aed822e7/internal/proxy/connect_handler.go#L132

subscriptions := make(map[string]centrifuge.SubscribeOptions, len(result.Channels))
// ...

// should be changed to these
                joinLeave := chOpts.JoinLeave
                if result.Subs != nil {
                    opt := result.Subs[ch]
                    if opt != nil && opt.Override != nil && opt.Override.ForcePushJoinLeave != nil {
                        joinLeave = opt.GetOverride().GetForcePushJoinLeave().GetValue()
                    }
                }

                subscriptions[ch] = centrifuge.SubscribeOptions{
                    EmitPresence:      chOpts.Presence,
                    EmitJoinLeave:     chOpts.JoinLeave,
                    PushJoinLeave:     joinLeave, // chOpts.ForcePushJoinLeave, // read directly from chOpts.ForcePushJoinLeave , subs from ConnectResponse cmd is ignored.
                    EnableRecovery:    chOpts.ForceRecovery,
                    EnablePositioning: chOpts.ForcePositioning,
                    RecoveryMode:      chOpts.GetRecoveryMode(),
                    Source:            subsource.ConnectProxy,
                    HistoryMetaTTL:    time.Duration(chOpts.HistoryMetaTTL),
                }
FZambia commented 2 months ago

Hello @morya ,

Thanks for the detailed report, you are right – subs field is not used now by Centrifugo when processing the response from the connect proxy.

I see the following things to be fixed here:

  1. Start processing ConnectResult.subs field in the same way we do in other places now. I just opened PR - https://github.com/centrifugal/centrifugo/pull/874
  2. Currently doc incorrectly describes override object - it still uses format from Centrifugo v3, while some fields were renamed in Centrifugo v4, so this should be updated to reflect correct field names (which correspond to current channel namespace options). PR with doc fix - https://github.com/centrifugal/centrifugal.dev/pull/52

One note btw, I see you are using both channels and subs in your connect result:

    return {
        "result": {
            ...
            "channels": ["room"],
            "subs": {
                "room": {
                    ...
                },
            }
        }
    }

You can stick to sth single here - whether only channels, or only to subs. After the fix they should be processed one after another, just subs allow setting some extra channel options while channels is just an array of server-side channels without any customization.

morya commented 2 months ago

Alright, I use channels for this kind of response from our project, which works perfectly, until I want to customize sub options.

I thought, it's should be the subs to customize sub options, and channels to describe server sub option, when I was digging through code of centrifugo.

I will stick to subs option and drop channels when new version is released.

I was planning open a PR, but afraid of send miss leading codes, which may lead to extra pointless review efforts.

Best regards.

morya commented 2 months ago

just confirmed that, github.com/centrifugal/gocent/v3 v3.3.0 is not working when using with these codes:

func doServerSubscribe(client string) {
    cc := gocent.New(gocent.Config{
        Addr: "http://127.0.0.1:8000/api",
        Key:  "fake-key",
    })
    err := cc.Subscribe(context.TODO(), "foo", client, gocent.WithPresence(true), gocent.WithJoinLeave(true))
    if err != nil {
        log.Fatalf("Server Subscribe failed %v", err)
    }
}

params from gocent.WithPresence(true), gocent.WithJoinLeave(true) will be ignored from centrifugo v3 to v5

encoded json result is:

{"method":"subscribe","params":{"channel":"foo","user":"3e3fc43b-bef9-4ab5-a3da-4c96c688351e","presence":true,"join_leave":true}}

and, this will be ignored when decode from centrifugo server with SubscribeRequest from internal/apiproto/api.pb.go

SubscribeRequest require all fields inside override sub field.

FZambia commented 2 months ago

just confirmed that, github.com/centrifugal/gocent/v3 v3.3.0 is not working when using with these codes:

Could you please open a separate issue in gocent about it?

morya commented 2 months ago

ok

FZambia commented 2 months ago

Should work correctly in v5.4.5