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);
