server: refactor TestAdminDebugRedirect test

Adjusts test to use more standard redirect ignorig behavior in stdlib,
and removes the test tenant override since this test works with
tenants now after some adjustements to URL handling.

The cockroachdb#120095 issue was a timeout that this change doesn't explicitly
deal with here since that problem isn't reproducible. The hope is
that modified redirect error handling might trigger a less error-prone
branch in the HTTP-client. There's nothing else to really change in
this test since it's quite simple and we haven't seen similar timeouts
persist in other HTTP tests.

Resolves: cockroachdb#120095
Resolves: cockroachdb#112955
Epic: None

Release note: None

pkg/server/debug/BUILD.bazel

@@ -65,7 +65,6 @@ go_test(
         "//pkg/server/srvtestutils",
         "//pkg/testutils",
         "//pkg/testutils/serverutils",
-        "//pkg/testutils/skip",
         "//pkg/testutils/testcluster",
         "//pkg/util/leaktest",
         "//pkg/util/log",

pkg/server/debug/debug_test.go

@@ -14,22 +14,19 @@ import (
 	"bytes"
 	"context"
 	"net/http"
-	"net/url"
 	"testing"
 
 	"github.com/cockroachdb/cockroach/pkg/base"
 	"github.com/cockroachdb/cockroach/pkg/server/debug"
 	"github.com/cockroachdb/cockroach/pkg/server/srvtestutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
-	"github.com/cockroachdb/cockroach/pkg/testutils/skip"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
-	"github.com/cockroachdb/errors"
 )
 
 // debugURL returns the root debug URL.
-func debugURL(s serverutils.ApplicationLayerInterface, path string) string {
-	return s.AdminURL().WithPath(debug.Endpoint).WithPath(path).String()
+func debugURL(s serverutils.ApplicationLayerInterface, path string) *serverutils.TestURL {
+	return s.AdminURL().WithPath(debug.Endpoint).WithPath(path)
 }
 
 // TestAdminDebugExpVar verifies that cmdline and memstats variables are
@@ -46,7 +43,7 @@ func TestAdminDebugExpVar(t *testing.T) {
 
 	ts := s.ApplicationLayer()
 
-	jI, err := srvtestutils.GetJSON(ts, debugURL(ts, "vars"))
+	jI, err := srvtestutils.GetJSON(ts, debugURL(ts, "vars").String())
 	if err != nil {
 		t.Fatalf("failed to fetch JSON: %v", err)
 	}
@@ -73,7 +70,7 @@ func TestAdminDebugMetrics(t *testing.T) {
 
 	ts := s.ApplicationLayer()
 
-	jI, err := srvtestutils.GetJSON(ts, debugURL(ts, "metrics"))
+	jI, err := srvtestutils.GetJSON(ts, debugURL(ts, "metrics").String())
 	if err != nil {
 		t.Fatalf("failed to fetch JSON: %v", err)
 	}
@@ -100,7 +97,7 @@ func TestAdminDebugPprof(t *testing.T) {
 
 	ts := s.ApplicationLayer()
 
-	body, err := srvtestutils.GetText(ts, debugURL(ts, "pprof/block?debug=1"))
+	body, err := srvtestutils.GetText(ts, debugURL(ts, "pprof/block?debug=1").String())
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -130,7 +127,7 @@ func TestAdminDebugTrace(t *testing.T) {
 	}
 
 	for _, c := range tc {
-		body, err := srvtestutils.GetText(ts, debugURL(ts, c.segment))
+		body, err := srvtestutils.GetText(ts, debugURL(ts, c.segment).String())
 		if err != nil {
 			t.Fatal(err)
 		}
@@ -148,7 +145,7 @@ func TestAdminDebugAuth(t *testing.T) {
 	defer s.Stopper().Stop(context.Background())
 	ts := s.ApplicationLayer()
 
-	url := debugURL(ts, "")
+	url := debugURL(ts, "").String()
 
 	// Unauthenticated.
 	client, err := ts.GetUnauthenticatedHTTPClient()
@@ -199,17 +196,14 @@ func TestAdminDebugRedirect(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
 
-	skip.WithIssue(t, 120095)
-
-	s := serverutils.StartServerOnly(t, base.TestServerArgs{
-		DefaultTestTenant: base.TestIsForStuffThatShouldWorkWithSharedProcessModeButDoesntYet(
-			base.TestTenantProbabilistic, 112955,
-		),
-	})
+	s := serverutils.StartServerOnly(t, base.TestServerArgs{})
 	defer s.Stopper().Stop(context.Background())
 	ts := s.ApplicationLayer()
 
 	expURL := debugURL(ts, "/")
+	// Drops the `?cluster=` query param if present.
+	expURL.RawQuery = ""
+
 	origURL := debugURL(ts, "/incorrect")
 
 	// Must be admin to access debug endpoints
@@ -218,29 +212,24 @@ func TestAdminDebugRedirect(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	// Don't follow redirects automatically.
-	redirectAttemptedError := errors.New("redirect")
 	client.CheckRedirect = func(req *http.Request, via []*http.Request) error {
-		return redirectAttemptedError
+		// Don't follow redirects automatically. This error is a special
+		// case in the `CheckRedirect` docs that forwards the last response
+		// instead of following the redirect.
+		return http.ErrUseLastResponse
 	}
 
-	resp, err := client.Get(origURL)
-	if urlError := (*url.Error)(nil); errors.As(err, &urlError) &&
-		errors.Is(urlError.Err, redirectAttemptedError) {
-		// Ignore the redirectAttemptedError.
-		err = nil
-	}
+	resp, err := client.Get(origURL.String())
 	if err != nil {
 		t.Fatal(err)
-	} else {
-		resp.Body.Close()
-		if resp.StatusCode != http.StatusMovedPermanently {
-			t.Errorf("expected status code %d; got %d", http.StatusMovedPermanently, resp.StatusCode)
-		}
-		if redirectURL, err := resp.Location(); err != nil {
-			t.Error(err)
-		} else if foundURL := redirectURL.String(); foundURL != expURL {
-			t.Errorf("expected location %s; got %s", expURL, foundURL)
-		}
+	}
+	defer resp.Body.Close()
+	if resp.StatusCode != http.StatusMovedPermanently {
+		t.Errorf("expected status code %d; got %d", http.StatusMovedPermanently, resp.StatusCode)
+	}
+	if redirectURL, err := resp.Location(); err != nil {
+		t.Error(err)
+	} else if foundURL := redirectURL.String(); foundURL != expURL.String() {
+		t.Errorf("expected location %s; got %s", expURL, foundURL)
 	}
 }