Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix HTTP tunnel #2448

Merged
merged 5 commits into from
May 1, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/ReleaseNotes.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ Current package versions:
- Fix [#2426](https://github.com/StackExchange/StackExchange.Redis/issues/2426): Don't restrict multi-slot operations on Envoy proxy; let the proxy decide ([#2428 by mgravell](https://github.com/StackExchange/StackExchange.Redis/pull/2428))
- Add: Support for `User`/`Password` in `DefaultOptionsProvider` to support token rotation scenarios ([#2445 by NickCraver](https://github.com/StackExchange/StackExchange.Redis/pull/2445))
- Fix [#2449](https://github.com/StackExchange/StackExchange.Redis/issues/2449): Resolve AOT trim warnings in `TryGetAzureRoleInstanceIdNoThrow` ([#2451 by eerhardt](https://github.com/StackExchange/StackExchange.Redis/pull/2451))
- Adds: Support for `HTTP/1.1 200 Connection established` in HTTP Tunnel ([#2448 by flobernd](https://github.com/StackExchange/StackExchange.Redis/pull/2448))

## 2.6.104

Expand Down
16 changes: 9 additions & 7 deletions src/StackExchange.Redis/Configuration/Tunnel.cs
Original file line number Diff line number Diff line change
Expand Up @@ -49,10 +49,10 @@ private sealed class HttpProxyTunnel : Tunnel
{
var encoding = Encoding.ASCII;
var ep = Format.ToString(endpoint);
const string Prefix = "CONNECT ", Suffix = " HTTP/1.1\r\n\r\n", ExpectedResponse = "HTTP/1.1 200 OK\r\n\r\n";
const string Prefix = "CONNECT ", Suffix = " HTTP/1.1\r\n\r\n", ExpectedResponse1 = "HTTP/1.1 200 OK\r\n\r\n", ExpectedResponse2 = "HTTP/1.1 200 Connection established\r\n\r\n";
byte[] chunk = ArrayPool<byte>.Shared.Rent(Math.Max(
encoding.GetByteCount(Prefix) + encoding.GetByteCount(ep) + encoding.GetByteCount(Suffix),
encoding.GetByteCount(ExpectedResponse)
Math.Max(encoding.GetByteCount(ExpectedResponse1), encoding.GetByteCount(ExpectedResponse2))
));
var offset = 0;
offset += encoding.GetBytes(Prefix, 0, Prefix.Length, chunk, offset);
Expand All @@ -76,10 +76,11 @@ static void SafeAbort(object? obj)
await args;

// we expect to see: "HTTP/1.1 200 OK\n"; note our buffer is definitely big enough already
int toRead = encoding.GetByteCount(ExpectedResponse), read;
int toRead = Math.Max(encoding.GetByteCount(ExpectedResponse1), encoding.GetByteCount(ExpectedResponse2)), read;
offset = 0;

while (toRead > 0)
var actualResponse = "";
while (toRead > 0 && !actualResponse.EndsWith("\r\n\r\n"))
{
args.SetBuffer(chunk, offset, toRead);
if (!socket.ReceiveAsync(args)) args.Complete();
Expand All @@ -88,11 +89,12 @@ static void SafeAbort(object? obj)
if (read <= 0) break; // EOF (since we're never doing zero-length reads)
toRead -= read;
offset += read;

actualResponse = encoding.GetString(chunk, 0, offset);
}
if (toRead != 0) throw new EndOfStreamException("EOF negotiating HTTP tunnel");
if (toRead != 0 && !actualResponse.EndsWith("\r\n\r\n")) throw new EndOfStreamException("EOF negotiating HTTP tunnel");
// lazy
var actualResponse = encoding.GetString(chunk, 0, offset);
if (ExpectedResponse != actualResponse)
if (ExpectedResponse1 != actualResponse && ExpectedResponse2 != actualResponse)
{
throw new InvalidOperationException("Unexpected response negotiating HTTP tunnel");
}
Expand Down
20 changes: 13 additions & 7 deletions src/StackExchange.Redis/ConfigurationOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -912,19 +912,25 @@ private ConfigurationOptions DoParse(string configuration, bool ignoreUnknown)
{
Tunnel = null;
}
else if (value.StartsWith("http:"))
else
{
value = value.Substring(5);
if (!Format.TryParseEndPoint(value, out var ep))
// For backwards compatibility with `http:address_with_port`.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO we should add tests for this change, i.e. that we get the expected values back out - even if that means poking at internal state after the parse

Copy link
Contributor Author

@flobernd flobernd Apr 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the new format to the roundtrip test. This should be sufficient to confirm that the resulting Tunnel is the same for both input formats (HttpProxyTunnel.ToString() internally uses this.EndPoint.ToString() which means address and port must be the same to end up with the same string).

IMO it would make sense to consider deprecating the old format for the following reasons:

  • Roundtrip is inconsistent now, if you use the URI format
  • URI format is standard
  • URI format supports authentication (username + password) if needed at some point of time

Edit: If you prefer, we can apply the parsing change in a different PR (or remove it at all - it's up to you).
Edit 2: Not sure why the unrelated test fails after my latest commit. Seems a random failure, maybe you can re-run the CI.

if (value.StartsWith("http:") && !value.StartsWith("http://"))
{
value = value.Insert(5, "//");
}

var uri = new Uri(value, UriKind.Absolute);
if (uri.Scheme != "http")
{
throw new ArgumentException("Tunnel cannot be parsed: " + value);
}
if (!Format.TryParseEndPoint($"{uri.Host}:{uri.Port}", out var ep))
{
throw new ArgumentException("HTTP tunnel cannot be parsed: " + value);
}
Tunnel = Tunnel.HttpProxy(ep);
}
else
{
throw new ArgumentException("Tunnel cannot be parsed: " + value);
}
break;
// Deprecated options we ignore...
case OptionKeys.HighPrioritySocketThreads:
Expand Down
12 changes: 7 additions & 5 deletions tests/StackExchange.Redis.Tests/ConfigTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -619,19 +619,21 @@ public async Task MutableOptions()
Assert.Equal(newPass, conn.RawConfig.Password);
}

[Fact]
public void HttpTunnelCanRoundtrip()
[Theory]
[InlineData("http://somewhere:22", "http:somewhere:22")]
[InlineData("http:somewhere:22", "http:somewhere:22")]
public void HttpTunnelCanRoundtrip(string input, string expected)
{
var config = ConfigurationOptions.Parse("127.0.0.1:6380,tunnel=http:somewhere:22");
var config = ConfigurationOptions.Parse($"127.0.0.1:6380,tunnel={input}");
var ip = Assert.IsType<IPEndPoint>(Assert.Single(config.EndPoints));
Assert.Equal(6380, ip.Port);
Assert.Equal("127.0.0.1", ip.Address.ToString());

Assert.NotNull(config.Tunnel);
Assert.Equal("http:somewhere:22", config.Tunnel.ToString());
Assert.Equal(expected, config.Tunnel.ToString());

var cs = config.ToString();
Assert.Equal("127.0.0.1:6380,tunnel=http:somewhere:22", cs);
Assert.Equal($"127.0.0.1:6380,tunnel={expected}", cs);
}

private class CustomTunnel : Tunnel { }
Expand Down