AWS RDS token based password provider by himanshug · Pull Request #9518 · apache/druid

himanshug · 2020-03-14T06:28:31Z

Description

Adds a new PasswordProvider impl to access AWS RDS DB instances using temporary AWS tokens. Since these tokens expire periodically, additional changes are made to ask PasswordProvider for password every time a new DB connection is established.

This PR has:

been self-reviewed.
- using the concurrency checklist (Remove this item if the PR doesn't have any relation to concurrency.)
added documentation for new or modified features or behaviors.
added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
added or updated version, license, or notice information in licenses.yaml
added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
added unit tests or modified existing tests to cover new code paths.
added integration tests.
been tested in a test Druid cluster.

Key changed/added classes in this PR

AWSRDSTokenPasswordProvider
BasicDataSourceExt

himanshug · 2020-03-26T07:12:06Z

Update: This patch has been successfully running on few Druid clusters using AWS Aurora RDS DB clusters and accessing same without password but using ephemeral AWS IAM tokens.

jihoonson · 2020-03-26T07:16:48Z

@himanshug thanks for the update. I removed the Area - SQL tag because I think it's about Druid SQL layer. I haven't looked through the whole PR yet, but it seems like this is in the core not an extension. But should it be an extension feature?

himanshug · 2020-03-26T07:26:04Z

removing Area - SQL makes sense now that I think about it.

I actually thought of putting the new PasswordProvider in an extension. Looked at existing aws related extensions and it did not fit in any of those , the ec2-extensions or s3-extensions . So, had put there along side other common AWS related code.

Anyways, It is totally possible to put AWSRDSTokenPasswordProvider in a new extension (aws-rds-extensions). However, BasicDataSourceExt would stay in core. I will update the PR . thanks.

jihoonson · 2020-03-26T16:56:46Z

Thanks, I will review soon.

jihoonson · 2020-03-30T23:27:31Z

+  <parent>
+    <groupId>org.apache.druid</groupId>
+    <artifactId>druid</artifactId>
+    <version>0.18.0-SNAPSHOT</version>


Would you mind updating the version to 0.19.0-SNAPSHOT?

stale · 2020-12-25T17:53:06Z

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the dev@druid.apache.org list. Thank you for your contributions.

himanshug · 2020-12-26T16:15:16Z

un-stale !

jihoonson

@himanshug thank you for your patience. I finally reviewed. The PR LGTM overall, but have left some comments about documentation. I'm also wondering if we can easily add some integration tests which might not be running on Travis, but can be run manually.

jihoonson · 2021-01-05T01:34:50Z

+
+import java.util.List;
+
+public class AWSModule implements DruidModule


Maybe AWSRDSModule?

will change

jihoonson · 2021-01-05T01:36:09Z

+ * This class exists so that PasswordProvider is asked for password every time a brand new connection is established
+ * with DB. PasswordProvider impls such as based on AWS tokens refresh the underlying token periodically since
+ * each token is valid for a certain period of time only.
+ * So, This class overrides[ummm copies] the methods from base class in order to keep track of connection properties


Haha, would you fix ummm copies?

not sure if there is anything to fix as of now , BasicDataSource isn't open enough for extension to let us dynamically get password from config everytime a new connection is setup.

Oh, I just assumed that you wanted to add a link instead of ummm copies. It would be enough to just remove it if there is nothing to fix.

i see :) , updated the comment to hopefully remove the link confusion

jihoonson · 2021-01-05T01:38:55Z

+This module provides AWS RDS token [password provider](../../operations/password-provider.md) provides temp token for accessing AWS RDS DB cluster.
+
+```json
+{ "type": "awsrdstoken", "user": "USER", "host": "HOST", "port": PORT, "region": "AWS_REGION" }


Maybe aws-rds-token?

will update

himanshug · 2021-01-05T04:52:54Z

@jihoonson thanks, I will merge it with master and get the build to pass so as to make it merge-able.

easiest way to manually test this is to deploy coordinator node on ec2 instance if coordinator starts successfully then it is good. manual integration test should also be possible, but steps to run such integration would still be to deploy some code inside an ec2 instance which can exercise the code here. integration test would be a nice addition though as a future PR specially as and when this extension gets wider adoption.

himanshug · 2021-01-06T02:13:49Z

@jihoonson I think it is ready now.

jihoonson

LGTM. Thanks @himanshug!

clintropolis

overall lgtm, sorry this one slipped through the cracks for so long

clintropolis · 2021-01-06T02:17:19Z

+    }
+
+    if (driverToUse == null) {
+      throw new RE("WTH! Couln't find a Driver");


nit: i think we've been trying to tone it down and be professional and shit 😛 , related #10270

changed, I thought "WTF" to "WTH" transition was that :)

clintropolis · 2021-01-06T02:23:56Z

+  ~ under the License.
+  -->
+
+This module provides AWS RDS token [password provider](../../operations/password-provider.md) provides temp token for accessing AWS RDS DB cluster.


to prevent confusion, should we more explicitly call out that this is not a metadata connector itself and just provides auth to be used by the extension that is appropriate for the RDS instance type?

reworded to hopefully remove the confusion

clintropolis · 2021-01-06T02:24:53Z

+To use this extension, make sure you [include](../../development/extensions.md#loading-extensions) it in your config file:
+
+```
+druid.extensions.loadList=["druid-aws-rds-extensions"]


likewise i guess it is unrealistic that you would have a load list with only this extension, maybe should include mysql or postgres here, though maybe this doesn't need spelled out and is ok as it is...

…Ext.java Co-authored-by: Clint Wylie <cjwylie@gmail.com>

himanshug · 2021-01-07T05:15:42Z

