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

okhttp: fix header scheme does not match transport type. (backport of #6260) #6264

Merged
merged 2 commits into from Oct 10, 2019
Merged

okhttp: fix header scheme does not match transport type. (backport of #6260) #6264

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f2704d1
okhttp: fix header scheme does not match transport type. (#6260)
ran-su Oct 10, 2019
2b66dcb
remove the extra argument.
ran-su Oct 10, 2019
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
16 changes: 13 additions & 3 deletions okhttp/src/main/java/io/grpc/okhttp/Headers.java
View file
Open in desktop
Expand Up @@ -34,7 +34,8 @@
*/
class Headers {

public static final Header SCHEME_HEADER = new Header(Header.TARGET_SCHEME, "https");
public static final Header HTTPS_SCHEME_HEADER = new Header(Header.TARGET_SCHEME, "https");
public static final Header HTTP_SCHEME_HEADER = new Header(Header.TARGET_SCHEME, "http");
public static final Header METHOD_HEADER = new Header(Header.TARGET_METHOD, GrpcUtil.HTTP_METHOD);
public static final Header METHOD_GET_HEADER = new Header(Header.TARGET_METHOD, "GET");
public static final Header CONTENT_TYPE_HEADER =
Expand All @@ -47,7 +48,12 @@ class Headers {
* application thread context.
*/
public static List<Header> createRequestHeaders(
Metadata headers, String defaultPath, String authority, String userAgent, boolean useGet) {
Metadata headers,
String defaultPath,
String authority,
String userAgent,
boolean useGet,
boolean usePlaintext) {
Preconditions.checkNotNull(headers, "headers");
Preconditions.checkNotNull(defaultPath, "defaultPath");
Preconditions.checkNotNull(authority, "authority");
Expand All @@ -61,7 +67,11 @@ public static List<Header> createRequestHeaders(
List<Header> okhttpHeaders = new ArrayList<>(7 + InternalMetadata.headerCount(headers));

// Set GRPC-specific headers.
okhttpHeaders.add(SCHEME_HEADER);
if (usePlaintext) {
okhttpHeaders.add(HTTP_SCHEME_HEADER);
} else {
okhttpHeaders.add(HTTPS_SCHEME_HEADER);
}
if (useGet) {
okhttpHeaders.add(METHOD_GET_HEADER);
} else {
Expand Down
9 changes: 8 additions & 1 deletion okhttp/src/main/java/io/grpc/okhttp/OkHttpClientStream.java
View file
Open in desktop
Expand Up @@ -381,7 +381,14 @@ private void sendBuffer(Buffer buffer, boolean endOfStream, boolean flush) {

@GuardedBy("lock")
private void streamReady(Metadata metadata, String path) {
requestHeaders = Headers.createRequestHeaders(metadata, path, authority, userAgent, useGet);
requestHeaders =
Headers.createRequestHeaders(
metadata,
path,
authority,
userAgent,
useGet,
transport.isUsingPlaintext());
transport.streamReadyToStart(OkHttpClientStream.this);
}
}
Expand Down
5 changes: 5 additions & 0 deletions okhttp/src/main/java/io/grpc/okhttp/OkHttpClientTransport.java
View file
Open in desktop
Expand Up @@ -310,6 +310,11 @@ protected void handleNotInUse() {
initTransportTracer();
}

// sslSocketFactory is set to null when use plaintext.
boolean isUsingPlaintext() {
return sslSocketFactory == null;
}

private void initTransportTracer() {
synchronized (lock) { // to make @GuardedBy linter happy
transportTracer.setFlowControlWindowReader(new TransportTracer.FlowControlReader() {
Expand Down
1 change: 1 addition & 0 deletions okhttp/src/test/java/io/grpc/okhttp/HeadersTest.java
View file
Open in desktop
Expand Up @@ -53,6 +53,7 @@ public void createRequestHeaders_sanitizes() {
path,
authority,
userAgent,
false,
false);

// 7 reserved headers, 1 user header
Expand Down
27 changes: 26 additions & 1 deletion okhttp/src/test/java/io/grpc/okhttp/OkHttpClientStreamTest.java
View file
Open in desktop
Expand Up @@ -25,6 +25,7 @@
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoMoreInteractions;
import static org.mockito.Mockito.when;

import com.google.common.io.BaseEncoding;
import io.grpc.CallOptions;
Expand Down Expand Up @@ -181,7 +182,7 @@ public void start_headerFieldOrder() throws IOException {
verify(mockedFrameWriter)
.synStream(eq(false), eq(false), eq(3), eq(0), headersCaptor.capture());
assertThat(headersCaptor.getValue()).containsExactly(
Headers.SCHEME_HEADER,
Headers.HTTPS_SCHEME_HEADER,
Headers.METHOD_HEADER,
new Header(Header.TARGET_AUTHORITY, "localhost"),
new Header(Header.TARGET_PATH, "/" + methodDescriptor.getFullMethodName()),
Expand All @@ -191,6 +192,30 @@ public void start_headerFieldOrder() throws IOException {
.inOrder();
}

@Test
public void start_headerPlaintext() throws IOException {
Metadata metaData = new Metadata();
metaData.put(GrpcUtil.USER_AGENT_KEY, "misbehaving-application");
when(transport.isUsingPlaintext()).thenReturn(true);
stream = new OkHttpClientStream(methodDescriptor, metaData, frameWriter, transport,
flowController, lock, MAX_MESSAGE_SIZE, INITIAL_WINDOW_SIZE, "localhost",
"good-application", StatsTraceContext.NOOP, transportTracer, CallOptions.DEFAULT);
stream.start(new BaseClientStreamListener());
stream.transportState().start(3);

verify(mockedFrameWriter)
.synStream(eq(false), eq(false), eq(3), eq(0), headersCaptor.capture());
assertThat(headersCaptor.getValue()).containsExactly(
Headers.HTTP_SCHEME_HEADER,
Headers.METHOD_HEADER,
new Header(Header.TARGET_AUTHORITY, "localhost"),
new Header(Header.TARGET_PATH, "/" + methodDescriptor.getFullMethodName()),
new Header(GrpcUtil.USER_AGENT_KEY.name(), "good-application"),
Headers.CONTENT_TYPE_HEADER,
Headers.TE_HEADER)
.inOrder();
}

@Test
public void getUnaryRequest() throws IOException {
MethodDescriptor<?, ?> getMethod = MethodDescriptor.<Void, Void>newBuilder()
Expand Down
6 changes: 3 additions & 3 deletions okhttp/src/test/java/io/grpc/okhttp/OkHttpClientTransportTest.java
View file
Open in desktop
Expand Up @@ -22,8 +22,8 @@
import static io.grpc.internal.ClientStreamListener.RpcProgress.REFUSED;
import static io.grpc.internal.GrpcUtil.DEFAULT_MAX_MESSAGE_SIZE;
import static io.grpc.okhttp.Headers.CONTENT_TYPE_HEADER;
import static io.grpc.okhttp.Headers.HTTP_SCHEME_HEADER;
import static io.grpc.okhttp.Headers.METHOD_HEADER;
import static io.grpc.okhttp.Headers.SCHEME_HEADER;
import static io.grpc.okhttp.Headers.TE_HEADER;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
Expand Down Expand Up @@ -649,7 +649,7 @@ public void addDefaultUserAgent() throws Exception {
stream.start(listener);
Header userAgentHeader = new Header(GrpcUtil.USER_AGENT_KEY.name(),
GrpcUtil.getGrpcUserAgent("okhttp", null));
List<Header> expectedHeaders = Arrays.asList(SCHEME_HEADER, METHOD_HEADER,
List<Header> expectedHeaders = Arrays.asList(HTTP_SCHEME_HEADER, METHOD_HEADER,
new Header(Header.TARGET_AUTHORITY, "notarealauthority:80"),
new Header(Header.TARGET_PATH, "/" + method.getFullMethodName()),
userAgentHeader, CONTENT_TYPE_HEADER, TE_HEADER);
Expand All @@ -666,7 +666,7 @@ public void overrideDefaultUserAgent() throws Exception {
OkHttpClientStream stream =
clientTransport.newStream(method, new Metadata(), CallOptions.DEFAULT);
stream.start(listener);
List<Header> expectedHeaders = Arrays.asList(SCHEME_HEADER, METHOD_HEADER,
List<Header> expectedHeaders = Arrays.asList(HTTP_SCHEME_HEADER, METHOD_HEADER,
new Header(Header.TARGET_AUTHORITY, "notarealauthority:80"),
new Header(Header.TARGET_PATH, "/" + method.getFullMethodName()),
new Header(GrpcUtil.USER_AGENT_KEY.name(),
Expand Down