Skip to content

Commit

Permalink
northd: Track max ACL tiers more accurately.
Browse files Browse the repository at this point in the history 
When ACL tiers were introduced, the code kept track of the highest ACL
tier so that when iterating through ACL tiers, we would not attempt to
advance the current tier past the highest configured tier.

Unfortunately, keeping track of a single max ACL tier doesn't work when
ACLs are evaluated in three separate places. ACLs can be evaluated on
ingress before load balancing, on ingress after load balancing, and on
egress. By only keeping track of a single max ACL tier, it means that we
could perform superfluous checks if one stage of ACLs has a higher max
tier than other stages. As an example, if ingress pre-load balancing
ACLs have a maximum tier of 1, and egress ACLs have a maximum tier of 2,
then it means that for all stages of ACLs, we will evaluate tiers 0, 1,
and 2 of ACLs, even though only one stage of ACLs uses tier 2.

From a pure functionality standpoint, this doesn't cause any issues.
Even if we advance the tier past the highest configured value, it
results in a no-op and the same net result happens.

However, the addition of sampling into ACLs has caused an unwanted side
effect. In the example scenario above, let's say the tier 1 ACL in the
ingress pre-load balancing stage evaluates to "pass". After the
evaluation, we send a sample for the "pass" result. We then advance
the tier to 2, then move back to ACL evaluation. There are no tier 2
ACLs, so we move on to the sampling stage again. We then send a second
sample for the previous "pass" result from tier 1. The result is
confusing since we've sent two samples for the same ACL evaluation.

To remedy this, we now track the max ACL tier in each of the stages
where ACLs are evaluated. Now there are no superfluous ACL evaluations
and no superfluous samples sent either.

Reported-at: https://issues.redhat.com/browse/FDP-760
Signed-off-by: Mark Michelson <mmichels@redhat.com>
Acked-by: Ales Musil <amusil@redhat.com>
Signed-off-by: Dumitru Ceara <dceara@redhat.com>
(cherry picked from commit 6aa66f9)
  • Loading branch information
@putnopvut @dceara
putnopvut authored and dceara committed Dec 11, 2024
1 parent 1fd216f commit 177ce93
Show file tree
Hide file tree
Showing 5 changed files with 247 additions and 20 deletions.
54 changes: 46 additions & 8 deletions northd/en-ls-stateful.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -204,16 +204,22 @@ ls_stateful_port_group_handler(struct engine_node *node, void *data_)
ovn_datapaths_find_by_index(input_data.ls_datapaths,
ls_stateful_rec->ls_index);
bool had_stateful_acl = ls_stateful_rec->has_stateful_acl;
uint64_t max_acl_tier = ls_stateful_rec->max_acl_tier;
struct acl_tier old_max = ls_stateful_rec->max_acl_tier;
bool had_acls = ls_stateful_rec->has_acls;
bool modified = false;

ls_stateful_record_reinit(ls_stateful_rec, od, ls_pg,
input_data.ls_port_groups);

struct acl_tier new_max = ls_stateful_rec->max_acl_tier;

/* Using memcmp for struct acl_tier is fine since there is no padding
* in the struct. However, if the structure is changed, the memcmp
* may need to be updated to compare individual struct fields.
*/
if ((had_stateful_acl != ls_stateful_rec->has_stateful_acl)
|| (had_acls != ls_stateful_rec->has_acls)
|| max_acl_tier != ls_stateful_rec->max_acl_tier) {
|| memcmp(&old_max, &new_max, sizeof(old_max))) {
modified = true;
}

