This looks wrong... vnet_set_input_acl_intfc(...) at line 93:
/* Return ok on ADD operation if feature is already enabled */
if (is_add &&
am->classify_table_index_by_sw_if_index[ti][sw_if_index] != ~0)
return 0;
It’s been that way for a very long time.
Thanks… Dave
From: vpp-dev-boun...@lists.fd.io [mailto:vpp-dev-boun...@lists.fd.io] On
Behalf Of Jon Loeliger
Sent: Saturday, December 9, 2017 11:23 AM
To: Andrew 👽 Yourtchenko <ayour...@gmail.com>
Cc: vpp-dev <vpp-dev@lists.fd.io>
Subject: Re: [vpp-dev] MACIP ACL replace causes ip4_table_index change?
On Sat, Dec 9, 2017 at 8:16 AM, Andrew 👽 Yourtchenko
<ayour...@gmail.com<mailto:ayour...@gmail.com>> wrote:
Jon,
Hi Andrew,
Thanks for taking a look at this issue!
on api trace: does the below work ? (even though the current scenario
is trivially reproducible, the api traces are very useful for tougher
cases, and save a lot of typing while storytelling).
DBGvpp# api trace on
..... do the things ....
DBGvpp# api trace save macip-trace
API trace saved to /tmp/macip-trace
DBGvpp# api trace custom-dump /tmp/macip-trace
SCRIPT: macip_acl_add_replace -1 count 1 count 1 \
ipv4 permit \
src mac 00:00:00:00:00:00 mask 00:00:00:00:00:00 \
src ip 0.0.0.0/0<http://0.0.0.0/0>, \
SCRIPT: macip_acl_interface_add_del sw_if_index 0 acl_index 0 add
SCRIPT: macip_acl_add_replace 0 count 1 count 1 \
ipv4 permit \
src mac 00:00:00:00:00:00 mask 00:00:00:00:00:00 \
src ip 0.0.0.0/0<http://0.0.0.0/0>, \
I think that is the right sequence.
Now, to the issue itself: it's exactly as I described, but with a twist:
vnet_set_input_acl_intfc(), which is used under the hood to assign the
inacl on the interfaces, is quite picky - if there is an existing
inacl applied, it just quietly does nothing. (@DaveB - this kinda
feels strange, I am not sure what the logic is behind doing that.)
Anyway, rather than debating on why it behaves this way, and,
especially since we actually are deleting the tables in question, it's
better to unapply the inacls first, and then reapply them after the
tables have been recreated.
This solves half the problem for me. It looks like I can properly
turn around and remove this ACL from the interface now!
But I still have doubts; or at least I don't understand why the
three table indices are 3 after initial creation, and 0 after they
are replaced.
The result is in https://gerrit.fd.io/r/#/c/9772/ - you can verify
that it addresses the issue.
I've left a comment on the code there.
Despite what Gerrit thinks, this code does compile and run for me!
So maybe just a "rebuild" request there will allow it to verify?
Now, going on to "how exactly did this slip through" - seems the macip
tests are quite a bit too lenient than they should be. I'll need to
address that as well, though probably I will split the dot1q/dot1ad
test cases out, and in the process refactor things a bit... so in the
interests of your time, maybe 9772 can go with just an actual code
fix.
I've not read through the test as they stand today.
I'd like to understand the "3 vs. 0" issue before I am happy to
Code Review +1 this patch.
I've dumped the entire debugging process into a log, which turned out
be fairly long, so to avoid sending the walls of text to the list,
I've dumped it elsewhere:
http://stdio.be/blog/2017-12-09-Debugging-VPP-MACIP-ACLs/
And, excellent! I read through that in quite some detail. And I understand
the "3 vs 0" issue I was seeing now!
The two pieces I missed were: The "show inacl type l2" to see where
the chain was starting, and then noticing that the chain had indeed been
reversed once the starting point was known.
So thanks for that excellent debug layout and explanation.
And thanks for including the intermediate step of showing why simply
updating the interface at the end wasn't sufficient. I had done that,
but hadn't yet gotten into the next function to see it was being ignored.
Thanks again for catching this.
Thanks for fixing this!
--a
So, with that, I've left a comment and a Review -1 on the patch.
And the patch didn't Verify. So where do we go from here?
I'm good with the patch, and we need to rebuild it. So do we just
re-build the same patch or re-submit it as a new patch? I will
either update or Review a-new to +1 it!
Thanks,
jdl
_______________________________________________
vpp-dev mailing list
vpp-dev@lists.fd.io
https://lists.fd.io/mailman/listinfo/vpp-dev
