CLDSRV-1031: Report the health of every configured KMS

The deep health check has one entry per configured KMS, keyed by its providerName, each with its own cache; KMS failures stay informational.

Author: dvasilas

CLAUDE.md

@@ -132,7 +132,8 @@ KMSs are configured as a `kms` array in `config.json`. Each entry has a
 `protocol`, a unique `providerName`, the settings of that protocol, and exactly
 one entry has `"default": true`. Keys are created in the default KMS, and each
 key is sent to the KMS its ARN names. A key ID without ARN in a request goes to
-the default KMS.
+the default KMS. The deep health check has one entry per KMS, keyed by its
+`providerName`; a failing KMS never fails start-up or readiness.
 
 | Protocol  | Description                                            |
 | --------- | ------------------------------------------------------ |

lib/kms/wrapper.js

@@ -12,7 +12,6 @@ const { KmsAWSClient } = require('@scality/arsenal').network;
 const Common = require('./common');
 const vault = require('../auth/vault');
 const Cache = require('./Cache');
-const cache = new Cache();
 const {
     KmsProtocol,
     makeBackend,
@@ -69,13 +68,14 @@ function getClientIdentifier(backend) {
  * Build a client for every entry of the `kms` config.
  * @param {object[]} entries - `config.kms`
  * @returns {object} `{ clientInstances, defaultInstance }`, with
- * `clientInstances` keyed by `type:protocol:provider`
+ * `clientInstances` keyed by `type:protocol:provider`, each with its own
+ * health check cache
  */
 function createClients(entries) {
     const instances = {};
     let defaultInstance;
     entries.forEach(entry => {
-        const instance = getClient(entry.protocol, entry.settings);
+        const instance = { ...getClient(entry.protocol, entry.settings), healthCache: new Cache() };
         instances[getClientIdentifier(instance.client.backend)] = instance;
         if (entry.default) {
             defaultInstance = instance;
@@ -99,8 +99,11 @@ const { clientInstances, defaultInstance } = createClients(config.kms);
 const { client, implName } = defaultInstance;
 const currentIdentifier = getClientIdentifier(client.backend);
 
+// Taken before the SSE-migration client is added, which is not health-checked
+const configuredInstances = Object.values(clientInstances);
+
 // Keys of every configured KMS are accepted
-const availableBackends = Object.values(clientInstances).map(instance => instance.client.backend);
+const availableBackends = configuredInstances.map(instance => instance.client.backend);
 
 let previousBackend;
 let previousIdentifier;
@@ -168,6 +171,84 @@ function getClientForKey(key, log) {
     };
 }
 
+/**
+ * Health of one KMS, from its cache when the last check is recent.
+ * @param {object} instance - client instance, from createClients
+ * @param {object} log - logger object
+ * @param {function} cb - callback, called with (null, result)
+ * @returns {undefined}
+ */
+function checkKmsHealth(instance, log, cb) {
+    const { client: kmsClient, healthCache } = instance;
+    const { provider } = kmsClient.backend;
+    if (!kmsClient.healthcheck) {
+        return cb(null, { code: 200, message: 'OK' });
+    }
+
+    const cachedResult = healthCache.getResult();
+    logger.debug('current KMS cache state', { provider, result: cachedResult });
+
+    const shouldRefreshCache = healthCache.shouldRefresh();
+
+    if (shouldRefreshCache) {
+        logger.debug('health check for KMS backend', { provider });
+        return kmsClient.healthcheck(log, err => {
+            let res;
+            if (err) {
+                res = {
+                    // The following response makes sure that if KMS is down,
+                    // cloudserver health check is still healthy.
+                    // Simply including an error code in the response won't cause the health check to fail.
+                    // Instead, the healthCheck logic detects errors by checking for the "error" field.
+                    code: err.code,
+                    message: 'KMS health check failed',
+                    description: err.description,
+                };
+                logger.warn('KMS health check failed', { provider, errorCode: err.code, error: err.description });
+            } else {
+                res = {
+                    code: 200,
+                    message: 'OK',
+                };
+                logger.info('KMS health check succeeded', { provider, res });
+            }
+
+            healthCache.setResult(res);
+            const updatedResult = healthCache.getResult();
+            logger.debug('updated KMS cache:', { provider, result: updatedResult });
+
+            return cb(null, updatedResult);
+        });
+    }
+
+    // Use cached healthcheck result if within a 1-hour window
+    logger.debug('using cached KMS health check', { provider, cachedResult });
+    return cb(null, cachedResult);
+}
+
+/**
+ * Health of each KMS, checked in parallel.
+ * @param {object[]} instances - client instances, from createClients
+ * @param {object} log - logger object
+ * @param {function} cb - callback, called with (null, result) where result
+ * has one entry per KMS, keyed by provider name
+ * @returns {undefined}
+ */
+function checkKmsHealthByProvider(instances, log, cb) {
+    async.map(
+        instances,
+        (instance, done) => checkKmsHealth(instance, log, done),
+        // No error: a failed check is reported in the KMS entry
+        (err, results) => {
+            const respBody = {};
+            instances.forEach((instance, i) => {
+                respBody[instance.client.backend.provider] = results[i];
+            });
+            return cb(null, respBody);
+        },
+    );
+}
+
 class KMS {
     /** Access to client for tests (to create a key without vault check and bucket) */
     static get client() {
@@ -547,55 +628,17 @@ class KMS {
         );
     }
 
+    /**
+     * Health of every configured KMS, keyed by provider name.
+     * @param {object} log - logger object
+     * @param {function} cb - callback, called with (null, result)
+     * @returns {undefined}
+     */
     static checkHealth(log, cb) {
-        if (!client.healthcheck) {
-            return cb(null, {
-                [implName]: { code: 200, message: 'OK' },
-            });
-        }
-
-        const cachedResult = cache.getResult();
-        logger.debug('current KMS cache state', { result: cachedResult });
-
-        const shouldRefreshCache = cache.shouldRefresh();
-
-        if (shouldRefreshCache) {
-            logger.debug('health check for KMS backend');
-            return client.healthcheck(log, err => {
-                let res;
-                if (err) {
-                    res = {
-                        // The following response makes sure that if KMS is down,
-                        // cloudserver health check is still healthy.
-                        // Simply including an error code in the response won't cause the health check to fail.
-                        // Instead, the healthCheck logic detects errors by checking for the "error" field.
-                        code: err.code,
-                        message: 'KMS health check failed',
-                        description: err.description,
-                    };
-                    logger.warn('KMS health check failed', { errorCode: err.code, error: err.description });
-                } else {
-                    res = {
-                        code: 200,
-                        message: 'OK',
-                    };
-                    logger.info('KMS health check succeeded', { res });
-                }
-
-                cache.setResult(res);
-                const updatedResult = cache.getResult();
-                logger.debug('updated KMS cache:', { result: updatedResult });
-
-                const respBody = { [implName]: updatedResult };
-                return cb(null, respBody);
-            });
-        }
-
-        // Use cached healthcheck result if within a 1-hour window
-        logger.debug('using cached KMS health check', { cachedResult });
-        return cb(null, { [implName]: cachedResult });
+        return checkKmsHealthByProvider(configuredInstances, log, cb);
     }
 }
 
 module.exports = KMS;
 module.exports.createClients = createClients;
+module.exports.checkKmsHealthByProvider = checkKmsHealthByProvider;

tests/functional/kmip/serverside_encryption.js

@@ -425,3 +425,30 @@ describe('KMIP backed server-side encryption with other KMSs configured', () =>
             assert.rejects(putBucketEncryption(bucketName, keyArn), assertRefusedNamingOnlyTheKey));
     });
 });
+
+describe('KMS health in the deep health check', () => {
+    function getDeepHealthcheck() {
+        return new Promise((resolve, reject) => {
+            const req = http.get({ host: config.ipAddress, port: serverConfig.metricsPort, path: '/ready' }, res => {
+                let body = '';
+                res.on('data', chunk => {
+                    body += chunk;
+                });
+                res.on('end', () => resolve({ statusCode: res.statusCode, body: JSON.parse(body) }));
+            });
+            req.on('error', reject);
+        });
+    }
+
+    it('should report every configured KMS, keyed by provider name', async () => {
+        const { statusCode, body } = await getDeepHealthcheck();
+
+        assert.strictEqual(statusCode, 200);
+        serverConfig.kms.forEach(({ protocol, providerName }) => {
+            const { lastChecked, ...health } = body[providerName];
+            assert.deepStrictEqual(health, { code: 200, message: 'OK' }, `KMS ${providerName}`);
+            // A KMS whose client has no health check reports OK without being checked; today only the KMIP client has one
+            assert.strictEqual(lastChecked !== undefined, protocol === KmsProtocol.kmip, `KMS ${providerName}`);
+        });
+    });
+});

tests/unit/encryption/checkHealth.js

@@ -3,9 +3,10 @@ const sinon = require('sinon');
 const { errors } = require('@scality/arsenal');
 
 const Cache = require('../../../lib/kms/Cache');
-const { DummyRequestLogger } = require('../helpers');
+const { DummyRequestLogger, kmsTestEntries, parseKmsEntries } = require('../helpers');
 const memBackend = require('../../../lib/kms/in_memory/backend');
 const kms = require('../../../lib/kms/wrapper');
+const logger = require('../../../lib/utilities/logger');
 
 const log = new DummyRequestLogger();
 
@@ -37,7 +38,7 @@ describe('KMS.checkHealth', () => {
         kms.checkHealth(log, (err, result) => {
             assert.ifError(err);
             assert.deepStrictEqual(result, {
-                memoryKms: { code: 200, message: 'OK' },
+                scality: { code: 200, message: 'OK' },
             });
 
             assert(shouldRefreshStub.notCalled, 'shouldRefresh should not be called');
@@ -56,7 +57,7 @@ describe('KMS.checkHealth', () => {
             const expectedLastChecked = new Date(clock.now).toISOString();
 
             assert.deepStrictEqual(result, {
-                memoryKms: { code: 200, message: 'OK', lastChecked: expectedLastChecked },
+                scality: { code: 200, message: 'OK', lastChecked: expectedLastChecked },
             });
 
             assert(shouldRefreshStub.calledOnce, 'shouldRefresh should be called once');
@@ -81,7 +82,7 @@ describe('KMS.checkHealth', () => {
             const expectedLastChecked = new Date(clock.now).toISOString();
 
             assert.deepStrictEqual(result, {
-                memoryKms: {
+                scality: {
                     code: 500,
                     message: 'KMS health check failed',
                     description: 'We encountered an internal error. Please try again.',
@@ -116,7 +117,7 @@ describe('KMS.checkHealth', () => {
 
                 const expectedLastChecked = new Date(clock.now).toISOString();
                 assert.deepStrictEqual(result, {
-                    memoryKms: {
+                    scality: {
                         code: 200,
                         message: 'OK',
                         lastChecked: expectedLastChecked,
@@ -134,3 +135,93 @@ describe('KMS.checkHealth', () => {
         });
     });
 });
+
+describe('KMS health of multiple KMSs', () => {
+    const { kmip: kmipEntry, otherKmip: otherKmipEntry, awsKms: awsEntry, file: fileEntry } = kmsTestEntries;
+    let clock;
+    let warnStub;
+
+    beforeEach(() => {
+        clock = sinon.useFakeTimers({
+            now: 1625077800000,
+            toFake: ['Date'],
+        });
+        sinon.stub(logger, 'info');
+        warnStub = sinon.stub(logger, 'warn');
+    });
+
+    afterEach(() => {
+        sinon.restore();
+        clock.restore();
+    });
+
+    function createInstances(entries) {
+        const { clientInstances } = kms.createClients(parseKmsEntries(entries));
+        return clientInstances;
+    }
+
+    function checkKmsHealthByProvider(instances) {
+        return new Promise(resolve =>
+            kms.checkKmsHealthByProvider(instances, log, (err, result) => {
+                assert.ifError(err);
+                resolve(result);
+            }),
+        );
+    }
+
+    it('should report one entry per KMS, keyed by provider name', async () => {
+        const instances = createInstances([kmipEntry, awsEntry, fileEntry]);
+        const kmipHealthcheck = sinon
+            .stub(instances['external:kmip:thales'].client, 'healthcheck')
+            .callsFake((log, cb) => cb(null));
+
+        const result = await checkKmsHealthByProvider(Object.values(instances));
+
+        // AWS KMS and file have no health check, they report OK without being checked
+        assert.deepStrictEqual(result, {
+            thales: { code: 200, message: 'OK', lastChecked: new Date(clock.now).toISOString() },
+            acme: { code: 200, message: 'OK' },
+            scality: { code: 200, message: 'OK' },
+        });
+        assert(kmipHealthcheck.calledOnce);
+    });
+
+    it('should report a failing KMS next to healthy ones', async () => {
+        const instances = createInstances([kmipEntry, awsEntry]);
+        sinon
+            .stub(instances['external:kmip:thales'].client, 'healthcheck')
+            .callsFake((log, cb) => cb(errors.InternalError));
+
+        const result = await checkKmsHealthByProvider(Object.values(instances));
+
+        assert.deepStrictEqual(result, {
+            thales: {
+                code: 500,
+                message: 'KMS health check failed',
+                description: 'We encountered an internal error. Please try again.',
+                lastChecked: new Date(clock.now).toISOString(),
+            },
+            acme: { code: 200, message: 'OK' },
+        });
+        assert(warnStub.calledOnceWith('KMS health check failed', sinon.match({ provider: 'thales', errorCode: 500 })));
+    });
+
+    it('should keep a health check cache per KMS', async () => {
+        const instances = createInstances([kmipEntry, otherKmipEntry]);
+        const thales = instances['external:kmip:thales'];
+        const hytrust = instances['external:kmip:hytrust'];
+        const thalesHealthcheck = sinon.stub(thales.client, 'healthcheck').callsFake((log, cb) => cb(null));
+        const hytrustHealthcheck = sinon
+            .stub(hytrust.client, 'healthcheck')
+            .callsFake((log, cb) => cb(errors.InternalError));
+
+        await checkKmsHealthByProvider([thales]);
+        const result = await checkKmsHealthByProvider([thales, hytrust]);
+
+        // thales comes from its cache, hytrust has no cached result yet
+        assert(thalesHealthcheck.calledOnce);
+        assert(hytrustHealthcheck.calledOnce);
+        assert.strictEqual(result.thales.code, 200);
+        assert.strictEqual(result.hytrust.code, 500);
+    });
+});