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

Implement Velocity Proxy server module #47

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

scottbot95
Copy link

Implement Velocity Proxy server module.

It's basically just a special-purpose version of the minecraft-servers module. We probably could just use the minecraft-servers module but some properties (specifically serverProperties and whitelist) don't really make any sense for Velocity.

@scottbot95 scottbot95 marked this pull request as draft October 24, 2023 01:51
@scottbot95 scottbot95 marked this pull request as ready for review October 24, 2023 02:23
Copy link
Owner

@Infinidoge Infinidoge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good! I assume this has been tested in your setup and works well?

The main thing I would like before merging this would be to have it run through nixpkgs-fmt, as that is the main formatter I use. (I really should add more documentation to the repo for stuff like that...)

flake.nix Show resolved Hide resolved
modules/velocity.nix Show resolved Hide resolved
modules/velocity.nix Show resolved Hide resolved
@scottbot95
Copy link
Author

I can run the formatter. Might take me a day or two before I can get back to this. I've also seen other flakes that include a style check in the actual flake checks so you can setup some GH actions to run flake check and it will fail if the files aren't properly formatted. I will take a look at how that's implemented. If it's easy enough I might just include it in this PR as well (or a separate PR).

@Misterio77
Copy link
Contributor

Misterio77 commented Dec 14, 2023

Hi! I have some thoughts on this:

  • I think velocity.toml has to be mutable. Or it'll blow up in the slightest change or missing key.
  • This can and should be implemented through minecraft-server.servers.* instead of systemd.services.* directly. The rule of thumb is: always use the higher-level abstraction you can get away with using.
    • You're missing on super important features such as files and symlinks, which you can, by the way, use to implement config instead of manually linking configFile.
    • Due to that, there's a huge amount of duplicated code. This means improvements to minecraft-server.servers are not available here, and increases maintenance burden.
  • Mapping all options like that is a major footgun for maintenance. I recommend using the settings pattern instead.
    • What if options change between versions? They do change a lot. Will we have to actively hunt for velocity changelogs and add dozens of version checks and warnings for deprecation? This quickly gets out of hand.
    • As an example on how it's impossible to be through with this mapping, I believe player-info-forwarding-mode is missing.

I hate to be this person but, I don't think we should add specialized modules. Minecraft network setups vary a lot (what if they want two proxies? etc). We should focus effort in improving the base module while always keeping module complexity in check. Most of this is already possible (the convenience is very, very small):

services.minecraft-servers.servers.velocity = {
  enable = true;
  package = pkgs.velocity-server;
  files."velocity.toml".value = {
    bind = "0.0.0.0:25565";
    forwarding-secret-file = ...;
    servers = (mapAttrs (_: v: "localhost:${v.serverProperties.server-port}") config.minecraft-servers.servers) // {
      try = [ "foo" ];
    };
  };
};

VS

services.minecraft-servers.velocity = {
  enable = true;
  address = "0.0.0.0";
  port = 25565;
  forwarding-secret-file = ...;
  config.try = [ "foo" ];
};

In my opinion, the first is actually easier to use and much more consistent. With it, there's no need to learn a separate NixOS option set for velocity, just combine what you already know about minecraft-servers.servers and about velocity.toml.

I don't think we should give special treatment to specific configuration files. Each server has different ones (sometimes multiple files), and we can't hope to keep up with their changes. Even server.properties and whitelist.json are only around to lower the entry barrier for people coming from services.minecraft-server, are implemented using files/symlinks (and can be safely ommited or replaced with their equivalents), and finally don't have their format fully modeled as nixos options (so they're super duper easy to maintain).

If we're sure we want a specialized variant, it should be a very, very thin wrapper around minecraft-servers.servers.

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

Successfully merging this pull request may close these issues.

3 participants