From dad987917f1a9f545faa2fc91fdaf0f831727db8 Mon Sep 17 00:00:00 2001 From: Pavel Zbitskiy Date: Thu, 19 Aug 2021 13:06:17 -0400 Subject: [PATCH 1/3] Fix ParseHostOrURL and enable expect tests * ParseHostOrURL does not accepts ":xxx:123" anymore * pingpongTest.exp is still disabled --- network/wsNetwork.go | 5 +++++ network/wsNetwork_test.go | 4 ++++ test/e2e-go/cli/goal/expect/goalCmdFlagsTest.exp | 2 +- test/e2e-go/cli/goal/expect/goalExpectCommon.exp | 6 +++--- test/e2e-go/cli/goal/expect/goal_expect_test.go | 1 - test/e2e-go/cli/goal/expect/tealConsensusTest.exp | 3 ++- test/framework/fixtures/expectFixture.go | 7 +++++++ 7 files changed, 22 insertions(+), 6 deletions(-) diff --git a/network/wsNetwork.go b/network/wsNetwork.go index 0a0006531d..0749deb5d9 100644 --- a/network/wsNetwork.go +++ b/network/wsNetwork.go @@ -1937,6 +1937,11 @@ func ParseHostOrURL(addr string) (*url.URL, error) { // This turns "[::]:4601" into "http://[::]:4601" which url.Parse can do parsed, e2 := url.Parse("http://" + addr) if e2 == nil { + // https://datatracker.ietf.org/doc/html/rfc1123#section-2 + // first character is relaxed to allow either a letter or a digit + if parsed.Host[0] == ':' && parsed.Host[1] != ':' { + return nil, errors.New("host name starts with colon") + } return parsed, nil } return parsed, err /* return original err, not our prefix altered try */ diff --git a/network/wsNetwork_test.go b/network/wsNetwork_test.go index 55f1e33a5d..a91a81ccd1 100644 --- a/network/wsNetwork_test.go +++ b/network/wsNetwork_test.go @@ -2067,6 +2067,7 @@ func TestParseHostOrURL(t *testing.T) { {"1.2.3.4:123", url.URL{Scheme: "http", Host: "1.2.3.4:123"}}, {"[::]:123", url.URL{Scheme: "http", Host: "[::]:123"}}, {"r2-devnet.devnet.algodev.network:4560", url.URL{Scheme: "http", Host: "r2-devnet.devnet.algodev.network:4560"}}, + {"::11.22.33.44:123", url.URL{Scheme: "http", Host: "::11.22.33.44:123"}}, } badUrls := []string{ "justahost", @@ -2078,6 +2079,9 @@ func TestParseHostOrURL(t *testing.T) { "//localhost:WAT", "://badaddress", // See rpcs/blockService_test.go TestRedirectFallbackEndpoints "://localhost:1234", + ":xxx", + ":xxx:1234", + "::11.22.33.44", } for _, tc := range urlTestCases { t.Run(tc.text, func(t *testing.T) { diff --git a/test/e2e-go/cli/goal/expect/goalCmdFlagsTest.exp b/test/e2e-go/cli/goal/expect/goalCmdFlagsTest.exp index 99638add52..d86cef47b4 100644 --- a/test/e2e-go/cli/goal/expect/goalCmdFlagsTest.exp +++ b/test/e2e-go/cli/goal/expect/goalCmdFlagsTest.exp @@ -24,7 +24,7 @@ if { [catch { TestGoalCommandLineFlags "goal node start -p :xxx:445" ".*is not a valid peer address.*" TestGoalCommandLineFlags "goal node start -p {http://1.2.3.4:5050;:xxx:445}" ".*is not a valid peer address.*" TestGoalCommandLineFlags "goal node start -p http://1.2.3.4:5050" "^Data directory not specified.*" - TestGoalCommandLineFlags "goal node start -p {http://1.2.3.4:5050;5.6.7.8}" "^Data directory not specified.*" + TestGoalCommandLineFlags "goal node start -p {http://1.2.3.4:5050;5.6.7.8:1234}" "^Data directory not specified.*" } EXCEPTION ] } { puts "ERROR in Goal Start Validity test: $EXCEPTION" diff --git a/test/e2e-go/cli/goal/expect/goalExpectCommon.exp b/test/e2e-go/cli/goal/expect/goalExpectCommon.exp index 1d8f9d0ed6..273d02db91 100644 --- a/test/e2e-go/cli/goal/expect/goalExpectCommon.exp +++ b/test/e2e-go/cli/goal/expect/goalExpectCommon.exp @@ -112,7 +112,7 @@ proc ::AlgorandGoal::StartNode { TEST_ALGO_DIR {SYSTEMD_MANAGED "False"} {PEER_A if { $SYSTEMD_MANAGED eq "True" } { expect { timeout { close; ::AlgorandGoal::Abort "Did not receive appropriate message during node start" } - "^This node is managed by systemd, you must run the following command to make your desired state change to your node:*" { puts "Goal showed correct error message for systemd" ; close} + "^This node is using systemd and should be managed with systemctl*" { puts "Goal showed correct error message for systemd" ; close} eof { ::AlgorandGoal::CheckEOF "Unable to start network" } } } else { @@ -146,7 +146,7 @@ proc ::AlgorandGoal::StopNode { TEST_ALGO_DIR {SYSTEMD_MANAGED ""} } { spawn goal node stop -d $TEST_ALGO_DIR expect { timeout { close; ::AlgorandGoal::Abort "Did not receive appropriate message during node stop" } - "*This node is managed by systemd, you must run the following command to make your desired state change to your node:*" { puts "Goal showed correct error message for systemd" ; close} + "^This node is using systemd and should be managed with systemctl*" { puts "Goal showed correct error message for systemd" ; close} eof { close; ::AlgorandGoal::Abort "Did not receive appropriate message before goal command completion" } } } @@ -173,7 +173,7 @@ proc ::AlgorandGoal::RestartNode { TEST_ALGO_DIR {SYSTEMD_MANAGED ""} } { spawn goal node restart -d $TEST_ALGO_DIR expect { timeout { close; ::AlgorandGoal::Abort "Did not receive appropriate message during node restart" } - "^This node is managed by systemd, you must run the following command to make your desired state change to your node:*" { puts "Goal showed correct error message for systemd" ; close} + "^This node is using systemd and should be managed with systemctl*" { puts "Goal showed correct error message for systemd" ; close} } } } EXCEPTION] } { diff --git a/test/e2e-go/cli/goal/expect/goal_expect_test.go b/test/e2e-go/cli/goal/expect/goal_expect_test.go index c094c4543a..ce22987f07 100644 --- a/test/e2e-go/cli/goal/expect/goal_expect_test.go +++ b/test/e2e-go/cli/goal/expect/goal_expect_test.go @@ -24,7 +24,6 @@ import ( // TestGoalWithExpect Process all expect script files with suffix Test.exp within the test/e2e-go/cli/goal/expect directory func TestGoalWithExpect(t *testing.T) { - t.Skip("goal expect test are disabled due to flakiness") et := fixtures.MakeExpectTest(t) et.Run() } diff --git a/test/e2e-go/cli/goal/expect/tealConsensusTest.exp b/test/e2e-go/cli/goal/expect/tealConsensusTest.exp index a3c24d5797..cd2b81cfd6 100755 --- a/test/e2e-go/cli/goal/expect/tealConsensusTest.exp +++ b/test/e2e-go/cli/goal/expect/tealConsensusTest.exp @@ -68,7 +68,8 @@ if { [catch { "\n" { ::AlgorandGoal::Abort $expect_out(buffer) } } - teal "$TEST_ROOT_DIR/big-app.teal" 2 4097 1 "int 0\nbalance\npop\n" + # MaxAppProgramLen = 2K * (1 + 3 pages max) + teal "$TEST_ROOT_DIR/big-app.teal" 2 8192 1 "int 0\nbalance\npop\n" spawn goal clerk compile "$TEST_ROOT_DIR/big-app.teal" expect { -re {[A-Z2-9]{58}} { ::AlgorandGoal::Abort "hash" } diff --git a/test/framework/fixtures/expectFixture.go b/test/framework/fixtures/expectFixture.go index 3620c1368e..f926b9052d 100644 --- a/test/framework/fixtures/expectFixture.go +++ b/test/framework/fixtures/expectFixture.go @@ -113,8 +113,15 @@ func MakeExpectTest(t *testing.T) *ExpectFixture { // Run Process all expect script files with suffix Test.exp within the current directory func (ef *ExpectFixture) Run() { + disabledTest := map[string]bool{ + "pingpongTest.exp": true, + } for testName := range ef.expectFiles { if match, _ := regexp.MatchString(ef.testFilter, testName); match { + if disabledTest[testName] { + continue + } + ef.t.Run(testName, func(t *testing.T) { partitiontest.PartitionTest(t) // Check if this expect test should by run, may SKIP From 37495da401bde2f7a2184fcbdb892bdd46916e5e Mon Sep 17 00:00:00 2001 From: Pavel Zbitskiy Date: Thu, 19 Aug 2021 13:20:18 -0400 Subject: [PATCH 2/3] Add some edge cases to TestParseHostOrURL --- network/wsNetwork.go | 2 +- network/wsNetwork_test.go | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/network/wsNetwork.go b/network/wsNetwork.go index 0749deb5d9..d415ed396b 100644 --- a/network/wsNetwork.go +++ b/network/wsNetwork.go @@ -1939,7 +1939,7 @@ func ParseHostOrURL(addr string) (*url.URL, error) { if e2 == nil { // https://datatracker.ietf.org/doc/html/rfc1123#section-2 // first character is relaxed to allow either a letter or a digit - if parsed.Host[0] == ':' && parsed.Host[1] != ':' { + if parsed.Host[0] == ':' && (len(parsed.Host) < 2 || parsed.Host[1] != ':') { return nil, errors.New("host name starts with colon") } return parsed, nil diff --git a/network/wsNetwork_test.go b/network/wsNetwork_test.go index a91a81ccd1..e19d20a848 100644 --- a/network/wsNetwork_test.go +++ b/network/wsNetwork_test.go @@ -2082,6 +2082,12 @@ func TestParseHostOrURL(t *testing.T) { ":xxx", ":xxx:1234", "::11.22.33.44", + ":a:1", + ":a:", + ":1", + ":a", + ":", + "", } for _, tc := range urlTestCases { t.Run(tc.text, func(t *testing.T) { From 41d65b37090a0abbc26dc6c863e157b0c6aeaebb Mon Sep 17 00:00:00 2001 From: Pavel Zbitskiy Date: Thu, 19 Aug 2021 14:02:05 -0400 Subject: [PATCH 3/3] CR changes --- network/wsNetwork.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/network/wsNetwork.go b/network/wsNetwork.go index d415ed396b..ace79c5b0b 100644 --- a/network/wsNetwork.go +++ b/network/wsNetwork.go @@ -1913,6 +1913,8 @@ var errBcastQFull = errors.New("broadcast queue full") var errURLNoHost = errors.New("could not parse a host from url") +var errURLColonHost = errors.New("host name starts with a colon") + // HostColonPortPattern matches "^[-a-zA-Z0-9.]+:\\d+$" e.g. "foo.com.:1234" var HostColonPortPattern = regexp.MustCompile("^[-a-zA-Z0-9.]+:\\d+$") @@ -1940,7 +1942,7 @@ func ParseHostOrURL(addr string) (*url.URL, error) { // https://datatracker.ietf.org/doc/html/rfc1123#section-2 // first character is relaxed to allow either a letter or a digit if parsed.Host[0] == ':' && (len(parsed.Host) < 2 || parsed.Host[1] != ':') { - return nil, errors.New("host name starts with colon") + return nil, errURLColonHost } return parsed, nil }