| Message ID | 20221122092933.291120-2-amusil@redhat.com |
|---|---|
| State | Accepted |
| Delegated to: | Dumitru Ceara |
| Headers | show |
| Series | Allow related traffic for LB | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
| ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
On 11/22/22 10:29, Ales Musil wrote: > Add action called ct_commit_nat, that performs > NAT while committing the connection. This is > useful for related traffic on which we need > to perform NAT, mainly ICMP. We need to > commit due to design decision of OvS[0]: > > "Connections identified as rel are separate from > the originating connection and must be committed separately." > > [0] http://www.openvswitch.org/support/dist-docs/ovs-fields.7.txt > > Reported-at: https://bugzilla.redhat.com/2126083 > Acked-by: Mark Michelson <mmichels@redhat.com> > Signed-off-by: Ales Musil <amusil@redhat.com> > --- > v3: Rebase on current main. > --- Hi Ales, The change looks mostly OK to me. I just have a few comments below. > include/ovn/actions.h | 3 +++ > lib/actions.c | 42 +++++++++++++++++++++++++++++++++++++++++- > ovn-sb.xml | 12 ++++++++++++ > tests/ovn.at | 5 +++++ > utilities/ovn-trace.c | 34 ++++++++++++++++++++++++++++++++++ > 5 files changed, 95 insertions(+), 1 deletion(-) > > diff --git a/include/ovn/actions.h b/include/ovn/actions.h > index fdb6ab08b..d1776d684 100644 > --- a/include/ovn/actions.h > +++ b/include/ovn/actions.h > @@ -74,6 +74,7 @@ struct ovn_extend_table; > OVNACT(CT_LB_MARK, ovnact_ct_lb) \ > OVNACT(SELECT, ovnact_select) \ > OVNACT(CT_CLEAR, ovnact_null) \ > + OVNACT(CT_COMMIT_NAT, ovnact_ct_nat) \ > OVNACT(CLONE, ovnact_nest) \ > OVNACT(ARP, ovnact_nest) \ > OVNACT(ICMP4, ovnact_nest) \ > @@ -277,6 +278,8 @@ struct ovnact_ct_nat { Can you please add OVNACT_CT_COMMIT_NAT to the comment just before 'struct ovnact_ct_nat {'? > uint16_t port_hi; > } port_range; > > + bool commit; /* Explicit commit action. */ > + > uint8_t ltable; /* Logical table ID of next table. */ > }; > > diff --git a/lib/actions.c b/lib/actions.c > index b59f364bf..fb688eeb1 100644 > --- a/lib/actions.c > +++ b/lib/actions.c > @@ -920,6 +920,7 @@ parse_ct_nat(struct action_context *ctx, const char *name, > return; > } > cn->ltable = ctx->pp->cur_ltable + 1; > + cn->commit = false; > > if (lexer_match(ctx->lexer, LEX_T_LPAREN)) { > if (ctx->lexer->token.type != LEX_T_INTEGER > @@ -929,9 +930,11 @@ parse_ct_nat(struct action_context *ctx, const char *name, > return; > } > if (ctx->lexer->token.format == LEX_F_IPV4) { > + cn->commit = true; > cn->family = AF_INET; > cn->ipv4 = ctx->lexer->token.value.ipv4; > } else if (ctx->lexer->token.format == LEX_F_IPV6) { > + cn->commit = true; > cn->family = AF_INET6; > cn->ipv6 = ctx->lexer->token.value.ipv6; > } > @@ -1004,6 +1007,24 @@ parse_CT_SNAT_IN_CZONE(struct action_context *ctx) > ovnact_put_CT_SNAT_IN_CZONE(ctx->ovnacts)); > } > > +static void > +parse_CT_COMMIT_NAT(struct action_context *ctx) > +{ > + add_prerequisite(ctx, "ip"); > + > + if (ctx->pp->cur_ltable >= ctx->pp->n_tables) { > + lexer_error(ctx->lexer, > + "\"ct_commit_related\" action not allowed in last table."); > + return; > + } > + > + struct ovnact_ct_nat *cn = ovnact_put_CT_COMMIT_NAT(ctx->ovnacts); > + cn->commit = true; > + cn->ltable = ctx->pp->cur_ltable + 1; > + cn->family = AF_UNSPEC; > + cn->port_range.exists = false; > +} > + > static void > format_ct_nat(const struct ovnact_ct_nat *cn, const char *name, struct ds *s) > { > @@ -1053,6 +1074,12 @@ format_CT_SNAT_IN_CZONE(const struct ovnact_ct_nat *cn, struct ds *s) > format_ct_nat(cn, "ct_snat_in_czone", s); > } > > +static void > +format_CT_COMMIT_NAT(const struct ovnact_ct_nat *cn OVS_UNUSED, struct ds *s) > +{ > + ds_put_cstr(s, "ct_commit_nat;"); > +} > + > static void > encode_ct_nat(const struct ovnact_ct_nat *cn, > const struct ovnact_encode_params *ep, > @@ -1104,7 +1131,7 @@ encode_ct_nat(const struct ovnact_ct_nat *cn, > > ofpacts->header = ofpbuf_push_uninit(ofpacts, nat_offset); > ct = ofpacts->header; > - if (cn->family == AF_INET || cn->family == AF_INET6) { > + if (cn->commit) { > ct->flags |= NX_CT_F_COMMIT; > } > ofpact_finish(ofpacts, &ct->ofpact); > @@ -1143,6 +1170,17 @@ encode_CT_SNAT_IN_CZONE(const struct ovnact_ct_nat *cn, > encode_ct_nat(cn, ep, true, ep->common_nat_ct_zone, ofpacts); > } > > +static void > +encode_CT_COMMIT_NAT(const struct ovnact_ct_nat *cn, > + const struct ovnact_encode_params *ep, > + struct ofpbuf *ofpacts) > +{ > + enum mf_field_id zone = ep->is_switch > + ? MFF_LOG_CT_ZONE > + : MFF_LOG_DNAT_ZONE; > + encode_ct_nat(cn, ep, false, zone, ofpacts); We only deal with DNAT (third argument is 'false'). I'm pretty sure there can also be a case when we need to allow and commit related SNATed traffic. Or am I missing something? > +} Thanks, Dumitru
On 11/25/22 12:08, Dumitru Ceara wrote: > On 11/22/22 10:29, Ales Musil wrote: >> Add action called ct_commit_nat, that performs >> NAT while committing the connection. This is >> useful for related traffic on which we need >> to perform NAT, mainly ICMP. We need to >> commit due to design decision of OvS[0]: >> >> "Connections identified as rel are separate from >> the originating connection and must be committed separately." >> >> [0] http://www.openvswitch.org/support/dist-docs/ovs-fields.7.txt >> >> Reported-at: https://bugzilla.redhat.com/2126083 >> Acked-by: Mark Michelson <mmichels@redhat.com> >> Signed-off-by: Ales Musil <amusil@redhat.com> >> --- >> v3: Rebase on current main. >> --- [...] >> @@ -1143,6 +1170,17 @@ encode_CT_SNAT_IN_CZONE(const struct ovnact_ct_nat *cn, >> encode_ct_nat(cn, ep, true, ep->common_nat_ct_zone, ofpacts); >> } >> >> +static void >> +encode_CT_COMMIT_NAT(const struct ovnact_ct_nat *cn, >> + const struct ovnact_encode_params *ep, >> + struct ofpbuf *ofpacts) >> +{ >> + enum mf_field_id zone = ep->is_switch >> + ? MFF_LOG_CT_ZONE >> + : MFF_LOG_DNAT_ZONE; >> + encode_ct_nat(cn, ep, false, zone, ofpacts); > > We only deal with DNAT (third argument is 'false'). I'm pretty sure > there can also be a case when we need to allow and commit related SNATed > traffic. Or am I missing something? > Ales pointed out privately that we only use the value of the third argument of echode_ct_nat() if the address family is unspecified. That's not the case. So we will be committing both SNATed and DNATed traffic. It should be fine as is. Regards, Dumitru
diff --git a/include/ovn/actions.h b/include/ovn/actions.h index fdb6ab08b..d1776d684 100644 --- a/include/ovn/actions.h +++ b/include/ovn/actions.h @@ -74,6 +74,7 @@ struct ovn_extend_table; OVNACT(CT_LB_MARK, ovnact_ct_lb) \ OVNACT(SELECT, ovnact_select) \ OVNACT(CT_CLEAR, ovnact_null) \ + OVNACT(CT_COMMIT_NAT, ovnact_ct_nat) \ OVNACT(CLONE, ovnact_nest) \ OVNACT(ARP, ovnact_nest) \ OVNACT(ICMP4, ovnact_nest) \ @@ -277,6 +278,8 @@ struct ovnact_ct_nat { uint16_t port_hi; } port_range; + bool commit; /* Explicit commit action. */ + uint8_t ltable; /* Logical table ID of next table. */ }; diff --git a/lib/actions.c b/lib/actions.c index b59f364bf..fb688eeb1 100644 --- a/lib/actions.c +++ b/lib/actions.c @@ -920,6 +920,7 @@ parse_ct_nat(struct action_context *ctx, const char *name, return; } cn->ltable = ctx->pp->cur_ltable + 1; + cn->commit = false; if (lexer_match(ctx->lexer, LEX_T_LPAREN)) { if (ctx->lexer->token.type != LEX_T_INTEGER @@ -929,9 +930,11 @@ parse_ct_nat(struct action_context *ctx, const char *name, return; } if (ctx->lexer->token.format == LEX_F_IPV4) { + cn->commit = true; cn->family = AF_INET; cn->ipv4 = ctx->lexer->token.value.ipv4; } else if (ctx->lexer->token.format == LEX_F_IPV6) { + cn->commit = true; cn->family = AF_INET6; cn->ipv6 = ctx->lexer->token.value.ipv6; } @@ -1004,6 +1007,24 @@ parse_CT_SNAT_IN_CZONE(struct action_context *ctx) ovnact_put_CT_SNAT_IN_CZONE(ctx->ovnacts)); } +static void +parse_CT_COMMIT_NAT(struct action_context *ctx) +{ + add_prerequisite(ctx, "ip"); + + if (ctx->pp->cur_ltable >= ctx->pp->n_tables) { + lexer_error(ctx->lexer, + "\"ct_commit_related\" action not allowed in last table."); + return; + } + + struct ovnact_ct_nat *cn = ovnact_put_CT_COMMIT_NAT(ctx->ovnacts); + cn->commit = true; + cn->ltable = ctx->pp->cur_ltable + 1; + cn->family = AF_UNSPEC; + cn->port_range.exists = false; +} + static void format_ct_nat(const struct ovnact_ct_nat *cn, const char *name, struct ds *s) { @@ -1053,6 +1074,12 @@ format_CT_SNAT_IN_CZONE(const struct ovnact_ct_nat *cn, struct ds *s) format_ct_nat(cn, "ct_snat_in_czone", s); } +static void +format_CT_COMMIT_NAT(const struct ovnact_ct_nat *cn OVS_UNUSED, struct ds *s) +{ + ds_put_cstr(s, "ct_commit_nat;"); +} + static void encode_ct_nat(const struct ovnact_ct_nat *cn, const struct ovnact_encode_params *ep, @@ -1104,7 +1131,7 @@ encode_ct_nat(const struct ovnact_ct_nat *cn, ofpacts->header = ofpbuf_push_uninit(ofpacts, nat_offset); ct = ofpacts->header; - if (cn->family == AF_INET || cn->family == AF_INET6) { + if (cn->commit) { ct->flags |= NX_CT_F_COMMIT; } ofpact_finish(ofpacts, &ct->ofpact); @@ -1143,6 +1170,17 @@ encode_CT_SNAT_IN_CZONE(const struct ovnact_ct_nat *cn, encode_ct_nat(cn, ep, true, ep->common_nat_ct_zone, ofpacts); } +static void +encode_CT_COMMIT_NAT(const struct ovnact_ct_nat *cn, + const struct ovnact_encode_params *ep, + struct ofpbuf *ofpacts) +{ + enum mf_field_id zone = ep->is_switch + ? MFF_LOG_CT_ZONE + : MFF_LOG_DNAT_ZONE; + encode_ct_nat(cn, ep, false, zone, ofpacts); +} + static void ovnact_ct_nat_free(struct ovnact_ct_nat *ct_nat OVS_UNUSED) { @@ -5143,6 +5181,8 @@ parse_action(struct action_context *ctx) parse_ct_lb_action(ctx, true); } else if (lexer_match_id(ctx->lexer, "ct_clear")) { ovnact_put_CT_CLEAR(ctx->ovnacts); + } else if (lexer_match_id(ctx->lexer, "ct_commit_nat")) { + parse_CT_COMMIT_NAT(ctx); } else if (lexer_match_id(ctx->lexer, "clone")) { parse_CLONE(ctx); } else if (lexer_match_id(ctx->lexer, "arp")) { diff --git a/ovn-sb.xml b/ovn-sb.xml index 4fe466134..0f093349e 100644 --- a/ovn-sb.xml +++ b/ovn-sb.xml @@ -1469,6 +1469,18 @@ Clears connection tracking state. </dd> + <dt><code>ct_commit_nat;</code></dt> + <dd> + <p> + Applies NAT and commits the connection to the CT. Automatically + moves on to the next table, as if followed by + <code>next</code>. + This is very useful for connections that are in related state for + already existing connections and allows the NAT to be applied + to them as well. + </p> + </dd> + <dt><code>clone { <var>action</var>; </code>...<code> };</code></dt> <dd> Makes a copy of the packet being processed and executes each diff --git a/tests/ovn.at b/tests/ovn.at index dde0f582b..66f5cb137 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -1418,6 +1418,11 @@ ct_snat_in_czone(192.168.1.2, 1000-100); ct_clear; encodes as ct_clear +# ct_commit_nat +ct_commit_nat; + encodes as ct(commit,table=19,zone=NXM_NX_REG13[0..15],nat) + has prereqs ip + # clone clone { ip4.dst = 255.255.255.255; output; }; next; encodes as clone(set_field:255.255.255.255->ip_dst,resubmit(,64)),resubmit(,19) diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c index 8e3a8d9ca..5d76ddb53 100644 --- a/utilities/ovn-trace.c +++ b/utilities/ovn-trace.c @@ -2438,6 +2438,35 @@ execute_ct_nat(const struct ovnact_ct_nat *ct_nat, * flow, not ct_flow. */ } +static void +execute_ct_commit_nat(const struct ovnact_ct_nat *ct_nat, + const struct ovntrace_datapath *dp, struct flow *uflow, + enum ovnact_pipeline pipeline, struct ovs_list *super) +{ + struct flow ct_flow = *uflow; + struct ds s = DS_EMPTY_INITIALIZER; + + ds_put_cstr(&s, "ct_commit_nat /* assuming no" + " un-nat entry, so no change */"); + + /* ct(nat) implies ct(). */ + if (!(ct_flow.ct_state & CS_TRACKED)) { + ct_flow.ct_state |= next_ct_state(&s); + } + + struct ovntrace_node *node = ovntrace_node_append( + super, OVNTRACE_NODE_TRANSFORMATION, "%s", ds_cstr(&s)); + ds_destroy(&s); + + /* Trace the actions in the next table. */ + trace__(dp, &ct_flow, ct_nat->ltable, pipeline, &node->subs); + + /* Upon return, we will trace the actions following the ct action in the + * original table. The pipeline forked, so we're using the original + * flow, not ct_flow. */ +} + + static void execute_ct_lb(const struct ovnact_ct_lb *ct_lb, const struct ovntrace_datapath *dp, struct flow *uflow, @@ -3095,6 +3124,11 @@ trace_actions(const struct ovnact *ovnacts, size_t ovnacts_len, flow_clear_conntrack(uflow); break; + case OVNACT_CT_COMMIT_NAT: + execute_ct_commit_nat(ovnact_get_CT_COMMIT_NAT(a), dp, uflow, + pipeline, super); + break; + case OVNACT_CLONE: execute_clone(ovnact_get_CLONE(a), dp, uflow, table_id, pipeline, super);