Expand Down Expand Up @@ -365,7 +371,8 @@ ls_stateful_record_set_acl_flags(struct ls_stateful_record *ls_stateful_rec,
const struct ls_port_group_table *ls_pgs)
{
ls_stateful_rec->has_stateful_acl = false;
ls_stateful_rec->max_acl_tier = 0;
memset(&ls_stateful_rec->max_acl_tier, 0,
sizeof ls_stateful_rec->max_acl_tier);
ls_stateful_rec->has_acls = false;

if (ls_stateful_record_set_acl_flags_(ls_stateful_rec, od->nbs->acls,
Expand All @@ -391,6 +398,38 @@ ls_stateful_record_set_acl_flags(struct ls_stateful_record *ls_stateful_rec,
}
}

static void
update_ls_max_acl_tier(struct ls_stateful_record *ls_stateful_rec,
const struct nbrec_acl *acl)
{
if (!acl->tier) {
return;
}

uint64_t *tier;

if (!strcmp(acl->direction, "from-lport")) {
if (smap_get_bool(&acl->options, "apply-after-lb", false)) {
tier = &ls_stateful_rec->max_acl_tier.ingress_post_lb;
} else {
tier = &ls_stateful_rec->max_acl_tier.ingress_pre_lb;
}
} else {
tier = &ls_stateful_rec->max_acl_tier.egress;
}

*tier = MAX(*tier, acl->tier);
}

static bool
ls_acl_tiers_are_maxed_out(struct acl_tier *acl_tier,
uint64_t max_allowed_acl_tier)
{
return acl_tier->ingress_post_lb == max_allowed_acl_tier &&
acl_tier->ingress_pre_lb == max_allowed_acl_tier &&
acl_tier->egress == max_allowed_acl_tier;
}

static bool
ls_stateful_record_set_acl_flags_(struct ls_stateful_record *ls_stateful_rec,
struct nbrec_acl **acls,
Expand All @@ -408,16 +447,15 @@ ls_stateful_record_set_acl_flags_(struct ls_stateful_record *ls_stateful_rec,
ls_stateful_rec->has_acls = true;
for (size_t i = 0; i < n_acls; i++) {
const struct nbrec_acl *acl = acls[i];
if (acl->tier > ls_stateful_rec->max_acl_tier) {
ls_stateful_rec->max_acl_tier = acl->tier;
}
update_ls_max_acl_tier(ls_stateful_rec, acl);
if (!ls_stateful_rec->has_stateful_acl
&& !strcmp(acl->action, "allow-related")) {
ls_stateful_rec->has_stateful_acl = true;
}
if (ls_stateful_rec->has_stateful_acl &&
ls_stateful_rec->max_acl_tier ==
nbrec_acl_col_tier.type.value.integer.max) {
ls_acl_tiers_are_maxed_out(
&ls_stateful_rec->max_acl_tier,
nbrec_acl_col_tier.type.value.integer.max)) {
return true;
}
}
Expand Down
8 changes: 7 additions & 1 deletion northd/en-ls-stateful.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,12 @@

struct lflow_ref;

struct acl_tier {
uint64_t ingress_pre_lb;
uint64_t ingress_post_lb;
uint64_t egress;
};

struct ls_stateful_record {
struct hmap_node key_node;

Expand All @@ -46,7 +52,7 @@ struct ls_stateful_record {
bool has_stateful_acl;
bool has_lb_vip;
bool has_acls;
uint64_t max_acl_tier;
struct acl_tier max_acl_tier;

/* 'lflow_ref' is used to reference logical flows generated for
* this ls_stateful record.
Expand Down
47 changes: 36 additions & 11 deletions northd/northd.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7336,49 +7336,55 @@ build_acl_action_lflows(const struct ls_stateful_record *ls_stateful_rec,
S_SWITCH_OUT_ACL_EVAL,
};

uint64_t max_acl_tiers[] = {
ls_stateful_rec->max_acl_tier.ingress_pre_lb,
ls_stateful_rec->max_acl_tier.ingress_post_lb,
ls_stateful_rec->max_acl_tier.egress,
};

ds_clear(actions);
ds_put_cstr(actions, REGBIT_ACL_VERDICT_ALLOW " = 0; "
REGBIT_ACL_VERDICT_DROP " = 0; "
REGBIT_ACL_VERDICT_REJECT " = 0; ");
if (ls_stateful_rec->max_acl_tier) {
ds_put_cstr(actions, REG_ACL_TIER " = 0; ");
}

size_t verdict_len = actions->length;

for (size_t i = 0; i < ARRAY_SIZE(stages); i++) {
enum ovn_stage stage = stages[i];
if (max_acl_tiers[i]) {
ds_put_cstr(actions, REG_ACL_TIER " = 0; ");
}
size_t verdict_tier_len = actions->length;
if (!ls_stateful_rec->has_acls) {
ovn_lflow_add(lflows, od, stage, 0, "1", "next;", lflow_ref);
continue;
}
ds_truncate(actions, verdict_len);
ds_truncate(actions, verdict_tier_len);
ds_put_cstr(actions, "next;");
ovn_lflow_add(lflows, od, stage, 1000,
REGBIT_ACL_VERDICT_ALLOW " == 1", ds_cstr(actions),
lflow_ref);
ds_truncate(actions, verdict_len);
ds_truncate(actions, verdict_tier_len);
ds_put_cstr(actions, debug_implicit_drop_action());
ovn_lflow_add(lflows, od, stage, 1000,
REGBIT_ACL_VERDICT_DROP " == 1",
ds_cstr(actions),
lflow_ref);
bool ingress = ovn_stage_get_pipeline(stage) == P_IN;

ds_truncate(actions, verdict_len);
ds_truncate(actions, verdict_tier_len);
build_acl_reject_action(actions, ingress);

ovn_lflow_metered(lflows, od, stage, 1000,
REGBIT_ACL_VERDICT_REJECT " == 1", ds_cstr(actions),
copp_meter_get(COPP_REJECT, od->nbs->copp,
meter_groups), lflow_ref);

ds_truncate(actions, verdict_len);
ds_truncate(actions, verdict_tier_len);
ds_put_cstr(actions, default_acl_action);
ovn_lflow_add(lflows, od, stage, 0, "1", ds_cstr(actions), lflow_ref);

struct ds tier_actions = DS_EMPTY_INITIALIZER;
for (size_t j = 0; j < ls_stateful_rec->max_acl_tier; j++) {
for (size_t j = 0; j < max_acl_tiers[i]; j++) {
ds_clear(match);
ds_put_format(match, REG_ACL_TIER " == %"PRIuSIZE, j);
ds_clear(&tier_actions);
Expand All @@ -7390,6 +7396,7 @@ build_acl_action_lflows(const struct ls_stateful_record *ls_stateful_rec,
ds_cstr(&tier_actions), lflow_ref);
}
ds_destroy(&tier_actions);
ds_truncate(actions, verdict_len);
}
}

Expand Down Expand Up @@ -7458,6 +7465,21 @@ build_acl_log_related_flows(const struct ovn_datapath *od,
&acl->header_, lflow_ref);
}

static uint64_t
choose_max_acl_tier(const struct ls_stateful_record *ls_stateful_rec,
const struct nbrec_acl *acl)
{
if (!strcmp(acl->direction, "from-lport")) {
if (smap_get_bool(&acl->options, "apply-after-lb", false)) {
return ls_stateful_rec->max_acl_tier.ingress_post_lb;
} else {
return ls_stateful_rec->max_acl_tier.ingress_pre_lb;
}
} else {
return ls_stateful_rec->max_acl_tier.egress;
}
}

static void
build_acls(const struct ls_stateful_record *ls_stateful_rec,
const struct ovn_datapath *od,
Expand Down Expand Up @@ -7654,8 +7676,9 @@ build_acls(const struct ls_stateful_record *ls_stateful_rec,
build_acl_log_related_flows(od, lflows, acl, has_stateful,
meter_groups, &match, &actions,
lflow_ref);
uint64_t max_acl_tier = choose_max_acl_tier(ls_stateful_rec, acl);
consider_acl(lflows, od, acl, has_stateful,
meter_groups, ls_stateful_rec->max_acl_tier,
meter_groups, max_acl_tier,
&match, &actions, lflow_ref);
build_acl_sample_flows(ls_stateful_rec, od, lflows, acl,
&match, &actions, sampling_apps,
Expand All @@ -7673,8 +7696,10 @@ build_acls(const struct ls_stateful_record *ls_stateful_rec,
build_acl_log_related_flows(od, lflows, acl, has_stateful,
meter_groups, &match, &actions,
lflow_ref);
uint64_t max_acl_tier = choose_max_acl_tier(ls_stateful_rec,
acl);
consider_acl(lflows, od, acl, has_stateful,
meter_groups, ls_stateful_rec->max_acl_tier,
meter_groups, max_acl_tier,
&match, &actions, lflow_ref);
build_acl_sample_flows(ls_stateful_rec, od, lflows, acl,
&match, &actions, sampling_apps,
Expand Down
34 changes: 34 additions & 0 deletions tests/ovn-northd.at
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13974,3 +13974,37 @@ OVS_WAIT_UNTIL([rbr_match_builtin])
check rbr_match_custom

AT_CLEANUP

OVN_FOR_EACH_NORTHD_NO_HV([
AT_SETUP([ACL mismatched tiers])
ovn_start

check ovn-nbctl ls-add S1

# Ingress pre-lb ACLs have only a tier 1 ACL configured.
# Ingress post-lb ACLs have tier up to 3 configured.
# Egress ACLs have up to tier 2 configured.
check ovn-nbctl --tier=1 acl-add S1 from-lport 1001 "tcp" allow
check ovn-nbctl --tier=3 --apply-after-lb acl-add S1 from-lport 1001 "tcp" allow
check ovn-nbctl --tier=2 acl-add S1 to-lport 1001 "tcp" allow

# Ingress pre-lb ACLs should only ever increment the tier to 1.
AT_CHECK([ovn-sbctl lflow-list S1 | grep ls_in_acl_action | grep priority=500 | ovn_strip_lflows], [0], [dnl
table=??(ls_in_acl_action ), priority=500 , match=(reg8[[30..31]] == 0), action=(reg8[[30..31]] = 1; next(pipeline=ingress,table=??);)
])

# Ingress post-lb ACLs should increment the tier to 3.
AT_CHECK([ovn-sbctl lflow-list S1 | grep ls_in_acl_after_lb_action | grep priority=500 | ovn_strip_lflows], [0], [dnl
table=??(ls_in_acl_after_lb_action), priority=500 , match=(reg8[[30..31]] == 0), action=(reg8[[30..31]] = 1; next(pipeline=ingress,table=??);)
table=??(ls_in_acl_after_lb_action), priority=500 , match=(reg8[[30..31]] == 1), action=(reg8[[30..31]] = 2; next(pipeline=ingress,table=??);)
table=??(ls_in_acl_after_lb_action), priority=500 , match=(reg8[[30..31]] == 2), action=(reg8[[30..31]] = 3; next(pipeline=ingress,table=??);)
])

# Egress ACLs should increment the tier to 2.
AT_CHECK([ovn-sbctl lflow-list S1 | grep ls_out_acl_action | grep priority=500 | ovn_strip_lflows], [0], [dnl
table=??(ls_out_acl_action ), priority=500 , match=(reg8[[30..31]] == 0), action=(reg8[[30..31]] = 1; next(pipeline=egress,table=??);)
table=??(ls_out_acl_action ), priority=500 , match=(reg8[[30..31]] == 1), action=(reg8[[30..31]] = 2; next(pipeline=egress,table=??);)
])

AT_CLEANUP
])
Loading

0 comments on commit 177ce93

Please sign in to comment.