Refactored RDS IAM authentication logic into a dedicated rdsauth package - #36847
Conversation
…ckage. Simplified and modularized IAM auth setup for MySQL connections.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
getvictor
changed the title
rdsauth pa…rdsauth package
WalkthroughRefactored RDS IAM authentication logic from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25–30 minutes
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
server/datastore/mysql/rdsauth/connector.go (1)
71-76: UnuseddriverNamefield.The
driverNamefield is stored in theConnectorstruct but never used. TheDriver()method returnsmysql.MySQLDriver{}directly regardless of this field. Consider removing it if not needed, or document why it's retained for future use.type Connector struct { - driverName string baseDSN string tokenGen *iamAuthTokenGenerator logger log.Logger }And update the factory:
return func(driverName, dsn string, logger log.Logger) (driver.Connector, error) { return &Connector{ - driverName: driverName, baseDSN: dsn, tokenGen: tokenGen, logger: logger, }, nil }, nilserver/datastore/mysql/common_mysql/common.go (1)
67-80: Unusederrvariable declaration on line 68.The
errvariable declared on line 68 is never used because both branches of the if-else create their own scopederrvariables with:=. This is not a bug but adds unnecessary clutter.dsn := generateMysqlConnectionString(*conf) var db *sqlx.DB - var err error if opts.ConnectorFactory != nil { connector, err := opts.ConnectorFactory(driverName, dsn, opts.Logger) if err != nil { return nil, fmt.Errorf("failed to create connector: %w", err) } db = sqlx.NewDb(sql.OpenDB(connector), driverName) } else { - db, err = sqlx.Open(driverName, dsn) + var err error + db, err = sqlx.Open(driverName, dsn) if err != nil { return nil, err } }Or alternatively, keep line 68 and use
=instead of:=in the else branch to be consistent.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
changes/36846-refactor-rds-iam(1 hunks)server/datastore/mysql/common_mysql/common.go(4 hunks)server/datastore/mysql/mysql.go(7 hunks)server/datastore/mysql/rdsauth/connector.go(6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit configuration file
When reviewing SQL queries that are added or modified, ensure that appropriate filtering criteria are applied—especially when a query is intended to return data for a specific entity (e.g., a single host). Check for missing WHERE clauses or incorrect filtering that could lead to incorrect or non-deterministic results (e.g., returning the first row instead of the correct one). Flag any queries that may return unintended results due to lack of precise scoping.
Files:
server/datastore/mysql/mysql.goserver/datastore/mysql/common_mysql/common.goserver/datastore/mysql/rdsauth/connector.go
🧠 Learnings (3)
📓 Common learnings
Learnt from: sgress454
Repo: fleetdm/fleet PR: 31075
File: server/datastore/mysql/common_mysql/aws_iam_auth.go:111-130
Timestamp: 2025-08-20T21:24:59.261Z
Learning: In Fleet's MySQL IAM authentication implementation, the TLS and AllowCleartextPasswords configuration is handled in the generateMysqlConnectionString function in server/datastore/mysql/common_mysql/common.go. When no password is configured and the endpoint is an RDS endpoint, the function automatically sets allowCleartextPasswords=true and configures appropriate TLS settings. The awsIAMAuthConnector receives a properly configured base DSN and only needs to inject the IAM token as the password.
Learnt from: sgress454
Repo: fleetdm/fleet PR: 31075
File: server/datastore/mysql/common_mysql/aws_iam_auth.go:111-130
Timestamp: 2025-08-20T21:24:59.261Z
Learning: In Fleet's MySQL IAM authentication implementation, the TLS and AllowCleartextPasswords configuration is handled in the generateMysqlConnectionString function in server/datastore/mysql/common_mysql/common.go, not in the awsIAMAuthConnector.Connect method. The connector receives a base DSN that already includes the necessary IAM authentication parameters.
📚 Learning: 2025-08-20T21:24:59.261Z
Learnt from: sgress454
Repo: fleetdm/fleet PR: 31075
File: server/datastore/mysql/common_mysql/aws_iam_auth.go:111-130
Timestamp: 2025-08-20T21:24:59.261Z
Learning: In Fleet's MySQL IAM authentication implementation, the TLS and AllowCleartextPasswords configuration is handled in the generateMysqlConnectionString function in server/datastore/mysql/common_mysql/common.go. When no password is configured and the endpoint is an RDS endpoint, the function automatically sets allowCleartextPasswords=true and configures appropriate TLS settings. The awsIAMAuthConnector receives a properly configured base DSN and only needs to inject the IAM token as the password.
Applied to files:
changes/36846-refactor-rds-iamserver/datastore/mysql/mysql.goserver/datastore/mysql/common_mysql/common.goserver/datastore/mysql/rdsauth/connector.go
📚 Learning: 2025-08-20T21:24:59.261Z
Learnt from: sgress454
Repo: fleetdm/fleet PR: 31075
File: server/datastore/mysql/common_mysql/aws_iam_auth.go:111-130
Timestamp: 2025-08-20T21:24:59.261Z
Learning: In Fleet's MySQL IAM authentication implementation, the TLS and AllowCleartextPasswords configuration is handled in the generateMysqlConnectionString function in server/datastore/mysql/common_mysql/common.go, not in the awsIAMAuthConnector.Connect method. The connector receives a base DSN that already includes the necessary IAM authentication parameters.
Applied to files:
changes/36846-refactor-rds-iamserver/datastore/mysql/mysql.goserver/datastore/mysql/common_mysql/common.goserver/datastore/mysql/rdsauth/connector.go
🧬 Code graph analysis (1)
server/datastore/mysql/rdsauth/connector.go (2)
server/aws_common/iam_auth.go (1)
IAMAuthTokenManager(30-37)server/config/config.go (1)
MysqlConfig(37-56)
🔇 Additional comments (10)
server/datastore/mysql/rdsauth/connector.go (3)
1-2: LGTM on package structure.Clean extraction of AWS IAM auth logic into a dedicated package. This aligns well with the PR objective of reducing AWS SDK dependencies in
common_mysql.
79-98: LGTM on Connect implementation.The approach of creating a fresh
mysql.Connectorper connection ensures the IAM token is properly injected each time. The token caching is handled by theIAMAuthTokenManager, which is the correct architecture per the existing Fleet patterns.
105-129: LGTM on factory pattern.The
NewConnectorFactoryproperly creates the token generator once and returns a closure that shares it across all connector instances. This ensures efficient token caching while enabling clean dependency injection intocommon_mysql.changes/36846-refactor-rds-iam (1)
1-1: LGTM on changelog entry.Appropriate description for this internal refactoring.
server/datastore/mysql/common_mysql/common.go (2)
20-24: LGTM on ConnectorFactory type.Well-documented type that cleanly abstracts the connector creation, enabling IAM auth injection without coupling
common_mysqlto AWS dependencies.
42-45: LGTM on DBOptions extension.The optional
ConnectorFactoryfield is well-documented and maintains backward compatibility.server/datastore/mysql/mysql.go (4)
259-262: LGTM on primary IAM auth setup.Clean integration point for IAM authentication before creating the primary DB connection.
269-281: LGTM on replica IAM auth handling.Correctly resets
ConnectorFactorybefore setting up IAM auth for replica, allowing replicas to have independent authentication configurations (e.g., different regions).
398-418: LGTM onsetupIAMAuthIfNeededimplementation.The detection logic correctly identifies when IAM auth should be used (no password provided but region is configured). The host/port parsing with fallback to default MySQL port 3306 is appropriate.
9-9: LGTM on import additions.The
netandrdsauthimports are appropriately used for the IAM authentication setup.Also applies to: 29-29
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #36847 +/- ##
==========================================
- Coverage 65.91% 65.91% -0.01%
==========================================
Files 2259 2259
Lines 184211 184233 +22
Branches 7646 7646
==========================================
+ Hits 121430 121438 +8
- Misses 51695 51711 +16
+ Partials 11086 11084 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@sgress454 I'm assigning this refactoring to you since you worked on RDS IAM. I'm trying to reduce the dependencies of the mysql_common package. |
|
|
||
| // awsIAMAuthConnector implements driver.Connector for IAM authentication | ||
| type awsIAMAuthConnector struct { | ||
| driverName string | ||
| baseDSN string | ||
| tokenGen *awsIAMAuthTokenGenerator | ||
| logger log.Logger | ||
| // Connector implements driver.Connector for IAM authentication | ||
| type Connector struct { | ||
| baseDSN string | ||
| tokenGen *iamAuthTokenGenerator | ||
| logger log.Logger | ||
| } |
There was a problem hiding this comment.
Looks like we weren't using the connector driverName ourselves, and sql.OpenDB(connector) only looks for the connector to meet a certain interface (not be a type including driverName), so 👍 to drop this.
2 participants
Simplified and modularized IAM auth setup for MySQL connections.
Related issue: Resolves #36846
Manually QA'ed by setting up RDS with IAM and running Fleet like:
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.