Allows QUIC connections to remain established even though the peer's

Allows QUIC connections to remain established even though the peer's
port has changed. Should make things a bit better for clients
experiencing NAT rebinding while talking QUIC to internal server.

QUIC connection port migration. Protected behind
FLAGS_quic_allow_port_migration

Merge internal change: 66523001

R=rch@chromium.org

[Ryan Hamilton, 2014-05-12 23:38:44]

lgtm

[wtc, 2014-05-13 16:37:15]

Patch set 1 LGTM. Note that a comment and an error message seem wrong. Please
resolve the issues before committing. Thanks.

https://codereview.chromium.org/279453004/diff/1/net/quic/quic_connection.cc
File net/quic/quic_connection.cc (right):

https://codereview.chromium.org/279453004/diff/1/net/quic/quic_connection.cc#...
net/quic/quic_connection.cc:1213: "IP or port migration is not yet a supported
feature");

I think this error message is wrong. At least, it contradicts the comment on
line 1217 that port migration is supported.

One possible solution is to use different error messages:

  if (!FLAGS_quic_allow_port_migration && peer_port_changed_) {
    SendConnectionCloseWithDetails(
        QUIC_ERROR_MIGRATING_ADDRESS, "Port migration is not allowed");
    return false;
  }
  if (peer_ip_changed_ || self_ip_changed_ || self_port_changed_) {
    SendConnectionCloseWithDetails(
        QUIC_ERROR_MIGRATING_ADDRESS,
        "IP migration is not yet a supported feature");
    return false;
  }

The second error message is still not correct for the self_port_changed_ case
though.

https://codereview.chromium.org/279453004/diff/1/net/quic/quic_connection.h
File net/quic/quic_connection.h (right):

https://codereview.chromium.org/279453004/diff/1/net/quic/quic_connection.h#n...
net/quic/quic_connection.h:757: // If true, and the IP has not changed, then we
can migrate the connection.

IMPORTANT: this comment doesn't match the code in
QuicConnection::ProcessValidatedPacket(). To match the code, this comment should
say:

  We do not support connection migration when the self port changed.

https://codereview.chromium.org/279453004/diff/1/net/quic/quic_flags.cc
File net/quic/quic_flags.cc (right):

https://codereview.chromium.org/279453004/diff/1/net/quic/quic_flags.cc#newco...
net/quic/quic_flags.cc:40: // If true, allow port migration of established QUIC
connections.

Nit: it would be nice to clarify whether we only allow *peer* port migration.

[ramant (doing other things), 2014-05-13 21:22:58]

On 2014/05/13 16:37:15, wtc wrote:
> Patch set 1 LGTM. Note that a comment and an error message seem wrong. Please
> resolve the issues before committing. Thanks.
> 
> https://codereview.chromium.org/279453004/diff/1/net/quic/quic_connection.cc
> File net/quic/quic_connection.cc (right):
> 
>
https://codereview.chromium.org/279453004/diff/1/net/quic/quic_connection.cc#...
> net/quic/quic_connection.cc:1213: "IP or port migration is not yet a supported
> feature");
> 
> I think this error message is wrong. At least, it contradicts the comment on
> line 1217 that port migration is supported.
> 
> One possible solution is to use different error messages:
> 
>   if (!FLAGS_quic_allow_port_migration && peer_port_changed_) {
>     SendConnectionCloseWithDetails(
>         QUIC_ERROR_MIGRATING_ADDRESS, "Port migration is not allowed");
>     return false;
>   }
>   if (peer_ip_changed_ || self_ip_changed_ || self_port_changed_) {
>     SendConnectionCloseWithDetails(
>         QUIC_ERROR_MIGRATING_ADDRESS,
>         "IP migration is not yet a supported feature");
>     return false;
>   }
> 
> The second error message is still not correct for the self_port_changed_ case
> though.
> 
> https://codereview.chromium.org/279453004/diff/1/net/quic/quic_connection.h
> File net/quic/quic_connection.h (right):
> 
>
https://codereview.chromium.org/279453004/diff/1/net/quic/quic_connection.h#n...
> net/quic/quic_connection.h:757: // If true, and the IP has not changed, then
we
> can migrate the connection.
> 
> IMPORTANT: this comment doesn't match the code in
> QuicConnection::ProcessValidatedPacket(). To match the code, this comment
should
> say:
> 
>   We do not support connection migration when the self port changed.
> 
> https://codereview.chromium.org/279453004/diff/1/net/quic/quic_flags.cc
> File net/quic/quic_flags.cc (right):
> 
>
https://codereview.chromium.org/279453004/diff/1/net/quic/quic_flags.cc#newco...
> net/quic/quic_flags.cc:40: // If true, allow port migration of established
QUIC
> connections.
> 
> Nit: it would be nice to clarify whether we only allow *peer* port migration.

Hi Robbie,
  Would like to changes suggested by wtc, if you don't have any objections.

thanks
raman

[Robbie Shade, 2014-05-13 21:31:21]

https://codereview.chromium.org/279453004/diff/1/net/quic/quic_connection.cc
File net/quic/quic_connection.cc (right):

https://codereview.chromium.org/279453004/diff/1/net/quic/quic_connection.cc#...
net/quic/quic_connection.cc:1213: "IP or port migration is not yet a supported
feature");
How about:

"Neither IP address migration, nor server port migration are supported."

https://codereview.chromium.org/279453004/diff/1/net/quic/quic_connection.h
File net/quic/quic_connection.h (right):

