Feature/multi account selection by daniebrill · Pull Request #177 · similarweb/finala

daniebrill · 2021-07-05T15:32:42Z

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?:

- does not work with data from older versions
- safe account id + account name for detected resources
- api endpoint for getting all accounts
- add accounts to filter in ui

Additional documentation:

/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

* elasticsearch.go getDynamicMatchQuery add: if filterName is Data.AccountID then build boolQuery for given AccountIDs

fit MockAWSManager to the modified AWSManager

…ssertion

* save arn in resourceID field + add values in test objects

+ add accounts to the provider store + Show Accounts in a chip-list like resources + add Accounts to filter

… into feature_multi_account_selection

add comments

… into feature_multi_account_selection

add spentAccounts in summary add things to success tests

Add multiaccount-selection functionality for RessourceCharts

remove account charts with not data found

kaplanelad

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 like BREAKING 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

kaplanelad · 2021-08-01T08:32:09Z

-		mq.MinimumShouldMatch("100%")
-		if operator == "and" {
-			mq = mq.Operator("and")
+		if name == "Data.AccountID" {


If we starting to manage specific logic on specific filters fields let's take the logic per filter outside of this function.
WDYT?

kaplanelad · 2021-08-01T08:36:47Z

+			if val, ok := spent.Aggregations["value"]; ok {
+				accountID, ok := AccountIdBucket.Key.(string)
+				if !ok {
+					log.Error("type assertion to string failed")


Suggested change

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")

kaplanelad · 2021-08-01T08:38:27Z

+					log.Error("type assertion to string failed")
+					continue
+				}
+				spentAccounts[accountID], _ = strconv.ParseFloat(string(val), 64)


Can you catch the parsing error?

kaplanelad · 2021-08-01T08:39:02Z

 	return executions, nil
 }

+func (sm *StorageManager) GetAccounts(executionID string, querylimit int) ([]storage.Accounts, error) {


document the fucntion

// GetAccounts .....

kaplanelad · 2021-08-01T08:41:32Z


+func (sm *StorageManager) GetAccounts(executionID string, querylimit int) ([]storage.Accounts, error) {
+	accounts := []storage.Accounts{}
+


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.

kaplanelad · 2021-08-01T08:51:03Z

 					}
 				}

+				Arn := "arn:aws:elasticloadbalancing:" + el.awsManager.GetRegion() + ":" + *el.awsManager.GetAccountIdentity().Account + ":loadbalancer/" + *instance.LoadBalancerName


same question above

kaplanelad · 2021-08-01T08:52:15Z

 					}
 				}

+				Arn := "arn:aws:ec2:" + ngw.awsManager.GetRegion() + ":" + *ngw.awsManager.GetAccountIdentity().Account + ":natgateway/" + *natgateway.NatGatewayId


Same question above

kaplanelad · 2021-08-01T08:52:56Z

 					}
 				}

+				Arn := "arn:aws:redshift:" + rdm.awsManager.GetRegion() + ":" + *rdm.awsManager.GetAccountIdentity().Account + ":cluster:" + *cluster.ClusterIdentifier


Same question above

kaplanelad · 2021-08-01T08:55:28Z

 		Data: EventStatusData{
-			Status: EventFetch,
+			Status:             EventFetch,
+			AccountInformation: accountSpecifiedFields.AccountName + "_" + accountSpecifiedFields.AccountID,


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

kaplanelad · 2021-08-01T08:57:31Z

 }
+
+// ExtractAccountInformation will return Account Name and Account ID
+func ExtractAccountInformation(account string) (string, string, error) {


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

kaplanelad · 2021-08-16T11:38:01Z

Hey @daniebrill any news?

daniebrill · 2021-08-17T21:00:39Z

Hey @kaplanelad, we are going to improve the code and react to your feedback next week.

… into feature/multi_account_selection

fix testutils

… into feature/multi_account_selection

fix bug: account is an array and not longer an object

kaplanelad · 2021-09-06T05:37:02Z

Hey @daniebrill,
please let me know when we can recheck PR.

daniebrill · 2021-09-06T18:39:58Z

Hi @kaplanelad ,
we finished the improvements, so you can recheck.

Jackafive753 and others added 21 commits May 28, 2021 14:10

#SOS-22

35a3f71

* elasticsearch.go getDynamicMatchQuery add: if filterName is Data.AccountID then build boolQuery for given AccountIDs

#SOS-15

5866b6e

fit MockAWSManager to the modified AWSManager

#SOS-23 Add getAccounts + save account information in event_status

839ae2f

#SOS-23 Use querylimit in aggregation + add error handling for type a…

6cd0a85

…ssertion

#SOS-23 Fix existing tests + code format

f8e931b

#SOS-23 Modify GetAccounts for specific execution ID

259ccf7

#SOS-23 Fit MockStorage to modified interface StorageDescriber

45db9b4

#SOS-23 Add tests in interpolation_test.go and server_test.go

2f3c15c

#SOS-20

1fd641a

* save arn in resourceID field + add values in test objects

#SOS-23 Add test for elasticsearch GetAccounts

9c157f2

#SOS-16

0f68ad2

+ add accounts to the provider store + Show Accounts in a chip-list like resources + add Accounts to filter

Merge remote-tracking branch 'origin/feature_multi_account_selection'…

2b48f30

… into feature_multi_account_selection

#SOS18 Add multiaccount selection functionality to tables

bf4c57d

#SOS-16

ecbe7ef

add comments

Merge remote-tracking branch 'origin/feature_multi_account_selection'…

5a7065e

… into feature_multi_account_selection

#SOS-18

5ea12e5

add spentAccounts in summary add things to success tests

#SOS-18

2f99548

Add multiaccount-selection functionality for RessourceCharts

fix warnings: unused statements

4df494c

# SOS-57

ab6405e

remove account charts with not data found

set account to filters if an account specific Chart is clicked

f8adc70

kaplanelad self-requested a review August 1, 2021 07:00

kaplanelad reviewed Aug 1, 2021

View reviewed changes

Jackafive753 and others added 5 commits August 24, 2021 16:11

if config not a valid aws-account then give account a default id

bbc3bb9

list all accounts also with duplicate ids

9c31755

better logging

801bb42

rollback arn assembling

8de13fd

Add comment to GetAccounts method in elasticsearch.go

61ead41

daniebrill and others added 8 commits August 24, 2021 17:09

Merge remote-tracking branch 'origin/feature/multi_account_selection'…

9376f00

… into feature/multi_account_selection

rename AccountID to AccountId

263001f

Merge remote-tracking branch 'origin/feature/multi_account_selection'…

f76f47e

… into feature/multi_account_selection

split AccountInformation in AccountName and AccountId

e61e776

fix testutils

Remove ExtractAccountInformation from interpolation.go

63f7b85

fix bug: account with no id and no name returned

2eebe76

Merge remote-tracking branch 'origin/feature/multi_account_selection'…

ec38174

… into feature/multi_account_selection

fix bug: no data return when filter

399e4ee

fix bug: account is an array and not longer an object

daniebrill requested a review from kaplanelad September 21, 2021 13:21

-					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")


		func (sm *StorageManager) GetAccounts(executionID string, querylimit int) ([]storage.Accounts, error) {
		accounts := []storage.Accounts{}

Conversation

daniebrill commented Jul 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaplanelad left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaplanelad commented Aug 16, 2021

Uh oh!

daniebrill commented Aug 17, 2021

Uh oh!

kaplanelad commented Sep 6, 2021

Uh oh!

daniebrill commented Sep 6, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

daniebrill commented Jul 5, 2021 •

edited

Loading