-
Notifications
You must be signed in to change notification settings - Fork 724
fix(qdev): configuration and execution not working with iam auth #8785
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,9 +24,11 @@ import ( | |
|
|
||
| // QDevConn holds the essential information to connect to AWS S3 | ||
| type QDevConn struct { | ||
| // AccessKeyId for AWS | ||
| // AuthType determines how to authenticate with AWS: "access_key" or "iam_role" | ||
| AuthType string `mapstructure:"authType" json:"authType"` | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion (low): const (
AuthTypeAccessKey = "access_key"
AuthTypeIAMRole = "iam_role"
)This prevents typo-driven bugs and makes future additions easier to grep for. |
||
| // AccessKeyId for AWS (required when AuthType is "access_key") | ||
| AccessKeyId string `mapstructure:"accessKeyId" json:"accessKeyId"` | ||
| // SecretAccessKey for AWS | ||
| // SecretAccessKey for AWS (required when AuthType is "access_key") | ||
| SecretAccessKey string `mapstructure:"secretAccessKey" json:"secretAccessKey"` | ||
| // Region for AWS S3 | ||
| Region string `mapstructure:"region" json:"region"` | ||
|
|
@@ -42,6 +44,11 @@ type QDevConn struct { | |
| IdentityStoreRegion string `mapstructure:"identityStoreRegion" json:"identityStoreRegion"` | ||
| } | ||
|
|
||
| // IsIAMRoleAuth returns true if the connection uses IAM role authentication | ||
| func (conn *QDevConn) IsIAMRoleAuth() bool { | ||
| return conn.AuthType == "iam_role" | ||
| } | ||
|
|
||
| func (conn *QDevConn) Sanitize() QDevConn { | ||
| conn.SecretAccessKey = utils.SanitizeString(conn.SecretAccessKey) | ||
| return *conn | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| /* | ||
| Licensed to the Apache Software Foundation (ASF) under one or more | ||
| contributor license agreements. See the NOTICE file distributed with | ||
| this work for additional information regarding copyright ownership. | ||
| The ASF licenses this file to You under the Apache License, Version 2.0 | ||
| (the "License"); you may not use this file except in compliance with | ||
| the License. You may obtain a copy of the License at | ||
|
|
||
| http://www.apache.org/licenses/LICENSE-2.0 | ||
|
|
||
| Unless required by applicable law or agreed to in writing, software | ||
| distributed under the License is distributed on an "AS IS" BASIS, | ||
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| See the License for the specific language governing permissions and | ||
| limitations under the License. | ||
| */ | ||
|
|
||
| package migrationscripts | ||
|
|
||
| import ( | ||
| "github.com/apache/incubator-devlake/core/context" | ||
| "github.com/apache/incubator-devlake/core/dal" | ||
| "github.com/apache/incubator-devlake/core/errors" | ||
| "github.com/apache/incubator-devlake/core/plugin" | ||
| ) | ||
|
|
||
| var _ plugin.MigrationScript = (*addAuthTypeToConnection)(nil) | ||
|
|
||
| type addAuthTypeToConnection struct{} | ||
|
|
||
| func (*addAuthTypeToConnection) Up(basicRes context.BasicRes) errors.Error { | ||
| db := basicRes.GetDal() | ||
|
|
||
| if !db.HasColumn("_tool_q_dev_connections", "auth_type") { | ||
| if err := db.AddColumn("_tool_q_dev_connections", "auth_type", dal.Varchar); err != nil { | ||
| return errors.Default.Wrap(err, "failed to add auth_type to _tool_q_dev_connections") | ||
| } | ||
| } | ||
|
|
||
| // Default existing rows to "access_key" since they were created before IAM role support | ||
| if err := db.Exec("UPDATE _tool_q_dev_connections SET auth_type = 'access_key' WHERE auth_type IS NULL OR auth_type = ''"); err != nil { | ||
| return errors.Default.Wrap(err, "failed to set default auth_type for existing connections") | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| func (*addAuthTypeToConnection) Version() uint64 { | ||
| return 20260320000001 | ||
| } | ||
|
|
||
| func (*addAuthTypeToConnection) Name() string { | ||
| return "add auth_type column to _tool_q_dev_connections for IAM role support" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,10 +29,14 @@ import ( | |
|
|
||
| func NewQDevS3Client(taskCtx plugin.TaskContext, connection *models.QDevConnection) (*QDevS3Client, errors.Error) { | ||
| // 创建AWS session | ||
| sess, err := session.NewSession(&aws.Config{ | ||
| Region: aws.String(connection.Region), | ||
| Credentials: credentials.NewStaticCredentials(connection.AccessKeyId, connection.SecretAccessKey, ""), | ||
| }) | ||
| cfg := &aws.Config{ | ||
| Region: aws.String(connection.Region), | ||
| } | ||
| // 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, "") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit (low): This block is nearly identical to |
||
| } | ||
| sess, err := session.NewSession(cfg) | ||
| if err != nil { | ||
| return nil, errors.Convert(err) | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion (medium): A validation function ideally shouldn't mutate its input. Setting the default
AuthTypehere meansvalidateConnectionhas a side effect that callers may not expect. Consider moving the defaulting logic to a dedicatednormalizeConnection()step (or into the caller) and keepingvalidateConnectionpure.