Cli cicd mode #308

pbalogh-sa · 2023-05-10T15:19:34Z

Description

Please provide a meaningful description of what this change will do, or is for. Bonus points for including links to
related issues, other PRs, or technical references.

Note that by not including a description, you are asking reviewers to do extra work to understand the context of this
change, which may lead to your PR taking much longer to review, or result in it not being reviewed at all.

Type of Change

[ ] Bug Fix
[ ] New Feature
[ ] Breaking Change
[ ] Refactor
[ ] Documentation
[ ] Other (please describe)

Checklist

I have read the contributing guidelines
Existing issues have been referenced (where applicable)
I have verified this change is not present in other open pull requests
Functionality is documented
All code style checks pass
New code contribution is covered by automated tests
All new and existing tests pass

Signed-off-by: Peter Balogh <[email protected]>

api/openapi.yaml

backend/pkg/database/gorm/target.go

cli/cmd/root.go

FrimIdan · 2023-05-11T12:57:29Z

cli/cmd/root.go

 	rootCmd.PersistentFlags().StringVar(&scanResultID, "scan-result-id", "", "the ScanResult ID to export the scan results to")
 	rootCmd.PersistentFlags().BoolVar(&mountVolume, "mount-attached-volume", false, "discover for an attached volume and mount it before the scan")
 	rootCmd.PersistentFlags().BoolVar(&waitForServerAttached, "wait-for-server-attached", false, "wait for the VMClarity server to attach the volume")
+	rootCmd.PersistentFlags().StringVar(&input, "input", "", "input for families")


what do you mean by input (and input type) for families? it will replace the input from config? is that only for the cicd mode?

The input can be a different type in the case of CICD mode. For example, it can be a directory and in the future we have to support container image, container info, and vm image input type. It makes sense only CICD mode because in orchestrated mode we are scanning VMs at the moment.

IMO the input for all this should be the VMClarity CLI input yaml, ideally the CLI should have little to no command line arguments so that backwards compatibility is easier to maintain. For example the yaml could look something like:

asset: type: VM ... loadFamilySettingsFromScanConfig: <ID or Name> sbom: inputs: - type: ROOTFS path: /

If we want to provide the option to run VMClarity CLI without a yaml input, then IMO CLI flags we provide should be direct overrides of the options in the yaml "helm style" e.g. --set asset.type=VM

agree, not sure why we need different cli flags for that mode

cli/cmd/root.go

cli/pkg/cicd/updater/updater.go

cli/pkg/cicd/updater/vmclarity.go

cli/pkg/cicd/vminfoprovider/aws.go

Signed-off-by: Peter Balogh <[email protected]>

ghost · 2023-05-17T10:34:33Z

api/openapi.yaml

@@ -1442,12 +1442,18 @@ components:
        - $ref: '#/components/schemas/VMInfo'
        - $ref: '#/components/schemas/PodInfo'
        - $ref: '#/components/schemas/DirInfo'
+        - $ref: '#/components/schemas/VMImage'
+        - $ref: '#/components/schemas/ContainerInfo'


Not sure why this is "ContainerInfo" and not just container... I think the same for the existing types too... why is it "VMInfo" and not just TargetType: "VM"... :/

The new target types were removed now.

api/openapi.yaml

ghost · 2023-05-17T10:39:43Z

api/openapi.yaml

+        objectType:
+          type: string
+        containerName:
+          type: string


I think we need some info about where the container is running and the image that its based on etc right?

new targets were removed

ghost · 2023-05-17T10:49:21Z

backend/pkg/database/gorm/target.go

+				Reason: fmt.Sprintf("Target directory exists with same name=%q and location=%q", *info.DirName, *info.Location),
+			}
+		}
+		return nil, nil // nolint:nilnil


If we're adding the other types in the API should make sure they are all covered here otherwise we won't be able to use them

new targets were removed

ghost · 2023-05-17T11:01:03Z

cli/cmd/root.go

 	rootCmd.PersistentFlags().StringVar(&scanResultID, "scan-result-id", "", "the ScanResult ID to export the scan results to")
 	rootCmd.PersistentFlags().BoolVar(&mountVolume, "mount-attached-volume", false, "discover for an attached volume and mount it before the scan")
 	rootCmd.PersistentFlags().BoolVar(&waitForServerAttached, "wait-for-server-attached", false, "wait for the VMClarity server to attach the volume")
