feat(core): Add secrets provider reload and refactor #7277

flipswitchingmonkey · 2023-09-27T11:54:20Z

This PR adds a message for queue mode which triggers an external secrets provider reload inside the workers if the configuration has changed on the main instance.

It also refactors some of the message handler code to remove cyclic dependencies, as well as remove unnecessary duplicate redis clients inside services (thanks to no more cyclic deps)

github-actions · 2023-09-27T11:54:35Z

Great PR! Please pay attention to the following items before merging:

Files matching packages/**:

If fixing bug, added test to cover scenario.
If addressing forum or Github issue, added link to description.

Files matching packages/**/*.ts:

Added unit tests to cover new or updated functionality.

Make sure to check off this list before asking for review.

mutdmour · 2023-09-27T12:04:22Z

packages/cli/src/services/orchestration.handler.service.ts

+import { MessageEventBus } from '../eventbus/MessageEventBus/MessageEventBus';
+
+@Service()
+export class OrchestrationHandlerService {


would be great if you can please add tests and coverage for all this new functionality

thats all part of the orchestration service tests. it's just refactored and split in two to get rid of the dependency cycles.

thanks for explaining.. sorry not enough context to review PR completely but just making sure we are adding tests

codecov · 2023-09-27T12:14:17Z

Codecov Report

Attention: 42 lines in your changes are missing coverage. Please review.

Comparison is base (5c57e2c) 32.98% compared to head (ccea1d2) 32.98%.
Report is 6 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7277      +/-   ##
==========================================
- Coverage   32.98%   32.98%   -0.01%     
==========================================
  Files        3358     3359       +1     
  Lines      200231   200279      +48     
  Branches    21978    21978              
==========================================
+ Hits        66055    66069      +14     
- Misses     133067   133103      +36     
+ Partials     1109     1107       -2

Files	Coverage Δ
...li/src/eventbus/MessageEventBus/MessageEventBus.ts	`44.11% <100.00%> (+1.72%)`	⬆️
packages/cli/src/AbstractServer.ts	`48.59% <50.00%> (+0.02%)`	⬆️
...src/services/orchestration/handleCommandMessage.ts	`71.42% <81.81%> (-7.52%)`	⬇️
packages/cli/src/commands/BaseCommand.ts	`51.02% <40.00%> (-0.50%)`	⬇️
...i/src/ExternalSecrets/ExternalSecretsManager.ee.ts	`72.82% <46.15%> (-1.91%)`	⬇️
.../cli/src/services/orchestration.handler.service.ts	`64.00% <64.00%> (ø)`
packages/cli/src/services/orchestration.service.ts	`56.41% <47.36%> (-6.45%)`	⬇️
packages/cli/src/worker/workerCommandHandler.ts	`21.42% <9.09%> (-2.82%)`	⬇️

... and 6 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

krynble

There's something a bit odd happening on the subscribe to command channel - I've made a video and I'm not sure how to replicate this, but there's something weird happening.

I'd like to ask you to add more log messages to what's happening under the hood in n8n so we can better diagnose this. Some of them can be info messages, as long as we don't flood the terminal. It's good to know what's happening and when.

Here's a link to the recording: https://www.loom.com/share/2c859bc7418a46cd8fba1785c4f4e8cc

krynble · 2023-09-27T14:34:16Z

packages/cli/src/services/orchestration/handleCommandMessage.ts

@@ -21,16 +22,15 @@ export async function handleCommandMessage(messageString: string) {
 		}
 		switch (message.command) {
 			case 'reloadLicense':
-				// at this point in time, only a single main instance is supported, thus this
-				// command _should_ never be caught currently (which is why we log a warning)
+				// at this point in time, only a single main instance is supported, thus this command _should_ never be caught currently
 				LoggerProxy.warn(


Suggested change

LoggerProxy.warn(

LoggerProxy.error(

Since this shouldn't happen because it indicates a faulty setup, I think we can display as error

krynble · 2023-09-27T14:44:11Z

packages/cli/src/services/orchestration.service.ts


 @Service()
 export class OrchestrationService {
 	private initialized = false;

 	redisPublisher: RedisServicePubSubPublisher;

-	redisSubscriber: RedisServicePubSubSubscriber;
+	get isQueueMode() {
+		return config.getEnv('executions.mode') === 'queue';


This is a tricky test to make - the worker.ts command does not require the environment variable (it does not check it). Only the webhook.ts one does.

So this is potentially a breaking change, if someone is running queue mode without setting the environment variable for workers (this shouldn't happen since environment variables should be the same for all containers, but...).

Given this check does not exist, we have two options:

Add the check to worker.ts just like webhook.ts so that we have a standardized process or

Check for the instanceType variable that is part of the BaseCommand abstract class

krynble

This part of the code is working fine, I think we have 2 outstanding issues:

The webhook processes are subscribing to worker response channels and I'm not sure if we should do this. This screenshot below shows a webhook getting a response from a worker success message. I guess only main should get this, not webhook so maybe this subscription should be avoided.

Disabling external secrets does not clean up already loaded secrets in memory, I mean, this action should be a full reload, including cleanup. To be discussed with @valya

cypress · 2023-09-28T08:50:26Z

Passing run #2309 ↗︎

0	242	3	0	0

Details:

🌳 pay-830-changes-to-external-secrets-settings 🖥️ browsers:node18.12.0-chrome1...
Project: n8n	Commit: `ccea1d2c31`
Status: Passed	Duration: 08:21 💡
Started: Sep 28, 2023 10:09 AM	Ended: Sep 28, 2023 10:17 AM

_{This comment has been generated by cypress-bot as a result of this
project's GitHub integration settings.}

github-actions · 2023-09-28T08:50:30Z

✅ All Cypress E2E specs passed

github-actions · 2023-09-28T10:17:56Z

✅ All Cypress E2E specs passed

# [1.9.0](https://github.com/n8n-io/n8n/compare/n8n@1.8.0...n8n@1.9.0) (2023-09-28) ### Bug Fixes * **Airtable Node:** Attachments field type fix ([#7227](#7227)) ([2af967c](2af967c)) * **core:** Change WorkflowHistory nodes/connections columns to be json ([#7282](#7282)) ([a80abad](a80abad)) * **core:** Fix binary data manager check on pruning ([#7251](#7251)) ([484035e](484035e)) * **core:** Fix missing execution ID in webhook-based workflow producing binary data ([#7244](#7244)) ([33991e9](33991e9)) * **core:** Handle filename* with quotes in Content-Disposition header ([#7229](#7229)) ([67b985f](67b985f)) * **core:** Make DNS resolution order configurable ([#7272](#7272)) ([5b3121c](5b3121c)) * **core:** Make senderId required for all command messages ([#7252](#7252)) ([4b01428](4b01428)) * **core:** Prevent executions from displaying Running status incorrectly ([#7261](#7261)) ([861cac5](861cac5)) * **core:** Use consistent timezone-aware timestamps in postgres ([#6948](#6948)) ([0132514](0132514)), closes [#2178](#2178) [#2810](#2810) [#3855](#3855) [#2813](#2813) * **editor:** Add debug feature docs link ([#7240](#7240)) ([4614e1e](4614e1e)) * **editor:** Fix SQL editor issue ([#7236](#7236)) ([647fc6c](647fc6c)) * **editor:** Ensure new Set node is on top of search list ([#7215](#7215)) ([2491ccf](2491ccf)) * **editor:** Forbid password reset when cloud account is limited in the number of users [7188](#7188) ([303bc8e](303bc8e)) * **HTTP Request Node:** Add suggestion how to fix '429 - too many requests' errors ([#7293](#7293)) ([0bc33b1](0bc33b1)) * **Item Lists Node:** Concatenate operation pairedItems fix ([#7286](#7286)) ([cde23a1](cde23a1)) * **Respond to Webhook Node:** JSON output from expression fix ([#7294](#7294)) ([8bc369d](8bc369d)) ### Features * Add onboarding flow ([#7212](#7212)) ([01e9340](01e9340)) * **core:** Add secrets provider reload and refactor ([#7277](#7277)) ([53a7502](53a7502)) * **core:** Add Tournament as the new default expression evaluator ([#6964](#6964)) ([bf74f09](bf74f09)) * **core:** Initial workflow history API ([#7234](#7234)) ([0083a9e](0083a9e)) * **core:** Introduce object store service ([#7225](#7225)) ([fa84545](fa84545)) * **editor:** Add user cloud ID to telemetry [#7232](#7232) ([60c152d](60c152d)) * **editor:** Rework banners framework and add email confirmation banner ([#7205](#7205)) ([b0e98b5](b0e98b5)) * **MISP Node:** Update credential to support HTTP Request node ([#7268](#7268)) ([e4c302c](e4c302c)) ### Performance Improvements * **core:** Skip unneeded calls on every pruning cycle ([#7260](#7260)) ([db01164](db01164)) Co-authored-by: netroy <netroy@users.noreply.github.com>

janober · 2023-09-28T16:19:25Z

Got released with n8n@1.9.0

* master: fix(editor): Separate cloud endpoint calls (#7312) fix(core): Account for itemless case on restoring binary data ID (#7305) feat(editor): Workflow history [WIP]- create workflow history list component (no-changelog) (#7186) feat(core): Add plan name to telemetry (no-changelog) (#7296) ci: Fix typescript incremental builds (no-changelog) (#7275) fix(Postgres Node): Node requires comma-separated string even when using a single parameter through an expression (#7300) fix(Notion Node): Rename Notion API Key to Internal Integration Token (#7176) 🚀 Release 1.9.0 (#7288) ci: Refactor DB tests (no-changelog) (#7292) fix(HTTP Request Node): Add suggestion how to fix '429 - too many requests' errors (#7293) fix(Respond to Webhook Node): JSON output from expression fix (#7294) feat(core): Add secrets provider reload and refactor (#7277)

add secrets provider reload and refactor

226f01f

n8n-assistant bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team labels Sep 27, 2023

cleanup and error handling

7ced020

flipswitchingmonkey marked this pull request as ready for review September 27, 2023 12:01

mutdmour reviewed Sep 27, 2023

View reviewed changes

flipswitchingmonkey requested a review from mutdmour September 27, 2023 12:16

flipswitchingmonkey added 2 commits September 27, 2023 15:09

Merge branch 'master' into pay-830-changes-to-external-secrets-settings

598ce09

bump license sdk

ab57307

flipswitchingmonkey requested a review from krynble September 27, 2023 14:14

krynble reviewed Sep 27, 2023

View reviewed changes

improve handling on non-main non-workers

305de7e

krynble previously approved these changes Sep 28, 2023

View reviewed changes

fix test

ccea1d2

flipswitchingmonkey dismissed krynble’s stale review via ccea1d2 September 28, 2023 10:02

krynble approved these changes Sep 28, 2023

View reviewed changes

flipswitchingmonkey merged commit 53a7502 into master Sep 28, 2023
18 checks passed

flipswitchingmonkey deleted the pay-830-changes-to-external-secrets-settings branch September 28, 2023 10:57

github-actions bot mentioned this pull request Sep 28, 2023

🚀 Release 1.9.0 #7288

Merged

janober added the Released label Sep 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core): Add secrets provider reload and refactor #7277

feat(core): Add secrets provider reload and refactor #7277

flipswitchingmonkey commented Sep 27, 2023

github-actions bot commented Sep 27, 2023

mutdmour Sep 27, 2023

flipswitchingmonkey Sep 27, 2023

mutdmour Sep 27, 2023

codecov bot commented Sep 27, 2023 •

edited

Loading

krynble left a comment •

edited

Loading

krynble Sep 27, 2023

krynble Sep 27, 2023

krynble left a comment

cypress bot commented Sep 28, 2023 •

edited

Loading

github-actions bot commented Sep 28, 2023

github-actions bot commented Sep 28, 2023

janober commented Sep 28, 2023

feat(core): Add secrets provider reload and refactor #7277

feat(core): Add secrets provider reload and refactor #7277

Conversation

flipswitchingmonkey commented Sep 27, 2023

github-actions bot commented Sep 27, 2023

mutdmour Sep 27, 2023

Choose a reason for hiding this comment

flipswitchingmonkey Sep 27, 2023

Choose a reason for hiding this comment

mutdmour Sep 27, 2023

Choose a reason for hiding this comment

codecov bot commented Sep 27, 2023 • edited Loading

Codecov Report

krynble left a comment • edited Loading

Choose a reason for hiding this comment

krynble Sep 27, 2023

Choose a reason for hiding this comment

krynble Sep 27, 2023

Choose a reason for hiding this comment

krynble left a comment

Choose a reason for hiding this comment

cypress bot commented Sep 28, 2023 • edited Loading

Passing run #2309 ↗︎

github-actions bot commented Sep 28, 2023

github-actions bot commented Sep 28, 2023

janober commented Sep 28, 2023

codecov bot commented Sep 27, 2023 •

edited

Loading

krynble left a comment •

edited

Loading

cypress bot commented Sep 28, 2023 •

edited

Loading