Skip to content

Commit

Permalink
fix(dig curl): enforce max operation runtime (#9)
Browse files Browse the repository at this point in the history
Use five seconds for dig without making it configurable. For now, this
default should be good enough. For DNS-over-HTTPS, ensure that the
timeout also applies to reading the body by setting client.Timeout.

For curl, implement the `--max-time` flag, default it to 30s, and ensure
it also applied to reading the body using client.Timeout.

Also, ensure we update the documentation accordingly.
  • Loading branch information
bassosimone authored Nov 30, 2024
1 parent 22461b9 commit 94e7992
Show file tree
Hide file tree
Showing 5 changed files with 25 additions and 2 deletions.
5 changes: 5 additions & 0 deletions internal/cli/curl/README.txt
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,11 @@ We currently support the following command line flags:
dash (`-`), we write to the stdout. If you specify `--logs` multiple
times, we write to the last FILE specified.

--max-time DURATION
Sets the maximum time that the transfer operation is allowed to take
in seconds (e.g., `--max-time 5`). If this flag is not specified, the
default max time is 30 seconds.

-o, --output FILE
Write the response body to FILE instead of using the stdout.

Expand Down
4 changes: 4 additions & 0 deletions internal/cli/curl/curl.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"io"
"os"
"strings"
"time"

"github.com/rbmk-project/common/cliutils"
"github.com/rbmk-project/rbmk/internal/testable"
Expand Down Expand Up @@ -53,6 +54,7 @@ func (cmd command) Main(ctx context.Context, argv ...string) error {
// 2. create initial task with defaults
task := &Task{
LogsWriter: io.Discard,
MaxTime: 30 * time.Second,
Method: "GET",
Output: os.Stdout,
ResolveMap: make(map[string]string),
Expand All @@ -65,6 +67,7 @@ func (cmd command) Main(ctx context.Context, argv ...string) error {

// 4. add flags to the parser
logfile := clip.String("logs", "", "path where to write structured logs")
maxTime := clip.Int64("max-time", 30, "maximum time to wait for the operation to finish")
output := clip.StringP("output", "o", "", "write to file instead of stdout")
method := clip.StringP("request", "X", "GET", "HTTP request method")
resolve := clip.StringArray("resolve", nil, "use addr instead of DNS")
Expand Down Expand Up @@ -110,6 +113,7 @@ func (cmd command) Main(ctx context.Context, argv ...string) error {
}

// 9. process other flags
task.MaxTime = time.Duration(*maxTime) * time.Second
task.Method = *method
if *verbose {
task.VerboseOutput = os.Stderr
Expand Down
12 changes: 11 additions & 1 deletion internal/cli/curl/task.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"log/slog"
"net"
"net/http"
"time"

"github.com/rbmk-project/common/dialonce"
"github.com/rbmk-project/dnscore"
Expand All @@ -22,6 +23,9 @@ type Task struct {
// LogsWriter is where we write structured logs
LogsWriter io.Writer

// MaxTime is the maximum time to wait for the operation to finish.
MaxTime time.Duration

// Method is the HTTP method to use
Method string

Expand All @@ -40,6 +44,10 @@ type Task struct {

// Run executes the curl task
func (task *Task) Run(ctx context.Context) error {
// Setup the overall operation timeout using the context
ctx, cancel := context.WithTimeout(ctx, task.MaxTime)
defer cancel()

// Set up the JSON logger for writing the measurements
logger := slog.New(slog.NewJSONHandler(task.LogsWriter, &slog.HandlerOptions{}))

Expand Down Expand Up @@ -68,11 +76,13 @@ func (task *Task) Run(ctx context.Context) error {
return nil, dnscore.ErrNoName
}

// Create the HTTP client to use
// Create the HTTP client to use and make sure we're using
// an overall operation timeout for the transfer
client := &http.Client{
CheckRedirect: func(req *http.Request, via []*http.Request) error {
return http.ErrUseLastResponse
},
Timeout: task.MaxTime, // ensure the overall operation is bounded
Transport: &httpLogTransport{
Logger: logger,
RoundTripper: &http.Transport{
Expand Down
2 changes: 2 additions & 0 deletions internal/cli/dig/README.txt
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ a query for the `A` record type. We support these record types:

If you specify `TYPE` multiple times, we emit a warning and use the last one.

Note that, by default, the query will timeout after five seconds.

We currently support the following command line flags:

-h, --help
Expand Down
4 changes: 3 additions & 1 deletion internal/cli/dig/task.go
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,8 @@ func (task *Task) newServerAddr(protocol dnscore.Protocol) string {
// Run runs the task and returns an error.
func (task *Task) Run(ctx context.Context) error {
// Setup the overal operation timeout using the context
ctx, cancel := context.WithTimeout(ctx, 5*time.Second)
const timeout = 5 * time.Second
ctx, cancel := context.WithTimeout(ctx, timeout)
defer cancel()

// Set up the JSON logger for writing the measurements
Expand All @@ -133,6 +134,7 @@ func (task *Task) Run(ctx context.Context) error {
transport.DialContext = netx.DialContext
transport.DialTLSContext = netx.DialTLSContext
transport.HTTPClient = &http.Client{
Timeout: timeout, // ensure the overall operation is bounded
Transport: &http.Transport{
DialContext: netx.DialContext,
DialTLSContext: netx.DialTLSContext,
Expand Down

0 comments on commit 94e7992

Please sign in to comment.