Skip to content
This repository has been archived by the owner on Jun 20, 2024. It is now read-only.

419 broadcast frametoobigerror #433

Merged
merged 2 commits into from
Mar 6, 2015

Conversation

dpw
Copy link
Contributor

@dpw dpw commented Mar 4, 2015

Addresses #419.

dpw added 2 commits March 5, 2015 15:13
If forwarding of a broadcast frame resulted in a FrameTooBigError, we
would generate an ICMP "fragmentation needed" packet that contravened
RFC1122 on two counts: First, we should not produce a ICMP error
message in response to a broadcast/multicast packet.  Secondly, we
should not produce an IP packet with a broadcast/multicast source IP
address.  (It also gave an ethernet frame with a broadcast source MAC,
which probably contravenes some IEEE spec.)

So when RelayBroadcast gets a FrameTooBigError, it should just drop
the packet.

Futhermore, if we fail to relay a broadcast on one connection, that's
not a good reason to give up entirely - we can proceed to attempt to
relay on the other next hop connections.  Errors other than
FrameTooBigError that propagated upwards were simply logged, so that
logging is moved down into RelayBroadcast.
@dpw dpw force-pushed the 419_broadcast_frametoobigerror branch from 6f0992f to 5b66daf Compare March 5, 2015 15:34
@dpw
Copy link
Contributor Author

dpw commented Mar 5, 2015

Rebased, which apparently causes GH to orphan all the comments against the previous version of the branch. Comments were addressed.

@bboreham
Copy link
Contributor

bboreham commented Mar 5, 2015

LGTM

@rade
Copy link
Member

rade commented Mar 5, 2015

!LGTM

@rade
Copy link
Member

rade commented Mar 5, 2015

!LGTM

or is that DNLGTM?

@rade rade self-assigned this Mar 5, 2015
rade added a commit that referenced this pull request Mar 6, 2015
Do not inject ICMP "fragmentation needed" for broadcast packets.

It's a contravention of RFC1122 and also breaks weave routing due to producing packets with broadcast addresses as the source.

Fixes #419.
@rade rade merged commit 0f7f49d into weaveworks:master Mar 6, 2015
@rade rade removed the in progress label Mar 6, 2015
@rade rade modified the milestone: 0.10.0 Apr 18, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants