| Message ID | 20220202172647.3258-1-dceara@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v2] acl-log: Log the direction (logical pipeline) of the matching ACL. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
Acked-by: Mark Michelson <mmichels@redhat.com> On 2/2/22 12:26, Dumitru Ceara wrote: > It's useful to differentiate between ingress and egress pipelines in the > ACL logs. To achieve this we expand the "log()" logical action and pass > an optional direction. > > This behavior change is implemented in a backwards compatible way such > that it doesn't break existing deployments even in the case when > ovn-northd gets updated before ovn-controller. > > To achieve this, ovn-northd determines first if all chassis in the > cluster have been upgraded to a version that supports the new "log()" > action format. > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1992641 > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > --- > v2: Add the "direction" after the "verdict" and "severity" fields in the > ACL logs. > --- > NEWS | 2 + > controller/chassis.c | 7 +++ > include/ovn/actions.h | 1 + > include/ovn/features.h | 1 + > lib/acl-log.c | 27 +++++++- > lib/acl-log.h | 37 ++++++++++- > lib/actions.c | 18 +++++- > northd/en-lflow.c | 1 + > northd/northd.c | 138 +++++++++++++++++++++++++---------------- > northd/northd.h | 7 +++ > ovn-sb.xml | 6 ++ > tests/ovn.at | 68 ++++++++++++++++---- > utilities/ovn-trace.c | 4 +- > 13 files changed, 246 insertions(+), 71 deletions(-) > > diff --git a/NEWS b/NEWS > index 194557410..a01820dfe 100644 > --- a/NEWS > +++ b/NEWS > @@ -1,5 +1,7 @@ > Post v21.12.0 > ------------- > + - When configured to log packtes matching ACLs, log the direction (logical > + pipeline) too. > > OVN v21.12.0 - 22 Dec 2021 > -------------------------- > diff --git a/controller/chassis.c b/controller/chassis.c > index 8a1559653..e77ae0b0c 100644 > --- a/controller/chassis.c > +++ b/controller/chassis.c > @@ -350,6 +350,7 @@ chassis_build_other_config(const struct ovs_chassis_cfg *ovs_cfg, > smap_replace(config, "is-interconn", > ovs_cfg->is_interconn ? "true" : "false"); > smap_replace(config, OVN_FEATURE_PORT_UP_NOTIF, "true"); > + smap_replace(config, OVN_FEATURE_ACL_LOG_DIRECTION, "true"); > } > > /* > @@ -455,6 +456,12 @@ chassis_other_config_changed(const struct ovs_chassis_cfg *ovs_cfg, > return true; > } > > + if (!smap_get_bool(&chassis_rec->other_config, > + OVN_FEATURE_ACL_LOG_DIRECTION, > + false)) { > + return true; > + } > + > return false; > } > > diff --git a/include/ovn/actions.h b/include/ovn/actions.h > index cdef5fb03..57222c78a 100644 > --- a/include/ovn/actions.h > +++ b/include/ovn/actions.h > @@ -370,6 +370,7 @@ struct ovnact_result { > /* OVNACT_LOG. */ > struct ovnact_log { > struct ovnact ovnact; > + uint8_t direction; /* One of LOG_DIRECTION_*. */ > uint8_t verdict; /* One of LOG_VERDICT_*. */ > uint8_t severity; /* One of LOG_SEVERITY_*. */ > char *name; > diff --git a/include/ovn/features.h b/include/ovn/features.h > index d12a8eb0d..e69e78e0a 100644 > --- a/include/ovn/features.h > +++ b/include/ovn/features.h > @@ -22,6 +22,7 @@ > > /* ovn-controller supported feature names. */ > #define OVN_FEATURE_PORT_UP_NOTIF "port-up-notif" > +#define OVN_FEATURE_ACL_LOG_DIRECTION "acl-log-dir" > > /* OVS datapath supported features. Based on availability OVN might generate > * different types of openflows. > diff --git a/lib/acl-log.c b/lib/acl-log.c > index 220b6dc30..fb981814f 100644 > --- a/lib/acl-log.c > +++ b/lib/acl-log.c > @@ -39,6 +39,21 @@ log_verdict_to_string(uint8_t verdict) > } > } > > +const char * > +log_direction_to_string(uint8_t direction) > +{ > + switch (direction) { > + case LOG_DIRECTION_NONE: > + return "none"; > + case LOG_DIRECTION_IN: > + return "IN"; > + case LOG_DIRECTION_OUT: > + return "OUT"; > + default: > + return "<unknown>"; > + } > +} > + > const char * > log_severity_to_string(uint8_t severity) > { > @@ -88,15 +103,23 @@ handle_acl_log(const struct flow *headers, struct ofpbuf *userdata) > return; > } > > + uint8_t direction = LOG_DIRECTION(lph->direction_verdict); > + uint8_t verdict = LOG_VERDICT(lph->direction_verdict); > + > size_t name_len = userdata->size; > char *name = name_len ? xmemdup0(userdata->data, name_len) : NULL; > > struct ds ds = DS_EMPTY_INITIALIZER; > ds_put_cstr(&ds, "name="); > json_string_escape(name_len ? name : "<unnamed>", &ds); > - ds_put_format(&ds, ", verdict=%s, severity=%s: ", > - log_verdict_to_string(lph->verdict), > + ds_put_format(&ds, ", verdict=%s, severity=%s", > + log_verdict_to_string(verdict), > log_severity_to_string(lph->severity)); > + if (direction != LOG_DIRECTION_NONE) { > + ds_put_format(&ds, ", direction=%s", > + log_direction_to_string(direction)); > + } > + ds_put_cstr(&ds, ": "); > flow_format(&ds, headers, NULL); > > VLOG_INFO("%s", ds_cstr(&ds)); > diff --git a/lib/acl-log.h b/lib/acl-log.h > index 4f23f790d..acc920856 100644 > --- a/lib/acl-log.h > +++ b/lib/acl-log.h > @@ -18,25 +18,51 @@ > #define ACL_LOG_H 1 > > #include <stdint.h> > +#include "compiler.h" > #include "openvswitch/types.h" > > struct ofpbuf; > struct flow; > > struct log_pin_header { > - uint8_t verdict; /* One of LOG_VERDICT_*. */ > + uint8_t direction_verdict; /* 4 bits for LOG_DIRECTION_* and > + * 4 bits for LOG_VERDICT_*. */ > uint8_t severity; /* One of LOG_SEVERITY*. */ > /* Followed by an optional string containing the rule's name. */ > }; > > +enum log_direction { > + LOG_DIRECTION_NONE, > + LOG_DIRECTION_IN, > + LOG_DIRECTION_OUT, > + LOG_DIRECTION_MAX, > +}; > + > enum log_verdict { > LOG_VERDICT_ALLOW, > LOG_VERDICT_DROP, > LOG_VERDICT_REJECT, > + LOG_VERDICT_MAX, > LOG_VERDICT_UNKNOWN = UINT8_MAX > }; > > +/* For backwards compatibility, use the least significant 4 bits for > + * verdict values and the most significant 4 bits for direction values. > + * > + * This is backwards compatible; old encodings will be decoded as: > + * - direction: NONE > + * - verdict: VERDICT > + */ > +#define LOG_VERDICT_BITS 4 > +#define LOG_DIRECTION_BITS 4 > +#define LOG_VERDICT_MASK ((1 << LOG_VERDICT_BITS) - 1) > +#define LOG_DIRECTION_MASK (0xFF ^ LOG_VERDICT_MASK) > + > +BUILD_ASSERT_DECL(LOG_VERDICT_MAX <= (1 << LOG_VERDICT_BITS)); > +BUILD_ASSERT_DECL(LOG_DIRECTION_MAX <= (1 << LOG_DIRECTION_BITS)); > + > const char *log_verdict_to_string(uint8_t verdict); > +const char *log_direction_to_string(uint8_t direction); > > > /* Severity levels. Based on RFC5424 levels. */ > @@ -46,6 +72,15 @@ const char *log_verdict_to_string(uint8_t verdict); > #define LOG_SEVERITY_INFO 6 > #define LOG_SEVERITY_DEBUG 7 > > +#define LOG_DIRECTION_VERDICT(DIR, VERDICT) \ > + ((DIR) << LOG_VERDICT_BITS | (VERDICT)) > + > +#define LOG_DIRECTION(DIR_VERDICT) \ > + (((DIR_VERDICT) & LOG_DIRECTION_MASK) >> LOG_VERDICT_BITS) > + > +#define LOG_VERDICT(DIR_VERDICT) \ > + ((DIR_VERDICT) & LOG_VERDICT_MASK) > + > const char *log_severity_to_string(uint8_t severity); > uint8_t log_severity_from_string(const char *name); > > diff --git a/lib/actions.c b/lib/actions.c > index d5d8391bb..4f37a1cb1 100644 > --- a/lib/actions.c > +++ b/lib/actions.c > @@ -3142,7 +3142,19 @@ encode_PUT_ND_RA_OPTS(const struct ovnact_put_opts *po, > static void > parse_log_arg(struct action_context *ctx, struct ovnact_log *log) > { > - if (lexer_match_id(ctx->lexer, "verdict")) { > + if (lexer_match_id(ctx->lexer, "direction")) { > + if (!lexer_force_match(ctx->lexer, LEX_T_EQUALS)) { > + return; > + } > + if (lexer_match_id(ctx->lexer, "IN")) { > + log->direction = LOG_DIRECTION_IN; > + } else if (lexer_match_id(ctx->lexer, "OUT")) { > + log->direction = LOG_DIRECTION_OUT; > + } else { > + lexer_syntax_error(ctx->lexer, "unknown direction"); > + return; > + } > + } else if (lexer_match_id(ctx->lexer, "verdict")) { > if (!lexer_force_match(ctx->lexer, LEX_T_EQUALS)) { > return; > } > @@ -3216,6 +3228,7 @@ parse_LOG(struct action_context *ctx) > struct ovnact_log *log = ovnact_put_LOG(ctx->ovnacts); > > /* Provide default values. */ > + log->direction = LOG_DIRECTION_NONE; > log->severity = LOG_SEVERITY_INFO; > log->verdict = LOG_VERDICT_UNKNOWN; > > @@ -3271,7 +3284,8 @@ encode_LOG(const struct ovnact_log *log, > meter_id, ofpacts); > > struct log_pin_header *lph = ofpbuf_put_uninit(ofpacts, sizeof *lph); > - lph->verdict = log->verdict; > + lph->direction_verdict = LOG_DIRECTION_VERDICT(log->direction, > + log->verdict); > lph->severity = log->severity; > > if (log->name) { > diff --git a/northd/en-lflow.c b/northd/en-lflow.c > index ffbdaf4e8..c91d6468d 100644 > --- a/northd/en-lflow.c > +++ b/northd/en-lflow.c > @@ -60,6 +60,7 @@ void en_lflow_run(struct engine_node *node, void *data OVS_UNUSED) > lflow_input.meter_groups = &northd_data->meter_groups; > lflow_input.lbs = &northd_data->lbs; > lflow_input.bfd_connections = &northd_data->bfd_connections; > + lflow_input.chassis_info = &northd_data->chassis_info; > lflow_input.ovn_internal_version_changed = > northd_data->ovn_internal_version_changed; > > diff --git a/northd/northd.c b/northd/northd.c > index fc7a64f99..8016ed10f 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -1383,24 +1383,33 @@ join_datapaths(struct northd_input *input_data, > } > } > > -static bool > -is_vxlan_mode(struct northd_input *input_data) > +static void > +chassis_data_init(struct northd_input *input_data, struct chassis_data *data) > { > + data->vxlan_mode = false; > + data->log_acl_direction = true; > + > const struct sbrec_chassis *chassis; > SBREC_CHASSIS_TABLE_FOR_EACH (chassis, input_data->sbrec_chassis) { > - for (int i = 0; i < chassis->n_encaps; i++) { > + if (data->log_acl_direction > + && !smap_get_bool(&chassis->other_config, > + OVN_FEATURE_ACL_LOG_DIRECTION, > + false)) { > + data->log_acl_direction = false; > + } > + for (int i = 0; !data->vxlan_mode && i < chassis->n_encaps; i++) { > if (!strcmp(chassis->encaps[i]->type, "vxlan")) { > - return true; > + data->vxlan_mode = true; > + break; > } > } > } > - return false; > } > > static uint32_t > -get_ovn_max_dp_key_local(struct northd_input *input_data) > +get_ovn_max_dp_key_local(struct chassis_data *chassis_info) > { > - if (is_vxlan_mode(input_data)) { > + if (chassis_info->vxlan_mode) { > /* OVN_MAX_DP_GLOBAL_NUM doesn't apply for vxlan mode. */ > return OVN_MAX_DP_VXLAN_KEY; > } > @@ -1408,14 +1417,14 @@ get_ovn_max_dp_key_local(struct northd_input *input_data) > } > > static void > -ovn_datapath_allocate_key(struct northd_input *input_data, > +ovn_datapath_allocate_key(struct chassis_data *chassis_info, > struct hmap *datapaths, struct hmap *dp_tnlids, > struct ovn_datapath *od, uint32_t *hint) > { > if (!od->tunnel_key) { > od->tunnel_key = ovn_allocate_tnlid(dp_tnlids, "datapath", > OVN_MIN_DP_KEY_LOCAL, > - get_ovn_max_dp_key_local(input_data), > + get_ovn_max_dp_key_local(chassis_info), > hint); > if (!od->tunnel_key) { > if (od->sb) { > @@ -1428,7 +1437,7 @@ ovn_datapath_allocate_key(struct northd_input *input_data, > } > > static void > -ovn_datapath_assign_requested_tnl_id(struct northd_input *input_data, > +ovn_datapath_assign_requested_tnl_id(struct chassis_data *chassis_info, > struct hmap *dp_tnlids, > struct ovn_datapath *od) > { > @@ -1438,7 +1447,7 @@ ovn_datapath_assign_requested_tnl_id(struct northd_input *input_data, > uint32_t tunnel_key = smap_get_int(other_config, "requested-tnl-key", 0); > if (tunnel_key) { > const char *interconn_ts = smap_get(other_config, "interconn-ts"); > - if (!interconn_ts && is_vxlan_mode(input_data) && > + if (!interconn_ts && chassis_info->vxlan_mode && > tunnel_key >= 1 << 12) { > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > VLOG_WARN_RL(&rl, "Tunnel key %"PRIu32" for datapath %s is " > @@ -1465,6 +1474,7 @@ ovn_datapath_assign_requested_tnl_id(struct northd_input *input_data, > * switch and router. */ > static void > build_datapaths(struct northd_input *input_data, > + struct chassis_data *chassis_info, > struct ovsdb_idl_txn *ovnsb_txn, > struct hmap *datapaths, > struct ovs_list *lr_list) > @@ -1478,10 +1488,10 @@ build_datapaths(struct northd_input *input_data, > struct hmap dp_tnlids = HMAP_INITIALIZER(&dp_tnlids); > struct ovn_datapath *od, *next; > LIST_FOR_EACH (od, list, &both) { > - ovn_datapath_assign_requested_tnl_id(input_data, &dp_tnlids, od); > + ovn_datapath_assign_requested_tnl_id(chassis_info, &dp_tnlids, od); > } > LIST_FOR_EACH (od, list, &nb_only) { > - ovn_datapath_assign_requested_tnl_id(input_data, &dp_tnlids, od); > + ovn_datapath_assign_requested_tnl_id(chassis_info, &dp_tnlids, od); > } > > /* Keep nonconflicting tunnel IDs that are already assigned. */ > @@ -1494,11 +1504,11 @@ build_datapaths(struct northd_input *input_data, > /* Assign new tunnel ids where needed. */ > uint32_t hint = 0; > LIST_FOR_EACH_SAFE (od, next, list, &both) { > - ovn_datapath_allocate_key(input_data, > + ovn_datapath_allocate_key(chassis_info, > datapaths, &dp_tnlids, od, &hint); > } > LIST_FOR_EACH_SAFE (od, next, list, &nb_only) { > - ovn_datapath_allocate_key(input_data, > + ovn_datapath_allocate_key(chassis_info, > datapaths, &dp_tnlids, od, &hint); > } > > @@ -4047,7 +4057,7 @@ ovn_port_add_tnlid(struct ovn_port *op, uint32_t tunnel_key) > } > > static void > -ovn_port_assign_requested_tnl_id(struct northd_input *input_data, > +ovn_port_assign_requested_tnl_id(struct chassis_data *chassis_info, > struct ovn_port *op) > { > const struct smap *options = (op->nbsp > @@ -4055,7 +4065,7 @@ ovn_port_assign_requested_tnl_id(struct northd_input *input_data, > : &op->nbrp->options); > uint32_t tunnel_key = smap_get_int(options, "requested-tnl-key", 0); > if (tunnel_key) { > - if (is_vxlan_mode(input_data) && > + if (chassis_info->vxlan_mode && > tunnel_key >= OVN_VXLAN_MIN_MULTICAST) { > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > VLOG_WARN_RL(&rl, "Tunnel key %"PRIu32" for port %s " > @@ -4074,12 +4084,11 @@ ovn_port_assign_requested_tnl_id(struct northd_input *input_data, > } > > static void > -ovn_port_allocate_key(struct northd_input *input_data, > - struct hmap *ports, > - struct ovn_port *op) > +ovn_port_allocate_key(struct chassis_data *chassis_info, > + struct hmap *ports, struct ovn_port *op) > { > if (!op->tunnel_key) { > - uint8_t key_bits = is_vxlan_mode(input_data)? 12 : 16; > + uint8_t key_bits = chassis_info->vxlan_mode ? 12 : 16; > op->tunnel_key = ovn_allocate_tnlid(&op->od->port_tnlids, "port", > 1, (1u << (key_bits - 1)) - 1, > &op->od->port_key_hint); > @@ -4101,6 +4110,7 @@ ovn_port_allocate_key(struct northd_input *input_data, > * datapaths. */ > static void > build_ports(struct northd_input *input_data, > + struct chassis_data *chassis_info, > struct ovsdb_idl_txn *ovnsb_txn, > struct ovsdb_idl_index *sbrec_chassis_by_name, > struct ovsdb_idl_index *sbrec_chassis_by_hostname, > @@ -4124,10 +4134,10 @@ build_ports(struct northd_input *input_data, > /* Assign explicitly requested tunnel ids first. */ > struct ovn_port *op, *next; > LIST_FOR_EACH (op, list, &both) { > - ovn_port_assign_requested_tnl_id(input_data, op); > + ovn_port_assign_requested_tnl_id(chassis_info, op); > } > LIST_FOR_EACH (op, list, &nb_only) { > - ovn_port_assign_requested_tnl_id(input_data, op); > + ovn_port_assign_requested_tnl_id(chassis_info, op); > } > > /* Keep nonconflicting tunnel IDs that are already assigned. */ > @@ -4139,10 +4149,10 @@ build_ports(struct northd_input *input_data, > > /* Assign new tunnel ids where needed. */ > LIST_FOR_EACH_SAFE (op, next, list, &both) { > - ovn_port_allocate_key(input_data, ports, op); > + ovn_port_allocate_key(chassis_info, ports, op); > } > LIST_FOR_EACH_SAFE (op, next, list, &nb_only) { > - ovn_port_allocate_key(input_data, ports, op); > + ovn_port_allocate_key(chassis_info, ports, op); > } > > /* For logical ports that are in both databases, update the southbound > @@ -6093,6 +6103,7 @@ build_acl_log_meter(struct ds *actions, const struct nbrec_acl *acl, > > static void > build_acl_log(struct ds *actions, const struct nbrec_acl *acl, > + enum ovn_stage stage, const struct chassis_data *chassis_info, > const struct shash *meter_groups) > { > if (!acl->log) { > @@ -6100,6 +6111,10 @@ build_acl_log(struct ds *actions, const struct nbrec_acl *acl, > } > > ds_put_cstr(actions, "log("); > + if (chassis_info->log_acl_direction) { > + ds_put_format(actions, "direction=%s, ", > + stage == S_SWITCH_IN_ACL ? "IN" : "OUT"); > + } > > if (acl->name) { > ds_put_format(actions, "name=\"%s\", ", acl->name); > @@ -6134,6 +6149,7 @@ build_reject_acl_rules(struct ovn_datapath *od, struct hmap *lflows, > enum ovn_stage stage, struct nbrec_acl *acl, > struct ds *extra_match, struct ds *extra_actions, > const struct ovsdb_idl_row *stage_hint, > + const struct chassis_data *chassis_info, > const struct shash *meter_groups) > { > struct ds match = DS_EMPTY_INITIALIZER; > @@ -6146,7 +6162,7 @@ build_reject_acl_rules(struct ovn_datapath *od, struct hmap *lflows, > ingress ? ovn_stage_get_table(S_SWITCH_OUT_QOS_MARK) > : ovn_stage_get_table(S_SWITCH_IN_L2_LKUP)); > > - build_acl_log(&actions, acl, meter_groups); > + build_acl_log(&actions, acl, stage, chassis_info, meter_groups); > if (extra_match->length > 0) { > ds_put_format(&match, "(%s) && ", extra_match->string); > } > @@ -6175,6 +6191,7 @@ build_reject_acl_rules(struct ovn_datapath *od, struct hmap *lflows, > static void > consider_acl(struct hmap *lflows, struct ovn_datapath *od, > struct nbrec_acl *acl, bool has_stateful, > + const struct chassis_data *chassis_info, > const struct shash *meter_groups, struct ds *match, > struct ds *actions) > { > @@ -6183,7 +6200,7 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od, > > if (!strcmp(acl->action, "allow-stateless")) { > ds_clear(actions); > - build_acl_log(actions, acl, meter_groups); > + build_acl_log(actions, acl, stage, chassis_info, meter_groups); > ds_put_cstr(actions, "next;"); > ovn_lflow_add_with_hint(lflows, od, stage, > acl->priority + OVN_ACL_PRI_OFFSET, > @@ -6198,7 +6215,7 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od, > * associated conntrack entry and would return "+invalid". */ > if (!has_stateful) { > ds_clear(actions); > - build_acl_log(actions, acl, meter_groups); > + build_acl_log(actions, acl, stage, chassis_info, meter_groups); > ds_put_cstr(actions, "next;"); > ovn_lflow_add_with_hint(lflows, od, stage, > acl->priority + OVN_ACL_PRI_OFFSET, > @@ -6227,7 +6244,7 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od, > ds_put_format(actions, REGBIT_ACL_LABEL" = 1; " > REG_LABEL" = %"PRId64"; ", acl->label); > } > - build_acl_log(actions, acl, meter_groups); > + build_acl_log(actions, acl, stage, chassis_info, meter_groups); > ds_put_cstr(actions, "next;"); > ovn_lflow_add_with_hint(lflows, od, stage, > acl->priority + OVN_ACL_PRI_OFFSET, > @@ -6252,7 +6269,7 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od, > ds_put_format(actions, REGBIT_ACL_LABEL" = 1; " > REG_LABEL" = %"PRId64"; ", acl->label); > } > - build_acl_log(actions, acl, meter_groups); > + build_acl_log(actions, acl, stage, chassis_info, meter_groups); > ds_put_cstr(actions, "next;"); > ovn_lflow_add_with_hint(lflows, od, stage, > acl->priority + OVN_ACL_PRI_OFFSET, > @@ -6272,11 +6289,12 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od, > ds_clear(actions); > ds_put_cstr(match, REGBIT_ACL_HINT_DROP " == 1"); > if (!strcmp(acl->action, "reject")) { > - build_reject_acl_rules(od, lflows, stage, acl, match, > - actions, &acl->header_, meter_groups); > + build_reject_acl_rules(od, lflows, stage, acl, match, actions, > + &acl->header_, chassis_info, > + meter_groups); > } else { > ds_put_format(match, " && (%s)", acl->match); > - build_acl_log(actions, acl, meter_groups); > + build_acl_log(actions, acl, stage, chassis_info, meter_groups); > ds_put_cstr(actions, "/* drop */"); > ovn_lflow_add_with_hint(lflows, od, stage, > acl->priority + OVN_ACL_PRI_OFFSET, > @@ -6299,11 +6317,12 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od, > ds_put_cstr(match, REGBIT_ACL_HINT_BLOCK " == 1"); > ds_put_cstr(actions, "ct_commit { ct_label.blocked = 1; }; "); > if (!strcmp(acl->action, "reject")) { > - build_reject_acl_rules(od, lflows, stage, acl, match, > - actions, &acl->header_, meter_groups); > + build_reject_acl_rules(od, lflows, stage, acl, match, actions, > + &acl->header_, chassis_info, > + meter_groups); > } else { > ds_put_format(match, " && (%s)", acl->match); > - build_acl_log(actions, acl, meter_groups); > + build_acl_log(actions, acl, stage, chassis_info, meter_groups); > ds_put_cstr(actions, "/* drop */"); > ovn_lflow_add_with_hint(lflows, od, stage, > acl->priority + OVN_ACL_PRI_OFFSET, > @@ -6317,10 +6336,11 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od, > ds_clear(match); > ds_clear(actions); > if (!strcmp(acl->action, "reject")) { > - build_reject_acl_rules(od, lflows, stage, acl, match, > - actions, &acl->header_, meter_groups); > + build_reject_acl_rules(od, lflows, stage, acl, match, actions, > + &acl->header_, chassis_info, > + meter_groups); > } else { > - build_acl_log(actions, acl, meter_groups); > + build_acl_log(actions, acl, stage, chassis_info, meter_groups); > ds_put_cstr(actions, "/* drop */"); > ovn_lflow_add_with_hint(lflows, od, stage, > acl->priority + OVN_ACL_PRI_OFFSET, > @@ -6400,7 +6420,9 @@ build_port_group_lswitches(struct northd_input *input_data, > > static void > build_acls(struct ovn_datapath *od, struct hmap *lflows, > - const struct hmap *port_groups, const struct shash *meter_groups) > + const struct chassis_data *chassis_info, > + const struct hmap *port_groups, > + const struct shash *meter_groups) > { > bool has_stateful = od->has_stateful_acl || od->has_lb_vip; > struct ds match = DS_EMPTY_INITIALIZER; > @@ -6515,15 +6537,15 @@ build_acls(struct ovn_datapath *od, struct hmap *lflows, > /* Ingress or Egress ACL Table (Various priorities). */ > for (size_t i = 0; i < od->nbs->n_acls; i++) { > struct nbrec_acl *acl = od->nbs->acls[i]; > - consider_acl(lflows, od, acl, has_stateful, meter_groups, &match, > - &actions); > + consider_acl(lflows, od, acl, has_stateful, chassis_info, meter_groups, > + &match, &actions); > } > struct ovn_port_group *pg; > HMAP_FOR_EACH (pg, key_node, port_groups) { > if (ovn_port_group_ls_find(pg, &od->nbs->header_.uuid)) { > for (size_t i = 0; i < pg->nb_pg->n_acls; i++) { > consider_acl(lflows, od, pg->nb_pg->acls[i], has_stateful, > - meter_groups, &match, &actions); > + chassis_info, meter_groups, &match, &actions); > } > } > } > @@ -7527,6 +7549,7 @@ build_lswitch_flows(const struct hmap *datapaths, > * Ingress tables 3 through 10. Egress tables 0 through 7. */ > static void > build_lswitch_lflows_pre_acl_and_acl(struct ovn_datapath *od, > + const struct chassis_data *chassis_info, > const struct hmap *port_groups, > struct hmap *lflows, > const struct shash *meter_groups) > @@ -7538,7 +7561,7 @@ build_lswitch_lflows_pre_acl_and_acl(struct ovn_datapath *od, > build_pre_lb(od, lflows); > build_pre_stateful(od, lflows); > build_acl_hints(od, lflows); > - build_acls(od, lflows, port_groups, meter_groups); > + build_acls(od, lflows, chassis_info, port_groups, meter_groups); > build_qos(od, lflows); > build_stateful(od, lflows); > build_lb_hairpin(od, lflows); > @@ -13248,6 +13271,7 @@ build_lrouter_nat_defrag_and_lb(struct ovn_datapath *od, struct hmap *lflows, > > > struct lswitch_flow_build_info { > + const struct chassis_data *chassis_info; > const struct hmap *datapaths; > const struct hmap *ports; > const struct hmap *port_groups; > @@ -13275,8 +13299,9 @@ build_lswitch_and_lrouter_iterate_by_od(struct ovn_datapath *od, > struct lswitch_flow_build_info *lsi) > { > /* Build Logical Switch Flows. */ > - build_lswitch_lflows_pre_acl_and_acl(od, lsi->port_groups, lsi->lflows, > - lsi->meter_groups); > + build_lswitch_lflows_pre_acl_and_acl(od, lsi->chassis_info, > + lsi->port_groups, > + lsi->lflows, lsi->meter_groups); > > build_fwd_group_lflows(od, lsi->lflows); > build_lswitch_lflows_admission_control(od, lsi->lflows); > @@ -13512,7 +13537,8 @@ fix_flow_map_size(struct hmap *lflow_map, > } > > static void > -build_lswitch_and_lrouter_flows(const struct hmap *datapaths, > +build_lswitch_and_lrouter_flows(const struct chassis_data *chassis_info, > + const struct hmap *datapaths, > const struct hmap *ports, > const struct hmap *port_groups, > struct hmap *lflows, > @@ -13564,6 +13590,7 @@ build_lswitch_and_lrouter_flows(const struct hmap *datapaths, > lsiv[index].meter_groups = meter_groups; > lsiv[index].lbs = lbs; > lsiv[index].bfd_connections = bfd_connections; > + lsiv[index].chassis_info = chassis_info; > lsiv[index].svc_check_match = svc_check_match; > lsiv[index].thread_lflow_counter = 0; > ds_init(&lsiv[index].match); > @@ -13602,6 +13629,7 @@ build_lswitch_and_lrouter_flows(const struct hmap *datapaths, > .meter_groups = meter_groups, > .lbs = lbs, > .bfd_connections = bfd_connections, > + .chassis_info = chassis_info, > .svc_check_match = svc_check_match, > .match = DS_EMPTY_INITIALIZER, > .actions = DS_EMPTY_INITIALIZER, > @@ -13757,7 +13785,8 @@ void build_lflows(struct lflow_input *input_data, > use_parallel_build = false; > reset_parallel = true; > } > - build_lswitch_and_lrouter_flows(input_data->datapaths, input_data->ports, > + build_lswitch_and_lrouter_flows(input_data->chassis_info, > + input_data->datapaths, input_data->ports, > input_data->port_groups, &lflows, > &mcast_groups, &igmp_groups, > input_data->meter_groups, input_data->lbs, > @@ -14869,6 +14898,9 @@ ovnnb_db_run(struct northd_input *input_data, > } > stopwatch_start(BUILD_LFLOWS_CTX_STOPWATCH_NAME, time_msec()); > > + /* Update chassis-specific supported data. */ > + chassis_data_init(input_data, &data->chassis_info); > + > /* Sync ipsec configuration. > * Copy nb_cfg from northbound to southbound database. > * Also set up to update sb_cfg once our southbound transaction commits. */ > @@ -14912,7 +14944,8 @@ ovnnb_db_run(struct northd_input *input_data, > smap_replace(&options, "svc_monitor_mac", svc_monitor_mac); > } > > - char *max_tunid = xasprintf("%d", get_ovn_max_dp_key_local(input_data)); > + char *max_tunid = > + xasprintf("%d", get_ovn_max_dp_key_local(&data->chassis_info)); > smap_replace(&options, "max_tunid", max_tunid); > free(max_tunid); > > @@ -14948,11 +14981,12 @@ ovnnb_db_run(struct northd_input *input_data, > check_lsp_is_up = !smap_get_bool(&nb->options, > "ignore_lsp_down", true); > > - build_datapaths(input_data, ovnsb_txn, &data->datapaths, &data->lr_list); > + build_datapaths(input_data, &data->chassis_info, ovnsb_txn, > + &data->datapaths, &data->lr_list); > build_ovn_lbs(input_data, ovnsb_txn, &data->datapaths, &data->lbs); > build_lrouter_lbs(&data->datapaths, &data->lbs); > - build_ports(input_data, ovnsb_txn, sbrec_chassis_by_name, > - sbrec_chassis_by_hostname, > + build_ports(input_data, &data->chassis_info, ovnsb_txn, > + sbrec_chassis_by_name, sbrec_chassis_by_hostname, > &data->datapaths, &data->ports); > build_lrouter_lbs_reachable_ips(&data->datapaths, &data->lbs); > build_ovn_lr_lbs(&data->datapaths, &data->lbs); > diff --git a/northd/northd.h b/northd/northd.h > index ebcb40de7..9329dbf47 100644 > --- a/northd/northd.h > +++ b/northd/northd.h > @@ -53,8 +53,14 @@ struct northd_input { > struct ovsdb_idl_index *sbrec_ip_mcast_by_dp; > }; > > +struct chassis_data { > + bool vxlan_mode; > + bool log_acl_direction; > +}; > + > struct northd_data { > /* Global state for 'en-northd'. */ > + struct chassis_data chassis_info; > struct hmap datapaths; > struct hmap ports; > struct hmap port_groups; > @@ -78,6 +84,7 @@ struct lflow_input { > /* Indexes */ > struct ovsdb_idl_index *sbrec_mcast_group_by_name_dp; > > + const struct chassis_data *chassis_info; > const struct hmap *datapaths; > const struct hmap *ports; > const struct hmap *port_groups; > diff --git a/ovn-sb.xml b/ovn-sb.xml > index 9ddacdf09..31e677c2b 100644 > --- a/ovn-sb.xml > +++ b/ovn-sb.xml > @@ -2174,6 +2174,12 @@ > <code>debug</code>. If a severity is not provided, the default > is <code>info</code>. > </dd> > + <dt><code>direction=</code><var>value</var></dt> > + <dd> > + The direction (logical pipeline) in which packets matched the > + flow. The value must be one of: <code>IN</code> for ingress > + pipeline, or <code>OUT</code> for egress pipeline. > + </dd> > <dt><code>verdict=</code><var>value</var></dt> > <dd> > The verdict for packets matching the flow. The value must be one > diff --git a/tests/ovn.at b/tests/ovn.at > index 957eb7850..cfb9818ce 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -8965,33 +8965,59 @@ ovn-nbctl lsp-set-addresses lp2 $lp2_mac > ovn-nbctl --wait=sb sync > wait_for_ports_up > > -ovn-nbctl acl-add lsw0 to-lport 1000 'tcp.dst==80' drop > -ovn-nbctl --log --severity=alert --name=drop-flow acl-add lsw0 to-lport 1000 'tcp.dst==81' drop > +ovn-nbctl acl-add lsw0 from-lport 1000 'tcp.dst==80' drop > +ovn-nbctl --log --severity=alert --name=drop-flow acl-add lsw0 from-lport 1000 'tcp.dst==81' drop > + > +ovn-nbctl acl-add lsw0 to-lport 1000 'tcp.dst==180' drop > +ovn-nbctl --log --severity=alert --name=drop-flow acl-add lsw0 to-lport 1000 'tcp.dst==181' drop > + > +ovn-nbctl acl-add lsw0 from-lport 1000 'tcp.dst==82' allow > +ovn-nbctl --log --severity=info --name=allow-flow acl-add lsw0 from-lport 1000 'tcp.dst==83' allow > > ovn-nbctl acl-add lsw0 to-lport 1000 'tcp.dst==82' allow > ovn-nbctl --log --severity=info --name=allow-flow acl-add lsw0 to-lport 1000 'tcp.dst==83' allow > > +ovn-nbctl acl-add lsw0 from-lport 1000 'tcp.dst==84' allow-related > +ovn-nbctl --log acl-add lsw0 from-lport 1000 'tcp.dst==85' allow-related > + > ovn-nbctl acl-add lsw0 to-lport 1000 'tcp.dst==84' allow-related > ovn-nbctl --log acl-add lsw0 to-lport 1000 'tcp.dst==85' allow-related > > -ovn-nbctl acl-add lsw0 to-lport 1000 'tcp.dst==86' reject > -ovn-nbctl --wait=hv --log --severity=alert --name=reject-flow acl-add lsw0 to-lport 1000 'tcp.dst==87' reject > +ovn-nbctl acl-add lsw0 from-lport 1000 'tcp.dst==86' reject > +ovn-nbctl --log --severity=alert --name=reject-flow acl-add lsw0 from-lport 1000 'tcp.dst==87' reject > + > +ovn-nbctl acl-add lsw0 to-lport 1000 'tcp.dst==186' reject > +ovn-nbctl --log --severity=alert --name=reject-flow acl-add lsw0 to-lport 1000 'tcp.dst==187' reject > + > +ovn-nbctl --wait=hv sync > > ovn-sbctl dump-flows > sbflows > AT_CAPTURE_FILE([sbflows]) > > -# Send packet that should be dropped without logging. > +# Send packet that should be dropped without logging in the ingress pipeline. > packet="inport==\"lp1\" && eth.src==$lp1_mac && eth.dst==$lp2_mac && > ip4 && ip.ttl==64 && ip4.src==$lp1_ip && ip4.dst==$lp2_ip && > tcp && tcp.flags==2 && tcp.src==4360 && tcp.dst==80" > as hv ovs-appctl -t ovn-controller inject-pkt "$packet" > > -# Send packet that should be dropped with logging. > +# Send packet that should be dropped with logging in the ingress pipeline. > packet="inport==\"lp1\" && eth.src==$lp1_mac && eth.dst==$lp2_mac && > ip4 && ip.ttl==64 && ip4.src==$lp1_ip && ip4.dst==$lp2_ip && > tcp && tcp.flags==2 && tcp.src==4361 && tcp.dst==81" > as hv ovs-appctl -t ovn-controller inject-pkt "$packet" > > +# Send packet that should be dropped without logging in the eggress pipeline. > +packet="inport==\"lp1\" && eth.src==$lp1_mac && eth.dst==$lp2_mac && > + ip4 && ip.ttl==64 && ip4.src==$lp1_ip && ip4.dst==$lp2_ip && > + tcp && tcp.flags==2 && tcp.src==4360 && tcp.dst==180" > +as hv ovs-appctl -t ovn-controller inject-pkt "$packet" > + > +# Send packet that should be dropped with logging in the egress pipeline. > +packet="inport==\"lp1\" && eth.src==$lp1_mac && eth.dst==$lp2_mac && > + ip4 && ip.ttl==64 && ip4.src==$lp1_ip && ip4.dst==$lp2_ip && > + tcp && tcp.flags==2 && tcp.src==4361 && tcp.dst==181" > +as hv ovs-appctl -t ovn-controller inject-pkt "$packet" > + > # Send packet that should be allowed without logging. > packet="inport==\"lp1\" && eth.src==$lp1_mac && eth.dst==$lp2_mac && > ip4 && ip.ttl==64 && ip4.src==$lp1_ip && ip4.dst==$lp2_ip && > @@ -9016,25 +9042,41 @@ packet="inport==\"lp1\" && eth.src==$lp1_mac && eth.dst==$lp2_mac && > tcp && tcp.flags==2 && tcp.src==4365 && tcp.dst==85" > as hv ovs-appctl -t ovn-controller inject-pkt "$packet" > > -# Send packet that should be rejected without logging. > +# Send packet that should be rejected without logging in the ingress pipeline. > packet="inport==\"lp1\" && eth.src==$lp1_mac && eth.dst==$lp2_mac && > ip4 && ip.ttl==64 && ip4.src==$lp1_ip && ip4.dst==$lp2_ip && > tcp && tcp.flags==2 && tcp.src==4366 && tcp.dst==86" > as hv ovs-appctl -t ovn-controller inject-pkt "$packet" > > -# Send packet that should be rejected with logging. > +# Send packet that should be rejected with logging in the ingress pipeline. > packet="inport==\"lp1\" && eth.src==$lp1_mac && eth.dst==$lp2_mac && > ip4 && ip.ttl==64 && ip4.src==$lp1_ip && ip4.dst==$lp2_ip && > tcp && tcp.flags==2 && tcp.src==4367 && tcp.dst==87" > as hv ovs-appctl -t ovn-controller inject-pkt "$packet" > > -OVS_WAIT_UNTIL([ test 4 = $(grep -c 'acl_log' hv/ovn-controller.log) ]) > +# Send packet that should be rejected without logging in the egress pipeline. > +packet="inport==\"lp1\" && eth.src==$lp1_mac && eth.dst==$lp2_mac && > + ip4 && ip.ttl==64 && ip4.src==$lp1_ip && ip4.dst==$lp2_ip && > + tcp && tcp.flags==2 && tcp.src==4366 && tcp.dst==186" > +as hv ovs-appctl -t ovn-controller inject-pkt "$packet" > + > +# Send packet that should be rejected with logging in the egress pipeline. > +packet="inport==\"lp1\" && eth.src==$lp1_mac && eth.dst==$lp2_mac && > + ip4 && ip.ttl==64 && ip4.src==$lp1_ip && ip4.dst==$lp2_ip && > + tcp && tcp.flags==2 && tcp.src==4367 && tcp.dst==187" > +as hv ovs-appctl -t ovn-controller inject-pkt "$packet" > + > +OVS_WAIT_UNTIL([ test 8 = $(grep -c 'acl_log' hv/ovn-controller.log) ]) > > AT_CHECK([grep 'acl_log' hv/ovn-controller.log | sed 's/.*name=/name=/'], [0], [dnl > -name="drop-flow", verdict=drop, severity=alert: tcp,vlan_tci=0x0000,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02,nw_src=192.168.1.2,nw_dst=192.168.1.3,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=4361,tp_dst=81,tcp_flags=syn > -name="allow-flow", verdict=allow, severity=info: tcp,vlan_tci=0x0000,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02,nw_src=192.168.1.2,nw_dst=192.168.1.3,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=4363,tp_dst=83,tcp_flags=syn > -name="<unnamed>", verdict=allow, severity=info: tcp,vlan_tci=0x0000,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02,nw_src=192.168.1.2,nw_dst=192.168.1.3,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=4365,tp_dst=85,tcp_flags=syn > -name="reject-flow", verdict=reject, severity=alert: tcp,vlan_tci=0x0000,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02,nw_src=192.168.1.2,nw_dst=192.168.1.3,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=4367,tp_dst=87,tcp_flags=syn > +name="drop-flow", verdict=drop, severity=alert, direction=IN: tcp,vlan_tci=0x0000,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02,nw_src=192.168.1.2,nw_dst=192.168.1.3,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=4361,tp_dst=81,tcp_flags=syn > +name="drop-flow", verdict=drop, severity=alert, direction=OUT: tcp,vlan_tci=0x0000,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02,nw_src=192.168.1.2,nw_dst=192.168.1.3,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=4361,tp_dst=181,tcp_flags=syn > +name="allow-flow", verdict=allow, severity=info, direction=IN: tcp,vlan_tci=0x0000,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02,nw_src=192.168.1.2,nw_dst=192.168.1.3,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=4363,tp_dst=83,tcp_flags=syn > +name="allow-flow", verdict=allow, severity=info, direction=OUT: tcp,vlan_tci=0x0000,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02,nw_src=192.168.1.2,nw_dst=192.168.1.3,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=4363,tp_dst=83,tcp_flags=syn > +name="<unnamed>", verdict=allow, severity=info, direction=IN: tcp,vlan_tci=0x0000,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02,nw_src=192.168.1.2,nw_dst=192.168.1.3,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=4365,tp_dst=85,tcp_flags=syn > +name="<unnamed>", verdict=allow, severity=info, direction=OUT: tcp,vlan_tci=0x0000,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02,nw_src=192.168.1.2,nw_dst=192.168.1.3,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=4365,tp_dst=85,tcp_flags=syn > +name="reject-flow", verdict=reject, severity=alert, direction=IN: tcp,vlan_tci=0x0000,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02,nw_src=192.168.1.2,nw_dst=192.168.1.3,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=4367,tp_dst=87,tcp_flags=syn > +name="reject-flow", verdict=reject, severity=alert, direction=OUT: tcp,vlan_tci=0x0000,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02,nw_src=192.168.1.2,nw_dst=192.168.1.3,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=4367,tp_dst=187,tcp_flags=syn > ]) > > OVN_CLEANUP([hv]) > diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c > index 0795913d3..26c0eaa9c 100644 > --- a/utilities/ovn-trace.c > +++ b/utilities/ovn-trace.c > @@ -2461,8 +2461,10 @@ execute_log(const struct ovnact_log *log, struct flow *uflow, > { > char *packet_str = flow_to_string(uflow, NULL); > ovntrace_node_append(super, OVNTRACE_NODE_TRANSFORMATION, > - "LOG: ACL name=%s, verdict=%s, severity=%s, packet=\"%s\"", > + "LOG: ACL name=%s, direction=%s, verdict=%s, " > + "severity=%s, packet=\"%s\"", > log->name ? log->name : "<unnamed>", > + log_direction_to_string(log->direction), > log_verdict_to_string(log->verdict), > log_severity_to_string(log->severity), > packet_str); >
On Wed, Feb 2, 2022 at 5:17 PM Mark Michelson <mmichels@redhat.com> wrote: > > Acked-by: Mark Michelson <mmichels@redhat.com> > > On 2/2/22 12:26, Dumitru Ceara wrote: > > It's useful to differentiate between ingress and egress pipelines in the > > ACL logs. To achieve this we expand the "log()" logical action and pass > > an optional direction. > > > > This behavior change is implemented in a backwards compatible way such > > that it doesn't break existing deployments even in the case when > > ovn-northd gets updated before ovn-controller. > > > > To achieve this, ovn-northd determines first if all chassis in the > > cluster have been upgraded to a version that supports the new "log()" > > action format. > > > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1992641 > > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > > --- > > v2: Add the "direction" after the "verdict" and "severity" fields in the > > ACL logs. Hi Dumitru, Thanks for the patch. The patch LGTM. I've an alternate suggestion to log the direction. When the packet is received by ovn-controller, it can determine the open flow table id from the table_id field of 'struct ofputil_packet_in'. From the open flow table id, it should be simple enough to figure out if this is from ingress or egress direction. If we take this approach, we don't have to make any changes to the 'acl_log' OVN action. Let me know what you think. Thanks Numan Numan > > --- > > NEWS | 2 + > > controller/chassis.c | 7 +++ > > include/ovn/actions.h | 1 + > > include/ovn/features.h | 1 + > > lib/acl-log.c | 27 +++++++- > > lib/acl-log.h | 37 ++++++++++- > > lib/actions.c | 18 +++++- > > northd/en-lflow.c | 1 + > > northd/northd.c | 138 +++++++++++++++++++++++++---------------- > > northd/northd.h | 7 +++ > > ovn-sb.xml | 6 ++ > > tests/ovn.at | 68 ++++++++++++++++---- > > utilities/ovn-trace.c | 4 +- > > 13 files changed, 246 insertions(+), 71 deletions(-) > > > > diff --git a/NEWS b/NEWS > > index 194557410..a01820dfe 100644 > > --- a/NEWS > > +++ b/NEWS > > @@ -1,5 +1,7 @@ > > Post v21.12.0 > > ------------- > > + - When configured to log packtes matching ACLs, log the direction (logical > > + pipeline) too. > > > > OVN v21.12.0 - 22 Dec 2021 > > -------------------------- > > diff --git a/controller/chassis.c b/controller/chassis.c > > index 8a1559653..e77ae0b0c 100644 > > --- a/controller/chassis.c > > +++ b/controller/chassis.c > > @@ -350,6 +350,7 @@ chassis_build_other_config(const struct ovs_chassis_cfg *ovs_cfg, > > smap_replace(config, "is-interconn", > > ovs_cfg->is_interconn ? "true" : "false"); > > smap_replace(config, OVN_FEATURE_PORT_UP_NOTIF, "true"); > > + smap_replace(config, OVN_FEATURE_ACL_LOG_DIRECTION, "true"); > > } > > > > /* > > @@ -455,6 +456,12 @@ chassis_other_config_changed(const struct ovs_chassis_cfg *ovs_cfg, > > return true; > > } > > > > + if (!smap_get_bool(&chassis_rec->other_config, > > + OVN_FEATURE_ACL_LOG_DIRECTION, > > + false)) { > > + return true; > > + } > > + > > return false; > > } > > > > diff --git a/include/ovn/actions.h b/include/ovn/actions.h > > index cdef5fb03..57222c78a 100644 > > --- a/include/ovn/actions.h > > +++ b/include/ovn/actions.h > > @@ -370,6 +370,7 @@ struct ovnact_result { > > /* OVNACT_LOG. */ > > struct ovnact_log { > > struct ovnact ovnact; > > + uint8_t direction; /* One of LOG_DIRECTION_*. */ > > uint8_t verdict; /* One of LOG_VERDICT_*. */ > > uint8_t severity; /* One of LOG_SEVERITY_*. */ > > char *name; > > diff --git a/include/ovn/features.h b/include/ovn/features.h > > index d12a8eb0d..e69e78e0a 100644 > > --- a/include/ovn/features.h > > +++ b/include/ovn/features.h > > @@ -22,6 +22,7 @@ > > > > /* ovn-controller supported feature names. */ > > #define OVN_FEATURE_PORT_UP_NOTIF "port-up-notif" > > +#define OVN_FEATURE_ACL_LOG_DIRECTION "acl-log-dir" > > > > /* OVS datapath supported features. Based on availability OVN might generate > > * different types of openflows. > > diff --git a/lib/acl-log.c b/lib/acl-log.c > > index 220b6dc30..fb981814f 100644 > > --- a/lib/acl-log.c > > +++ b/lib/acl-log.c > > @@ -39,6 +39,21 @@ log_verdict_to_string(uint8_t verdict) > > } > > } > > > > +const char * > > +log_direction_to_string(uint8_t direction) > > +{ > > + switch (direction) { > > + case LOG_DIRECTION_NONE: > > + return "none"; > > + case LOG_DIRECTION_IN: > > + return "IN"; > > + case LOG_DIRECTION_OUT: > > + return "OUT"; > > + default: > > + return "<unknown>"; > > + } > > +} > > + > > const char * > > log_severity_to_string(uint8_t severity) > > { > > @@ -88,15 +103,23 @@ handle_acl_log(const struct flow *headers, struct ofpbuf *userdata) > > return; > > } > > > > + uint8_t direction = LOG_DIRECTION(lph->direction_verdict); > > + uint8_t verdict = LOG_VERDICT(lph->direction_verdict); > > + > > size_t name_len = userdata->size; > > char *name = name_len ? xmemdup0(userdata->data, name_len) : NULL; > > > > struct ds ds = DS_EMPTY_INITIALIZER; > > ds_put_cstr(&ds, "name="); > > json_string_escape(name_len ? name : "<unnamed>", &ds); > > - ds_put_format(&ds, ", verdict=%s, severity=%s: ", > > - log_verdict_to_string(lph->verdict), > > + ds_put_format(&ds, ", verdict=%s, severity=%s", > > + log_verdict_to_string(verdict), > > log_severity_to_string(lph->severity)); > > + if (direction != LOG_DIRECTION_NONE) { > > + ds_put_format(&ds, ", direction=%s", > > + log_direction_to_string(direction)); > > + } > > + ds_put_cstr(&ds, ": "); > > flow_format(&ds, headers, NULL); > > > > VLOG_INFO("%s", ds_cstr(&ds)); > > diff --git a/lib/acl-log.h b/lib/acl-log.h > > index 4f23f790d..acc920856 100644 > > --- a/lib/acl-log.h > > +++ b/lib/acl-log.h > > @@ -18,25 +18,51 @@ > > #define ACL_LOG_H 1 > > > > #include <stdint.h> > > +#include "compiler.h" > > #include "openvswitch/types.h" > > > > struct ofpbuf; > > struct flow; > > > > struct log_pin_header { > > - uint8_t verdict; /* One of LOG_VERDICT_*. */ > > + uint8_t direction_verdict; /* 4 bits for LOG_DIRECTION_* and > > + * 4 bits for LOG_VERDICT_*. */ > > uint8_t severity; /* One of LOG_SEVERITY*. */ > > /* Followed by an optional string containing the rule's name. */ > > }; > > > > +enum log_direction { > > + LOG_DIRECTION_NONE, > > + LOG_DIRECTION_IN, > > + LOG_DIRECTION_OUT, > > + LOG_DIRECTION_MAX, > > +}; > > + > > enum log_verdict { > > LOG_VERDICT_ALLOW, > > LOG_VERDICT_DROP, > > LOG_VERDICT_REJECT, > > + LOG_VERDICT_MAX, > > LOG_VERDICT_UNKNOWN = UINT8_MAX > > }; > > > > +/* For backwards compatibility, use the least significant 4 bits for > > + * verdict values and the most significant 4 bits for direction values. > > + * > > + * This is backwards compatible; old encodings will be decoded as: > > + * - direction: NONE > > + * - verdict: VERDICT > > + */ > > +#define LOG_VERDICT_BITS 4 > > +#define LOG_DIRECTION_BITS 4 > > +#define LOG_VERDICT_MASK ((1 << LOG_VERDICT_BITS) - 1) > > +#define LOG_DIRECTION_MASK (0xFF ^ LOG_VERDICT_MASK) > > + > > +BUILD_ASSERT_DECL(LOG_VERDICT_MAX <= (1 << LOG_VERDICT_BITS)); > > +BUILD_ASSERT_DECL(LOG_DIRECTION_MAX <= (1 << LOG_DIRECTION_BITS)); > > + > > const char *log_verdict_to_string(uint8_t verdict); > > +const char *log_direction_to_string(uint8_t direction); > > > > > > /* Severity levels. Based on RFC5424 levels. */ > > @@ -46,6 +72,15 @@ const char *log_verdict_to_string(uint8_t verdict); > > #define LOG_SEVERITY_INFO 6 > > #define LOG_SEVERITY_DEBUG 7 > > > > +#define LOG_DIRECTION_VERDICT(DIR, VERDICT) \ > > + ((DIR) << LOG_VERDICT_BITS | (VERDICT)) > > + > > +#define LOG_DIRECTION(DIR_VERDICT) \ > > + (((DIR_VERDICT) & LOG_DIRECTION_MASK) >> LOG_VERDICT_BITS) > > + > > +#define LOG_VERDICT(DIR_VERDICT) \ > > + ((DIR_VERDICT) & LOG_VERDICT_MASK) > > + > > const char *log_severity_to_string(uint8_t severity); > > uint8_t log_severity_from_string(const char *name); > > > > diff --git a/lib/actions.c b/lib/actions.c > > index d5d8391bb..4f37a1cb1 100644 > > --- a/lib/actions.c > > +++ b/lib/actions.c > > @@ -3142,7 +3142,19 @@ encode_PUT_ND_RA_OPTS(const struct ovnact_put_opts *po, > > static void > > parse_log_arg(struct action_context *ctx, struct ovnact_log *log) > > { > > - if (lexer_match_id(ctx->lexer, "verdict")) { > > + if (lexer_match_id(ctx->lexer, "direction")) { > > + if (!lexer_force_match(ctx->lexer, LEX_T_EQUALS)) { > > + return; > > + } > > + if (lexer_match_id(ctx->lexer, "IN")) { > > + log->direction = LOG_DIRECTION_IN; > > + } else if (lexer_match_id(ctx->lexer, "OUT")) { > > + log->direction = LOG_DIRECTION_OUT; > > + } else { > > + lexer_syntax_error(ctx->lexer, "unknown direction"); > > + return; > > + } > > + } else if (lexer_match_id(ctx->lexer, "verdict")) { > > if (!lexer_force_match(ctx->lexer, LEX_T_EQUALS)) { > > return; > > } > > @@ -3216,6 +3228,7 @@ parse_LOG(struct action_context *ctx) > > struct ovnact_log *log = ovnact_put_LOG(ctx->ovnacts); > > > > /* Provide default values. */ > > + log->direction = LOG_DIRECTION_NONE; > > log->severity = LOG_SEVERITY_INFO; > > log->verdict = LOG_VERDICT_UNKNOWN; > > > > @@ -3271,7 +3284,8 @@ encode_LOG(const struct ovnact_log *log, > > meter_id, ofpacts); > > > > struct log_pin_header *lph = ofpbuf_put_uninit(ofpacts, sizeof *lph); > > - lph->verdict = log->verdict; > > + lph->direction_verdict = LOG_DIRECTION_VERDICT(log->direction, > > + log->verdict); > > lph->severity = log->severity; > > > > if (log->name) { > > diff --git a/northd/en-lflow.c b/northd/en-lflow.c > > index ffbdaf4e8..c91d6468d 100644 > > --- a/northd/en-lflow.c > > +++ b/northd/en-lflow.c > > @@ -60,6 +60,7 @@ void en_lflow_run(struct engine_node *node, void *data OVS_UNUSED) > > lflow_input.meter_groups = &northd_data->meter_groups; > > lflow_input.lbs = &northd_data->lbs; > > lflow_input.bfd_connections = &northd_data->bfd_connections; > > + lflow_input.chassis_info = &northd_data->chassis_info; > > lflow_input.ovn_internal_version_changed = > > northd_data->ovn_internal_version_changed; > > > > diff --git a/northd/northd.c b/northd/northd.c > > index fc7a64f99..8016ed10f 100644 > > --- a/northd/northd.c > > +++ b/northd/northd.c > > @@ -1383,24 +1383,33 @@ join_datapaths(struct northd_input *input_data, > > } > > } > > > > -static bool > > -is_vxlan_mode(struct northd_input *input_data) > > +static void > > +chassis_data_init(struct northd_input *input_data, struct chassis_data *data) > > { > > + data->vxlan_mode = false; > > + data->log_acl_direction = true; > > + > > const struct sbrec_chassis *chassis; > > SBREC_CHASSIS_TABLE_FOR_EACH (chassis, input_data->sbrec_chassis) { > > - for (int i = 0; i < chassis->n_encaps; i++) { > > + if (data->log_acl_direction > > + && !smap_get_bool(&chassis->other_config, > > + OVN_FEATURE_ACL_LOG_DIRECTION, > > + false)) { > > + data->log_acl_direction = false; > > + } > > + for (int i = 0; !data->vxlan_mode && i < chassis->n_encaps; i++) { > > if (!strcmp(chassis->encaps[i]->type, "vxlan")) { > > - return true; > > + data->vxlan_mode = true; > > + break; > > } > > } > > } > > - return false; > > } > > > > static uint32_t > > -get_ovn_max_dp_key_local(struct northd_input *input_data) > > +get_ovn_max_dp_key_local(struct chassis_data *chassis_info) > > { > > - if (is_vxlan_mode(input_data)) { > > + if (chassis_info->vxlan_mode) { > > /* OVN_MAX_DP_GLOBAL_NUM doesn't apply for vxlan mode. */ > > return OVN_MAX_DP_VXLAN_KEY; > > } > > @@ -1408,14 +1417,14 @@ get_ovn_max_dp_key_local(struct northd_input *input_data) > > } > > > > static void > > -ovn_datapath_allocate_key(struct northd_input *input_data, > > +ovn_datapath_allocate_key(struct chassis_data *chassis_info, > > struct hmap *datapaths, struct hmap *dp_tnlids, > > struct ovn_datapath *od, uint32_t *hint) > > { > > if (!od->tunnel_key) { > > od->tunnel_key = ovn_allocate_tnlid(dp_tnlids, "datapath", > > OVN_MIN_DP_KEY_LOCAL, > > - get_ovn_max_dp_key_local(input_data), > > + get_ovn_max_dp_key_local(chassis_info), > > hint); > > if (!od->tunnel_key) { > > if (od->sb) { > > @@ -1428,7 +1437,7 @@ ovn_datapath_allocate_key(struct northd_input *input_data, > > } > > > > static void > > -ovn_datapath_assign_requested_tnl_id(struct northd_input *input_data, > > +ovn_datapath_assign_requested_tnl_id(struct chassis_data *chassis_info, > > struct hmap *dp_tnlids, > > struct ovn_datapath *od) > > { > > @@ -1438,7 +1447,7 @@ ovn_datapath_assign_requested_tnl_id(struct northd_input *input_data, > > uint32_t tunnel_key = smap_get_int(other_config, "requested-tnl-key", 0); > > if (tunnel_key) { > > const char *interconn_ts = smap_get(other_config, "interconn-ts"); > > - if (!interconn_ts && is_vxlan_mode(input_data) && > > + if (!interconn_ts && chassis_info->vxlan_mode && > > tunnel_key >= 1 << 12) { > > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > > VLOG_WARN_RL(&rl, "Tunnel key %"PRIu32" for datapath %s is " > > @@ -1465,6 +1474,7 @@ ovn_datapath_assign_requested_tnl_id(struct northd_input *input_data, > > * switch and router. */ > > static void > > build_datapaths(struct northd_input *input_data, > > + struct chassis_data *chassis_info, > > struct ovsdb_idl_txn *ovnsb_txn, > > struct hmap *datapaths, > > struct ovs_list *lr_list) > > @@ -1478,10 +1488,10 @@ build_datapaths(struct northd_input *input_data, > > struct hmap dp_tnlids = HMAP_INITIALIZER(&dp_tnlids); > > struct ovn_datapath *od, *next; > > LIST_FOR_EACH (od, list, &both) { > > - ovn_datapath_assign_requested_tnl_id(input_data, &dp_tnlids, od); > > + ovn_datapath_assign_requested_tnl_id(chassis_info, &dp_tnlids, od); > > } > > LIST_FOR_EACH (od, list, &nb_only) { > > - ovn_datapath_assign_requested_tnl_id(input_data, &dp_tnlids, od); > > + ovn_datapath_assign_requested_tnl_id(chassis_info, &dp_tnlids, od); > > } > > > > /* Keep nonconflicting tunnel IDs that are already assigned. */ > > @@ -1494,11 +1504,11 @@ build_datapaths(struct northd_input *input_data, > > /* Assign new tunnel ids where needed. */ > > uint32_t hint = 0; > > LIST_FOR_EACH_SAFE (od, next, list, &both) { > > - ovn_datapath_allocate_key(input_data, > > + ovn_datapath_allocate_key(chassis_info, > > datapaths, &dp_tnlids, od, &hint); > > } > > LIST_FOR_EACH_SAFE (od, next, list, &nb_only) { > > - ovn_datapath_allocate_key(input_data, > > + ovn_datapath_allocate_key(chassis_info, > > datapaths, &dp_tnlids, od, &hint); > > } > > > > @@ -4047,7 +4057,7 @@ ovn_port_add_tnlid(struct ovn_port *op, uint32_t tunnel_key) > > } > > > > static void > > -ovn_port_assign_requested_tnl_id(struct northd_input *input_data, > > +ovn_port_assign_requested_tnl_id(struct chassis_data *chassis_info, > > struct ovn_port *op) > > { > > const struct smap *options = (op->nbsp > > @@ -4055,7 +4065,7 @@ ovn_port_assign_requested_tnl_id(struct northd_input *input_data, > > : &op->nbrp->options); > > uint32_t tunnel_key = smap_get_int(options, "requested-tnl-key", 0); > > if (tunnel_key) { > > - if (is_vxlan_mode(input_data) && > > + if (chassis_info->vxlan_mode && > > tunnel_key >= OVN_VXLAN_MIN_MULTICAST) { > > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > > VLOG_WARN_RL(&rl, "Tunnel key %"PRIu32" for port %s " > > @@ -4074,12 +4084,11 @@ ovn_port_assign_requested_tnl_id(struct northd_input *input_data, > > } > > > > static void > > -ovn_port_allocate_key(struct northd_input *input_data, > > - struct hmap *ports, > > - struct ovn_port *op) > > +ovn_port_allocate_key(struct chassis_data *chassis_info, > > + struct hmap *ports, struct ovn_port *op) > > { > > if (!op->tunnel_key) { > > - uint8_t key_bits = is_vxlan_mode(input_data)? 12 : 16; > > + uint8_t key_bits = chassis_info->vxlan_mode ? 12 : 16; > > op->tunnel_key = ovn_allocate_tnlid(&op->od->port_tnlids, "port", > > 1, (1u << (key_bits - 1)) - 1, > > &op->od->port_key_hint); > > @@ -4101,6 +4110,7 @@ ovn_port_allocate_key(struct northd_input *input_data, > > * datapaths. */ > > static void > > build_ports(struct northd_input *input_data, > > + struct chassis_data *chassis_info, > > struct ovsdb_idl_txn *ovnsb_txn, > > struct ovsdb_idl_index *sbrec_chassis_by_name, > > struct ovsdb_idl_index *sbrec_chassis_by_hostname, > > @@ -4124,10 +4134,10 @@ build_ports(struct northd_input *input_data, > > /* Assign explicitly requested tunnel ids first. */ > > struct ovn_port *op, *next; > > LIST_FOR_EACH (op, list, &both) { > > - ovn_port_assign_requested_tnl_id(input_data, op); > > + ovn_port_assign_requested_tnl_id(chassis_info, op); > > } > > LIST_FOR_EACH (op, list, &nb_only) { > > - ovn_port_assign_requested_tnl_id(input_data, op); > > + ovn_port_assign_requested_tnl_id(chassis_info, op); > > } > > > > /* Keep nonconflicting tunnel IDs that are already assigned. */ > > @@ -4139,10 +4149,10 @@ build_ports(struct northd_input *input_data, > > > > /* Assign new tunnel ids where needed. */ > > LIST_FOR_EACH_SAFE (op, next, list, &both) { > > - ovn_port_allocate_key(input_data, ports, op); > > + ovn_port_allocate_key(chassis_info, ports, op); > > } > > LIST_FOR_EACH_SAFE (op, next, list, &nb_only) { > > - ovn_port_allocate_key(input_data, ports, op); > > + ovn_port_allocate_key(chassis_info, ports, op); > > } > > > > /* For logical ports that are in both databases, update the southbound > > @@ -6093,6 +6103,7 @@ build_acl_log_meter(struct ds *actions, const struct nbrec_acl *acl, > > > > static void > > build_acl_log(struct ds *actions, const struct nbrec_acl *acl, > > + enum ovn_stage stage, const struct chassis_data *chassis_info, > > const struct shash *meter_groups) > > { > > if (!acl->log) { > > @@ -6100,6 +6111,10 @@ build_acl_log(struct ds *actions, const struct nbrec_acl *acl, > > } > > > > ds_put_cstr(actions, "log("); > > + if (chassis_info->log_acl_direction) { > > + ds_put_format(actions, "direction=%s, ", > > + stage == S_SWITCH_IN_ACL ? "IN" : "OUT"); > > + } > > > > if (acl->name) { > > ds_put_format(actions, "name=\"%s\", ", acl->name); > > @@ -6134,6 +6149,7 @@ build_reject_acl_rules(struct ovn_datapath *od, struct hmap *lflows, > > enum ovn_stage stage, struct nbrec_acl *acl, > > struct ds *extra_match, struct ds *extra_actions, > > const struct ovsdb_idl_row *stage_hint, > > + const struct chassis_data *chassis_info, > > const struct shash *meter_groups) > > { > > struct ds match = DS_EMPTY_INITIALIZER; > > @@ -6146,7 +6162,7 @@ build_reject_acl_rules(struct ovn_datapath *od, struct hmap *lflows, > > ingress ? ovn_stage_get_table(S_SWITCH_OUT_QOS_MARK) > > : ovn_stage_get_table(S_SWITCH_IN_L2_LKUP)); > > > > - build_acl_log(&actions, acl, meter_groups); > > + build_acl_log(&actions, acl, stage, chassis_info, meter_groups); > > if (extra_match->length > 0) { > > ds_put_format(&match, "(%s) && ", extra_match->string); > > } > > @@ -6175,6 +6191,7 @@ build_reject_acl_rules(struct ovn_datapath *od, struct hmap *lflows, > > static void > > consider_acl(struct hmap *lflows, struct ovn_datapath *od, > > struct nbrec_acl *acl, bool has_stateful, > > + const struct chassis_data *chassis_info, > > const struct shash *meter_groups, struct ds *match, > > struct ds *actions) > > { > > @@ -6183,7 +6200,7 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od, > > > > if (!strcmp(acl->action, "allow-stateless")) { > > ds_clear(actions); > > - build_acl_log(actions, acl, meter_groups); > > + build_acl_log(actions, acl, stage, chassis_info, meter_groups); > > ds_put_cstr(actions, "next;"); > > ovn_lflow_add_with_hint(lflows, od, stage, > > acl->priority + OVN_ACL_PRI_OFFSET, > > @@ -6198,7 +6215,7 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od, > > * associated conntrack entry and would return "+invalid". */ > > if (!has_stateful) { > > ds_clear(actions); > > - build_acl_log(actions, acl, meter_groups); > > + build_acl_log(actions, acl, stage, chassis_info, meter_groups); > > ds_put_cstr(actions, "next;"); > > ovn_lflow_add_with_hint(lflows, od, stage, > > acl->priority + OVN_ACL_PRI_OFFSET, > > @@ -6227,7 +6244,7 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od, > > ds_put_format(actions, REGBIT_ACL_LABEL" = 1; " > > REG_LABEL" = %"PRId64"; ", acl->label); > > } > > - build_acl_log(actions, acl, meter_groups); > > + build_acl_log(actions, acl, stage, chassis_info, meter_groups); > > ds_put_cstr(actions, "next;"); > > ovn_lflow_add_with_hint(lflows, od, stage, > > acl->priority + OVN_ACL_PRI_OFFSET, > > @@ -6252,7 +6269,7 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od, > > ds_put_format(actions, REGBIT_ACL_LABEL" = 1; " > > REG_LABEL" = %"PRId64"; ", acl->label); > > } > > - build_acl_log(actions, acl, meter_groups); > > + build_acl_log(actions, acl, stage, chassis_info, meter_groups); > > ds_put_cstr(actions, "next;"); > > ovn_lflow_add_with_hint(lflows, od, stage, > > acl->priority + OVN_ACL_PRI_OFFSET, > > @@ -6272,11 +6289,12 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od, > > ds_clear(actions); > > ds_put_cstr(match, REGBIT_ACL_HINT_DROP " == 1"); > > if (!strcmp(acl->action, "reject")) { > > - build_reject_acl_rules(od, lflows, stage, acl, match, > > - actions, &acl->header_, meter_groups); > > + build_reject_acl_rules(od, lflows, stage, acl, match, actions, > > + &acl->header_, chassis_info, > > + meter_groups); > > } else { > > ds_put_format(match, " && (%s)", acl->match); > > - build_acl_log(actions, acl, meter_groups); > > + build_acl_log(actions, acl, stage, chassis_info, meter_groups); > > ds_put_cstr(actions, "/* drop */"); > > ovn_lflow_add_with_hint(lflows, od, stage, > > acl->priority + OVN_ACL_PRI_OFFSET, > > @@ -6299,11 +6317,12 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od, > > ds_put_cstr(match, REGBIT_ACL_HINT_BLOCK " == 1"); > > ds_put_cstr(actions, "ct_commit { ct_label.blocked = 1; }; "); > > if (!strcmp(acl->action, "reject")) { > > - build_reject_acl_rules(od, lflows, stage, acl, match, > > - actions, &acl->header_, meter_groups); > > + build_reject_acl_rules(od, lflows, stage, acl, match, actions, > > + &acl->header_, chassis_info, > > + meter_groups); > > } else { > > ds_put_format(match, " && (%s)", acl->match); > > - build_acl_log(actions, acl, meter_groups); > > + build_acl_log(actions, acl, stage, chassis_info, meter_groups); > > ds_put_cstr(actions, "/* drop */"); > > ovn_lflow_add_with_hint(lflows, od, stage, > > acl->priority + OVN_ACL_PRI_OFFSET, > > @@ -6317,10 +6336,11 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od, > > ds_clear(match); > > ds_clear(actions); > > if (!strcmp(acl->action, "reject")) { > > - build_reject_acl_rules(od, lflows, stage, acl, match, > > - actions, &acl->header_, meter_groups); > > + build_reject_acl_rules(od, lflows, stage, acl, match, actions, > > + &acl->header_, chassis_info, > > + meter_groups); > > } else { > > - build_acl_log(actions, acl, meter_groups); > > + build_acl_log(actions, acl, stage, chassis_info, meter_groups); > > ds_put_cstr(actions, "/* drop */"); > > ovn_lflow_add_with_hint(lflows, od, stage, > > acl->priority + OVN_ACL_PRI_OFFSET, > > @@ -6400,7 +6420,9 @@ build_port_group_lswitches(struct northd_input *input_data, > > > > static void > > build_acls(struct ovn_datapath *od, struct hmap *lflows, > > - const struct hmap *port_groups, const struct shash *meter_groups) > > + const struct chassis_data *chassis_info, > > + const struct hmap *port_groups, > > + const struct shash *meter_groups) > > { > > bool has_stateful = od->has_stateful_acl || od->has_lb_vip; > > struct ds match = DS_EMPTY_INITIALIZER; > > @@ -6515,15 +6537,15 @@ build_acls(struct ovn_datapath *od, struct hmap *lflows, > > /* Ingress or Egress ACL Table (Various priorities). */ > > for (size_t i = 0; i < od->nbs->n_acls; i++) { > > struct nbrec_acl *acl = od->nbs->acls[i]; > > - consider_acl(lflows, od, acl, has_stateful, meter_groups, &match, > > - &actions); > > + consider_acl(lflows, od, acl, has_stateful, chassis_info, meter_groups, > > + &match, &actions); > > } > > struct ovn_port_group *pg; > > HMAP_FOR_EACH (pg, key_node, port_groups) { > > if (ovn_port_group_ls_find(pg, &od->nbs->header_.uuid)) { > > for (size_t i = 0; i < pg->nb_pg->n_acls; i++) { > > consider_acl(lflows, od, pg->nb_pg->acls[i], has_stateful, > > - meter_groups, &match, &actions); > > + chassis_info, meter_groups, &match, &actions); > > } > > } > > } > > @@ -7527,6 +7549,7 @@ build_lswitch_flows(const struct hmap *datapaths, > > * Ingress tables 3 through 10. Egress tables 0 through 7. */ > > static void > > build_lswitch_lflows_pre_acl_and_acl(struct ovn_datapath *od, > > + const struct chassis_data *chassis_info, > > const struct hmap *port_groups, > > struct hmap *lflows, > > const struct shash *meter_groups) > > @@ -7538,7 +7561,7 @@ build_lswitch_lflows_pre_acl_and_acl(struct ovn_datapath *od, > > build_pre_lb(od, lflows); > > build_pre_stateful(od, lflows); > > build_acl_hints(od, lflows); > > - build_acls(od, lflows, port_groups, meter_groups); > > + build_acls(od, lflows, chassis_info, port_groups, meter_groups); > > build_qos(od, lflows); > > build_stateful(od, lflows); > > build_lb_hairpin(od, lflows); > > @@ -13248,6 +13271,7 @@ build_lrouter_nat_defrag_and_lb(struct ovn_datapath *od, struct hmap *lflows, > > > > > > struct lswitch_flow_build_info { > > + const struct chassis_data *chassis_info; > > const struct hmap *datapaths; > > const struct hmap *ports; > > const struct hmap *port_groups; > > @@ -13275,8 +13299,9 @@ build_lswitch_and_lrouter_iterate_by_od(struct ovn_datapath *od, > > struct lswitch_flow_build_info *lsi) > > { > > /* Build Logical Switch Flows. */ > > - build_lswitch_lflows_pre_acl_and_acl(od, lsi->port_groups, lsi->lflows, > > - lsi->meter_groups); > > + build_lswitch_lflows_pre_acl_and_acl(od, lsi->chassis_info, > > + lsi->port_groups, > > + lsi->lflows, lsi->meter_groups); > > > > build_fwd_group_lflows(od, lsi->lflows); > > build_lswitch_lflows_admission_control(od, lsi->lflows); > > @@ -13512,7 +13537,8 @@ fix_flow_map_size(struct hmap *lflow_map, > > } > > > > static void > > -build_lswitch_and_lrouter_flows(const struct hmap *datapaths, > > +build_lswitch_and_lrouter_flows(const struct chassis_data *chassis_info, > > + const struct hmap *datapaths, > > const struct hmap *ports, > > const struct hmap *port_groups, > > struct hmap *lflows, > > @@ -13564,6 +13590,7 @@ build_lswitch_and_lrouter_flows(const struct hmap *datapaths, > > lsiv[index].meter_groups = meter_groups; > > lsiv[index].lbs = lbs; > > lsiv[index].bfd_connections = bfd_connections; > > + lsiv[index].chassis_info = chassis_info; > > lsiv[index].svc_check_match = svc_check_match; > > lsiv[index].thread_lflow_counter = 0; > > ds_init(&lsiv[index].match); > > @@ -13602,6 +13629,7 @@ build_lswitch_and_lrouter_flows(const struct hmap *datapaths, > > .meter_groups = meter_groups, > > .lbs = lbs, > > .bfd_connections = bfd_connections, > > + .chassis_info = chassis_info, > > .svc_check_match = svc_check_match, > > .match = DS_EMPTY_INITIALIZER, > > .actions = DS_EMPTY_INITIALIZER, > > @@ -13757,7 +13785,8 @@ void build_lflows(struct lflow_input *input_data, > > use_parallel_build = false; > > reset_parallel = true; > > } > > - build_lswitch_and_lrouter_flows(input_data->datapaths, input_data->ports, > > + build_lswitch_and_lrouter_flows(input_data->chassis_info, > > + input_data->datapaths, input_data->ports, > > input_data->port_groups, &lflows, > > &mcast_groups, &igmp_groups, > > input_data->meter_groups, input_data->lbs, > > @@ -14869,6 +14898,9 @@ ovnnb_db_run(struct northd_input *input_data, > > } > > stopwatch_start(BUILD_LFLOWS_CTX_STOPWATCH_NAME, time_msec()); > > > > + /* Update chassis-specific supported data. */ > > + chassis_data_init(input_data, &data->chassis_info); > > + > > /* Sync ipsec configuration. > > * Copy nb_cfg from northbound to southbound database. > > * Also set up to update sb_cfg once our southbound transaction commits. */ > > @@ -14912,7 +14944,8 @@ ovnnb_db_run(struct northd_input *input_data, > > smap_replace(&options, "svc_monitor_mac", svc_monitor_mac); > > } > > > > - char *max_tunid = xasprintf("%d", get_ovn_max_dp_key_local(input_data)); > > + char *max_tunid = > > + xasprintf("%d", get_ovn_max_dp_key_local(&data->chassis_info)); > > smap_replace(&options, "max_tunid", max_tunid); > > free(max_tunid); > > > > @@ -14948,11 +14981,12 @@ ovnnb_db_run(struct northd_input *input_data, > > check_lsp_is_up = !smap_get_bool(&nb->options, > > "ignore_lsp_down", true); > > > > - build_datapaths(input_data, ovnsb_txn, &data->datapaths, &data->lr_list); > > + build_datapaths(input_data, &data->chassis_info, ovnsb_txn, > > + &data->datapaths, &data->lr_list); > > build_ovn_lbs(input_data, ovnsb_txn, &data->datapaths, &data->lbs); > > build_lrouter_lbs(&data->datapaths, &data->lbs); > > - build_ports(input_data, ovnsb_txn, sbrec_chassis_by_name, > > - sbrec_chassis_by_hostname, > > + build_ports(input_data, &data->chassis_info, ovnsb_txn, > > + sbrec_chassis_by_name, sbrec_chassis_by_hostname, > > &data->datapaths, &data->ports); > > build_lrouter_lbs_reachable_ips(&data->datapaths, &data->lbs); > > build_ovn_lr_lbs(&data->datapaths, &data->lbs); > > diff --git a/northd/northd.h b/northd/northd.h > > index ebcb40de7..9329dbf47 100644 > > --- a/northd/northd.h > > +++ b/northd/northd.h > > @@ -53,8 +53,14 @@ struct northd_input { > > struct ovsdb_idl_index *sbrec_ip_mcast_by_dp; > > }; > > > > +struct chassis_data { > > + bool vxlan_mode; > > + bool log_acl_direction; > > +}; > > + > > struct northd_data { > > /* Global state for 'en-northd'. */ > > + struct chassis_data chassis_info; > > struct hmap datapaths; > > struct hmap ports; > > struct hmap port_groups; > > @@ -78,6 +84,7 @@ struct lflow_input { > > /* Indexes */ > > struct ovsdb_idl_index *sbrec_mcast_group_by_name_dp; > > > > + const struct chassis_data *chassis_info; > > const struct hmap *datapaths; > > const struct hmap *ports; > > const struct hmap *port_groups; > > diff --git a/ovn-sb.xml b/ovn-sb.xml > > index 9ddacdf09..31e677c2b 100644 > > --- a/ovn-sb.xml > > +++ b/ovn-sb.xml > > @@ -2174,6 +2174,12 @@ > > <code>debug</code>. If a severity is not provided, the default > > is <code>info</code>. > > </dd> > > + <dt><code>direction=</code><var>value</var></dt> > > + <dd> > > + The direction (logical pipeline) in which packets matched the > > + flow. The value must be one of: <code>IN</code> for ingress > > + pipeline, or <code>OUT</code> for egress pipeline. > > + </dd> > > <dt><code>verdict=</code><var>value</var></dt> > > <dd> > > The verdict for packets matching the flow. The value must be one > > diff --git a/tests/ovn.at b/tests/ovn.at > > index 957eb7850..cfb9818ce 100644 > > --- a/tests/ovn.at > > +++ b/tests/ovn.at > > @@ -8965,33 +8965,59 @@ ovn-nbctl lsp-set-addresses lp2 $lp2_mac > > ovn-nbctl --wait=sb sync > > wait_for_ports_up > > > > -ovn-nbctl acl-add lsw0 to-lport 1000 'tcp.dst==80' drop > > -ovn-nbctl --log --severity=alert --name=drop-flow acl-add lsw0 to-lport 1000 'tcp.dst==81' drop > > +ovn-nbctl acl-add lsw0 from-lport 1000 'tcp.dst==80' drop > > +ovn-nbctl --log --severity=alert --name=drop-flow acl-add lsw0 from-lport 1000 'tcp.dst==81' drop > > + > > +ovn-nbctl acl-add lsw0 to-lport 1000 'tcp.dst==180' drop > > +ovn-nbctl --log --severity=alert --name=drop-flow acl-add lsw0 to-lport 1000 'tcp.dst==181' drop > > + > > +ovn-nbctl acl-add lsw0 from-lport 1000 'tcp.dst==82' allow > > +ovn-nbctl --log --severity=info --name=allow-flow acl-add lsw0 from-lport 1000 'tcp.dst==83' allow > > > > ovn-nbctl acl-add lsw0 to-lport 1000 'tcp.dst==82' allow > > ovn-nbctl --log --severity=info --name=allow-flow acl-add lsw0 to-lport 1000 'tcp.dst==83' allow > > > > +ovn-nbctl acl-add lsw0 from-lport 1000 'tcp.dst==84' allow-related > > +ovn-nbctl --log acl-add lsw0 from-lport 1000 'tcp.dst==85' allow-related > > + > > ovn-nbctl acl-add lsw0 to-lport 1000 'tcp.dst==84' allow-related > > ovn-nbctl --log acl-add lsw0 to-lport 1000 'tcp.dst==85' allow-related > > > > -ovn-nbctl acl-add lsw0 to-lport 1000 'tcp.dst==86' reject > > -ovn-nbctl --wait=hv --log --severity=alert --name=reject-flow acl-add lsw0 to-lport 1000 'tcp.dst==87' reject > > +ovn-nbctl acl-add lsw0 from-lport 1000 'tcp.dst==86' reject > > +ovn-nbctl --log --severity=alert --name=reject-flow acl-add lsw0 from-lport 1000 'tcp.dst==87' reject > > + > > +ovn-nbctl acl-add lsw0 to-lport 1000 'tcp.dst==186' reject > > +ovn-nbctl --log --severity=alert --name=reject-flow acl-add lsw0 to-lport 1000 'tcp.dst==187' reject > > + > > +ovn-nbctl --wait=hv sync > > > > ovn-sbctl dump-flows > sbflows > > AT_CAPTURE_FILE([sbflows]) > > > > -# Send packet that should be dropped without logging. > > +# Send packet that should be dropped without logging in the ingress pipeline. > > packet="inport==\"lp1\" && eth.src==$lp1_mac && eth.dst==$lp2_mac && > > ip4 && ip.ttl==64 && ip4.src==$lp1_ip && ip4.dst==$lp2_ip && > > tcp && tcp.flags==2 && tcp.src==4360 && tcp.dst==80" > > as hv ovs-appctl -t ovn-controller inject-pkt "$packet" > > > > -# Send packet that should be dropped with logging. > > +# Send packet that should be dropped with logging in the ingress pipeline. > > packet="inport==\"lp1\" && eth.src==$lp1_mac && eth.dst==$lp2_mac && > > ip4 && ip.ttl==64 && ip4.src==$lp1_ip && ip4.dst==$lp2_ip && > > tcp && tcp.flags==2 && tcp.src==4361 && tcp.dst==81" > > as hv ovs-appctl -t ovn-controller inject-pkt "$packet" > > > > +# Send packet that should be dropped without logging in the eggress pipeline. > > +packet="inport==\"lp1\" && eth.src==$lp1_mac && eth.dst==$lp2_mac && > > + ip4 && ip.ttl==64 && ip4.src==$lp1_ip && ip4.dst==$lp2_ip && > > + tcp && tcp.flags==2 && tcp.src==4360 && tcp.dst==180" > > +as hv ovs-appctl -t ovn-controller inject-pkt "$packet" > > + > > +# Send packet that should be dropped with logging in the egress pipeline. > > +packet="inport==\"lp1\" && eth.src==$lp1_mac && eth.dst==$lp2_mac && > > + ip4 && ip.ttl==64 && ip4.src==$lp1_ip && ip4.dst==$lp2_ip && > > + tcp && tcp.flags==2 && tcp.src==4361 && tcp.dst==181" > > +as hv ovs-appctl -t ovn-controller inject-pkt "$packet" > > + > > # Send packet that should be allowed without logging. > > packet="inport==\"lp1\" && eth.src==$lp1_mac && eth.dst==$lp2_mac && > > ip4 && ip.ttl==64 && ip4.src==$lp1_ip && ip4.dst==$lp2_ip && > > @@ -9016,25 +9042,41 @@ packet="inport==\"lp1\" && eth.src==$lp1_mac && eth.dst==$lp2_mac && > > tcp && tcp.flags==2 && tcp.src==4365 && tcp.dst==85" > > as hv ovs-appctl -t ovn-controller inject-pkt "$packet" > > > > -# Send packet that should be rejected without logging. > > +# Send packet that should be rejected without logging in the ingress pipeline. > > packet="inport==\"lp1\" && eth.src==$lp1_mac && eth.dst==$lp2_mac && > > ip4 && ip.ttl==64 && ip4.src==$lp1_ip && ip4.dst==$lp2_ip && > > tcp && tcp.flags==2 && tcp.src==4366 && tcp.dst==86" > > as hv ovs-appctl -t ovn-controller inject-pkt "$packet" > > > > -# Send packet that should be rejected with logging. > > +# Send packet that should be rejected with logging in the ingress pipeline. > > packet="inport==\"lp1\" && eth.src==$lp1_mac && eth.dst==$lp2_mac && > > ip4 && ip.ttl==64 && ip4.src==$lp1_ip && ip4.dst==$lp2_ip && > > tcp && tcp.flags==2 && tcp.src==4367 && tcp.dst==87" > > as hv ovs-appctl -t ovn-controller inject-pkt "$packet" > > > > -OVS_WAIT_UNTIL([ test 4 = $(grep -c 'acl_log' hv/ovn-controller.log) ]) > > +# Send packet that should be rejected without logging in the egress pipeline. > > +packet="inport==\"lp1\" && eth.src==$lp1_mac && eth.dst==$lp2_mac && > > + ip4 && ip.ttl==64 && ip4.src==$lp1_ip && ip4.dst==$lp2_ip && > > + tcp && tcp.flags==2 && tcp.src==4366 && tcp.dst==186" > > +as hv ovs-appctl -t ovn-controller inject-pkt "$packet" > > + > > +# Send packet that should be rejected with logging in the egress pipeline. > > +packet="inport==\"lp1\" && eth.src==$lp1_mac && eth.dst==$lp2_mac && > > + ip4 && ip.ttl==64 && ip4.src==$lp1_ip && ip4.dst==$lp2_ip && > > + tcp && tcp.flags==2 && tcp.src==4367 && tcp.dst==187" > > +as hv ovs-appctl -t ovn-controller inject-pkt "$packet" > > + > > +OVS_WAIT_UNTIL([ test 8 = $(grep -c 'acl_log' hv/ovn-controller.log) ]) > > > > AT_CHECK([grep 'acl_log' hv/ovn-controller.log | sed 's/.*name=/name=/'], [0], [dnl > > -name="drop-flow", verdict=drop, severity=alert: tcp,vlan_tci=0x0000,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02,nw_src=192.168.1.2,nw_dst=192.168.1.3,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=4361,tp_dst=81,tcp_flags=syn > > -name="allow-flow", verdict=allow, severity=info: tcp,vlan_tci=0x0000,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02,nw_src=192.168.1.2,nw_dst=192.168.1.3,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=4363,tp_dst=83,tcp_flags=syn > > -name="<unnamed>", verdict=allow, severity=info: tcp,vlan_tci=0x0000,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02,nw_src=192.168.1.2,nw_dst=192.168.1.3,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=4365,tp_dst=85,tcp_flags=syn > > -name="reject-flow", verdict=reject, severity=alert: tcp,vlan_tci=0x0000,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02,nw_src=192.168.1.2,nw_dst=192.168.1.3,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=4367,tp_dst=87,tcp_flags=syn > > +name="drop-flow", verdict=drop, severity=alert, direction=IN: tcp,vlan_tci=0x0000,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02,nw_src=192.168.1.2,nw_dst=192.168.1.3,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=4361,tp_dst=81,tcp_flags=syn > > +name="drop-flow", verdict=drop, severity=alert, direction=OUT: tcp,vlan_tci=0x0000,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02,nw_src=192.168.1.2,nw_dst=192.168.1.3,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=4361,tp_dst=181,tcp_flags=syn > > +name="allow-flow", verdict=allow, severity=info, direction=IN: tcp,vlan_tci=0x0000,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02,nw_src=192.168.1.2,nw_dst=192.168.1.3,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=4363,tp_dst=83,tcp_flags=syn > > +name="allow-flow", verdict=allow, severity=info, direction=OUT: tcp,vlan_tci=0x0000,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02,nw_src=192.168.1.2,nw_dst=192.168.1.3,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=4363,tp_dst=83,tcp_flags=syn > > +name="<unnamed>", verdict=allow, severity=info, direction=IN: tcp,vlan_tci=0x0000,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02,nw_src=192.168.1.2,nw_dst=192.168.1.3,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=4365,tp_dst=85,tcp_flags=syn > > +name="<unnamed>", verdict=allow, severity=info, direction=OUT: tcp,vlan_tci=0x0000,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02,nw_src=192.168.1.2,nw_dst=192.168.1.3,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=4365,tp_dst=85,tcp_flags=syn > > +name="reject-flow", verdict=reject, severity=alert, direction=IN: tcp,vlan_tci=0x0000,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02,nw_src=192.168.1.2,nw_dst=192.168.1.3,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=4367,tp_dst=87,tcp_flags=syn > > +name="reject-flow", verdict=reject, severity=alert, direction=OUT: tcp,vlan_tci=0x0000,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02,nw_src=192.168.1.2,nw_dst=192.168.1.3,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=4367,tp_dst=187,tcp_flags=syn > > ]) > > > > OVN_CLEANUP([hv]) > > diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c > > index 0795913d3..26c0eaa9c 100644 > > --- a/utilities/ovn-trace.c > > +++ b/utilities/ovn-trace.c > > @@ -2461,8 +2461,10 @@ execute_log(const struct ovnact_log *log, struct flow *uflow, > > { > > char *packet_str = flow_to_string(uflow, NULL); > > ovntrace_node_append(super, OVNTRACE_NODE_TRANSFORMATION, > > - "LOG: ACL name=%s, verdict=%s, severity=%s, packet=\"%s\"", > > + "LOG: ACL name=%s, direction=%s, verdict=%s, " > > + "severity=%s, packet=\"%s\"", > > log->name ? log->name : "<unnamed>", > > + log_direction_to_string(log->direction), > > log_verdict_to_string(log->verdict), > > log_severity_to_string(log->severity), > > packet_str); > > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 2/8/22 18:25, Numan Siddique wrote: > On Wed, Feb 2, 2022 at 5:17 PM Mark Michelson <mmichels@redhat.com> wrote: >> >> Acked-by: Mark Michelson <mmichels@redhat.com> >> >> On 2/2/22 12:26, Dumitru Ceara wrote: >>> It's useful to differentiate between ingress and egress pipelines in the >>> ACL logs. To achieve this we expand the "log()" logical action and pass >>> an optional direction. >>> >>> This behavior change is implemented in a backwards compatible way such >>> that it doesn't break existing deployments even in the case when >>> ovn-northd gets updated before ovn-controller. >>> >>> To achieve this, ovn-northd determines first if all chassis in the >>> cluster have been upgraded to a version that supports the new "log()" >>> action format. >>> >>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1992641 >>> Signed-off-by: Dumitru Ceara <dceara@redhat.com> >>> --- >>> v2: Add the "direction" after the "verdict" and "severity" fields in the >>> ACL logs. > > Hi Dumitru, > > Thanks for the patch. The patch LGTM. > > I've an alternate suggestion to log the direction. > When the packet is received by ovn-controller, it can determine the > open flow table id from the table_id field of 'struct > ofputil_packet_in'. > From the open flow table id, it should be simple enough to figure out > if this is from ingress or egress direction. > > If we take this approach, we don't have to make any changes to the > 'acl_log' OVN action. > > Let me know what you think. > You're right this is way better, thanks for the suggestion! I sent a v3: http://patchwork.ozlabs.org/project/ovn/list/?series=285670 Thanks, Dumitru
diff --git a/NEWS b/NEWS index 194557410..a01820dfe 100644 --- a/NEWS +++ b/NEWS @@ -1,5 +1,7 @@ Post v21.12.0 ------------- + - When configured to log packtes matching ACLs, log the direction (logical + pipeline) too. OVN v21.12.0 - 22 Dec 2021 -------------------------- diff --git a/controller/chassis.c b/controller/chassis.c index 8a1559653..e77ae0b0c 100644 --- a/controller/chassis.c +++ b/controller/chassis.c @@ -350,6 +350,7 @@ chassis_build_other_config(const struct ovs_chassis_cfg *ovs_cfg, smap_replace(config, "is-interconn", ovs_cfg->is_interconn ? "true" : "false"); smap_replace(config, OVN_FEATURE_PORT_UP_NOTIF, "true"); + smap_replace(config, OVN_FEATURE_ACL_LOG_DIRECTION, "true"); } /* @@ -455,6 +456,12 @@ chassis_other_config_changed(const struct ovs_chassis_cfg *ovs_cfg, return true; } + if (!smap_get_bool(&chassis_rec->other_config, + OVN_FEATURE_ACL_LOG_DIRECTION, + false)) { + return true; + } + return false; } diff --git a/include/ovn/actions.h b/include/ovn/actions.h index cdef5fb03..57222c78a 100644 --- a/include/ovn/actions.h +++ b/include/ovn/actions.h @@ -370,6 +370,7 @@ struct ovnact_result { /* OVNACT_LOG. */ struct ovnact_log { struct ovnact ovnact; + uint8_t direction; /* One of LOG_DIRECTION_*. */ uint8_t verdict; /* One of LOG_VERDICT_*. */ uint8_t severity; /* One of LOG_SEVERITY_*. */ char *name; diff --git a/include/ovn/features.h b/include/ovn/features.h index d12a8eb0d..e69e78e0a 100644 --- a/include/ovn/features.h +++ b/include/ovn/features.h @@ -22,6 +22,7 @@ /* ovn-controller supported feature names. */ #define OVN_FEATURE_PORT_UP_NOTIF "port-up-notif" +#define OVN_FEATURE_ACL_LOG_DIRECTION "acl-log-dir" /* OVS datapath supported features. Based on availability OVN might generate * different types of openflows. diff --git a/lib/acl-log.c b/lib/acl-log.c index 220b6dc30..fb981814f 100644 --- a/lib/acl-log.c +++ b/lib/acl-log.c @@ -39,6 +39,21 @@ log_verdict_to_string(uint8_t verdict) } } +const char * +log_direction_to_string(uint8_t direction) +{ + switch (direction) { + case LOG_DIRECTION_NONE: + return "none"; + case LOG_DIRECTION_IN: + return "IN"; + case LOG_DIRECTION_OUT: + return "OUT"; + default: + return "<unknown>"; + } +} + const char * log_severity_to_string(uint8_t severity) { @@ -88,15 +103,23 @@ handle_acl_log(const struct flow *headers, struct ofpbuf *userdata) return; } + uint8_t direction = LOG_DIRECTION(lph->direction_verdict); + uint8_t verdict = LOG_VERDICT(lph->direction_verdict); + size_t name_len = userdata->size; char *name = name_len ? xmemdup0(userdata->data, name_len) : NULL; struct ds ds = DS_EMPTY_INITIALIZER; ds_put_cstr(&ds, "name="); json_string_escape(name_len ? name : "<unnamed>", &ds); - ds_put_format(&ds, ", verdict=%s, severity=%s: ", - log_verdict_to_string(lph->verdict), + ds_put_format(&ds, ", verdict=%s, severity=%s", + log_verdict_to_string(verdict), log_severity_to_string(lph->severity)); + if (direction != LOG_DIRECTION_NONE) { + ds_put_format(&ds, ", direction=%s", + log_direction_to_string(direction)); + } + ds_put_cstr(&ds, ": "); flow_format(&ds, headers, NULL); VLOG_INFO("%s", ds_cstr(&ds)); diff --git a/lib/acl-log.h b/lib/acl-log.h index 4f23f790d..acc920856 100644 --- a/lib/acl-log.h +++ b/lib/acl-log.h @@ -18,25 +18,51 @@ #define ACL_LOG_H 1 #include <stdint.h> +#include "compiler.h" #include "openvswitch/types.h" struct ofpbuf; struct flow; struct log_pin_header { - uint8_t verdict; /* One of LOG_VERDICT_*. */ + uint8_t direction_verdict; /* 4 bits for LOG_DIRECTION_* and + * 4 bits for LOG_VERDICT_*. */ uint8_t severity; /* One of LOG_SEVERITY*. */ /* Followed by an optional string containing the rule's name. */ }; +enum log_direction { + LOG_DIRECTION_NONE, + LOG_DIRECTION_IN, + LOG_DIRECTION_OUT, + LOG_DIRECTION_MAX, +}; + enum log_verdict { LOG_VERDICT_ALLOW, LOG_VERDICT_DROP, LOG_VERDICT_REJECT, + LOG_VERDICT_MAX, LOG_VERDICT_UNKNOWN = UINT8_MAX }; +/* For backwards compatibility, use the least significant 4 bits for + * verdict values and the most significant 4 bits for direction values. + * + * This is backwards compatible; old encodings will be decoded as: + * - direction: NONE + * - verdict: VERDICT + */ +#define LOG_VERDICT_BITS 4 +#define LOG_DIRECTION_BITS 4 +#define LOG_VERDICT_MASK ((1 << LOG_VERDICT_BITS) - 1) +#define LOG_DIRECTION_MASK (0xFF ^ LOG_VERDICT_MASK) + +BUILD_ASSERT_DECL(LOG_VERDICT_MAX <= (1 << LOG_VERDICT_BITS)); +BUILD_ASSERT_DECL(LOG_DIRECTION_MAX <= (1 << LOG_DIRECTION_BITS)); + const char *log_verdict_to_string(uint8_t verdict); +const char *log_direction_to_string(uint8_t direction); /* Severity levels. Based on RFC5424 levels. */ @@ -46,6 +72,15 @@ const char *log_verdict_to_string(uint8_t verdict); #define LOG_SEVERITY_INFO 6 #define LOG_SEVERITY_DEBUG 7 +#define LOG_DIRECTION_VERDICT(DIR, VERDICT) \ + ((DIR) << LOG_VERDICT_BITS | (VERDICT)) + +#define LOG_DIRECTION(DIR_VERDICT) \ + (((DIR_VERDICT) & LOG_DIRECTION_MASK) >> LOG_VERDICT_BITS) + +#define LOG_VERDICT(DIR_VERDICT) \ + ((DIR_VERDICT) & LOG_VERDICT_MASK) + const char *log_severity_to_string(uint8_t severity); uint8_t log_severity_from_string(const char *name); diff --git a/lib/actions.c b/lib/actions.c index d5d8391bb..4f37a1cb1 100644 --- a/lib/actions.c +++ b/lib/actions.c @@ -3142,7 +3142,19 @@ encode_PUT_ND_RA_OPTS(const struct ovnact_put_opts *po, static void parse_log_arg(struct action_context *ctx, struct ovnact_log *log) { - if (lexer_match_id(ctx->lexer, "verdict")) { + if (lexer_match_id(ctx->lexer, "direction")) { + if (!lexer_force_match(ctx->lexer, LEX_T_EQUALS)) { + return; + } + if (lexer_match_id(ctx->lexer, "IN")) { + log->direction = LOG_DIRECTION_IN; + } else if (lexer_match_id(ctx->lexer, "OUT")) { + log->direction = LOG_DIRECTION_OUT; + } else { + lexer_syntax_error(ctx->lexer, "unknown direction"); + return; + } + } else if (lexer_match_id(ctx->lexer, "verdict")) { if (!lexer_force_match(ctx->lexer, LEX_T_EQUALS)) { return; } @@ -3216,6 +3228,7 @@ parse_LOG(struct action_context *ctx) struct ovnact_log *log = ovnact_put_LOG(ctx->ovnacts); /* Provide default values. */ + log->direction = LOG_DIRECTION_NONE; log->severity = LOG_SEVERITY_INFO; log->verdict = LOG_VERDICT_UNKNOWN; @@ -3271,7 +3284,8 @@ encode_LOG(const struct ovnact_log *log, meter_id, ofpacts); struct log_pin_header *lph = ofpbuf_put_uninit(ofpacts, sizeof *lph); - lph->verdict = log->verdict; + lph->direction_verdict = LOG_DIRECTION_VERDICT(log->direction, + log->verdict); lph->severity = log->severity; if (log->name) { diff --git a/northd/en-lflow.c b/northd/en-lflow.c index ffbdaf4e8..c91d6468d 100644 --- a/northd/en-lflow.c +++ b/northd/en-lflow.c @@ -60,6 +60,7 @@ void en_lflow_run(struct engine_node *node, void *data OVS_UNUSED) lflow_input.meter_groups = &northd_data->meter_groups; lflow_input.lbs = &northd_data->lbs; lflow_input.bfd_connections = &northd_data->bfd_connections; + lflow_input.chassis_info = &northd_data->chassis_info; lflow_input.ovn_internal_version_changed = northd_data->ovn_internal_version_changed; diff --git a/northd/northd.c b/northd/northd.c index fc7a64f99..8016ed10f 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -1383,24 +1383,33 @@ join_datapaths(struct northd_input *input_data, } } -static bool -is_vxlan_mode(struct northd_input *input_data) +static void +chassis_data_init(struct northd_input *input_data, struct chassis_data *data) { + data->vxlan_mode = false; + data->log_acl_direction = true; + const struct sbrec_chassis *chassis; SBREC_CHASSIS_TABLE_FOR_EACH (chassis, input_data->sbrec_chassis) { - for (int i = 0; i < chassis->n_encaps; i++) { + if (data->log_acl_direction + && !smap_get_bool(&chassis->other_config, + OVN_FEATURE_ACL_LOG_DIRECTION, + false)) { + data->log_acl_direction = false; + } + for (int i = 0; !data->vxlan_mode && i < chassis->n_encaps; i++) { if (!strcmp(chassis->encaps[i]->type, "vxlan")) { - return true; + data->vxlan_mode = true; + break; } } } - return false; } static uint32_t -get_ovn_max_dp_key_local(struct northd_input *input_data) +get_ovn_max_dp_key_local(struct chassis_data *chassis_info) { - if (is_vxlan_mode(input_data)) { + if (chassis_info->vxlan_mode) { /* OVN_MAX_DP_GLOBAL_NUM doesn't apply for vxlan mode. */ return OVN_MAX_DP_VXLAN_KEY; } @@ -1408,14 +1417,14 @@ get_ovn_max_dp_key_local(struct northd_input *input_data) } static void -ovn_datapath_allocate_key(struct northd_input *input_data, +ovn_datapath_allocate_key(struct chassis_data *chassis_info, struct hmap *datapaths, struct hmap *dp_tnlids, struct ovn_datapath *od, uint32_t *hint) { if (!od->tunnel_key) { od->tunnel_key = ovn_allocate_tnlid(dp_tnlids, "datapath", OVN_MIN_DP_KEY_LOCAL, - get_ovn_max_dp_key_local(input_data), + get_ovn_max_dp_key_local(chassis_info), hint); if (!od->tunnel_key) { if (od->sb) { @@ -1428,7 +1437,7 @@ ovn_datapath_allocate_key(struct northd_input *input_data, } static void -ovn_datapath_assign_requested_tnl_id(struct northd_input *input_data, +ovn_datapath_assign_requested_tnl_id(struct chassis_data *chassis_info, struct hmap *dp_tnlids, struct ovn_datapath *od) { @@ -1438,7 +1447,7 @@ ovn_datapath_assign_requested_tnl_id(struct northd_input *input_data, uint32_t tunnel_key = smap_get_int(other_config, "requested-tnl-key", 0); if (tunnel_key) { const char *interconn_ts = smap_get(other_config, "interconn-ts"); - if (!interconn_ts && is_vxlan_mode(input_data) && + if (!interconn_ts && chassis_info->vxlan_mode && tunnel_key >= 1 << 12) { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); VLOG_WARN_RL(&rl, "Tunnel key %"PRIu32" for datapath %s is " @@ -1465,6 +1474,7 @@ ovn_datapath_assign_requested_tnl_id(struct northd_input *input_data, * switch and router. */ static void build_datapaths(struct northd_input *input_data, + struct chassis_data *chassis_info, struct ovsdb_idl_txn *ovnsb_txn, struct hmap *datapaths, struct ovs_list *lr_list) @@ -1478,10 +1488,10 @@ build_datapaths(struct northd_input *input_data, struct hmap dp_tnlids = HMAP_INITIALIZER(&dp_tnlids); struct ovn_datapath *od, *next; LIST_FOR_EACH (od, list, &both) { - ovn_datapath_assign_requested_tnl_id(input_data, &dp_tnlids, od); + ovn_datapath_assign_requested_tnl_id(chassis_info, &dp_tnlids, od); } LIST_FOR_EACH (od, list, &nb_only) { - ovn_datapath_assign_requested_tnl_id(input_data, &dp_tnlids, od); + ovn_datapath_assign_requested_tnl_id(chassis_info, &dp_tnlids, od); } /* Keep nonconflicting tunnel IDs that are already assigned. */ @@ -1494,11 +1504,11 @@ build_datapaths(struct northd_input *input_data, /* Assign new tunnel ids where needed. */ uint32_t hint = 0; LIST_FOR_EACH_SAFE (od, next, list, &both) { - ovn_datapath_allocate_key(input_data, + ovn_datapath_allocate_key(chassis_info, datapaths, &dp_tnlids, od, &hint); } LIST_FOR_EACH_SAFE (od, next, list, &nb_only) { - ovn_datapath_allocate_key(input_data, + ovn_datapath_allocate_key(chassis_info, datapaths, &dp_tnlids, od, &hint); } @@ -4047,7 +4057,7 @@ ovn_port_add_tnlid(struct ovn_port *op, uint32_t tunnel_key) } static void -ovn_port_assign_requested_tnl_id(struct northd_input *input_data, +ovn_port_assign_requested_tnl_id(struct chassis_data *chassis_info, struct ovn_port *op) { const struct smap *options = (op->nbsp @@ -4055,7 +4065,7 @@ ovn_port_assign_requested_tnl_id(struct northd_input *input_data, : &op->nbrp->options); uint32_t tunnel_key = smap_get_int(options, "requested-tnl-key", 0); if (tunnel_key) { - if (is_vxlan_mode(input_data) && + if (chassis_info->vxlan_mode && tunnel_key >= OVN_VXLAN_MIN_MULTICAST) { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); VLOG_WARN_RL(&rl, "Tunnel key %"PRIu32" for port %s " @@ -4074,12 +4084,11 @@ ovn_port_assign_requested_tnl_id(struct northd_input *input_data, } static void -ovn_port_allocate_key(struct northd_input *input_data, - struct hmap *ports, - struct ovn_port *op) +ovn_port_allocate_key(struct chassis_data *chassis_info, + struct hmap *ports, struct ovn_port *op) { if (!op->tunnel_key) { - uint8_t key_bits = is_vxlan_mode(input_data)? 12 : 16; + uint8_t key_bits = chassis_info->vxlan_mode ? 12 : 16; op->tunnel_key = ovn_allocate_tnlid(&op->od->port_tnlids, "port", 1, (1u << (key_bits - 1)) - 1, &op->od->port_key_hint); @@ -4101,6 +4110,7 @@ ovn_port_allocate_key(struct northd_input *input_data, * datapaths. */ static void build_ports(struct northd_input *input_data, + struct chassis_data *chassis_info, struct ovsdb_idl_txn *ovnsb_txn, struct ovsdb_idl_index *sbrec_chassis_by_name, struct ovsdb_idl_index *sbrec_chassis_by_hostname, @@ -4124,10 +4134,10 @@ build_ports(struct northd_input *input_data, /* Assign explicitly requested tunnel ids first. */ struct ovn_port *op, *next; LIST_FOR_EACH (op, list, &both) { - ovn_port_assign_requested_tnl_id(input_data, op); + ovn_port_assign_requested_tnl_id(chassis_info, op); } LIST_FOR_EACH (op, list, &nb_only) { - ovn_port_assign_requested_tnl_id(input_data, op); + ovn_port_assign_requested_tnl_id(chassis_info, op); } /* Keep nonconflicting tunnel IDs that are already assigned. */ @@ -4139,10 +4149,10 @@ build_ports(struct northd_input *input_data, /* Assign new tunnel ids where needed. */ LIST_FOR_EACH_SAFE (op, next, list, &both) { - ovn_port_allocate_key(input_data, ports, op); + ovn_port_allocate_key(chassis_info, ports, op); } LIST_FOR_EACH_SAFE (op, next, list, &nb_only) { - ovn_port_allocate_key(input_data, ports, op); + ovn_port_allocate_key(chassis_info, ports, op); } /* For logical ports that are in both databases, update the southbound @@ -6093,6 +6103,7 @@ build_acl_log_meter(struct ds *actions, const struct nbrec_acl *acl, static void build_acl_log(struct ds *actions, const struct nbrec_acl *acl, + enum ovn_stage stage, const struct chassis_data *chassis_info, const struct shash *meter_groups) { if (!acl->log) { @@ -6100,6 +6111,10 @@ build_acl_log(struct ds *actions, const struct nbrec_acl *acl, } ds_put_cstr(actions, "log("); + if (chassis_info->log_acl_direction) { + ds_put_format(actions, "direction=%s, ", + stage == S_SWITCH_IN_ACL ? "IN" : "OUT"); + } if (acl->name) { ds_put_format(actions, "name=\"%s\", ", acl->name); @@ -6134,6 +6149,7 @@ build_reject_acl_rules(struct ovn_datapath *od, struct hmap *lflows, enum ovn_stage stage, struct nbrec_acl *acl, struct ds *extra_match, struct ds *extra_actions, const struct ovsdb_idl_row *stage_hint, + const struct chassis_data *chassis_info, const struct shash *meter_groups) { struct ds match = DS_EMPTY_INITIALIZER; @@ -6146,7 +6162,7 @@ build_reject_acl_rules(struct ovn_datapath *od, struct hmap *lflows, ingress ? ovn_stage_get_table(S_SWITCH_OUT_QOS_MARK) : ovn_stage_get_table(S_SWITCH_IN_L2_LKUP)); - build_acl_log(&actions, acl, meter_groups); + build_acl_log(&actions, acl, stage, chassis_info, meter_groups); if (extra_match->length > 0) { ds_put_format(&match, "(%s) && ", extra_match->string); } @@ -6175,6 +6191,7 @@ build_reject_acl_rules(struct ovn_datapath *od, struct hmap *lflows, static void consider_acl(struct hmap *lflows, struct ovn_datapath *od, struct nbrec_acl *acl, bool has_stateful, + const struct chassis_data *chassis_info, const struct shash *meter_groups, struct ds *match, struct ds *actions) { @@ -6183,7 +6200,7 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od, if (!strcmp(acl->action, "allow-stateless")) { ds_clear(actions); - build_acl_log(actions, acl, meter_groups); + build_acl_log(actions, acl, stage, chassis_info, meter_groups); ds_put_cstr(actions, "next;"); ovn_lflow_add_with_hint(lflows, od, stage, acl->priority + OVN_ACL_PRI_OFFSET, @@ -6198,7 +6215,7 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od, * associated conntrack entry and would return "+invalid". */ if (!has_stateful) { ds_clear(actions); - build_acl_log(actions, acl, meter_groups); + build_acl_log(actions, acl, stage, chassis_info, meter_groups); ds_put_cstr(actions, "next;"); ovn_lflow_add_with_hint(lflows, od, stage, acl->priority + OVN_ACL_PRI_OFFSET, @@ -6227,7 +6244,7 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od, ds_put_format(actions, REGBIT_ACL_LABEL" = 1; " REG_LABEL" = %"PRId64"; ", acl->label); } - build_acl_log(actions, acl, meter_groups); + build_acl_log(actions, acl, stage, chassis_info, meter_groups); ds_put_cstr(actions, "next;"); ovn_lflow_add_with_hint(lflows, od, stage, acl->priority + OVN_ACL_PRI_OFFSET, @@ -6252,7 +6269,7 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od, ds_put_format(actions, REGBIT_ACL_LABEL" = 1; " REG_LABEL" = %"PRId64"; ", acl->label); } - build_acl_log(actions, acl, meter_groups); + build_acl_log(actions, acl, stage, chassis_info, meter_groups); ds_put_cstr(actions, "next;"); ovn_lflow_add_with_hint(lflows, od, stage, acl->priority + OVN_ACL_PRI_OFFSET, @@ -6272,11 +6289,12 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od, ds_clear(actions); ds_put_cstr(match, REGBIT_ACL_HINT_DROP " == 1"); if (!strcmp(acl->action, "reject")) { - build_reject_acl_rules(od, lflows, stage, acl, match, - actions, &acl->header_, meter_groups); + build_reject_acl_rules(od, lflows, stage, acl, match, actions, + &acl->header_, chassis_info, + meter_groups); } else { ds_put_format(match, " && (%s)", acl->match); - build_acl_log(actions, acl, meter_groups); + build_acl_log(actions, acl, stage, chassis_info, meter_groups); ds_put_cstr(actions, "/* drop */"); ovn_lflow_add_with_hint(lflows, od, stage, acl->priority + OVN_ACL_PRI_OFFSET, @@ -6299,11 +6317,12 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od, ds_put_cstr(match, REGBIT_ACL_HINT_BLOCK " == 1"); ds_put_cstr(actions, "ct_commit { ct_label.blocked = 1; }; "); if (!strcmp(acl->action, "reject")) { - build_reject_acl_rules(od, lflows, stage, acl, match, - actions, &acl->header_, meter_groups); + build_reject_acl_rules(od, lflows, stage, acl, match, actions, + &acl->header_, chassis_info, + meter_groups); } else { ds_put_format(match, " && (%s)", acl->match); - build_acl_log(actions, acl, meter_groups); + build_acl_log(actions, acl, stage, chassis_info, meter_groups); ds_put_cstr(actions, "/* drop */"); ovn_lflow_add_with_hint(lflows, od, stage, acl->priority + OVN_ACL_PRI_OFFSET, @@ -6317,10 +6336,11 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od, ds_clear(match); ds_clear(actions); if (!strcmp(acl->action, "reject")) { - build_reject_acl_rules(od, lflows, stage, acl, match, - actions, &acl->header_, meter_groups); + build_reject_acl_rules(od, lflows, stage, acl, match, actions, + &acl->header_, chassis_info, + meter_groups); } else { - build_acl_log(actions, acl, meter_groups); + build_acl_log(actions, acl, stage, chassis_info, meter_groups); ds_put_cstr(actions, "/* drop */"); ovn_lflow_add_with_hint(lflows, od, stage, acl->priority + OVN_ACL_PRI_OFFSET, @@ -6400,7 +6420,9 @@ build_port_group_lswitches(struct northd_input *input_data, static void build_acls(struct ovn_datapath *od, struct hmap *lflows, - const struct hmap *port_groups, const struct shash *meter_groups) + const struct chassis_data *chassis_info, + const struct hmap *port_groups, + const struct shash *meter_groups) { bool has_stateful = od->has_stateful_acl || od->has_lb_vip; struct ds match = DS_EMPTY_INITIALIZER; @@ -6515,15 +6537,15 @@ build_acls(struct ovn_datapath *od, struct hmap *lflows, /* Ingress or Egress ACL Table (Various priorities). */ for (size_t i = 0; i < od->nbs->n_acls; i++) { struct nbrec_acl *acl = od->nbs->acls[i]; - consider_acl(lflows, od, acl, has_stateful, meter_groups, &match, - &actions); + consider_acl(lflows, od, acl, has_stateful, chassis_info, meter_groups, + &match, &actions); } struct ovn_port_group *pg; HMAP_FOR_EACH (pg, key_node, port_groups) { if (ovn_port_group_ls_find(pg, &od->nbs->header_.uuid)) { for (size_t i = 0; i < pg->nb_pg->n_acls; i++) { consider_acl(lflows, od, pg->nb_pg->acls[i], has_stateful, - meter_groups, &match, &actions); + chassis_info, meter_groups, &match, &actions); } } } @@ -7527,6 +7549,7 @@ build_lswitch_flows(const struct hmap *datapaths, * Ingress tables 3 through 10. Egress tables 0 through 7. */ static void build_lswitch_lflows_pre_acl_and_acl(struct ovn_datapath *od, + const struct chassis_data *chassis_info, const struct hmap *port_groups, struct hmap *lflows, const struct shash *meter_groups) @@ -7538,7 +7561,7 @@ build_lswitch_lflows_pre_acl_and_acl(struct ovn_datapath *od, build_pre_lb(od, lflows); build_pre_stateful(od, lflows); build_acl_hints(od, lflows); - build_acls(od, lflows, port_groups, meter_groups); + build_acls(od, lflows, chassis_info, port_groups, meter_groups); build_qos(od, lflows); build_stateful(od, lflows); build_lb_hairpin(od, lflows); @@ -13248,6 +13271,7 @@ build_lrouter_nat_defrag_and_lb(struct ovn_datapath *od, struct hmap *lflows, struct lswitch_flow_build_info { + const struct chassis_data *chassis_info; const struct hmap *datapaths; const struct hmap *ports; const struct hmap *port_groups; @@ -13275,8 +13299,9 @@ build_lswitch_and_lrouter_iterate_by_od(struct ovn_datapath *od, struct lswitch_flow_build_info *lsi) { /* Build Logical Switch Flows. */ - build_lswitch_lflows_pre_acl_and_acl(od, lsi->port_groups, lsi->lflows, - lsi->meter_groups); + build_lswitch_lflows_pre_acl_and_acl(od, lsi->chassis_info, + lsi->port_groups, + lsi->lflows, lsi->meter_groups); build_fwd_group_lflows(od, lsi->lflows); build_lswitch_lflows_admission_control(od, lsi->lflows); @@ -13512,7 +13537,8 @@ fix_flow_map_size(struct hmap *lflow_map, } static void -build_lswitch_and_lrouter_flows(const struct hmap *datapaths, +build_lswitch_and_lrouter_flows(const struct chassis_data *chassis_info, + const struct hmap *datapaths, const struct hmap *ports, const struct hmap *port_groups, struct hmap *lflows, @@ -13564,6 +13590,7 @@ build_lswitch_and_lrouter_flows(const struct hmap *datapaths, lsiv[index].meter_groups = meter_groups; lsiv[index].lbs = lbs; lsiv[index].bfd_connections = bfd_connections; + lsiv[index].chassis_info = chassis_info; lsiv[index].svc_check_match = svc_check_match; lsiv[index].thread_lflow_counter = 0; ds_init(&lsiv[index].match); @@ -13602,6 +13629,7 @@ build_lswitch_and_lrouter_flows(const struct hmap *datapaths, .meter_groups = meter_groups, .lbs = lbs, .bfd_connections = bfd_connections, + .chassis_info = chassis_info, .svc_check_match = svc_check_match, .match = DS_EMPTY_INITIALIZER, .actions = DS_EMPTY_INITIALIZER, @@ -13757,7 +13785,8 @@ void build_lflows(struct lflow_input *input_data, use_parallel_build = false; reset_parallel = true; } - build_lswitch_and_lrouter_flows(input_data->datapaths, input_data->ports, + build_lswitch_and_lrouter_flows(input_data->chassis_info, + input_data->datapaths, input_data->ports, input_data->port_groups, &lflows, &mcast_groups, &igmp_groups, input_data->meter_groups, input_data->lbs, @@ -14869,6 +14898,9 @@ ovnnb_db_run(struct northd_input *input_data, } stopwatch_start(BUILD_LFLOWS_CTX_STOPWATCH_NAME, time_msec()); + /* Update chassis-specific supported data. */ + chassis_data_init(input_data, &data->chassis_info); + /* Sync ipsec configuration. * Copy nb_cfg from northbound to southbound database. * Also set up to update sb_cfg once our southbound transaction commits. */ @@ -14912,7 +14944,8 @@ ovnnb_db_run(struct northd_input *input_data, smap_replace(&options, "svc_monitor_mac", svc_monitor_mac); } - char *max_tunid = xasprintf("%d", get_ovn_max_dp_key_local(input_data)); + char *max_tunid = + xasprintf("%d", get_ovn_max_dp_key_local(&data->chassis_info)); smap_replace(&options, "max_tunid", max_tunid); free(max_tunid); @@ -14948,11 +14981,12 @@ ovnnb_db_run(struct northd_input *input_data, check_lsp_is_up = !smap_get_bool(&nb->options, "ignore_lsp_down", true); - build_datapaths(input_data, ovnsb_txn, &data->datapaths, &data->lr_list); + build_datapaths(input_data, &data->chassis_info, ovnsb_txn, + &data->datapaths, &data->lr_list); build_ovn_lbs(input_data, ovnsb_txn, &data->datapaths, &data->lbs); build_lrouter_lbs(&data->datapaths, &data->lbs); - build_ports(input_data, ovnsb_txn, sbrec_chassis_by_name, - sbrec_chassis_by_hostname, + build_ports(input_data, &data->chassis_info, ovnsb_txn, + sbrec_chassis_by_name, sbrec_chassis_by_hostname, &data->datapaths, &data->ports); build_lrouter_lbs_reachable_ips(&data->datapaths, &data->lbs); build_ovn_lr_lbs(&data->datapaths, &data->lbs); diff --git a/northd/northd.h b/northd/northd.h index ebcb40de7..9329dbf47 100644 --- a/northd/northd.h +++ b/northd/northd.h @@ -53,8 +53,14 @@ struct northd_input { struct ovsdb_idl_index *sbrec_ip_mcast_by_dp; }; +struct chassis_data { + bool vxlan_mode; + bool log_acl_direction; +}; + struct northd_data { /* Global state for 'en-northd'. */ + struct chassis_data chassis_info; struct hmap datapaths; struct hmap ports; struct hmap port_groups; @@ -78,6 +84,7 @@ struct lflow_input { /* Indexes */ struct ovsdb_idl_index *sbrec_mcast_group_by_name_dp; + const struct chassis_data *chassis_info; const struct hmap *datapaths; const struct hmap *ports; const struct hmap *port_groups; diff --git a/ovn-sb.xml b/ovn-sb.xml index 9ddacdf09..31e677c2b 100644 --- a/ovn-sb.xml +++ b/ovn-sb.xml @@ -2174,6 +2174,12 @@ <code>debug</code>. If a severity is not provided, the default is <code>info</code>. </dd> + <dt><code>direction=</code><var>value</var></dt> + <dd> + The direction (logical pipeline) in which packets matched the + flow. The value must be one of: <code>IN</code> for ingress + pipeline, or <code>OUT</code> for egress pipeline. + </dd> <dt><code>verdict=</code><var>value</var></dt> <dd> The verdict for packets matching the flow. The value must be one diff --git a/tests/ovn.at b/tests/ovn.at index 957eb7850..cfb9818ce 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -8965,33 +8965,59 @@ ovn-nbctl lsp-set-addresses lp2 $lp2_mac ovn-nbctl --wait=sb sync wait_for_ports_up -ovn-nbctl acl-add lsw0 to-lport 1000 'tcp.dst==80' drop -ovn-nbctl --log --severity=alert --name=drop-flow acl-add lsw0 to-lport 1000 'tcp.dst==81' drop +ovn-nbctl acl-add lsw0 from-lport 1000 'tcp.dst==80' drop +ovn-nbctl --log --severity=alert --name=drop-flow acl-add lsw0 from-lport 1000 'tcp.dst==81' drop + +ovn-nbctl acl-add lsw0 to-lport 1000 'tcp.dst==180' drop +ovn-nbctl --log --severity=alert --name=drop-flow acl-add lsw0 to-lport 1000 'tcp.dst==181' drop + +ovn-nbctl acl-add lsw0 from-lport 1000 'tcp.dst==82' allow +ovn-nbctl --log --severity=info --name=allow-flow acl-add lsw0 from-lport 1000 'tcp.dst==83' allow ovn-nbctl acl-add lsw0 to-lport 1000 'tcp.dst==82' allow ovn-nbctl --log --severity=info --name=allow-flow acl-add lsw0 to-lport 1000 'tcp.dst==83' allow +ovn-nbctl acl-add lsw0 from-lport 1000 'tcp.dst==84' allow-related +ovn-nbctl --log acl-add lsw0 from-lport 1000 'tcp.dst==85' allow-related + ovn-nbctl acl-add lsw0 to-lport 1000 'tcp.dst==84' allow-related ovn-nbctl --log acl-add lsw0 to-lport 1000 'tcp.dst==85' allow-related -ovn-nbctl acl-add lsw0 to-lport 1000 'tcp.dst==86' reject -ovn-nbctl --wait=hv --log --severity=alert --name=reject-flow acl-add lsw0 to-lport 1000 'tcp.dst==87' reject +ovn-nbctl acl-add lsw0 from-lport 1000 'tcp.dst==86' reject +ovn-nbctl --log --severity=alert --name=reject-flow acl-add lsw0 from-lport 1000 'tcp.dst==87' reject + +ovn-nbctl acl-add lsw0 to-lport 1000 'tcp.dst==186' reject +ovn-nbctl --log --severity=alert --name=reject-flow acl-add lsw0 to-lport 1000 'tcp.dst==187' reject + +ovn-nbctl --wait=hv sync ovn-sbctl dump-flows > sbflows AT_CAPTURE_FILE([sbflows]) -# Send packet that should be dropped without logging. +# Send packet that should be dropped without logging in the ingress pipeline. packet="inport==\"lp1\" && eth.src==$lp1_mac && eth.dst==$lp2_mac && ip4 && ip.ttl==64 && ip4.src==$lp1_ip && ip4.dst==$lp2_ip && tcp && tcp.flags==2 && tcp.src==4360 && tcp.dst==80" as hv ovs-appctl -t ovn-controller inject-pkt "$packet" -# Send packet that should be dropped with logging. +# Send packet that should be dropped with logging in the ingress pipeline. packet="inport==\"lp1\" && eth.src==$lp1_mac && eth.dst==$lp2_mac && ip4 && ip.ttl==64 && ip4.src==$lp1_ip && ip4.dst==$lp2_ip && tcp && tcp.flags==2 && tcp.src==4361 && tcp.dst==81" as hv ovs-appctl -t ovn-controller inject-pkt "$packet" +# Send packet that should be dropped without logging in the eggress pipeline. +packet="inport==\"lp1\" && eth.src==$lp1_mac && eth.dst==$lp2_mac && + ip4 && ip.ttl==64 && ip4.src==$lp1_ip && ip4.dst==$lp2_ip && + tcp && tcp.flags==2 && tcp.src==4360 && tcp.dst==180" +as hv ovs-appctl -t ovn-controller inject-pkt "$packet" + +# Send packet that should be dropped with logging in the egress pipeline. +packet="inport==\"lp1\" && eth.src==$lp1_mac && eth.dst==$lp2_mac && + ip4 && ip.ttl==64 && ip4.src==$lp1_ip && ip4.dst==$lp2_ip && + tcp && tcp.flags==2 && tcp.src==4361 && tcp.dst==181" +as hv ovs-appctl -t ovn-controller inject-pkt "$packet" + # Send packet that should be allowed without logging. packet="inport==\"lp1\" && eth.src==$lp1_mac && eth.dst==$lp2_mac && ip4 && ip.ttl==64 && ip4.src==$lp1_ip && ip4.dst==$lp2_ip && @@ -9016,25 +9042,41 @@ packet="inport==\"lp1\" && eth.src==$lp1_mac && eth.dst==$lp2_mac && tcp && tcp.flags==2 && tcp.src==4365 && tcp.dst==85" as hv ovs-appctl -t ovn-controller inject-pkt "$packet" -# Send packet that should be rejected without logging. +# Send packet that should be rejected without logging in the ingress pipeline. packet="inport==\"lp1\" && eth.src==$lp1_mac && eth.dst==$lp2_mac && ip4 && ip.ttl==64 && ip4.src==$lp1_ip && ip4.dst==$lp2_ip && tcp && tcp.flags==2 && tcp.src==4366 && tcp.dst==86" as hv ovs-appctl -t ovn-controller inject-pkt "$packet" -# Send packet that should be rejected with logging. +# Send packet that should be rejected with logging in the ingress pipeline. packet="inport==\"lp1\" && eth.src==$lp1_mac && eth.dst==$lp2_mac && ip4 && ip.ttl==64 && ip4.src==$lp1_ip && ip4.dst==$lp2_ip && tcp && tcp.flags==2 && tcp.src==4367 && tcp.dst==87" as hv ovs-appctl -t ovn-controller inject-pkt "$packet" -OVS_WAIT_UNTIL([ test 4 = $(grep -c 'acl_log' hv/ovn-controller.log) ]) +# Send packet that should be rejected without logging in the egress pipeline. +packet="inport==\"lp1\" && eth.src==$lp1_mac && eth.dst==$lp2_mac && + ip4 && ip.ttl==64 && ip4.src==$lp1_ip && ip4.dst==$lp2_ip && + tcp && tcp.flags==2 && tcp.src==4366 && tcp.dst==186" +as hv ovs-appctl -t ovn-controller inject-pkt "$packet" + +# Send packet that should be rejected with logging in the egress pipeline. +packet="inport==\"lp1\" && eth.src==$lp1_mac && eth.dst==$lp2_mac && + ip4 && ip.ttl==64 && ip4.src==$lp1_ip && ip4.dst==$lp2_ip && + tcp && tcp.flags==2 && tcp.src==4367 && tcp.dst==187" +as hv ovs-appctl -t ovn-controller inject-pkt "$packet" + +OVS_WAIT_UNTIL([ test 8 = $(grep -c 'acl_log' hv/ovn-controller.log) ]) AT_CHECK([grep 'acl_log' hv/ovn-controller.log | sed 's/.*name=/name=/'], [0], [dnl -name="drop-flow", verdict=drop, severity=alert: tcp,vlan_tci=0x0000,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02,nw_src=192.168.1.2,nw_dst=192.168.1.3,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=4361,tp_dst=81,tcp_flags=syn -name="allow-flow", verdict=allow, severity=info: tcp,vlan_tci=0x0000,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02,nw_src=192.168.1.2,nw_dst=192.168.1.3,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=4363,tp_dst=83,tcp_flags=syn -name="<unnamed>", verdict=allow, severity=info: tcp,vlan_tci=0x0000,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02,nw_src=192.168.1.2,nw_dst=192.168.1.3,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=4365,tp_dst=85,tcp_flags=syn -name="reject-flow", verdict=reject, severity=alert: tcp,vlan_tci=0x0000,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02,nw_src=192.168.1.2,nw_dst=192.168.1.3,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=4367,tp_dst=87,tcp_flags=syn +name="drop-flow", verdict=drop, severity=alert, direction=IN: tcp,vlan_tci=0x0000,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02,nw_src=192.168.1.2,nw_dst=192.168.1.3,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=4361,tp_dst=81,tcp_flags=syn +name="drop-flow", verdict=drop, severity=alert, direction=OUT: tcp,vlan_tci=0x0000,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02,nw_src=192.168.1.2,nw_dst=192.168.1.3,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=4361,tp_dst=181,tcp_flags=syn +name="allow-flow", verdict=allow, severity=info, direction=IN: tcp,vlan_tci=0x0000,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02,nw_src=192.168.1.2,nw_dst=192.168.1.3,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=4363,tp_dst=83,tcp_flags=syn +name="allow-flow", verdict=allow, severity=info, direction=OUT: tcp,vlan_tci=0x0000,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02,nw_src=192.168.1.2,nw_dst=192.168.1.3,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=4363,tp_dst=83,tcp_flags=syn +name="<unnamed>", verdict=allow, severity=info, direction=IN: tcp,vlan_tci=0x0000,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02,nw_src=192.168.1.2,nw_dst=192.168.1.3,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=4365,tp_dst=85,tcp_flags=syn +name="<unnamed>", verdict=allow, severity=info, direction=OUT: tcp,vlan_tci=0x0000,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02,nw_src=192.168.1.2,nw_dst=192.168.1.3,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=4365,tp_dst=85,tcp_flags=syn +name="reject-flow", verdict=reject, severity=alert, direction=IN: tcp,vlan_tci=0x0000,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02,nw_src=192.168.1.2,nw_dst=192.168.1.3,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=4367,tp_dst=87,tcp_flags=syn +name="reject-flow", verdict=reject, severity=alert, direction=OUT: tcp,vlan_tci=0x0000,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02,nw_src=192.168.1.2,nw_dst=192.168.1.3,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=4367,tp_dst=187,tcp_flags=syn ]) OVN_CLEANUP([hv]) diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c index 0795913d3..26c0eaa9c 100644 --- a/utilities/ovn-trace.c +++ b/utilities/ovn-trace.c @@ -2461,8 +2461,10 @@ execute_log(const struct ovnact_log *log, struct flow *uflow, { char *packet_str = flow_to_string(uflow, NULL); ovntrace_node_append(super, OVNTRACE_NODE_TRANSFORMATION, - "LOG: ACL name=%s, verdict=%s, severity=%s, packet=\"%s\"", + "LOG: ACL name=%s, direction=%s, verdict=%s, " + "severity=%s, packet=\"%s\"", log->name ? log->name : "<unnamed>", + log_direction_to_string(log->direction), log_verdict_to_string(log->verdict), log_severity_to_string(log->severity), packet_str);
It's useful to differentiate between ingress and egress pipelines in the ACL logs. To achieve this we expand the "log()" logical action and pass an optional direction. This behavior change is implemented in a backwards compatible way such that it doesn't break existing deployments even in the case when ovn-northd gets updated before ovn-controller. To achieve this, ovn-northd determines first if all chassis in the cluster have been upgraded to a version that supports the new "log()" action format. Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1992641 Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- v2: Add the "direction" after the "verdict" and "severity" fields in the ACL logs. --- NEWS | 2 + controller/chassis.c | 7 +++ include/ovn/actions.h | 1 + include/ovn/features.h | 1 + lib/acl-log.c | 27 +++++++- lib/acl-log.h | 37 ++++++++++- lib/actions.c | 18 +++++- northd/en-lflow.c | 1 + northd/northd.c | 138 +++++++++++++++++++++++++---------------- northd/northd.h | 7 +++ ovn-sb.xml | 6 ++ tests/ovn.at | 68 ++++++++++++++++---- utilities/ovn-trace.c | 4 +- 13 files changed, 246 insertions(+), 71 deletions(-)