On Fri, Dec 5, 2025 at 2:07 PM Erlon Rodrigues Cruz <[email protected]> wrote:
> Hey Ales, > > I have a few questions regarding your proposal. I have started an > implementation, but I couldn't get it to work, > so I'm not sending the patch here just yet. > Hi Erlon, thank you very much for working on this! > >> I was thinking about this and I still don't like the fact that we >> add a custom parser into northd while we have a functional and very >> much tested parser in ovn-controller. I would propose the following >> solution. When the acl translation is set to true we could add >> a special external_id to SB logical_flow. This would serve as an >> indication for ovn-controller. Now to do the replacement we would >> need a second symtab that would basically do the correct replacement >> e.g. >> > > For this part specifically, I'm trying to add a marker into the flow[1], > but the external_ids fields > seems to be ignored[2], so I wasn't able to get the flows to use my custom > translation table. > That is right, fortunately we have the "tags" which we could use, here we need to just ensure that the tags change within the flow is correctly reflected by ovn-controller, to me it seems like that should be the case but we will need to test that out. > >> expr_symtab_add_predicate(symtab, "tcp", "ct_proto == 6"); >> expr_symtab_add_field(symtab, "tcp.src", MFF_CT_TP_DST, "tcp", false); >> >> for all supported protocols (we should support replacement of all of >> them). And by simply using the second symtab in >> "convert_match_to_expr()", when appropriate, we would have the >> replacement done for "free" by the current expression parser without >> any significant change. >> > > Also, can you give a quick feedback in this preliminary version[3] just to > make sure I'm > not deviating too much from the desirable solution? > > [1] > https://github.com/sombrafam/ovn/commit/962b2033803363fb21c8268a5ac594871f9a6fa2#diff-c2f0b8ca455ebae86af4cae6350ed2cc922f197cbb659ec6fbeddafb3b928f3eR1184 > [2] > https://github.com/ovn-org/ovn/blob/962b2033803363fb21c8268a5ac594871f9a6fa2/controller/ovn-controller.c#L6766 > [3] > https://github.com/ovn-org/ovn/commit/962b2033803363fb21c8268a5ac594871f9a6fa2 > So it looks fine overall, but I have a few points: 1) flow_desc seems strange, we will probably need a new field in struct ovn_lflow that will be populated by lflow_table_add_lflow, similar to io_port, and corresponding macro. 2) I don't think we need to do the strstr in the match string, if the translation is enabled we should populate that for all stateful ACLs, only the ones with UDP/TCP will be replaced in the controller. 3) There is no need for the expr_symtab_clone() function, you could just do ovn_init_symtab(&acl_ct_symtab), remove the symbols and re-add them. Hopefully that helps. Regards, Ales _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
