From e994c51e45766bc8202dde4f13f386e9b84fab73 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Thu, 2 May 2024 12:48:06 +0200 Subject: [PATCH 1/3] fix(core): Prevent occassional 429s on license init in multi-main setup --- packages/cli/src/License.ts | 35 +++++++++++-- packages/cli/src/commands/BaseCommand.ts | 10 ++-- packages/cli/src/commands/start.ts | 2 + .../orchestration/main/MultiMainSetup.ee.ts | 10 +++- packages/cli/test/unit/License.test.ts | 51 +++++++++++++++++++ 5 files changed, 98 insertions(+), 10 deletions(-) diff --git a/packages/cli/src/License.ts b/packages/cli/src/License.ts index ce678939c8b97..436fd9d51ce2c 100644 --- a/packages/cli/src/License.ts +++ b/packages/cli/src/License.ts @@ -41,6 +41,27 @@ export class License { private readonly usageMetricsService: UsageMetricsService, ) {} + /** + * Whether this instance should renew the license - on init and periodically. + */ + private renewalEnabled() { + const isSingleMain = !config.getEnv('multiMainSetup.enabled'); + + if (isSingleMain) { + return ( + config.getEnv('generic.instanceType') === 'main' && + config.getEnv('license.autoRenewEnabled') + ); + } + + /** + * In multi-main setup, all mains start off with `unset` status and renewal disabled. + * On becoming leader or follower, each will enable or disable renewal, respectively. + * This ensures the mains do not cause a 429 (too many requests) on license init. + */ + return config.getEnv('multiMainSetup.instanceType') === 'leader'; + } + async init(instanceType: N8nInstanceType = 'main') { if (this.manager) { this.logger.warn('License manager already initialized or shutting down'); @@ -53,7 +74,6 @@ export class License { const isMainInstance = instanceType === 'main'; const server = config.getEnv('license.serverUrl'); - const autoRenewEnabled = isMainInstance && config.getEnv('license.autoRenewEnabled'); const offlineMode = !isMainInstance; const autoRenewOffset = config.getEnv('license.autoRenewOffset'); const saveCertStr = isMainInstance @@ -66,13 +86,15 @@ export class License { ? async () => await this.usageMetricsService.collectUsageMetrics() : async () => []; + const renewalEnabled = this.renewalEnabled(); + try { this.manager = new LicenseManager({ server, tenantId: config.getEnv('license.tenantId'), productIdentifier: `n8n-${N8N_VERSION}`, - autoRenewEnabled, - renewOnInit: autoRenewEnabled, + autoRenewEnabled: renewalEnabled, + renewOnInit: renewalEnabled, autoRenewOffset, offlineMode, logger: this.logger, @@ -126,7 +148,7 @@ export class License { if (this.orchestrationService.isMultiMainSetupEnabled && !isMultiMainLicensed) { this.logger.debug( - '[Multi-main setup] License changed with no support for multi-main setup - no new followers will be allowed to init. To restore multi-main setup, please upgrade to a license that supporst this feature.', + '[Multi-main setup] License changed with no support for multi-main setup - no new followers will be allowed to init. To restore multi-main setup, please upgrade to a license that supports this feature.', ); } } @@ -335,4 +357,9 @@ export class License { isWithinUsersLimit() { return this.getUsersLimit() === UNLIMITED_LICENSE_QUOTA; } + + async reinit() { + this.manager?.reset(); + await this.init(); + } } diff --git a/packages/cli/src/commands/BaseCommand.ts b/packages/cli/src/commands/BaseCommand.ts index faa5db165df43..e7721b33da00f 100644 --- a/packages/cli/src/commands/BaseCommand.ts +++ b/packages/cli/src/commands/BaseCommand.ts @@ -41,6 +41,8 @@ export abstract class BaseCommand extends Command { protected shutdownService: ShutdownService = Container.get(ShutdownService); + protected license: License; + /** * How long to wait for graceful shutdown before force killing the process. */ @@ -269,13 +271,13 @@ export abstract class BaseCommand extends Command { } async initLicense(): Promise { - const license = Container.get(License); - await license.init(this.instanceType ?? 'main'); + this.license = Container.get(License); + await this.license.init(this.instanceType ?? 'main'); const activationKey = config.getEnv('license.activationKey'); if (activationKey) { - const hasCert = (await license.loadCertStr()).length > 0; + const hasCert = (await this.license.loadCertStr()).length > 0; if (hasCert) { return this.logger.debug('Skipping license activation'); @@ -283,7 +285,7 @@ export abstract class BaseCommand extends Command { try { this.logger.debug('Attempting license activation'); - await license.activate(activationKey); + await this.license.activate(activationKey); this.logger.debug('License init complete'); } catch (e) { this.logger.error('Could not activate license', e as Error); diff --git a/packages/cli/src/commands/start.ts b/packages/cli/src/commands/start.ts index 3b7ee763a3244..74a60bf16b445 100644 --- a/packages/cli/src/commands/start.ts +++ b/packages/cli/src/commands/start.ts @@ -211,9 +211,11 @@ export class Start extends BaseCommand { orchestrationService.multiMainSetup .on('leader-stepdown', async () => { + await this.license.reinit(); // to disable renewal await this.activeWorkflowRunner.removeAllTriggerAndPollerBasedWorkflows(); }) .on('leader-takeover', async () => { + await this.license.reinit(); // to enable renewal await this.activeWorkflowRunner.addAllTriggerAndPollerBasedWorkflows(); }); } diff --git a/packages/cli/src/services/orchestration/main/MultiMainSetup.ee.ts b/packages/cli/src/services/orchestration/main/MultiMainSetup.ee.ts index eda788ae670ce..5f74f1931bd73 100644 --- a/packages/cli/src/services/orchestration/main/MultiMainSetup.ee.ts +++ b/packages/cli/src/services/orchestration/main/MultiMainSetup.ee.ts @@ -77,7 +77,10 @@ export class MultiMainSetup extends EventEmitter { config.set('multiMainSetup.instanceType', 'follower'); - this.emit('leader-stepdown'); // lost leadership - stop triggers, pollers, pruning + /** + * Lost leadership - stop triggers, pollers, pruning, wait tracking, license renewal + */ + this.emit('leader-stepdown'); await this.tryBecomeLeader(); } @@ -97,7 +100,10 @@ export class MultiMainSetup extends EventEmitter { await this.redisPublisher.setExpiration(this.leaderKey, this.leaderKeyTtl); - this.emit('leader-takeover'); // gained leadership - start triggers, pollers, pruning, wait-tracking + /** + * Gained leadership - start triggers, pollers, pruning, wait-tracking, license renewal + */ + this.emit('leader-takeover'); } else { config.set('multiMainSetup.instanceType', 'follower'); } diff --git a/packages/cli/test/unit/License.test.ts b/packages/cli/test/unit/License.test.ts index 354e672b0bfdb..d109fe633b703 100644 --- a/packages/cli/test/unit/License.test.ts +++ b/packages/cli/test/unit/License.test.ts @@ -175,3 +175,54 @@ describe('License', () => { expect(mainPlan).toBeUndefined(); }); }); + +describe('License', () => { + beforeEach(() => { + config.load(config.default); + }); + + describe('init', () => { + test('in single-main setup, should enable renewal', async () => { + config.set('multiMainSetup.enabled', false); + + await new License(mock(), mock(), mock(), mock(), mock()).init(); + + expect(LicenseManager).toHaveBeenCalledWith( + expect.objectContaining({ autoRenewEnabled: true, renewOnInit: true }), + ); + }); + + test('in multi-main setup, should disable renewal if unset', async () => { + config.set('multiMainSetup.enabled', true); + config.set('multiMainSetup.instanceType', 'unset'); + + await new License(mock(), mock(), mock(), mock(), mock()).init(); + + expect(LicenseManager).toHaveBeenCalledWith( + expect.objectContaining({ autoRenewEnabled: false, renewOnInit: false }), + ); + }); + + test('in multi-main setup, should enable renewal if leader', async () => { + config.set('multiMainSetup.enabled', true); + config.set('multiMainSetup.instanceType', 'leader'); + + await new License(mock(), mock(), mock(), mock(), mock()).init(); + + expect(LicenseManager).toHaveBeenCalledWith( + expect.objectContaining({ autoRenewEnabled: true, renewOnInit: true }), + ); + }); + + test('in multi-main setup, should disable renewal if follower', async () => { + config.set('multiMainSetup.enabled', true); + config.set('multiMainSetup.instanceType', 'follower'); + + await new License(mock(), mock(), mock(), mock(), mock()).init(); + + expect(LicenseManager).toHaveBeenCalledWith( + expect.objectContaining({ autoRenewEnabled: false, renewOnInit: false }), + ); + }); + }); +}); From 9107f09c657449f5d24a3495a5f22efcb4bfad3c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Thu, 2 May 2024 13:12:50 +0200 Subject: [PATCH 2/3] Fix worker test --- packages/cli/src/License.ts | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/packages/cli/src/License.ts b/packages/cli/src/License.ts index 436fd9d51ce2c..01c79fbdd03a6 100644 --- a/packages/cli/src/License.ts +++ b/packages/cli/src/License.ts @@ -44,22 +44,19 @@ export class License { /** * Whether this instance should renew the license - on init and periodically. */ - private renewalEnabled() { - const isSingleMain = !config.getEnv('multiMainSetup.enabled'); - - if (isSingleMain) { - return ( - config.getEnv('generic.instanceType') === 'main' && - config.getEnv('license.autoRenewEnabled') - ); - } + private renewalEnabled(instanceType: N8nInstanceType) { + if (instanceType !== 'main') return false; /** - * In multi-main setup, all mains start off with `unset` status and renewal disabled. + * In multi-main setup, all mains start off with `unset` status and so renewal disabled. * On becoming leader or follower, each will enable or disable renewal, respectively. * This ensures the mains do not cause a 429 (too many requests) on license init. */ - return config.getEnv('multiMainSetup.instanceType') === 'leader'; + if (config.getEnv('multiMainSetup.enabled')) { + return config.getEnv('multiMainSetup.instanceType') === 'leader'; + } + + return config.getEnv('license.autoRenewEnabled'); } async init(instanceType: N8nInstanceType = 'main') { @@ -86,7 +83,7 @@ export class License { ? async () => await this.usageMetricsService.collectUsageMetrics() : async () => []; - const renewalEnabled = this.renewalEnabled(); + const renewalEnabled = this.renewalEnabled(instanceType); try { this.manager = new LicenseManager({ From 9bc9a0f83d7f5a423ab1a4c61f10ad2099b296f1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Thu, 2 May 2024 14:22:17 +0200 Subject: [PATCH 3/3] Expand coverage --- packages/cli/src/License.ts | 6 +- packages/cli/test/unit/License.test.ts | 89 +++++++++++++++++--------- 2 files changed, 62 insertions(+), 33 deletions(-) diff --git a/packages/cli/src/License.ts b/packages/cli/src/License.ts index 01c79fbdd03a6..1e31222571b10 100644 --- a/packages/cli/src/License.ts +++ b/packages/cli/src/License.ts @@ -47,16 +47,18 @@ export class License { private renewalEnabled(instanceType: N8nInstanceType) { if (instanceType !== 'main') return false; + const autoRenewEnabled = config.getEnv('license.autoRenewEnabled'); + /** * In multi-main setup, all mains start off with `unset` status and so renewal disabled. * On becoming leader or follower, each will enable or disable renewal, respectively. * This ensures the mains do not cause a 429 (too many requests) on license init. */ if (config.getEnv('multiMainSetup.enabled')) { - return config.getEnv('multiMainSetup.instanceType') === 'leader'; + return autoRenewEnabled && config.getEnv('multiMainSetup.instanceType') === 'leader'; } - return config.getEnv('license.autoRenewEnabled'); + return autoRenewEnabled; } async init(instanceType: N8nInstanceType = 'main') { diff --git a/packages/cli/test/unit/License.test.ts b/packages/cli/test/unit/License.test.ts index d109fe633b703..182656f39a8f1 100644 --- a/packages/cli/test/unit/License.test.ts +++ b/packages/cli/test/unit/License.test.ts @@ -182,47 +182,74 @@ describe('License', () => { }); describe('init', () => { - test('in single-main setup, should enable renewal', async () => { - config.set('multiMainSetup.enabled', false); - - await new License(mock(), mock(), mock(), mock(), mock()).init(); - - expect(LicenseManager).toHaveBeenCalledWith( - expect.objectContaining({ autoRenewEnabled: true, renewOnInit: true }), - ); + describe('in single-main setup', () => { + describe('with `license.autoRenewEnabled` enabled', () => { + it('should enable renewal', async () => { + config.set('multiMainSetup.enabled', false); + + await new License(mock(), mock(), mock(), mock(), mock()).init(); + + expect(LicenseManager).toHaveBeenCalledWith( + expect.objectContaining({ autoRenewEnabled: true, renewOnInit: true }), + ); + }); + }); + + describe('with `license.autoRenewEnabled` disabled', () => { + it('should disable renewal', async () => { + config.set('license.autoRenewEnabled', false); + + await new License(mock(), mock(), mock(), mock(), mock()).init(); + + expect(LicenseManager).toHaveBeenCalledWith( + expect.objectContaining({ autoRenewEnabled: false, renewOnInit: false }), + ); + }); + }); }); - test('in multi-main setup, should disable renewal if unset', async () => { - config.set('multiMainSetup.enabled', true); - config.set('multiMainSetup.instanceType', 'unset'); + describe('in multi-main setup', () => { + describe('with `license.autoRenewEnabled` disabled', () => { + test.each(['unset', 'leader', 'follower'])( + 'if %s status, should disable removal', + async (status) => { + config.set('multiMainSetup.enabled', true); + config.set('multiMainSetup.instanceType', status); + config.set('license.autoRenewEnabled', false); - await new License(mock(), mock(), mock(), mock(), mock()).init(); + await new License(mock(), mock(), mock(), mock(), mock()).init(); - expect(LicenseManager).toHaveBeenCalledWith( - expect.objectContaining({ autoRenewEnabled: false, renewOnInit: false }), - ); - }); + expect(LicenseManager).toHaveBeenCalledWith( + expect.objectContaining({ autoRenewEnabled: false, renewOnInit: false }), + ); + }, + ); + }); - test('in multi-main setup, should enable renewal if leader', async () => { - config.set('multiMainSetup.enabled', true); - config.set('multiMainSetup.instanceType', 'leader'); + describe('with `license.autoRenewEnabled` enabled', () => { + test.each(['unset', 'follower'])('if %s status, should disable removal', async (status) => { + config.set('multiMainSetup.enabled', true); + config.set('multiMainSetup.instanceType', status); + config.set('license.autoRenewEnabled', false); - await new License(mock(), mock(), mock(), mock(), mock()).init(); + await new License(mock(), mock(), mock(), mock(), mock()).init(); - expect(LicenseManager).toHaveBeenCalledWith( - expect.objectContaining({ autoRenewEnabled: true, renewOnInit: true }), - ); - }); + expect(LicenseManager).toHaveBeenCalledWith( + expect.objectContaining({ autoRenewEnabled: false, renewOnInit: false }), + ); + }); - test('in multi-main setup, should disable renewal if follower', async () => { - config.set('multiMainSetup.enabled', true); - config.set('multiMainSetup.instanceType', 'follower'); + it('if leader status, should enable renewal', async () => { + config.set('multiMainSetup.enabled', true); + config.set('multiMainSetup.instanceType', 'leader'); - await new License(mock(), mock(), mock(), mock(), mock()).init(); + await new License(mock(), mock(), mock(), mock(), mock()).init(); - expect(LicenseManager).toHaveBeenCalledWith( - expect.objectContaining({ autoRenewEnabled: false, renewOnInit: false }), - ); + expect(LicenseManager).toHaveBeenCalledWith( + expect.objectContaining({ autoRenewEnabled: true, renewOnInit: true }), + ); + }); + }); }); }); });