fix(qdev): configuration and execution not working with iam auth#8785
fix(qdev): configuration and execution not working with iam auth#8785ReeceXW wants to merge 2 commits intoapache:mainfrom
Conversation
dosubot
bot
added
size:L
|
@warren830, could you please review this PR when you have a moment? Thank you! |
warren830
left a comment
warren830
left a comment
There was a problem hiding this comment.
Thanks for the PR — overall clean and well-scoped. IAM credential chain handling is correct, backward compatibility is solid (defaults to access_key, migration backfills existing rows), and test coverage hits the key paths.
One additional concern not tied to a specific diff line:
Potential bug (medium): MergeFromRequest in models/connection.go uses DecodeMapStruct to overwrite fields from the request body. If a PATCH request omits authType, the field may be decoded as "", which then silently falls back to "access_key" via the validation default. This could unexpectedly flip an iam_role connection back to access_key. Consider adding the same guard pattern used for SecretAccessKey — preserve the existing AuthType when the incoming value is empty.
A few inline suggestions below — #1 (mutation in validation) and the MergeFromRequest issue above are worth addressing before merge; the rest are nice-to-haves.
| type QDevConn struct { | ||
| // AccessKeyId for AWS | ||
| // AuthType determines how to authenticate with AWS: "access_key" or "iam_role" | ||
| AuthType string `mapstructure:"authType" json:"authType"` |
There was a problem hiding this comment.
Suggestion (low): "access_key" and "iam_role" are repeated across validation, tests, migration, and both client files. Consider defining constants:
const (
AuthTypeAccessKey = "access_key"
AuthTypeIAMRole = "iam_role"
)This prevents typo-driven bugs and makes future additions easier to grep for.
| return errors.Default.New("SecretAccessKey is required") | ||
| // Default to access_key auth type if not specified | ||
| if connection.AuthType == "" { | ||
| connection.AuthType = "access_key" |
There was a problem hiding this comment.
Suggestion (medium): A validation function ideally shouldn't mutate its input. Setting the default AuthType here means validateConnection has a side effect that callers may not expect. Consider moving the defaulting logic to a dedicated normalizeConnection() step (or into the caller) and keeping validateConnection pure.
| } | ||
| // Only set static credentials for access_key auth; IAM role uses the default credential chain | ||
| if !connection.IsIAMRoleAuth() { | ||
| cfg.Credentials = credentials.NewStaticCredentials(connection.AccessKeyId, connection.SecretAccessKey, "") |
There was a problem hiding this comment.
Nit (low): This block is nearly identical to identity_client.go lines 51-59. A small helper like newAWSSession(conn *models.QDevConnection, region string) (*session.Session, error) would DRY this up. Not blocking — just a suggestion for a follow-up cleanup.
3 participants
Summary
In the UI you are able to choose IAM authentication instead of an access key and secret but the API doesn't actually support this; it will just fail. I've added the type parameter to the API and the necessary AWS configuration to make this work.