Skip to content

Commit

Permalink
refactor(httpclientx): couple URL and cloudfront host header (#1584)
Browse files Browse the repository at this point in the history
  • Loading branch information
bassosimone authored May 2, 2024
1 parent 1056c98 commit 9dbe15c
Show file tree
Hide file tree
Showing 27 changed files with 549 additions and 304 deletions.
15 changes: 9 additions & 6 deletions internal/enginelocate/cloudflare.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,15 @@ func cloudflareIPLookup(
resolver model.Resolver,
) (string, error) {
// get the raw response body
data, err := httpclientx.GetRaw(ctx, "https://www.cloudflare.com/cdn-cgi/trace", &httpclientx.Config{
Authorization: "", // not needed
Client: httpClient,
Logger: logger,
UserAgent: userAgent,
})
data, err := httpclientx.GetRaw(
ctx,
httpclientx.NewEndpoint("https://www.cloudflare.com/cdn-cgi/trace"),
&httpclientx.Config{
Authorization: "", // not needed
Client: httpClient,
Logger: logger,
UserAgent: userAgent,
})

// handle the error case
if err != nil {
Expand Down
15 changes: 9 additions & 6 deletions internal/enginelocate/ubuntu.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,15 @@ func ubuntuIPLookup(
resolver model.Resolver,
) (string, error) {
// read the HTTP response and parse as XML
v, err := httpclientx.GetXML[*ubuntuResponse](ctx, "https://geoip.ubuntu.com/lookup", &httpclientx.Config{
Authorization: "", // not needed
Client: httpClient,
Logger: logger,
UserAgent: userAgent,
})
v, err := httpclientx.GetXML[*ubuntuResponse](
ctx,
httpclientx.NewEndpoint("https://geoip.ubuntu.com/lookup"),
&httpclientx.Config{
Authorization: "", // not needed
Client: httpClient,
Logger: logger,
UserAgent: userAgent,
})

// handle the error case
if err != nil {
Expand Down
55 changes: 29 additions & 26 deletions internal/httpclientx/DESIGN.md
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,8 @@ to a single IP address, it might still be useful to fallback.
and, if so, with which status code, which is needed to intercept `401` responses
to take the appropriate logging-in actions.

7. Make the design extensible such that re-adding unused functionality (such
as cloud fronting) does not require us to refactor the code much.
7. Make the design extensible such that re-adding unused functionality
does not require us to refactor the code much.

8. Functional equivalent with existing packages (modulo the existing
functionality that is not relevant anymore).
Expand All @@ -65,8 +65,6 @@ overly complex code, which hampers maintenance.
2. implementing algorithms such as logging in to the OONI backend and requesting
tokens, which should be the responsibility of another package.

3. implementing cloud fronting, which is not used anymore.

## Design

This package supports the following operations:
Expand All @@ -78,13 +76,18 @@ type Config struct {
UserAgent string
}

func GetJSON[Output any](ctx context.Context, URL string, config *Config) (Output, error)
type Endpoint struct {
URL string
Host string // optional for cloudfronting
}

func GetJSON[Output any](ctx context.Context, epnt *Endpoint, config *Config) (Output, error)

func GetRaw(ctx context.Context, URL string, config *Config) ([]byte, error)
func GetRaw(ctx context.Context, epnt *Endpoint, config *Config) ([]byte, error)

func GetXML[Output any](ctx context.Context, URL string, config *Config) (Output, error)
func GetXML[Output any](ctx context.Context, epnt *Endpoint, config *Config) (Output, error)

func PostJSON[Input, Output any](ctx context.Context, URL string, input Input, config *Config) (Output, error)
func PostJSON[Input, Output any](ctx context.Context, epnt *Endpoint, input Input, config *Config) (Output, error)
```

(The `*Config` is the last argument because it is handy to create it inline when calling
Expand Down Expand Up @@ -141,7 +144,7 @@ They all construct the same `*Overlapped` struct, which looks like this:

```Go
type Overlapped[Output any] struct {
RunFunc func(ctx context.Context, URL string) (Output, error)
RunFunc func(ctx context.Context, epnt *Endpoint) (Output, error)

ScheduleInterval time.Duration
}
Expand All @@ -153,30 +156,33 @@ name (i.e., `NewOverlappedGetXML` configures `RunFunc` to run `GetXML`).
Then, we define the following method:

```Go
func (ovx *Overlapped[Output]) Run(ctx context.Context, URLs ...string) (Output, error)
func (ovx *Overlapped[Output]) Run(ctx context.Context, epnts ...*Endpoint) (Output, error)
```

This method starts N goroutines to issue the API calls with each URL. (A classic example is for
the URLs to be `https://0.th.ooni.org/`, `https://1.th.ooni.org/` and so on.)
This method starts N goroutines to issue the API calls with each endpoint URL. (A classic example
is for the URLs to be `https://0.th.ooni.org/`, `https://1.th.ooni.org/` and so on.)

By default, `ScheduleInterval` is 15 seconds. If the first URL does not provide a result
By default, `ScheduleInterval` is 15 seconds. If the first endpoint URL does not provide a result
within 15 seconds, we try the second one. That is, every 15 seconds, we will attempt using
another URL, until there's a successful response or we run out of URLs.
another endpoint URL, until there's a successful response or we run out of URLs.

As soon as we have a successful response, we cancel all the other pending operations
that may exist. Once all operations have terminated, we return to the caller.

### Extensibility

We use the `Config` object to package common settings. Thus adding a new field (e.g., a
custom `Host` header to implement cloud fronting), only means the following:
We use the `Config` object to package common settings. Thus adding a new field, only means
the following:

1. Adding a new OPTIONAL field to `Config`.

2. Honoring this field inside the internal implementation.

_Et voilà_, this should allow for minimal efforts API upgrades.

In fact, we used this strategy to easily add support for cloudfront in
[probe-cli#1577](https://github.com/ooni/probe-cli/pull/1577).

### Functionality Comparison

This section compares side-by-side the operations performed by each implementation
Expand All @@ -196,7 +202,7 @@ We compare to `httpapi.Call` and `httpx.GetJSONWithQuery`.
| join path and base URL | NO | yes | yes |
| append query to URL | NO | yes | yes |
| NewRequestWithContext | yes️ | yes | yes |
| handle cloud front | NO | yes | yes |
| handle cloud front | yes | yes | yes |
| set Authorization | yes | yes | yes |
| set Accept | NO | yes | yes |
| set User-Agent | yes ️ | yes | yes |
Expand All @@ -223,13 +229,10 @@ Regarding all the other cases for which `GetJSON` is marked as "NO":
code to join together a base URL, possibly including a base path, a path, and a query (and we're
introducing the new `./internal/urlx` package to handle this situation).

3. `GetJSON` does not handle cloud fronting because we don't use it. The design where the
`Config` contains mandatory and optional fields would still allow doing that easily.

4. Setting the `Accept` header does not seem to matter in out context because we mostly
3. Setting the `Accept` header does not seem to matter in out context because we mostly
call API for which there's no need for content negotiation.

5. It's difficult to say whether a body size was exactly the amount specified for truncation
4. It's difficult to say whether a body size was exactly the amount specified for truncation
or the body has been truncated. While this is a corner case, it seems perhaps wiser to let
the caller try parsing the body and failing if it is indeed truncated.

Expand All @@ -244,7 +247,7 @@ Here we're comparing to `httpapi.Call` and `httpx.FetchResource`.
| join path and base URL | NO | yes | yes |
| append query to URL | NO | yes | yes |
| NewRequestWithContext | yes️ | yes | yes |
| handle cloud front | NO | yes | yes |
| handle cloud front | yes | yes | yes |
| set Authorization | yes | yes | yes |
| set Accept | NO | yes | yes |
| set User-Agent | yes ️ | yes | yes |
Expand Down Expand Up @@ -272,7 +275,7 @@ two APIs, the caller would need to fetch a raw body and then manually parse XML.
| join path and base URL | NO | N/A | N/A |
| append query to URL | NO | N/A | N/A |
| NewRequestWithContext | yes️ | N/A | N/A |
| handle cloud front | NO | N/A | N/A |
| handle cloud front | yes | N/A | N/A |
| set Authorization | yes | N/A | N/A |
| set Accept | NO | N/A | N/A |
| set User-Agent | yes ️ | N/A | N/A |
Expand Down Expand Up @@ -302,7 +305,7 @@ Here we're comparing to `httpapi.Call` and `httpx.PostJSON`.
| join path and base URL | NO | yes | yes |
| append query to URL | NO | yes | yes |
| NewRequestWithContext | yes️ | yes | yes |
| handle cloud front | NO | yes | yes |
| handle cloud front | yes | yes | yes |
| set Authorization | yes | yes | yes |
| set Accept | NO | yes | yes |
| set Content-Type | yes | yes | yes |
Expand Down Expand Up @@ -330,7 +333,7 @@ that is part of the same package, while in this package we marshal in `PostJSON`
Consider the following code snippet:

```Go
resp, err := httpclientx.GetJSON[*APIResponse](ctx, URL, config)
resp, err := httpclientx.GetJSON[*APIResponse](ctx, epnt, config)
runtimex.Assert((resp == nil && err != nil) || (resp != nil && err == nil), "ouch")
```

Expand Down
3 changes: 0 additions & 3 deletions internal/httpclientx/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,6 @@ type Config struct {
// Client is the MANDATORY [model.HTTPClient] to use.
Client model.HTTPClient

// Host is the OPTIONAL Host header to use to implement cloudfronting.
Host string

// Logger is the MANDATORY [model.Logger] to use.
Logger model.Logger

Expand Down
49 changes: 49 additions & 0 deletions internal/httpclientx/endpoint.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
package httpclientx

import "github.com/ooni/probe-cli/v3/internal/model"

// Endpoint is an HTTP endpoint.
//
// The zero value is invalid; construct using [NewEndpoint].
type Endpoint struct {
// URL is the MANDATORY endpoint URL.
URL string

// Host is the OPTIONAL host header to use for cloudfronting.
Host string
}

// NewEndpoint constructs a new [*Endpoint] instance using the given URL.
func NewEndpoint(URL string) *Endpoint {
return &Endpoint{
URL: URL,
Host: "",
}
}

// WithHostOverride returns a copy of the [*Endpoint] using the given host header override.
func (e *Endpoint) WithHostOverride(host string) *Endpoint {
return &Endpoint{
URL: e.URL,
Host: host,
}
}

// NewEndpointFromModelOOAPIServices constructs new [*Endpoint] instances from the
// given [model.OOAPIService] instances, assigning the host header if "cloudfront", and
// skipping all the entries that are neither "https" not "cloudfront".
func NewEndpointFromModelOOAPIServices(svcs ...model.OOAPIService) (epnts []*Endpoint) {
for _, svc := range svcs {
epnt := NewEndpoint(svc.Address)
switch svc.Type {
case "cloudfront":
epnt = epnt.WithHostOverride(svc.Front)
fallthrough
case "https":
epnts = append(epnts, epnt)
default:
// skip entry
}
}
return
}
65 changes: 65 additions & 0 deletions internal/httpclientx/endpoint_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
package httpclientx

import (
"testing"

"github.com/google/go-cmp/cmp"
"github.com/ooni/probe-cli/v3/internal/model"
)

func TestEndpoint(t *testing.T) {
t.Run("the constructor only assigns the URL", func(t *testing.T) {
epnt := NewEndpoint("https://www.example.com/")
if epnt.URL != "https://www.example.com/" {
t.Fatal("unexpected URL")
}
if epnt.Host != "" {
t.Fatal("unexpected host")
}
})

t.Run("we can optionally get a copy with an assigned host header", func(t *testing.T) {
epnt := NewEndpoint("https://www.example.com/").WithHostOverride("www.cloudfront.com")
if epnt.URL != "https://www.example.com/" {
t.Fatal("unexpected URL")
}
if epnt.Host != "www.cloudfront.com" {
t.Fatal("unexpected host")
}
})

t.Run("we can convert from model.OOAPIService", func(t *testing.T) {
services := []model.OOAPIService{{
Address: "",
Type: "onion",
Front: "",
}, {
Address: "https://www.example.com/",
Type: "https",
}, {
Address: "",
Type: "onion",
Front: "",
}, {
Address: "https://www.example.com/",
Type: "cloudfront",
Front: "www.cloudfront.com",
}, {
Address: "",
Type: "onion",
Front: "",
}}

expect := []*Endpoint{{
URL: "https://www.example.com/",
}, {
URL: "https://www.example.com/",
Host: "www.cloudfront.com",
}}

got := NewEndpointFromModelOOAPIServices(services...)
if diff := cmp.Diff(expect, got); diff != "" {
t.Fatal(diff)
}
})
}
10 changes: 5 additions & 5 deletions internal/httpclientx/getjson.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,18 +15,18 @@ import (
//
// - ctx is the cancellable context;
//
// - URL is the URL to use;
// - epnt is the HTTP [*Endpoint] to use;
//
// - config contains the config.
//
// This function either returns an error or a valid Output.
func GetJSON[Output any](ctx context.Context, URL string, config *Config) (Output, error) {
return NewOverlappedGetJSON[Output](config).Run(ctx, URL)
func GetJSON[Output any](ctx context.Context, epnt *Endpoint, config *Config) (Output, error) {
return NewOverlappedGetJSON[Output](config).Run(ctx, epnt)
}

func getJSON[Output any](ctx context.Context, URL string, config *Config) (Output, error) {
func getJSON[Output any](ctx context.Context, epnt *Endpoint, config *Config) (Output, error) {
// read the raw body
rawrespbody, err := GetRaw(ctx, URL, config)
rawrespbody, err := GetRaw(ctx, epnt, config)

// handle the case of error
if err != nil {
Expand Down
Loading

0 comments on commit 9dbe15c

Please sign in to comment.