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.

Sign up for GitHub

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

Jump to bottom

netty: Support pseudo headers in all GrpcHttp2RequestHeaders methods #9004

Merged
merged 5 commits into from Mar 22, 2022
Merged

netty: Support pseudo headers in all GrpcHttp2RequestHeaders methods #9004

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1e59f1a
netty: Support pseudo headers in all GrpcHttp2RequestHeaders methods
ejona86 Mar 21, 2022
3afddae
Update GrpcHttp2HeadersUtilsTest.java
ejona86 Mar 21, 2022
ce8fcf5
Use getall() instead of get()
ejona86 Mar 21, 2022
658d4ce
Tests for exceptions
ejona86 Mar 21, 2022
28bc057
Fix style; oops copy/paste
ejona86 Mar 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
61 changes: 36 additions & 25 deletions netty/src/main/java/io/grpc/netty/GrpcHttp2HeadersUtils.java
View file
Open in desktop
Expand Up @@ -340,7 +340,12 @@ public Http2Headers add(CharSequence csName, CharSequence csValue) {
AsciiString name = validateName(requireAsciiString(csName));
AsciiString value = requireAsciiString(csValue);
if (isPseudoHeader(name)) {
addPseudoHeader(name, value);
AsciiString previous = getPseudoHeader(name);
Copy link
Member Author

Choose a reason for hiding this comment

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

It'd be possible to implement this without a get-then-set if setPseudoHeader() returned the old value. We'd have to re-set the old value if there was one already present, but that's not the worst. Returning-old-valuesetPseudoHeader() would seem to most benefit remove(), but it isn't safe for remove because it would throw for remove(":status'). Having the returning-old-value setPseudoHeader() made it really easy to make mistakes, so I went with the KISS approach. These code paths don't matter for performance.

if (previous != null) {
PlatformDependent.throwException(
connectionError(PROTOCOL_ERROR, "Duplicate %s header", name));
}
setPseudoHeader(name, value);
return this;
}
if (equals(TE_HEADER, name)) {
Expand All @@ -353,44 +358,42 @@ public Http2Headers add(CharSequence csName, CharSequence csValue) {
@Override
public CharSequence get(CharSequence csName) {
AsciiString name = requireAsciiString(csName);
checkArgument(!isPseudoHeader(name), "Use direct accessor methods for pseudo headers.");
if (isPseudoHeader(name)) {
return getPseudoHeader(name);
}
if (equals(TE_HEADER, name)) {
return te;
}
return get(name);
}

private void addPseudoHeader(CharSequence csName, CharSequence csValue) {
AsciiString name = requireAsciiString(csName);
AsciiString value = requireAsciiString(csValue);
private AsciiString getPseudoHeader(AsciiString name) {
ejona86 marked this conversation as resolved.
Show resolved Hide resolved
if (equals(PATH_HEADER, name)) {
return path;
} else if (equals(AUTHORITY_HEADER, name)) {
return authority;
} else if (equals(METHOD_HEADER, name)) {
return method;
} else if (equals(SCHEME_HEADER, name)) {
return scheme;
} else {
return null;
}
}

private void setPseudoHeader(AsciiString name, AsciiString value) {
if (equals(PATH_HEADER, name)) {
if (path != null) {
PlatformDependent.throwException(
connectionError(PROTOCOL_ERROR, "Duplicate :path header"));
}
path = value;
} else if (equals(AUTHORITY_HEADER, name)) {
if (authority != null) {
PlatformDependent.throwException(
connectionError(PROTOCOL_ERROR, "Duplicate :authority header"));
}
authority = value;
} else if (equals(METHOD_HEADER, name)) {
if (method != null) {
PlatformDependent.throwException(
connectionError(PROTOCOL_ERROR, "Duplicate :method header"));
}
method = value;
} else if (equals(SCHEME_HEADER, name)) {
if (scheme != null) {
PlatformDependent.throwException(
connectionError(PROTOCOL_ERROR, "Duplicate :scheme header"));
}
scheme = value;
} else {
PlatformDependent.throwException(
connectionError(PROTOCOL_ERROR, "Illegal pseudo-header '%s' in request.", name));
throw new AssertionError(); // Make flow control obvious to javac
}
}

Expand Down Expand Up @@ -418,8 +421,12 @@ public CharSequence scheme() {
public List<CharSequence> getAll(CharSequence csName) {
AsciiString name = requireAsciiString(csName);
if (isPseudoHeader(name)) {
// This code should never be reached.
throw new IllegalArgumentException("Use direct accessor methods for pseudo headers.");
AsciiString value = getPseudoHeader(name);
if (value == null) {
return Collections.emptyList();
} else {
return Collections.singletonList(value);
}
ejona86 marked this conversation as resolved.
Show resolved Hide resolved
}
if (equals(TE_HEADER, name)) {
return Collections.singletonList((CharSequence) te);
ejona86 marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -431,8 +438,12 @@ public List<CharSequence> getAll(CharSequence csName) {
public boolean remove(CharSequence csName) {
AsciiString name = requireAsciiString(csName);
if (isPseudoHeader(name)) {
// This code should never be reached.
throw new IllegalArgumentException("Use direct accessor methods for pseudo headers.");
if (getPseudoHeader(name) == null) {
return false;
} else {
setPseudoHeader(name, null);
return true;
}
}
if (equals(TE_HEADER, name)) {
boolean wasPresent = te != null;
Expand Down
125 changes: 125 additions & 0 deletions netty/src/test/java/io/grpc/netty/GrpcHttp2HeadersUtilsTest.java
View file
Open in desktop
Expand Up @@ -22,6 +22,7 @@
import static io.netty.util.AsciiString.of;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;

import com.google.common.collect.Iterables;
import com.google.common.io.BaseEncoding;
Expand Down Expand Up @@ -133,6 +134,130 @@ public void decode_emptyHeaders() throws Http2Exception {
assertThat(decodedHeaders.toString()).contains("[]");
}

// contains() is used by Netty 4.1.75+. https://github.com/grpc/grpc-java/issues/8981
// Just implement everything pseudo headers for all methods; too many recent breakages.
@Test
public void grpcHttp2RequestHeaders_pseudoHeaders_notPresent() {
Http2Headers http2Headers = new GrpcHttp2RequestHeaders(2);
assertThat(http2Headers.get(AsciiString.of(":path"))).isNull();
assertThat(http2Headers.get(AsciiString.of(":authority"))).isNull();
assertThat(http2Headers.get(AsciiString.of(":method"))).isNull();
assertThat(http2Headers.get(AsciiString.of(":scheme"))).isNull();
assertThat(http2Headers.get(AsciiString.of(":status"))).isNull();

assertThat(http2Headers.getAll(AsciiString.of(":path"))).isEmpty();
assertThat(http2Headers.getAll(AsciiString.of(":authority"))).isEmpty();
assertThat(http2Headers.getAll(AsciiString.of(":method"))).isEmpty();
assertThat(http2Headers.getAll(AsciiString.of(":scheme"))).isEmpty();
assertThat(http2Headers.getAll(AsciiString.of(":status"))).isEmpty();
ejona86 marked this conversation as resolved.
Show resolved Hide resolved

assertThat(http2Headers.contains(AsciiString.of(":path"))).isFalse();
assertThat(http2Headers.contains(AsciiString.of(":authority"))).isFalse();
assertThat(http2Headers.contains(AsciiString.of(":method"))).isFalse();
assertThat(http2Headers.contains(AsciiString.of(":scheme"))).isFalse();
assertThat(http2Headers.contains(AsciiString.of(":status"))).isFalse();

assertThat(http2Headers.remove(AsciiString.of(":path"))).isFalse();
assertThat(http2Headers.remove(AsciiString.of(":authority"))).isFalse();
assertThat(http2Headers.remove(AsciiString.of(":method"))).isFalse();
assertThat(http2Headers.remove(AsciiString.of(":scheme"))).isFalse();
assertThat(http2Headers.remove(AsciiString.of(":status"))).isFalse();
}

@Test
public void grpcHttp2RequestHeaders_pseudoHeaders_present() {
Http2Headers http2Headers = new GrpcHttp2RequestHeaders(2);
http2Headers.add(AsciiString.of(":path"), AsciiString.of("mypath"));
ejona86 marked this conversation as resolved.
Show resolved Hide resolved
http2Headers.add(AsciiString.of(":authority"), AsciiString.of("myauthority"));
http2Headers.add(AsciiString.of(":method"), AsciiString.of("mymethod"));
http2Headers.add(AsciiString.of(":scheme"), AsciiString.of("myscheme"));

assertThat(http2Headers.get(AsciiString.of(":path"))).isEqualTo(AsciiString.of("mypath"));
assertThat(http2Headers.get(AsciiString.of(":authority")))
.isEqualTo(AsciiString.of("myauthority"));
assertThat(http2Headers.get(AsciiString.of(":method"))).isEqualTo(AsciiString.of("mymethod"));
assertThat(http2Headers.get(AsciiString.of(":scheme"))).isEqualTo(AsciiString.of("myscheme"));

assertThat(http2Headers.getAll(AsciiString.of(":path")))
.containsExactly(AsciiString.of("mypath"));
assertThat(http2Headers.getAll(AsciiString.of(":authority")))
.containsExactly(AsciiString.of("myauthority"));
assertThat(http2Headers.getAll(AsciiString.of(":method")))
.containsExactly(AsciiString.of("mymethod"));
assertThat(http2Headers.getAll(AsciiString.of(":scheme")))
.containsExactly(AsciiString.of("myscheme"));

assertThat(http2Headers.contains(AsciiString.of(":path"))).isTrue();
assertThat(http2Headers.contains(AsciiString.of(":authority"))).isTrue();
assertThat(http2Headers.contains(AsciiString.of(":method"))).isTrue();
assertThat(http2Headers.contains(AsciiString.of(":scheme"))).isTrue();

assertThat(http2Headers.remove(AsciiString.of(":path"))).isTrue();
assertThat(http2Headers.remove(AsciiString.of(":authority"))).isTrue();
assertThat(http2Headers.remove(AsciiString.of(":method"))).isTrue();
assertThat(http2Headers.remove(AsciiString.of(":scheme"))).isTrue();

assertThat(http2Headers.contains(AsciiString.of(":path"))).isFalse();
assertThat(http2Headers.contains(AsciiString.of(":authority"))).isFalse();
assertThat(http2Headers.contains(AsciiString.of(":method"))).isFalse();
assertThat(http2Headers.contains(AsciiString.of(":scheme"))).isFalse();
}

@Test
public void grpcHttp2RequestHeaders_pseudoHeaders_set() {
Http2Headers http2Headers = new GrpcHttp2RequestHeaders(2);
http2Headers.set(AsciiString.of(":path"), AsciiString.of("mypath"));
http2Headers.set(AsciiString.of(":authority"), AsciiString.of("myauthority"));
http2Headers.set(AsciiString.of(":method"), AsciiString.of("mymethod"));
http2Headers.set(AsciiString.of(":scheme"), AsciiString.of("myscheme"));

assertThat(http2Headers.getAll(AsciiString.of(":path")))
.containsExactly(AsciiString.of("mypath"));
assertThat(http2Headers.getAll(AsciiString.of(":authority")))
.containsExactly(AsciiString.of("myauthority"));
assertThat(http2Headers.getAll(AsciiString.of(":method")))
.containsExactly(AsciiString.of("mymethod"));
assertThat(http2Headers.getAll(AsciiString.of(":scheme")))
.containsExactly(AsciiString.of("myscheme"));

http2Headers.set(AsciiString.of(":path"), AsciiString.of("mypath2"));
http2Headers.set(AsciiString.of(":authority"), AsciiString.of("myauthority2"));
http2Headers.set(AsciiString.of(":method"), AsciiString.of("mymethod2"));
http2Headers.set(AsciiString.of(":scheme"), AsciiString.of("myscheme2"));

assertThat(http2Headers.getAll(AsciiString.of(":path")))
.containsExactly(AsciiString.of("mypath2"));
assertThat(http2Headers.getAll(AsciiString.of(":authority")))
.containsExactly(AsciiString.of("myauthority2"));
assertThat(http2Headers.getAll(AsciiString.of(":method")))
.containsExactly(AsciiString.of("mymethod2"));
assertThat(http2Headers.getAll(AsciiString.of(":scheme")))
.containsExactly(AsciiString.of("myscheme2"));
}

@Test
public void grpcHttp2RequestHeaders_pseudoHeaders_addWhenPresent_throws() {
Http2Headers http2Headers = new GrpcHttp2RequestHeaders(2);
http2Headers.add(AsciiString.of(":path"), AsciiString.of("mypath"));
try {
http2Headers.add(AsciiString.of(":path"), AsciiString.of("mypath2"));
fail("Expected exception");
} catch (Exception ex) {
// expected
}
}

@Test
public void grpcHttp2RequestHeaders_pseudoHeaders_addInvalid_throws() {
Http2Headers http2Headers = new GrpcHttp2RequestHeaders(2);
try {
http2Headers.add(AsciiString.of(":status"), AsciiString.of("mystatus"));
fail("Expected exception");
} catch (Exception ex) {
// expected
}
}

@Test
public void dupBinHeadersWithComma() {
Key<byte[]> key = Key.of("bytes-bin", BINARY_BYTE_MARSHALLER);
Expand Down