-
-
Notifications
You must be signed in to change notification settings - Fork 209
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
refactor: add service package and start splitting HTTP handling #1427
refactor: add service package and start splitting HTTP handling #1427
Conversation
I still need to write tests for the new package, and the various new "services", but besides that it's in a reviewable state :) |
Skimmed over your PR and it looks very good! 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Read through it and I think you did an amazing job. 👍
This approach looks like a really good foundation for a step wise implementation without end-user obstruction.
74b8931
to
3f8a177
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## refactor/services-dependencies #1427 +/- ##
=================================================================
Coverage ? 91.24%
=================================================================
Files ? 91
Lines ? 5424
Branches ? 0
=================================================================
Hits ? 4949
Misses ? 390
Partials ? 85 ☔ View full report in Codecov by Sentry. |
c731e89
to
c55ca81
Compare
Allow helpertest to use util since only the tests need helpertests, we can use util from there without creating a circular dependency.
86ff02c
to
9da89c3
Compare
Package service exposes types to abstract services from the networking. The idea is that we build a set of services and a set of network endpoints (Listener). The services are then assigned to endpoints based on the address(es) they were configured for. Actual service to endpoint binding is not handled by the abstractions in this package as it is protocol specific. The general pattern is to make a "server" that wraps a service, and can then be started on an endpoint using a `Serve` method, similar to `http.Server`. To support exposing multiple compatible services on a single endpoint (example: DoH + metrics on a single port), services can implement `Merger`.
b2db1a8
to
5571e19
Compare
I'm satisfied with where tests are at now. Codecov doesn't seem to understand the PR being based on a branch that's not targeting main (or maybe it's because I changed it), so there's no explicit percentage diff, but most new code is covered. Also the e2e tests have been pretty flaky for me today, not sure if it's my changes or not. |
@ThinkChaos I want to test this on my setup but it isn't working. I read through the comments so to make it work in testing I need to change the yaml in config.go? |
@t-e-s-tweb thanks for the interesting and trying it out, this PR doesn't change what's possible to configure yet, just changes the code to make that easy as a follow-up. |
I am using blocky as blocking server over dns-over-https. I want to migrate all dns-over-https handling over to all the new changes in this PR. |
Then building the branch and making no changes to the config should work. |
GitHub auto closed this with no option for me to reopen because the target branch was merged into main. (I was hoping GitHub had the same logic as GitLab where merging a MR changes the other MRs targeting that branch to now target where the first MR was merged, but no I guess not) |
Progress for #1206.
No user visible changes in this PR: it's just refactoring and the new config cannot be set from the YAML file.
Instead the new config structs are filled with the info from
ports:
until we're ready to deprecateports
fully.The first part of the commits refactor how we setup the HTTP listeners.
The second part adds a new
service
package with interfaces used to split upserver.Server
.The commits after adding that package refactor the DoH, metrics, and API to be services.
Here's the docs that package as an overview of the design:
This doesn't touch DNS listener/server code (besides moving the TLS config to a single spot), but I think we'll be able to re-use the same code to setup DNS listeners, and split the DNS server logic into a bunch of services + one server.
The follow-up work is to continue removing things from
configureHTTPRouter
until it's empty and we can remove the transitionalhttpMiscService
.At that point we'll be ready to deprecate
ports
and switch to the newservices
config format under discussion in #1206.