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.

Sign up for GitHub

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

Jump to bottom

ECS Task IAM profile credentials ignored in repository-s3 plugin #31864

Merged
merged 1 commit into from
Jul 19, 2018
Merged

ECS Task IAM profile credentials ignored in repository-s3 plugin #31864

merged 1 commit into from
Jul 19, 2018

Conversation

vladimirdolzhenko
Copy link
Contributor

@vladimirdolzhenko vladimirdolzhenko commented Jul 6, 2018 •
edited
Loading

ECS Task IAM profile credentials ignored in repository-s3 plugin

Closes #26913

Based on PR #31918

EC2ContainerCredentialsProviderWrapper - utilises IAM role for a task - both relative to http://169.254.170.2 and absolute + fail over to InstanceProfileCredentialsProvider that is currently in use.

provider relies on environment variables common to containers

@vladimirdolzhenko vladimirdolzhenko added the :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs label Jul 6, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

@DaveCTurner
Copy link
Contributor

We discussed this and, as per #26913 (comment), think that the DefaultAWSCredentialsProviderChain is too liberal with its choice of sources for credentials. However com.amazonaws.auth.EC2ContainerCredentialsProviderWrapper looks suitable: it only queries the metadata services, but supports both container credentials and instance profile credentials.

However I think we shouldn't proceed here without a bit more testing: specifically, that we do correctly get the credentials from the metadata services. I think we can extend S3Fixture to act as the appropriate metadata services: we can set the URI to the container metadata service using the AWS_CONTAINER_CREDENTIALS_FULL_URI environment variable, and the URI for the instance metadata service via the com.amazonaws.sdk.ec2MetadataServiceEndpointOverride system property.

@vladimirdolzhenko vladimirdolzhenko requested review from alpar-t and DaveCTurner and removed request for tlrx July 11, 2018 10:50
@vladimirdolzhenko
Copy link
Contributor Author

@DaveCTurner @atorok this PR extends #31918 - environment variable has to be specified to redirect ECS to custom endpoint

alpar-t
alpar-t previously approved these changes Jul 11, 2018
View reviewed changes
Copy link
Contributor

@alpar-t alpar-t left a comment

Choose a reason for hiding this comment

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

Gradle LGTM

