Orchestrator - Discovery + Run scan job #3

fishkerez · 2022-11-13T08:18:43Z

Add Orchestrator logic, same as in KubeClarity.

Main features:

Get a runtime scan request with scan scope
discover all the instances that need to scan
for each instance, run a scanner job:
- take a snapshot of the target instance root volume
- spin a scanning job with the snapshot attach to it
- delete the job with all relevant resources when done

Discovery flow according to Scan Scope Configuration:

All or…
By regions, which in each region the user can choose specific:
- VPCs which in each VPC the user can choose specific security groups

In each scope type the user can choose tag selector:

Non-running (stopped) VMs
Excluded Tags
Included Tags

runtime_scan/pkg/scanner/job_managment.go

runtime_scan/pkg/config/config.go

runtime_scan/pkg/config/scan_config.go

runtime_scan/pkg/types/types.go

runtime_scan/pkg/orchestrator/orchestrator.go

runtime_scan/pkg/config/config.go

runtime_scan/pkg/provider/aws/client.go

ghost · 2022-11-14T15:37:24Z

runtime_scan/pkg/provider/aws/client.go

+func (c *Client) WaitForInstanceReady(instance types.Instance) error {
+	ticker := time.NewTicker(3 * time.Second)
+	defer ticker.Stop()
+	timeout := time.After(3 * time.Minute)


We should probably use a context.WithTimeout here so that if we timeout here the timeout goes through to the ec2Client too like:

ctx := context.WithTimeout(context.Background(), 3 * time.Minute) for { select { case <-time.After(3 * time.Second) out, err := c.ec2Client.DescribeInstances(ctx, &ec2.DescribeInstancesInput{ .... case <- ctx.Done(): return ctx.Err() } }

I've also changed to putting the time.After into the for loop because using a ticker can result in two requests happening immediately if the request round trip takes about the same about of time as the ticker.

runtime_scan/pkg/provider/aws/client.go

runtime_scan/pkg/config/config.go

runtime_scan/pkg/provider/aws/client.go

runtime_scan/pkg/scanner/job_managment.go

ghost · 2022-11-15T08:53:51Z

runtime_scan/pkg/scanner/job_managment.go

+	select {
+	case <-data.resultChan:
+		log.WithFields(s.logFields).Infof("Instance scanned result has arrived. instanceID=%v", data.instance.ID)
+	case <-ticker.C:


Suggested change

case <-ticker.C:

case <- time.After(s.scanConfig.JobResultTimeout):

let's put a todo with sam's comment and then in the end of the project see what is the diff between the orchestrator here and kubeclarity, and handle the comments as part of the diff. WDYT?

ghost · 2022-11-15T08:54:47Z

runtime_scan/pkg/scanner/job_managment.go

+		data.timeout = true
+		data.completed = true
+		s.Unlock()
+	case <-ks:


ditto: replace with context

runtime_scan/pkg/scanner/job_managment.go

ghost · 2022-11-15T09:07:12Z

runtime_scan/pkg/scanner/job_managment.go

+	return s.providerClient.RunScanningJob(jobConfig)
+}
+
+func (s *Scanner) deleteJobIfNeeded(job *types.Job, isSuccessfulJob, isCompletedJob bool) {


doesn't need to be part of *Scanner

let's put a todo with sam's comment and then in the end of the project see what is the diff between the orchestrator here and kubeclarity, and handle the comments as part of the diff. WDYT?

runtime_scan/pkg/types/types.go

ghost

Thanks Eriz, lets get this merged and follow up with any cleanups/refactors

ghost · 2022-11-17T12:44:12Z

runtime_scan/pkg/provider/aws/client.go

-
-func (c *Client) LaunchInstance(ami, deviceName, subnetID string, snapshot types.Snapshot) (types.Instance, error) {
-	out, err := c.ec2Client.RunInstances(context.TODO(), &ec2.RunInstancesInput{
+func (c *Client) LaunchInstance(ctx context.Context, snapshot provider.Snapshot) (provider.Instance, error) {


We can make this a function of the snapshot interface i.e.:

instance := snapshot.LaunchScannerInstance(ctx context.Context)

then all the snapshot info that we need can remain private to the snapshot implementation struct.

Alternatively we can cast the snapshot when we receive it then we can use any private fields, and prevent weird errors:

func (c *Client) LaunchInstance(ctx context.Context, snapshot provider.Snapshot) (provider.Instance, error) { awsSnapshot, ok := snapshot.(*SnapshotImpl) if !ok { return nil, fmt.Errorf("can not launch AWS instance with non-aws snapshot") } }

this can be done in a follow up

ghost · 2022-11-17T12:50:30Z

runtime_scan/pkg/provider/aws/volume.go

+	}
+	out, err := v.ec2Client.CreateSnapshot(ctx, &params, func(options *ec2.Options) {
+		options.Region = v.region
+	})


I wonder if the requirement to move the snapshots between regions is AWS specific weirdness, I think if that is the case, that can be part of this function. The AWS provider snapshot implementation could then keep track of both src and dest snapshot ID's for the cleanup.

This can be cleaned up in follow up PRs

ghost · 2022-11-17T13:03:44Z

runtime_scan/pkg/scanner/job_managment.go

+
+	cpySnapshot, err := snapshot.Copy(ctx, s.region)
+	if err != nil {
+		return provider.Job{}, fmt.Errorf("failed to copy snapshot %v: %v", snapshot.GetID(), err)
 	}


Maybe wrap this in `if s.Region != snapshot.GetRegion()" ? In the case when we don't need to copy the snapshot we can leave SrcSnapshot as nil in the job for the cleanup

This can be cleaned up in follow up PRs

ghost · 2022-11-17T13:16:59Z

runtime_scan/pkg/provider/types.go

+	ImageID        string
+	DeviceName     string
+	SubnetID       string
+}


nit: I don't think the Job/JobConfig belong in the provider package, they are part of the scanner and should refer to objects from the providers.

This can be cleaned up in follow up PRs

akpsgit · 2022-11-20T09:17:25Z

runtime_scan/pkg/provider/aws/client.go

@@ -304,18 +304,30 @@ func (c *Client) ListAllRegions(ctx context.Context) ([]Region, error) {
 	return ret, nil
 }

+// AND logic - if excludeTags = {tag1:val1, tag2:val2},
+// then instance will be excluded only if he have ALL this tags ({tag1:val1, tag2:val2})


// then an instance will be excluded only if it has ALL these tags ({tag1:val1, tag2:val2})

akpsgit · 2022-11-20T09:21:50Z

runtime_scan/pkg/provider/aws/client_test.go

@@ -314,7 +314,7 @@ func Test_hasExcludedTags(t *testing.T) {
 			want: false,
 		},
 		{
-			name: "instance has excluded tags",
+			name: "instance does not have ALL the excluded tags",


worth also adding a test where one of the tags in the excluded is not matched (case of partial matching)

There is also this test- instance does not have ALL the excluded tags

akpsgit · 2022-11-20T09:32:38Z

runtime_scan/pkg/provider/aws/client_test.go

@@ -77,7 +77,7 @@ import (
 //			},
 //		},
 //		ScanStopped: true,
-//		IncludeTags: []*types.Tag{
+//		TagSelector: []*types.Tag{


can we delete the dead tests?

akpsgit · 2022-11-20T09:51:20Z

runtime_scan/pkg/provider/aws/client_test.go

+)
+
+//
+//func TestClient_ListAllRegions(t *testing.T) {


do we want to delete this?

fishkerez requested review from akpsgit, FrimIdan and a user and removed request for akpsgit, FrimIdan and a user November 13, 2022 08:20

fishkerez self-assigned this Nov 13, 2022

Erez Fishhimer added 4 commits November 13, 2022 11:07

discovery + run job

2494e88

ready

7135862

discovery + scanning

7c07e9e

small changes

1812b5a

fishkerez force-pushed the targets-discovery branch from 802baba to 1812b5a Compare November 13, 2022 09:09

fishkerez requested review from FrimIdan and akpsgit and removed request for akpsgit and FrimIdan November 13, 2022 09:10

Erez Fishhimer added 5 commits November 13, 2022 11:13

add licenses

a96f3a2

edit makefile and dockerfile

a99ee23

remove comment

78c902d

use scanner subnet id

da39798

add some more unit tests

5fbb089

fishkerez commented Nov 13, 2022

View reviewed changes

runtime_scan/pkg/scanner/job_managment.go Outdated Show resolved Hide resolved

Erez Fishhimer added 2 commits November 13, 2022 15:21

move scanner logic to the specific provider client

b3efb7f

remove comment

048701f

akpsgit reviewed Nov 13, 2022

View reviewed changes

runtime_scan/pkg/config/config.go Show resolved Hide resolved

akpsgit reviewed Nov 13, 2022

View reviewed changes

runtime_scan/pkg/config/scan_config.go Outdated Show resolved Hide resolved

akpsgit reviewed Nov 13, 2022

View reviewed changes

runtime_scan/pkg/config/scan_config.go Outdated Show resolved Hide resolved

akpsgit reviewed Nov 13, 2022

View reviewed changes

runtime_scan/pkg/types/types.go Outdated Show resolved Hide resolved

akpsgit reviewed Nov 13, 2022

View reviewed changes

runtime_scan/pkg/orchestrator/orchestrator.go Outdated Show resolved Hide resolved

akpsgit requested review from a user, pbalogh-sa and FrimIdan and removed request for FrimIdan, a user and pbalogh-sa November 13, 2022 16:39

simplify

311b738

ghost suggested changes Nov 14, 2022

View reviewed changes

ghost suggested changes Nov 15, 2022

View reviewed changes

Erez Fishhimer added 2 commits November 17, 2022 13:53

review

827ba21

wip

84fc1de

ghost previously approved these changes Nov 17, 2022

View reviewed changes

move aws specific types to aws package

a5c4676

fishkerez dismissed ghost ’s stale review via a5c4676 November 20, 2022 07:09

Erez Fishhimer added 4 commits November 20, 2022 09:12

add license

0cf422b

format

f3f8610

some changes

30975bf

exclude tags and logic

d6f276b

akpsgit reviewed Nov 20, 2022

View reviewed changes

review

aa9dc6d

akpsgit previously approved these changes Nov 20, 2022

View reviewed changes

lint

b6b1935

fishkerez dismissed akpsgit’s stale review via b6b1935 November 20, 2022 10:50

Erez Fishhimer added 2 commits November 20, 2022 12:59

formt

5dc3c09

lint

2504140

fishkerez merged commit cc41f0d into main Nov 20, 2022

fishkerez deleted the targets-discovery branch November 20, 2022 11:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Orchestrator - Discovery + Run scan job #3

Orchestrator - Discovery + Run scan job #3

fishkerez commented Nov 13, 2022 •

edited

ghost Nov 14, 2022

fishkerez Nov 15, 2022

ghost Nov 15, 2022

akpsgit Nov 16, 2022

ghost Nov 15, 2022

ghost Nov 15, 2022

akpsgit Nov 16, 2022

ghost left a comment

ghost Nov 17, 2022

ghost Nov 17, 2022

ghost Nov 17, 2022

ghost Nov 17, 2022

ghost Nov 17, 2022

ghost Nov 17, 2022

ghost Nov 17, 2022

ghost Nov 17, 2022

akpsgit Nov 20, 2022

akpsgit Nov 20, 2022

fishkerez Nov 20, 2022

akpsgit Nov 20, 2022

fishkerez Nov 20, 2022

akpsgit Nov 20, 2022

fishkerez Nov 20, 2022

	case <-ticker.C:
	case <- time.After(s.scanConfig.JobResultTimeout):

Orchestrator - Discovery + Run scan job #3

Orchestrator - Discovery + Run scan job #3

Conversation

fishkerez commented Nov 13, 2022 • edited

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

ghost left a 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

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

fishkerez commented Nov 13, 2022 •

edited