Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Allow user to toggle Connection header #25

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

Closed
troylar wants to merge 2 commits into bcuff:master from troylar:master
Closed

Conversation

@troylar
Copy link

@troylar troylar commented Aug 10, 2017

Our requests were failing because the ElasticSearch library creates a Connection:Keep-Alive header by default. Amazon expects the request to be signed with the header Connection:close. Overriding this header allowed us to successfully query the AWS ES cluster.

This pull request simply allows you to switch between "Keep-Alive" and "close."

Copy link
Author

@troylar troylar left a comment

Choose a reason for hiding this comment

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

This is the error we are getting from AWS:

The request signature we calculated does not match the signature you provided. Check your AWS Secret Access Key and signing method. Consult the service documentation for details.\n\nThe Canonical String for this request should have been\n'GET\n/_cluster/health\n\naccept:application/json\nconnection:close\ncontent-type:application/json\nhost:search-poc-test-vp7ggjs3lreczj7w7b3kzs5qqy.us-east-2.es.amazonaws.com\nx-amz-date:20170810T211424Z\n\naccept;connection;content-type;host;x-amz-date\ne3b0c44298fc1c149afbf4c8996fb92427ae

protected override System.Net.HttpWebRequest CreateHttpWebRequest(RequestData requestData)
{
var request = base.CreateHttpWebRequest(requestData);
request.Headers.Remove("Connection");
Copy link
Author

Choose a reason for hiding this comment

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

This is duplicate code, but the request object is different for both methods.

Copy link
Owner

bcuff commented Aug 10, 2017

Any chance you're going through a proxy that might be messing with the HTTP? We are successfully connecting directly to an AWS Elasticsearch service cluster using https:// and have never encountered this. Also using Keep Alive is important for performance reasons.

Copy link
Author

troylar commented Aug 10, 2017 via email

Interesting. Two of us are getting the same response in two different states on two completely different networks on two completely different clusters.
...
On Aug 10, 2017 5:21 PM, "Brandon" ***@***.***> wrote: Any chance you're going through a proxy that might be messing with the HTTP? We are successfully connecting directly to an AWS Elasticsearch service cluster using https:// and have never encountered this. Also using Keep Alive is important for performance reasons. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#25 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB151FW0FSKgC6UbsPv7ubG5mcjyRRiBks5sW3RYgaJpZM4Oz_Hg> .

Copy link
Owner

bcuff commented Aug 10, 2017

You can try omitting the connection header from the signature. I don't think it needs to be part of the signature. See f3f3a52

If that fixes the issue let me know and we can include that change. I would hate to turn off keep-alive by default.

Copy link
Author

troylar commented Aug 10, 2017 via email
edited
Loading

100% agree with you and we can enable it by default. I don't like this solution long term because it doesn't really address the "why" but it does work for now.

Copy link
Author

troylar commented Aug 11, 2017

So removing the Connection header worked as well.

Copy link
Owner

bcuff commented Aug 11, 2017

Cool. I'll merge that commit and publish tomorrow morning. Going to close this PR. thanks for reporting the issue.

@bcuff bcuff closed this Aug 11, 2017
Copy link
Author

troylar commented Aug 11, 2017

@bcuff FYI -- this where the keep-alive is being set in the Elasticsearch-net code . . .It only happens for the DOTNETCORE directive. (Line 225)

https://github.com/elastic/elasticsearch-net/blob/70571f15a8fcd5b6397b9aa9c3a9c81d37014b51/src/Elasticsearch.Net/Connection/HttpConnection-CoreFx.cs

Copy link
Author

troylar commented Aug 11, 2017

And where the addition was made two months ago. Just wanted to give a full context. Thanks for making this change.

elastic/elasticsearch-net@0e66e5b

Copy link
Owner

bcuff commented Aug 11, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

AltStyle によって変換されたページ (->オリジナル) /