fix(core): Make senderId required for all command messages #7252

flipswitchingmonkey · 2023-09-25T15:42:53Z

all commands sent between main instance and workers need to contain a server id to prevent senders from reacting to their own messages, causing loops

this PR makes sure all sent messages contain a sender id by default as part of constructing a sending redis client.

…is-messages

github-actions · 2023-09-25T15:43:06Z

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.

netroy · 2023-09-25T15:48:59Z

packages/cli/src/commands/BaseCommand.ts

 	protected server?: AbstractServer;

-	async init(): Promise<void> {
+	async init(instanceType?: N8nInstanceType): Promise<void> {


Can we make this an abstract property in BaseCommand? then each command can define this value before the constructor,and we don't need to wait until init to determine this value.

Moved setting the instance type and queue mode id into the constructors

packages/cli/src/config/schema.ts

codecov · 2023-09-25T16:04:46Z

Codecov Report

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

Comparison is base (77d6e3f) 32.93% compared to head (624566a) 32.93%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #7252   +/-   ##
=======================================
  Coverage   32.93%   32.93%           
=======================================
  Files        3355     3355           
  Lines      200130   200139    +9     
  Branches    21944    21938    -6     
=======================================
+ Hits        65910    65914    +4     
- Misses     133109   133117    +8     
+ Partials     1111     1108    -3

Files	Coverage Δ
packages/cli/src/commands/worker.ts	`25.60% <100.00%> (+0.72%)`	⬆️
packages/cli/src/config/schema.ts	`81.81% <ø> (ø)`
...li/src/eventbus/MessageEventBus/MessageEventBus.ts	`42.39% <100.00%> (+2.65%)`	⬆️
.../MessageEventBusWriter/MessageEventBusLogWriter.ts	`54.36% <100.00%> (ø)`
.../cli/src/services/redis/RedisServiceBaseClasses.ts	`70.00% <100.00%> (+5.71%)`	⬆️
...s/cli/src/services/redis/RedisServiceListSender.ts	`50.00% <100.00%> (-2.95%)`	⬇️
.../src/services/redis/RedisServicePubSubPublisher.ts	`61.11% <100.00%> (+2.28%)`	⬆️
packages/cli/src/AbstractServer.ts	`48.57% <0.00%> (ø)`
packages/cli/src/Server.ts	`0.00% <0.00%> (ø)`
packages/cli/src/services/orchestration.service.ts	`62.85% <50.00%> (+7.53%)`	⬆️
... and 7 more

... and 4 files with indirect coverage changes

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

…id-required-for-all-redis-messages

netroy

LGTM. Thanks 🙏🏽

netroy · 2023-09-26T09:39:11Z

packages/cli/src/commands/BaseCommand.ts

 		const license = Container.get(License);
-		await license.init(this.instanceId, instanceType);
+		await license.init(this.instanceId, this.instanceType ?? 'main');


This was why I wanted to create a ServerCommand class, so that all cli commands don't end up using instanceType='main'. But that can be fixed in another PR.

netroy · 2023-09-26T09:40:43Z

packages/cli/src/commands/start.ts

@@ -65,6 +66,12 @@ export class Start extends BaseCommand {

 	protected server = new Server();

+	constructor(argv: string[], cmdConfig: IConfig) {
+		super(argv, cmdConfig);
+		this.setInstanceType('main');


when I suggested using an abstract property, I meant that we can then overwrite the property in the derived class without having to override the constructor like this.
but for now this work fine.

netroy · 2023-09-26T09:43:25Z

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

 			case 'reloadLicense':
-				await Container.get(License).reload();
+				// 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)
+				LoggerProxy.warn(
+					'Received command to reload license via Redis, but this should not have happened and is not supported on the main instance yet.',
+				);
+				// once multiple main instances are supported, this command should be handled
+				// await Container.get(License).reload();


code like this should stay in a WIP branch, instead of being merged in master.

netroy · 2023-09-26T09:45:58Z

packages/cli/src/services/redis/RedisServiceCommands.ts

@@ -12,7 +12,7 @@ export type RedisServiceCommand =
 * @field payload: Optional arguments to be sent with the command.
 */
 type RedisServiceBaseCommand = {
-	senderId?: string;
+	senderId: string;


should we perhaps remove this from here? this property gives an impression that whenever we are publishing a command, we can include a senderId, which isn't true. not to mention, we have to do Omit<RedisServiceCommandObject, 'senderId'> to block people from adding a senderId in the payload.

I could go the inverse and remove it here and have another interface with added senderId

cypress · 2023-09-26T10:02:09Z

2 flaky tests on run #2271 ↗︎

0	242	3	0	2

Details:

🌳 pay-847-make-senderid-required-for-all-redis-messages 🖥️ browsers:node18.12....
Project: n8n	Commit: `624566a251`
Status: Passed	Duration: 08:13 💡
Started: Sep 26, 2023 9:53 AM	Ended: Sep 26, 2023 10:02 AM

2-credentials.cy.ts • 1 flaky test

View Output Video

Test		Artifacts
Credentials > should create a new credential using Add Credential button		`Output` `Screenshots` `Video`

28-debug.cy.ts • 1 flaky test

View Output Video

Test		Artifacts
Debug > should be able to debug executions		`Output` `Screenshots` `Video`

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

github-actions · 2023-09-26T10:02:13Z

✅ All Cypress E2E specs passed

csuermann · 2023-09-26T11:11:34Z

packages/cli/src/License.ts

@@ -64,6 +67,7 @@ export class License {
 				loadCertStr: async () => this.loadCertStr(),
 				saveCertStr,
 				deviceFingerprint: () => instanceId,
+				onFeatureChange,


didn't we want to set offlineMode to true on workers?

const offlineMode = !isMainInstance;

ah, sorry, I didn't realize that GitHub was hiding these lines 🤦‍♂️

# [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:04Z

Got released with n8n@1.9.0

flipswitchingmonkey added 6 commits September 25, 2023 13:56

make queueModeId generally available

7157b72

Merge branch 'master' into pay-847-make-senderid-required-for-all-red…

5110e9d

…is-messages

fix double event handling

5b3f225

remove console log

5042361

lint fix

0af97b4

fix tests

45e34cb

flipswitchingmonkey marked this pull request as ready for review September 25, 2023 15:44

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

netroy requested changes Sep 25, 2023

View reviewed changes

flipswitchingmonkey added 2 commits September 26, 2023 07:30

remove env var

f30eae3

set queue mode id in constructor

38c8671

flipswitchingmonkey requested a review from netroy September 26, 2023 06:04

flipswitchingmonkey added 2 commits September 26, 2023 08:26

lint fix (╯°□°)╯︵ ┻━┻

189c525

lock fix

9513f5f

flipswitchingmonkey requested a review from krynble September 26, 2023 07:56

Merge remote-tracking branch 'origin/master' into pay-847-make-sender…

624566a

…id-required-for-all-redis-messages

netroy approved these changes Sep 26, 2023

View reviewed changes

csuermann reviewed Sep 26, 2023

View reviewed changes

flipswitchingmonkey merged commit 4b01428 into master Sep 26, 2023
26 checks passed

flipswitchingmonkey deleted the pay-847-make-senderid-required-for-all-redis-messages branch September 26, 2023 11:58

This was referenced Sep 27, 2023

🚀 Release 1.9.0 #7280

Closed

🚀 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

fix(core): Make senderId required for all command messages #7252

fix(core): Make senderId required for all command messages #7252

flipswitchingmonkey commented Sep 25, 2023

github-actions bot commented Sep 25, 2023

netroy Sep 25, 2023 •

edited

Loading

flipswitchingmonkey Sep 26, 2023

codecov bot commented Sep 25, 2023 •

edited

Loading

netroy left a comment

netroy Sep 26, 2023 •

edited

Loading

netroy Sep 26, 2023

netroy Sep 26, 2023

netroy Sep 26, 2023

flipswitchingmonkey Sep 26, 2023

cypress bot commented Sep 26, 2023

github-actions bot commented Sep 26, 2023

csuermann Sep 26, 2023

flipswitchingmonkey Sep 26, 2023

csuermann Sep 26, 2023

janober commented Sep 28, 2023

fix(core): Make senderId required for all command messages #7252

fix(core): Make senderId required for all command messages #7252

Conversation

flipswitchingmonkey commented Sep 25, 2023

github-actions bot commented Sep 25, 2023

netroy Sep 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 25, 2023 • edited Loading

Codecov Report

netroy left a comment

Choose a reason for hiding this comment

netroy Sep 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cypress bot commented Sep 26, 2023

2 flaky tests on run #2271 ↗︎

github-actions bot commented Sep 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janober commented Sep 28, 2023

netroy Sep 25, 2023 •

edited

Loading

codecov bot commented Sep 25, 2023 •

edited

Loading

netroy Sep 26, 2023 •

edited

Loading