#584 Added Support for SSM based Instance #1159

Horiodino · 2023-11-03T08:10:11Z

Problem

Support AWS Systems Manager managed-instane #584

Changes Made

https://github.com/tailwarden/komiser/pull/1159/files

Notes

this hasn't been tested yet , i opened this pull request to view the code for the reviewers to know what i did
and if theirs any problem please correct me .

Checklist

Code follows the contributing guidelines
Changes have been thoroughly tested
Documentation has been updated, if necessary
Any dependencies have been added to the project, if necessary

Reviewers

@[username of the reviewer]

Azanul

Thanks for your contribution.
I've a few comments regarding the changes

The changes belong to a new file in systemsmanager folder
Take inspiration from how otherFetchDataFunctions are written
don't use "." imports

…anager folder.

Azanul

Drop the cost calculation as we'll be going with AWS cost explorer API integration for AWS provider

Horiodino · 2023-11-17T14:54:58Z

Drop the cost calculation as we'll be going with AWS cost explorer API integration for AWS provider

yep i removed the cost monitoring and will be doing test on it . any suggestion where i should i call the function from

Horiodino · 2023-11-23T17:32:03Z

almost done with testing and going to do some changes 👍

Horiodino · 2023-11-24T04:22:56Z

@Azanul ```
[
{
"id": ,
"resourceId": "i-0b360c042863610a2",
"provider": "AWS",
"account": "my-account",
"accountId": "my-account-id",
"service": "EC2",
"region": "ap-south-1",
"name": "t2.micro",
"createdAt": "2023-11-24T02:22:33Z",
"fetchedAt": "2023-11-24T09:45:21.242287061+05:30",
"cost": 0,
"metadata": null,
"relations": null,
"tags": [
{
"key": "Name",
"value": "test"
}
],
"link": "https://ap-south-1.console.aws.amazon.com/ec2/home?region=ap-south-1#InstanceDetails:instanceId=i-0b360c042863610a2",
"Value": ""
}
]

Azanul

failing CLI

Azanul · 2023-11-24T05:27:14Z

providers/aws/systemsmanager/managedinstances.go

+				Region:     client.AWSClient.Region,
+				ResourceId: *ec2instance.InstanceId,
+				Name:       *ec2instance.Name,
+				Cost:       0.0, // No cost calculation in this version


Suggested change

Cost: 0.0, // No cost calculation in this version

Azanul · 2023-11-24T05:27:27Z

providers/aws/systemsmanager/managedinstances.go

+				Name:       *ec2instance.Name,
+				Cost:       0.0, // No cost calculation in this version
+				CreatedAt:  *ec2instance.RegistrationDate,
+				Tags:       nil, // No tags in this version


Suggested change

Tags: nil, // No tags in this version

Horiodino · 2023-11-24T06:06:07Z

failing CLI

its because i didn't committed latest changes , i am asking what should be in the

"id":

once the id is populated i will push the commits

Azanul · 2023-11-24T06:11:09Z

failing CLI

its because i didn't committed latest changes , i am asking what should be in the
"id": 
once the id is populated i will push the commits

I think that's auto. It'll be filled by the respective DB.

Azanul

I see too many API requests being sent. Could you move client creation to the main function and create helper functions to fetch respective data for all instances at once instead of making an API call for each instance.
refer to other implementations for error handling

Horiodino · 2023-11-28T13:09:32Z

I see too many API requests being sent. Could you move client creation to the main function and create helper functions to fetch respective data for all instances at once instead of making an API call for each instance.

refer to other implementations for error handling

Ok

Horiodino · 2023-12-08T17:59:46Z

hey @Azanul now i am fetching the all instance info at once which reduce the API calls , and now just few API calls , i have also tested it

[
  {
    "id": 0,
    "resourceId": "i-037adb5a71eb0be30",
    "provider": "AWS",
    "account": "my-account :) ",
    "accountId": "my-id :) ",
    "service": "EC2",
    "region": "ap-south-1",
    "name": "t2.micro",
    "createdAt": "2023-12-08T17:32:49Z",
    "fetchedAt": "2023-12-08T23:15:30.813538473+05:30",
    "cost": 0,
    "metadata": null,
    "relations": null,
    "tags": [
      {
        "key": "Name",
        "value": "komi"
      }
    ],
    "link": "https://ap-south-1.console.aws.amazon.com/ec2/home?region=ap-south-1#InstanceDetails:instanceId=i-037adb5a71eb0be30",
    "Value": ""
  }
]

AvineshTripathi · 2024-01-04T13:27:18Z

providers/aws/systemsmanager/managedinstances.go

+	ec2Client := ec2.NewFromConfig(*client.AWSClient)
+
+	ssmOutput, err := ssmClient.DescribeInstanceInformation(ctx, &ssm.DescribeInstanceInformationInput{
+		MaxResults: aws.Int32(50),


Any reason we are keeping this parameter

it tells what instances are managed ec2 instances, and later we are using describe because it provides more info regarding the instance, and the parameter is set to MaxResults: aws.Int32(50) is maximum value we can use (Defined by aws). another reason is making as less as possible call, thats why the MaxResults: aws.Int32(50)

In case of SSM managing more instances > 50 we would still need those too as we want users to know what resources in what quantity are active

I didn't know that we have pagination token, let me add it so it will fetch all ssm intance even its >50 👍

@AvineshTripathi thanks for pointing it out , i removed the MaxResults because if the ssm-instance is less than 5 it will through error now just using pagination token 👍 .

AvineshTripathi · 2024-01-04T13:29:05Z

providers/aws/systemsmanager/managedinstances.go

+		MaxResults: aws.Int32(50),
+	})
+	if err != nil {
+		log.Fatal(err)


Suggested change

log.Fatal(err)

return resources, err

AvineshTripathi · 2024-01-04T13:41:36Z

providers/aws/systemsmanager/managedinstances.go

+		InstanceIds: instanceIds,
+	})
+	if err != nil {
+		return nil, err


Suggested change

return nil, err

return resources, err

some nits for readability

AvineshTripathi · 2024-01-04T13:41:50Z

providers/aws/systemsmanager/managedinstances.go

+
+	account, accountID, err := fetchID(ctx, client)
+	if err != nil {
+		return nil, err


Suggested change

return nil, err

return resources, err

AvineshTripathi · 2024-01-04T13:43:20Z

providers/aws/systemsmanager/managedinstances.go

+					Provider:   "AWS",
+					Account:    account,
+					AccountId:  accountID,
+					Service:    "EC2",


should we call this SSM Instance ?

yep, thanks for noticing it out 😃

Please donot add - and use space that is how we have done for other resources

AvineshTripathi

LGTM

Azanul

Please fix the failing CI. You need to call getManagedEc2. Check out other implementations.

…evelop

Azanul · 2024-01-24T07:56:27Z

@Horiodino Still failing

Horiodino · 2024-01-24T08:30:29Z

@Horiodino Still failing

in aws.go on line 103 i have added

providers/aws/systemsmanager/managedinstances.go

Co-authored-by: Azanul Haque <[email protected]>

Horiodino · 2024-01-24T12:43:53Z

o i added it but i forgot to commit it , sorry

* enhancement: added ssm based instance * enhancement: added cost-monitoring on smm based instances * Refactor: Removed dot imports and organized changes into the systemsmanager folder. * enhancement: Removed the cost calculation logic and related code * implemented suggested changes * Reduced API Requests & bulk data fetching for instances. * added suggested changes * added pagination token for fetching ssm-instance-info * fixed the failing CI * Update providers/aws/systemsmanager/managedinstances.go Co-authored-by: Azanul Haque <[email protected]> --------- Co-authored-by: Azanul Haque <[email protected]> Co-authored-by: LABOUARDY Mohamed <[email protected]>

Horiodino added 2 commits October 28, 2023 23:59

enhancement: added ssm based instance

ce7d668

enhancement: added cost-monitoring on smm based instances

a4d260d

Horiodino requested review from mlabouardy, ShubhamPalriwala, AvineshTripathi, greghub and Traxmaxx as code owners November 3, 2023 08:10

Azanul requested changes Nov 3, 2023

View reviewed changes

Horiodino and others added 2 commits November 15, 2023 10:11

Refactor: Removed dot imports and organized changes into the systemsm…

b9926c9

…anager folder.

Merge branch 'tailwarden:develop' into develop

f0982b7

Azanul requested changes Nov 16, 2023

View reviewed changes

enhancement: Removed the cost calculation logic and related code

bfaa0ef

Azanul requested changes Nov 24, 2023

View reviewed changes

Horiodino and others added 2 commits November 24, 2023 12:18

implemented suggested changes

304128d

Merge branch 'develop' into develop

e551500

Azanul requested changes Nov 28, 2023

View reviewed changes

Merge branch 'tailwarden:develop' into develop

9061768

Horiodino requested a review from jakepage91 as a code owner December 8, 2023 17:47

Reduced API Requests & bulk data fetching for instances.

59189f7

Horiodino requested a review from Azanul December 19, 2023 04:56

Horiodino added 2 commits December 23, 2023 18:06

Merge branch 'tailwarden:develop' into develop

e4b4a8e

Merge branch 'tailwarden:develop' into develop

990af2b

Azanul approved these changes Dec 26, 2023

View reviewed changes

AvineshTripathi reviewed Jan 4, 2024

View reviewed changes

Horiodino added 2 commits January 4, 2024 16:54

added suggested changes

18ee21d

added pagination token for fetching ssm-instance-info

6002acb

Horiodino requested a review from AvineshTripathi January 7, 2024 17:43

AvineshTripathi approved these changes Jan 9, 2024

View reviewed changes

Merge branch 'develop' into develop

1711704

Azanul requested changes Jan 13, 2024

View reviewed changes

mlabouardy and others added 3 commits January 22, 2024 11:01

Merge branch 'develop' into develop

07bc31e

fixed the failing CI

852c0d6

Merge branch 'develop' of https://github.com/Horiodino/komiser into d…

3af0e07

…evelop

Horiodino requested a review from Azanul January 24, 2024 04:40

Azanul requested changes Jan 24, 2024

View reviewed changes

providers/aws/systemsmanager/managedinstances.go Outdated Show resolved Hide resolved

Update providers/aws/systemsmanager/managedinstances.go

3c1b7b7

Co-authored-by: Azanul Haque <[email protected]>

Azanul approved these changes Jan 24, 2024

View reviewed changes

Azanul merged commit 2a23700 into tailwarden:develop Jan 24, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#584 Added Support for SSM based Instance #1159

#584 Added Support for SSM based Instance #1159

Horiodino commented Nov 3, 2023 •

edited

Loading

Azanul left a comment

Azanul left a comment

Horiodino commented Nov 17, 2023

Horiodino commented Nov 23, 2023

Horiodino commented Nov 24, 2023 •

edited

Loading

Azanul left a comment

Azanul Nov 24, 2023

Azanul Nov 24, 2023

Horiodino commented Nov 24, 2023

Azanul commented Nov 24, 2023

Azanul left a comment

Horiodino commented Nov 28, 2023

Horiodino commented Dec 8, 2023

AvineshTripathi Jan 4, 2024

Horiodino Jan 4, 2024

AvineshTripathi Jan 5, 2024

Horiodino Jan 7, 2024 •

edited

Loading

Horiodino Jan 7, 2024

AvineshTripathi Jan 4, 2024

AvineshTripathi Jan 4, 2024

AvineshTripathi Jan 4, 2024

AvineshTripathi Jan 4, 2024

Horiodino Jan 4, 2024

AvineshTripathi Jan 5, 2024

Horiodino Jan 7, 2024

AvineshTripathi left a comment

Azanul left a comment

Azanul commented Jan 24, 2024

Horiodino commented Jan 24, 2024

Horiodino commented Jan 24, 2024

#584 Added Support for SSM based Instance #1159

#584 Added Support for SSM based Instance #1159

Conversation

Horiodino commented Nov 3, 2023 • edited Loading

Problem

Changes Made

Notes

Checklist

Reviewers

Azanul left a comment

Choose a reason for hiding this comment

Azanul left a comment

Choose a reason for hiding this comment

Horiodino commented Nov 17, 2023

Horiodino commented Nov 23, 2023

Horiodino commented Nov 24, 2023 • edited Loading

Azanul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Horiodino commented Nov 24, 2023

Azanul commented Nov 24, 2023

Azanul left a comment

Choose a reason for hiding this comment

Horiodino commented Nov 28, 2023

Horiodino commented Dec 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Horiodino Jan 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AvineshTripathi left a comment

Choose a reason for hiding this comment

Azanul left a comment

Choose a reason for hiding this comment

Azanul commented Jan 24, 2024

Horiodino commented Jan 24, 2024

Horiodino commented Jan 24, 2024

Horiodino commented Nov 3, 2023 •

edited

Loading

Horiodino commented Nov 24, 2023 •

edited

Loading

Horiodino Jan 7, 2024 •

edited

Loading