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

Added support for Bungeecord's proxy_protocol option #8

Merged
merged 1 commit into from
Apr 19, 2021

Conversation

itzg
Copy link
Contributor

@itzg itzg commented Apr 18, 2021

I have a request itzg/docker-mc-proxy#51 from a user of my bungeecord Docker image (and indirectly itzg/mc-monitor#6 ) to support Bungeecord's proxy_protocol option.

Since this new configuration of Pinger needed to be combined with context/timeout support I decided to introduce a functional options pattern to allow for backward/forward compatible options on the New function.

For example:

	options := []mcpinger.McPingerOption{}
	if c.Timeout > 0 {
		options = append(options, mcpinger.WithTimeout(c.Timeout))
	}
	if c.UseProxy {
		options = append(options, mcpinger.WithProxyProto(c.ProxyVersion))
	}
	pinger := mcpinger.New(c.Host, uint16(c.Port), options...)

@raqbit
Copy link
Owner

raqbit commented Apr 19, 2021

Looks good!

I've been working on rebasing mc-pinger on mcproto, which is an implementation of the Minecraft protocol in Go: #9. mcproto started as the protocol bits of mc-pinger, but slowly got out of sync with added (de-)serialization code generation, support for SRV records and some other features. With these changes I've also redone the mc-pinger API, to make it more like net.Dial et al (Breaking change, but as the API surface is small it should not be much of an issue).

With this PR in mind, I think adding options to each of the Ping-functions would be nice, especially with a WithProtoOption() option to pass options to the underlying mcproto library (add custom packets, add proxy support, configure logging etc.)

If you have the time, I'd appreciate it if you could take a look at the mcproto draft PR and give some feedback so I know if I'm heading into the right direction with this new mc-pinger API.

I could merge this PR right now, but then I'd expect to be breaking the API with those changes. Another option is to include proxy support in mcproto, and then adding the option to the new mc-pinger API.

What do you like best?

@itzg
Copy link
Contributor Author

itzg commented Apr 19, 2021

That sounds like a great strategy and I'll definitely look over that PR.

To address the user request I would prefer to merge this PR. After that I can take my time with the subsequent release to incorporate the mcproto related changes.

@raqbit raqbit merged commit d244b73 into raqbit:master Apr 19, 2021
@raqbit
Copy link
Owner

raqbit commented Apr 19, 2021

I've released v0.2.0 just now, next release will probably be the one including the mcproto changes.

Thanks!

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.

2 participants