🧹 split loading container images from opening connections #3237

imilchev · 2024-02-07T17:50:25Z

split loading of containers to a point after connecting. Creating the connection shouldn't be as expensive as it currently is

github-actions · 2024-02-07T17:54:37Z

Test Results

768 tests ±0 768 ✅ ±0 24s ⏱️ -1s
85 suites ±0 0 💤 ±0
2 files ±0 0 ❌ ±0

Results for commit e4ae430. ± Comparison against base commit 8ff1bae.

This pull request removes 6 and adds 6 tests. Note that renamed tests count towards both.

go.mondoo.com/cnquery/v10/llx ‑ TestRawData_JSON/0001-01-01_00:00:00_+0000_UTC
go.mondoo.com/cnquery/v10/llx ‑ TestRawData_JSON/292277026596-12-04_15:30:07_+0000_UTC
go.mondoo.com/cnquery/v10/llx ‑ TestRawData_JSON/292277026596-12-04_15:30:09_+0000_UTC
go.mondoo.com/cnquery/v10/llx ‑ TestSuccess/2024-02-12_11:54:29.364510299_+0000_UTC_m=+0.009455796
go.mondoo.com/cnquery/v10/llx ‑ TestTruthy/2024-02-12_11:54:29.364510299_+0000_UTC_m=+0.009455796
go.mondoo.com/cnquery/v10/llx ‑ TestTruthy/2024-02-12_11:54:29.364510299_+0000_UTC_m=+0.009455796#01

go.mondoo.com/cnquery/v10/llx ‑ TestRawData_JSON/0001-01-01_00:53:28_+0053_LMT
go.mondoo.com/cnquery/v10/llx ‑ TestRawData_JSON/292277026596-12-04_16:23:37_+0053_LMT
go.mondoo.com/cnquery/v10/llx ‑ TestRawData_JSON/292277026596-12-04_16:30:07_+0100_CET
go.mondoo.com/cnquery/v10/llx ‑ TestSuccess/2024-02-13_11:47:16.200813457_+0100_CET_m=+0.030162273
go.mondoo.com/cnquery/v10/llx ‑ TestTruthy/2024-02-13_11:47:16.200813457_+0100_CET_m=+0.030162273
go.mondoo.com/cnquery/v10/llx ‑ TestTruthy/2024-02-13_11:47:16.200813457_+0100_CET_m=+0.030162273#01

♻️ This comment has been updated with latest results.

czunker · 2024-02-12T04:57:46Z

providers/os/connection/docker_container.go

 		var img v1.Image
-		var rc io.ReadCloser
 		loadedImage := false
 		if asset.Connections[0].Options != nil {
 			if _, ok := asset.Connections[0].Options[COMPRESSED_IMAGE]; ok {


I cannot find the place where this is set. Previously, it was set in L212.

czunker · 2024-02-12T04:59:32Z

I got errors when running this with a registry scan.

cnspec scan container registry gcr.io/mondoo-dev-262313/ubuntu 
→ loaded configuration from /home/christian/workspace/mondoo/sa-edge-default.json using source --config
→ using service account credentials
→ discover related assets for 1 asset(s)
x tar> could not fetch tar file error="seek /tmp/mondoo.inspection972519126: file already closed"
→ fetch meta information from container registry registry=gcr.io/mondoo-dev-262313/ubuntu
→ resolved image image=gcr.io/mondoo-dev-262313/ubuntu@sha256:e5dd9dbb37df5b731a6688fa49f4003359f6f126958c9c928f937bec69836320 name=gcr.io/mondoo-dev-262313/ubuntu@e5dd9dbb37df
→ resolved image image=gcr.io/mondoo-dev-262313/ubuntu@sha256:a98d9dcf3a34b2b78e78c61d003eb4d7a90d30fd54451686e2f0bd2ef5f602ac name=gcr.io/mondoo-dev-262313/ubuntu@a98d9dcf3a34
→ resolved image image=gcr.io/mondoo-dev-262313/ubuntu@sha256:0925d086715714114c1988f7c947db94064fd385e171a63c07730f1fa014e6f9 name=gcr.io/mondoo-dev-262313/ubuntu@0925d0867157
→ resolved image image=gcr.io/mondoo-dev-262313/ubuntu@sha256:61844ceb1dd55aa110ca578bd4a042200bc64bb5d702c9a19b9fb90409565da0 name=gcr.io/mondoo-dev-262313/ubuntu@61844ceb1dd5
→ synchronize assets





 gcr.io/mondoo-dev-262313/ubuntu@0925d0867157 ─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────   0%
 gcr.io/mondoo-dev-262313/ubuntu@0925d0867157 ─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────  n/a score: U
 gcr.io/mondoo-dev-262313/ubuntu@61844ceb1dd5 ─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────   0%
 gcr.io/mondoo-dev-262313/ubuntu@61844ceb1dd5 ─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────  n/a score: U
 0/4 scanned 3/4 n/a                          ─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────   0%
 0/4 scanned 4/4 n/a                          ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 100%
x tar> could not fetch tar file error="seek /tmp/mondoo.inspection854299611: file already closed"

czunker · 2024-02-12T05:01:48Z

The same for a single image:

cnspec scan container registry gcr.io/mondoo-dev-262313/ubuntu@sha256:e5dd9dbb37df5b731a6688fa49f4003359f6f126958c9c928f937bec69836320 --verbose              INT ✘ │ 05:59:52 
DBG using provider os with connector container
DBG Started a new runtime (2 total)
DBG no need to update provider last-refresh=7m15.141729554s provider=os
DBG Log level set to debug
DBG Started a new runtime (3 total)
→ loaded configuration from /etc/opt/mondoo/mondoo.yml using source default
→ using service account credentials
DBG Started a new runtime (4 total)
DBG initialize client authentication issuer=mondoo/ams kid=//agents.api.mondoo.app/spaces/dazzling-golick-767384/serviceaccounts/2AyMRoi8pfJa6gOqq16QWstmKPK
→ discover related assets for 1 asset(s)
DBG Started a new runtime (5 total)
DBG found valid container registry reference ref=gcr.io/mondoo-dev-262313/ubuntu@sha256:e5dd9dbb37df5b731a6688fa49f4003359f6f126958c9c928f937bec69836320
DBG initialize client authentication issuer=mondoo/ams kid=//agents.api.mondoo.app/spaces/dazzling-golick-767384/serviceaccounts/2AyMRoi8pfJa6gOqq16QWstmKPK
DBG tar> extracted image to temporary file
x tar> could not fetch tar file error="seek /tmp/mondoo.inspection3322352812: file already closed"

imilchev · 2024-02-12T11:15:28Z

I made a mistake in the code. I fixed it now and it seems to be working as expected. Also rebased on main to get the latest fixes.

@czunker please re-test

czunker · 2024-02-12T14:13:09Z

I tried both the registry and a single image. Both worked and also cleaned up the images.

But I'm not sure whether this is working as intended. When scanning a single image, it still removes two temporary files at the end:

DBG tar> remove temporary tar file on connection close tar=/tmp/mondoo.inspection1895662558
DBG tar> remove temporary tar file on connection close tar=/tmp/mondoo.inspection1501979699

From the PR description, I'd only expected one such message.

imilchev · 2024-02-12T16:27:07Z

I tried both the registry and a single image. Both worked and also cleaned up the images.

But I'm not sure whether this is working as intended. When scanning a single image, it still removes two temporary files at the end:
DBG tar> remove temporary tar file on connection close tar=/tmp/mondoo.inspection1895662558
DBG tar> remove temporary tar file on connection close tar=/tmp/mondoo.inspection1501979699
From the PR description, I'd only expected one such message.

this is intended. This is the first step in fixing the issue. With this change we split off the image pull and we only pull it if we try to do anything with it (not before that). Currently, we try to detect the OS during discovery, which triggers the pulling of the image (because we resolve the OS by looking through the filesystem of the container.

After this PR is in we will make sure OS isn't detected during discovery. This will be a special behaviour of scanning containers. The OS will be filled in only during a scan, not during discovery. With that approach, we will be pulling the container at scan time and clean it up directly after the scan.

One caveat in that approach is that for container images we would have to call SynchronizeAssets after the scan is done

imilchev · 2024-02-12T18:04:34Z

providers/os/connection/docker_container.go

 		loadedImage := false
 		if asset.Connections[0].Options != nil {
 			if _, ok := asset.Connections[0].Options[COMPRESSED_IMAGE]; ok {
+				var rc io.ReadCloser
 				// read image from disk
 				img, rc, err = image.LoadImageFromDisk(asset.Connections[0].Options[COMPRESSED_IMAGE])


it's set right here

Signed-off-by: Ivan Milchev <[email protected]>

czunker

LGTM. Thanks @imilchev

Signed-off-by: Ivan Milchev <[email protected]>

czunker reviewed Feb 12, 2024

View reviewed changes

imilchev force-pushed the ivan/split-container-loading branch from 69e89b1 to f40630d Compare February 12, 2024 11:15

imilchev commented Feb 12, 2024

View reviewed changes

imilchev added 6 commits February 13, 2024 11:45

🧹 split loading container images from opening connections

a2980fb

Signed-off-by: Ivan Milchev <[email protected]>

fix log levels

75a1cc5

Signed-off-by: Ivan Milchev <[email protected]>

fix linter

3f35102

Signed-off-by: Ivan Milchev <[email protected]>

more linter fixes

79b6766

Signed-off-by: Ivan Milchev <[email protected]>

fix container pull

ae2e27f

Signed-off-by: Ivan Milchev <[email protected]>

remove logic for compressed image

e4ae430

Signed-off-by: Ivan Milchev <[email protected]>

imilchev force-pushed the ivan/split-container-loading branch from e1aeff4 to e4ae430 Compare February 13, 2024 10:45

czunker approved these changes Feb 13, 2024

View reviewed changes

commit change core manifest

7d1d2a5

Signed-off-by: Ivan Milchev <[email protected]>

imilchev merged commit 97a7fc5 into main Feb 13, 2024
5 checks passed

imilchev deleted the ivan/split-container-loading branch February 13, 2024 11:01

github-actions bot locked and limited conversation to collaborators Feb 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🧹 split loading container images from opening connections #3237

🧹 split loading container images from opening connections #3237

imilchev commented Feb 7, 2024 •

edited

Loading

github-actions bot commented Feb 7, 2024 •

edited

Loading

czunker Feb 12, 2024

czunker commented Feb 12, 2024

czunker commented Feb 12, 2024

imilchev commented Feb 12, 2024 •

edited

Loading

czunker commented Feb 12, 2024

imilchev commented Feb 12, 2024

imilchev Feb 12, 2024

czunker left a comment

🧹 split loading container images from opening connections #3237

🧹 split loading container images from opening connections #3237

Conversation

imilchev commented Feb 7, 2024 • edited Loading

github-actions bot commented Feb 7, 2024 • edited Loading

Test Results

czunker Feb 12, 2024

Choose a reason for hiding this comment

czunker commented Feb 12, 2024

czunker commented Feb 12, 2024

imilchev commented Feb 12, 2024 • edited Loading

czunker commented Feb 12, 2024

imilchev commented Feb 12, 2024

imilchev Feb 12, 2024

Choose a reason for hiding this comment

czunker left a comment

Choose a reason for hiding this comment

imilchev commented Feb 7, 2024 •

edited

Loading

github-actions bot commented Feb 7, 2024 •

edited

Loading

imilchev commented Feb 12, 2024 •

edited

Loading