thanks @clintropolis

* refresh db pwd * aws iam token password provider * fix analyze-dependencies build * fix doc build * add ut for BasicDataSourceExt * more doc updates * more doc update * moving aws token password provider to new extension * remove duplicate changes * make all config inline * extension docs * refresh db password in SQL Firehose code path as well * add ut * fix build * add new extension to distribution * rds lib is not provided * fix license build * add version to license * change parent version to 0.19.0-snapshot * address review comments * fix core/ code coverage * Update server/src/main/java/org/apache/druid/metadata/BasicDataSourceExt.java Co-authored-by: Clint Wylie <cjwylie@gmail.com> * address review comments * fix spellchecker * remove inadvertant website file change Co-authored-by: Clint Wylie <cjwylie@gmail.com>

jihoonson · 2021-02-12T23:21:42Z

+libraries:
+  - com.amazonaws: aws-java-sdk-rds


Hi @himanshug, did you adopt source code from the AWS SDK? If so, source_paths field should be added. Otherwise, license_category should be binary.

source code is not copied here , I guess this should be "binary" , yes. .. will update.

jihoonson · 2021-02-12T23:24:24Z

+  }
+
+  @Override
+  protected ConnectionFactory createConnectionFactory() throws SQLException


@himanshug Did you adopt this source code from org.apache.commons.dbcp2.BasicDataSource? If so, it should be added in the licenses.yaml file.

yeah, most code in this file is copied from org.apache.commons.dbcp2.BasicDataSource , will send a PR shortly.

… password provider in PR apache#9518

… password provider in PR #9518 (#10885) * license.yaml fixes for code introduced related to AWS RDS token based password provider in PR #9518 * add notice for commons-dbcp in license file * add version and update NOTICE file

himanshug added 2 commits March 13, 2020 20:37

refresh db pwd

c024ec6

aws iam token password provider

5cef48e

himanshug force-pushed the aws_pwd_provider branch from 7d33814 to 5cef48e Compare March 14, 2020 06:33

himanshug added 4 commits March 14, 2020 14:52

fix analyze-dependencies build

85932c0

fix doc build

2314fa4

add ut for BasicDataSourceExt

955b62f

more doc updates

553f622

himanshug added Area - SQL Feature labels Mar 14, 2020

more doc update

ede86cc

jihoonson removed the Area - SQL label Mar 26, 2020

himanshug added 12 commits March 26, 2020 14:41

Merge remote-tracking branch 'apache/master' into aws_pwd_provider

162e522

moving aws token password provider to new extension

f908756

remove duplicate changes

35a0527

make all config inline

52ff609

extension docs

52efefc

refresh db password in SQL Firehose code path as well

6969403

add ut

27f5652

fix build

ee98fc8

add new extension to distribution

46e2264

rds lib is not provided

ca82501

fix license build

1a7726c

add version to license

f5214e0

jihoonson added the Area - Extension label Mar 30, 2020

jihoonson reviewed Mar 30, 2020

View reviewed changes

Merge remote-tracking branch 'apache/master' into aws_pwd_provider

22a35aa

stale Bot removed the stale label Oct 26, 2020

stale Bot added the stale label Dec 25, 2020

jihoonson removed the stale label Jan 5, 2021

jihoonson reviewed Jan 5, 2021

View reviewed changes

himanshug added 3 commits January 5, 2021 07:48

Merge remote-tracking branch 'origin/master' into aws_pwd_provider

6ef6341

address review comments

6ab5687

fix core/ code coverage

f04c23a

jihoonson approved these changes Jan 6, 2021

View reviewed changes

clintropolis reviewed Jan 6, 2021

View reviewed changes

himanshug and others added 4 commits January 6, 2021 13:57

Update server/src/main/java/org/apache/druid/metadata/BasicDataSource…

254de29

…Ext.java Co-authored-by: Clint Wylie <cjwylie@gmail.com>

address review comments

fe2ab6a

fix spellchecker

71c452b

remove inadvertant website file change

3cee6fc

clintropolis approved these changes Jan 6, 2021

View reviewed changes

clintropolis added the Release Notes label Jan 6, 2021

himanshug merged commit c7b1212 into apache:master Jan 7, 2021

jihoonson added this to the 0.21.0 milestone Jan 8, 2021

jihoonson mentioned this pull request Jan 13, 2021

[Draft] 0.21.0 Release Notes #10752

Closed

jihoonson reviewed Feb 12, 2021

View reviewed changes

himanshug added a commit to himanshug/druid that referenced this pull request Feb 12, 2021

license.yaml fixes for code introduced related to AWS RDS token based…

7ee003c

… password provider in PR apache#9518

himanshug mentioned this pull request Feb 12, 2021

license.yaml fixes for code introduced related to AWS RDS token based password provider in PR #9518 #10885

Merged


		import java.util.List;

		public class AWSModule implements DruidModule

Conversation

himanshug commented Mar 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Key changed/added classes in this PR

Uh oh!

himanshug commented Mar 26, 2020

Uh oh!

jihoonson commented Mar 26, 2020

Uh oh!

himanshug commented Mar 26, 2020

Uh oh!

jihoonson commented Mar 26, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stale Bot commented Dec 25, 2020

Uh oh!

himanshug commented Dec 26, 2020

Uh oh!

jihoonson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

himanshug commented Jan 5, 2021

Uh oh!

himanshug commented Jan 6, 2021

Uh oh!

jihoonson left a comment

Choose a reason for hiding this comment

Uh oh!

clintropolis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

himanshug Jan 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

himanshug commented Jan 7, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

himanshug commented Mar 14, 2020 •

edited

Loading

himanshug Jan 6, 2021 •

edited

Loading