On 1/10/23 16:58, Ales Musil wrote:
> On Mon, Jan 9, 2023 at 3:35 PM Ales Musil <[email protected]> wrote:
> 
>> In order to be backward compatible add feature flag
>> that ensures that the CT related flows are skipped
>> if needed.
>>
>> Signed-off-by: Ales Musil <[email protected]>
>> ---
>> v2: Fix the wrong * position.
>> ---
>>  controller/chassis.c   |   7 +++
>>  include/ovn/features.h |   1 +
>>  northd/northd.c        |  51 +++++++++++++------
>>  northd/northd.h        |   1 +
>>  tests/ovn-northd.at    | 109 +++++++++++++++++++++++++++++++++++++++--
>>  5 files changed, 149 insertions(+), 20 deletions(-)
>>
>> diff --git a/controller/chassis.c b/controller/chassis.c
>> index 685d9b2ae..98f8da2be 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_LB_RELATED, "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_LB_RELATED,
>> +                       false)) {
>> +        return true;
>> +    }
>> +
>>      return false;
>>  }
>>
>> diff --git a/include/ovn/features.h b/include/ovn/features.h
>> index 679f67457..5bcd68739 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_LB_RELATED "ovn-ct-lb-related"
>>
>>  /* OVS datapath supported features.  Based on availability OVN might
>> generate
>>   * different types of openflows.
>> diff --git a/northd/northd.c b/northd/northd.c
>> index 4751feab4..72f7118a8 100644
>> --- a/northd/northd.c
>> +++ b/northd/northd.c
>> @@ -447,6 +447,15 @@ build_chassis_features(const struct northd_input
>> *input_data,
>>              chassis_features->mac_binding_timestamp) {
>>              chassis_features->mac_binding_timestamp = false;
>>          }
>> +
>> +        bool ct_lb_related =
>> +            smap_get_bool(&chassis->other_config,
>> +                          OVN_FEATURE_CT_LB_RELATED,
>> +                          false);
>> +        if (!ct_lb_related &&
>> +            chassis_features->ct_lb_related) {
>> +            chassis_features->ct_lb_related = false;
>> +        }
>>      }
>>  }
>>
>> @@ -6786,14 +6795,17 @@ build_acls(struct ovn_datapath *od, const struct
>> chassis_features *features,
>>           * a dynamically negotiated FTP data channel), but will allow
>>           * related traffic such as an ICMP Port Unreachable through
>>           * that's generated from a non-listening UDP port.  */
>> +        const char *ct_acl_action = features->ct_lb_related
>> +                                    ? "ct_commit_nat;"
>> +                                    : "next;";
>>          ds_clear(&match);
>>          ds_put_format(&match, "!ct.est && ct.rel && !ct.new%s && %s == 0",
>>                        use_ct_inv_match ? " && !ct.inv" : "",
>>                        ct_blocked_match);
>>          ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX - 3,
>> -                      ds_cstr(&match), "ct_commit_nat;");
>> +                      ds_cstr(&match), ct_acl_action);
>>          ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX - 3,
>> -                      ds_cstr(&match), "ct_commit_nat;");
>> +                      ds_cstr(&match), ct_acl_action);
>>
>>          /* Ingress and Egress ACL Table (Priority 65532).
>>           *
>> @@ -10484,9 +10496,11 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip
>> *lb_vip,
>>                                 struct hmap *lflows,
>>                                 struct ds *match, struct ds *action,
>>                                 const struct shash *meter_groups,
>> -                               bool ct_lb_mark)
>> +                               const struct chassis_features *features)
>>  {
>> -    const char *ct_natted = ct_lb_mark ? "ct_mark.natted" :
>> "ct_label.natted";
>> +    const char *ct_natted = features->ct_no_masked_label
>> +                            ? "ct_mark.natted"
>> +                            : "ct_label.natted";
>>      char *skip_snat_new_action = NULL;
>>      char *skip_snat_est_action = NULL;
>>      char *new_match;
>> @@ -10497,7 +10511,7 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip
>> *lb_vip,
>>
>>      bool reject = build_lb_vip_actions(lb_vip, vips_nb, action,
>>                                         lb->selection_fields, false,
>> -                                       ct_lb_mark);
>> +                                       features->ct_no_masked_label);
>>      bool drop = !!strncmp(ds_cstr(action), "ct_lb", strlen("ct_lb"));
>>      if (!drop) {
>>          /* Remove the trailing ");". */
>> @@ -10519,9 +10533,11 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip
>> *lb_vip,
>>      }
>>
>>      if (lb->skip_snat) {
>> +        const char *skip_snat = features->ct_lb_related && !drop
>> +                                ? "; skip_snat);"
>> +                                : "";
>>          skip_snat_new_action = xasprintf("flags.skip_snat_for_lb = 1;
>> %s%s",
>> -                                         ds_cstr(action),
>> -                                         drop ? "" : "; skip_snat);");
>> +                                         ds_cstr(action), skip_snat);
>>          skip_snat_est_action = xasprintf("flags.skip_snat_for_lb = 1; "
>>                                           "next;");
>>      }
>> @@ -10654,9 +10670,11 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip
>> *lb_vip,
>>              skip_snat_new_action, est_match,
>>              skip_snat_est_action, lflows, prio, meter_groups);
>>
>> +    const char *force_snat = features->ct_lb_related && !drop
>> +                             ? "; force_snat);"
>> +                             : "";
>>      char *new_actions = xasprintf("flags.force_snat_for_lb = 1; %s%s",
>> -                                  ds_cstr(action),
>> -                                  drop ? "" : "; force_snat);");
>> +                                  ds_cstr(action), force_snat);
>>      build_gw_lrouter_nat_flows_for_lb(lb, gw_router_force_snat,
>>              n_gw_router_force_snat, reject, new_match,
>>              new_actions, est_match,
>> @@ -10911,7 +10929,7 @@ build_lrouter_flows_for_lb(struct ovn_northd_lb
>> *lb, struct hmap *lflows,
>>
>>          build_lrouter_nat_flows_for_lb(lb_vip, lb, &lb->vips_nb[i],
>>                                         lflows, match, action,
>> meter_groups,
>> -                                       features->ct_no_masked_label);
>> +                                       features);
>>
>>          if (!build_empty_lb_event_flow(lb_vip, lb, match, action)) {
>>              continue;
>> @@ -14221,7 +14239,7 @@ build_lrouter_nat_defrag_and_lb(struct
>> ovn_datapath *od, struct hmap *lflows,
>>                                  const struct hmap *ports, struct ds
>> *match,
>>                                  struct ds *actions,
>>                                  const struct shash *meter_groups,
>> -                                bool ct_lb_mark)
>> +                                const struct chassis_features *features)
>>  {
>>      if (!od->nbr) {
>>          return;
>> @@ -14252,9 +14270,11 @@ build_lrouter_nat_defrag_and_lb(struct
>> ovn_datapath *od, struct hmap *lflows,
>>       * a dynamically negotiated FTP data channel), but will allow
>>       * related traffic such as an ICMP Port Unreachable through
>>       * that's generated from a non-listening UDP port.  */
>> -    if (od->has_lb_vip) {
>> +    if (od->has_lb_vip && features->ct_lb_related) {
>>          ds_clear(match);
>> -        const char *ct_flag_reg = ct_lb_mark ? "ct_mark" : "ct_label";
>> +        const char *ct_flag_reg = features->ct_no_masked_label
>> +                                  ? "ct_mark"
>> +                                  : "ct_label";
>>
>>          ds_put_cstr(match, "ct.rel && !ct.est && !ct.new");
>>          size_t match_len = match->length;
>> @@ -14477,7 +14497,7 @@ build_lrouter_nat_defrag_and_lb(struct
>> ovn_datapath *od, struct hmap *lflows,
>>
>>      if (od->nbr->n_nat) {
>>          ds_clear(match);
>> -        const char *ct_natted = ct_lb_mark ?
>> +        const char *ct_natted = features->ct_no_masked_label ?
>>                                  "ct_mark.natted" :
>>                                  "ct_label.natted";
>>          ds_put_format(match, "ip && %s == 1", ct_natted);
>> @@ -14594,7 +14614,7 @@ build_lswitch_and_lrouter_iterate_by_od(struct
>> ovn_datapath *od,
>>      build_lrouter_arp_nd_for_datapath(od, lsi->lflows, lsi->meter_groups);
>>      build_lrouter_nat_defrag_and_lb(od, lsi->lflows, lsi->ports,
>> &lsi->match,
>>                                      &lsi->actions, lsi->meter_groups,
>> -                                    lsi->features->ct_no_masked_label);
>> +                                    lsi->features);
>>      build_lb_affinity_default_flows(od, lsi->lflows);
>>  }
>>
>> @@ -16086,6 +16106,7 @@ northd_init(struct northd_data *data)
>>      data->features = (struct chassis_features) {
>>          .ct_no_masked_label = true,
>>          .mac_binding_timestamp = true,
>> +        .ct_lb_related = true,
>>      };
>>      data->ovn_internal_version_changed = false;
>>  }
>> diff --git a/northd/northd.h b/northd/northd.h
>> index ff8727cb7..4d9055296 100644
>> --- a/northd/northd.h
>> +++ b/northd/northd.h
>> @@ -71,6 +71,7 @@ struct northd_input {
>>  struct chassis_features {
>>      bool ct_no_masked_label;
>>      bool mac_binding_timestamp;
>> +    bool ct_lb_related;
>>  };
>>
>>  struct northd_data {
>> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
>> index 56c1e6c2e..2eb735bbe 100644
>> --- a/tests/ovn-northd.at
>> +++ b/tests/ovn-northd.at
>> @@ -5127,7 +5127,8 @@ AT_CHECK([grep "lr_out_snat" lr0flows | sed
>> 's/table=./table=?/' | sort], [0], [
>>  ])
>>
>>  check ovn-sbctl chassis-add gw1 geneve 127.0.0.1 \
>> -  -- set chassis gw1 other_config:ct-no-masked-label="true"
>> +  -- set chassis gw1 other_config:ct-no-masked-label="true" \
>> +  -- set chassis gw1 other_config:ovn-ct-lb-related="true"
>>
>>  # Create a distributed gw port on lr0
>>  check ovn-nbctl ls-add public
>> @@ -7875,7 +7876,7 @@ AT_CHECK([ovn-sbctl lflow-list | grep
>> 'ls.*acl.*blocked' ], [0], [dnl
>>    table=7 (ls_in_acl_hint     ), priority=4    , match=(!ct.new && ct.est
>> && !ct.rpl && ct_label.blocked == 0), action=(reg0[[8]] = 1; reg0[[10]] =
>> 1; next;)
>>    table=7 (ls_in_acl_hint     ), priority=2    , match=(ct.est &&
>> ct_label.blocked == 1), action=(reg0[[9]] = 1; next;)
>>    table=7 (ls_in_acl_hint     ), priority=1    , match=(ct.est &&
>> ct_label.blocked == 0), action=(reg0[[10]] = 1; next;)
>> -  table=8 (ls_in_acl          ), priority=65532, match=(!ct.est && ct.rel
>> && !ct.new && !ct.inv && ct_label.blocked == 0), action=(ct_commit_nat;)
>> +  table=8 (ls_in_acl          ), priority=65532, match=(!ct.est && ct.rel
>> && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;)
>>    table=8 (ls_in_acl          ), priority=65532, match=(ct.est && !ct.rel
>> && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0),
>> action=(reg0[[9]] = 0; reg0[[10]] = 0; next;)
>>    table=8 (ls_in_acl          ), priority=65532, match=(ct.inv || (ct.est
>> && ct.rpl && ct_label.blocked == 1)), action=(drop;)
>>    table=8 (ls_in_acl          ), priority=1    , match=(ip && ct.est &&
>> ct_label.blocked == 1), action=(reg0[[1]] = 1; next;)
>> @@ -7883,7 +7884,7 @@ AT_CHECK([ovn-sbctl lflow-list | grep
>> 'ls.*acl.*blocked' ], [0], [dnl
>>    table=3 (ls_out_acl_hint    ), priority=4    , match=(!ct.new && ct.est
>> && !ct.rpl && ct_label.blocked == 0), action=(reg0[[8]] = 1; reg0[[10]] =
>> 1; next;)
>>    table=3 (ls_out_acl_hint    ), priority=2    , match=(ct.est &&
>> ct_label.blocked == 1), action=(reg0[[9]] = 1; next;)
>>    table=3 (ls_out_acl_hint    ), priority=1    , match=(ct.est &&
>> ct_label.blocked == 0), action=(reg0[[10]] = 1; next;)
>> -  table=4 (ls_out_acl         ), priority=65532, match=(!ct.est && ct.rel
>> && !ct.new && !ct.inv && ct_label.blocked == 0), action=(ct_commit_nat;)
>> +  table=4 (ls_out_acl         ), priority=65532, match=(!ct.est && ct.rel
>> && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;)
>>    table=4 (ls_out_acl         ), priority=65532, match=(ct.est && !ct.rel
>> && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), action=(next;)
>>    table=4 (ls_out_acl         ), priority=65532, match=(ct.inv || (ct.est
>> && ct.rpl && ct_label.blocked == 1)), action=(drop;)
>>    table=4 (ls_out_acl         ), priority=1    , match=(ip && ct.est &&
>> ct_label.blocked == 1), action=(reg0[[1]] = 1; next;)
>> @@ -7897,7 +7898,7 @@ AT_CHECK([ovn-sbctl lflow-list | grep
>> 'ls.*acl.*blocked' ], [0], [dnl
>>    table=7 (ls_in_acl_hint     ), priority=4    , match=(!ct.new && ct.est
>> && !ct.rpl && ct_mark.blocked == 0), action=(reg0[[8]] = 1; reg0[[10]] = 1;
>> next;)
>>    table=7 (ls_in_acl_hint     ), priority=2    , match=(ct.est &&
>> ct_mark.blocked == 1), action=(reg0[[9]] = 1; next;)
>>    table=7 (ls_in_acl_hint     ), priority=1    , match=(ct.est &&
>> ct_mark.blocked == 0), action=(reg0[[10]] = 1; next;)
>> -  table=8 (ls_in_acl          ), priority=65532, match=(!ct.est && ct.rel
>> && !ct.new && !ct.inv && ct_mark.blocked == 0), action=(ct_commit_nat;)
>> +  table=8 (ls_in_acl          ), priority=65532, match=(!ct.est && ct.rel
>> && !ct.new && !ct.inv && ct_mark.blocked == 0), action=(next;)
>>    table=8 (ls_in_acl          ), priority=65532, match=(ct.est && !ct.rel
>> && !ct.new && !ct.inv && ct.rpl && ct_mark.blocked == 0), action=(reg0[[9]]
>> = 0; reg0[[10]] = 0; next;)
>>    table=8 (ls_in_acl          ), priority=65532, match=(ct.inv || (ct.est
>> && ct.rpl && ct_mark.blocked == 1)), action=(drop;)
>>    table=8 (ls_in_acl          ), priority=1    , match=(ip && ct.est &&
>> ct_mark.blocked == 1), action=(reg0[[1]] = 1; next;)
>> @@ -7905,7 +7906,7 @@ AT_CHECK([ovn-sbctl lflow-list | grep
>> 'ls.*acl.*blocked' ], [0], [dnl
>>    table=3 (ls_out_acl_hint    ), priority=4    , match=(!ct.new && ct.est
>> && !ct.rpl && ct_mark.blocked == 0), action=(reg0[[8]] = 1; reg0[[10]] = 1;
>> next;)
>>    table=3 (ls_out_acl_hint    ), priority=2    , match=(ct.est &&
>> ct_mark.blocked == 1), action=(reg0[[9]] = 1; next;)
>>    table=3 (ls_out_acl_hint    ), priority=1    , match=(ct.est &&
>> ct_mark.blocked == 0), action=(reg0[[10]] = 1; next;)
>> -  table=4 (ls_out_acl         ), priority=65532, match=(!ct.est && ct.rel
>> && !ct.new && !ct.inv && ct_mark.blocked == 0), action=(ct_commit_nat;)
>> +  table=4 (ls_out_acl         ), priority=65532, match=(!ct.est && ct.rel
>> && !ct.new && !ct.inv && ct_mark.blocked == 0), action=(next;)
>>    table=4 (ls_out_acl         ), priority=65532, match=(ct.est && !ct.rel
>> && !ct.new && !ct.inv && ct.rpl && ct_mark.blocked == 0), action=(next;)
>>    table=4 (ls_out_acl         ), priority=65532, match=(ct.inv || (ct.est
>> && ct.rpl && ct_mark.blocked == 1)), action=(drop;)
>>    table=4 (ls_out_acl         ), priority=1    , match=(ip && ct.est &&
>> ct_mark.blocked == 1), action=(reg0[[1]] = 1; next;)
>> @@ -8425,3 +8426,101 @@ check_row_count sb:Chassis_Template_Var 0
>>
>>  AT_CLEANUP
>>  ])
>> +
>> +OVN_FOR_EACH_NORTHD_NO_HV([
>> +AT_SETUP([Load balancer CT related backwards compatibility])
>> +AT_KEYWORDS([lb])
>> +ovn_start
>> +
>> +check ovn-nbctl                                               \
>> +  -- ls-add ls                                                \
>> +  -- lr-add lr -- set logical_router lr options:chassis=local \
>> +  -- lb-add lb-test 192.168.0.1 192.168.1.10                  \
>> +  -- ls-lb-add ls lb-test                                     \
>> +  -- lr-lb-add lr lb-test
>> +
>> +m4_define([DUMP_FLOWS_SORTED], [sed 's/table=[[0-9]]\{1,2\}/table=?/' |
>> sort])
>> +
>> +AS_BOX([No chassis registered - CT related flows should be installed])
>> +check ovn-nbctl --wait=sb sync
>> +ovn-sbctl dump-flows | DUMP_FLOWS_SORTED > lflows0
>> +
>> +AT_CHECK([grep -e "lr_in_defrag" -e "lr_in_dnat" lflows0], [0], [dnl
>> +  table=? (lr_in_defrag       ), priority=0    , match=(1), action=(next;)
>> +  table=? (lr_in_defrag       ), priority=100  , match=(ip && ip4.dst ==
>> 192.168.0.1), action=(reg0 = 192.168.0.1; ct_dnat;)
>> +  table=? (lr_in_defrag       ), priority=50   , match=(icmp || icmp6),
>> action=(ct_dnat;)
>> +  table=? (lr_in_dnat         ), priority=0    , match=(1), action=(next;)
>> +  table=? (lr_in_dnat         ), priority=110  , match=(ct.est && !ct.rel
>> && ip4 && reg0 == 192.168.0.1 && ct_mark.natted == 1), action=(next;)
>> +  table=? (lr_in_dnat         ), priority=110  , match=(ct.new && !ct.rel
>> && ip4 && reg0 == 192.168.0.1), action=(ct_lb_mark(backends=192.168.1.10);)
>> +  table=? (lr_in_dnat         ), priority=50   , match=(ct.rel && !ct.est
>> && !ct.new), action=(ct_commit_nat;)
>> +  table=? (lr_in_dnat         ), priority=70   , match=(ct.rel && !ct.est
>> && !ct.new && ct_mark.force_snat == 1), action=(flags.force_snat_for_lb =
>> 1; ct_commit_nat;)
>> +  table=? (lr_in_dnat         ), priority=70   , match=(ct.rel && !ct.est
>> && !ct.new && ct_mark.skip_snat == 1), action=(flags.skip_snat_for_lb = 1;
>> ct_commit_nat;)
>> +])
>> +
>> +AT_CHECK([grep -e "ls_in_acl" -e "ls_out_acl" lflows0 | grep
>> "priority=65532"], [0], [dnl
>> +  table=? (ls_in_acl          ), priority=65532, match=(!ct.est && ct.rel
>> && !ct.new && !ct.inv && ct_mark.blocked == 0), action=(ct_commit_nat;)
>> +  table=? (ls_in_acl          ), priority=65532, match=(ct.est && !ct.rel
>> && !ct.new && !ct.inv && ct.rpl && ct_mark.blocked == 0), action=(reg0[[9]]
>> = 0; reg0[[10]] = 0; next;)
>> +  table=? (ls_in_acl          ), priority=65532, match=(ct.inv || (ct.est
>> && ct.rpl && ct_mark.blocked == 1)), action=(drop;)
>> +  table=? (ls_in_acl          ), priority=65532, match=(nd || nd_ra ||
>> nd_rs || mldv1 || mldv2), action=(next;)
>> +  table=? (ls_out_acl         ), priority=65532, match=(!ct.est && ct.rel
>> && !ct.new && !ct.inv && ct_mark.blocked == 0), action=(ct_commit_nat;)
>> +  table=? (ls_out_acl         ), priority=65532, match=(ct.est && !ct.rel
>> && !ct.new && !ct.inv && ct.rpl && ct_mark.blocked == 0), action=(next;)
>> +  table=? (ls_out_acl         ), priority=65532, match=(ct.inv || (ct.est
>> && ct.rpl && ct_mark.blocked == 1)), action=(drop;)
>> +  table=? (ls_out_acl         ), priority=65532, match=(nd || nd_ra ||
>> nd_rs || mldv1 || mldv2), action=(next;)
>> +])
>> +
>> +
>> +AS_BOX([Chassis registered that doesn't support CT related])
>> +check ovn-sbctl chassis-add hv geneve 127.0.0.1
>> +check ovn-nbctl --wait=sb sync
>> +ovn-sbctl dump-flows | DUMP_FLOWS_SORTED > lflows1
>> +
>> +AT_CHECK([grep -e "lr_in_defrag" -e "lr_in_dnat" lflows1], [0], [dnl
>> +  table=? (lr_in_defrag       ), priority=0    , match=(1), action=(next;)
>> +  table=? (lr_in_defrag       ), priority=100  , match=(ip && ip4.dst ==
>> 192.168.0.1), action=(reg0 = 192.168.0.1; ct_dnat;)
>> +  table=? (lr_in_dnat         ), priority=0    , match=(1), action=(next;)
>> +  table=? (lr_in_dnat         ), priority=110  , match=(ct.est && !ct.rel
>> && ip4 && reg0 == 192.168.0.1 && ct_label.natted == 1), action=(next;)
>> +  table=? (lr_in_dnat         ), priority=110  , match=(ct.new && !ct.rel
>> && ip4 && reg0 == 192.168.0.1), action=(ct_lb(backends=192.168.1.10);)
>> +])
>> +
>> +AT_CHECK([grep -e "ls_in_acl" -e "ls_out_acl" lflows1 | grep
>> "priority=65532"], [0], [dnl
>> +  table=? (ls_in_acl          ), priority=65532, match=(!ct.est && ct.rel
>> && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;)
>> +  table=? (ls_in_acl          ), priority=65532, match=(ct.est && !ct.rel
>> && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0),
>> action=(reg0[[9]] = 0; reg0[[10]] = 0; next;)
>> +  table=? (ls_in_acl          ), priority=65532, match=(ct.inv || (ct.est
>> && ct.rpl && ct_label.blocked == 1)), action=(drop;)
>> +  table=? (ls_in_acl          ), priority=65532, match=(nd || nd_ra ||
>> nd_rs || mldv1 || mldv2), action=(next;)
>> +  table=? (ls_out_acl         ), priority=65532, match=(!ct.est && ct.rel
>> && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;)
>> +  table=? (ls_out_acl         ), priority=65532, match=(ct.est && !ct.rel
>> && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), action=(next;)
>> +  table=? (ls_out_acl         ), priority=65532, match=(ct.inv || (ct.est
>> && ct.rpl && ct_label.blocked == 1)), action=(drop;)
>> +  table=? (ls_out_acl         ), priority=65532, match=(nd || nd_ra ||
>> nd_rs || mldv1 || mldv2), action=(next;)
>> +])
>> +
>> +AS_BOX([Chassis upgrades and supports CT related])
>> +check ovn-sbctl set chassis hv other_config:ct-no-masked-label=true
>> +check ovn-sbctl set chassis hv other_config:ovn-ct-lb-related=true
>> +check ovn-nbctl --wait=sb sync
>> +ovn-sbctl dump-flows | DUMP_FLOWS_SORTED > lflows2
>> +
>> +AT_CHECK([grep -e "lr_in_defrag" -e "lr_in_dnat" lflows2], [0], [dnl
>> +  table=? (lr_in_defrag       ), priority=0    , match=(1), action=(next;)
>> +  table=? (lr_in_defrag       ), priority=100  , match=(ip && ip4.dst ==
>> 192.168.0.1), action=(reg0 = 192.168.0.1; ct_dnat;)
>> +  table=? (lr_in_defrag       ), priority=50   , match=(icmp || icmp6),
>> action=(ct_dnat;)
>> +  table=? (lr_in_dnat         ), priority=0    , match=(1), action=(next;)
>> +  table=? (lr_in_dnat         ), priority=110  , match=(ct.est && !ct.rel
>> && ip4 && reg0 == 192.168.0.1 && ct_mark.natted == 1), action=(next;)
>> +  table=? (lr_in_dnat         ), priority=110  , match=(ct.new && !ct.rel
>> && ip4 && reg0 == 192.168.0.1), action=(ct_lb_mark(backends=192.168.1.10);)
>> +  table=? (lr_in_dnat         ), priority=50   , match=(ct.rel && !ct.est
>> && !ct.new), action=(ct_commit_nat;)
>> +  table=? (lr_in_dnat         ), priority=70   , match=(ct.rel && !ct.est
>> && !ct.new && ct_mark.force_snat == 1), action=(flags.force_snat_for_lb =
>> 1; ct_commit_nat;)
>> +  table=? (lr_in_dnat         ), priority=70   , match=(ct.rel && !ct.est
>> && !ct.new && ct_mark.skip_snat == 1), action=(flags.skip_snat_for_lb = 1;
>> ct_commit_nat;)
>> +])
>> +
>> +AT_CHECK([grep -e "ls_in_acl" -e "ls_out_acl" lflows2 | grep
>> "priority=65532"], [0], [dnl
>> +  table=? (ls_in_acl          ), priority=65532, match=(!ct.est && ct.rel
>> && !ct.new && !ct.inv && ct_mark.blocked == 0), action=(ct_commit_nat;)
>> +  table=? (ls_in_acl          ), priority=65532, match=(ct.est && !ct.rel
>> && !ct.new && !ct.inv && ct.rpl && ct_mark.blocked == 0), action=(reg0[[9]]
>> = 0; reg0[[10]] = 0; next;)
>> +  table=? (ls_in_acl          ), priority=65532, match=(ct.inv || (ct.est
>> && ct.rpl && ct_mark.blocked == 1)), action=(drop;)
>> +  table=? (ls_in_acl          ), priority=65532, match=(nd || nd_ra ||
>> nd_rs || mldv1 || mldv2), action=(next;)
>> +  table=? (ls_out_acl         ), priority=65532, match=(!ct.est && ct.rel
>> && !ct.new && !ct.inv && ct_mark.blocked == 0), action=(ct_commit_nat;)
>> +  table=? (ls_out_acl         ), priority=65532, match=(ct.est && !ct.rel
>> && !ct.new && !ct.inv && ct.rpl && ct_mark.blocked == 0), action=(next;)
>> +  table=? (ls_out_acl         ), priority=65532, match=(ct.inv || (ct.est
>> && ct.rpl && ct_mark.blocked == 1)), action=(drop;)
>> +  table=? (ls_out_acl         ), priority=65532, match=(nd || nd_ra ||
>> nd_rs || mldv1 || mldv2), action=(next;)
>> +])
>> +
>> +AT_CLEANUP
>> +])
>> --
>> 2.39.0
>>
>>
> Just to make it clear, we want to backport the "ICMP related" series [0]
> down to 21.12. This is needed so we are backward compatible with older
> controllers on those branches.
> Also that being said, we are missing Reported-at: tag for this patch.
> 

Thanks for the explanation, Ales!  The code looks OK to me but I have
one complaint: the feature we're actually guarding is "ct_commit_nat".
It happens to be used for LB related traffic.

I think I'd change the feature name to OVN_FEATURE_CT_COMMIT_NAT or similar.

What do you think?

Thanks,
Dumitru

_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to