LuckPerms

LuckPerms

41.4k Downloads

Rename default contexts on Bungee/Velocity

BrainStone opened this issue · 14 comments

commented

I have suggested it before, but never in it's own issue.
My proposal is as follows:

  • What's currently the server context should be renamed to the proxy context.
  • What's currently the world context should be renamed to the server context.

This makes per server nodes consistent with the bungee.

Additionally the backend servers should get the context proxy=none, so you can still have nodes that target individual servers (either just the bungee or just the backend server).

This is especially useful for per server groups and their respective prefixes.

commented

I'd like to address the one issue that has been brought up many many times, which is essentially that implementing this would be a breaking change.
Many have argued that a change in storage would be required, or that it is impossible to implement without breaking older stuff etc.

My two cents to this are as follows:

  • Change of storage format is not required, as a new database field for the proxy context is not necessary due to the rarity of it being used. That context should just go to where the other ones are going.
  • I see two ways of how to deal with introducing breaking changes:
    • Just break it. Jump a major version, have a banner up for like 2 years and call it a day. People running into this issue are typically fairly adept and clearly have decently deep understanding of what's going on
    • My preferred option would be to just add a config setting. When migrating the config, set it to false and when generating a new config set it to true (or vice versa, depending on which way the config works). This even allows people to opt in or opt out, depending on their needs.
commented

I was implying that but that’s a fair point to point out.

commented

In addition, I would suggest to also change server: in the config.yml of LPB to proxy: so that people won't get confused and to keep a certain consistency.
https://github.com/lucko/LuckPerms/blob/c86f6c0f368610152112ece73c898aa0954cfcc9/bungee/src/main/resources/config.yml#L29-L34

commented

Bumping it because I've yet seen another case where this causes massive issues.

commented

Yeah I support this change.

commented

It's pretty difficult to explain why it happens to some people. Sure, we can just tell them to set the prefixes again with a world context to cover the proxy but some people have a lot of groups and we typically get a lot of resistance when suggesting this solution.

Please consider adding proxy context soon 🙏

commented

It's not just difficult to explain the why but it's even more difficult to suggest somewhat decent solutions. Even helper groups that inherit the actual group twice, once with the per server context and once with the per world context, always fall short somewhere.
It always boils down to being a lot of work that really isn't necessary or the setup being very stiff. Often both.

I know you dread of making that change, but it needs to happen. The sooner, the better.

And frankly I don't know what concerns are left, but you major concern regarding upgrading has been addressed. So if there's anything else, feel free to start a discussion. We're always happy to find solutions to the problems there are.

commented

I'd like to keep the server context as it is, but rename world to connected-server on Bungee/Velocity.
I'm open to other names, ideally something shorter. backend-server is another consideration.

Currently, the server context is a way of isolating permissions to a specific LP instance, whether that's a Minecraft server or a proxy server doesn't matter - it's still a server.
Making server mean something different on the proxy would create an issue similar to the one at the moment with world.
On this basis, I don't think the behaviour of server really needs to be changed. (unless anyone has anything to say to convince me otherwise)

commented

This is especially useful for per server groups and their respective prefixes.

I guess this is a fair point I overlooked, hmm

commented

One point has been mentioned several times here, which is, that it helps to distinguish context more easly, especially when using LP on both proxy and server.
Most people won't think of "server" when they want to set a perm, group, ... on a proxy-level. A server for them is often the backend one and it can easly get confusing with when you want to set permissions for the Spigot Server only and then one for the proxy only...
2 different instances of LP both using the server context can become really confusing as you can't quite know which one now is about the proxy and which about the actual server.

commented

Currently, the server context is a way of isolating permissions to a specific LP instance

That is correct and that is exactly the issue here.

Contexts are used to narrow then the when and where a permission applies. The LP instances themselves are utterly useless in operation. While it makes sense being able to differentiate them (ideally with a dedicated context) it's not something the majority of users care about. They care about the when and where a permission or group applies.

Now that means to them they don't care how the backend servers are essentially worlds to the proxy. All they care is that they want to give groups and permissions, when a player is on a certain backend server. They care what server the player plays on, not which LP instance to target.
A fairly common use case is to have per server groups. Now while typically only the backend servers matter it's not unheard off that permissions and groups on the bungee should match the ones on the backend server.
Currently the only way to achieve this somewhat elegantly is by using a helper group. As mentioned that quickly runs into issue when there are many groups and multiple servers. The operations of the plugin become very complicated and it can quickly mean that only one person is able to properly set per server ranks. A very common complaint from server admins that did actually understand my proposed setup is that they don't know how to make sure their staff knows how to properly set per server ranks. And frankly most people had no idea what to do. As they seem to be not able to understand that LP refers to instances and not places.

Again I do understand your initial intention behind adding those contexts in the first place. Being able to target a specific LP instance makes a lot of sense on the surface. But it's not what people use this feature for. They couldn't care less about the instances. They only care about the wheres and whens of a player. Most commonly the server they currently play on.

So to summarize I propose:

  • Make sure the server context always refers to the backend server the player is on. For the backend server that naturally is equivalent with the LP instance
  • Add the proxy context to be able to differentiate the different proxies. It might make sense adding proxy=none to the backend server but I would be against that.
    Also change the server setting in the proxy config to proxy.
  • Add a config option that reenables the current behavior. If it's missing it defaults to true, but in the default config it's false. This means that for servers updating nothing changes but new servers will use the new and improved contexts
  • Add a context to identify the LP instance. Like instance. It would default to the same name as either the server or proxy setting in the respective LP config. That way you have the best of both worlds, should you ever actually need to target a specific LP instance instead of a backend server. Maybe even rename the server config to instance-name in the LP config. And the server and proxy contexts get adjusted accordingly.
    I personally am against this context but could definately understand an argument being made for it.

And if you are still not convinced how cumbersome the current system is, just setup a bungee network with 3 backend servers and 10 groups. Make sure you can add any group to a player that only applies to any one specific player with one command. For both the backend server and the bungee. Additionally the actual permission and prefix holding groups cannot have permissions with contexts. You can verify everything working by making sure the /lp user <user> info and the /lpb user <user> info always show the same prefixes.

commented

Bumping this again.

commented

There's no need to bump, I haven't forgotten about this. I just work on suggestions as and when I have time.

This is a complex problem with a potentially large impact on users setups if implemented incorrectly. It is also not something I consider to be urgent, the current system (whilst I admit is not perfect) is at least functional at the moment (and has been for the past 3/4 years...)

As well as the complexity of the code change, it also has knock-on effects on documentation and other behaviours (e.g. the include-global setting) which need to be considered and worked on.

commented

I've had an idea on how this could be implemented and minimize impact when updating:

Add a config setting that reenables the old (insane) behavior. In the default config it is set to false. But if the setting is missing it defaults to true.

EDIT:
Aww damn! I missed the anniversary of this issue! :(