In the current codebase ct_commit {} action clears ct_state metadata of
the incoming packet. This behaviour introduces an issue if we need to
check the connection tracking state in the subsequent pipeline stages,
e.g. for hairpin traffic:table=14(ls_in_pre_hairpin ), priority=100 , match=(ip && ct.trk), action=(reg0[6] = chk_lb_hairpin(); reg0[12] = chk_lb_hairpin_reply(); next;) Fix the issue introducing ct_commit_continue action used to allow the ct packet to proceed in the pipeline instead of the original one. Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2103086 Signed-off-by: Lorenzo Bianconi <[email protected]> --- Changes since v1: - introduce new nested action ct_commit_continue instead of modifying ct_commit_v2 --- controller/chassis.c | 7 +++++ include/ovn/actions.h | 2 ++ include/ovn/features.h | 1 + lib/actions.c | 61 ++++++++++++++++++++++++++++++++++++++--- northd/northd.c | 40 +++++++++++++++++++++++---- northd/northd.h | 2 ++ northd/ovn-northd.8.xml | 7 +++++ ovn-sb.xml | 15 ++++++++++ tests/ovn-controller.at | 42 ++++++++++++++++++++++++++++ tests/ovn-northd.at | 8 +++--- tests/ovn.at | 4 +++ utilities/ovn-trace.c | 2 ++ 12 files changed, 177 insertions(+), 14 deletions(-) diff --git a/controller/chassis.c b/controller/chassis.c index 685d9b2ae..8dc7ecc07 100644 --- a/controller/chassis.c +++ b/controller/chassis.c @@ -352,6 +352,7 @@ chassis_build_other_config(const struct ovs_chassis_cfg *ovs_cfg, smap_replace(config, OVN_FEATURE_PORT_UP_NOTIF, "true"); smap_replace(config, OVN_FEATURE_CT_NO_MASKED_LABEL, "true"); smap_replace(config, OVN_FEATURE_MAC_BINDING_TIMESTAMP, "true"); + smap_replace(config, OVN_FEATURE_CT_COMMIT_CONTINUE, "true"); } /* @@ -469,6 +470,12 @@ chassis_other_config_changed(const struct ovs_chassis_cfg *ovs_cfg, return true; } + if (!smap_get_bool(&chassis_rec->other_config, + OVN_FEATURE_CT_COMMIT_CONTINUE, + false)) { + return true; + } + return false; } diff --git a/include/ovn/actions.h b/include/ovn/actions.h index a56351081..927818976 100644 --- a/include/ovn/actions.h +++ b/include/ovn/actions.h @@ -66,6 +66,7 @@ struct ovn_extend_table; OVNACT(CT_NEXT, ovnact_ct_next) \ OVNACT(CT_COMMIT_V1, ovnact_ct_commit_v1) \ OVNACT(CT_COMMIT_V2, ovnact_nest) \ + OVNACT(CT_COMMIT_CONTINUE, ovnact_nest) \ OVNACT(CT_DNAT, ovnact_ct_nat) \ OVNACT(CT_SNAT, ovnact_ct_nat) \ OVNACT(CT_DNAT_IN_CZONE, ovnact_ct_nat) \ @@ -321,6 +322,7 @@ struct ovnact_nest { struct ovnact ovnact; struct ovnact *nested; size_t nested_len; + uint8_t ltable; /* Logical table ID of next table. */ }; /* OVNACT_GET_ARP, OVNACT_GET_ND. */ diff --git a/include/ovn/features.h b/include/ovn/features.h index 679f67457..0ad8a27b9 100644 --- a/include/ovn/features.h +++ b/include/ovn/features.h @@ -24,6 +24,7 @@ #define OVN_FEATURE_PORT_UP_NOTIF "port-up-notif" #define OVN_FEATURE_CT_NO_MASKED_LABEL "ct-no-masked-label" #define OVN_FEATURE_MAC_BINDING_TIMESTAMP "mac-binding-timestamp" +#define OVN_FEATURE_CT_COMMIT_CONTINUE "ct-commit-continue" /* OVS datapath supported features. Based on availability OVN might generate * different types of openflows. diff --git a/lib/actions.c b/lib/actions.c index 47ec654e1..807b84127 100644 --- a/lib/actions.c +++ b/lib/actions.c @@ -766,6 +766,13 @@ parse_CT_COMMIT(struct action_context *ctx) if (ctx->lexer->token.type == LEX_T_LCURLY) { parse_nested_action(ctx, OVNACT_CT_COMMIT_V2, "ip", WR_CT_COMMIT); + + if (ctx->lexer->error) { + return; + } + + struct ovnact_nest *on = ctx->ovnacts->header; + on->ltable = 0; } else if (ctx->lexer->token.type == LEX_T_LPAREN) { parse_CT_COMMIT_V1(ctx); } else { @@ -775,6 +782,7 @@ parse_CT_COMMIT(struct action_context *ctx) OVNACT_ALIGN(sizeof *on)); on->nested_len = 0; on->nested = NULL; + on->ltable = 0; } } @@ -871,13 +879,13 @@ format_CT_COMMIT_V2(const struct ovnact_nest *on, struct ds *s) } static void -encode_CT_COMMIT_V2(const struct ovnact_nest *on, - const struct ovnact_encode_params *ep OVS_UNUSED, - struct ofpbuf *ofpacts) +encode_ct_commit_nested(const struct ovnact_nest *on, + const struct ovnact_encode_params *ep, + uint8_t recirc_table, struct ofpbuf *ofpacts) { struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts); ct->flags = NX_CT_F_COMMIT; - ct->recirc_table = NX_CT_RECIRC_NONE; + ct->recirc_table = recirc_table; ct->zone_src.field = ep->is_switch ? mf_from_id(MFF_LOG_CT_ZONE) : mf_from_id(MFF_LOG_DNAT_ZONE); @@ -907,6 +915,49 @@ encode_CT_COMMIT_V2(const struct ovnact_nest *on, ct = ofpacts->header; ofpact_finish(ofpacts, &ct->ofpact); } + +static void +encode_CT_COMMIT_V2(const struct ovnact_nest *on, + const struct ovnact_encode_params *ep, + struct ofpbuf *ofpacts) +{ + encode_ct_commit_nested(on, ep, NX_CT_RECIRC_NONE, ofpacts); +} + +static void +parse_CT_COMMIT_CONTINUE(struct action_context *ctx) +{ + int table = ctx->pp->cur_ltable + 1; + if (table >= ctx->pp->n_tables) { + table = 0; + } + parse_nested_action(ctx, OVNACT_CT_COMMIT_CONTINUE, "ip", + WR_CT_COMMIT); + + struct ovnact_nest *on = ctx->ovnacts->header; + on->ltable = table; +} + +static void +format_CT_COMMIT_CONTINUE(const struct ovnact_nest *on, struct ds *s) +{ + if (on->nested_len) { + format_nested_action(on, "ct_commit_continue", s); + } else { + ds_put_cstr(s, "ct_commit_continue;"); + } +} + +static void +encode_CT_COMMIT_CONTINUE(const struct ovnact_nest *on, + const struct ovnact_encode_params *ep, + struct ofpbuf *ofpacts) +{ + uint8_t recirc_table = first_ptable(ep, ep->pipeline) + on->ltable; + + encode_ct_commit_nested(on, ep, recirc_table, ofpacts); +} + static void parse_ct_nat(struct action_context *ctx, const char *name, @@ -5288,6 +5339,8 @@ parse_action(struct action_context *ctx) parse_DEC_TTL(ctx); } else if (lexer_match_id(ctx->lexer, "ct_next")) { parse_CT_NEXT(ctx); + } else if (lexer_match_id(ctx->lexer, "ct_commit_continue")) { + parse_CT_COMMIT_CONTINUE(ctx); } else if (lexer_match_id(ctx->lexer, "ct_commit")) { parse_CT_COMMIT(ctx); } else if (lexer_match_id(ctx->lexer, "ct_dnat")) { diff --git a/northd/northd.c b/northd/northd.c index 74facce7a..5170e20e2 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -446,6 +446,14 @@ build_chassis_features(const struct northd_input *input_data, chassis_features->mac_binding_timestamp) { chassis_features->mac_binding_timestamp = false; } + + bool ct_commit_continue = + smap_get_bool(&chassis->other_config, + OVN_FEATURE_CT_COMMIT_CONTINUE, + false); + if (!ct_commit_continue && chassis_features->ct_commit_continue) { + chassis_features->ct_commit_continue = false; + } } } @@ -5494,6 +5502,7 @@ ls_get_acl_flags(struct ovn_datapath *od) { od->has_acls = false; od->has_stateful_acl = false; + od->has_apply_after_lb_acls = false; if (od->nbs->n_acls) { od->has_acls = true; @@ -5502,7 +5511,9 @@ ls_get_acl_flags(struct ovn_datapath *od) struct nbrec_acl *acl = od->nbs->acls[i]; if (!strcmp(acl->action, "allow-related")) { od->has_stateful_acl = true; - return; + } + if (smap_get_bool(&acl->options, "apply-after-lb", false)) { + od->has_apply_after_lb_acls = true; } } } @@ -5516,7 +5527,9 @@ ls_get_acl_flags(struct ovn_datapath *od) struct nbrec_acl *acl = ls_pg->nb_pg->acls[i]; if (!strcmp(acl->action, "allow-related")) { od->has_stateful_acl = true; - return; + } + if (smap_get_bool(&acl->options, "apply-after-lb", false)) { + od->has_apply_after_lb_acls = true; } } } @@ -7447,9 +7460,17 @@ build_stateful(struct ovn_datapath *od, * We always set ct_mark.blocked to 0 here as * any packet that makes it this far is part of a connection we * want to allow to continue. */ - ds_put_format(&actions, "ct_commit { %s = 0; " - "ct_label.label = " REG_LABEL "; }; next;", - ct_block_action); + if (features->ct_commit_continue && od->has_apply_after_lb_acls) { + ds_put_format(&actions, + "ct_commit_continue { %s = 0; " + "ct_label.label = " REG_LABEL "; };", + ct_block_action); + } else { + ds_put_format(&actions, + "ct_commit { %s = 0; " + "ct_label.label = " REG_LABEL "; }; next;", + ct_block_action); + } ovn_lflow_add(lflows, od, S_SWITCH_IN_STATEFUL, 100, REGBIT_CONNTRACK_COMMIT" == 1 && " REGBIT_ACL_LABEL" == 1", @@ -7464,7 +7485,13 @@ build_stateful(struct ovn_datapath *od, * any packet that makes it this far is part of a connection we * want to allow to continue. */ ds_clear(&actions); - ds_put_format(&actions, "ct_commit { %s = 0; }; next;", ct_block_action); + if (features->ct_commit_continue && od->has_apply_after_lb_acls) { + ds_put_format(&actions, "ct_commit_continue { %s = 0; };", + ct_block_action); + } else { + ds_put_format(&actions, "ct_commit { %s = 0; }; next;", + ct_block_action); + } ovn_lflow_add(lflows, od, S_SWITCH_IN_STATEFUL, 100, REGBIT_CONNTRACK_COMMIT" == 1 && " REGBIT_ACL_LABEL" == 0", @@ -15875,6 +15902,7 @@ northd_init(struct northd_data *data) data->features = (struct chassis_features) { .ct_no_masked_label = true, .mac_binding_timestamp = true, + .ct_commit_continue = true, }; data->ovn_internal_version_changed = false; } diff --git a/northd/northd.h b/northd/northd.h index 7942c0a34..fee68d1e7 100644 --- a/northd/northd.h +++ b/northd/northd.h @@ -69,6 +69,7 @@ struct northd_input { struct chassis_features { bool ct_no_masked_label; bool mac_binding_timestamp; + bool ct_commit_continue; }; struct northd_data { @@ -211,6 +212,7 @@ struct ovn_datapath { bool has_unknown; bool has_acls; bool has_vtep_lports; + bool has_apply_after_lb_acls; /* IPAM data. */ struct ipam_info ipam_info; diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml index dffbba96d..6a6425dd4 100644 --- a/northd/ovn-northd.8.xml +++ b/northd/ovn-northd.8.xml @@ -1108,6 +1108,13 @@ action based on a hint provided by the previous tables (with a match for <code>reg0[1] == 1 && reg0[13] == 0</code>). </li> + + <li> + If the ACL is configured with <code>apply-after-lb</code> option, + <code>ct_commit_continue</code> action will be used instead of + <code>ct_commit</code> in order to preserve ct_state metadata. + </li> + <li> A priority-0 flow that simply moves traffic to the next table. </li> diff --git a/ovn-sb.xml b/ovn-sb.xml index 4f485b860..6f759b428 100644 --- a/ovn-sb.xml +++ b/ovn-sb.xml @@ -1408,6 +1408,21 @@ </p> </dd> + <dt><code>ct_commit_continue { };</code></dt> + <dt><code>ct_commit_continue { ct_mark=<var>value[/mask]</var>; };</code></dt> + <dt><code>ct_commit_continue { ct_label=<var>value[/mask]</var>; };</code></dt> + <dt><code>ct_commit_continue { ct_mark=<var>value[/mask]</var>; ct_label=<var>value[/mask]</var>; };</code></dt> + + <dd> + <p> + <code>ct_commit_continue</code> action exports the same features + supported by <code>ct_commit</code> but allow the packet committed + to the ct table to continue the processing in the next pipeline + stage. This is useful to maintain ct metadata of the processed + packet. + </p> + </dd> + <dt><code>ct_dnat;</code></dt> <dt><code>ct_dnat(<var>IP</var>);</code></dt> <dd> diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at index 6bc9ba75d..67c74f9cd 100644 --- a/tests/ovn-controller.at +++ b/tests/ovn-controller.at @@ -2499,3 +2499,45 @@ AT_CHECK([GET_LOCAL_TEMPLATE_VARS], [1], []) AT_CLEANUP ]) + +OVN_FOR_EACH_NORTHD([ +AT_SETUP([ovn-controller - ct_commit_continue]) +AT_KEYWORDS([ct_commit_continue]) + +ovn_start + +net_add n1 +sim_add hv1 +ovs-vsctl add-br br-phys +ovn_attach n1 br-phys 192.168.0.1 + +check ovn-nbctl ls-add sw0 \ + -- lsp-add sw0 sw0-p0 \ + -- lsp-set-addresses sw0-p0 "00:00:00:00:00:01 192.168.1.1" + +as hv1 +ovs-vsctl \ + -- add-port br-int vif0 \ + -- set Interface vif0 external_ids:iface-id=sw0-p0 + +check ovn-nbctl pg-add pg0 sw0-p0 +check ovn-nbctl --apply-after-lb acl-add pg0 from-lport 1004 "ip4 && ip4.dst == 192.168.1.2" drop +check ovn-nbctl --apply-after-lb acl-add pg0 from-lport 1002 "ip4 && tcp" allow-related +check ovn-nbctl --apply-after-lb acl-add pg0 from-lport 1003 "ip4 && icmp" allow-related +check ovn-nbctl --apply-after-lb acl-add pg0 from-lport 1001 "ip4" drop + +check ovn-nbctl lb-add lb0 192.168.1.10 192.168.1.2 +check ovn-nbctl ls-lb-add sw0 lb0 + +check ovn-nbctl --wait=hv sync +wait_for_ports_up + +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=23 | grep table=24 | sed -e 's/cookie=0x[[a-z,0-9]]*/cookie=0x0/; s/duration=[[0-9]]*.[[0-9]]*s/duration=<cleared>/' |sort], [0], [dnl + cookie=0x0 duration=<cleared>, table=23, n_packets=0, n_bytes=0, idle_age=0, priority=100,ip,reg0=0x2/0x2002,metadata=0x1 actions=ct(commit,table=24,zone=NXM_NX_REG13[[0..15]],nat(src),exec(load:0->NXM_NX_CT_MARK[[0]])) + cookie=0x0 duration=<cleared>, table=23, n_packets=0, n_bytes=0, idle_age=0, priority=100,ip,reg0=0x2002/0x2002,metadata=0x1 actions=ct(commit,table=24,zone=NXM_NX_REG13[[0..15]],nat(src),exec(load:0->NXM_NX_CT_MARK[[0]],move:NXM_NX_XXREG0[[0..31]]->NXM_NX_CT_LABEL[[96..127]])) + cookie=0x0 duration=<cleared>, table=23, n_packets=0, n_bytes=0, idle_age=0, priority=100,ipv6,reg0=0x2/0x2002,metadata=0x1 actions=ct(commit,table=24,zone=NXM_NX_REG13[[0..15]],nat(src),exec(load:0->NXM_NX_CT_MARK[[0]])) + cookie=0x0 duration=<cleared>, table=23, n_packets=0, n_bytes=0, idle_age=0, priority=100,ipv6,reg0=0x2002/0x2002,metadata=0x1 actions=ct(commit,table=24,zone=NXM_NX_REG13[[0..15]],nat(src),exec(load:0->NXM_NX_CT_MARK[[0]],move:NXM_NX_XXREG0[[0..31]]->NXM_NX_CT_LABEL[[96..127]])) +]) + +AT_CLEANUP +]) diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index 9a76ca340..7eb965ce8 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -6623,8 +6623,8 @@ AT_CHECK([grep -e "ls_in_lb " lsflows | sed 's/table=../table=??/' | sort], [0], AT_CHECK([grep -e "ls_in_stateful" lsflows | sed 's/table=../table=??/' | sort], [0], [dnl table=??(ls_in_stateful ), priority=0 , match=(1), action=(next;) - table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_mark.blocked = 0; }; next;) - table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_mark.blocked = 0; ct_label.label = reg3; }; next;) + table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit_continue { ct_mark.blocked = 0; };) + table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit_continue { ct_mark.blocked = 0; ct_label.label = reg3; };) ]) AS_BOX([Remove and add the ACLs back with a few ACLs with apply-after-lb option]) @@ -6676,8 +6676,8 @@ AT_CHECK([grep -e "ls_in_lb " lsflows | sed 's/table=../table=??/' | sort], [0], AT_CHECK([grep -e "ls_in_stateful" lsflows | sed 's/table=../table=??/' | sort], [0], [dnl table=??(ls_in_stateful ), priority=0 , match=(1), action=(next;) - table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_mark.blocked = 0; }; next;) - table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_mark.blocked = 0; ct_label.label = reg3; }; next;) + table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit_continue { ct_mark.blocked = 0; };) + table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit_continue { ct_mark.blocked = 0; ct_label.label = reg3; };) ]) AT_CLEANUP diff --git a/tests/ovn.at b/tests/ovn.at index f3bd53242..ed4a2f50d 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -1187,6 +1187,10 @@ ct_commit { ct_mark=1; }; formats as ct_commit { ct_mark = 1; }; encodes as ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0x1->ct_mark)) has prereqs ip +ct_commit_continue { ct_mark=1; }; + formats as ct_commit_continue { ct_mark = 1; }; + encodes as ct(commit,table=19,zone=NXM_NX_REG13[0..15],exec(set_field:0x1->ct_mark)) + has prereqs ip ct_commit { ct_mark=1/1; }; formats as ct_commit { ct_mark = 1/1; }; encodes as ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0x1/0x1->ct_mark)) diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c index 79ed5a9af..e1def9eea 100644 --- a/utilities/ovn-trace.c +++ b/utilities/ovn-trace.c @@ -3098,6 +3098,8 @@ trace_actions(const struct ovnact *ovnacts, size_t ovnacts_len, case OVNACT_CT_COMMIT_V2: /* Nothing to do. */ break; + case OVNACT_CT_COMMIT_CONTINUE: + break; case OVNACT_CT_DNAT: execute_ct_nat(ovnact_get_CT_DNAT(a), dp, uflow, pipeline, super); -- 2.38.1 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
