feat(microservice): add TLS over TCP support #7516

Flusinerd · 2021-07-11T12:10:15Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/nestjs/nest/blob/master/CONTRIBUTING.md
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?
Adds an additional property useTls?: boolean to the options for client and server. If useTls is set, it intersects the Nodes tls server / client options with the nest server/client options.

If useTls is set to true, it uses node's tls module instead of net to allow TCP over TLS support

[ ] Bugfix
[ X ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: #6745

What is the new behavior?

If useTls === true then it will use tls instead of net to create the server / client and intersect the appropriate options with the nest options

Does this PR introduce a breaking change?

[ ] Yes
[ X ] No

Other information

add tcp tls client options add tcp tls server options add tcp over tls support

… into feat/microservice-tcp-tls

fix client-tcp not beeing able to handle empty options

fix isCustomClientOptions type-guard not beeing able to handle undefined

Flusinerd · 2021-07-11T12:15:03Z

For docs, should I update the docs in nestjs/docs.nestjs.com?

I think we could either add another section in:
https://docs.nestjs.com/microservices/basics#client

Or make an own page for TCP Client or TCP TLS Client

coveralls · 2021-07-11T12:17:05Z

Pull Request Test Coverage Report for Build a18b5f60-086f-4c47-a111-2d294565e81c

25 of 33 (75.76%) changed or added relevant lines in 4 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.03%) to 94.103%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
packages/microservices/client/client-tcp.ts	15	23	65.22%

Totals
Change from base Build 38eae0db-18bc-4193-8057-69d114a8416f:	-0.03%
Covered Lines:	5713
Relevant Lines:	6071

💛 - Coveralls

add TcpTlsOptions to MicroserviceOptions type

kamilmysliwiec · 2021-07-12T06:48:50Z

For docs, should I update the docs in nestjs/docs.nestjs.com?

Correct

I think we could either add another section in:

Let's add a new section in the TCP chapter

fix client not connecting when using tls

Flusinerd · 2021-07-12T10:48:23Z

Docs have been updated as well, the PR is linked above

Flusinerd · 2021-07-12T10:50:23Z

I can add a sample later that I used for testing, should I include a self-signed certificate in the sample?
I'm waiting for #7486 to merged, since this includes support for multi-application samples.

kamilmysliwiec · 2021-07-27T10:31:27Z

packages/microservices/client/client-proxy-factory.ts

-        return new ClientTCP(options as TcpClientOptions['options']);
+        const uncheckedOptions = options as
+          | TcpClientOptions['options']
+          | TcpTlsClientOptions['options']
+          | undefined;
+        if (uncheckedOptions && uncheckedOptions.useTls === true) {
+          return new ClientTCP(options as TcpTlsClientOptions['options']);
+        } else {
+          return new ClientTCP(
+            options as TcpClientOptions['options'] | undefined,
+          );
+        }


As this condition seems to be used only for type-checking purposes, can we just force cast options to the appropriate type and remove this if?

kamilmysliwiec · 2021-07-27T10:32:13Z

packages/microservices/client/client-tcp.ts

+    if (options === undefined) {
+      this.options = {};
+    }


What's the reason for this change?

kamilmysliwiec · 2021-07-27T10:32:47Z

packages/microservices/test/client/client-tcp.spec.ts

+
+  // describe('tls', () => {
+  //   it('should upgrade to TLS', () => {
+  //     const jsonSocket = new ClientTCP({ useTls: true }).createSocket();
+  //     expect(jsonSocket.socket).instanceOf(TLSSocket);
+  //   });
+  //   it('should not upgrade to TLS, if not requested', () => {
+  //     const jsonSocket = new ClientTCP({ useTls: false }).createSocket();
+  //     expect(jsonSocket.socket).instanceOf(NetSocket);
+  //   });
+  // });


Is this intentionally commented out?

kamilmysliwiec · 2021-07-27T10:33:03Z

I left a few comments. Can we also add an integration test?

Flusinerd · 2021-07-30T08:32:11Z

Hi, ty for the the feedback I'll try work on this this weekend.

AnisJS09 · 2021-08-07T10:09:33Z

packages/microservices/client/client-proxy.ts

