Skip to content
This repository has been archived by the owner on Apr 1, 2022. It is now read-only.

Container scan (Analyze/Test w/ upload) #173

Merged
merged 16 commits into from
Jan 14, 2021
Merged

Container scan (Analyze/Test w/ upload) #173

merged 16 commits into from
Jan 14, 2021

Conversation

skilly-lily
Copy link
Contributor

@skilly-lily skilly-lily commented Dec 17, 2020

Fixes fossas/issues#312
Fixes fossas/issues#313
Fixes fossas/issues#314
Fixes fossas/issues#315

See fossas/issues#312 for more info.

Note that this is unlikely to merge until testing is complete, but is ready for review.

@skilly-lily skilly-lily requested review from cnr, zlav and jssblck December 17, 2020 02:31
Copy link
Contributor

@cnr cnr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great to me. A couple of minor comments, but nothing major

src/App/Fossa/Container.hs Outdated Show resolved Hide resolved
src/App/Fossa/Container/Test.hs Outdated Show resolved Hide resolved
-- , wigginsBinaryPath :: Path Abs File
-- }

data PackagedBinary
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a clever way of structuring this, and is probably good. I'm going to suggest another similar way to structure this, but don't feel obligated to change anything.

Rather than using PackagedBinary as a key for other functions like extractedPath, we can directly include that information in a record.

I would use this structure for PackagedBinary:

data PackagedBinary
  { packagedBinaryName :: Text
  , packagedBinaryBS :: ByteString
  }

and then we could replace, e.g.,

embeddedBinaryWiggins :: ByteString

with

embeddedBinaryWiggins :: PackagedBinary

I understand this loses us the clever allBins implementation, and compile-time warnings if we forget to include something in there, so I'm kind of split.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I'm not sure I understand the advantage of doing it the way you proposed. Can you explain a little more in detail why we might want to do it that way?

src/App/Fossa/EmbeddedBinary.hs Outdated Show resolved Hide resolved
@@ -100,6 +105,29 @@ appMain = do
unless vpsReportJsonOutput $ die "report command currently only supports JSON output. Please try `fossa report --json REPORT_NAME`"
baseDir <- validateDir vpsReportBaseDir
VPSReport.reportMain baseDir apiOpts logSeverity vpsReportTimeout vpsReportType override
--
ContainerCommand ContainerOptions {..} -> do
dieOnWindows "container scanning"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😢

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think windows support is later.

let apiOpts = ApiOpts optBaseUrl apikey
ContainerTest.testMain apiOpts logSeverity containerTestTimeout containerTestOutputType override containerTestImage
--
DumpBinsCommand dir -> do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great idea to add

Signed-off-by: Wesley Van Melle <van.melle.wes@gmail.com>
Signed-off-by: Wesley Van Melle <van.melle.wes@gmail.com>
Signed-off-by: Wesley Van Melle <van.melle.wes@gmail.com>
Signed-off-by: Wesley Van Melle <van.melle.wes@gmail.com>
Signed-off-by: Wesley Van Melle <van.melle.wes@gmail.com>
Signed-off-by: Wesley Van Melle <van.melle.wes@gmail.com>
Signed-off-by: Wesley Van Melle <van.melle.wes@gmail.com>
Signed-off-by: Wesley Van Melle <van.melle.wes@gmail.com>
Signed-off-by: Wesley Van Melle <van.melle.wes@gmail.com>
Signed-off-by: Wesley Van Melle <van.melle.wes@gmail.com>
Signed-off-by: Wesley Van Melle <van.melle.wes@gmail.com>
<*> obj .: "type"
<*> obj .: "purl"
<*> obj .: "metadataType"
-- We delete "files" as early as possible, which reduces
Copy link
Member

@zlav zlav Jan 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you elaborate on what a file is? I get why we need a comment here but I don't really understand what it is saying. I'm guessing this may make sense to me as I read further.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's some output from the syft binary that is under the nested json key artifacts[n].metadata.files.

{
  "artifacts": [
    {
      "metadata": {
        "files": [
          // enormous array of info we don't need
        ]
      }
    }
  ]
}

Since this value is unnecessary AND huge, we take steps to prevent loading it into memory as much as possible. Since haskell lazily evaluates data by default, and the Data.Map and Data.Map.Lazy internal map structure are strict in keys, but lazy in values, we can potentially remove the files key from this map without ever trying to hold the entire array in memory at once.

The comment is saying "we retain the json-like map structure, but we want to ignore the 'files' field to save a ton of memory."

case issuesCount issues of
0 -> logInfo "Test passed! 0 issues found"
n -> do
logError $ "Test failed. Number of issues found: " <> pretty n
Copy link
Member

@zlav zlav Jan 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know you were mirroring what we do in fossa test right now, but I feel like this should only print if the user has a full API token. Right now I think the user will see the following if they have a push only token.

Test failed. Number of issues found: Check webapp for more details, or use a full-access API key (currently using a push-only API key)

We should probably split these up so the user sees the following with a push only token.

Test failed. Check webapp for more details, or use a full-access API key (currently using a push-only API key)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why should this be different from fossa test? Also, why should we hide the number of issues when the count is always sent back?

Signed-off-by: Wesley Van Melle <van.melle.wes@gmail.com>
@skilly-lily skilly-lily merged commit 01e1545 into master Jan 14, 2021
@skilly-lily skilly-lily deleted the container-scan branch January 14, 2021 18:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants