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.

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

[Breaking Change] Make transport settings required #639

Merged
merged 5 commits into from
Nov 21, 2023

Conversation

K-Cully
Copy link
Member

@K-Cully K-Cully commented Nov 21, 2023

The current listener and proxy creation logic falls-back on creating insecure listeners and proxies when transport settings are not defined.
This change updates the behaviour to throw when no transport settings are provider or transport settings cannot be loaded from the TransportSettings section of the hosting service's settings.xml file.

Since transport settings can still be defined explicitly, the implementation still allows for insecure connection creation, if the user understands well how to initialise transport settings in an insecure way.
However, most users will use defaults, or follow the Service Fabric documentation which details using LoadFrom to set up secure remoting, so this is not a concern.

FabricTransportSettings.LoadFrom could have been used instead but I think having a descriptive exception message is valuable, so kept TryLoadFrom.

@K-Cully K-Cully added the enhancement New feature or request label Nov 21, 2023
@K-Cully K-Cully requested a review from a team as a code owner November 21, 2023 10:39
@K-Cully K-Cully marked this pull request as draft November 21, 2023 13:52
@K-Cully K-Cully marked this pull request as ready for review November 21, 2023 14:01
MaxDac
MaxDac previously approved these changes Nov 21, 2023
@K-Cully K-Cully merged commit f4bcaf3 into main Nov 21, 2023
@K-Cully K-Cully deleted the K-Cully/remove-insecure-remoting branch November 21, 2023 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants