Feat 1817 add iam auth to mysql and redis#32488
Conversation
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…17-add-iam-auth-to-mysql-and-redis
Why? I opened a separate docs PR to merge into the docs branch: https://github.com/fleetdm/fleet/pull/32219/files
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #32488 +/- ##
==========================================
- Coverage 64.02% 63.94% -0.09%
==========================================
Files 1987 1991 +4
Lines 195579 195652 +73
Branches 6461 6360 -101
==========================================
- Hits 125221 125100 -121
- Misses 60560 60746 +186
- Partials 9798 9806 +8
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:
|
|
|
||
| // Auto-detect ElastiCache and use IAM auth if no password is provided | ||
| useIAMAuth := false | ||
| if conf.Password == "" && conf.Region != "" && conf.CacheName != "" { |
There was a problem hiding this comment.
I think the region should be optional, in the vast majority of cases the region will just be whatever the default is from the environment, and here we don't need it because CacheName can be used.
There was a problem hiding this comment.
You're right, although we do need a region to generate the auth token. I think if we change this to
region: cfg.Region
it should fall back to the default region if provided.
| // Auto-detect RDS and use IAM auth if no password is provided | ||
| var iamTokenGen *awsIAMAuthTokenGenerator | ||
| useIAMAuth := false | ||
| if conf.Password == "" && conf.PasswordPath == "" && conf.Region != "" { |
There was a problem hiding this comment.
Not entirely sold on requiring the region, is it required for the other AWS integration features?
There was a problem hiding this comment.
It is -- see the Fleet server configuration docs and the various _region configs.
There was a problem hiding this comment.
Digging into this deeper, we're doing something similar to what I suggested above where it falls back to the default region if configured (or rather, overrides the default regions in LoadAWSConfig if one is provided).
For MySQL there's a bit of a quandary because, without a flag specifying that you're using RDS, we don't have a good way to determine when to try IAM auth. Your original PR handled this by parsing the server address. This is very handy when it works but introduces an amount of brittleness and edge cases that we'd rather not support. So in the absence of a specific use_iam_auth flag (proposed and rejected due to inconsistency with the rest of the config), the presence of region + absence of password is the best indicator we have that IAM auth is desired.
| m.cacheMu.Lock() | ||
| defer m.cacheMu.Unlock() | ||
|
|
||
| // Double-check in case another goroutine generated a token while we were waiting |
There was a problem hiding this comment.
Nit for the future: (no need to change and re-test)
For simplicity, I would have just used a sync.Mutex and just lock the whole GetToken method. Not sure if there are performance gains, and they are negligible next to network times, etc.
| "multiStatements": []string{"true"}, | ||
| } | ||
| if conf.TLSConfig != "" { | ||
| if conf.Password == "" && conf.PasswordPath == "" && conf.Region != "" { |
There was a problem hiding this comment.
Nit: I would use if useIAMAuth for clarity. Or add a comment (to not invalidate all the testing already done) that we allow clear text passwords because IAM.
There was a problem hiding this comment.
def should use useIAMAuth here rather than repeating this logic, will update
There was a problem hiding this comment.
Oh wait this is in another method, we haven't set useIAMAuth here. I could make a helper method for it but for the sake of not running the tests again, I'll leave this be.
| } | ||
|
|
||
| if conf.UseTLS { | ||
| var err error |
for #1817
Details
This PR gives Fleet servers the ability to connect to RDS MySQL and Elasticache Redis via AWS Identity and Access Management (IAM). It is based almost entirely on the work of @titanous, branched from his original pull request. The main differences between his branch and this are:
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.
Testing