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

Nats web socket opts improvements #623

Merged
merged 12 commits into from
Sep 9, 2024
Merged

Nats web socket opts improvements #623

merged 12 commits into from
Sep 9, 2024

Conversation

Ivandemidov00
Copy link
Contributor

Resolves #610

Ivandemidov00 and others added 6 commits August 29, 2024 22:09
@Ivandemidov00
NatsWebSocketOpts improvements (nats-io#610)
dde4a05 
* Separate model NatsWebSocketOpts
* Сalled NatsTlsOpts.AuthenticateAsClientAsync parameters before passing it to the ConfigureWebSocketOpts
* RequestHeaders overwrite the header specified in ConfigureWebSocketOpts
@Ivandemidov00
NatsWebSocketOpts improvements (nats-io#610)
c39da39 
* Added test
@mtmk
Build fixes
863e415
@mtmk
Build fixes
a234b42
@mtmk
Build fixes
9fa2f18
@mtmk
dotnet format
718eef7
@Ivandemidov00
Copy link
Contributor Author

@caleblloyd can you take look? Did I interpret all the ideas you had in mind correctly?

@mtmk mtmk requested a review from caleblloyd August 31, 2024 19:47
@caleblloyd
Copy link
Collaborator

I will try to take a look tomorrow!

@caleblloyd
nats-io#623 reccomendations
4da55d4 
Signed-off-by: Caleb Lloyd <[email protected]>
@caleblloyd
Copy link
Collaborator

Here are some un-tested recommendations based on your PR: 4da55d4

Feel free to cherry-pick that commit into this branch if you think it looks good. The 2 main things left to do:

  • Make sure adding TLS Options / Request Headers is ignored on Blazor and does not throw
  • Add tests for TLS Options ClientCertificates / RemoteCertificateValidationCallback over WebSockets

@Ivandemidov00
Copy link
Contributor Author

Here are some un-tested recommendations based on your PR: 4da55d4

Feel free to cherry-pick that commit into this branch if you think it looks good. The 2 main things left to do:

* Make sure adding TLS Options / Request Headers is ignored on Blazor and does not throw

* Add tests for TLS Options ClientCertificates / RemoteCertificateValidationCallback over WebSockets

Thanks, I'll run some tests

@Ivandemidov00
NatsWebSocketOpts improvements (nats-io#610)
e6b9d1c 
* Fence added, an occurs exceptions when add headers to blazor
@Ivandemidov00
Copy link
Contributor Author

@caleblloyd I checked when adding headers to Blazor, an error appears.
And I have my doubts, what kind of behavior would we like to see?

  • Connect the client at any cost, without adding headers when it's Blazor
  • Throwing the PlatformNotSupportedException error that was received while adding headers to the Blazor wss client connection

@caleblloyd
Copy link
Collaborator

It’s more the TLS options I was worried about but maybe it is ok to throw there too? I don’t know how common it is for someone to configure an Isomorphic client for Blazor and .NET projects.

@caleblloyd
Copy link
Collaborator

I think we don't need detect or ignore Blazor right now, since Headers would need to be explicitly added. I changed the TLS check to look for TLS Certs being explicitly added. Also added some tests for WebSocketSecure

caleblloyd
caleblloyd approved these changes Sep 9, 2024
View reviewed changes
Copy link
Collaborator

@caleblloyd caleblloyd left a comment

Choose a reason for hiding this comment

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

Approved, @mtmk can you take a quick look and merge if you're happy?

@mtmk mtmk added this to the 2.4.0 milestone Sep 9, 2024
@Ivandemidov00
Copy link
Contributor Author

@caleblloyd Thanks for help :)

mtmk
mtmk approved these changes Sep 9, 2024
View reviewed changes
Copy link
Collaborator

@mtmk mtmk left a comment

Choose a reason for hiding this comment

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

LGTM thank you @caleblloyd and @Ivandemidov00 💯

@mtmk mtmk merged commit 71dc676 into nats-io:main Sep 9, 2024
10 checks passed
@Ivandemidov00 Ivandemidov00 deleted the NatsWebSocketOpts-improvements branch September 10, 2024 02:56
mtmk added a commit that referenced this pull request Sep 11, 2024
@mtmk
Release 2.4.0-preview.3
fcbb552 
* Fixed consume pending message calculation
* ServiceProvider callback for NATS DI configuration (#619)
* Nats web socket opts improvements (#623)
* Fix various disposable issues (#625)
* Make NuidWriter public (#618)
* NatsOpts.ConfigureWebSocketOpts callback handler (#605)
@mtmk mtmk mentioned this pull request Sep 11, 2024
Merged
mtmk added a commit that referenced this pull request Sep 11, 2024
@mtmk
Release 2.4.0-preview.3 (#627)
7cb325d 
* Fixed consume pending message calculation
* ServiceProvider callback for NATS DI configuration (#619)
* Nats web socket opts improvements (#623)
* Fix various disposable issues (#625)
* Make NuidWriter public (#618)
* NatsOpts.ConfigureWebSocketOpts callback handler (#605)
divyeshio pushed a commit to divyeshio/nats.net that referenced this pull request Sep 13, 2024
@Ivandemidov00 @mtmk
@caleblloyd @divyeshio
Nats web socket opts improvements (nats-io#623)
dd0aca3 
* NatsWebSocketOpts improvements (nats-io#610)

* Separate model NatsWebSocketOpts
* Сalled NatsTlsOpts.AuthenticateAsClientAsync parameters before passing it to the ConfigureWebSocketOpts
* RequestHeaders overwrite the header specified in ConfigureWebSocketOpts

* NatsWebSocketOpts improvements (nats-io#610)

* Added test

* Build fixes

* Build fixes

* Build fixes

* dotnet format

* nats-io#623 reccomendations

Signed-off-by: Caleb Lloyd <[email protected]>

* NatsWebSocketOpts improvements (nats-io#610)

* Fence added, an occurs exceptions when add headers to blazor

* Revert "NatsWebSocketOpts improvements (nats-io#610)"

This reverts commit e6b9d1c.

* WebSocketSecure tests

Signed-off-by: Caleb Lloyd <[email protected]>

* fix format

Signed-off-by: Caleb Lloyd <[email protected]>

* simplify WebSocketOptionsTest

Signed-off-by: Caleb Lloyd <[email protected]>

---------

Signed-off-by: Caleb Lloyd <[email protected]>
Co-authored-by: Ziya Suzen <[email protected]>
Co-authored-by: Caleb Lloyd <[email protected]>
divyeshio pushed a commit to divyeshio/nats.net that referenced this pull request Sep 13, 2024
@mtmk @divyeshio
Release 2.4.0-preview.3 (nats-io#627)
759f6b9 
* Fixed consume pending message calculation
* ServiceProvider callback for NATS DI configuration (nats-io#619)
* Nats web socket opts improvements (nats-io#623)
* Fix various disposable issues (nats-io#625)
* Make NuidWriter public (nats-io#618)
* NatsOpts.ConfigureWebSocketOpts callback handler (nats-io#605)
mtmk added a commit that referenced this pull request Sep 17, 2024
@mtmk
Release 2.4.0
5be21e4 
* Fixed consume pending message calculation (#626)
* ServiceProvider callback for NATS DI configuration (#619)
* Nats web socket opts improvements (#623)
* Fix various disposable issues (#625)
* Make NuidWriter public (#618)
* NatsOpts.ConfigureWebSocketOpts callback handler (#605)
* Simplified NATS client (#607)
* Update docs (#595)
* Add default timeout to initial commands (#594)
* Extensive logging for reconnect debugging (#593)
* Add clear next step navigation to API index doc (#592)
* Add NATS client implementation (#589)
@mtmk mtmk mentioned this pull request Sep 17, 2024
Merged
mtmk added a commit that referenced this pull request Sep 17, 2024
@mtmk
Release 2.4.0 (#633)
5fb5a92 
* Fixed consume pending message calculation (#626)
* ServiceProvider callback for NATS DI configuration (#619)
* Nats web socket opts improvements (#623)
* Fix various disposable issues (#625)
* Make NuidWriter public (#618)
* NatsOpts.ConfigureWebSocketOpts callback handler (#605)
* Simplified NATS client (#607)
* Update docs (#595)
* Add default timeout to initial commands (#594)
* Extensive logging for reconnect debugging (#593)
* Add clear next step navigation to API index doc (#592)
* Add NATS client implementation (#589)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@caleblloyd caleblloyd caleblloyd approved these changes

@mtmk mtmk mtmk approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.4.0
Development

Successfully merging this pull request may close these issues.

NatsWebSocketOpts improvements
3 participants
@Ivandemidov00 @caleblloyd @mtmk