Skip to content

Commit

Permalink
document why some QA tests with redirects are broken
Browse files Browse the repository at this point in the history
  • Loading branch information
bassosimone committed Nov 23, 2023
1 parent 766d4d0 commit f3701a0
Show file tree
Hide file tree
Showing 3 changed files with 31 additions and 2 deletions.
8 changes: 8 additions & 0 deletions internal/experiment/webconnectivitylte/analysishttpcore.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ package webconnectivitylte
//

import (
"log"

"github.com/ooni/probe-cli/v3/internal/model"
"github.com/ooni/probe-cli/v3/internal/netxlite"
)
Expand Down Expand Up @@ -33,6 +35,12 @@ func (tk *TestKeys) analysisHTTPToplevel(logger model.Logger) {
finalRequest := tk.Requests[0]
tk.HTTPExperimentFailure = finalRequest.Failure

{
for idx, req := range tk.Requests {
log.Printf("REQUEST STACK: #%d %s", idx, req.Request.URL)
}
}

// don't perform any futher analysis without TH data
if tk.Control == nil || tk.ControlRequest == nil {
return
Expand Down
23 changes: 22 additions & 1 deletion internal/experiment/webconnectivitylte/secureflow.go
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,27 @@ func (t *SecureFlow) Run(parentCtx context.Context, index int64) error {
return err
}

// TODO(bassosimone): we're missing high-level information about the
// transaction, which implies we are missing failed HTTP requests
// to compare to, which causes netemx integration tests such as this
//
// TestQA/redirectWithConsistentDNSAndThenConnectionRefusedForHTTP
//
// to fail because they cannot find the corresponding request to
// compare to. Therefore, in such a test case, we incorrectly say
// that bit.ly is accessible where instead we should conclude
// there's blocking at the TCP/IP level.
//
// An initial fix for this issue probably relies on making sure
// that we don't conclude there is a success when the actual
// endpoint fails. However, I think there are broader ooni/data
// implications at stake here. The fact that we do not have a
// request violates the assumptions of the analysis code we have
// written, which means, whatever we do, we need to respect the
// previously agreed conventions in some way.
//
// For this reason, any fix for this should be carefully thought out.

// create trace
trace := measurexlite.NewTrace(index, t.ZeroTime)

Expand Down Expand Up @@ -349,7 +370,7 @@ func (t *SecureFlow) maybeFollowRedirects(ctx context.Context, resp *http.Respon
return // broken response from server
}
// TODO(https://github.com/ooni/probe/issues/2628): we need to handle
// the care where the redirect URL is incomplete
// the case where the redirect URL is incomplete
t.Logger.Infof("redirect to: %s", location.String())
resolvers := &DNSResolvers{
CookieJar: t.CookieJar,
Expand Down
2 changes: 1 addition & 1 deletion internal/experiment/webconnectivityqa/redirect.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import (
func redirectWithConsistentDNSAndThenConnectionRefusedForHTTP() *TestCase {
return &TestCase{
Name: "redirectWithConsistentDNSAndThenConnectionRefusedForHTTP",
Flags: TestCaseFlagNoLTE, // BUG: LTE thinks this website is accessible (WTF?!)
Flags: 0, // BUG: LTE thinks this website is accessible (WTF?!)
Input: "https://bit.ly/32447",
Configure: func(env *netemx.QAEnv) {

Expand Down

0 comments on commit f3701a0

Please sign in to comment.