+	rootCmd.PersistentFlags().StringVar(&input, "input", "", "input for families")


IMO the input for all this should be the VMClarity CLI input yaml, ideally the CLI should have little to no command line arguments so that backwards compatibility is easier to maintain. For example the yaml could look something like:

asset: type: VM ... loadFamilySettingsFromScanConfig: <ID or Name> sbom: inputs: - type: ROOTFS path: /

If we want to provide the option to run VMClarity CLI without a yaml input, then IMO CLI flags we provide should be direct overrides of the options in the yaml "helm style" e.g. --set asset.type=VM

ghost · 2023-05-17T11:19:13Z

cli/pkg/cicd/vminfoprovider/aws.go

+	if err != nil {
+		return "", "", fmt.Errorf("failed to read availability-zone response body: %v", err)
+	}
+	az := string(availabilityZone)


We can just query http://169.254.169.254/latest/meta-data/placement/region for the region, then there is no need to post-process it.

thx, I missed it somehow.

ghost · 2023-05-17T11:39:52Z

cli/pkg/cli/cli.go

@@ -39,6 +40,7 @@ const (
 type CLI struct {
 	state.Manager
 	presenter.Presenter
+	updater.Updater


I don't think there is a need for a new concept here, I think almost everything can be hidden inside the existing state.Manager and presenter.Presenter concepts, if we create a new "CICDStateManagerPresenter" which wraps the VMClarity state manager and presenter, the "WaitForVolumeAttachment" and "IsAborted" functions can be empty.

"MarkInProgress" can:

Create the Asset as Defined by the user

Create a Scan with just that Asset defined as the scanned asset

Create a ScanResult with that Scan ID

Call the VMClarity state manager MarkInProgress with the ScanResult ID

then "MarkDone" can:

Call the normal VMClarity state manager MarkDone

Update the Scan with the summary from the ScanResult and mark it as completed.

All the presenter functions will be passed straight through except the ScanResult ID from MarkInProgress will be injected. This way the work that Erez is doing to allow for incremental status updates etc will still work even in the CI/CD flow.

If we don't need the incremental updates stuff and we don't want a Scan in the system until we've succeeded, we can leave MarkInProgress empty, and then cache all the results as they arrive through our presenter interface, and then do everything we need to do in the "MarkDone".

The update was removed from the cli

ghost · 2023-05-17T11:41:03Z

runtime_scan/pkg/config/config.go

-
-	GrypeServerAddress string
+	// Scanner server and exploit DB addresses for families
+	FamiliesAddresses families.Addresses


Why are we changing this in this PR?

I changed back

ghost · 2023-05-17T11:45:12Z

shared/pkg/families/config.go

Not sure this belongs in shared, this requires families logic to know about the VMClarity API concepts. If we want to put it anywhere "common" it should be in the VMClarity CLI package, and then the runtime_scan can import it from there.

ghost · 2023-05-17T11:51:35Z

shared/pkg/families/config.go

+	ChkrootkitBinaryPath          = "CHKROOTKIT_BINARY_PATH"
+	ExploitDBAddress              = "EXPLOIT_DB_ADDRESS"
+	TrivyServerAddress            = "TRIVY_SERVER_ADDRESS"
+	GrypeServerAddress            = "GRYPE_SERVER_ADDRESS"


families logic should only receive a configuration struct from the outside, it should not be doing any loading of configurations itself, that is the responsibility of the CLI.

The CLI code uses viper + yaml, we should not be using environment variables like this anywhere in this flow, and if we want defaults then we should be handling it as part of loading that configuration. E.g. setting a default for rootkits.scannerconfig.chkrootkit.binarypath.

I removed from families

Signed-off-by: Peter Balogh <[email protected]>

cli/cmd/root.go

cli/pkg/standalone/updater/updater.go

shared/pkg/utils/utils.go

Signed-off-by: Peter Balogh <[email protected]>

akpsgit · 2023-05-31T13:01:20Z

#334

Signed-off-by: Peter Balogh <[email protected]>

chrisgacsal · 2023-06-07T18:31:25Z

cli/pkg/config/config.go

+func setValuesFromArgs(values []string) {
+	if len(values) == 0 {
+		return
+	}
+	for _, v := range values {
+		key, value := splitKeyValue(v)
+		viper.SetDefault(key, value)
+	}
+}
+
+func splitKeyValue(value string) (string, string) {
+	keyValue := strings.Split(value, "=")
+	return keyValue[0], keyValue[1]
+}


I'd not add a separate function just for splitting a string.

Suggested change

func setValuesFromArgs(values []string) {

if len(values) == 0 {

return

}

for _, v := range values {

key, value := splitKeyValue(v)

viper.SetDefault(key, value)

}

}

func splitKeyValue(value string) (string, string) {

keyValue := strings.Split(value, "=")

return keyValue[0], keyValue[1]

}

func setValuesFromArgs(values []string) {

if len(values) == 0 {

return

}

for _, value := range values {

v := strings.Split(value, "=")

if len(v) == 2 {

viper.SetDefault(v[0], v[1])

} else {

// do something here?

}

}

}

chrisgacsal · 2023-06-07T18:33:57Z

cli/pkg/standalone/initiator/initiator.go

+type Config struct {
+	client         *backendclient.BackendClient
+	fmConfig       *families.Config
+	scanConfigID   string
+	scanConfigName string
+	input          string
+	asset          cliconfig.Asset
+}
+
+func CreateConfig(
+	client *backendclient.BackendClient,
+	fmConfig *families.Config,
+	scanConfigID, scanConfigName, input string,
+	asset cliconfig.Asset,
+) Config {
+	return Config{
+		client:         client,
+		fmConfig:       fmConfig,
+		scanConfigID:   scanConfigID,
+		scanConfigName: scanConfigName,
+		input:          input,
+		asset:          asset,
+	}
+}


I do not see the purpose of having a CreateConfig (which should be really called NewConfig) here. Just make the Config attributes exported.

Suggested change

type Config struct {

client *backendclient.BackendClient

fmConfig *families.Config

scanConfigID string

scanConfigName string

input string

asset cliconfig.Asset

}

func CreateConfig(

client *backendclient.BackendClient,

fmConfig *families.Config,

scanConfigID, scanConfigName, input string,

asset cliconfig.Asset,

) Config {

return Config{

client: client,

fmConfig: fmConfig,

scanConfigID: scanConfigID,

scanConfigName: scanConfigName,

input: input,

asset: asset,

}

}

type Config struct {

Client *backendclient.BackendClient

FamiliesConfig *families.Config

ScanConfigID string

ScanConfigName string

Input string

Asset cliconfig.Asset

}

chrisgacsal · 2023-06-07T18:42:16Z

cli/pkg/standalone/initiator/vmclarity.go

+	Config
+}
+
+func newVMClarityInitiator(config Config) (*VMClarityInitiator, error) {


Suggested change

func newVMClarityInitiator(config Config) (*VMClarityInitiator, error) {

func New(config Config) (*VMClarityInitiator, error) {

chrisgacsal · 2023-06-07T18:46:38Z

cli/pkg/standalone/initiator/vmclarity.go

+func createInitScanResultSummary() *models.ScanFindingsSummary {
+	return &models.ScanFindingsSummary{
+		TotalExploits:          utils.PointerTo(0),
+		TotalMalware:           utils.PointerTo(0),
+		TotalMisconfigurations: utils.PointerTo(0),
+		TotalPackages:          utils.PointerTo(0),
+		TotalRootkits:          utils.PointerTo(0),
+		TotalSecrets:           utils.PointerTo(0),
+		TotalVulnerabilities: &models.VulnerabilityScanSummary{
+			TotalCriticalVulnerabilities:   utils.PointerTo(0),
+			TotalHighVulnerabilities:       utils.PointerTo(0),
+			TotalMediumVulnerabilities:     utils.PointerTo(0),
+			TotalLowVulnerabilities:        utils.PointerTo(0),
+			TotalNegligibleVulnerabilities: utils.PointerTo(0),
+		},
+	}
+}
+
+func createInitScanSummary() *models.ScanSummary {
+	return &models.ScanSummary{
+		JobsCompleted:          utils.PointerTo(0),
+		JobsLeftToRun:          utils.PointerTo(1),
+		TotalExploits:          utils.PointerTo(0),
+		TotalMalware:           utils.PointerTo(0),
+		TotalMisconfigurations: utils.PointerTo(0),
+		TotalPackages:          utils.PointerTo(0),
+		TotalRootkits:          utils.PointerTo(0),
+		TotalSecrets:           utils.PointerTo(0),
+		TotalVulnerabilities: &models.VulnerabilityScanSummary{
+			TotalCriticalVulnerabilities:   utils.PointerTo(0),
+			TotalHighVulnerabilities:       utils.PointerTo(0),
+			TotalMediumVulnerabilities:     utils.PointerTo(0),
+			TotalLowVulnerabilities:        utils.PointerTo(0),
+			TotalNegligibleVulnerabilities: utils.PointerTo(0),
+		},
+	}
+}


I'd consolidate these in api/models package as these are used in the scanwatcher controller as well.

chrisgacsal · 2023-06-07T18:57:52Z

cli/pkg/standalone/vminfoprovider/aws.go

+type AWSInfoProvider struct{}
+
+func CreateNewAWSInfoProvider() *AWSInfoProvider {
+	return &AWSInfoProvider{}
+}
+
+func (a *AWSInfoProvider) GetVMInfo() (string, string, error) {
+	instanceIDResp, err := http.Get("http://169.254.169.254/latest/meta-data/instance-id") // nolint:noctx
+	if err != nil {
+		return "", "", fmt.Errorf("failed to get instance-id: %v", err)
+	}
+	defer instanceIDResp.Body.Close()
+	instanceID, err := io.ReadAll(instanceIDResp.Body)
+	if err != nil {
+		return "", "", fmt.Errorf("failed to read instance-id response body: %v", err)
+	}
+
+	regionResp, err := http.Get("http://169.254.169.254/latest/meta-data/placement/region") // nolint:noctx
+	if err != nil {
+		return "", "", fmt.Errorf("failed to get region: %v", err)
+	}
+	defer regionResp.Body.Close()
+	region, err := io.ReadAll(regionResp.Body)
+	if err != nil {
+		return "", "", fmt.Errorf("failed to read region response body: %v", err)
+	}
+
+	// cut the last character from the availability-zone in order to return only the region
+	return string(instanceID), string(region), nil
+}


Would it be better to add a new interface to the provider package? Like this:

type Provider interface { Kind() models.CloudProvider Discoverer Scanner } type Discoverer interface { // Omitted } type Scanner interface { // Omitted } type AssetInformer interface { // <-- new interface AssetInfo(context.Context) (models.TargetType, error) }

This way the provider implementations could decide if they want to implement the all the interfaces (e.g. Provider) or just the subset of it.
Personally I avoid having interfaces related to the same infra layer scattered across different packages in the code base.

chrisgacsal · 2023-06-07T19:00:58Z

shared/pkg/backendclient/client.go

@@ -619,3 +619,34 @@ func (b *BackendClient) PostFinding(ctx context.Context, finding models.Finding)
 		return nil, fmt.Errorf("failed to create a finding. status code=%v", resp.StatusCode())
 	}
 }
+


This bloats the backendclient unnecessarily https://github.com/openclarity/vmclarity/pull/308/files#diff-189663ef212b0528411bdd1bc250e2c49471f3ae97cf265d63bf6d087212b314R623-R652
(btw, do not get having the GetScanResultSummary either)

chrisgacsal · 2023-06-07T19:04:22Z

shared/pkg/families/config.go

 	"github.com/openclarity/vmclarity/shared/pkg/families/vulnerabilities"
 )

+const (


Are these default values? If so, please add Default prefix to their names. Otherwise these do not belong here in my opinion.

github-actions · 2023-08-13T00:17:21Z

Thank you for your contribution! This PR has been automatically marked as stale because it has no recent activity in the last 60 days. It will be closed in 14 days, if no further activity occurs. If this pull request is still relevant, please leave a comment to let us know, and the stale label will be automatically removed.

pbalogh-sa · 2023-08-16T11:22:23Z

implemented in: #500

pbalogh-sa added 7 commits May 2, 2023 12:22

feat(API): add new target types to API

468f143

refactor: scanfamilies config creation

544c8ec

Signed-off-by: Peter Balogh <[email protected]>

feat: initial CICD mode

5dd0feb

Signed-off-by: Peter Balogh <[email protected]>

fix: init summary

e3dd089

Signed-off-by: Peter Balogh <[email protected]>

update target uniqueness check in order to add dirInfo spport

f770ce2

Signed-off-by: Peter Balogh <[email protected]>

init scan summary

1332341

Signed-off-by: Peter Balogh <[email protected]>

add vm info support

56b4681

pbalogh-sa marked this pull request as ready for review May 11, 2023 12:39

pbalogh-sa requested a review from a team as a code owner May 11, 2023 12:39

pbalogh-sa added 2 commits May 11, 2023 14:48

merge main and resolve conflicts

4a1fb20

Signed-off-by: Peter Balogh <[email protected]>

fix: lint errors

4306119

Signed-off-by: Peter Balogh <[email protected]>

FrimIdan reviewed May 11, 2023

View reviewed changes

fix: changes according to review

675cb40

Signed-off-by: Peter Balogh <[email protected]>

ghost suggested changes May 17, 2023

View reviewed changes

pbalogh-sa added 7 commits May 18, 2023 16:09

remove updater and update scan summary in status manager

cc9b7f3

Signed-off-by: Peter Balogh <[email protected]>

fix log

5a1d37d

extend cli config

9514f5b

Signed-off-by: Peter Balogh <[email protected]>

add --set flag

fb5df78

rename cicd mode to standalone

8de4f79

Signed-off-by: Peter Balogh <[email protected]>

remove target types that are not supported yet

e4b2999

Signed-off-by: Peter Balogh <[email protected]>

fix: fn name

427ca6f

pbalogh-sa marked this pull request as draft May 19, 2023 13:55

fix: read config from file

27dbf01

Signed-off-by: Peter Balogh <[email protected]>

pbalogh-sa marked this pull request as ready for review May 19, 2023 14:37

fix: lint error

4f7bb86

Signed-off-by: Peter Balogh <[email protected]>

FrimIdan reviewed May 29, 2023

View reviewed changes

cli/cmd/root.go Outdated Show resolved Hide resolved

FrimIdan reviewed May 29, 2023

View reviewed changes

cli/pkg/standalone/updater/updater.go Outdated Show resolved Hide resolved

FrimIdan reviewed May 29, 2023

View reviewed changes

shared/pkg/utils/utils.go Outdated Show resolved Hide resolved

pbalogh-sa added 2 commits May 31, 2023 08:39

review

f09e8ec

Signed-off-by: Peter Balogh <[email protected]>

reword help for input

2886499

add helper for updating scan summary

aa14676

Signed-off-by: Peter Balogh <[email protected]>

merge main and resolve conclicts

aea9f78

Signed-off-by: Peter Balogh <[email protected]>

chrisgacsal suggested changes Jun 7, 2023

View reviewed changes

pbalogh-sa marked this pull request as draft June 9, 2023 12:38

github-actions bot added the stale label Aug 13, 2023

pbalogh-sa closed this Aug 16, 2023

chrisgacsal deleted the cli-cicd-mode branch October 12, 2023 20:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cli cicd mode #308

Cli cicd mode #308

pbalogh-sa commented May 10, 2023

FrimIdan May 11, 2023

pbalogh-sa May 16, 2023

ghost May 17, 2023

FrimIdan May 29, 2023

ghost May 17, 2023

pbalogh-sa May 31, 2023

ghost May 17, 2023

pbalogh-sa May 31, 2023

ghost May 17, 2023

pbalogh-sa May 31, 2023

ghost May 17, 2023

ghost May 17, 2023

pbalogh-sa May 17, 2023

ghost May 17, 2023

pbalogh-sa May 31, 2023

ghost May 17, 2023

pbalogh-sa May 31, 2023

ghost May 17, 2023

ghost May 17, 2023

pbalogh-sa May 31, 2023

akpsgit commented May 31, 2023

chrisgacsal Jun 7, 2023

chrisgacsal Jun 7, 2023

chrisgacsal Jun 7, 2023

chrisgacsal Jun 7, 2023

chrisgacsal Jun 7, 2023 •

edited

Loading

chrisgacsal Jun 7, 2023

chrisgacsal Jun 7, 2023

github-actions bot commented Aug 13, 2023

pbalogh-sa commented Aug 16, 2023

	func newVMClarityInitiator(config Config) (*VMClarityInitiator, error) {
	func New(config Config) (*VMClarityInitiator, error) {

Cli cicd mode #308

Cli cicd mode #308

Conversation

pbalogh-sa commented May 10, 2023

Description

Type of Change

Checklist

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

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

akpsgit commented May 31, 2023

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

chrisgacsal Jun 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 13, 2023

pbalogh-sa commented Aug 16, 2023

chrisgacsal Jun 7, 2023 •

edited

Loading