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

Return useful error message on potential HTTP connect to Transport port #10108

Merged

Conversation

spinscale
Copy link
Contributor

In case a HTTP client connects to the transport protocol and issues a
HTTP method followed by a space, we can just try to be smart and return
a string back to the client to point the user to the fact that the wrong
port has been used.

Closes #2139

bufferStartsWith(buffer, readerIndex, 'H', 'E', 'A', 'D', ' ') ||
bufferStartsWith(buffer, readerIndex, 'D', 'E', 'L', 'E', 'T', 'E', ' ') ||
bufferStartsWith(buffer, readerIndex, 'O', 'P', 'T', 'I', 'O', 'N', 'S', ' ') ||
bufferStartsWith(buffer, readerIndex, 'T', 'R', 'A', 'C', 'E', ' ')) {
Copy link
Member

Choose a reason for hiding this comment

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

Missing PATCH here?

@dakrone
Copy link
Member

dakrone commented Mar 16, 2015

Left one comment, This looks pretty good to me, now I'm just curious how different clients will handle a non-HTTP response :)

@spinscale
Copy link
Contributor Author

I tested with curl and chrome and those displayed correctly.. (display as in returned the string as response)

@spinscale spinscale force-pushed the 1503-issue-http-detection-on-transport branch from 22221f4 to a1fdfe7 Compare March 16, 2015 18:52
@spinscale
Copy link
Contributor Author

updated to support PATCH

@spinscale spinscale force-pushed the 1503-issue-http-detection-on-transport branch 2 times, most recently from d8b5771 to 7d9e146 Compare March 16, 2015 20:58
return true;
}

public class HttpOnTransportException extends ElasticsearchException {
Copy link
Member

Choose a reason for hiding this comment

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

Can you add javadocs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

@dakrone
Copy link
Member

dakrone commented Mar 16, 2015

Left comments about adding documentation, LGTM

if (ctx.getChannel().isOpen()) {
ChannelBuffer buffer = ChannelBuffers.wrappedBuffer(e.getCause().getMessage().getBytes(Charsets.UTF_8));
ctx.getChannel().write(buffer);
ctx.getChannel().close();
Copy link
Member

Choose a reason for hiding this comment

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

I think this close() should be in a finally block in case the write fails

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed this to use the channel future as it was intended to be.. so when the future is done, close is called

@dakrone
Copy link
Member

dakrone commented Mar 17, 2015

LGTM

In case a HTTP client connects to the transport protocol and issues a
HTTP method followed by a space, we can just try to be smart and return
a string back to the client to point the user to the fact that the wrong
port has been used.

Closes elastic#2139
Closes elastic#10108
@spinscale spinscale force-pushed the 1503-issue-http-detection-on-transport branch from 2b777f1 to b833451 Compare March 17, 2015 16:56
@spinscale spinscale merged commit b833451 into elastic:master Mar 17, 2015
spinscale added a commit that referenced this pull request Mar 17, 2015
In case a HTTP client connects to the transport protocol and issues a
HTTP method followed by a space, we can just try to be smart and return
a string back to the client to point the user to the fact that the wrong
port has been used.

Closes #2139
Closes #10108
@clintongormley clintongormley added >enhancement :Distributed/Network Http and internode communication implementations labels May 29, 2015
@clintongormley clintongormley changed the title Transport: Return useful error message on potential HTTP connect Return useful error message on potential HTTP connect to Transport port Jun 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed/Network Http and internode communication implementations >enhancement v1.6.0 v2.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants