diff --git a/docs/index.md b/docs/index.md index 72f00844..d9329a0d 100644 --- a/docs/index.md +++ b/docs/index.md @@ -15,7 +15,7 @@ markdown2extras: tables, code-friendly Copyright 2021 Joyent, Inc. Copyright 2021 The University of Queensland Copyright 2024 MNX Cloud, Inc. - Copyright 2025 Edgecast Cloud LLC. + Copyright 2026 Edgecast Cloud LLC. --> @@ -904,7 +904,7 @@ The section describes API changes in CloudAPI versions. invalid `block_size` on a disk will now report errors rather than silently fail. -# 9.18.0 +## 9.18.0 - Support for ED25519 keys. @@ -5435,6 +5435,7 @@ id | UUID | Unique id for this disk boot | Boolean | If `true`, this is the boot disk image | UUID | The image from which the disk was created size | Integer | The size of the disk in mebibytes or "remaining". If "remaining", size will be set to the difference between the package quota and sum of the other disks. +block_size | Integer | Optional. The disk's block size in bytes. Must be 512-131072 and a power of 2. Cannot be set on an image-backed disk, which includes the boot disk. When not specified, the block size is chosen for you and reported back on subsequent requests. ### Returns @@ -6684,6 +6685,7 @@ dangerous_allow_shrink | Boolean | Optional, whether a disk can reduce size id | String | This disk's UUID pci_slot | String | This disk's PCI slot size | Number | Size in MiB (before resize) +block_size | Number | The disk's block size in bytes boot | Boolean | If this is the VM's boot disk state | String | Current state of disk (i.e. 'resizing') @@ -6754,6 +6756,7 @@ Fetch a specific disk on a bhyve VM. id | String | This disk's UUID pci_slot | String | This disk's PCI slot size | Number | Size in MiB +block_size | Number | The disk's block size in bytes boot | Boolean | If this is the VM's boot disk state | String | Current state of disk @@ -6820,6 +6823,7 @@ List all disk on a bhyve VM. id | String | This disk's UUID pci_slot | String | This disk's PCI slot size | Number | Size in MiB +block_size | Number | The disk's block size in bytes boot | Boolean | If this is the VM's boot disk state | String | Current state of disk diff --git a/lib/endpoints/disks.js b/lib/endpoints/disks.js index 9e1951c3..51731a32 100644 --- a/lib/endpoints/disks.js +++ b/lib/endpoints/disks.js @@ -7,6 +7,7 @@ /* * Copyright 2020 Joyent, Inc. * Copyright 2024 MNX Cloud, Inc. + * Copyright 2026 Edgecast Cloud LLC. */ /* @@ -20,7 +21,6 @@ var util = require('util'), var assert = require('assert-plus'); var restify = require('restify'); var string2uuid = require('uuid-by-string'); -var diskValidation = require('../validation/disk'); // --- Globals @@ -58,7 +58,7 @@ function createDisk(req, res, next) { req.vm.disks = req.vm.disks || []; if (req.vm.brand !== 'bhyve') { - next(InvalidArgumentError('Disk Creation is supported only for ' + + next(new InvalidArgumentError('Disk Creation is supported only for ' + 'BHYVE VMs')); return; } @@ -89,24 +89,6 @@ function createDisk(req, res, next) { return; } - // Check requested block size, if any. - req.vm.disks.forEach(function vrs(disk, i) { - if (disk.hasOwnProperty('block_size')) { - if (disk.hasOwnProperty('image_uuid')) { - throw new InvalidArgumentError( - 'Cannot set block_size and image_uuid.' - ); - } - if (!diskValidation.validRecordSize(disk.block_size)) { - throw new InvalidArgumentError( - 'Invalid block_size: ' + disk.block_size + - ' on disk: ' + i + '. Must be 512-131072 and ' + - 'a power of 2.' - ); - } - } - }); - if (pciSlot) { var diskId = getDiskUuid(vmUuid, pciSlot); } @@ -665,5 +647,6 @@ function mount(server, before, pre, post) { module.exports = { + _createDisk: createDisk, // exported for testing only mount: mount }; diff --git a/lib/validation/disk.js b/lib/validation/disk.js index cfc99c8d..f366f331 100644 --- a/lib/validation/disk.js +++ b/lib/validation/disk.js @@ -6,18 +6,37 @@ /* * Copyright 2024 MNX Cloud, Inc. + * Copyright 2026 Edgecast Cloud LLC. */ -// This is the same function from vmadm. +/* + * This mirrors vmadm, which validates disks.*.block_size as an integer (a + * string that parses cleanly as one is accepted too) before applying the range + * and power-of-2 checks below. Without the type check those comparisons coerce + * non-numbers to NaN, which is false against every bound, so "garbage", {} and + * 4096.5 would all be reported as valid. + */ function validRecordSize(candidate) { - if (candidate < 512) { + var size; + + if (typeof (candidate) !== 'number' && typeof (candidate) !== 'string') { + return (false); + } + + size = Number(candidate); + + if (!Number.isInteger(size)) { + return (false); + } + + if (size < 512) { // too low return (false); - } else if (candidate > 131072) { + } else if (size > 131072) { // too high return (false); - } else if ((candidate & (candidate - 1)) !== 0) { + } else if ((size & (size - 1)) !== 0) { // not a power of 2 return (false); } diff --git a/test/createDisk.test.js b/test/createDisk.test.js new file mode 100644 index 00000000..07964685 --- /dev/null +++ b/test/createDisk.test.js @@ -0,0 +1,220 @@ +/* + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. + */ + +/* + * Copyright 2026 Edgecast Cloud LLC. + */ + +/* + * Unit tests for the CreateMachineDisk handler. These need no live datacenter: + * the VM is supplied directly and vmapi.createDisk() is stubbed. + */ + +var test = require('tape'); + +var createDisk = require('../lib/endpoints/disks')._createDisk; + +// --- Globals + +var IMAGE_UUID = 'ede15ae3-a2ed-4636-a4d5-c130cbd9c297'; +var OWNER_UUID = '7b315468-c6be-46dc-b99b-9c1f59224693'; +var VM_UUID = '3fdfbfe8-1c98-4d0e-b95b-1a4d5f8b7d3a'; +var LOGIN = 'bob'; + +// --- Helpers + +/* + * An image-backed bhyve boot disk as vmadm actually reports it. block_size is + * read back from the zvol's volblocksize, so it is always present, including + * alongside image_uuid. See TritonDataCenter/sdc-cloudapi#156. + */ +function bootDisk() { + return { + boot: true, + block_size: 8192, + image_uuid: IMAGE_UUID, + pci_slot: '0:4:0', + size: 10240 + }; +} + +function mkReq(opts) { + var created = []; + + return { + created: created, + req: { + _auditCtx: { caller: LOGIN }, + account: { login: LOGIN, uuid: OWNER_UUID }, + getId: function getId() { return 'test-request-id'; }, + log: { debug: function debug() {} }, + params: opts.params, + sdc: { + vmapi: { + createDisk: function stubCreateDisk(params, _opts, cb) { + created.push(params); + setImmediate(cb, null, { job_uuid: 'test-job' }); + } + } + }, + vm: opts.vm + } + }; +} + +function mkRes() { + var res = { headers: {} }; + + res.header = function header(name, value) { + res.headers[name] = value; + }; + + res.send = function send(body) { + res.body = body; + }; + + return res; +} + +function stoppedBhyveVm(disks) { + return { + brand: 'bhyve', + disks: disks, + flexible_disk_size: 102400, + state: 'stopped', + uuid: VM_UUID + }; +} + +// --- Tests + +/* + * The regression from TRITON-2459: createDisk validated the VM's persisted + * disks rather than the requested one, so every image-backed bhyve boot disk + * tripped the "Cannot set block_size and image_uuid" check. The throw was + * synchronous, so it escaped to cloudapi's uncaughtException handler and the + * caller saw a 500 InternalError. + */ +test('CreateMachineDisk accepts a VM with an image-backed boot disk', + function (t) { + var ctx = mkReq({ + params: { pci_slot: '0:4:4', size: 20480 }, + vm: stoppedBhyveVm([ + bootDisk(), + { block_size: 4096, pci_slot: '0:4:1', size: 10240 } + ]) + }); + var res = mkRes(); + + createDisk(ctx.req, res, function next(err) { + t.ifError(err, 'err'); + + t.equal(ctx.created.length, 1, 'vmapi.createDisk() called once'); + + var args = ctx.created[0]; + t.equal(args.uuid, VM_UUID, 'vm uuid'); + t.equal(args.owner_uuid, OWNER_UUID, 'owner uuid'); + t.equal(args.pci_slot, '0:4:4', 'pci_slot'); + t.equal(args.size, 20480, 'size'); + t.equal(args.origin, 'cloudapi', 'origin'); + t.ok(!args.hasOwnProperty('block_size'), + 'block_size not sent to vmapi'); + + t.ok(res.body, 'response body'); + t.equal(res.body.pci_slot, '0:4:4', 'response pci_slot'); + t.equal(res.body.size, 20480, 'response size'); + t.equal(res.body.state, 'creating', 'response state'); + t.equal(res.body.boot, false, 'response boot'); + t.equal(res.headers.Location, + '/' + LOGIN + '/machines/' + VM_UUID + '/disks/' + res.body.id, + 'Location header'); + + t.end(); + }); +}); + + +/* + * block_size on an existing disk describes the zvol as it already is. Whether + * it is a value cloudapi would accept on a create request is irrelevant here, + * and must not block adding an unrelated disk. + */ +test('CreateMachineDisk does not validate existing disks', function (t) { + var ctx = mkReq({ + params: { pci_slot: '0:4:4', size: 20480 }, + vm: stoppedBhyveVm([{ block_size: 1234, pci_slot: '0:4:0', + size: 10240 }]) + }); + + createDisk(ctx.req, mkRes(), function next(err) { + t.ifError(err, 'err'); + t.equal(ctx.created.length, 1, 'vmapi.createDisk() called once'); + t.end(); + }); +}); + + +test('CreateMachineDisk with size "remaining"', function (t) { + var ctx = mkReq({ + params: { pci_slot: '0:4:4', size: 'remaining' }, + vm: stoppedBhyveVm([bootDisk(), { pci_slot: '0:4:1', size: 512 }]) + }); + + createDisk(ctx.req, mkRes(), function next(err) { + t.ifError(err, 'err'); + t.equal(ctx.created[0].size, 102400 - 10240 - 512, 'remaining size'); + t.end(); + }); +}); + + +test('CreateMachineDisk without pci_slot returns 202', function (t) { + var ctx = mkReq({ + params: { size: 20480 }, + vm: stoppedBhyveVm([bootDisk()]) + }); + var res = mkRes(); + + createDisk(ctx.req, res, function next(err) { + t.ifError(err, 'err'); + t.equal(ctx.created.length, 1, 'vmapi.createDisk() called once'); + t.equal(res.body, 202, 'status 202'); + t.end(); + }); +}); + + +test('CreateMachineDisk rejects non-bhyve VMs', function (t) { + var ctx = mkReq({ + params: { pci_slot: '0:4:4', size: 20480 }, + vm: { brand: 'joyent', disks: [], state: 'stopped', uuid: VM_UUID } + }); + + createDisk(ctx.req, mkRes(), function next(err) { + t.ok(err, 'err'); + t.equal(err.body.code, 'InvalidArgument', 'err code'); + t.equal(ctx.created.length, 0, 'vmapi.createDisk() not called'); + t.end(); + }); +}); + + +test('CreateMachineDisk rejects a running VM', function (t) { + var vm = stoppedBhyveVm([bootDisk()]); + vm.state = 'running'; + + var ctx = mkReq({ + params: { pci_slot: '0:4:4', size: 20480 }, + vm: vm + }); + + createDisk(ctx.req, mkRes(), function next(err) { + t.ok(err, 'err'); + t.equal(err.body.code, 'InvalidArgument', 'err code'); + t.equal(ctx.created.length, 0, 'vmapi.createDisk() not called'); + t.end(); + }); +}); diff --git a/test/machines.94.test.js b/test/machines.94.test.js index bc2c3319..14b3d804 100644 --- a/test/machines.94.test.js +++ b/test/machines.94.test.js @@ -6,6 +6,7 @@ /* * Copyright 2020 Joyent, Inc. + * Copyright 2026 Edgecast Cloud LLC. */ var test = require('tape'); @@ -13,6 +14,7 @@ var util = require('util'); var vasync = require('vasync'); var common = require('./common'); +var diskValidation = require('../lib/validation/disk'); var machinesCommon = require('./machines/common'); var checkMachine = machinesCommon.checkMachine; var deleteMachine = require('./machines/delete'); @@ -79,6 +81,31 @@ function checkDisks(t, expectedDisks, actualDisks) { }); } +/* + * block_size is reported back from the zvol's volblocksize, which varies with + * the pool's layout, so it can only be checked for validity and not against a + * fixed value. Everything else must match exactly. + */ +function checkDiskEqual(t, actualDisk, expectedDisk, msg) { + var disk = Object.assign({}, actualDisk); + + if (disk.hasOwnProperty('block_size')) { + t.ok(diskValidation.validRecordSize(disk.block_size), + 'valid block_size: ' + disk.block_size); + delete disk.block_size; + } + + t.deepEqual(disk, expectedDisk, msg); +} + +function checkDisksEqual(t, actualDisks, expectedDisks, msg) { + t.equal(actualDisks.length, expectedDisks.length, 'disk count'); + + actualDisks.forEach(function check(disk, idx) { + checkDiskEqual(t, disk, expectedDisks[idx], msg); + }); +} + function checkDisksQuota(t, disks, quota) { var disksSum = disks.reduce(function sumDisk(sum, disk) { return sum + disk.size; @@ -440,7 +467,7 @@ test('No disks/inflexible disk package', function (suite) { CLIENT.get(diskPath, function gotDisk(err, req, res, disk) { t.ifError(err); - t.deepEqual(disk, { + checkDiskEqual(t, disk, { id: DISK_UUID, boot: false, pci_slot: '0:4:1', @@ -717,7 +744,7 @@ test('Disks/flexible disk package', function (suite) { t.strictEqual(body.flexible, true); checkDisksQuota(t, body.disks, BHYVE_128_FLEXIBLE.quota - body.free_space); - t.deepEqual(body.disks, expectedDisks); + checkDisksEqual(t, body.disks, expectedDisks); t.end(); }); }); @@ -764,7 +791,7 @@ test('Disks/flexible disk package', function (suite) { }, function resizeDisk(err, req, res, disk) { t.ifError(err, 'err'); - t.deepEqual(disk, { + checkDiskEqual(t, disk, { id: DISK_UUID, pci_slot: '0:4:1', size: 512, @@ -793,7 +820,7 @@ test('Disks/flexible disk package', function (suite) { return; } - t.deepEqual(disk, { + checkDiskEqual(t, disk, { id: DISK_UUID, pci_slot: '0:4:1', size: 128, @@ -844,7 +871,7 @@ test('Disks/flexible disk package', function (suite) { } t.ifError(err, 'err'); - t.deepEqual(disk, { + checkDiskEqual(t, disk, { id: DISK_UUID, pci_slot: '0:4:1', size: 128, @@ -901,7 +928,7 @@ test('Disks/flexible disk package', function (suite) { t.ifError(err, 'err'); - t.deepEqual(disk, { + checkDiskEqual(t, disk, { id: DISK_UUID, pci_slot: '0:4:4', size: 256, @@ -984,7 +1011,7 @@ test('Disks/flexible disk package', function (suite) { t.ifError(err, 'err'); if (count === 1) { - t.deepEqual(disk, { + checkDiskEqual(t, disk, { id: DISK_UUID, pci_slot: '0:4:2', size: BHYVE_128_FLEXIBLE.quota - @@ -1096,7 +1123,7 @@ test('Disks sum to quota/flex disk package', function (suite) { t.strictEqual(body.flexible, true); checkDisksQuota(t, body.disks, BHYVE_128_FLEXIBLE.quota); - t.deepEqual(body.disks, expectedDisks); + checkDisksEqual(t, body.disks, expectedDisks); t.end(); }); }); @@ -1203,7 +1230,7 @@ test('Disks with remaining/flex disk package', function (suite) { t.strictEqual(body.flexible, true); checkDisksQuota(t, body.disks, BHYVE_128_FLEXIBLE.quota - body.free_space); - t.deepEqual(body.disks, expectedDisks); + checkDisksEqual(t, body.disks, expectedDisks); t.end(); }); }); diff --git a/test/validationDisk.test.js b/test/validationDisk.test.js new file mode 100644 index 00000000..231684d0 --- /dev/null +++ b/test/validationDisk.test.js @@ -0,0 +1,68 @@ +/* + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. + */ + +/* + * Copyright 2026 Edgecast Cloud LLC. + */ + +var test = require('tape'); + +var validRecordSize = require('../lib/validation/disk').validRecordSize; + +// --- Tests + +test('validRecordSize accepts the supported range', function (t) { + [512, 1024, 4096, 8192, 16384, 65536, 131072].forEach(function chk(size) { + t.ok(validRecordSize(size), size + ' is valid'); + }); + + t.end(); +}); + + +test('validRecordSize accepts integer strings', function (t) { + t.ok(validRecordSize('8192'), '"8192" is valid'); + t.notOk(validRecordSize('8193'), '"8193" is not a power of 2'); + t.end(); +}); + + +test('validRecordSize rejects out-of-range and non-power-of-2', function (t) { + [0, 511, 1000, 8193, 131073, 262144, -8192].forEach(function chk(size) { + t.notOk(validRecordSize(size), size + ' is invalid'); + }); + + t.end(); +}); + + +/* + * Without a type check the range comparisons coerce these to NaN, which is + * false against every bound, and (NaN & NaN) is 0, so each one used to be + * reported as a valid record size. + */ +test('validRecordSize rejects non-integers', function (t) { + var cases = [ + ['4096.5', 4096.5], + ['"garbage"', 'garbage'], + ['{}', {}], + ['[]', []], + ['[8192]', [8192]], + ['null', null], + ['undefined', undefined], + ['NaN', NaN], + ['Infinity', Infinity], + ['true', true], + ['false', false], + ['empty string', ''] + ]; + + cases.forEach(function chk(c) { + t.notOk(validRecordSize(c[1]), c[0] + ' is invalid'); + }); + + t.end(); +});