Fix crash on unexpected ova files #1106
Conversation
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #1106 +/- ##
==========================================
- Coverage 15.70% 15.64% -0.06%
==========================================
Files 112 112
Lines 23075 23089 +14
==========================================
- Hits 3623 3612 -11
- Misses 19165 19192 +27
+ Partials 287 285 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
I should probably mention that I don't really see any cases where these 'generic' devices are used by MTV, so I'm not even sure what the goal of this code is. It seems clear that the |
jonner
force-pushed
the
ova-crash-elementname
branch
from
5a9e04d to
85d1a4d
Compare
I was testing an ova file generated by virtualbox and it did not satisfy
the assumptions made within the ova provider code. In particular, not
all devices contained an `ElementName` xml element. Don't crash
when encountering images like this.
Error encountered was as follows:
2024/10/11 17:38:06 http: panic serving 10.217.0.134:34376: runtime error: slice bounds out of range [:-2]
goroutine 7659 [running]:
net/http.(*conn).serve.func1()
GOROOT/src/net/http/server.go:1868 +0xb9
panic({0x25c640?, 0xc0003070b0?})
GOROOT/src/runtime/panic.go:920 +0x270
main.convertToVmStruct({0xc000223c00, 0x3, 0x4?}, {0xc0002aaec0, 0x3, 0x68943e?})
cmd/ova-provider-server/ova-provider-server.go:470 +0x105c
main.vmHandler({0x2f1a48?, 0xc00021c700}, 0xc00014db18?)
cmd/ova-provider-server/ova-provider-server.go:229 +0x7a
net/http.HandlerFunc.ServeHTTP(0x4a6500?, {0x2f1a48?, 0xc00021c700?}, 0x68711a?)
GOROOT/src/net/http/server.go:2136 +0x29
net/http.(*ServeMux).ServeHTTP(0x718080?, {0x2f1a48, 0xc00021c700}, 0xc000200200)
GOROOT/src/net/http/server.go:2514 +0x142
net/http.serverHandler.ServeHTTP({0xc000547a70?}, {0x2f1a48?, 0xc00021c700?}, 0x6?)
GOROOT/src/net/http/server.go:2938 +0x8e
net/http.(*conn).serve(0xc0002021b0, {0x2f2058, 0xc0000ad2c0})
GOROOT/src/net/http/server.go:2009 +0x5f4
created by net/http.(*Server).Serve in goroutine 1
GOROOT/src/net/http/server.go:3086 +0x5cb
Signed-off-by: Jonathon Jongsma <[email protected]>
When encountering an ova file, we iterate throught the devices listed in the <VirtualHardwareSection> of the file and attempt to identify items that are useful to us (notably Network Adapters, memory definition, etc). The remaining items are stored as a list of generic devices with a `Kind` that is a free-form string. It appears that we tried to make the generic devices slightly nicer by stripping off number suffixes from the names of the devices and setting the device `Kind` to e.g. "SCSI Controller" rather than "SCSI Controller 1". But the code assumes that *all* `Items` in this file have the same numeric suffix format for `ElementName`. In reality, the `Items` vary quite a bit. This variation occurs both between different `Items` in the same file, and especially between ova files that are generated by different sources. For example, in an ova file that was generated with vsphere 7.0, the `ElementName` for my video device is simply "Video Card" with no suffix, whereas the Hard Disk has an `ElementName` of "Hard Disk 1". So the ova provider transforms the video card into a generic device with Kind set to the truncated string "Video Ca". In addition, I encountered ova files that were produced by VirtualBox where many of the `Items` in this list did not even contain `ElementName` elements, but did have `Description`s. So to make the ova provider more robust to files that don't follow our current assumptions: - only trim the suffix if the `ElementName` string ends with a space and a digit - if the `ElementName` does not exist at all, use `Description` - If neither of these elements exist, set the `Kind` to "Unknown" Signed-off-by: Jonathon Jongsma <[email protected]>
jonner
force-pushed
the
ova-crash-elementname
branch
from
85d1a4d to
33118b9
Compare
|
I am very confused by this error since my code doesn't seem like it would affect anything relevant to that test failure. I ran the test against this branch in my personal repository and the ova test case passed (https://github.com/jonner/forklift/actions/runs/11370782873), so maybe it's just a test environment issue? The error itself is very generic ("Plan plan-test not ready within 15s") so it's hard to tell exactly what might be causing it... |
I don't think we do, @jonner ? |
|
@yaacov there is no issue filed for this patch. It was just something I uncovered while working on MTV-1577, but the root cause is different than that issue. As I mentioned in the commit logs, I only observed these issues while using an ova file generated by VirtualBox, which is probably not something that we claim to support. Nevertheless, it doesn't hurt to be a bit more defensive to avoid crashes. The test failure issue is still unclear to me as it did pass CI in my personal repository on github... |
|
See descriptions on the individual commits. This just makes the ova provider a bit more robust to unexpected inputs.