diff --git a/@xen-orchestra/acl/src/actions/network.mts b/@xen-orchestra/acl/src/actions/network.mts index 25fe6bd481..4b8efe1b19 100644 --- a/@xen-orchestra/acl/src/actions/network.mts +++ b/@xen-orchestra/acl/src/actions/network.mts @@ -4,5 +4,6 @@ export default { read: true, update: { tags: true, + other_config: true, }, } diff --git a/@xen-orchestra/acl/src/actions/vif.mts b/@xen-orchestra/acl/src/actions/vif.mts index 4bdd1c4396..96a8375231 100644 --- a/@xen-orchestra/acl/src/actions/vif.mts +++ b/@xen-orchestra/acl/src/actions/vif.mts @@ -10,5 +10,6 @@ export default { lockingMode: true, rateLimit: true, txChecksumming: true, + other_config: true, }, } diff --git a/@xen-orchestra/rest-api/src/open-api/schema/build-openapi-schema.mts b/@xen-orchestra/rest-api/src/open-api/schema/build-openapi-schema.mts index 546fa20fe0..362e093155 100644 --- a/@xen-orchestra/rest-api/src/open-api/schema/build-openapi-schema.mts +++ b/@xen-orchestra/rest-api/src/open-api/schema/build-openapi-schema.mts @@ -16,11 +16,16 @@ export function buildOpenApiSchema(def: Record): OpenAP if (field.type === 'enum') { property.type = 'string' property.enum = field.enum + } else if (field.type === 'object') { + property.type = 'object' + const nested = buildOpenApiSchema(field.fields) + property.properties = nested.properties + if (nested.required?.length) property.required = nested.required } else { property.type = field.type } - if (field.example !== undefined) { + if ('example' in field && field.example !== undefined) { property.example = field.example } diff --git a/@xen-orchestra/rest-api/src/router/external-router.mts b/@xen-orchestra/rest-api/src/router/external-router.mts index 5a92f68d04..1ee794f9ae 100644 --- a/@xen-orchestra/rest-api/src/router/external-router.mts +++ b/@xen-orchestra/rest-api/src/router/external-router.mts @@ -278,11 +278,14 @@ function buildZodSchema(def: Record): z.ZodObject + optional?: boolean + } -export type ParamFieldDefinition = Exclude +export type ParamFieldDefinition = Exclude + +export type QueryFieldDefinition = Exclude export interface RouteDefinition { method: 'get' | 'post' | 'put' | 'delete' | 'patch' @@ -57,7 +64,7 @@ export interface RouteDefinition { description?: string tags?: string[] params?: Record - query?: Record + query?: Record body?: Record responses?: Array<{ status: number diff --git a/CHANGELOG.unreleased.md b/CHANGELOG.unreleased.md index 21823397a3..28f51957e2 100644 --- a/CHANGELOG.unreleased.md +++ b/CHANGELOG.unreleased.md @@ -12,6 +12,7 @@ > Users must be able to say: "Nice enhancement, I'm eager to test it" - [REST API] Expose `GET /backup-repositories/:id/health` and `POST /backup-repositories/:id/actions/benchmark` routes (PR [#9847](https://github.com/vatesfr/xen-orchestra/pull/9847)) +- [sdn-controller] Add `POST /rest/v0/plugins/sdn-controller/networks/:id/actions/update_traffic_rule` and `POST /rest/v0/plugins/sdn-controller/vifs/:id/actions/update_traffic_rule` (PR [#9936](https://github.com/vatesfr/xen-orchestra/pull/9936)) ### Bug fixes @@ -34,12 +35,13 @@ > Keep this list alphabetically ordered to avoid merge conflicts - - @vates/types minor - @xen-orchestra/acl minor - @xen-orchestra/backups patch - @xen-orchestra/proxy minor - @xen-orchestra/rest-api minor - xo-server minor +- xo-server-sdn-controller minor + diff --git a/docs/docs/xo6/acl-v2.md b/docs/docs/xo6/acl-v2.md index 65f036a0ad..d7cc0085d1 100644 --- a/docs/docs/xo6/acl-v2.md +++ b/docs/docs/xo6/acl-v2.md @@ -116,12 +116,12 @@ Actions are written using the exact string you pass in a privilege. A parent act | `vdi` | `read`, `create`, `delete`, `boot`, `export-content`, `import-content`, `migrate-send`, `update:tags` | | `vdi-snapshot` | `read`, `delete`, `export`, `update:tags` | | `vdi-unmanaged` | `read` | -| `vif` | `connect`, `create`, `delete`, `disconnect`, `read`, `update:allowedIpv4Addresses`, `update:allowedIpv6Addresses`, `update:lockingMode`, `update:rateLimit`, `update:txChecksumming` | +| `vif` | `connect`, `create`, `delete`, `disconnect`, `read`, `update:allowedIpv4Addresses`, `update:allowedIpv6Addresses`, `update:lockingMode`, `update:rateLimit`, `update:txChecksumming`,`update:other_config` | | `vbd` | `read`, `create`, `delete`, `connect`, `disconnect` | | `sr` | `read`, `delete`, `forget`, `migrate-receive`, `reclaim-space`, `scan`, `import:vdi`, `import:vm`, `update:tags` | | `host` | `read`, `allow-vm`, `join-pool`, `export:logs`,`migrate-receive`, `update:tags`, `disable`, `enable`, `evacuate` | | `pool` | `add-host`, `read`, `emergency-shutdown`, `rolling-reboot`, `rolling-update`, `create:network`, `create:vm`, `update:tags` | -| `network` | `read`, `create`, `delete`, `update:tags` | +| `network` | `read`, `create`, `delete`, `update:tags`,`update:other_config` | | `pif` | `read`, `update:management` | | `pbd` | `read`, `plug`, `unplug` | | `pci` | `read` | diff --git a/packages/xo-server-sdn-controller/package.json b/packages/xo-server-sdn-controller/package.json index 7788083f6e..3ea2001181 100644 --- a/packages/xo-server-sdn-controller/package.json +++ b/packages/xo-server-sdn-controller/package.json @@ -30,6 +30,7 @@ "dependencies": { "@vates/async-each": "^1.0.3", "@vates/coalesce-calls": "^0.1.0", + "@vates/types": "^1.26.0", "@xen-orchestra/log": "^0.7.2", "@xen-orchestra/openflow": "^0.1.4", "ipaddr.js": "^2.0.1", diff --git a/packages/xo-server-sdn-controller/src/index.js b/packages/xo-server-sdn-controller/src/index.js index f1399de996..9ace127454 100644 --- a/packages/xo-server-sdn-controller/src/index.js +++ b/packages/xo-server-sdn-controller/src/index.js @@ -14,8 +14,8 @@ import { PrivateNetwork } from './private-network/private-network' import { TlsHelper } from './utils/tls-helper' import { instantiateController } from './openflow-controller' import { randomBytes } from 'crypto' -import { invalidParameters } from 'xo-common/api-errors.js' - +import { invalidParameters, noSuchObject } from 'xo-common/api-errors.js' +import { SDN_CONTROLLER_OF_RULES_KEY } from '@vates/types' // ============================================================================= const log = createLogger('xo:xo-server-sdn-controller') @@ -255,6 +255,28 @@ function isControllerNeeded(xapi) { return !(controller?.protocol === PROTOCOL && controller.address === '' && controller.port === 0) } +function validateRuleFields(obj, errors, prefix = '') { + if (!obj.direction || typeof obj.direction !== 'string') { + errors.push(`${prefix}direction is required and must be a string`) + } + if (!obj.ipRange || typeof obj.ipRange !== 'string') { + errors.push(`${prefix}ipRange is required and must be a string`) + } + if (!obj.protocol || typeof obj.protocol !== 'string') { + errors.push(`${prefix}protocol is required and must be a string`) + } + if (obj.port != null && !Number.isInteger(obj.port)) { + errors.push(`${prefix}port must be an integer`) + } +} + +function validateRuleWithAllow(obj, errors, prefix = '') { + if (obj.allow === undefined || typeof obj.allow !== 'boolean') { + errors.push(`${prefix}allow is required and must be a boolean`) + } + validateRuleFields(obj, errors, prefix) +} + // ============================================================================= class SDNController extends EventEmitter { @@ -466,25 +488,7 @@ class SDNController extends EventEmitter { ':id/actions/add_traffic_rule': { _post: async (req, res, next) => { const validationErrors = [] - - if (req.body.allow === undefined || typeof req.body.allow !== 'boolean') { - validationErrors.push('allow is required and must be a boolean') - } - - if (!req.body.direction || typeof req.body.direction !== 'string') { - validationErrors.push('direction is required and must be a string') - } - - if (!req.body.ipRange || typeof req.body.ipRange !== 'string') { - validationErrors.push('ipRange is required and must be a string') - } - - if (!req.body.protocol || typeof req.body.protocol !== 'string') { - validationErrors.push('protocol is required and must be a string') - } - if (req.body.port && Number.isInteger(req.body.port) === false) { - validationErrors.push('port must be an integer') - } + validateRuleWithAllow(req.body, validationErrors) if (validationErrors.length > 0) { throw invalidParameters(validationErrors) } @@ -497,7 +501,7 @@ class SDNController extends EventEmitter { networkId: req.params.id, } - if (req.body.port) { + if (req.body.port != null) { rule.port = req.body.port } @@ -514,22 +518,7 @@ class SDNController extends EventEmitter { ':id/actions/delete_traffic_rule': { _post: async (req, res, next) => { const validationErrors = [] - - if (!req.body.direction || typeof req.body.direction !== 'string') { - validationErrors.push('direction is required and must be a string') - } - - if (!req.body.ipRange || typeof req.body.ipRange !== 'string') { - validationErrors.push('ipRange is required and must be a string') - } - - if (!req.body.protocol || typeof req.body.protocol !== 'string') { - validationErrors.push('protocol is required and must be a string') - } - if (req.body.port && Number.isInteger(req.body.port) === false) { - validationErrors.push('port must be an integer') - } - + validateRuleFields(req.body, validationErrors) if (validationErrors.length > 0) { throw invalidParameters(validationErrors) } @@ -541,7 +530,7 @@ class SDNController extends EventEmitter { networkId: req.params.id, } - if (req.body.port) { + if (req.body.port != null) { rule.port = req.body.port } @@ -560,30 +549,7 @@ class SDNController extends EventEmitter { ':id/actions/add_traffic_rule': { _post: async (req, res, next) => { const validationErrors = [] - if (!req.body) { - validationErrors.push('body is required') - throw invalidParameters(validationErrors) - } - - if (req.body.allow === undefined || typeof req.body.allow !== 'boolean') { - validationErrors.push('allow is required and must be a boolean') - } - - if (!req.body.direction || typeof req.body.direction !== 'string') { - validationErrors.push('direction is required and must be a string') - } - - if (!req.body.ipRange || typeof req.body.ipRange !== 'string') { - validationErrors.push('ipRange is required and must be a string') - } - - if (!req.body.protocol || typeof req.body.protocol !== 'string') { - validationErrors.push('protocol is required and must be a string') - } - if (req.body.port && Number.isInteger(req.body.port) === false) { - validationErrors.push('port must be an integer') - } - + validateRuleWithAllow(req.body ?? {}, validationErrors) if (validationErrors.length > 0) { throw invalidParameters(validationErrors) } @@ -613,22 +579,7 @@ class SDNController extends EventEmitter { ':id/actions/delete_traffic_rule': { _post: async (req, res, next) => { const validationErrors = [] - - if (!req.body.direction || typeof req.body.direction !== 'string') { - validationErrors.push('direction is required and must be a string') - } - - if (!req.body.ipRange || typeof req.body.ipRange !== 'string') { - validationErrors.push('ipRange is required and must be a string') - } - - if (!req.body.protocol || typeof req.body.protocol !== 'string') { - validationErrors.push('protocol is required and must be a string') - } - if (req.body.port && Number.isInteger(req.body.port) === false) { - validationErrors.push('port must be an integer') - } - + validateRuleFields(req.body, validationErrors) if (validationErrors.length > 0) { throw invalidParameters(validationErrors) } @@ -658,7 +609,188 @@ class SDNController extends EventEmitter { }, '/plugins/sdn-controller' ) + this._cleaners.push( + this._xo.registerRestRoutes( + [ + { + endpoint: '/networks/{id}/actions/update_traffic_rule', + description: + 'Update a rule on a network, needs the exact old rule fields.\n\nRequired privilege:\n - resource: network, action: update:other_config', + method: 'post', + tags: ['sdn-controller'], + params: { + id: { type: 'string', example: 'b97f4e69-d275-4b25-9dc9-c1ac4e9b3fa5' }, + }, + query: { + sync: { type: 'boolean', optional: true }, + }, + body: { + oldRule: { + type: 'object', + fields: { + allow: { type: 'boolean', example: true }, + direction: { type: 'string', example: 'to' }, + ipRange: { type: 'string', example: '192.168.0.0/24' }, + protocol: { type: 'string', example: 'tcp' }, + port: { type: 'number', example: 443, optional: true }, + }, + }, + newRule: { + type: 'object', + fields: { + allow: { type: 'boolean', example: true, optional: true }, + direction: { type: 'string', example: 'to', optional: true }, + ipRange: { type: 'string', example: '10.0.0.0/8', optional: true }, + protocol: { type: 'string', example: 'tcp', optional: true }, + port: { type: 'number', example: 80, optional: true }, + }, + }, + }, + responses: [ + { + status: 204, + description: 'Rule updated successfully', + }, + { + status: 404, + description: 'Old rule does not exist on this network', + }, + ], + middlewares: [ + { name: 'json' }, + { name: 'acl', acls: { resource: 'network', action: 'update:other_config', objectId: 'params.id' } }, + ], + callback: ({ req, createAction }) => { + return createAction( + async () => { + const { oldRule, newRule: partialNewRule } = req.body + const networkId = req.params.id + const network = this._xo.getObject(networkId, 'network') + const networkRules = JSON.parse(network.other_config[SDN_CONTROLLER_OF_RULES_KEY] || '[]').map( + JSON.parse + ) + if ( + !networkRules.some( + rule => + rule.allow === oldRule.allow && + rule.direction === oldRule.direction && + rule.ipRange === oldRule.ipRange && + rule.port === oldRule.port && + rule.protocol.toLowerCase() === oldRule.protocol.toLowerCase() + ) + ) { + throw noSuchObject(JSON.stringify(oldRule), 'traffic-rule') + } + const newRule = { ...oldRule, ...partialNewRule } + + await this._deleteNetworkOfRule({ ...oldRule, networkId }) + await this._addNetworkRule({ ...newRule, networkId }) + }, + { + sync: req.query.sync ?? false, + statusCode: 204, + taskProperties: { + name: 'update network traffic rule', + objectId: req.params.id, + params: req.body, + objectType: 'network', + }, + } + ) + }, + }, + { + endpoint: '/vifs/{id}/actions/update_traffic_rule', + description: + 'Update a rule on a VIF, needs the exact old rule fields.\n\nRequired privilege:\n - resource: vif, action: update:other_config', + method: 'post', + tags: ['sdn-controller'], + params: { + id: { type: 'string', example: 'b97f4e69-d275-4b25-9dc9-c1ac4e9b3fa5' }, + }, + query: { + sync: { type: 'boolean', optional: true }, + }, + body: { + oldRule: { + type: 'object', + fields: { + allow: { type: 'boolean', example: true }, + direction: { type: 'string', example: 'from' }, + ipRange: { type: 'string', example: '192.168.0.0/24' }, + protocol: { type: 'string', example: 'tcp' }, + port: { type: 'number', example: 443, optional: true }, + }, + }, + newRule: { + type: 'object', + fields: { + allow: { type: 'boolean', example: true, optional: true }, + direction: { type: 'string', example: 'to', optional: true }, + ipRange: { type: 'string', example: '10.0.0.0/8', optional: true }, + protocol: { type: 'string', example: 'tcp', optional: true }, + port: { type: 'number', example: 80, optional: true }, + }, + }, + }, + + responses: [ + { + status: 204, + description: 'Rule updated successfully', + }, + { + status: 404, + description: 'Old rule does not exist on this VIF', + }, + ], + middlewares: [ + { name: 'json' }, + { name: 'acl', acls: { resource: 'vif', action: 'update:other_config', objectId: 'params.id' } }, + ], + callback: ({ req, createAction }) => { + return createAction( + async () => { + const { oldRule, newRule: partialNewRule } = req.body + const vifId = req.params.id + const vif = this._xo.getObject(vifId, 'VIF') + const rawVifRules = vif.other_config[SDN_CONTROLLER_OF_RULES_KEY] + const vifRules = rawVifRules !== undefined ? JSON.parse(rawVifRules).map(JSON.parse) : [] + if ( + !vifRules.some( + rule => + rule.allow === oldRule.allow && + rule.direction === oldRule.direction && + rule.ipRange === oldRule.ipRange && + rule.port === oldRule.port && + rule.protocol.toLowerCase() === oldRule.protocol.toLowerCase() + ) + ) { + throw noSuchObject(JSON.stringify(oldRule), 'traffic-rule') + } + + const newRule = { ...oldRule, ...partialNewRule } + await this._deleteRule({ ...oldRule, vifId }) + await this._addRule({ ...newRule, vifId }) + }, + { + sync: req.query.sync ?? false, + statusCode: 204, + taskProperties: { + name: 'update vif traffic rule', + objectId: req.params.id, + params: req.body, + objectType: 'VIF', + }, + } + ) + }, + }, + ], + '/plugins/sdn-controller' + ) + ) forOwn(this._xo.getAllXapis(), xapi => { if (xapi.status === 'connected') { this._handleConnectedXapi(xapi) diff --git a/packages/xo-server/src/xo-mixins/_rest-api.test.mjs b/packages/xo-server/src/xo-mixins/_rest-api.test.mjs index 502a1139fc..00c6ec1995 100644 --- a/packages/xo-server/src/xo-mixins/_rest-api.test.mjs +++ b/packages/xo-server/src/xo-mixins/_rest-api.test.mjs @@ -658,7 +658,10 @@ describe('RestApi', () => { const response = await get(port, '/param-validation/baz') assert.equal(response.status, 422) }) - + it('rejects object in route param', async () => { + const response = await get(port, '/param-validation/{type}') + assert.equal(response.status, 422) + }) it('accepts valid required query param', async () => { const response = await get(port, '/query-required-validation?name=hello') assert.equal(response.status, 200) @@ -691,6 +694,124 @@ describe('RestApi', () => { const response = await post(port, '/body-validation', { body: {} }) assert.equal(response.status, 422) }) + + describe('object type', () => { + before(() => { + restApi.registerRestRoutes( + [ + { + endpoint: '/body-object-validation', + method: 'post', + middlewares: [{ name: 'json' }], + body: { + meta: { + type: 'object', + fields: { + key: { type: 'string' }, + count: { type: 'number' }, + }, + }, + }, + callback: ({ req }) => ({ meta: req.body.meta }), + }, + ], + '' + ) + }) + + it('accepts valid nested object in body', async () => { + const response = await post(port, '/body-object-validation', { body: { meta: { key: 'hello', count: 42 } } }) + assert.equal(response.status, 200) + assert.deepEqual(await response.json(), { meta: { key: 'hello', count: 42 } }) + }) + + it('rejects invalid type for nested field in body', async () => { + const response = await post(port, '/body-object-validation', { body: { meta: { key: 123, count: 42 } } }) + assert.equal(response.status, 422) + }) + + it('rejects missing required nested field in body', async () => { + const response = await post(port, '/body-object-validation', { body: { meta: { key: 'hello' } } }) + assert.equal(response.status, 422) + }) + + it('rejects missing required object field in body', async () => { + const response = await post(port, '/body-object-validation', { body: {} }) + assert.equal(response.status, 422) + }) + + it('accepts optional object field absent from body', async () => { + const unregister = restApi.registerRestRoutes( + [ + { + endpoint: '/body-optional-object-test', + method: 'post', + middlewares: [{ name: 'json' }], + body: { + meta: { + type: 'object', + fields: { key: { type: 'string' } }, + optional: true, + }, + }, + callback: ({ req }) => ({ hasBody: req.body.meta !== undefined }), + }, + ], + '/' + ) + try { + const response = await post(port, '/body-optional-object-test', { body: {} }) + assert.equal(response.status, 200) + assert.deepEqual(await response.json(), { hasBody: false }) + } finally { + unregister() + } + }) + + it('rejects object type in route params at runtime', async () => { + const unregister = restApi.registerRestRoutes( + [ + { + endpoint: '/runtime-param-object-test/{data}', + method: 'get', + params: { + data: { type: 'object', fields: { key: { type: 'string' } } }, + }, + callback: ({ req }) => ({ data: req.params.data }), + }, + ], + '/' + ) + try { + const response = await get(port, '/runtime-param-object-test/hello') + assert.equal(response.status, 422) + } finally { + unregister() + } + }) + + it('rejects object type in query params at runtime', async () => { + const unregister = restApi.registerRestRoutes( + [ + { + endpoint: '/runtime-query-object-test', + method: 'get', + query: { + data: { type: 'object', fields: { key: { type: 'string' } } }, + }, + callback: ({ req }) => ({ data: req.query.data }), + }, + ], + '/' + ) + try { + const response = await get(port, '/runtime-query-object-test?data=hello') + assert.equal(response.status, 422) + } finally { + unregister() + } + }) + }) }) describe('middlewares', () => { @@ -1226,6 +1347,41 @@ describe('RestApi', () => { assert.ok(pathEntry !== undefined) assert.equal(pathEntry.get.responses['200'].description, 'A custom OK') }) + + it('object field in response schema appears in swagger spec with nested properties', async () => { + restApi.registerRestRoutes( + [ + { + endpoint: '/swagger-response-object', + method: 'get', + responses: [ + { + status: 200, + description: 'A response with a nested object', + schema: { + data: { + type: 'object', + fields: { + id: { type: 'string' }, + count: { type: 'number' }, + }, + }, + }, + }, + ], + callback: () => ({}), + }, + ], + '/' + ) + const spec = await (await fetchSwagger(port)).json() + const pathEntry = spec.paths['/swagger-response-object'] + assert.ok(pathEntry !== undefined) + const responseSchema = pathEntry.get.responses['200'].content?.['application/json']?.schema + assert.equal(responseSchema?.properties?.data?.type, 'object') + assert.ok(responseSchema?.properties?.data?.properties?.id !== undefined) + assert.ok(responseSchema?.properties?.data?.properties?.count !== undefined) + }) }) }) })