On Sat, Dec 9, 2017 at 8:16 AM, Andrew 👽 Yourtchenko <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, \ > > 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, \ > 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
