Skip to content

Commit

Permalink
feat(dnscheck): implement richer input (#1618)
Browse files Browse the repository at this point in the history 
This diff:

1. modifies `./internal/registry` and its `dnscheck.go` file such that
`dnscheck` has its own private target loader;
2. modifies `./internal/targetloading` to allow reusing the code to read
static input from CLI and from files;
3. modifies `./internal/model` to pass optional richer input to
experiments;
4. modifies `./internal/engine` to pass richer input to experiments;
5. modifies `./internal/experiment/dnscheck` to read and use the richer
input it is passed;
6. implements a custom target loader for
`./internal/experiment/dnscheck` returning some static entries;
7. modifies `./internal/oonirun` to make sure richer-input experiments
honor their options by moving option loading before the loading of
targets.

Once this diff is merged, `miniooni` and `ooniprobe` will run `dnscheck`
with richer input. Obviously, we are still lacking an API to fetch
richer input, so for now this is static richer input, suitable as a
starting point to land more diffs.

Part of ooni/probe#2607.
  • Loading branch information
@bassosimone
bassosimone authored Jun 7, 2024
1 parent 3424bd0 commit 41a4282
Show file tree
Hide file tree
Showing 20 changed files with 909 additions and 109 deletions.
29 changes: 22 additions & 7 deletions internal/engine/experiment.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,11 +109,16 @@ func (e *experiment) SubmitAndUpdateMeasurementContext(
}

// newMeasurement creates a new measurement for this experiment with the given input.
func (e *experiment) newMeasurement(input string) *model.Measurement {
func (e *experiment) newMeasurement(target model.ExperimentTarget) *model.Measurement {
utctimenow := time.Now().UTC()
// TODO(bassosimone,DecFox): move here code that supports filling the options field
// when there is richer input, which currently is inside ./internal/oonirun.
//
// We MUST do this because the current solution only works for OONI Run and when
// there are command line options but does not work for API/static targets.
m := &model.Measurement{
DataFormatVersion: model.OOAPIReportDefaultDataFormatVersion,
Input: model.MeasurementInput(input),
Input: model.MeasurementInput(target.Input()),
MeasurementStartTime: utctimenow.Format(model.MeasurementDateFormat),
MeasurementStartTimeSaved: utctimenow,
ProbeIP: model.DefaultProbeIP,
Expand Down Expand Up @@ -204,19 +209,29 @@ func (e *experiment) MeasureWithContext(
ctx = bytecounter.WithExperimentByteCounter(ctx, e.byteCounter)

// Create a new measurement that the experiment measurer will finish filling
// by adding the test keys etc. Please, note that, as of 2024-06-05, we're using
// the measurement Input to provide input to an experiment. We'll probably
// change this, when we'll have finished implementing richer input.
measurement := e.newMeasurement(target.Input())
// by adding the test keys etc. Please, note that, as of 2024-06-06:
//
// 1. Experiments using richer input receive input via the Target field
// and ignore (*Measurement).Input, which however contains the same value
// that would be returned by the Target.Input method.
//
// 2. Other experiments use (*Measurement).Input.
//
// Here we're passing the whole target to newMeasurement such that we're able
// to record options values in addition to the input value.
measurement := e.newMeasurement(target)

// Record when we started the experiment, to compute the runtime.
start := time.Now()

// Prepare the arguments for the experiment measurer
// Prepare the arguments for the experiment measurer.
//
// Only richer-input-aware experiments honour the Target field.
args := &model.ExperimentArgs{
Callbacks: e.callbacks,
Measurement: measurement,
Session: e.session,
Target: target,
}

// Invoke the measurer. Conventionally, an error being returned here
Expand Down
2 changes: 1 addition & 1 deletion internal/engine/experiment_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ func TestExperimentHonoursSharingDefaults(t *testing.T) {
t.Fatal(err)
}
exp := builder.NewExperiment().(*experiment)
return exp.newMeasurement("")
return exp.newMeasurement(model.NewOOAPIURLInfoWithDefaultCategoryAndCountry(""))
}
type spec struct {
name string
Expand Down
50 changes: 28 additions & 22 deletions internal/experiment/dnscheck/dnscheck.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"github.com/ooni/probe-cli/v3/internal/legacy/tracex"
"github.com/ooni/probe-cli/v3/internal/model"
"github.com/ooni/probe-cli/v3/internal/runtimex"
"github.com/ooni/probe-cli/v3/internal/targetloading"
)

const (
Expand Down Expand Up @@ -96,7 +97,6 @@ type TestKeys struct {

// Measurer performs the measurement.
type Measurer struct {
Config
Endpoints *Endpoints
}

Expand All @@ -114,7 +114,7 @@ func (m *Measurer) ExperimentVersion() string {
// errors are in addition to any other errors returned by the low level packages
// that are used by this experiment to implement its functionality.
var (
ErrInputRequired = errors.New("this experiment needs input")
ErrInputRequired = targetloading.ErrInputRequired
ErrInvalidURL = errors.New("the input URL is invalid")
ErrUnsupportedURLScheme = errors.New("unsupported URL scheme")
)
Expand All @@ -125,6 +125,14 @@ func (m *Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error {
measurement := args.Measurement
sess := args.Session

// 0. obtain the richer input target, config, and input or panic
if args.Target == nil {
return ErrInputRequired
}
target := args.Target.(*Target)
config, input := target.Options, target.URL
sess.Logger().Infof("dnscheck: using richer input: %+v %+v", config, input)

// 1. fill the measurement with test keys
tk := new(TestKeys)
tk.Lookups = make(map[string]urlgetter.TestKeys)
Expand All @@ -133,20 +141,19 @@ func (m *Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error {

// 2. select the domain to resolve or use default and, while there, also
// ensure that we register all the other options we're using.
domain := m.Config.Domain
domain := config.Domain
if domain == "" {
domain = defaultDomain
}
tk.DefaultAddrs = m.Config.DefaultAddrs
tk.DefaultAddrs = config.DefaultAddrs
tk.Domain = domain
tk.HTTP3Enabled = m.Config.HTTP3Enabled
tk.HTTPHost = m.Config.HTTPHost
tk.TLSServerName = m.Config.TLSServerName
tk.TLSVersion = m.Config.TLSVersion
tk.HTTP3Enabled = config.HTTP3Enabled
tk.HTTPHost = config.HTTPHost
tk.TLSServerName = config.TLSServerName
tk.TLSVersion = config.TLSVersion
tk.Residual = m.Endpoints != nil

// 3. parse the input URL describing the resolver to use
input := string(measurement.Input)
if input == "" {
return ErrInputRequired
}
Expand Down Expand Up @@ -191,7 +198,7 @@ func (m *Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error {
for _, addr := range addrs {
allAddrs[addr] = true
}
for _, addr := range strings.Split(m.Config.DefaultAddrs, " ") {
for _, addr := range strings.Split(config.DefaultAddrs, " ") {
if addr != "" {
allAddrs[addr] = true
}
Expand All @@ -208,10 +215,10 @@ func (m *Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error {
for addr := range allAddrs {
inputs = append(inputs, urlgetter.MultiInput{
Config: urlgetter.Config{
DNSHTTPHost: m.httpHost(URL.Host),
DNSTLSServerName: m.tlsServerName(URL.Hostname()),
DNSTLSVersion: m.Config.TLSVersion,
HTTP3Enabled: m.Config.HTTP3Enabled,
DNSHTTPHost: config.httpHost(URL.Host),
DNSTLSServerName: config.tlsServerName(URL.Hostname()),
DNSTLSVersion: config.TLSVersion,
HTTP3Enabled: config.HTTP3Enabled,
RejectDNSBogons: true, // bogons are errors in this context
ResolverURL: makeResolverURL(URL, addr),
Timeout: 15 * time.Second,
Expand Down Expand Up @@ -244,17 +251,17 @@ func (m *Measurer) lookupHost(ctx context.Context, hostname string, r model.Reso

// httpHost returns the configured HTTP host, if set, otherwise
// it will return the host provide as argument.
func (m *Measurer) httpHost(httpHost string) string {
if m.Config.HTTPHost != "" {
return m.Config.HTTPHost
func (c *Config) httpHost(httpHost string) string {
if c.HTTPHost != "" {
return c.HTTPHost
}
return httpHost
}

// tlsServerName is like httpHost for the TLS server name.
func (m *Measurer) tlsServerName(tlsServerName string) string {
if m.Config.TLSServerName != "" {
return m.Config.TLSServerName
func (c *Config) tlsServerName(tlsServerName string) string {
if c.TLSServerName != "" {
return c.TLSServerName
}
return tlsServerName
}
Expand Down Expand Up @@ -311,9 +318,8 @@ func makeResolverURL(URL *url.URL, addr string) string {
}

// NewExperimentMeasurer creates a new ExperimentMeasurer.
func NewExperimentMeasurer(config Config) model.ExperimentMeasurer {
func NewExperimentMeasurer() model.ExperimentMeasurer {
return &Measurer{
Config: config,
Endpoints: nil, // disabled by default
}
}
89 changes: 65 additions & 24 deletions internal/experiment/dnscheck/dnscheck_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,44 +8,40 @@ import (
"time"

"github.com/apex/log"
"github.com/ooni/probe-cli/v3/internal/legacy/mockable"
"github.com/ooni/probe-cli/v3/internal/mocks"
"github.com/ooni/probe-cli/v3/internal/model"
)

func TestHTTPHostWithOverride(t *testing.T) {
m := Measurer{Config: Config{HTTPHost: "antani"}}
result := m.httpHost("mascetti")
if result != "antani" {
c := &Config{HTTPHost: "antani"}
if result := c.httpHost("mascetti"); result != "antani" {
t.Fatal("not the result we expected")
}
}

func TestHTTPHostWithoutOverride(t *testing.T) {
m := Measurer{Config: Config{}}
result := m.httpHost("mascetti")
if result != "mascetti" {
c := &Config{}
if result := c.httpHost("mascetti"); result != "mascetti" {
t.Fatal("not the result we expected")
}
}

func TestTLSServerNameWithOverride(t *testing.T) {
m := Measurer{Config: Config{TLSServerName: "antani"}}
result := m.tlsServerName("mascetti")
if result != "antani" {
c := &Config{TLSServerName: "antani"}
if result := c.tlsServerName("mascetti"); result != "antani" {
t.Fatal("not the result we expected")
}
}

func TestTLSServerNameWithoutOverride(t *testing.T) {
m := Measurer{Config: Config{}}
result := m.tlsServerName("mascetti")
if result != "mascetti" {
c := &Config{}
if result := c.tlsServerName("mascetti"); result != "mascetti" {
t.Fatal("not the result we expected")
}
}

func TestExperimentNameAndVersion(t *testing.T) {
measurer := NewExperimentMeasurer(Config{Domain: "example.com"})
measurer := NewExperimentMeasurer()
if measurer.ExperimentName() != "dnscheck" {
t.Error("unexpected experiment name")
}
Expand All @@ -55,11 +51,17 @@ func TestExperimentNameAndVersion(t *testing.T) {
}

func TestDNSCheckFailsWithoutInput(t *testing.T) {
measurer := NewExperimentMeasurer(Config{Domain: "example.com"})
measurer := NewExperimentMeasurer()
args := &model.ExperimentArgs{
Callbacks: model.NewPrinterCallbacks(log.Log),
Measurement: new(model.Measurement),
Session: newsession(),
Target: &Target{
URL: "", // explicitly empty
Options: &Config{
Domain: "example.com",
},
},
}
err := measurer.Run(context.Background(), args)
if !errors.Is(err, ErrInputRequired) {
Expand All @@ -68,11 +70,15 @@ func TestDNSCheckFailsWithoutInput(t *testing.T) {
}

func TestDNSCheckFailsWithInvalidURL(t *testing.T) {
measurer := NewExperimentMeasurer(Config{})
measurer := NewExperimentMeasurer()
args := &model.ExperimentArgs{
Callbacks: model.NewPrinterCallbacks(log.Log),
Measurement: &model.Measurement{Input: "Not a valid URL \x7f"},
Session: newsession(),
Target: &Target{
URL: "Not a valid URL \x7f",
Options: &Config{},
},
}
err := measurer.Run(context.Background(), args)
if !errors.Is(err, ErrInvalidURL) {
Expand All @@ -81,11 +87,15 @@ func TestDNSCheckFailsWithInvalidURL(t *testing.T) {
}

func TestDNSCheckFailsWithUnsupportedProtocol(t *testing.T) {
measurer := NewExperimentMeasurer(Config{})
measurer := NewExperimentMeasurer()
args := &model.ExperimentArgs{
Callbacks: model.NewPrinterCallbacks(log.Log),
Measurement: &model.Measurement{Input: "file://1.1.1.1"},
Session: newsession(),
Target: &Target{
URL: "file://1.1.1.1",
Options: &Config{},
},
}
err := measurer.Run(context.Background(), args)
if !errors.Is(err, ErrUnsupportedURLScheme) {
Expand All @@ -96,21 +106,40 @@ func TestDNSCheckFailsWithUnsupportedProtocol(t *testing.T) {
func TestWithCancelledContext(t *testing.T) {
ctx, cancel := context.WithCancel(context.Background())
cancel() // immediately cancel the context
measurer := NewExperimentMeasurer(Config{
DefaultAddrs: "1.1.1.1 1.0.0.1",
})
measurer := NewExperimentMeasurer()
measurement := &model.Measurement{Input: "dot://one.one.one.one"}
args := &model.ExperimentArgs{
Callbacks: model.NewPrinterCallbacks(log.Log),
Measurement: measurement,
Session: newsession(),
Target: &Target{
URL: "dot://one.one.one.one",
Options: &Config{
DefaultAddrs: "1.1.1.1 1.0.0.1",
},
},
}
err := measurer.Run(ctx, args)
if err != nil {
t.Fatal(err)
}
}

func TestDNSCheckFailsWithNilTarget(t *testing.T) {
measurer := NewExperimentMeasurer()
measurement := &model.Measurement{Input: "dot://one.one.one.one"}
args := &model.ExperimentArgs{
Callbacks: model.NewPrinterCallbacks(log.Log),
Measurement: measurement,
Session: newsession(),
Target: nil, // explicitly nil
}
err := measurer.Run(context.Background(), args)
if !errors.Is(err, ErrInputRequired) {
t.Fatal("unexpected err", err)
}
}

func TestMakeResolverURL(t *testing.T) {
// test address substitution
addr := "255.255.255.0"
Expand Down Expand Up @@ -140,14 +169,18 @@ func TestDNSCheckValid(t *testing.T) {
t.Skip("skip test in short mode")
}

measurer := NewExperimentMeasurer(Config{
DefaultAddrs: "1.1.1.1 1.0.0.1",
})
measurer := NewExperimentMeasurer()
measurement := model.Measurement{Input: "dot://one.one.one.one:853"}
args := &model.ExperimentArgs{
Callbacks: model.NewPrinterCallbacks(log.Log),
Measurement: &measurement,
Session: newsession(),
Target: &Target{
URL: "dot://one.one.one.one:853",
Options: &Config{
DefaultAddrs: "1.1.1.1 1.0.0.1",
},
},
}
err := measurer.Run(context.Background(), args)
if err != nil {
Expand All @@ -169,7 +202,11 @@ func TestDNSCheckValid(t *testing.T) {
}

func newsession() model.ExperimentSession {
return &mockable.Session{MockableLogger: log.Log}
return &mocks.Session{
MockLogger: func() model.Logger {
return log.Log
},
}
}

func TestDNSCheckWait(t *testing.T) {
Expand All @@ -187,6 +224,10 @@ func TestDNSCheckWait(t *testing.T) {
Callbacks: model.NewPrinterCallbacks(log.Log),
Measurement: &measurement,
Session: newsession(),
Target: &Target{
URL: input,
Options: &Config{},
},
}
err := measurer.Run(context.Background(), args)
if err != nil {
Expand Down
Loading

0 comments on commit 41a4282

Please sign in to comment.