From 88c1c4ad7b554f47285a10f293c3a903208270c4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Thu, 24 Oct 2024 11:37:19 +0200 Subject: [PATCH] refactor(core): Add external secrets log scope (#11224) --- .../@n8n/config/src/configs/logging.config.ts | 2 ++ .../external-secrets-manager.ee.test.ts | 4 ++-- .../external-secrets-manager.ee.ts | 22 ++++++++++++++++--- .../aws-secrets/aws-secrets-manager.ts | 18 ++++++++++++++- .../azure-key-vault/azure-key-vault.ts | 17 +++++++++++++- .../gcp-secrets-manager.ts | 16 ++++++++++++-- .../src/external-secrets/providers/vault.ts | 7 ++++++ .../external-secrets.api.test.ts | 18 +++++++++++++-- 8 files changed, 93 insertions(+), 11 deletions(-) diff --git a/packages/@n8n/config/src/configs/logging.config.ts b/packages/@n8n/config/src/configs/logging.config.ts index 736e32a903034..0568eaf79167f 100644 --- a/packages/@n8n/config/src/configs/logging.config.ts +++ b/packages/@n8n/config/src/configs/logging.config.ts @@ -4,6 +4,7 @@ import { StringArray } from '../utils'; /** Scopes (areas of functionality) to filter logs by. */ export const LOG_SCOPES = [ 'concurrency', + 'external-secrets', 'license', 'multi-main-setup', 'pubsub', @@ -64,6 +65,7 @@ export class LoggingConfig { * Supported log scopes: * * - `concurrency` + * - `external-secrets` * - `license` * - `multi-main-setup` * - `pubsub` diff --git a/packages/cli/src/external-secrets/__tests__/external-secrets-manager.ee.test.ts b/packages/cli/src/external-secrets/__tests__/external-secrets-manager.ee.test.ts index b1a87271f9ae2..05eabd104fa52 100644 --- a/packages/cli/src/external-secrets/__tests__/external-secrets-manager.ee.test.ts +++ b/packages/cli/src/external-secrets/__tests__/external-secrets-manager.ee.test.ts @@ -13,7 +13,7 @@ import { FailedProvider, MockProviders, } from '@test/external-secrets/utils'; -import { mockInstance } from '@test/mocking'; +import { mockInstance, mockLogger } from '@test/mocking'; describe('External Secrets Manager', () => { const connectedDate = '2023-08-01T12:32:29.000Z'; @@ -49,7 +49,7 @@ describe('External Secrets Manager', () => { license.isExternalSecretsEnabled.mockReturnValue(true); settingsRepo.getEncryptedSecretsProviderSettings.mockResolvedValue(settings); manager = new ExternalSecretsManager( - mock(), + mockLogger(), settingsRepo, license, providersMock, diff --git a/packages/cli/src/external-secrets/external-secrets-manager.ee.ts b/packages/cli/src/external-secrets/external-secrets-manager.ee.ts index ec7c3ed0cf54f..2de681a7d62be 100644 --- a/packages/cli/src/external-secrets/external-secrets-manager.ee.ts +++ b/packages/cli/src/external-secrets/external-secrets-manager.ee.ts @@ -1,5 +1,5 @@ import { Cipher } from 'n8n-core'; -import { jsonParse, type IDataObject, ApplicationError } from 'n8n-workflow'; +import { jsonParse, type IDataObject, ApplicationError, ensureError } from 'n8n-workflow'; import { Service } from 'typedi'; import { SettingsRepository } from '@/databases/repositories/settings.repository'; @@ -39,7 +39,9 @@ export class ExternalSecretsManager { private readonly cipher: Cipher, private readonly eventService: EventService, private readonly publisher: Publisher, - ) {} + ) { + this.logger = this.logger.scoped('external-secrets'); + } async init(): Promise { if (!this.initialized) { @@ -57,6 +59,8 @@ export class ExternalSecretsManager { } return await this.initializingPromise; } + + this.logger.debug('External secrets manager initialized'); } shutdown() { @@ -66,6 +70,8 @@ export class ExternalSecretsManager { void p.disconnect().catch(() => {}); }); Object.values(this.initRetryTimeouts).forEach((v) => clearTimeout(v)); + + this.logger.debug('External secrets manager shut down'); } async reloadAllProviders(backoff?: number) { @@ -77,6 +83,8 @@ export class ExternalSecretsManager { for (const provider of providers) { await this.reloadProvider(provider, backoff); } + + this.logger.debug('External secrets managed reloaded all providers'); } broadcastReloadExternalSecretsProviders() { @@ -191,6 +199,8 @@ export class ExternalSecretsManager { } }), ); + + this.logger.debug('External secrets manager updated secrets'); } getProvider(provider: string): SecretsProvider | undefined { @@ -261,6 +271,8 @@ export class ExternalSecretsManager { if (newProvider) { this.providers[provider] = newProvider; } + + this.logger.debug(`External secrets manager reloaded provider ${provider}`); } async setProviderSettings(provider: string, data: IDataObject, userId?: string) { @@ -382,8 +394,12 @@ export class ExternalSecretsManager { try { await this.providers[provider].update(); this.broadcastReloadExternalSecretsProviders(); + this.logger.debug(`External secrets manager updated provider ${provider}`); return true; - } catch { + } catch (error) { + this.logger.debug(`External secrets manager failed to update provider ${provider}`, { + error: ensureError(error), + }); return false; } } diff --git a/packages/cli/src/external-secrets/providers/aws-secrets/aws-secrets-manager.ts b/packages/cli/src/external-secrets/providers/aws-secrets/aws-secrets-manager.ts index 2c17aee5a6332..6c2c0669fb79a 100644 --- a/packages/cli/src/external-secrets/providers/aws-secrets/aws-secrets-manager.ts +++ b/packages/cli/src/external-secrets/providers/aws-secrets/aws-secrets-manager.ts @@ -1,8 +1,10 @@ import type { INodeProperties } from 'n8n-workflow'; +import Container from 'typedi'; import { UnknownAuthTypeError } from '@/errors/unknown-auth-type.error'; import { DOCS_HELP_NOTICE, EXTERNAL_SECRETS_NAME_REGEX } from '@/external-secrets/constants'; import type { SecretsProvider, SecretsProviderState } from '@/interfaces'; +import { Logger } from '@/logging/logger.service'; import { AwsSecretsClient } from './aws-secrets-client'; import type { AwsSecretsManagerContext } from './types'; @@ -76,10 +78,16 @@ export class AwsSecretsManager implements SecretsProvider { private client: AwsSecretsClient; + constructor(private readonly logger = Container.get(Logger)) { + this.logger = this.logger.scoped('external-secrets'); + } + async init(context: AwsSecretsManagerContext) { this.assertAuthType(context); this.client = new AwsSecretsClient(context.settings); + + this.logger.debug('AWS Secrets Manager provider initialized'); } async test() { @@ -87,9 +95,15 @@ export class AwsSecretsManager implements SecretsProvider { } async connect() { - const [wasSuccessful] = await this.test(); + const [wasSuccessful, errorMsg] = await this.test(); this.state = wasSuccessful ? 'connected' : 'error'; + + if (wasSuccessful) { + this.logger.debug('AWS Secrets Manager provider connected'); + } else { + this.logger.error('AWS Secrets Manager provider failed to connect', { errorMsg }); + } } async disconnect() { @@ -104,6 +118,8 @@ export class AwsSecretsManager implements SecretsProvider { this.cachedSecrets = Object.fromEntries( supportedSecrets.map((s) => [s.secretName, s.secretValue]), ); + + this.logger.debug('AWS Secrets Manager provider secrets updated'); } getSecret(name: string) { diff --git a/packages/cli/src/external-secrets/providers/azure-key-vault/azure-key-vault.ts b/packages/cli/src/external-secrets/providers/azure-key-vault/azure-key-vault.ts index e753f0abbf9b5..7961f21bad24b 100644 --- a/packages/cli/src/external-secrets/providers/azure-key-vault/azure-key-vault.ts +++ b/packages/cli/src/external-secrets/providers/azure-key-vault/azure-key-vault.ts @@ -1,8 +1,11 @@ import type { SecretClient } from '@azure/keyvault-secrets'; +import { ensureError } from 'n8n-workflow'; import type { INodeProperties } from 'n8n-workflow'; +import Container from 'typedi'; import { DOCS_HELP_NOTICE, EXTERNAL_SECRETS_NAME_REGEX } from '@/external-secrets/constants'; import type { SecretsProvider, SecretsProviderState } from '@/interfaces'; +import { Logger } from '@/logging/logger.service'; import type { AzureKeyVaultContext } from './types'; @@ -64,8 +67,14 @@ export class AzureKeyVault implements SecretsProvider { private settings: AzureKeyVaultContext['settings']; + constructor(private readonly logger = Container.get(Logger)) { + this.logger = this.logger.scoped('external-secrets'); + } + async init(context: AzureKeyVaultContext) { this.settings = context.settings; + + this.logger.debug('Azure Key Vault provider initialized'); } async connect() { @@ -78,8 +87,12 @@ export class AzureKeyVault implements SecretsProvider { const credential = new ClientSecretCredential(tenantId, clientId, clientSecret); this.client = new SecretClient(`https://${vaultName}.vault.azure.net/`, credential); this.state = 'connected'; - } catch { + this.logger.debug('Azure Key Vault provider connected'); + } catch (error) { this.state = 'error'; + this.logger.error('Azure Key Vault provider failed to connect', { + error: ensureError(error), + }); } } @@ -119,6 +132,8 @@ export class AzureKeyVault implements SecretsProvider { acc[cur.name] = cur.value; return acc; }, {}); + + this.logger.debug('Azure Key Vault provider secrets updated'); } getSecret(name: string) { diff --git a/packages/cli/src/external-secrets/providers/gcp-secrets-manager/gcp-secrets-manager.ts b/packages/cli/src/external-secrets/providers/gcp-secrets-manager/gcp-secrets-manager.ts index e6bcd11209c36..c4bf71cb72169 100644 --- a/packages/cli/src/external-secrets/providers/gcp-secrets-manager/gcp-secrets-manager.ts +++ b/packages/cli/src/external-secrets/providers/gcp-secrets-manager/gcp-secrets-manager.ts @@ -1,8 +1,10 @@ import type { SecretManagerServiceClient as GcpClient } from '@google-cloud/secret-manager'; -import { jsonParse, type INodeProperties } from 'n8n-workflow'; +import { ensureError, jsonParse, type INodeProperties } from 'n8n-workflow'; +import Container from 'typedi'; import { DOCS_HELP_NOTICE, EXTERNAL_SECRETS_NAME_REGEX } from '@/external-secrets/constants'; import type { SecretsProvider, SecretsProviderState } from '@/interfaces'; +import { Logger } from '@/logging/logger.service'; import type { GcpSecretsManagerContext, @@ -38,6 +40,10 @@ export class GcpSecretsManager implements SecretsProvider { private settings: GcpSecretAccountKey; + constructor(private readonly logger = Container.get(Logger)) { + this.logger = this.logger.scoped('external-secrets'); + } + async init(context: GcpSecretsManagerContext) { this.settings = this.parseSecretAccountKey(context.settings.serviceAccountKey); } @@ -53,8 +59,12 @@ export class GcpSecretsManager implements SecretsProvider { projectId, }); this.state = 'connected'; - } catch { + this.logger.debug('GCP Secrets Manager provider connected'); + } catch (error) { this.state = 'error'; + this.logger.debug('GCP Secrets Manager provider failed to connect', { + error: ensureError(error), + }); } } @@ -114,6 +124,8 @@ export class GcpSecretsManager implements SecretsProvider { if (cur) acc[cur.name] = cur.value; return acc; }, {}); + + this.logger.debug('GCP Secrets Manager provider secrets updated'); } getSecret(name: string) { diff --git a/packages/cli/src/external-secrets/providers/vault.ts b/packages/cli/src/external-secrets/providers/vault.ts index 398c40745df7f..0f1e93a5da5cf 100644 --- a/packages/cli/src/external-secrets/providers/vault.ts +++ b/packages/cli/src/external-secrets/providers/vault.ts @@ -237,6 +237,7 @@ export class VaultProvider extends SecretsProvider { constructor(readonly logger = Container.get(Logger)) { super(); + this.logger = this.logger.scoped('external-secrets'); } async init(settings: SecretsProviderSettings): Promise { @@ -257,6 +258,8 @@ export class VaultProvider extends SecretsProvider { } return config; }); + + this.logger.debug('Vault provider initialized'); } async connect(): Promise { @@ -408,6 +411,7 @@ export class VaultProvider extends SecretsProvider { kvVersion: string, path: string, ): Promise<[string, IDataObject] | null> { + this.logger.debug(`Getting kv secrets from ${mountPath}${path} (version ${kvVersion})`); let listPath = mountPath; if (kvVersion === '2') { listPath += 'metadata/'; @@ -441,6 +445,7 @@ export class VaultProvider extends SecretsProvider { secretPath += path + key; try { const secretResp = await this.#http.get>(secretPath); + this.logger.debug(`Vault provider retrieved secrets from ${secretPath}`); return [ key, kvVersion === '2' @@ -457,6 +462,7 @@ export class VaultProvider extends SecretsProvider { .filter((v): v is [string, IDataObject] => v !== null), ); const name = path.substring(0, path.length - 1); + this.logger.debug(`Vault provider retrieved kv secrets from ${name}`); return [name, data]; } @@ -479,6 +485,7 @@ export class VaultProvider extends SecretsProvider { ).filter((v): v is [string, IDataObject] => v !== null), ); this.cachedSecrets = secrets; + this.logger.debug('Vault provider secrets updated'); } async test(): Promise<[boolean] | [boolean, string]> { diff --git a/packages/cli/test/integration/external-secrets/external-secrets.api.test.ts b/packages/cli/test/integration/external-secrets/external-secrets.api.test.ts index 3418576be1b31..c36340108ec8c 100644 --- a/packages/cli/test/integration/external-secrets/external-secrets.api.test.ts +++ b/packages/cli/test/integration/external-secrets/external-secrets.api.test.ts @@ -18,7 +18,7 @@ import { MockProviders, TestFailProvider, } from '../../shared/external-secrets/utils'; -import { mockInstance } from '../../shared/mocking'; +import { mockInstance, mockLogger } from '../../shared/mocking'; import { createOwner, createUser } from '../shared/db/users'; import type { SuperAgentTest } from '../shared/types'; import { setupTestServer } from '../shared/utils'; @@ -52,12 +52,14 @@ async function getExternalSecretsSettings(): Promise(); +const logger = mockLogger(); + const resetManager = async () => { Container.get(ExternalSecretsManager).shutdown(); Container.set( ExternalSecretsManager, new ExternalSecretsManager( - mock(), + logger, Container.get(SettingsRepository), Container.get(License), mockProvidersInstance, @@ -108,6 +110,18 @@ beforeAll(async () => { const member = await createUser(); authMemberAgent = testServer.authAgentFor(member); config.set('userManagement.isInstanceOwnerSetUp', true); + Container.set( + ExternalSecretsManager, + new ExternalSecretsManager( + logger, + Container.get(SettingsRepository), + Container.get(License), + mockProvidersInstance, + Container.get(Cipher), + eventService, + mock(), + ), + ); }); beforeEach(async () => {