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

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

Closed
morya opened this issue Aug 10, 2024 · 8 comments
Closed

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

morya opened this issue Aug 10, 2024 · 8 comments

Comments

@morya
Copy link

morya commented Aug 10, 2024

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
Copy link
Author

morya commented Aug 10, 2024

yes, a workaround method,

just split subscribe to another API command ( I think)

@morya
Copy link
Author

morya commented Aug 10, 2024

I think, the related code lines are:

for _, ch := range result.Channels {

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
Copy link
Member

FZambia commented Aug 11, 2024

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 - Process connect proxy result subs #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 - Fix docs for subscribe options override centrifugal.dev#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
Copy link
Author

morya commented Aug 11, 2024

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
Copy link
Author

morya commented Aug 12, 2024

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
Copy link
Member

FZambia commented Aug 12, 2024

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
Copy link
Author

morya commented Aug 12, 2024

ok

@FZambia
Copy link
Member

FZambia commented Aug 14, 2024

Should work correctly in v5.4.5

@FZambia FZambia closed this as completed Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants