-
Notifications
You must be signed in to change notification settings - Fork 0
setting dnstt globally available DNS options and enabling kindling transport #296
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
base: main
Are you sure you want to change the base?
Conversation
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.
Pull request overview
This PR adds an integration test for DNSTT configurations and introduces a new gzipped YAML configuration file. The test validates each DNS configuration by creating DNS tunnels and making HTTP requests through them.
Changes:
- Added integration test to validate DNSTT configurations from an embedded YAML file
- Introduced
dnstt.yml.gzconfiguration file with DNSTT resolver configurations - Tests each DoH and DoT resolver configuration with actual HTTP requests
Reviewed changes
Copilot reviewed 1 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| kindling/dnstt/integration_test.go | New integration test that loads DNSTT configs and validates them by making HTTP requests through DNS tunnels |
| kindling/dnstt/dnstt.yml.gz | New gzipped YAML configuration file containing DNSTT resolver configurations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ganizing-dnstt-options
… be referred by other kindling transports
…turn close functions from DNSTT
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.
Pull request overview
Copilot reviewed 6 out of 8 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Pull request overview
Copilot reviewed 8 out of 11 changed files in this pull request and generated 10 comments.
Comments suppressed due to low confidence (1)
kindling/dnstt/parser_test.go:82
- Potential goroutine leak and channel blocking: The channel 'updated' is created but never closed. If the test doesn't expect an update (expectUpdate=false), the channel is never used and leaked. If an update is expected but the subscription callback is invoked multiple times or not at all, the test could hang or leak goroutines. Consider using a buffered channel or explicitly closing it after use.
updated := make(chan struct{})
if tt.expectUpdate {
events.Subscribe(func(e DNSTTUpdateEvent) {
assert.NotEmpty(t, e.YML)
updated <- struct{}{}
})
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Pull request overview
Copilot reviewed 10 out of 14 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@myleshorton @garmr-ulfr this PR should be ready for review now, the selection algorithm is testing each dns tunnel until it finds 10 dns tunnels that are working. I've initially set to wait for 30 seconds (with the hope it could select at least a few dns tunnels) but we might want to change that. |
…ganizing-dnstt-options
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.
Pull request overview
Copilot reviewed 10 out of 14 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
kindling/dnstt/parser_test.go:1
- The channel 'updated' is closed inside the subscription callback but may never be triggered in some test cases, which could cause the test to hang. The deferred close was removed on line 76, but there's no alternative cleanup mechanism if the event is never published.
package dnstt
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This PR is setting globally available DNS configurations that worked during tests and it's also enabling DNSTT as a kindling transport. Since there's a lot of DoH configs available, there's also a change in how the algorithm is selecting (it test dns tunnels in parallel and select the first 10 of them) tunnels and creating the kindling options.