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

[OPIK-135] Add support to RDS auth for MySQL #306

Merged
merged 6 commits into from
Sep 24, 2024

Conversation

thiagohora
Copy link
Contributor

@thiagohora thiagohora commented Sep 23, 2024

Details

Screenshot 2024-09-24 at 12 08 59

Issues

OPIK-135

Testing

Locally, assuming a ROLE with the required access to RDS.

Documentation

In order to enable the Authentication using the AWS IAM mechanism, the user needs the following:

  1. Enable IAM DB authentication. See this link
  2. Create an IAM Policy for IAM database access. See link
  3. Setting AWSAuthenticationPlugin as authentication mode for the target MySQL user. See link.
  4. Setup AWS credentials. See link.
  5. Change database driver to software.amazon.jdbc.Driver instead of com.mysql.cj.jdbc.Driver
# Use the new env var
STATE_DB_DRIVER_CLASS=software.amazon.jdbc.Driver
  1. Mofify env var: STATE_DB_PROTOCOL to update the connection string. Use jdbc:aws-wrapper:mysql:// instead of jdbc:mysql://
  2. Set the STATE_DB_PLUGINS to iam or other valid plugins. For more valid options, see link
STATE_DB_PLUGINS='iam'
  1. Set STATE_DB_PASS to empty.

@thiagohora thiagohora requested a review from a team as a code owner September 23, 2024 18:44
@thiagohora thiagohora self-assigned this Sep 23, 2024
Nimrod007
Nimrod007 previously approved these changes Sep 23, 2024
Copy link
Collaborator

@Nimrod007 Nimrod007 left a comment

Choose a reason for hiding this comment

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

nice! what additional parameters do we add for URL ? i think its cleaner to have an additional config and not url changes. but this looks ok for now

@thiagohora
Copy link
Contributor Author

nice! what additional parameters do we add for URL ? i think its cleaner to have an additional config and not url changes. but this looks ok for now

&wrapperPlugins=iam is used to enable the IAM auth, the issue that Dropwizard defines in the DB configuration schema. What we can do is Document how to create a volume to change the config.yml

andrescrz
andrescrz previously approved these changes Sep 24, 2024
Copy link
Collaborator

@andrescrz andrescrz left a comment

Choose a reason for hiding this comment

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

LGTM.

@thiagohora
Copy link
Contributor Author

Hi @andrescrz and @Nimrod007,

The E2E tests are failing because the variable changes are not backward compatible.

Run docker logs opik-backend-1 > /home/runner/work/opik/opik/opik-backend_p3.9.log
java.sql.SQLException: Driver:com.mysql.cj.jdbc.Driver@5aaaa446 returned null for URL:mysql:3306/opik?createDatabaseIfNotExist=true&rewriteBatchedStatements=true
   at org.apache.tomcat.jdbc.pool.PooledConnection.connectUsingDriver(PooledConnection.java:365)
   at org.apache.tomcat.jdbc.pool.PooledConnection.connect(PooledConnection.java:227)
   at org.apache.tomcat.jdbc.pool.ConnectionPool.createConnection(ConnectionPool.java:779)
   at org.apache.tomcat.jdbc.pool.ConnectionPool.borrowConnection(ConnectionPool.java:707)
   at org.apache.tomcat.jdbc.pool.ConnectionPool.init(ConnectionPool.java:506)
   at org.apache.tomcat.jdbc.pool.ConnectionPool.<init>(ConnectionPool.java:155)
   at org.apache.tomcat.jdbc.pool.DataSourceProxy.pCreatePool(DataSourceProxy.java:[11](https://github.com/comet-ml/opik/actions/runs/11010029638/job/30571004042?pr=306#step:8:12)8)
   at org.apache.tomcat.jdbc.pool.DataSourceProxy.createPool(DataSourceProxy.java:107)

@andrescrz
Copy link
Collaborator

Hi @andrescrz and @Nimrod007,

The E2E tests are failing because the variable changes are not backward compatible.

Run docker logs opik-backend-1 > /home/runner/work/opik/opik/opik-backend_p3.9.log
java.sql.SQLException: Driver:com.mysql.cj.jdbc.Driver@5aaaa446 returned null for URL:mysql:3306/opik?createDatabaseIfNotExist=true&rewriteBatchedStatements=true
   at org.apache.tomcat.jdbc.pool.PooledConnection.connectUsingDriver(PooledConnection.java:365)
   at org.apache.tomcat.jdbc.pool.PooledConnection.connect(PooledConnection.java:227)
   at org.apache.tomcat.jdbc.pool.ConnectionPool.createConnection(ConnectionPool.java:779)
   at org.apache.tomcat.jdbc.pool.ConnectionPool.borrowConnection(ConnectionPool.java:707)
   at org.apache.tomcat.jdbc.pool.ConnectionPool.init(ConnectionPool.java:506)
   at org.apache.tomcat.jdbc.pool.ConnectionPool.<init>(ConnectionPool.java:155)
   at org.apache.tomcat.jdbc.pool.DataSourceProxy.pCreatePool(DataSourceProxy.java:[11](https://github.com/comet-ml/opik/actions/runs/11010029638/job/30571004042?pr=306#step:8:12)8)
   at org.apache.tomcat.jdbc.pool.DataSourceProxy.createPool(DataSourceProxy.java:107)

Thanks for letting us know. This would've passed if the end 2 end tests Github action was using an image containing the changes in the PR instead of the latest main version.

Nimrod007
Nimrod007 previously approved these changes Sep 24, 2024
@thiagohora
Copy link
Contributor Author

Added disbaled test to help/test setup

@thiagohora thiagohora merged commit 04a50de into main Sep 24, 2024
4 of 9 checks passed
@thiagohora thiagohora deleted the OPIK-135/add_support_to_aws_rds_auth_mysql branch September 24, 2024 10:32
dsblank pushed a commit that referenced this pull request Oct 4, 2024
* [OPIK-135] Add support to RDS auth for MySQL

* Change variables

* Update Helm documentation

* Add AWS tests

---------

Co-authored-by: CometActions <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants