From 65df4399bfd4ad08249c5f60cb6825fe6c98328b Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Tue, 25 Jun 2024 16:49:42 +0200 Subject: [PATCH] refactor(probeservices): use better naming (#1628) Calling the endpoint base URL (e.g., `https://www.example.com/`) "endpoint" could cause confusion because usually an API endpoint is something like `https://www.example.com/api/v1/check-in`. To obviate this semantic issue, this diff attempts to avoid calling base URLs as endpoint throughout the tree. I am going to account this work as part of https://github.com/ooni/backend/issues/754 because this is one of the possibly issues on which we can account this work. The original conversation with @ainghazal, which prodded me to make these changes, was related to his feedback after implementing https://github.com/ooni/probe-cli/pull/1625. --- .../engine/experiment_integration_test.go | 4 +- internal/engine/session.go | 4 +- internal/engine/session_integration_test.go | 2 +- internal/probeservices/benchselect.go | 23 ++-- internal/probeservices/probeservices.go | 20 +-- internal/probeservices/probeservices_test.go | 116 +++++++++--------- 6 files changed, 86 insertions(+), 83 deletions(-) diff --git a/internal/engine/experiment_integration_test.go b/internal/engine/experiment_integration_test.go index 1d9a7f91a3..26a37509b4 100644 --- a/internal/engine/experiment_integration_test.go +++ b/internal/engine/experiment_integration_test.go @@ -3,6 +3,7 @@ package engine import ( "context" "encoding/json" + "errors" "net/http" "net/http/httptest" "os" @@ -11,6 +12,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/ooni/probe-cli/v3/internal/model" + "github.com/ooni/probe-cli/v3/internal/probeservices" "github.com/ooni/probe-cli/v3/internal/registry" ) @@ -384,7 +386,7 @@ func TestOpenReportNewClientFailure(t *testing.T) { Type: "antani", } err = exp.OpenReportContext(context.Background()) - if err.Error() != "probe services: unsupported endpoint type" { + if !errors.Is(err, probeservices.ErrUnsupportedServiceType) { t.Fatal(err) } } diff --git a/internal/engine/session.go b/internal/engine/session.go index 1a6bc5443b..fbd17d22bf 100644 --- a/internal/engine/session.go +++ b/internal/engine/session.go @@ -673,8 +673,8 @@ func (s *Session) MaybeLookupBackendsContext(ctx context.Context) error { if selected == nil { return ErrAllProbeServicesFailed } - s.logger.Infof("session: using probe services: %+v", selected.Endpoint) - s.selectedProbeService = &selected.Endpoint + s.logger.Infof("session: using probe services: %+v", selected.Service) + s.selectedProbeService = &selected.Service s.availableTestHelpers = selected.TestHelpers return nil } diff --git a/internal/engine/session_integration_test.go b/internal/engine/session_integration_test.go index 77a1e22fc4..9f211e930e 100644 --- a/internal/engine/session_integration_test.go +++ b/internal/engine/session_integration_test.go @@ -483,7 +483,7 @@ func TestNewOrchestraClientProbeServicesNewClientFailure(t *testing.T) { svc.Type = "antani" // should really not be supported for a long time } client, err := sess.newOrchestraClient(context.Background()) - if !errors.Is(err, probeservices.ErrUnsupportedEndpoint) { + if !errors.Is(err, probeservices.ErrUnsupportedServiceType) { t.Fatal("not the error we expected") } if client != nil { diff --git a/internal/probeservices/benchselect.go b/internal/probeservices/benchselect.go index 154cc4a2af..894100d256 100644 --- a/internal/probeservices/benchselect.go +++ b/internal/probeservices/benchselect.go @@ -19,8 +19,8 @@ func Default() []model.OOAPIService { }} } -// SortEndpoints gives priority to https, then cloudfronted, then onion. -func SortEndpoints(in []model.OOAPIService) (out []model.OOAPIService) { +// SortServices gives priority to https, then cloudfronted, then onion. +func SortServices(in []model.OOAPIService) (out []model.OOAPIService) { for _, entry := range in { if entry.Type == "https" { out = append(out, entry) @@ -39,7 +39,7 @@ func SortEndpoints(in []model.OOAPIService) (out []model.OOAPIService) { return } -// OnlyHTTPS returns the HTTPS endpoints only. +// OnlyHTTPS returns the HTTPS services only. func OnlyHTTPS(in []model.OOAPIService) (out []model.OOAPIService) { for _, entry := range in { if entry.Type == "https" { @@ -49,9 +49,9 @@ func OnlyHTTPS(in []model.OOAPIService) (out []model.OOAPIService) { return } -// OnlyFallbacks returns the fallback endpoints only. +// OnlyFallbacks returns the fallback services only. func OnlyFallbacks(in []model.OOAPIService) (out []model.OOAPIService) { - for _, entry := range SortEndpoints(in) { + for _, entry := range SortServices(in) { if entry.Type != "https" { out = append(out, entry) } @@ -67,15 +67,16 @@ type Candidate struct { // Err indicates whether the service works. Err error - // Endpoint is the service endpoint. - Endpoint model.OOAPIService + // Service is the service to use. + Service model.OOAPIService - // TestHelpers contains the data returned by the endpoint. + // TestHelpers contains the data returned by the service when + // querying the /api/v1/test-helpers API endpoint. TestHelpers map[string][]model.OOAPIService } func (c *Candidate) try(ctx context.Context, sess Session) { - client, err := NewClient(sess, c.Endpoint) + client, err := NewClient(sess, c.Service) if err != nil { c.Err = err return @@ -85,11 +86,11 @@ func (c *Candidate) try(ctx context.Context, sess Session) { c.Duration = time.Since(start) c.Err = err c.TestHelpers = testhelpers - sess.Logger().Debugf("probe services: %+v: %+v %s", c.Endpoint, err, c.Duration) + sess.Logger().Debugf("probe services: %+v: %+v %s", c.Service, err, c.Duration) } func try(ctx context.Context, sess Session, svc model.OOAPIService) *Candidate { - candidate := &Candidate{Endpoint: svc} + candidate := &Candidate{Service: svc} candidate.try(ctx, sess) return candidate } diff --git a/internal/probeservices/probeservices.go b/internal/probeservices/probeservices.go index 16c8a195cf..c7313610c8 100644 --- a/internal/probeservices/probeservices.go +++ b/internal/probeservices/probeservices.go @@ -1,6 +1,6 @@ // Package probeservices contains code to contact OONI probe services. // -// The probe services are HTTPS endpoints distributed across a bunch of data +// The probe services are HTTPS services distributed across a bunch of data // centres implementing a bunch of OONI APIs. When started, OONI will benchmark // the available probe services and select the fastest one. Eventually all the // possible OONI APIs will run as probe services. @@ -32,8 +32,8 @@ import ( ) var ( - // ErrUnsupportedEndpoint indicates that we don't support this endpoint type. - ErrUnsupportedEndpoint = errors.New("probe services: unsupported endpoint type") + // ErrUnsupportedServiceType indicates that we don't support this service type. + ErrUnsupportedServiceType = errors.New("probe services: unsupported service type") // ErrUnsupportedCloudFrontAddress indicates that we don't support this // cloudfront address (e.g. wrong scheme, presence of port). @@ -90,11 +90,11 @@ func (c Client) GetCredsAndAuth() (*model.OOAPILoginCredentials, *model.OOAPILog return creds, auth, nil } -// NewClient creates a new client for the specified probe services endpoint. This -// function fails, e.g., we don't support the specified endpoint. -func NewClient(sess Session, endpoint model.OOAPIService) (*Client, error) { +// NewClient creates a new client for the specified probe services service. This +// function fails, e.g., we don't support the specified service. +func NewClient(sess Session, service model.OOAPIService) (*Client, error) { client := &Client{ - BaseURL: endpoint.Address, + BaseURL: service.Address, HTTPClient: sess.DefaultHTTPClient(), Host: "", KVStore: sess.KeyValueStore(), @@ -104,7 +104,7 @@ func NewClient(sess Session, endpoint model.OOAPIService) (*Client, error) { StateFile: NewStateFile(sess.KeyValueStore()), UserAgent: sess.UserAgent(), } - switch endpoint.Type { + switch service.Type { case "https": return client, nil case "cloudfront": @@ -119,13 +119,13 @@ func NewClient(sess Session, endpoint model.OOAPIService) (*Client, error) { return nil, ErrUnsupportedCloudFrontAddress } client.Host = URL.Hostname() - URL.Host = endpoint.Front + URL.Host = service.Front client.BaseURL = URL.String() if _, err := url.Parse(client.BaseURL); err != nil { return nil, err } return client, nil default: - return nil, ErrUnsupportedEndpoint + return nil, ErrUnsupportedServiceType } } diff --git a/internal/probeservices/probeservices_test.go b/internal/probeservices/probeservices_test.go index dcb32667bf..5f31d75c8d 100644 --- a/internal/probeservices/probeservices_test.go +++ b/internal/probeservices/probeservices_test.go @@ -47,13 +47,13 @@ func TestNewClientHTTPS(t *testing.T) { } } -func TestNewClientUnsupportedEndpoint(t *testing.T) { +func TestNewClientUnsupportedService(t *testing.T) { client, err := NewClient( &mockable.Session{}, model.OOAPIService{ Address: "https://x.org", Type: "onion", }) - if !errors.Is(err, ErrUnsupportedEndpoint) { + if !errors.Is(err, ErrUnsupportedServiceType) { t.Fatal("not the error we expected") } if client != nil { @@ -193,7 +193,7 @@ func TestDefaultProbeServicesWorkAsIntended(t *testing.T) { } } -func TestSortEndpoints(t *testing.T) { +func TestSortServices(t *testing.T) { in := []model.OOAPIService{{ Type: "onion", Address: "httpo://jehhrikjjqrlpufu.onion", @@ -216,7 +216,7 @@ func TestSortEndpoints(t *testing.T) { Type: "onion", Address: "httpo://jehhrikjjqrlpufu.onion", }} - out := SortEndpoints(in) + out := SortServices(in) diff := cmp.Diff(out, expect) if diff != "" { t.Fatal(diff) @@ -259,7 +259,7 @@ func TestOnlyHTTPS(t *testing.T) { } func TestOnlyFallbacks(t *testing.T) { - // put onion first so we also verify that we sort the endpoints + // put onion first so we also verify that we sort the services in := []model.OOAPIService{{ Type: "onion", Address: "httpo://jehhrikjjqrlpufu.onion", @@ -293,7 +293,7 @@ func TestOnlyFallbacks(t *testing.T) { } func TestTryAllCanceledContext(t *testing.T) { - // put onion first so we also verify that we sort the endpoints + // put onion first so we also verify that we sort the services in := []model.OOAPIService{{ Type: "onion", Address: "httpo://jehhrikjjqrlpufu.onion", @@ -328,11 +328,11 @@ func TestTryAllCanceledContext(t *testing.T) { if !errors.Is(out[0].Err, context.Canceled) { t.Fatal("invalid error") } - if out[0].Endpoint.Type != "https" { - t.Fatal("invalid endpoint type") + if out[0].Service.Type != "https" { + t.Fatal("invalid service type") } - if out[0].Endpoint.Address != "https://ams-ps-nonexistent.ooni.io" { - t.Fatal("invalid endpoint type") + if out[0].Service.Address != "https://ams-ps-nonexistent.ooni.io" { + t.Fatal("invalid service type") } // if out[1].Duration <= 0 { @@ -341,11 +341,11 @@ func TestTryAllCanceledContext(t *testing.T) { if !errors.Is(out[1].Err, context.Canceled) { t.Fatal("invalid error") } - if out[1].Endpoint.Type != "https" { - t.Fatal("invalid endpoint type") + if out[1].Service.Type != "https" { + t.Fatal("invalid service type") } - if out[1].Endpoint.Address != "https://hkg-ps-nonexistent.ooni.io" { - t.Fatal("invalid endpoint type") + if out[1].Service.Address != "https://hkg-ps-nonexistent.ooni.io" { + t.Fatal("invalid service type") } // if out[2].Duration <= 0 { @@ -354,11 +354,11 @@ func TestTryAllCanceledContext(t *testing.T) { if !errors.Is(out[2].Err, context.Canceled) { t.Fatal("invalid error") } - if out[2].Endpoint.Type != "https" { - t.Fatal("invalid endpoint type") + if out[2].Service.Type != "https" { + t.Fatal("invalid service type") } - if out[2].Endpoint.Address != "https://mia-ps-nonexistent.ooni.io" { - t.Fatal("invalid endpoint type") + if out[2].Service.Address != "https://mia-ps-nonexistent.ooni.io" { + t.Fatal("invalid service type") } // if out[3].Duration <= 0 { @@ -367,28 +367,28 @@ func TestTryAllCanceledContext(t *testing.T) { if !errors.Is(out[3].Err, context.Canceled) { t.Fatal("invalid error") } - if out[3].Endpoint.Type != "cloudfront" { - t.Fatal("invalid endpoint type") + if out[3].Service.Type != "cloudfront" { + t.Fatal("invalid service type") } - if out[3].Endpoint.Front != "dkyhjv0wpi2dk.cloudfront.net" { - t.Fatal("invalid endpoint type") + if out[3].Service.Front != "dkyhjv0wpi2dk.cloudfront.net" { + t.Fatal("invalid service type") } - if out[3].Endpoint.Address != "https://dkyhjv0wpi2dk.cloudfront.net" { - t.Fatal("invalid endpoint type") + if out[3].Service.Address != "https://dkyhjv0wpi2dk.cloudfront.net" { + t.Fatal("invalid service type") } // - // Note: here duration may be zero because the endpoint is not supported + // Note: here duration may be zero because the service is not supported // and so we don't basically do anything. But it also may be nonzero since // we also run tests in the cloud, which is slower than my desktop. So, I // have not written a specific test concerning out[4].Duration. - if !errors.Is(out[4].Err, ErrUnsupportedEndpoint) { + if !errors.Is(out[4].Err, ErrUnsupportedServiceType) { t.Fatal("invalid error") } - if out[4].Endpoint.Type != "onion" { - t.Fatal("invalid endpoint type") + if out[4].Service.Type != "onion" { + t.Fatal("invalid service type") } - if out[4].Endpoint.Address != "httpo://jehhrikjjqrlpufu.onion" { - t.Fatal("invalid endpoint type") + if out[4].Service.Address != "httpo://jehhrikjjqrlpufu.onion" { + t.Fatal("invalid service type") } } @@ -396,7 +396,7 @@ func TestTryAllIntegrationWeRaceForFastestHTTPS(t *testing.T) { if testing.Short() { t.Skip("skip test in short mode") } - // put onion first so we also verify that we sort the endpoints + // put onion first so we also verify that we sort the services in := []model.OOAPIService{{ Type: "onion", Address: "httpo://jehhrikjjqrlpufu.onion", @@ -423,11 +423,11 @@ func TestTryAllIntegrationWeRaceForFastestHTTPS(t *testing.T) { if out[0].Err != nil { t.Fatal("invalid error") } - if out[0].Endpoint.Type != "https" { - t.Fatal("invalid endpoint type") + if out[0].Service.Type != "https" { + t.Fatal("invalid service type") } - if out[0].Endpoint.Address != "https://api.ooni.io" { - t.Fatal("invalid endpoint address") + if out[0].Service.Address != "https://api.ooni.io" { + t.Fatal("invalid service address") } } @@ -435,7 +435,7 @@ func TestTryAllIntegrationWeFallback(t *testing.T) { if testing.Short() { t.Skip("skip test in short mode") } - // put onion first so we also verify that we sort the endpoints + // put onion first so we also verify that we sort the services in := []model.OOAPIService{{ Type: "onion", Address: "httpo://jehhrikjjqrlpufu.onion", @@ -468,11 +468,11 @@ func TestTryAllIntegrationWeFallback(t *testing.T) { if !strings.HasSuffix(out[0].Err.Error(), "no such host") { t.Fatal("invalid error") } - if out[0].Endpoint.Type != "https" { - t.Fatal("invalid endpoint type") + if out[0].Service.Type != "https" { + t.Fatal("invalid service type") } - if out[0].Endpoint.Address != "https://ps-nonexistent.ooni.io" { - t.Fatal("invalid endpoint type") + if out[0].Service.Address != "https://ps-nonexistent.ooni.io" { + t.Fatal("invalid service type") } // if out[1].Duration <= 0 { @@ -481,11 +481,11 @@ func TestTryAllIntegrationWeFallback(t *testing.T) { if !strings.HasSuffix(out[1].Err.Error(), "no such host") { t.Fatal("invalid error") } - if out[1].Endpoint.Type != "https" { - t.Fatal("invalid endpoint type") + if out[1].Service.Type != "https" { + t.Fatal("invalid service type") } - if out[1].Endpoint.Address != "https://hkg-ps-nonexistent.ooni.nu" { - t.Fatal("invalid endpoint type") + if out[1].Service.Address != "https://hkg-ps-nonexistent.ooni.nu" { + t.Fatal("invalid service type") } // if out[2].Duration <= 0 { @@ -494,11 +494,11 @@ func TestTryAllIntegrationWeFallback(t *testing.T) { if !strings.HasSuffix(out[2].Err.Error(), "no such host") { t.Fatal("invalid error") } - if out[2].Endpoint.Type != "https" { - t.Fatal("invalid endpoint type") + if out[2].Service.Type != "https" { + t.Fatal("invalid service type") } - if out[2].Endpoint.Address != "https://mia-ps2-nonexistent.ooni.nu" { - t.Fatal("invalid endpoint type") + if out[2].Service.Address != "https://mia-ps2-nonexistent.ooni.nu" { + t.Fatal("invalid service type") } // if out[3].Duration <= 0 { @@ -507,13 +507,13 @@ func TestTryAllIntegrationWeFallback(t *testing.T) { if out[3].Err != nil { t.Fatal("invalid error") } - if out[3].Endpoint.Type != "cloudfront" { - t.Fatal("invalid endpoint type") + if out[3].Service.Type != "cloudfront" { + t.Fatal("invalid service type") } - if out[3].Endpoint.Address != "https://dkyhjv0wpi2dk.cloudfront.net" { - t.Fatal("invalid endpoint type") + if out[3].Service.Address != "https://dkyhjv0wpi2dk.cloudfront.net" { + t.Fatal("invalid service type") } - if out[3].Endpoint.Front != "dkyhjv0wpi2dk.cloudfront.net" { + if out[3].Service.Front != "dkyhjv0wpi2dk.cloudfront.net" { t.Fatal("invalid front") } } @@ -537,32 +537,32 @@ func TestSelectBestOnlyFailures(t *testing.T) { func TestSelectBestSelectsTheFastest(t *testing.T) { in := []*Candidate{{ Duration: 10 * time.Millisecond, - Endpoint: model.OOAPIService{ + Service: model.OOAPIService{ Address: "https://ps1.ooni.nonexistent", Type: "https", }, }, { Duration: 4 * time.Millisecond, - Endpoint: model.OOAPIService{ + Service: model.OOAPIService{ Address: "https://ps2.ooni.nonexistent", Type: "https", }, }, { Duration: 7 * time.Millisecond, - Endpoint: model.OOAPIService{ + Service: model.OOAPIService{ Address: "https://ps3.ooni.nonexistent", Type: "https", }, }, { Duration: 11 * time.Millisecond, - Endpoint: model.OOAPIService{ + Service: model.OOAPIService{ Address: "https://ps4.ooni.nonexistent", Type: "https", }, }} expected := &Candidate{ Duration: 4 * time.Millisecond, - Endpoint: model.OOAPIService{ + Service: model.OOAPIService{ Address: "https://ps2.ooni.nonexistent", Type: "https", },