fix: Don't actually use storeabsolutepath directly#916
Conversation
| for i := range scanResult.Inventory.Packages { | ||
| for i2 := range scanResult.Inventory.Packages[i].Locations { | ||
| scanResult.Inventory.Packages[i].Locations[i2] = | ||
| "/" + scanResult.Inventory.Packages[i].Locations[i2] |
There was a problem hiding this comment.
I thought we also support Windows containers? I assume those don't use "/" for the root
There was a problem hiding this comment.
I don't think we do..., if I understand correctly windows containers are very different from linux containers, and many of the assumptions we made when building container scanning would not work (e.g. all paths being forward slashes). @mleyvajr100 correct me if I'm wrong!
There was a problem hiding this comment.
Hm, if these don't work as expected for Windows containers we should either open a bug and add a TODO linking to it, or state somewhere that we only support Linux containers for now. I'll defer to @mleyvajr100 on whether we currently support these or not
There was a problem hiding this comment.
@another-rex at least for the linux vs windows paths stuff, I though we used the filepath package to deal with windows file paths?
There was a problem hiding this comment.
Based on some looking around last week, it seems like windows containers are indeed different. Even though I think we do correctly handle the paths having backslashes, not 100% sure we can handle windows containers. Happy to document that in our codebase. Wdyt @another-rex ?
There was a problem hiding this comment.
Yeah let's document that we only support linux containers (which is the majority of containers afaik) until we can do more investigation into windows containers. It also requires virtualisation to run unlike linux containers, which makes it a lot harder to test / experiment with.
| // Actually convert it to absolute path here. | ||
| if storeAbsPath { | ||
| for i := range scanResult.Inventory.Packages { | ||
| for i2 := range scanResult.Inventory.Packages[i].Locations { |
There was a problem hiding this comment.
Why use indexes? Why not just for _, pkg := range packages, for _, location := range locations
There was a problem hiding this comment.
I think you need to use indexes to assign to Locations, otherwise you'll just be updating a local variable.
As for Packages I just kept it consistent there, but that does result in these very long lines. I'll update packages to use the other loop
There was a problem hiding this comment.
Ah, fair. Maybe use something like p and l instead of i i2 to make it clear which index is which
There was a problem hiding this comment.
using another iteration variable sounds better to me as Erik suggested
| "/" + scanResult.Inventory.Packages[i].Locations[i2] | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Drive by note: It seems like the entire ScanContainer method is untested. As we add more and more complex behavior like this one to it we should introduce some unit tests for them with a fake FS + chain layers
There was a problem hiding this comment.
Yeah agreed, much of the tests are essentially done through osv-scanner currently, which is not ideal.
There was a problem hiding this comment.
I can add some tests. I remember trying to add some tests a while back but there was some debate about adding them. Don't remember why, but I agree, the ScanContainer function should be tested using the fake layer / chain layers that Rex created.
There was a problem hiding this comment.
Ah I was already working on it, just got distracted and didn't push it 😅. Made #933
There was a problem hiding this comment.
After 933 is merged I'll make a test case for scanning with StoreAbsolutePath so we can test this change.
There was a problem hiding this comment.
Sounds good! #933 looks good to me.
3 participants
Delay storeAbsolutePath to later for container scanning, as it will turn paths into windows paths, which is incorrect and breaks layer tracing.
An alternative to this implementation would be to edit the expandAbsolutePath function to take into account the capability.OS field to expand paths correctly. Maybe that would be a better implementation.