-          | KafkaOptions['options']).serializer) ||
+        (
+          options as
+            | RedisOptions['options']


what do you think about extracting these types into a Union type declared, something like :

Suggested change

| RedisOptions['options']

const type Options = NatsOptions['options']

| MqttOptions['options']

| TcpClientOptions['options']

| TcpTlsClientOptions['options']

| RmqOptions['options']

| KafkaOptions['options'];

Flusinerd · 2021-08-07T10:12:12Z

Currently I dont have that much time to work on this @AnisJS09 Do you want to continue?

Flusinerd · 2022-01-16T23:44:30Z

Okay, I managed to get some time to continue working on this.
I worked on the review from @kamilmysliwiec and think I got it sorted.

Looking into integration testing right now. It seems like there are no e2e tests for TCP implemented atm.
So what do you want to be covered for TCP clients?
Just a simple addition using sync / async / rxjs of and rxjs from + exception and pub / sub.
Similar to how its done in the nats example?

kamilmysliwiec · 2022-01-17T07:50:17Z

TCP tests https://github.com/nestjs/nest/blob/master/integration/microservices/e2e/sum-rpc.spec.ts

Flusinerd · 2022-01-17T10:52:22Z

Ahh alright. I completely missed that. Will work on this later then and finish this feature request.

add integration tests

Flusinerd · 2022-01-17T12:56:36Z

Okay, I added the e2e tests.
I included a self signed certificate.

integration/microservices/e2e/sum-rpc-tls.spec.ts

kamilmysliwiec · 2022-03-01T13:27:18Z

packages/microservices/client/client-tcp.ts

+    if (this.useTls) {
+      this.netSocket.connect(this.port, this.host);
+      source$ = this.connect$(
+        this.socket.netSocket,
+        ERROR_EVENT,
+        TLS_CONNECT_EVENT,
+      ).pipe(
+        tap(() => {
+          this.isConnected = true;
+          this.socket.on(MESSAGE_EVENT, (buffer: WritePacket & PacketId) =>
+            this.handleResponse(buffer),
+          );
+        }),
+        share(),
+      );
+    } else {
+      source$ = this.connect$(this.socket.netSocket).pipe(
+        tap(() => {
+          this.isConnected = true;
+          this.socket.on(MESSAGE_EVENT, (buffer: WritePacket & PacketId) =>
+            this.handleResponse(buffer),
+          );
+        }),
+        share(),
+      );
+      this.socket.connect(this.port, this.host);
+    }


If the only difference here is whether to use netSocket or socket (depending on whether TLS is on) we should probably refactor this logic to avoid repetitiveness.

nomaxg · 2022-10-28T19:51:20Z

@Flusinerd @kamilmysliwiec is this actively being worked on? I would be happy to pick it up

Flusinerd · 2022-10-28T19:55:38Z

Hi. I don't think so. Feel free to continue

kamilmysliwiec · 2023-02-01T12:04:34Z

Moving onto this PR #10628

Flusinerd added 5 commits July 11, 2021 01:33

feat(microservice): add tcp over tls support

8bbf128

add tcp tls client options add tcp tls server options add tcp over tls support

feat(microservice): add tcp over tls support

567c902

add tcp tls client options add tcp tls server options add tcp over tls support

Merge branch 'feat/microservice-tcp-tls' of github.com:Flusinerd/nest…

b2883c7

… into feat/microservice-tcp-tls

feat(microservice): add tcp over tls support

ae124a9

fix client-tcp not beeing able to handle empty options

feat(microservice): add tcp over tls support

0cf63a3

fix isCustomClientOptions type-guard not beeing able to handle undefined

feat(microservice): add tcp over tls support

18896a5

add TcpTlsOptions to MicroserviceOptions type

feat(microservice): add tcp over tls support

18eb60d

fix client not connecting when using tls

Flusinerd mentioned this pull request Jul 12, 2021

feat(microservice): add TLS over TCP support nestjs/docs.nestjs.com#1961

Merged

10 tasks

Flusinerd changed the title ~~feat(microservice): add TCP over TLS support~~ feat(microservice): add TLS over TCP support Jul 17, 2021

kamilmysliwiec mentioned this pull request Jul 27, 2021

Support TCP microservice over TLS #6745

Closed

kamilmysliwiec reviewed Jul 27, 2021

View reviewed changes

AnisJS09 reviewed Aug 7, 2021

View reviewed changes

kamilmysliwiec added scope: microservices type: feature 🎉 status: wip ⚡️ labels Sep 28, 2021

Merge branch 'master' into feat/microservice-tcp-tls

4dd99be

feat(microservice): add tcp over tls support

2d49664

feat(microservice): add tcp over tls support

a6e2b5f

add integration tests

kamilmysliwiec reviewed Mar 1, 2022

View reviewed changes

integration/microservices/e2e/sum-rpc-tls.spec.ts Outdated Show resolved Hide resolved

Update integration/microservices/e2e/sum-rpc-tls.spec.ts

4589984

kamilmysliwiec reviewed Mar 1, 2022

View reviewed changes

kamilmysliwiec closed this Feb 1, 2023

Flusinerd deleted the feat/microservice-tcp-tls branch July 3, 2023 16:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(microservice): add TLS over TCP support #7516

feat(microservice): add TLS over TCP support #7516

Flusinerd commented Jul 11, 2021 •

edited

Loading

Flusinerd commented Jul 11, 2021 •

edited

Loading

coveralls commented Jul 11, 2021 •

edited

Loading

kamilmysliwiec commented Jul 12, 2021

Flusinerd commented Jul 12, 2021

Flusinerd commented Jul 12, 2021

kamilmysliwiec Jul 27, 2021

kamilmysliwiec Jul 27, 2021

kamilmysliwiec Jul 27, 2021

kamilmysliwiec commented Jul 27, 2021

Flusinerd commented Jul 30, 2021

AnisJS09 Aug 7, 2021

Flusinerd commented Aug 7, 2021

Flusinerd commented Jan 16, 2022

kamilmysliwiec commented Jan 17, 2022

Flusinerd commented Jan 17, 2022

Flusinerd commented Jan 17, 2022

kamilmysliwiec Mar 1, 2022

nomaxg commented Oct 28, 2022

Flusinerd commented Oct 28, 2022

kamilmysliwiec commented Feb 1, 2023

-            | RedisOptions['options']
+            const type Options = NatsOptions['options']
+            | MqttOptions['options']
+            | TcpClientOptions['options']
+            | TcpTlsClientOptions['options']
+            | RmqOptions['options']
+            | KafkaOptions['options'];

feat(microservice): add TLS over TCP support #7516

feat(microservice): add TLS over TCP support #7516

Conversation

Flusinerd commented Jul 11, 2021 • edited Loading

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Flusinerd commented Jul 11, 2021 • edited Loading

coveralls commented Jul 11, 2021 • edited Loading

Pull Request Test Coverage Report for Build a18b5f60-086f-4c47-a111-2d294565e81c

💛 - Coveralls

kamilmysliwiec commented Jul 12, 2021

Flusinerd commented Jul 12, 2021

Flusinerd commented Jul 12, 2021

kamilmysliwiec Jul 27, 2021

Choose a reason for hiding this comment

kamilmysliwiec Jul 27, 2021

Choose a reason for hiding this comment

kamilmysliwiec Jul 27, 2021

Choose a reason for hiding this comment

kamilmysliwiec commented Jul 27, 2021

Flusinerd commented Jul 30, 2021

AnisJS09 Aug 7, 2021

Choose a reason for hiding this comment

Flusinerd commented Aug 7, 2021

Flusinerd commented Jan 16, 2022

kamilmysliwiec commented Jan 17, 2022

Flusinerd commented Jan 17, 2022

Flusinerd commented Jan 17, 2022

kamilmysliwiec Mar 1, 2022

Choose a reason for hiding this comment

nomaxg commented Oct 28, 2022

Flusinerd commented Oct 28, 2022

kamilmysliwiec commented Feb 1, 2023

Flusinerd commented Jul 11, 2021 •

edited

Loading

Flusinerd commented Jul 11, 2021 •

edited

Loading

coveralls commented Jul 11, 2021 •

edited

Loading