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

Deprecate specifying credentials through env vars, sys props, and remove profile files #22567

Merged
merged 4 commits into from Jan 19, 2017

Conversation

rjernst
Copy link
Member

@rjernst rjernst commented Jan 11, 2017

This is a follow up to #22479, where storing credentials secure way was
added.

closes #21041

… props, and profile files

This is a follow up to elastic#22479, where storing credentials secure way was
added.
@rjernst
Copy link
Member Author

rjernst commented Jan 11, 2017

I'm not sure how to test this, since before we didn't have tests, and all of these methods rely on sys props or env vars. I've considered having special test tasks for each one of these within the repository-s3 plugin. It might work, it would just mean adding some test classes as excluded in the normal test run. Thoughts?

@dadoonet
Copy link
Member

I think your change will also "fix" somehow what we discussed in #21041.
Can you do it as well for ec2 discovery?

I know that we have infra tests that probably needs to be modified if we do that for ec2 as well.

@rjernst
Copy link
Member Author

rjernst commented Jan 11, 2017

@dadoonet Yes I will do it for ec2 as well, but in a followup (I still need to convert ec2 to the new secure setting infra). And I want to get #22520 worked out before doing that (which will further simplify the settings we support for 6.0, and should also be done with ec2).

@rjernst
Copy link
Member Author

rjernst commented Jan 11, 2017

Ok I added tests as I described above, and this actually caught a bug in my new implementation (the aws providers all throw exception if they are not found, and the default chain handles this by wrapping in try/catch).

@rjernst
Copy link
Member Author

rjernst commented Jan 11, 2017

Given #21041, which I had forgot about, I removed profile file support entirely in this change (it didn't work with the security manager).

@rjernst
Copy link
Member Author

rjernst commented Jan 12, 2017

retest this please

Copy link
Member

@dadoonet dadoonet left a comment

Choose a reason for hiding this comment

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

I left one comment

credentials = new StaticCredentialsProvider(syspropCredentials);
} else {
logger.debug("Using instance profile credentials");
credentials = new InstanceProfileCredentialsProvider();
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 that using instance profile is a good thing actually and I have seen many people relying on that.
Basically credentials are only stored on AWS platform. Elasticsearch does not need to know them explicitly. So no one has to enter any access_key/secret_key either on elasticsearch.yml (in clear - which is not secured) or in elasticsearch keystore.

I'd not remove that option, at least, without speaking to the community/users.

Copy link
Member Author

Choose a reason for hiding this comment

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

I did not deprecate instance profile. Notice it is different from the rest.

Copy link
Member

Choose a reason for hiding this comment

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

But if we run your code without setting any key/secret in elasticsearch.yml (or in the vault), then we will print the deprecated notice you wrote in getDeprecatedCredentials, right?

I mean that when you are using instance profile, you don't have to add any key/secret as it is automatically provided by AWS platform.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, if it is using instance profile creds, no deprecation warning is given. Instance profile provider is not using getDeprecatedCredentials.

Copy link
Member

Choose a reason for hiding this comment

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

I see. I misread the code.

logger.debug("Using instance profile credentials");
credentials = new InstanceProfileCredentialsProvider();
}
}
} else {
logger.debug("Using basic key/secret credentials");
credentials = new StaticCredentialsProvider(new BasicAWSCredentials(key.toString(), secret.toString()));
Copy link
Member

Choose a reason for hiding this comment

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

With what I just said, I'd use here a ProviderChain with:

  • StaticCredentialsProvider
  • InstanceProfileCredentialsProvider

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see a reason to use a provider chain. We select exactly one set of credentials we are planning to use. I don't like having fallbacks (which a provider chain does, with its nasty blanket try/catch) when there are errors.

Copy link
Member

@dadoonet dadoonet Jan 18, 2017

Choose a reason for hiding this comment

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

Then you need to test if the user provided a key/secret pair and in that case use a StaticCredentialsProvider. If not, use a InstanceProfileCredentialsProvider. This is what a chain is doing behind the scene but I agree that you can write that part of the code yourself if you wish.

Copy link
Member Author

Choose a reason for hiding this comment

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

What is there works, there are new tests that prove this. A provider chain would only obfuscate what is really happening. As it is now, it is very clear which methods are deprecated (env vars and sys props) and which are supported (instance profile). We check if env vars and sys props exist up front, instead of lazily like the provider chain would.

@dadoonet dadoonet changed the title S3 repository: Deprecate specifying credentials through env vars, sys props, and profile files S3 repository: Deprecate specifying credentials through env vars, sys props, and remove profile files Jan 16, 2017
@rjernst
Copy link
Member Author

rjernst commented Jan 19, 2017

@dadoonet Any more thoughts here?

@dadoonet
Copy link
Member

@rjernst I added a comment on the issue yesterday: #22567 (comment)

Copy link
Member

@dadoonet dadoonet left a comment

Choose a reason for hiding this comment

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

LGTM

@rjernst rjernst merged commit c5b4bba into elastic:master Jan 19, 2017
@rjernst rjernst deleted the keystore3 branch January 19, 2017 20:36
rjernst added a commit that referenced this pull request Jan 19, 2017
… props, and remove profile files (#22567)

* S3 repository: Deprecate specifying credentials through env vars and sys props

This is a follow up to #22479, where storing credentials secure way was
added.
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Jan 20, 2017
* master: (117 commits)
  Add missing serialization BWC for disk usage estimates
  Expose disk usage estimates in nodes stats
  S3 repository: Deprecate specifying credentials through env vars, sys props, and remove profile files (elastic#22567)
  Fix Eclipse project generation
  Fix deprecation logging for lenient booleans
  Remove @Header we no longer need
  Make lexer abstract
  [Docs] Remove outdated info about enabling/disabling doc_values (elastic#22694)
  Move lexer hacks to EnhancedPainlessLexer
  Fix incorrect args order passed to createAggregator
  Improve painless's javadocs
  Add TestWithDependenciesPlugin to build (elastic#22646)
  Add parsing from xContent to SearchProfileShardResults and nested classes (elastic#22649)
  Add unit tests for FiltersAggregator (elastic#22678)
  Don't register search response listener in transport clients
  unmute FieldStatsIntegrationIT.testGeoPointNotIndexed, fix already pushed
  Mute FieldStatsIntegrationIT.testGeoPointNotIndexed, for now
  Painless: Add augmentation to string for base 64 (elastic#22665)
  Fix NPE on FieldStats with mixed cluster on version pre/post 5.2 (elastic#22688)
  Add parsing methods for UpdateResponse (elastic#22586)
  ...
@clintongormley clintongormley changed the title S3 repository: Deprecate specifying credentials through env vars, sys props, and remove profile files Deprecate specifying credentials through env vars, sys props, and remove profile files Jan 20, 2017
@clintongormley clintongormley added :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs and removed :Plugin Repository S3 labels Feb 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>deprecation :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v5.3.0 v6.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't use ~/.aws/credentials file with discovery-ec2 plugin
3 participants