DTLS 1.3 configuration and architecture
Description
I have made a WIP DTLS 1.3 configuration and architecture.
As suggested by @JoeTurki, it implementes a functional options pattern to provide flexibility in the future. A DTLS 1.3 configuration is created with NewConfigVersion13(c Config, opts ...OptionVersion13) (*Config, error) where an option/config can be implemented like this:
func WithOption13(f func(b bool)) OptionVersion13 {
return func(c *Config) error {
c.comeConfig = b
return nil
}
}
*/
The codebase for DTLS 1.2 and DTLS 1.3 splits off in conn.go with the new . I suggest we split off as much code as possible suffixed with handshake13 function and the new handshakeFSM in handshaker_13.go (note that neither reflects DTLS 1.3 logic yet, just a copy/paste)_13 to keep the development of DTLS 1.3 separate from 1.2.
This PR contains a test of enabling DTLS 1.3 and verifies that an error is returned correctly, as we have not yet started to implement DTLS 1.3 flights.
I would appreciate some input from other maintainers on this! @JoeTurki, @daenney, @Sean-Der.
Reference issue
Fixes #731
I'm just concerned that if we expose a public APIs, it'll be extremely difficult to change or remove it later, just my personal take about the dtls 1.3 suffix, maybe a sub package? what do you think?
@JoeTurki thanks!
I agree that we should keep the API private until the DTLS 1.3 implementation is ready; we should only make it public as the last thing to do. If that is what you meant by it?
Regarding the sub-package or suffix: It depends. Using a sub-package I am afraid to loose access to some private fields in structs or functions that can be valuable for the implementation. Using a suffix, we can move fast without those problems, but it will be a bit messy file-wise (especially with flights). Will definitely have another look to see what's possible. I think a combination might be a good solution.
I agree that we should keep the API private until the DTLS 1.3 implementation is ready; we should only make it public as the last thing to do. If that is what you meant by it?
Yeah this will be a good call, private or sub package, but private will be better, I agree , for example: if we expose NewConfigVersion13 we'll not be able to remove it in the future :(
Codecov Report
:x: Patch coverage is 40.38462% with 155 lines in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 79.07%. Comparing base (7b68bd9) to head (2dc8c37).
:warning: Report is 7 commits behind head on master.
| Files with missing lines | Patch % | Lines |
|---|---|---|
| conn.go | 45.09% | 78 Missing and 6 partials :warning: |
| flight_13.go | 9.52% | 38 Missing :warning: |
| handshaker_13.go | 52.72% | 26 Missing :warning: |
| flighthandler_13.go | 0.00% | 4 Missing :warning: |
| config.go | 50.00% | 2 Missing and 1 partial :warning: |
:x: Your patch check has failed because the patch coverage (40.38%) is below the target coverage (70.00%). You can increase the patch coverage or adjust the target coverage.
Additional details and impacted files
@@ Coverage Diff @@
## master #738 +/- ##
==========================================
- Coverage 79.13% 79.07% -0.07%
==========================================
Files 102 104 +2
Lines 6917 5855 -1062
==========================================
- Hits 5474 4630 -844
+ Misses 1068 837 -231
- Partials 375 388 +13
| Flag | Coverage Δ | |
|---|---|---|
| go | 79.07% <40.38%> (-0.09%) |
:arrow_down: |
| wasm | ? |
Flags with carried forward coverage won't be shown. Click here to find out more.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
:rocket: New features to boost your workflow:
- :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
This PR is ready for review. The goal here is not to be complete, but to provide a best-effort skeleton with my current understanding of how the DTLS 1.3 implementation would look. It still contains some duplicate code in conn.go (the linter loves to complain about it), which shall be modified when we implement more of the DTLS 1.3 spec (such as the new record layer encoding #755 ).
I still think the _13 suffix is a good way of structuring to move on without large refactorings. This reduces the number of new public APIs (now private) we would need to expose when using a sub-package. I reduced the flight handlers files by grouping them into flighthandlers_client_13 and flighthandlers_server_13. We can always decide to make a sub-package later if we see it necessary.