From 6306c09963c3e05f5a1eb1a48ae226baa40ff3a1 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Tue, 6 Apr 2021 13:03:50 +0200 Subject: [PATCH 1/4] fix: disable maxRuntime when not WebConnectivity See https://github.com/ooni/probe/issues/1433 --- cmd/ooniprobe/internal/nettests/nettests.go | 8 +++++++- internal/version/version.go | 2 +- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/cmd/ooniprobe/internal/nettests/nettests.go b/cmd/ooniprobe/internal/nettests/nettests.go index 89dd005..5e5f9f2 100644 --- a/cmd/ooniprobe/internal/nettests/nettests.go +++ b/cmd/ooniprobe/internal/nettests/nettests.go @@ -120,6 +120,11 @@ func (c *Controller) Run(builder *engine.ExperimentBuilder, inputs []string) err log.Debug("disabling maxRuntime when running in the background") maxRuntime = 0 } + _, isWebConnectivity := c.nt.(WebConnectivity) + if !isWebConnectivity { + log.Debug("disabling maxRuntime without Web Connectivity") + maxRuntime = 0 + } start := time.Now() c.ntStartTime = start for idx, input := range inputs { @@ -214,7 +219,8 @@ func (c *Controller) Run(builder *engine.ExperimentBuilder, inputs []string) err func (c *Controller) OnProgress(perc float64, msg string) { // when we have maxRuntime, honor it maxRuntime := time.Duration(c.Probe.Config().Nettests.WebsitesMaxRuntime) * time.Second - if c.RunType == "manual" && maxRuntime > 0 { + _, isWebConnectivity := c.nt.(WebConnectivity) + if c.RunType == "manual" && maxRuntime > 0 && isWebConnectivity { elapsed := time.Since(c.ntStartTime) perc = float64(elapsed) / float64(maxRuntime) eta := maxRuntime.Seconds() - elapsed.Seconds() diff --git a/internal/version/version.go b/internal/version/version.go index c01bb74..1f506bc 100644 --- a/internal/version/version.go +++ b/internal/version/version.go @@ -3,5 +3,5 @@ package version const ( // Version is the software version - Version = "3.9.0" + Version = "3.9.1" ) From 818bf2cba500ba5b3a569c9ce9e80e838d0712cd Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 7 Apr 2021 13:54:56 +0200 Subject: [PATCH 2/4] fix: move preventMistakes in InputLoader This fixes an issue where URLs provided with --input are not accepted by the preventMistakes filter. The filter itself needs to execute _only_ on URLs returned by the checkIn API, rather than on URLs returned by the InputLoader, which may instead be user provided. Reference issue: https://github.com/ooni/probe/issues/1435 --- .../internal/nettests/web_connectivity.go | 25 ------ .../nettests/web_connectivity_test.go | 77 ---------------- internal/engine/inputloader.go | 64 +++++++++++++- internal/engine/inputloader_test.go | 87 +++++++++++++++++++ 4 files changed, 150 insertions(+), 103 deletions(-) delete mode 100644 cmd/ooniprobe/internal/nettests/web_connectivity_test.go diff --git a/cmd/ooniprobe/internal/nettests/web_connectivity.go b/cmd/ooniprobe/internal/nettests/web_connectivity.go index c304701..746e040 100644 --- a/cmd/ooniprobe/internal/nettests/web_connectivity.go +++ b/cmd/ooniprobe/internal/nettests/web_connectivity.go @@ -9,30 +9,6 @@ import ( "github.com/ooni/probe-cli/v3/internal/engine/model" ) -// preventMistakes makes the code more robust with respect to any possible -// integration issue where the backend returns to us URLs that don't -// belong to the category codes we requested. -func preventMistakes(input []model.URLInfo, categories []string) (output []model.URLInfo) { - if len(categories) <= 0 { - return input - } - for _, entry := range input { - var found bool - for _, cat := range categories { - if entry.CategoryCode == cat { - found = true - break - } - } - if !found { - log.Warnf("URL %+v not in %+v; skipping", entry, categories) - continue - } - output = append(output, entry) - } - return -} - func lookupURLs(ctl *Controller, categories []string) ([]string, map[int64]int64, error) { inputloader := &engine.InputLoader{ CheckInConfig: &model.CheckInConfig{ @@ -56,7 +32,6 @@ func lookupURLs(ctl *Controller, categories []string) ([]string, map[int64]int64 if err != nil { return nil, nil, err } - testlist = preventMistakes(testlist, categories) var urls []string urlIDMap := make(map[int64]int64) for idx, url := range testlist { diff --git a/cmd/ooniprobe/internal/nettests/web_connectivity_test.go b/cmd/ooniprobe/internal/nettests/web_connectivity_test.go deleted file mode 100644 index 5c17c64..0000000 --- a/cmd/ooniprobe/internal/nettests/web_connectivity_test.go +++ /dev/null @@ -1,77 +0,0 @@ -package nettests - -import ( - "testing" - - "github.com/google/go-cmp/cmp" - "github.com/ooni/probe-cli/v3/internal/engine/model" -) - -func TestPreventMistakesWithCategories(t *testing.T) { - input := []model.URLInfo{{ - CategoryCode: "NEWS", - URL: "https://repubblica.it/", - CountryCode: "IT", - }, { - CategoryCode: "HACK", - URL: "https://2600.com", - CountryCode: "XX", - }, { - CategoryCode: "FILE", - URL: "https://addons.mozilla.org/", - CountryCode: "XX", - }} - desired := []model.URLInfo{{ - CategoryCode: "NEWS", - URL: "https://repubblica.it/", - CountryCode: "IT", - }, { - CategoryCode: "FILE", - URL: "https://addons.mozilla.org/", - CountryCode: "XX", - }} - output := preventMistakes(input, []string{"NEWS", "FILE"}) - if diff := cmp.Diff(desired, output); diff != "" { - t.Fatal(diff) - } -} - -func TestPreventMistakesWithoutCategoriesAndNil(t *testing.T) { - input := []model.URLInfo{{ - CategoryCode: "NEWS", - URL: "https://repubblica.it/", - CountryCode: "IT", - }, { - CategoryCode: "HACK", - URL: "https://2600.com", - CountryCode: "XX", - }, { - CategoryCode: "FILE", - URL: "https://addons.mozilla.org/", - CountryCode: "XX", - }} - output := preventMistakes(input, nil) - if diff := cmp.Diff(input, output); diff != "" { - t.Fatal(diff) - } -} - -func TestPreventMistakesWithoutCategoriesAndEmpty(t *testing.T) { - input := []model.URLInfo{{ - CategoryCode: "NEWS", - URL: "https://repubblica.it/", - CountryCode: "IT", - }, { - CategoryCode: "HACK", - URL: "https://2600.com", - CountryCode: "XX", - }, { - CategoryCode: "FILE", - URL: "https://addons.mozilla.org/", - CountryCode: "XX", - }} - output := preventMistakes(input, []string{}) - if diff := cmp.Diff(input, output); diff != "" { - t.Fatal(diff) - } -} diff --git a/internal/engine/inputloader.go b/internal/engine/inputloader.go index 4f28d3e..d3d3d6e 100644 --- a/internal/engine/inputloader.go +++ b/internal/engine/inputloader.go @@ -7,6 +7,7 @@ import ( "fmt" "io/fs" + "github.com/apex/log" "github.com/ooni/probe-cli/v3/internal/engine/internal/fsx" "github.com/ooni/probe-cli/v3/internal/engine/model" ) @@ -26,6 +27,12 @@ type InputLoaderSession interface { config *model.CheckInConfig) (*model.CheckInInfo, error) } +// InputLoaderLogger is the logger according to an InputLoader. +type InputLoaderLogger interface { + // Warnf formats and emits a warning message. + Warnf(format string, v ...interface{}) +} + // InputLoader loads input according to the specified policy // either from command line and input files or from OONI services. The // behaviour depends on the input policy as described below. @@ -68,6 +75,11 @@ type InputLoader struct { // this field. InputPolicy InputPolicy + // Logger is the optional logger that the InputLoader + // should be using. If not set, we will use the default + // logger of github.com/apex/log. + Logger InputLoaderLogger + // Session is the current measurement session. You // MUST fill in this field. Session InputLoaderSession @@ -193,7 +205,7 @@ func (il *InputLoader) loadRemote(ctx context.Context) ([]model.URLInfo, error) // concerned about NOT passing it a NULL pointer. config = &model.CheckInConfig{} } - reply, err := il.Session.CheckIn(ctx, config) + reply, err := il.checkIn(ctx, config) if err != nil { return nil, err } @@ -202,3 +214,53 @@ func (il *InputLoader) loadRemote(ctx context.Context) ([]model.URLInfo, error) } return reply.WebConnectivity.URLs, nil } + +// checkIn executes the check-in and filters the returned URLs to exclude +// the URLs that are not part of the requested categories. This is done for +// robustness, just in case we or the API do something wrong. +func (il *InputLoader) checkIn( + ctx context.Context, config *model.CheckInConfig) (*model.CheckInInfo, error) { + reply, err := il.Session.CheckIn(ctx, config) + if err != nil { + return nil, err + } + // Note: safe to assume that reply is not nil if err is nil + if reply.WebConnectivity != nil && len(reply.WebConnectivity.URLs) > 0 { + reply.WebConnectivity.URLs = il.preventMistakes( + reply.WebConnectivity.URLs, config.WebConnectivity.CategoryCodes, + ) + } + return reply, nil +} + +// preventMistakes makes the code more robust with respect to any possible +// integration issue where the backend returns to us URLs that don't +// belong to the category codes we requested. +func (il *InputLoader) preventMistakes(input []model.URLInfo, categories []string) (output []model.URLInfo) { + if len(categories) <= 0 { + return input + } + for _, entry := range input { + var found bool + for _, cat := range categories { + if entry.CategoryCode == cat { + found = true + break + } + } + if !found { + il.logger().Warnf("URL %+v not in %+v; skipping", entry, categories) + continue + } + output = append(output, entry) + } + return +} + +// logger returns the configured logger or apex/log's default. +func (il *InputLoader) logger() InputLoaderLogger { + if il.Logger != nil { + return il.Logger + } + return log.Log +} diff --git a/internal/engine/inputloader_test.go b/internal/engine/inputloader_test.go index 764796c..3193166 100644 --- a/internal/engine/inputloader_test.go +++ b/internal/engine/inputloader_test.go @@ -407,3 +407,90 @@ func TestInputLoaderCheckInSuccessWithSomeURLs(t *testing.T) { t.Fatal(diff) } } + +func TestPreventMistakesWithCategories(t *testing.T) { + input := []model.URLInfo{{ + CategoryCode: "NEWS", + URL: "https://repubblica.it/", + CountryCode: "IT", + }, { + CategoryCode: "HACK", + URL: "https://2600.com", + CountryCode: "XX", + }, { + CategoryCode: "FILE", + URL: "https://addons.mozilla.org/", + CountryCode: "XX", + }} + desired := []model.URLInfo{{ + CategoryCode: "NEWS", + URL: "https://repubblica.it/", + CountryCode: "IT", + }, { + CategoryCode: "FILE", + URL: "https://addons.mozilla.org/", + CountryCode: "XX", + }} + il := &InputLoader{} + output := il.preventMistakes(input, []string{"NEWS", "FILE"}) + if diff := cmp.Diff(desired, output); diff != "" { + t.Fatal(diff) + } +} + +func TestPreventMistakesWithoutCategoriesAndNil(t *testing.T) { + input := []model.URLInfo{{ + CategoryCode: "NEWS", + URL: "https://repubblica.it/", + CountryCode: "IT", + }, { + CategoryCode: "HACK", + URL: "https://2600.com", + CountryCode: "XX", + }, { + CategoryCode: "FILE", + URL: "https://addons.mozilla.org/", + CountryCode: "XX", + }} + il := &InputLoader{} + output := il.preventMistakes(input, nil) + if diff := cmp.Diff(input, output); diff != "" { + t.Fatal(diff) + } +} + +func TestPreventMistakesWithoutCategoriesAndEmpty(t *testing.T) { + input := []model.URLInfo{{ + CategoryCode: "NEWS", + URL: "https://repubblica.it/", + CountryCode: "IT", + }, { + CategoryCode: "HACK", + URL: "https://2600.com", + CountryCode: "XX", + }, { + CategoryCode: "FILE", + URL: "https://addons.mozilla.org/", + CountryCode: "XX", + }} + il := &InputLoader{} + output := il.preventMistakes(input, []string{}) + if diff := cmp.Diff(input, output); diff != "" { + t.Fatal(diff) + } +} + +// InputLoaderFakeLogger is a fake InputLoaderLogger. +type InputLoaderFakeLogger struct{} + +// Warnf implements InputLoaderLogger.Warnf +func (ilfl *InputLoaderFakeLogger) Warnf(format string, v ...interface{}) {} + +func TestInputLoaderLoggerWorksAsIntended(t *testing.T) { + logger := &InputLoaderFakeLogger{} + inputLoader := &InputLoader{Logger: logger} + out := inputLoader.logger() + if out != logger { + t.Fatal("logger not working as intended") + } +} From f48115a2c9101b15e141ec48ad1de14cb938f390 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 7 Apr 2021 13:57:28 +0200 Subject: [PATCH 3/4] chore: set version to v3.9.2 --- internal/version/version.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/version/version.go b/internal/version/version.go index 1f506bc..33d7915 100644 --- a/internal/version/version.go +++ b/internal/version/version.go @@ -3,5 +3,5 @@ package version const ( // Version is the software version - Version = "3.9.1" + Version = "3.9.2" ) From c5f52d51e340da94799f90f06c2493f68026353f Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 7 Apr 2021 14:55:04 +0200 Subject: [PATCH 4/4] fix: disable maxRuntime with --input or --input-file (#305) When this happens, the user is expressing the intention of explicitly testing all the input they provided. So, disable maxRuntime in these cases. Part of https://github.com/ooni/probe/issues/1436. --- cmd/ooniprobe/internal/nettests/nettests.go | 7 ++++++- cmd/ooniprobe/internal/nettests/web_connectivity.go | 1 - 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/cmd/ooniprobe/internal/nettests/nettests.go b/cmd/ooniprobe/internal/nettests/nettests.go index 5e5f9f2..ff9d987 100644 --- a/cmd/ooniprobe/internal/nettests/nettests.go +++ b/cmd/ooniprobe/internal/nettests/nettests.go @@ -125,6 +125,10 @@ func (c *Controller) Run(builder *engine.ExperimentBuilder, inputs []string) err log.Debug("disabling maxRuntime without Web Connectivity") maxRuntime = 0 } + if len(c.Inputs) > 0 || len(c.InputFiles) > 0 { + log.Debug("disabling maxRuntime with user-provided input") + maxRuntime = 0 + } start := time.Now() c.ntStartTime = start for idx, input := range inputs { @@ -220,7 +224,8 @@ func (c *Controller) OnProgress(perc float64, msg string) { // when we have maxRuntime, honor it maxRuntime := time.Duration(c.Probe.Config().Nettests.WebsitesMaxRuntime) * time.Second _, isWebConnectivity := c.nt.(WebConnectivity) - if c.RunType == "manual" && maxRuntime > 0 && isWebConnectivity { + userProvidedInput := len(c.Inputs) > 0 || len(c.InputFiles) > 0 + if c.RunType == "manual" && maxRuntime > 0 && isWebConnectivity && !userProvidedInput { elapsed := time.Since(c.ntStartTime) perc = float64(elapsed) / float64(maxRuntime) eta := maxRuntime.Seconds() - elapsed.Seconds() diff --git a/cmd/ooniprobe/internal/nettests/web_connectivity.go b/cmd/ooniprobe/internal/nettests/web_connectivity.go index 746e040..4ea021f 100644 --- a/cmd/ooniprobe/internal/nettests/web_connectivity.go +++ b/cmd/ooniprobe/internal/nettests/web_connectivity.go @@ -27,7 +27,6 @@ func lookupURLs(ctl *Controller, categories []string) ([]string, map[int64]int64 SourceFiles: ctl.InputFiles, StaticInputs: ctl.Inputs, } - log.Infof("Calling CheckIn API with %s runType", ctl.RunType) testlist, err := inputloader.Load(context.Background()) if err != nil { return nil, nil, err