Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(microservice): TCP microservice over TLS #10628

Merged
merged 2 commits into from Apr 5, 2023
Merged

feat(microservice): TCP microservice over TLS #10628

merged 2 commits into from Apr 5, 2023

Conversation

nomaxg
Copy link
Contributor

@nomaxg nomaxg commented Dec 2, 2022 •
edited

Builds off of @Flusinerd's work to add the option to connect to a TCP microservice over TLS.

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

Adds the ability to pass through TLS options to both the client and the server by means of an optional tlsOptions property.

  • 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 tlsOptions are specified, a tls server will be instantiated server-side, and a TLSSocket will be used to connect to the server client-side. Under the hood, options are passed down to the Node tls library.

Does this PR introduce a breaking change?

  • Yes
  • [ x] No

Other information

@nomaxg
feat(microservice): add tcp over tls support
b9c235a 
Signed-off-by: nomaxg <noahgolub2@gmail.com>
@coveralls
Copy link

coveralls commented Dec 2, 2022 •
edited

Pull Request Test Coverage Report for Build ab5aa6c9-6cd1-41e3-99b5-fb1f16ae7606

  • 12 of 13 (92.31%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.01%) to 93.414%
Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/microservices/server/server-tcp.ts 4 5 80.0%
Totals Coverage Status
Change from base Build e91ba0bb-da30-4334-a0c2-615d1d5bb062: 0.01%
Covered Lines: 6212
Relevant Lines: 6650
💛 - Coveralls

@nomaxg
Copy link
Contributor Author

nomaxg commented Dec 9, 2022

I'm not sure how to address the samples failure, the failing sample appears to build and run fine for me locally. Any suggestions?

Flusinerd
Flusinerd suggested changes Dec 9, 2022
View reviewed changes
Copy link
Contributor

@Flusinerd Flusinerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like you mixed ConnectionOptions and TlsOptions

packages/microservices/client/client-tcp.ts Show resolved Hide resolved
packages/microservices/interfaces/microservice-configuration.interface.ts Outdated Show resolved Hide resolved
packages/microservices/interfaces/microservice-configuration.interface.ts Outdated Show resolved Hide resolved
@nomaxg
Copy link
Contributor Author

nomaxg commented Jan 4, 2023

Seems like you mixed ConnectionOptions and TlsOptions

thanks! updated

@nomaxg nomaxg requested a review from Flusinerd January 4, 2023 18:55
@nomaxg
Copy link
Contributor Author

nomaxg commented Jan 4, 2023

Seems like you mixed ConnectionOptions and TlsOptions

thanks! updated

Actually I believe that the configuration was correct before - ClientTCP should use ConnectionOptions in the call to tls.connect

@kamilmysliwiec kamilmysliwiec mentioned this pull request Feb 1, 2023
Closed
3 tasks
@kamilmysliwiec
Copy link
Member

samples workflow is now failing with the following error:

image

kamilmysliwiec
kamilmysliwiec reviewed Feb 1, 2023
View reviewed changes
packages/microservices/client/client-tcp.ts Outdated
Comment on lines 49 to 51
if (!this.tlsOptions) {
this.socket.connect(this.port, this.host);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was the reason for moving this up? We should also add an inline comment on why "connect" is not required for tls

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved this back to its original location, added a comment

@nomaxg
fix(microservices): Correctly use ConnectionOptions
d771c52 
Signed-off-by: nomaxg <noahgolub2@gmail.com>
@nomaxg
Copy link
Contributor Author

nomaxg commented Feb 23, 2023

Thanks for the review, @kamilmysliwiec. Fixed the samples failure by using ConnectionOptions instead of TlsOptions in TcpOptions (@Flusinerd you were correct about this). Added an inline comment explaining why connecting twice isn't necessary for tls sockets.

@nomaxg nomaxg requested review from kamilmysliwiec and removed request for Flusinerd February 23, 2023 16:46
@kamilmysliwiec kamilmysliwiec merged commit a8ae4f4 into nestjs:master Apr 5, 2023
@kamilmysliwiec
Copy link
Member

LGTM

@kamilmysliwiec
Copy link
Member

Would you like to create a PR to the docs (with an additional section about this feature) @nomaxg? This could be based off this PR nestjs/docs.nestjs.com#1961

@nomaxg
Copy link
Contributor Author

nomaxg commented Apr 5, 2023

Would you like to create a PR to the docs (with an additional section about this feature) @nomaxg? This could be based off this PR nestjs/docs.nestjs.com#1961

sure! I'll work on it

@sync-by-unito sync-by-unito bot mentioned this pull request May 9, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Flusinerd Flusinerd Flusinerd requested changes

@kamilmysliwiec kamilmysliwiec Awaiting requested review from kamilmysliwiec

Assignees
No one assigned
Labels
scope: microservices type: feature 🎉
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@nomaxg @coveralls @kamilmysliwiec @Flusinerd