https://codereview.chromium.org/279453004/diff/1/net/quic/quic_connection.h#n...
net/quic/quic_connection.h:757: // If true, and the IP has not changed, then we
can migrate the connection.
On 2014/05/13 16:37:15, wtc wrote:
> 
> IMPORTANT: this comment doesn't match the code in
> QuicConnection::ProcessValidatedPacket(). To match the code, this comment
should
> say:
> 
>   We do not support connection migration when the self port changed.


Correct - this comment is wrong, make the change wtc suggests.

https://codereview.chromium.org/279453004/diff/1/net/quic/quic_flags.cc
File net/quic/quic_flags.cc (right):

https://codereview.chromium.org/279453004/diff/1/net/quic/quic_flags.cc#newco...
net/quic/quic_flags.cc:40: // If true, allow port migration of established QUIC
connections.
On 2014/05/13 16:37:15, wtc wrote:
> 
> Nit: it would be nice to clarify whether we only allow *peer* port migration.

Yes, this should be changed to:

"If true, allow peer port migration of established QUIC connections."

[wtc, 2014-05-13 23:20:41]

Robbie: thank you for the reply.

https://codereview.chromium.org/279453004/diff/1/net/quic/quic_connection.cc
File net/quic/quic_connection.cc (right):

https://codereview.chromium.org/279453004/diff/1/net/quic/quic_connection.cc#...
net/quic/quic_connection.cc:1213: "IP or port migration is not yet a supported
feature");

On 2014/05/13 21:31:21, Robbie Shade wrote:
> How about:
> 
> "Neither IP address migration, nor server port migration are supported."

This sounds good. Did you mean "server port" or "self port"?

Also, do you think we should use a different error message for the
!FLAGS_quic_allow_port_migration && peer_port_changed_) case?

[Robbie Shade, 2014-05-13 23:30:11]

https://codereview.chromium.org/279453004/diff/1/net/quic/quic_connection.cc
File net/quic/quic_connection.cc (right):

https://codereview.chromium.org/279453004/diff/1/net/quic/quic_connection.cc#...
net/quic/quic_connection.cc:1213: "IP or port migration is not yet a supported
feature");
> > "Neither IP address migration, nor server port migration are supported."
> 
> This sounds good. Did you mean "server port" or "self port"?

Yep "self port" is correct, although in practice "server port" is also right as
the only expected use case for this is a client behind a NAT.

> Also, do you think we should use a different error message for the
> !FLAGS_quic_allow_port_migration && peer_port_changed_) case?

Not worth changing I don't think - the flag will be removed in a few weeks once
it's been verified as safe.

[ramant (doing other things), 2014-05-14 05:30:23]

Made the changes in https://codereview.chromium.org/286693003/

https://codereview.chromium.org/279453004/diff/1/net/quic/quic_connection.cc
File net/quic/quic_connection.cc (right):

https://codereview.chromium.org/279453004/diff/1/net/quic/quic_connection.cc#...
net/quic/quic_connection.cc:1213: "IP or port migration is not yet a supported
feature");
On 2014/05/13 23:30:11, Robbie Shade wrote:
> > > "Neither IP address migration, nor server port migration are supported."
> > 
> > This sounds good. Did you mean "server port" or "self port"?
> 
> Yep "self port" is correct, although in practice "server port" is also right
as
> the only expected use case for this is a client behind a NAT.

Done.


> 
> > Also, do you think we should use a different error message for the
> > !FLAGS_quic_allow_port_migration && peer_port_changed_) case?
> 
> Not worth changing I don't think - the flag will be removed in a few weeks
once
> it's been verified as safe.

Acknowledged.

https://codereview.chromium.org/279453004/diff/1/net/quic/quic_connection.cc#...
net/quic/quic_connection.cc:1213: "IP or port migration is not yet a supported
feature");
On 2014/05/13 21:31:21, Robbie Shade wrote:
> How about:
> 
> "Neither IP address migration, nor server port migration are supported."

Made this change in
https://codereview.chromium.org/286693003/diff/1/net/quic/quic_connection.cc

Done.

https://codereview.chromium.org/279453004/diff/1/net/quic/quic_connection.h
File net/quic/quic_connection.h (right):

https://codereview.chromium.org/279453004/diff/1/net/quic/quic_connection.h#n...
net/quic/quic_connection.h:757: // If true, and the IP has not changed, then we
can migrate the connection.
On 2014/05/13 21:31:21, Robbie Shade wrote:
> On 2014/05/13 16:37:15, wtc wrote:
> > 
> > IMPORTANT: this comment doesn't match the code in
> > QuicConnection::ProcessValidatedPacket(). To match the code, this comment
> should
> > say:
> > 
> >   We do not support connection migration when the self port changed.
> 
> 
> Correct - this comment is wrong, make the change wtc suggests.

Made this change in
https://codereview.chromium.org/286693003/diff/1/net/quic/quic_connection.h

Done.

https://codereview.chromium.org/279453004/diff/1/net/quic/quic_flags.cc
File net/quic/quic_flags.cc (right):

https://codereview.chromium.org/279453004/diff/1/net/quic/quic_flags.cc#newco...
net/quic/quic_flags.cc:40: // If true, allow port migration of established QUIC
connections.
On 2014/05/13 21:31:21, Robbie Shade wrote:
> On 2014/05/13 16:37:15, wtc wrote:
> > 
> > Nit: it would be nice to clarify whether we only allow *peer* port
migration.
> 
> Yes, this should be changed to:
> 
> "If true, allow peer port migration of established QUIC connections."

Made this change in
https://codereview.chromium.org/286693003/diff/1/net/quic/quic_flags.cc

Done.