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

Check result of ThousandIsland.Socket.handshake/1 #10

Merged
merged 6 commits into from
Feb 1, 2022

Conversation

moogle19
Copy link
Contributor

Added a check for the ThousandIsland.Socket.handshake/1 return value, in case it fails (e.g. if the path to the tls cert is messed up).

PS: Sorry for the many changes, but my editor removed the whitespaces at the end of the lines.
The real change is just the case statement at the bottom.

@mtrudel
Copy link
Owner

mtrudel commented Jan 31, 2022

Great PR! I can't believe I missed that in my original workup.

A couple of requests in advance of merging:

  1. Would you be able to clean up the extra whitespace, just to keep the PR tight? (feel free to rebase / push a revert commit as desired; I'll squash this branch down when merging)
  2. Could you please cover this in a test (you can induce a busted handshake pretty easily with tls options on the client - likely by mandating a super small set of ciphers or having the server require a client cert).

@ewildgoose
Copy link
Contributor

I also think that the typespec for Socket.handshake is incorrect? It specifies something like {:error, String.t()}, however, looking at the erlang docs for handshake() it defines the return value as :ssl.reason(), which is in turn defined as:

closed | timeout | error_alert()

I'm wondering if we should use :ssl.reason() or any(), since arguably it's overrideable by a custom transport? Opinion?

@mtrudel
Copy link
Owner

mtrudel commented Jan 31, 2022

The SSL module has some odd bits in its typespecs, and if memory serves I think I took a bit of a shortcut there and just punted on the error tuple spec. If you want to sort them all out and tighten up our public spec @ewildgoose that would be very welcome work. So long as dialyzer passes I'm happy.

@moogle19
Copy link
Contributor Author

moogle19 commented Feb 1, 2022

@mtrudel
I added some tests, but I am not really happy with using capture_log for it.
But I am not quite sure how to test a failure in a single handler without some hacky stuff like capture_log or (ab)use the TelemetryCollector to collect the [:handler, :error].

Do you have any other ideas how to extract the error value from a single failing handler?

@moogle19
Copy link
Contributor Author

moogle19 commented Feb 1, 2022

Okay, I think I found an acceptable solution by sending the error result to the test process and checking it via assert_received.
If you have a better solution, please let me know!

@mtrudel
Copy link
Owner

mtrudel commented Feb 1, 2022

Solution looks good! Just waiting on CI.

@mtrudel mtrudel merged commit 6d4f50f into mtrudel:main Feb 1, 2022
@ewildgoose ewildgoose mentioned this pull request Feb 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants