From 71e68c68adaaf75ba0da1e4cf06bb0434b7b79d2 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Fri, 22 Mar 2024 09:12:35 +0100 Subject: [PATCH 1/2] feat(oonimkall): add generic HTTP transaction support This diff adds generic HTTP transaction support so that @aanorbel can manage OONI Run v2 URLs without a need to change the engine. By doing that, we reduce coupling between components. In the future, we may consider allowing the app to perform more HTTP-based operations, through this API. Closes https://github.com/ooni/probe/issues/2693 --- pkg/oonimkall/httpx.go | 82 ++++++++++++++ .../{xoonirun_test.go => httpx_test.go} | 104 +++++++++--------- pkg/oonimkall/xoonirun.go | 76 ------------- pkg/oonimkall/xoonirun_internal_test.go | 10 -- 4 files changed, 131 insertions(+), 141 deletions(-) create mode 100644 pkg/oonimkall/httpx.go rename pkg/oonimkall/{xoonirun_test.go => httpx_test.go} (53%) delete mode 100644 pkg/oonimkall/xoonirun.go delete mode 100644 pkg/oonimkall/xoonirun_internal_test.go diff --git a/pkg/oonimkall/httpx.go b/pkg/oonimkall/httpx.go new file mode 100644 index 0000000000..005df5ce46 --- /dev/null +++ b/pkg/oonimkall/httpx.go @@ -0,0 +1,82 @@ +package oonimkall + +// +// HTTP eXtensions +// + +import ( + "errors" + "net/http" + + "github.com/ooni/probe-cli/v3/internal/netxlite" +) + +// Implementation note: I am keeping this API as simple as possible. Obviously, there +// is room for improvements and possible caveats. For example: +// +// 1. we may want to send a POST request with a body (not yet implemented); +// +// 2. we may want to disable failing if status code is not 200 (not yet implemented); +// +// 3. we may want to see the response status code (not yet implemented); +// +// 4. we may want to efficiently support binary bodies (not yet implemented). +// +// If needed, we will adapt the API and implement new features. + +// HTTPRequest is an HTTP request to send. +type HTTPRequest struct { + // Method is the MANDATORY request method. + Method string + + // URL is the MANDATORY request URL. + URL string +} + +// HTTPResponse is an HTTP response. +type HTTPResponse struct { + // Body is the response body. + Body string +} + +// HTTPDo performs an HTTP request and returns the response. +// +// This method uses the default HTTP client of the session, which is the same +// client that the OONI engine uses to communicate with the OONI backend. +// +// This method throws an exception if the HTTP request status code is not 200. +func (sess *Session) HTTPDo(ctx *Context, jreq *HTTPRequest) (*HTTPResponse, error) { + sess.mtx.Lock() + defer sess.mtx.Unlock() + return sess.httpDoLocked(ctx, jreq) +} + +func (sess *Session) httpDoLocked(ctx *Context, jreq *HTTPRequest) (*HTTPResponse, error) { + clnt := sess.sessp.DefaultHTTPClient() + + req, err := http.NewRequestWithContext(ctx.ctx, jreq.Method, jreq.URL, nil) + if err != nil { + return nil, err + } + + resp, err := clnt.Do(req) + if err != nil { + return nil, err + } + defer resp.Body.Close() + + if resp.StatusCode != 200 { + return nil, errors.New("xoonirun: HTTP request failed") + } + + rawResp, err := netxlite.ReadAllContext(ctx.ctx, resp.Body) + if err != nil { + return nil, err + } + + jResp := &HTTPResponse{ + Body: string(rawResp), + } + + return jResp, nil +} diff --git a/pkg/oonimkall/xoonirun_test.go b/pkg/oonimkall/httpx_test.go similarity index 53% rename from pkg/oonimkall/xoonirun_test.go rename to pkg/oonimkall/httpx_test.go index 7a88193049..98c0d7a4dc 100644 --- a/pkg/oonimkall/xoonirun_test.go +++ b/pkg/oonimkall/httpx_test.go @@ -1,7 +1,6 @@ package oonimkall_test import ( - "encoding/json" "net" "net/http" "net/http/httptest" @@ -12,12 +11,25 @@ import ( "github.com/google/go-cmp/cmp" "github.com/ooni/probe-cli/v3/internal/runtimex" "github.com/ooni/probe-cli/v3/internal/testingx" + "github.com/ooni/probe-cli/v3/pkg/oonimkall" ) -func TestOONIRunFetch(t *testing.T) { - t.Run("we can fetch a OONI Run link descriptor", func(t *testing.T) { - if testing.Short() { - t.Skip("skip test in short mode") +func TestSessionHTTPDo(t *testing.T) { + t.Run("on success", func(t *testing.T) { + // Implementation note: because we need to backport this patch to the release/3.18 + // branch, it would be quite verbose and burdensome use netem to implement this test, + // since release/3.18 is lagging behind from master in terms of netemx. + const expectedResponseBody = "Hello, World!\r\n" + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + w.Write([]byte(expectedResponseBody)) + })) + defer server.Close() + + req := &oonimkall.HTTPRequest{ + Method: "GET", + URL: server.URL, } sess, err := NewSessionForTesting() @@ -25,44 +37,12 @@ func TestOONIRunFetch(t *testing.T) { t.Fatal(err) } - rawResp, err := sess.OONIRunFetch(sess.NewContext(), 9408643002) + resp, err := sess.HTTPDo(sess.NewContext(), req) if err != nil { t.Fatal(err) } - expect := map[string]any{ - "archived": false, - "descriptor_creation_time": "2023-07-18T15:38:21Z", - "descriptor": map[string]any{ - "author": "simone@openobservatory.org", - "description": "We use this OONI Run descriptor for writing integration tests for ooni/probe-cli/v3/pkg/oonimkall.", - "description_intl": map[string]any{}, - "icon": "", - "name": "OONIMkAll Integration Testing", - "name_intl": map[string]any{}, - "nettests": []any{ - map[string]any{ - "backend_options": map[string]any{}, - "inputs": []any{string("https://www.example.com/")}, - "is_background_run_enabled": false, - "is_manual_run_enabled": false, - "options": map[string]any{}, - "test_name": "web_connectivity", - }, - }, - "short_description": "Integration testing descriptor for ooni/probe-cli/v3/pkg/oonimkall.", - "short_description_intl": map[string]any{}, - }, - "mine": false, - "translation_creation_time": "2023-07-18T15:38:21Z", - "v": 1.0, - } - - var got map[string]any - runtimex.Try0(json.Unmarshal([]byte(rawResp), &got)) - t.Log(got) - - if diff := cmp.Diff(expect, got); diff != "" { + if diff := cmp.Diff(expectedResponseBody, resp.Body); diff != "" { t.Fatal(diff) } }) @@ -73,14 +53,17 @@ func TestOONIRunFetch(t *testing.T) { t.Fatal(err) } - URL := &url.URL{Host: "\t"} // this URL is invalid + req := &oonimkall.HTTPRequest{ + Method: "GET", + URL: "\t", // this URL is invalid + } - rawResp, err := sess.OONIRunFetchWithURL(sess.NewContext(), URL) - if !strings.HasSuffix(err.Error(), `invalid URL escape "%09"`) { + resp, err := sess.HTTPDo(sess.NewContext(), req) + if !strings.HasSuffix(err.Error(), `invalid control character in URL`) { t.Fatal("unexpected error", err) } - if rawResp != "" { - t.Fatal("expected empty raw response") + if resp != nil { + t.Fatal("expected nil response") } }) @@ -90,19 +73,22 @@ func TestOONIRunFetch(t *testing.T) { })) defer server.Close() - URL := runtimex.Try1(url.Parse(server.URL)) + req := &oonimkall.HTTPRequest{ + Method: "GET", + URL: server.URL, + } sess, err := NewSessionForTesting() if err != nil { t.Fatal(err) } - rawResp, err := sess.OONIRunFetchWithURL(sess.NewContext(), URL) + resp, err := sess.HTTPDo(sess.NewContext(), req) if !strings.HasSuffix(err.Error(), "HTTP request failed") { t.Fatal("unexpected error", err) } - if rawResp != "" { - t.Fatal("expected empty raw response") + if resp != nil { + t.Fatal("expected nil response") } }) @@ -119,17 +105,22 @@ func TestOONIRunFetch(t *testing.T) { Path: "/", } + req := &oonimkall.HTTPRequest{ + Method: "GET", + URL: URL.String(), + } + sess, err := NewSessionForTesting() if err != nil { t.Fatal(err) } - rawResp, err := sess.OONIRunFetchWithURL(sess.NewContext(), URL) + resp, err := sess.HTTPDo(sess.NewContext(), req) if !strings.HasSuffix(err.Error(), "connection_reset") { t.Fatal("unexpected error", err) } - if rawResp != "" { - t.Fatal("expected empty raw response") + if resp != nil { + t.Fatal("expected nil response") } }) @@ -149,19 +140,22 @@ func TestOONIRunFetch(t *testing.T) { })) defer server.Close() - URL := runtimex.Try1(url.Parse(server.URL)) + req := &oonimkall.HTTPRequest{ + Method: "GET", + URL: server.URL, + } sess, err := NewSessionForTesting() if err != nil { t.Fatal(err) } - rawResp, err := sess.OONIRunFetchWithURL(sess.NewContext(), URL) + resp, err := sess.HTTPDo(sess.NewContext(), req) if !strings.HasSuffix(err.Error(), "connection_reset") { t.Fatal("unexpected error", err) } - if rawResp != "" { - t.Fatal("expected empty raw response") + if resp != nil { + t.Fatal("expected nil response") } }) } diff --git a/pkg/oonimkall/xoonirun.go b/pkg/oonimkall/xoonirun.go deleted file mode 100644 index d39898ba7f..0000000000 --- a/pkg/oonimkall/xoonirun.go +++ /dev/null @@ -1,76 +0,0 @@ -package oonimkall - -// -// eXperimental OONI Run code. -// - -import ( - "errors" - "fmt" - "net/http" - "net/url" - - "github.com/ooni/probe-cli/v3/internal/netxlite" -) - -// OONIRunFetch fetches a given OONI run descriptor. -// -// The ID argument is the unique identifier of the OONI Run link. For example, in: -// -// https://api.ooni.io/api/_/ooni_run/fetch/297500125102 -// -// The OONI Run link ID is 297500125102. -// -// Warning: this API is currently experimental and we only expose it to facilitate -// developing OONI Run v2. Do not use this API in production. -func (sess *Session) OONIRunFetch(ctx *Context, ID int64) (string, error) { - sess.mtx.Lock() - defer sess.mtx.Unlock() - - // TODO(bassosimone): this code should be changed to use the probeservices.Client - // rather than using an hardcoded URL once we switch to production code. Until then, - // we are going to use the test backend server. - - // For example: https://ams-pg-test.ooni.org/api/_/ooni_run/fetch/297500125102 - URL := &url.URL{ - Scheme: "https", - Opaque: "", - User: nil, - Host: "ams-pg-test.ooni.org", - Path: fmt.Sprintf("/api/_/ooni_run/fetch/%d", ID), - RawPath: "", - OmitHost: false, - ForceQuery: false, - RawQuery: "", - Fragment: "", - RawFragment: "", - } - - return sess.ooniRunFetchWithURLLocked(ctx, URL) -} - -func (sess *Session) ooniRunFetchWithURLLocked(ctx *Context, URL *url.URL) (string, error) { - clnt := sess.sessp.DefaultHTTPClient() - - req, err := http.NewRequestWithContext(ctx.ctx, "GET", URL.String(), nil) - if err != nil { - return "", err - } - - resp, err := clnt.Do(req) - if err != nil { - return "", err - } - defer resp.Body.Close() - - if resp.StatusCode != 200 { - return "", errors.New("xoonirun: HTTP request failed") - } - - rawResp, err := netxlite.ReadAllContext(ctx.ctx, resp.Body) - if err != nil { - return "", err - } - - return string(rawResp), nil -} diff --git a/pkg/oonimkall/xoonirun_internal_test.go b/pkg/oonimkall/xoonirun_internal_test.go deleted file mode 100644 index 026d25e509..0000000000 --- a/pkg/oonimkall/xoonirun_internal_test.go +++ /dev/null @@ -1,10 +0,0 @@ -package oonimkall - -import "net/url" - -// OONIRunFetchWithURL is exposed to tests to exercise ooniRunFetchWithURLLocked -func (sess *Session) OONIRunFetchWithURL(ctx *Context, URL *url.URL) (string, error) { - sess.mtx.Lock() - defer sess.mtx.Unlock() - return sess.ooniRunFetchWithURLLocked(ctx, URL) -} From fafa63e689bef90f2554dbac7e3ead94e8407fe3 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Fri, 22 Mar 2024 09:17:35 +0100 Subject: [PATCH 2/2] x --- pkg/oonimkall/httpx.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/oonimkall/httpx.go b/pkg/oonimkall/httpx.go index 005df5ce46..bc5d6f33c8 100644 --- a/pkg/oonimkall/httpx.go +++ b/pkg/oonimkall/httpx.go @@ -66,7 +66,7 @@ func (sess *Session) httpDoLocked(ctx *Context, jreq *HTTPRequest) (*HTTPRespons defer resp.Body.Close() if resp.StatusCode != 200 { - return nil, errors.New("xoonirun: HTTP request failed") + return nil, errors.New("httpx: HTTP request failed") } rawResp, err := netxlite.ReadAllContext(ctx.ctx, resp.Body)