diff --git a/DESIGN.md b/DESIGN.md index 85cad5be25..983189ff62 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -220,3 +220,19 @@ set; and the cert subscription shares the same debounced `scheduleRebuild` (same its coalescing must stay a superset-safe no-op for the single-swap #586 case. Regression coverage: `integrationTests/security/cert-key-reload.test.ts` deterministically pins the cert-before-key ordering (it fails by design without the rebuild trigger); `cert-reload.test.ts` guards the cert-only #586 path. + +## `set_configuration` replication is opt-in; `replicateOperation` is default-on (`config/configUtils.ts`) + +`server.replication.replicateOperation` (installed by harper-pro's replicator) fans out whenever +`req.replicated \!== false` — absence of the flag means "replicate". That default-on contract is what +DDL ops rely on (`dropSchema`/`dropTable` call it unconditionally), so a handler that mirrors the +drop_schema pattern without a guard silently becomes replicate-by-default. `setConfiguration` must +stay **opt-in** (`if (replicated)` truthy guard) because config bodies routinely carry node-local +params (ports, paths, node identity) that would clobber peers. Two invariants to preserve: +`replicated` must remain in the handler's destructure strip-list on both origin and peers (peers +receive `replicated: false` in the forwarded body; anything not stripped is treated as a config +param), and there is deliberately **no** per-param node-local/cluster-wide guard here — per-field +replicability metadata is deferred to the cluster-level-config work (CORE-3018), which will own that +schema. Per-peer failures never reject: they come back as `{status: 'failed', reason, node}` entries +in `response.replicated[]`, and `message` still reads as success (same contract as drop_schema), so +operators must inspect the array for per-node outcomes. diff --git a/config/configUtils.ts b/config/configUtils.ts index dc94991f98..ca6894aea9 100644 --- a/config/configUtils.ts +++ b/config/configUtils.ts @@ -830,9 +830,33 @@ export function getConfiguration() { */ export async function setConfiguration(setConfigJson) { // eslint-disable-next-line no-unused-vars - const { operation, hdb_user, hdbAuthHeader, ...configFields } = setConfigJson; + const { operation, hdb_user, hdbAuthHeader, replicated, ...configFields } = setConfigJson; + // Operation-control field, not a config param: enforce boolean (matching other + // `replicated` surfaces, e.g. analyticsValidator) before any local write so a + // malformed value like the string "false" — which is truthy — can't apply config + // locally or trigger an unintended fan-out. + if (replicated !== undefined && typeof replicated !== 'boolean') { + throw handleHDBError( + new Error(), + `'replicated' must be a boolean`, + HTTP_STATUS_CODES.BAD_REQUEST, + undefined, + undefined, + true + ); + } try { updateConfigValue(undefined, undefined, configFields, true); + if (replicated) { + // Opt-in fan-out to all cluster nodes (#660). replicateOperation forwards the + // body with `replicated: false`, so peers apply locally without re-replicating; + // per-node outcomes are returned on `response.replicated`. `replicated` must + // stay out of configFields on both origin and peers, or it would be written to + // the config file as a config param. + const response = await server.replication.replicateOperation(setConfigJson); + response.message = CONFIGURE_SUCCESS_RESPONSE; + return response; + } return CONFIGURE_SUCCESS_RESPONSE; } catch (err) { if (typeof err === 'string' || err instanceof String) { diff --git a/integrationTests/apiTests/configuration.test.mjs b/integrationTests/apiTests/configuration.test.mjs index a7c6ba94e0..c41aa1d9e7 100644 --- a/integrationTests/apiTests/configuration.test.mjs +++ b/integrationTests/apiTests/configuration.test.mjs @@ -256,6 +256,47 @@ suite('Configuration', (ctx) => { .expect(400); }); + // ── set_configuration + replicated (#660) ─────────────────────────────── + // Real cluster fan-out lives in harper-pro; without it, the base server's + // replication stub rejects a truthy `replicated`. These tests pin the + // single-node contract: the flag routes through the operation as an option + // (never a config key), the local write still lands first (same origin-first + // ordering as drop_schema), and the rejection is explicit. + + test('set_configuration with replicated: true on non-clustered instance rejects explicitly', async () => { + const r = await client + .req() + .send({ operation: 'set_configuration', logging_rotation_maxSize: '14M', replicated: true }); + assert.ok(r.status >= 400, `expected error status, got ${r.status}\n${r.text}`); + assert.match(r.body.error ?? '', /Replication not implemented/, r.text); + }); + + test('set_configuration with string "false" replicated is rejected before applying', async () => { + await client + .req() + .send({ operation: 'set_configuration', logging_rotation_maxSize: '16M', replicated: 'false' }) + .expect((r) => assert.match(r.body.error ?? '', /replicated/, r.text)) + .expect(400); + // The malformed request must not have applied its config change locally. + await client + .req() + .send({ operation: 'get_configuration' }) + .expect((r) => assert.notEqual(r.body.logging.rotation.maxSize, '16M', r.text)) + .expect(200); + }); + + test('replicated flag is never persisted to configuration', async () => { + await client + .req() + .send({ operation: 'get_configuration' }) + .expect((r) => { + // Local write from the rejected replicated call above still applied (origin-first). + assert.equal(r.body.logging.rotation.maxSize, '14M', r.text); + assert.equal(r.body.replicated, undefined, r.text); + }) + .expect(200); + }); + // ── non-superuser role restrictions ───────────────────────────────────── test('add non-SU role and user', async () => { diff --git a/unitTests/config/configUtils.test.js b/unitTests/config/configUtils.test.js index 072b41707f..b940373884 100644 --- a/unitTests/config/configUtils.test.js +++ b/unitTests/config/configUtils.test.js @@ -1073,6 +1073,130 @@ describe('Test configUtils module', () => { expect(error.name).to.equal(STRING_ERROR); }); + + describe('replicated: true', () => { + // configUtils reads `server` from the shared module object, so stubbing the + // property (the same way harper-pro installs the real implementation) reaches it. + const { server } = require('#src/server/Server'); + let replicate_operation_stub; + let original_replicate_operation; + + beforeEach(() => { + original_replicate_operation = server.replication.replicateOperation; + replicate_operation_stub = sandbox.stub().resolves({ + message: '', + replicated: [{ message: 'ok', node: 'peer-1' }], + }); + server.replication.replicateOperation = replicate_operation_stub; + }); + + afterEach(() => { + server.replication.replicateOperation = original_replicate_operation; + }); + + it('Test replicated: true fans out and returns per-node results', async () => { + const test_set_config_json = { + operation: 'set_configuration', + http_corsAccessList: ['harper.fast'], + replicated: true, + hdb_user: {}, + hdb_auth_header: 'test_header', + }; + + const result = await config_utils_rw.setConfiguration(test_set_config_json); + + expect(replicate_operation_stub.calledOnceWith(test_set_config_json)).to.equal(true); + expect(result.message).to.equal(CONFIGURE_SUCCESS_RESPONSE); + expect(result.replicated).to.eql([{ message: 'ok', node: 'peer-1' }]); + }); + + it('Test replicated is stripped from the config fields written to file', async () => { + const test_set_config_json = { + operation: 'set_configuration', + http_corsAccessList: ['harper.fast'], + replicated: true, + hdb_user: {}, + hdb_auth_header: 'test_header', + }; + + await config_utils_rw.setConfiguration(test_set_config_json); + + const config_fields = update_config_value_stub.firstCall.args[2]; + expect(config_fields).to.eql({ + http_corsAccessList: ['harper.fast'], + hdb_auth_header: 'test_header', + }); + }); + + it('Test no replicated flag does not fan out', async () => { + const test_set_config_json = { + operation: 'set_configuration', + operationsApi_processes: 18, + hdb_user: {}, + hdb_auth_header: 'test_header', + }; + + const result = await config_utils_rw.setConfiguration(test_set_config_json); + + expect(replicate_operation_stub.called).to.equal(false); + expect(result).to.equal(CONFIGURE_SUCCESS_RESPONSE); + }); + + it('Test replicated: false (peer receiving a fanned-out call) does not re-replicate', async () => { + const test_set_config_json = { + operation: 'set_configuration', + http_corsAccessList: ['harper.fast'], + replicated: false, + hdb_user: {}, + hdb_auth_header: 'test_header', + }; + + const result = await config_utils_rw.setConfiguration(test_set_config_json); + + expect(replicate_operation_stub.called).to.equal(false); + expect(result).to.equal(CONFIGURE_SUCCESS_RESPONSE); + const config_fields = update_config_value_stub.firstCall.args[2]; + expect(config_fields).to.not.have.property('replicated'); + }); + + it('Test non-boolean replicated is rejected before any local write', async () => { + for (const bad of ['false', 'true', 1, 0]) { + let error; + try { + await config_utils_rw.setConfiguration({ + operation: 'set_configuration', + http_corsAccessList: ['harper.fast'], + replicated: bad, + }); + } catch (err) { + error = err; + } + + expect(error, `expected rejection for replicated: ${JSON.stringify(bad)}`).to.not.equal(undefined); + expect(error.http_resp_msg ?? error.message).to.include('replicated'); + } + expect(update_config_value_stub.called).to.equal(false); + expect(replicate_operation_stub.called).to.equal(false); + }); + + it('Test local config write failure propagates without fanning out', async () => { + update_config_value_stub.throws(STRING_ERROR); + + let error; + try { + await config_utils_rw.setConfiguration({ + operation: 'set_configuration', + http_corsAccessList: ['harper.fast'], + replicated: true, + }); + } catch (err) { + error = err; + } + + expect(error.name).to.equal(STRING_ERROR); + expect(replicate_operation_stub.called).to.equal(false); + }); + }); }); describe('Test readConfigFile function', () => { let properties_reader_rw;