| Message ID | a4a2f1b8107c0e3a4fee7eb691d6e89e7b94baef.1648561123.git.lorenzo.bianconi@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] northd: fix stateless nat with allowed_ext_ips | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
Hi Lorenzo, Please add the following to the commit message: Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2066990 I have a few more things below. On 3/29/22 09:39, Lorenzo Bianconi wrote: > When a nat rule is configured in stateless mode there is a 1:1 mapping > between external_ip and logical_ip. Do not modify dst/src ips in > S_ROUTER_IN_UNSNAT/S_ROUTER_OUT_UNDNAT stages for stateless nat entries > since they will be properly modified in S_ROUTER_IN_DNAT/S_ROUTER_OUT_SNAT > stages. > This patch will allow respecting allowed_ext_ips for stateless nat > rules. > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > --- > northd/northd.c | 41 +++++++++++++++++++++++++++++++++++------ > tests/ovn-northd.at | 4 ++-- > tests/ovn.at | 4 ++-- > 3 files changed, 39 insertions(+), 10 deletions(-) > > diff --git a/northd/northd.c b/northd/northd.c > index a2cf8d6fc..ba0e1f9d7 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -11890,6 +11890,38 @@ build_gateway_redirect_flows_for_lrouter( > ds_cstr(match), ds_cstr(actions), > stage_hint); > } > + > + for (int i = 0; i < od->nbr->n_nat; i++) { > + const struct nbrec_nat *nat = nat = od->nbr->nat[i]; There appears to be an extra assignment above. > + if (!lrouter_nat_is_stateless(nat) || > + strcmp(nat->type, "dnat_and_snat")) { > + continue; > + } > + > + bool is_v6 = false; > + ovs_be32 ip, mask; > + char *error = ip_parse_masked(nat->external_ip, &ip, &mask); > + if (error || mask != OVS_BE32_MAX) { > + free(error); > + struct in6_addr ipv6, mask_v6, v6_exact = IN6ADDR_EXACT_INIT; > + error = ipv6_parse_masked(nat->external_ip, &ipv6, &mask_v6); > + if (error || memcmp(&mask_v6, &v6_exact, sizeof(mask_v6))) { > + /* Invalid for both IPv4 and IPv6 */ > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); > + VLOG_WARN_RL(&rl, "bad external ip %s for nat", > + nat->external_ip); > + free(error); > + continue; > + } > + is_v6 = true; > + } You can avoid parsing IP addresses here by using the nat_entries on the ovn_datapath. Example: const struct ovn_nat *nat = od->nat_entries[i]; if (!lrouter_nat_is_stateless(nat->nb) || strcmp(nat->nb->type, "dnat_and_snat")) { continue; } bool is_v6 = nat_entry_is_v6(nat); > + ds_clear(match); > + ds_put_format(match, "ip && ip%s.dst == %s", is_v6 ? "6" : "4", > + nat->external_ip); > + ovn_lflow_add(lflows, od, S_ROUTER_IN_GW_REDIRECT, 100, > + ds_cstr(match), "drop;"); > + } > + > /* Packets are allowed by default. */ > ovn_lflow_add(lflows, od, S_ROUTER_IN_GW_REDIRECT, 0, "1", "next;"); > } > @@ -12638,8 +12670,7 @@ build_lrouter_in_unsnat_flow(struct hmap *lflows, struct ovn_datapath *od, > ds_put_format(match, "ip && ip%s.dst == %s", > is_v6 ? "6" : "4", nat->external_ip); > if (!strcmp(nat->type, "dnat_and_snat") && stateless) { > - ds_put_format(actions, "ip%s.dst=%s; next;", > - is_v6 ? "6" : "4", nat->logical_ip); > + ds_put_format(actions, "next;"); > } else { > ds_put_cstr(actions, "ct_snat;"); > } > @@ -12664,8 +12695,7 @@ build_lrouter_in_unsnat_flow(struct hmap *lflows, struct ovn_datapath *od, > } > > if (!strcmp(nat->type, "dnat_and_snat") && stateless) { > - ds_put_format(actions, "ip%s.dst=%s; next;", > - is_v6 ? "6" : "4", nat->logical_ip); > + ds_put_format(actions, "next;"); > } else { > ds_put_cstr(actions, "ct_snat_in_czone;"); > } > @@ -12818,8 +12848,7 @@ build_lrouter_out_undnat_flow(struct hmap *lflows, struct ovn_datapath *od, > > if (!strcmp(nat->type, "dnat_and_snat") && > lrouter_nat_is_stateless(nat)) { > - ds_put_format(actions, "ip%s.src=%s; next;", > - is_v6 ? "6" : "4", nat->external_ip); > + ds_put_format(actions, "next;"); > } else { > ds_put_format(actions, > od->is_gw_router ? "ct_dnat;" : "ct_dnat_in_czone;"); > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index 17d4f31b3..e5e93b870 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -895,7 +895,7 @@ ovn-nbctl lr-nat-del R1 dnat_and_snat 172.16.1.1 > echo > echo "IPv4: stateless" > ovn-nbctl --wait=sb --stateless lr-nat-add R1 dnat_and_snat 172.16.1.1 50.0.0.11 > -check_flow_match_sets 2 0 0 2 2 0 0 > +check_flow_match_sets 2 0 0 1 1 0 0 > ovn-nbctl lr-nat-del R1 dnat_and_snat 172.16.1.1 > > echo > @@ -907,7 +907,7 @@ ovn-nbctl lr-nat-del R1 dnat_and_snat fd01::1 > echo > echo "IPv6: stateless" > ovn-nbctl --wait=sb --stateless lr-nat-add R1 dnat_and_snat fd01::1 fd11::2 > -check_flow_match_sets 2 0 0 0 0 2 2 > +check_flow_match_sets 2 0 0 0 0 1 1 > > AT_CLEANUP > ]) > diff --git a/tests/ovn.at b/tests/ovn.at > index 166b5f72e..925949075 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -21708,8 +21708,8 @@ AT_CHECK([for regex in ct_snat ct_dnat ip4.dst= ip4.src=; do > grep -c "$regex" sbflows; > done], [0], [0 > 0 > -2 > -2 > +1 > +1 > ]) > > echo "----------- Post Traffic hv1 dump -----------" >
diff --git a/northd/northd.c b/northd/northd.c index a2cf8d6fc..ba0e1f9d7 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -11890,6 +11890,38 @@ build_gateway_redirect_flows_for_lrouter( ds_cstr(match), ds_cstr(actions), stage_hint); } + + for (int i = 0; i < od->nbr->n_nat; i++) { + const struct nbrec_nat *nat = nat = od->nbr->nat[i]; + if (!lrouter_nat_is_stateless(nat) || + strcmp(nat->type, "dnat_and_snat")) { + continue; + } + + bool is_v6 = false; + ovs_be32 ip, mask; + char *error = ip_parse_masked(nat->external_ip, &ip, &mask); + if (error || mask != OVS_BE32_MAX) { + free(error); + struct in6_addr ipv6, mask_v6, v6_exact = IN6ADDR_EXACT_INIT; + error = ipv6_parse_masked(nat->external_ip, &ipv6, &mask_v6); + if (error || memcmp(&mask_v6, &v6_exact, sizeof(mask_v6))) { + /* Invalid for both IPv4 and IPv6 */ + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); + VLOG_WARN_RL(&rl, "bad external ip %s for nat", + nat->external_ip); + free(error); + continue; + } + is_v6 = true; + } + ds_clear(match); + ds_put_format(match, "ip && ip%s.dst == %s", is_v6 ? "6" : "4", + nat->external_ip); + ovn_lflow_add(lflows, od, S_ROUTER_IN_GW_REDIRECT, 100, + ds_cstr(match), "drop;"); + } + /* Packets are allowed by default. */ ovn_lflow_add(lflows, od, S_ROUTER_IN_GW_REDIRECT, 0, "1", "next;"); } @@ -12638,8 +12670,7 @@ build_lrouter_in_unsnat_flow(struct hmap *lflows, struct ovn_datapath *od, ds_put_format(match, "ip && ip%s.dst == %s", is_v6 ? "6" : "4", nat->external_ip); if (!strcmp(nat->type, "dnat_and_snat") && stateless) { - ds_put_format(actions, "ip%s.dst=%s; next;", - is_v6 ? "6" : "4", nat->logical_ip); + ds_put_format(actions, "next;"); } else { ds_put_cstr(actions, "ct_snat;"); } @@ -12664,8 +12695,7 @@ build_lrouter_in_unsnat_flow(struct hmap *lflows, struct ovn_datapath *od, } if (!strcmp(nat->type, "dnat_and_snat") && stateless) { - ds_put_format(actions, "ip%s.dst=%s; next;", - is_v6 ? "6" : "4", nat->logical_ip); + ds_put_format(actions, "next;"); } else { ds_put_cstr(actions, "ct_snat_in_czone;"); } @@ -12818,8 +12848,7 @@ build_lrouter_out_undnat_flow(struct hmap *lflows, struct ovn_datapath *od, if (!strcmp(nat->type, "dnat_and_snat") && lrouter_nat_is_stateless(nat)) { - ds_put_format(actions, "ip%s.src=%s; next;", - is_v6 ? "6" : "4", nat->external_ip); + ds_put_format(actions, "next;"); } else { ds_put_format(actions, od->is_gw_router ? "ct_dnat;" : "ct_dnat_in_czone;"); diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index 17d4f31b3..e5e93b870 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -895,7 +895,7 @@ ovn-nbctl lr-nat-del R1 dnat_and_snat 172.16.1.1 echo echo "IPv4: stateless" ovn-nbctl --wait=sb --stateless lr-nat-add R1 dnat_and_snat 172.16.1.1 50.0.0.11 -check_flow_match_sets 2 0 0 2 2 0 0 +check_flow_match_sets 2 0 0 1 1 0 0 ovn-nbctl lr-nat-del R1 dnat_and_snat 172.16.1.1 echo @@ -907,7 +907,7 @@ ovn-nbctl lr-nat-del R1 dnat_and_snat fd01::1 echo echo "IPv6: stateless" ovn-nbctl --wait=sb --stateless lr-nat-add R1 dnat_and_snat fd01::1 fd11::2 -check_flow_match_sets 2 0 0 0 0 2 2 +check_flow_match_sets 2 0 0 0 0 1 1 AT_CLEANUP ]) diff --git a/tests/ovn.at b/tests/ovn.at index 166b5f72e..925949075 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -21708,8 +21708,8 @@ AT_CHECK([for regex in ct_snat ct_dnat ip4.dst= ip4.src=; do grep -c "$regex" sbflows; done], [0], [0 0 -2 -2 +1 +1 ]) echo "----------- Post Traffic hv1 dump -----------"
When a nat rule is configured in stateless mode there is a 1:1 mapping between external_ip and logical_ip. Do not modify dst/src ips in S_ROUTER_IN_UNSNAT/S_ROUTER_OUT_UNDNAT stages for stateless nat entries since they will be properly modified in S_ROUTER_IN_DNAT/S_ROUTER_OUT_SNAT stages. This patch will allow respecting allowed_ext_ips for stateless nat rules. Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> --- northd/northd.c | 41 +++++++++++++++++++++++++++++++++++------ tests/ovn-northd.at | 4 ++-- tests/ovn.at | 4 ++-- 3 files changed, 39 insertions(+), 10 deletions(-)