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

http-proxy: attach headers to connection exception #8824

Merged
merged 1 commit into from Feb 2, 2019

Conversation

@carl-mastrangelo
Copy link
Member

carl-mastrangelo commented Jan 31, 2019

Motivation:
When a proxy fails to connect, it includes useful error detail in
the headers.

Modification:

  • Add an HTTP Specific ProxyConnectException
  • Attach headers (if any) in the event of a non-200 response

Result:
Able to surface more useful error info to applications

@carl-mastrangelo carl-mastrangelo requested a review from normanmaurer Jan 31, 2019
@netty-bot

This comment has been minimized.

Copy link

netty-bot commented Jan 31, 2019

Can one of the admins verify this patch?

@normanmaurer normanmaurer requested review from ejona86 and slandelle Jan 31, 2019
public static class HttpProxyConnectException extends ProxyConnectException {
private static final long serialVersionUID = -8824334609292146066L;

private HttpHeaders headers;

This comment has been minimized.

Copy link
@normanmaurer

This comment has been minimized.

Copy link
@carl-mastrangelo

carl-mastrangelo Jan 31, 2019

Author Member

Done.

this.headers = headers;
}

public HttpHeaders headers() {

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Jan 31, 2019

Member

add java doc to indicate that this may return null

This comment has been minimized.

Copy link
@carl-mastrangelo

carl-mastrangelo Jan 31, 2019

Author Member

Done. Not sure what the policy is on @Nullable, but might be worth adding.

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Feb 1, 2019

Member

@carl-mastrangelo we should definitely start to use these annotations for netty 5 .

/**
* Specific case of a connection failure, which may include headers from the proxy.
*/
public static class HttpProxyConnectException extends ProxyConnectException {

This comment has been minimized.

Copy link
@normanmaurer

This comment has been minimized.

Copy link
@carl-mastrangelo

carl-mastrangelo Jan 31, 2019

Author Member

Done. I recall netty users like overriding classes, and ProxyConnectException is non final, for reference.

}
}).connect(new InetSocketAddress("localhost", 1234)).sync().channel().close().sync();
sf.channel().close().sync();
group.shutdownGracefully();

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Jan 31, 2019

Member

shutdown in a finally block ?

This comment has been minimized.

Copy link
@carl-mastrangelo

carl-mastrangelo Jan 31, 2019

Author Member

Done.

@Override
protected void initChannel(Channel ch) {
ch.pipeline().addFirst(null, new HttpProxyHandler(addr));
ch.pipeline().addLast(new ChannelHandlerAdapter() {

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Jan 31, 2019

Member

use a ChannelInboundHandlerAdapter as exceptionCaught(....) is deprecated in ChannelHandler

This comment has been minimized.

Copy link
@carl-mastrangelo

carl-mastrangelo Jan 31, 2019

Author Member

Done.


@Override
protected void initChannel(Channel ch) {
ch.pipeline().addFirst(null, new HttpResponseEncoder());

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Jan 31, 2019

Member

remove null ?

This comment has been minimized.

Copy link
@carl-mastrangelo

carl-mastrangelo Jan 31, 2019

Author Member

Done. btw, I do this because I avoid varargs methods due to allocations.

HttpVersion.HTTP_1_1,
HttpResponseStatus.BAD_GATEWAY);
response.headers().add("name", "value");
response.headers().add(HttpHeaderNames.CONTENT_LENGTH, "0");

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Jan 31, 2019

Member

move the response creation close to the point where its written ?

This comment has been minimized.

Copy link
@carl-mastrangelo

carl-mastrangelo Jan 31, 2019

Author Member

Done.

public void testExceptionDuringConnect() throws Exception {
EventLoopGroup group = new DefaultEventLoopGroup(1);
final LocalAddress addr = new LocalAddress("a");
final HttpHeaders headers = new DefaultHttpHeaders();

This comment has been minimized.

Copy link
@normanmaurer

This comment has been minimized.

Copy link
@carl-mastrangelo

carl-mastrangelo Jan 31, 2019

Author Member

Done.

}
});
}
}).connect(new InetSocketAddress("localhost", 1234)).sync().channel().close().sync();

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Jan 31, 2019

Member

use a LocalAddress ?

This comment has been minimized.

Copy link
@carl-mastrangelo

carl-mastrangelo Jan 31, 2019

Author Member

Can't, the ProxyHandler complains that it isn't an inet socket address (this bit me too, fyi).

ch.pipeline().addFirst(null, new HttpResponseEncoder());
ch.writeAndFlush(response);
}
}).bind(addr);

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Jan 31, 2019

Member

wait until bind is complete ?

Copy link
Member Author

carl-mastrangelo left a comment

PTAL

