Feature/multi account selection #177
Conversation
/collector/structs.go + AccountSpecifiedFiels struct to save AccountId and AccountName /collector/aws/common/structs.go + add Method getAccountName() /collector/aws/detector.go + save in DetectorManager the accountName from config file /collector/resources/* + save AccountSpecifiedFields with values in detectedStructs
… into feature_multi_account_selection
… into feature_multi_account_selection
There was a problem hiding this comment.
Hey @daniebrill,
Thanks for your contribution! this feature is amazing for our product!
before the technical review I have few notes:
-
when I ran your branch on our data we didn't saw details in the UI / the server returned empty data.
It looks likeBREAKING CHANGES. If someone installs the new version on his current envioronment it will not work. -
I ran the code from scratch with the following configuration and I'm seeing only one account:
---
name: test-1
log_level: info
api_server:
address: http://127.0.0.1:8081
bulk_interval: 5s
providers:
aws:
accounts:
- name: Account A
profile: finala-sts
regions:
- Region X
- name: Account B
profile: finala-sts
regions:
- Region Y
metrics:
....In Accounts: section I'm seeing only Account B filter (expected: Account A, Account B correct me i'm if i'm wrong), in the table view
I'm seeing all the accounts collector events
Amazing work and thank you for your help to moving Finala forward
| mq.MinimumShouldMatch("100%") | ||
| if operator == "and" { | ||
| mq = mq.Operator("and") | ||
| if name == "Data.AccountID" { |
There was a problem hiding this comment.
If we starting to manage specific logic on specific filters fields let's take the logic per filter outside of this function.
WDYT?
| if val, ok := spent.Aggregations["value"]; ok { | ||
| accountID, ok := AccountIdBucket.Key.(string) | ||
| if !ok { | ||
| log.Error("type assertion to string failed") |
There was a problem hiding this comment.
| log.Error("type assertion to string failed") | |
| log.WithFields(log.Fields{ | |
| "search_account_id": AccountIdBucket.Key.(string), | |
| "val": val, | |
| }).Error("type assertion to string failed") |
| log.Error("type assertion to string failed") | ||
| continue | ||
| } | ||
| spentAccounts[accountID], _ = strconv.ParseFloat(string(val), 64) |
There was a problem hiding this comment.
Can you catch the parsing error?
| @@ -298,6 +318,44 @@ func (sm *StorageManager) GetExecutions(queryLimit int) ([]storage.Executions, e | |||
| return executions, nil | |||
| } | |||
|
|
|||
| func (sm *StorageManager) GetAccounts(executionID string, querylimit int) ([]storage.Accounts, error) { | |||
There was a problem hiding this comment.
document the fucntion
// GetAccounts .....| @@ -298,6 +318,44 @@ func (sm *StorageManager) GetExecutions(queryLimit int) ([]storage.Executions, e | |||
| return executions, nil | |||
| } | |||
|
|
|||
| func (sm *StorageManager) GetAccounts(executionID string, querylimit int) ([]storage.Accounts, error) { | |||
| accounts := []storage.Accounts{} | |||
|
|
|||
There was a problem hiding this comment.
can you add logger instance with global fields that will show for each log that you write (better visibility ):
logger := log.WithField("execution_id", executionID) then use logger for any log line.
collector/aws/resources/elb.go
Outdated
| @@ -166,16 +171,28 @@ func (el *ELBManager) Detect(metrics []config.MetricConfig) (interface{}, error) | |||
| } | |||
| } | |||
|
|
|||
| Arn := "arn:aws:elasticloadbalancing:" + el.awsManager.GetRegion() + ":" + *el.awsManager.GetAccountIdentity().Account + ":loadbalancer/" + *instance.LoadBalancerName | |||
| @@ -166,18 +171,30 @@ func (ngw *NatGatewayManager) Detect(metrics []config.MetricConfig) (interface{} | |||
| } | |||
| } | |||
|
|
|||
| Arn := "arn:aws:ec2:" + ngw.awsManager.GetRegion() + ":" + *ngw.awsManager.GetAccountIdentity().Account + ":natgateway/" + *natgateway.NatGatewayId | |||
collector/aws/resources/redshift.go
Outdated
| @@ -147,18 +152,30 @@ func (rdm *RedShiftManager) Detect(metrics []config.MetricConfig) (interface{}, | |||
| } | |||
| } | |||
|
|
|||
| Arn := "arn:aws:redshift:" + rdm.awsManager.GetRegion() + ":" + *rdm.awsManager.GetAccountIdentity().Account + ":cluster:" + *cluster.ClusterIdentifier | |||
collector/collector.go
Outdated
| cm.updateServiceStatus(EventCollector{ | ||
| ResourceName: resourceName, | ||
| Data: EventStatusData{ | ||
| Status: EventFetch, | ||
| Status: EventFetch, | ||
| AccountInformation: accountSpecifiedFields.AccountName + "_" + accountSpecifiedFields.AccountID, |
There was a problem hiding this comment.
I have 2 questions:
- why not separate these fields? AccountName and AccountID?
- if we keep is it with concat string let move this logic to one place which you using it in 2 different functions
CollectStart,CollectFinish
interpolation/interpolation.go
Outdated
| @@ -59,3 +59,12 @@ func ExtractExecutionName(executionId string) (string, error) { | |||
|
|
|||
| return executionArr[0], nil | |||
| } | |||
|
|
|||
| // ExtractAccountInformation will return Account Name and Account ID | |||
| func ExtractAccountInformation(account string) (string, string, error) { | |||
There was a problem hiding this comment.
if you will split this value (as i maintain in collector/collector.go) you will not need this function. correct me if I'm wrong
|
Hey @daniebrill any news? |
|
Hey @kaplanelad, we are going to improve the code and react to your feedback next week. |
… into feature/multi_account_selection
… into feature/multi_account_selection
fix testutils
… into feature/multi_account_selection
fix bug: account is an array and not longer an object
|
Hey @daniebrill, |
|
Hi @kaplanelad , |
What type of PR is this?
UI Feature
Backend Feature
What this PR does / why we need it:
Add support for multiple accounts to UI + necessary functionality to backend
Does this PR introduce a breaking change, if so what is it?:
Additional documentation: