🐛 Set container image platform before calling SynchronizeAssets #172
Conversation
|
I do not think we should do that approach if we do not cache the image |
policy/scan/local_scanner.go
Outdated
| // TODO: this is a temporary fix for populating the platform information for assets that only set it | ||
| // during a scan, e.g. container images. This should be removed once we start using MQL for retrieving | ||
| // platform information. | ||
| if err := setPlatforms(ctx, assetList, im, job.DoRecord); err != nil { |
There was a problem hiding this comment.
this is problematic. Opening a connection can be very expensive. I think it would be better if you opened the connection, get the platform id, call sync asset, then scan the asset.
I think ebs scanning is one of the things that will suffer from this. cc @vjeffrey
There was a problem hiding this comment.
I could do that. The only downside is that there will be 2 code paths that do asset synchronization
There was a problem hiding this comment.
i'm not sure i understand why there would be 2
There was a problem hiding this comment.
I changed it. Not sure if that is exactly what you had in mind
There was a problem hiding this comment.
what is the original problem this is all fixing? is it that some assets do not get platform information until they are scanned?
There was a problem hiding this comment.
@vjeffrey yup, previously we had a SyncAssets call after the scan was done that was populating that info (pre-v7). Thats no longer there
There was a problem hiding this comment.
i think i'm still unclear, bc there also other cases where we don't have the platform info, right? e.g. aws with ec2 discovery, we likely won't be able to connect directly to all the instances, so a bunch of them won't have the platform info unless we scan the instance directly.
There was a problem hiding this comment.
This is not the latest change. In the latest change the assets which platform changes are being sync-ed
There was a problem hiding this comment.
👍 got it, so basically anything that doesn't get platform information during the first sync call never gets its platform information in the console, because we're no longer calling sync/set asset as part of the scan path.
just tested this with ebs volume scanning, it definitely suffers from the same issue. a lot of stuff is going to suffer from this (ec2 instances that are discovered along with an aws account, for example)
|
If we go with this route, we also need to fix it for cnquery scan command |
|
we had a mtg about this. current idea:
|
|
I updated the code according to what we discussed. Also opened a follow-up issue for looking into batching |
Signed-off-by: Ivan Milchev <ivan@mondoo.com>
Signed-off-by: Ivan Milchev <ivan@mondoo.com>
Signed-off-by: Ivan Milchev <ivan@mondoo.com>
Signed-off-by: Ivan Milchev <ivan@mondoo.com>
Signed-off-by: Ivan Milchev <ivan@mondoo.com>
imilchev
force-pushed
the
ivan/set-container-platform
branch
from
8b72b06
to
b4fb894
Compare
Signed-off-by: Ivan Milchev <ivan@mondoo.com>
imilchev
force-pushed
the
ivan/set-container-platform
branch
from
b4fb894
to
7ec9c8f
Compare
Discussed with @preslavgerchev and @jaym and we agreed that we should add a temporary fix for this problem until we have a more long term solution. Please see if you can come up with any other type of asset that might have the same issue so I can add it to the function that fixes the platform information