/**
* Specific case of a connection failure, which may include headers from the proxy.
*/
public static class HttpProxyConnectException extends ProxyConnectException {

This comment has been minimized.

Copy link
@carl-mastrangelo

carl-mastrangelo Jan 31, 2019

Author Member

Done. I recall netty users like overriding classes, and ProxyConnectException is non final, for reference.

public static class HttpProxyConnectException extends ProxyConnectException {
private static final long serialVersionUID = -8824334609292146066L;

private HttpHeaders headers;

This comment has been minimized.

Copy link
@carl-mastrangelo

carl-mastrangelo Jan 31, 2019

Author Member

Done.

this.headers = headers;
}

public HttpHeaders headers() {

This comment has been minimized.

Copy link
@carl-mastrangelo

carl-mastrangelo Jan 31, 2019

Author Member

Done. Not sure what the policy is on @Nullable, but might be worth adding.

public void testExceptionDuringConnect() throws Exception {
EventLoopGroup group = new DefaultEventLoopGroup(1);
final LocalAddress addr = new LocalAddress("a");
final HttpHeaders headers = new DefaultHttpHeaders();

This comment has been minimized.

Copy link
@carl-mastrangelo

carl-mastrangelo Jan 31, 2019

Author Member

Done.

HttpVersion.HTTP_1_1,
HttpResponseStatus.BAD_GATEWAY);
response.headers().add("name", "value");
response.headers().add(HttpHeaderNames.CONTENT_LENGTH, "0");

This comment has been minimized.

Copy link
@carl-mastrangelo

carl-mastrangelo Jan 31, 2019

Author Member

Done.


@Override
protected void initChannel(Channel ch) {
ch.pipeline().addFirst(null, new HttpResponseEncoder());

This comment has been minimized.

Copy link
@carl-mastrangelo

carl-mastrangelo Jan 31, 2019

Author Member

Done. btw, I do this because I avoid varargs methods due to allocations.

@Override
protected void initChannel(Channel ch) {
ch.pipeline().addFirst(null, new HttpProxyHandler(addr));
ch.pipeline().addLast(new ChannelHandlerAdapter() {

This comment has been minimized.

Copy link
@carl-mastrangelo

carl-mastrangelo Jan 31, 2019

Author Member

Done.

}
});
}
}).connect(new InetSocketAddress("localhost", 1234)).sync().channel().close().sync();

This comment has been minimized.

Copy link
@carl-mastrangelo

carl-mastrangelo Jan 31, 2019

Author Member

Can't, the ProxyHandler complains that it isn't an inet socket address (this bit me too, fyi).

}
}).connect(new InetSocketAddress("localhost", 1234)).sync().channel().close().sync();
sf.channel().close().sync();
group.shutdownGracefully();

This comment has been minimized.

Copy link
@carl-mastrangelo

carl-mastrangelo Jan 31, 2019

Author Member

Done.

@ejona86
ejona86 approved these changes Feb 1, 2019
Copy link
Member

ejona86 left a comment

Make inboundHeaders a local variable, then LGTM

Copy link
Member

normanmaurer left a comment

almost there :)

}
}).bind(addr);
serverChannel = sf.channel();
sf.sync();

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Feb 1, 2019

Member

nit: you can merge the two lines.

serverChannel = sf.sync().channel();

This comment has been minimized.

Copy link
@carl-mastrangelo

carl-mastrangelo Feb 1, 2019

Author Member

Done.

new ChannelInitializer<Channel>() {
@Override
protected void initChannel(Channel ch) {
ch.pipeline().addFirst(null, new HttpProxyHandler(addr));

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Feb 1, 2019

Member

nit: remove null to be consistent

This comment has been minimized.

Copy link
@carl-mastrangelo

carl-mastrangelo Feb 1, 2019

Author Member

Whoops, done.

this.headers = headers;
}

public HttpHeaders headers() {

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Feb 1, 2019

Member

@carl-mastrangelo we should definitely start to use these annotations for netty 5 .

@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Feb 1, 2019

@netty-bot test this please

@carl-mastrangelo carl-mastrangelo force-pushed the carl-mastrangelo:proxyhead branch from 1bcf9da to ca113d3 Feb 1, 2019
Copy link
Member Author

carl-mastrangelo left a comment

Comments addressed and rebased.

Let me know what you want me to do for the master branch.

}
}).bind(addr);
serverChannel = sf.channel();
sf.sync();

This comment has been minimized.

Copy link
@carl-mastrangelo

carl-mastrangelo Feb 1, 2019

Author Member

Done.

new ChannelInitializer<Channel>() {
@Override
protected void initChannel(Channel ch) {
ch.pipeline().addFirst(null, new HttpProxyHandler(addr));

This comment has been minimized.

Copy link
@carl-mastrangelo

carl-mastrangelo Feb 1, 2019

Author Member

Whoops, done.

import org.junit.Test;

import java.net.InetAddress;
import java.net.InetSocketAddress;

import static org.junit.Assert.*;

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Feb 1, 2019

Member

sorry missed this before but now that you import .* why you need to also import the methods directly ?

This comment has been minimized.

Copy link
@carl-mastrangelo

carl-mastrangelo Feb 1, 2019

Author Member

Bad IDE config. Google's styleguide forbids wildcard import, so IntelliJ's auto fix action imported this once, and imported * once. Fixed.

@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Feb 1, 2019

@netty-bot test this please

Motivation:
When a proxy fails to connect, it includes useful error detail in
the headers.

Modification:
- Add an HTTP Specific ProxyConnectException
- Attach headers (if any) in the event of a non-200 response

Result:
Able to surface more useful error info to applications
@carl-mastrangelo carl-mastrangelo force-pushed the carl-mastrangelo:proxyhead branch from a995518 to 84ecc3d Feb 1, 2019
@normanmaurer normanmaurer merged commit 95bc819 into netty:4.1 Feb 2, 2019
@normanmaurer normanmaurer added this to the 4.1.34.Final milestone Feb 2, 2019
normanmaurer added a commit that referenced this pull request Feb 2, 2019
Motivation:
When a proxy fails to connect, it includes useful error detail in
the headers.

Modification:
- Add an HTTP Specific ProxyConnectException
- Attach headers (if any) in the event of a non-200 response

Result:
Able to surface more useful error info to applications
@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Feb 2, 2019

@carl-mastrangelo thanks a lot... I also cherry-picked into master

@carl-mastrangelo carl-mastrangelo deleted the carl-mastrangelo:proxyhead branch Feb 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.