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

Support for .NETFramework 4.5 and .NETStandard 1.3 #16

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

Merged
bcuff merged 9 commits into bcuff:master from nanestev:master
Dec 5, 2016

Conversation

@nanestev
Copy link
Collaborator

@nanestev nanestev commented Nov 16, 2016
edited
Loading

Support for net45 comes at the price of a couple compiler directives because ElasticSearch.Net for net45 has different API. Also I needed to create abstraction of request and header collection to be able to reuse the original code with both HttpRequestMessage/WebHeaderCollection and HttpWebRequest/HttpRequestHeaders and all the differences that come with them. Very few other changes of the original code like ditching JavaScriptSerializer in favor of JSON.Net. Unit tests run for both netcore and net451 (because NUnit runner is targetting net451).
The signing key was not present so had to switch off signing, but you can always switch it back on.
Also I added Token property to the AwsSettings class. It is needed, because we can't use role credentials without it (for example when working with assumed roles or ECS task roles).

Cheers!
N

@nanestev nanestev mentioned this pull request Nov 16, 2016
Copy link
Owner

@bcuff bcuff left a comment

Choose a reason for hiding this comment

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

Sorry for the late reply. I've been busy lately. This looks mostly good. There are just a few things to resolve.

Thanks!

/// Gets or sets the security token
/// Required.
/// </summary>
public string Token { get; set; }
Copy link
Owner

Choose a reason for hiding this comment

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

we shouldn't need to expose this since it's only necessary to provide it when using instance profile credentials. Instance profile credentials will be used when no access key or sercret are provided.

Copy link
Collaborator Author

@nanestev nanestev Nov 18, 2016
edited
Loading

Choose a reason for hiding this comment

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

It is actually needed when we use role credentials. Storing credentials in config files is discouraged in favour of using roles, and to follow that route we will need the token. That is exactly my case by the way - I run my app in a docker container which is registered in an ECS task. The task is assigned a role which is different from the instance's role and has all the necessary permissions. I extract credentials from my task role and have to pass them along with the token to be able to use them. There are other scenarios when we might need to pass the token. For example, when we want to work with assumed roles or want to use temporary (session) credentials derived from other credentials. In fact, I was thinking that since the user knows best what credentials they want to use including the token, they should always pass all authentication data to the library. Or we can call internally Amazon.Runtime.FallbackCredentialsFactory.GetCredentials() which will create dependency on AWSSDK.Core, but it's still better than always assuming that we should get the instance role. These further changes are perhaps subject of a longer conversation, so let's leave it as it is for now.

Copy link
Owner

Choose a reason for hiding this comment

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

Okay, that makes sense then.


namespace Elasticsearch.Net.Aws
{
public class HttpRequestMessageAdapter : IRequest
Copy link
Owner

Choose a reason for hiding this comment

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

This can be internal. I'd rather expose only what the consumer needs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree.


namespace Elasticsearch.Net.Aws
{
public class HttpWebRequestAdapter : IRequest
Copy link
Owner

Choose a reason for hiding this comment

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

internal.


namespace Elasticsearch.Net.Aws
{
public interface IHeaders
Copy link
Owner

Choose a reason for hiding this comment

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

internal


namespace Elasticsearch.Net.Aws
{
public interface IRequest
Copy link
Owner

Choose a reason for hiding this comment

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

internal

Copy link

Hey, what's the status of this pull-req? Been testing locally, would love to have access to a pre-release on nuget.org :)

Thanks

Copy link
Owner

bcuff commented Dec 5, 2016

@davemcdermid Looks good. I just need to test it on my end before I push a release. I'll try to get that done soon.

@bcuff bcuff merged commit 35d3a28 into bcuff:master Dec 5, 2016
Copy link
Owner

bcuff commented Dec 5, 2016

@davemcdermid @nanestev nuget released as 2.2.0

nanestev reacted with thumbs up emoji

Copy link

Fantastic, thank you!

Copy link
Owner

bcuff commented Dec 5, 2016

you're welcome

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

Reviewers

@bcuff bcuff bcuff approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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