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

Drop packets from unknown interfaces in classifier table #199

Merged
merged 2 commits into from
Dec 18, 2019

Conversation

wenyingd
Copy link
Contributor

@wenyingd wenyingd commented Dec 6, 2019

  1. Change the priority of tableMiss from 80 to 0. Otherwise there would be
    another flow entry with action "Normal" added by OVS automatically after
    the bridge is created.
  2. Change the default action of classifier table to drop packets.

Fixes #195

Signed-off-by: wenyingd wenyingd@vmware.com

@antrea-bot
Copy link
Collaborator

Thanks for your PR.
Unit tests and code linters are run automatically every time the PR is updated.
E2e tests can only be triggered by a member of the vmware-tanzu organization. Regular contributors to the project should join the org.

The following commands are available:

  • /test-e2e: to trigger e2e tests. This command can only be run by members of the vmware-tanzu organization
  • /skip-e2e: to skip e2e tests. This command can only be run by members of the vmware-tanzu organization

@wenyingd
Copy link
Contributor Author

wenyingd commented Dec 6, 2019

/test-e2e

@wenyingd wenyingd changed the title Drop packets from unknown interfaces in classifier table [WIP]Drop packets from unknown interfaces in classifier table Dec 6, 2019
jianjuns
jianjuns previously approved these changes Dec 7, 2019
@jianjuns
Copy link
Contributor

jianjuns commented Dec 9, 2019

@wenyingd: if we do not drop the packets, what will be the problem? I was thinking in some testing scenarios, could we create an extra interface on the bridge.

@wenyingd
Copy link
Contributor Author

wenyingd commented Dec 10, 2019

@jianjuns If we don't drop the packets, packets sent from non-Pod/non-gw0/non-tunnel could be forwarded normally. Then it is possible that the unleagel packets can be sent to pods.

We current found an issue with openflow binding, if Openflow entries are not realized before GARP(it is possible because OF binding is async, even if the flow entries is sent out from the UDS channel, it might be not realized at that time), the GARP packets are forwarded by the Normal flow. But after change the default action to "drop", the GARP packets might be dropped, and it affects several "Conformance" test. But it should be another issue about race between different goroutines. More details about the isse please refer to #195

" I was thinking in some testing scenarios, could we create an extra interface on the bridge." Do you mean these interfaces have no specific Openflow rules installed, and we want to leverage the Normal flow to forward the packets? Shall we install specific flow entries for these interfaces in such test cases?

@jianjuns
Copy link
Contributor

But non-Pod/tunnel/gw ports must be added manually for some purpose if they exist, so I feel no need to handle them specially.
But my question is more that if we do not add the default drop flow, will we hit some issues. For example, before the classification flow is installed for a normal Pod port, if we do not drop the packets will the packets be forwarded wrongly?

@wenyingd
Copy link
Contributor Author

If we don't drop the packets, they should be forwarded normally, it is following OVS default actions after a bridge is created.

Current code has another flow entry("priority=80,ip,actions=resubmit(,10)") which will enforce IP packets into table10(Spoofing guard table). Table10 will check inport/IP/Mac bindings, and drop all unlegal packets. If the packet is from unkown source, it should be dropped here. I will remove this flow in this PR, no matter the default actions in table0 is to drop or normal.

@jianjuns
Copy link
Contributor

Ok. I feel still better to drop the packets from unknown sources at least for now, to drop packets from a Pod interface that has no flow (inc. networkpolicy flows) installed. We could consider another approach later if we have cases to add test ports to the bridge.

@wenyingd
Copy link
Contributor Author

/test-e2e

@wenyingd wenyingd changed the title [WIP]Drop packets from unknown interfaces in classifier table Drop packets from unknown interfaces in classifier table Dec 13, 2019
@wenyingd
Copy link
Contributor Author

Update according to @tnqn comment #195 (comment)

antoninbas
antoninbas previously approved these changes Dec 13, 2019
Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

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

This LGTM, even though in the long term I'd rather rely on notifications from the OF switch if possible (#195 (comment))

@wenyingd
Copy link
Contributor Author

/test-e2e

@wenyingd
Copy link
Contributor Author

Rebase to master to enforce golangci-lint check.

antoninbas
antoninbas previously approved these changes Dec 17, 2019
Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

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

LGTM,

longer term we can implement something more robust (see #195 (comment))

@antoninbas antoninbas requested a review from jianjuns December 17, 2019 19:17
jianjuns
jianjuns previously approved these changes Dec 17, 2019
count := 0
for count < 3 {
select {
case <-time.Tick(50 * time.Millisecond):
Copy link
Contributor

Choose a reason for hiding this comment

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

I know other implementations use Fibonacci sequence (like 50, 50, 100). Good with your current implementation too.

1. Change the priority of tableMiss from 80 to 0. Otherwise there would be
   another flow entry with action "Normal" added by OVS automatically after
   the bridge is created.
2. Change the default action of classifier table to drop packets.

Signed-off-by: wenyingd <wenyingd@vmware.com>
Send 3 GARP packets in another goroutine with 50ms interval.
@wenyingd
Copy link
Contributor Author

/test-e2e

Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

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

Approving again

@wenyingd wenyingd merged commit bf79709 into antrea-io:master Dec 18, 2019
@wenyingd wenyingd deleted the issue195 branch December 18, 2019 03:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Packets from unknown interface are allowed in table 0
5 participants