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

don't close health check directly #3690

Merged
merged 1 commit into from
Jun 13, 2024
Merged

Conversation

Outfluencer
Copy link
Collaborator

@Outfluencer Outfluencer commented Jun 1, 2024

Fixes #3689
Just check if the connection is a health check if so don't log exceptions

@md-5
Copy link
Member

md-5 commented Jun 12, 2024

So you're just leaving a connection dangling because it started with a health check header?

This does not seem right and would be a DoS vector

@Outfluencer
Copy link
Collaborator Author

It can only be a health check if proxy protocol is enabled and if so, you should firewall the port,
so i see no ddos risk

@Outfluencer
Copy link
Collaborator Author

Also the behaviour is now the same as before, only exceptions are not printed for healthchecks

@md-5
Copy link
Member

md-5 commented Jun 12, 2024

Also the behaviour is now the same as before, only exceptions are not printed for healthchecks

Right, so surely now the bug 6e17517 was designed to fix is back?

@andreasdc
Copy link

It can only be a health check if proxy protocol is enabled and if so, you should firewall the port, so i see no ddos risk

Everyone can send the health check if I'm not wrong.

@Outfluencer
Copy link
Collaborator Author

What do you mean? the problem is that if you send custom data and try to receive a custom response via haproxy it does not work with my old pr, now it does

@Outfluencer
Copy link
Collaborator Author

The bug is not back but we got this as problem with my change #3689

@Outfluencer
Copy link
Collaborator Author

It can only be a health check if proxy protocol is enabled and if so, you should firewall the port, so i see no ddos risk

Everyone can send the health check if I'm not wrong.

BungeeCord only decodes bytes to a health check if the HaProxyDecoder is in the pipe, that is only the case if proxy protocol is enabled, and if so you should firewall the port so only the right HAProxy can access it

@andreasdc
Copy link

It can only be a health check if proxy protocol is enabled and if so, you should firewall the port, so i see no ddos risk

Everyone can send the health check if I'm not wrong.

BungeeCord only decodes bytes to a health check if the HaProxyDecoder is in the pipe, that is only the case if proxy protocol is enabled, and if so you should firewall the port so only the right HAProxy can access it

That's right, but I think everyone can use the proxy to send such packets, but I'm not sure.

@Outfluencer
Copy link
Collaborator Author

It can only be a health check if proxy protocol is enabled and if so, you should firewall the port, so i see no ddos risk

Everyone can send the health check if I'm not wrong.

BungeeCord only decodes bytes to a health check if the HaProxyDecoder is in the pipe, that is only the case if proxy protocol is enabled, and if so you should firewall the port so only the right HAProxy can access it

That's right, but I think everyone can use the proxy to send such packets, but I'm not sure.

?

@md-5
Copy link
Member

md-5 commented Jun 12, 2024

What do you mean? the problem is that if you send custom data and try to receive a custom response via haproxy it does not work with my old pr, now it does

This change reverts 6e17517 which fixed #3608

How is #3608 not reintroduced in cases where HAProxy doesn't try to send custom data?

Not printing an exception to the console after the connection is already closed will not affect HAProxy

@Outfluencer
Copy link
Collaborator Author

What do you mean? the problem is that if you send custom data and try to receive a custom response via haproxy it does not work with my old pr, now it does

This change reverts 6e17517 which fixed #3608

How is #3608 not reintroduced in cases where HAProxy doesn't try to send custom data?

Not printing an exception to the console after the connection is already closed will not affect HAProxy

Why will it not affect it? We just dont want to log the exception that is caused by the connection close of an health check.
Isnt this the solution for it?

@md-5
Copy link
Member

md-5 commented Jun 12, 2024

I see, so #3608 is purely cosmetic

@Outfluencer
Copy link
Collaborator Author

I see, so #3608 is purely cosmetic

Yes

@md-5 md-5 merged commit 006a14a into SpigotMC:master Jun 13, 2024
4 checks passed
@andreasdc
Copy link

Also the behaviour is now the same as before, only exceptions are not printed for healthchecks

How it's the same if channel is not being closed with the latest change?

@Outfluencer
Copy link
Collaborator Author

Also the behaviour is now the same as before, only exceptions are not printed for healthchecks

How it's the same if channel is not being closed with the latest change?

Because haProxy closes the connection by itself by default. I only closed the channel so we dont receive rst from the haProxy that causes the exception. Now we receive the rst und the exception is thrown but we dont print it for healthchecks. In both cases there are no exceptions spammed anymore

thxrben pushed a commit to thxrben/BungeeCord that referenced this pull request Jun 16, 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

Successfully merging this pull request may close these issues.

HAProxy layer 7 health-check not possible anymore
3 participants