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

fix pcn-iptables interface matching issue #252

Merged
merged 1 commit into from
Jan 8, 2020
Merged

fix pcn-iptables interface matching issue #252

merged 1 commit into from
Jan 8, 2020

Conversation

goldenrye
Copy link
Contributor

@goldenrye goldenrye commented Dec 8, 2019

pcn-iptables uses 0 as wild card in port's hash map, which conflicts if
a port was assigned with id 0. Don't assign port id 0 to any port should
be able to fix the issue #233 .

@goldenrye goldenrye requested a review from a team as a code owner December 8, 2019 21:20
src/polycubed/src/cube.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@sebymiano sebymiano left a comment

Choose a reason for hiding this comment

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

@goldenrye I don't think this is the right way to go.
As you can see here the wildcard, if exists, is embedded directly into the data plane; then, it is not even needed to perform a lookup into the map.

This issue you have found is related to the control plane, that should use a different port (e.g., -1) to save the wildcard and then embed the corresponding bit-vector.

What you should do is to change this line with a special port number (e.g., -1) containing the wildcard and then modify this line to check if there is a wildcard with that special port and embed it into the code.
In this way you don't have to change anything internally to polycube, which btw may break the existing applications, and you are allowed to use port 0 into the filtering set of iptables interface.

@goldenrye
Copy link
Contributor Author

goldenrye commented Dec 9, 2019

Ok thanks for all the comments and suggestions. I have modified the wild card from 0 to 0xffff and add some static assert to ensure no conflicting with regular port.

BTW I think we should decouple the port id with the value _POLYCUBE_MAX_PORT, there is no reason to assume the port id is less than _POLYCUBE_MAX_PORT - I didn't actually see any code depends on this assumption.

@mauriciovasquezbernal
Copy link
Contributor

BTW I think we should decouple the port id with the value _POLYCUBE_MAX_PORT, there is no reason to assume the port id is less than _POLYCUBE_MAX_PORT - I didn't actually see any code depends on this assumption.

Each cube has an array map that saves the relationship port_id -> endpoint (cube_id | port_id):

// table used to save ports to endpoint relation
BPF_TABLE_SHARED("array", int, u32, forward_chain_, _POLYCUBE_MAX_PORTS);

The result of the lookup in that table is a pair of values, the destination cube and port of the packet, based on that a tail call is done to the next cube:

u32 *next = forward_chain_.lookup(&out_port);

In order to remove the requirement that port_id < _POLYCUBE_MAX_PORT that map would have to be changed to a hash map. I think it is not worth to do because introduces overhead and cannot see what is the benefit of removing that constraint.

Could you please clarify why you think this constraint should be removed?

@goldenrye
Copy link
Contributor Author

In order to remove the requirement that port_id < _POLYCUBE_MAX_PORT that map would have to be changed to a hash map. I think it is not worth to do because introduces overhead and cannot see what is the benefit of removing that constraint.

Could you please clarify why you think this constraint should be removed?

If the forwarding table use port id as the index, the modification is not worthy, I don't have very strong case to support the modification. My argument is based on that port id is only used as value and any user referring to the value shouldn't have any assumption the range of the value.

@mauriciovasquezbernal
Copy link
Contributor

My argument is based on that port id is only used as value and any user referring to the value shouldn't have any assumption the range of the value.

I don't know how pcn-iptables works but in the general cube idea the port_id is an internal reference and should be never exposed to the user. Cubes' ports are identified by the port name.

… rule

will be mistakenly regarded as wild-card rule, so even a packet from a
diffrent port will be allowed passing by mistake. There are 2 changes in
this commit:
- update the iptables submodule pointing to the latest code base so interface
  parameter can be supported
- change the wild-card index from 0 to 0xffff for pcn-iptables service because
  id 0 could be conflicting with a regular port, and recover the original failed
  test case.

Signed-off-by: Jianwen Pi<jianwpi@gmail.com>
@goldenrye
Copy link
Contributor Author

@mauriciovasquezbernal @sebymiano all the tests are passed, please review the code, thanks!

Copy link
Collaborator

@sebymiano sebymiano left a comment

Choose a reason for hiding this comment

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

Looks good to me now.

@frisso frisso merged commit 4f237e7 into master Jan 8, 2020
@frisso frisso deleted the jpi-iptables branch January 8, 2020 07:26
@acloudiator acloudiator linked an issue Feb 18, 2020 that may be closed by this pull request
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.

[BUG] Failing test in pcn-iptables service
4 participants