splewis / get5

CS:GO Sourcemod plugin for competitive matches/scrims
GNU General Public License v3.0
558 stars 176 forks source link

Make ServerID string #940

Closed FlowingSPDG closed 1 year ago

FlowingSPDG commented 1 year ago

Hello, currently I'm developing Got5(github.com/FlowingSPDG/Got5 ). I am working on authentication system for remote logging(get5_remote_log_url), but I think I will need some change to implement auths.

Because current get5 remote logging only supports one HTTP header. This is making me very difficult to implement auth because there is no way to identify match without matchID(get5 won't send any MatchID if no match loaded e.g. match_config_load_fail.) so I decided to get corresponding match by using serverID but it only supports integer(afaik). My current setup generates game server on each match so it can be very big value(costs firestore performance).

so I want to make ServerID string to make more scalability on get5/get5 related system.

Thank you, my English is not very good enough to explain why I need this, so feel free to ask me a question to get more details.

https://github.com/FlowingSPDG/Got5/issues/25

https://github.com/FlowingSPDG/Got5/blob/f9c9a32347f2a659ec6e4dd4c1e645385408b6b8/route/events.go#L290-L333

FlowingSPDG commented 1 year ago

This is suggestion/feature request btw.

nickdnk commented 1 year ago

You should only see match_config_load_fail if your match configuration is invalid. In an automated system, this should be totally preventable.

You can use a JWT to include whatever you want in the Authorization header: https://jwt.io/ - that should solve the problem.

nickdnk commented 1 year ago

It seems Cvars may be limited to 128 characters for some reason, so JWTs may be problematic. You can just do: Authorization: <token>;<server id> and split the string on ; or something similar, if you want to provide more than 1 value in your header.

FlowingSPDG commented 1 year ago

You should only see match_config_load_fail if your match configuration is invalid. In an automated system, this should be totally preventable.

You can use a JWT to include whatever you want in the Authorization header: https://jwt.io/ - that should solve the problem.

match_config_load_fail is just an example, what I wanted to do is implement auth system to prevent receiving request from unknown game servers or attackers. I'll try Authorization: <token>;<server id> way, and possibly I want to try JWT way(if you can increase number of character limit on Auth header)

nickdnk commented 1 year ago

Authorization does exactly this, so I don't understand what the problem is if the proposed solution won't work.

Why can you not use integers for server ID?

FlowingSPDG commented 1 year ago

Currently Im using firestore(firebase) for Database, but using integer on document key causes performance issue(https://firebase.google.com/docs/firestore/best-practices#hotspots) . And in my case, server is kinda like disposable(once match is done, we simply recreates new server on Google Compute Engine) so server ID kept increasing.

nickdnk commented 1 year ago

Why can't you just cast the integer to a string on your end? I would assume that a string with just integers in it would be equivalent, key-wise, to any other string.

FlowingSPDG commented 1 year ago

No as I said before, it causes hot spot(performance spike/higher latency) on Firestore. Using string on ServerID makes more expandability for my purpose.

Why ServerID is restricted to Integer when MatchID supports string? I know making it takes your time but there is no reason to reject expanding availability of get5.

nickdnk commented 1 year ago

No as I said before, it causes hot spot(performance spike/higher latency) on Firestore. Using string on ServerID makes more expandability for my purpose.

Why ServerID is restricted to Integer when MatchID supports string? I know making it takes your time but there is no reason to reject expanding availability of get5.

This is not a string vs. integer issue, this is a key lexicography problem. Also, this is not a an issue unless you have 500+ operations per second for lexicographically close keys, according to the documentation you linked to. I don't know what you're building exactly or why a document-based database is a good choice for it (normal SQL should be far superior, or at the very least equally good, for the kind of data structure CSGO produces), but this is not really an issue Get5 can be concerned with. Is this a problem you are already facing, or something you a worried about because it says so? Remember not to preemptively microoptimize.

With that said; there is no reason that the server ID needs to be an integer and I never said we couldn't or wouldn't change it. It was just built this way originally. It can be changed, but it affects the Get5 MySQL plugin, which currently uses integers for the server ID column, and it's a breaking change in both the JSON events and the SourceMod forwards. We can do it, and I also think a string makes more sense, I just wanted to understand exactly why this was such big deal first.

FlowingSPDG commented 1 year ago

hmm okay then I think I was worrying about it too much I just added JWT auth for my system(it was really close to 128 chars) so I do not have any issue for now, however if it is possible to change serverID to string instead of integer it is going to be awesome for me. I know it costs on development because some event and backup system relies on serverID so it is not easy dicision for now it can be int for me, but using string gives more expandability for developers so I still want it if I have chance!

nickdnk commented 1 year ago

@FlowingSPDG Please try the latest pre-release for string support in get5_server_id: https://github.com/splewis/get5/releases/tag/v0.13.0-53494a3

FlowingSPDG commented 1 year ago

@nickdnk Thank you! I will try pre-release on my system!