plugins/repository-s3/build.gradle Outdated
@@ -302,8 +320,13 @@ task s3Fixture(type: AntFixture) {
env 'S3FIXTURE_TEMPORARY_KEY', "${s3TemporaryAccessKey}"
env 'S3FIXTURE_TEMPORARY_SESSION_TOKEN', "${s3TemporarySessionToken}"
env 'S3FIXTURE_EC2_BUCKET_NAME', "${s3EC2Bucket}"
env 'S3FIXTURE_EC2_CREDENTIALS', "${s3EC2Credentials}"
Copy link
Contributor

Choose a reason for hiding this comment

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

There's no need for a Gstring here, could have used the s3EC2Credentials variable directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good spot - thanks @atorok 👍

@vladimirdolzhenko vladimirdolzhenko mentioned this pull request Jul 11, 2018
Merged
@vladimirdolzhenko
Copy link
Contributor Author

@DaveCTurner could you pls have a look - it is ready for review as #31918 is merged to master

@DaveCTurner
Copy link
Contributor

This seems to have been rebased, losing earlier review comments. Please could you try and avoid rebasing open PRs?

The changes to the S3Service and AmazonS3Fixture LGTM, but I'm unsure if there's a neater way to run the integ test clusters than this. @atorok or @ywelsch does this look ok to you?

@DaveCTurner DaveCTurner dismissed alpar-t’s stale review July 18, 2018 12:41

Substantial changes, and a rebase, since this review was given. Please could you look again?

@vladimirdolzhenko
Copy link
Contributor Author

@DaveCTurner sorry for the rebase - it was really huge number of changes that this PR is based on

@alpar-t
Copy link
Contributor

alpar-t commented Jul 18, 2018

LGTM I don't think we can do better for now.

DaveCTurner
DaveCTurner approved these changes Jul 19, 2018
View reviewed changes
Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

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

Ok thanks @atorok, LGTM too then.

@vladimirdolzhenko vladimirdolzhenko merged commit 7c0fc20 into elastic:master Jul 19, 2018
vladimirdolzhenko pushed a commit that referenced this pull request Jul 19, 2018
ECS Task IAM profile credentials ignored in repository-s3 plugin (#31864
625b65c 
)

Closes #26913

(cherry picked from commit 7c0fc20)
@vladimirdolzhenko
Copy link
Contributor Author

thanks @DaveCTurner and @atorok for the review

@vladimirdolzhenko vladimirdolzhenko deleted the 26913 branch July 19, 2018 10:57
@JoeNguyen93 JoeNguyen93 mentioned this pull request Jul 19, 2018
Open
dnhatn added a commit that referenced this pull request Jul 20, 2018
@dnhatn
Merge branch 'master' into ccr
d0f3ed5 
* master:
  Painless: Simplify Naming in Lookup Package (#32177)
  Handle missing values in painless (#32207)
  add support for write index resolution when creating/updating documents (#31520)
  ECS Task IAM profile credentials ignored in repository-s3 plugin (#31864)
  Remove indication of future multi-homing support (#32187)
  Rest test - allow for snapshots to take 0 milliseconds
  Make x-pack-core generate a pom file
  Rest HL client: Add put watch action (#32026)
  Build: Remove pom generation for plugin zip files (#32180)
  Fix comments causing errors with Java 11
  Fix rollup on date fields that don't support epoch_millis (#31890)
  Detect and prevent configuration that triggers a Gradle bug (#31912)
  [test] port linux package packaging tests (#31943)
  Revert "Introduce a Hashing Processor (#31087)" (#32178)
  Remove empty @return from JavaDoc
  Adjust SSLDriver behavior for JDK11 changes (#32145)
  [test] use randomized runner in packaging tests (#32109)
  Add support for field aliases. (#32172)
  Painless: Fix caching bug and clean up addPainlessClass. (#32142)
  Call setReferences() on custom referring tokenfilters in _analyze (#32157)
  Fix BwC Tests looking for UUID Pre 6.4 (#32158)
  Improve docs for search preferences (#32159)
  use before instead of onOrBefore
  Add more contexts to painless execute api (#30511)
  Add EC2 credential test for repository-s3 (#31918)
  A replica can be promoted and started in one cluster state update (#32042)
  Fix Java 11 javadoc compile problem
  Fix CP for namingConventions when gradle home has spaces (#31914)
  Fix `range` queries on `_type` field for singe type indices (#31756)
  [DOCS] Update TLS on Docker for 6.3 (#32114)
  ESIndexLevelReplicationTestCase doesn't support replicated failures but it's good to know what they are
  Remove versionType from translog (#31945)
  Switch distribution to new style Requests (#30595)
  Build: Skip jar tests if jar disabled
  Painless: Add PainlessClassBuilder (#32141)
  Build: Make additional test deps of check (#32015)
  Disable C2 from using AVX-512 on JDK 10 (#32138)
  Build: Move shadow customizations into common code (#32014)
  Painless: Fix Bug with Duplicate PainlessClasses (#32110)
  Remove empty @param from Javadoc
  Re-disable packaging tests on suse boxes
  Docs: Fix missing example script quote (#32010)
  [ML] Wait for aliases in multi-node tests (#32086)
  [ML] Move analyzer dependencies out of categorization config (#32123)
  Ensure to release translog snapshot in primary-replica resync (#32045)
  Handle TokenizerFactory  TODOs (#32063)
  Relax TermVectors API to work with textual fields other than TextFieldType (#31915)
  Updates the build to gradle 4.9 (#32087)
  Mute :qa:mixed-cluster indices.stats/10_index/Index - all’
  Check that client methods match API defined in the REST spec (#31825)
  Enable testing in FIPS140 JVM (#31666)
  Fix put mappings java API documentation (#31955)
  Add exclusion option to `keep_types` token filter (#32012)
  [Test] Modify assert statement for ssl handshake (#32072)
@rjernst rjernst mentioned this pull request Dec 6, 2018
Closed
@jeffnappi jeffnappi mentioned this pull request Dec 16, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DaveCTurner DaveCTurner DaveCTurner approved these changes

@alpar-t alpar-t alpar-t left review comments

Assignees
No one assigned
Labels
:Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs >enhancement v6.4.0 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@vladimirdolzhenko @elasticmachine @DaveCTurner @alpar-t @colings86