| Message ID | 20231018075634.75983-6-amusil@redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Ilya Maximets |
| Headers | show |
| Series | Expose CT limit via DB | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On 10/18/23 09:56, Ales Musil wrote: > Make sure that if any zone limit was set via DB > all zones are forced to be set there also. This > is done by tracking which datapath has zone limit > protection and it is reflected in the dpctl command. > > If the datapath is protected the dpctl command will > return permission error. > > Signed-off-by: Ales Musil <amusil@redhat.com> > --- > v5: Rebase on top of current master. > Address comments from Ilya: > - Add more user friendly error message to the dpctl. > - Fix style related problems. > v4: Rebase on top of current master. > Make the protection datapath wide. > --- > lib/ct-dpif.c | 27 +++++++++++++++++++++++++++ > lib/ct-dpif.h | 2 ++ > lib/dpctl.c | 12 ++++++++++++ > ofproto/ofproto-dpif.c | 14 ++++++++++++++ > ofproto/ofproto-provider.h | 5 +++++ > ofproto/ofproto.c | 11 +++++++++++ > ofproto/ofproto.h | 2 ++ > tests/system-traffic.at | 36 ++++++++++++++++++++++++++++++++++++ > vswitchd/bridge.c | 7 +++++++ > 9 files changed, 116 insertions(+) > > diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c > index 2ee045164..41d2dc4d7 100644 > --- a/lib/ct-dpif.c > +++ b/lib/ct-dpif.c > @@ -23,6 +23,7 @@ > #include "openvswitch/ofp-ct.h" > #include "openvswitch/ofp-parse.h" > #include "openvswitch/vlog.h" > +#include "sset.h" > > VLOG_DEFINE_THIS_MODULE(ct_dpif); > > @@ -32,6 +33,10 @@ struct flags { > const char *name; > }; > > +/* Protection for CT zone limit per datapath. */ > +static struct sset ct_limit_protection = > + SSET_INITIALIZER(&ct_limit_protection); > + > static void ct_dpif_format_counters(struct ds *, > const struct ct_dpif_counters *); > static void ct_dpif_format_timestamp(struct ds *, > @@ -1064,3 +1069,25 @@ ct_dpif_get_features(struct dpif *dpif, enum ct_features *features) > ? dpif->dpif_class->ct_get_features(dpif, features) > : EOPNOTSUPP); > } > + > +void > +ct_dpif_set_zone_limit_protection(struct dpif *dpif, bool protected) > +{ > + if (sset_contains(&ct_limit_protection, dpif->full_name) == protected) { > + return; > + } > + > + if (protected) { > + sset_add(&ct_limit_protection, dpif->full_name); > + } else { > + sset_find_and_delete(&ct_limit_protection, dpif->full_name); > + } > + VLOG_INFO("The CT zone limit protection is %s for \"%s\".", > + protected ? "enabled" : "disabled", dpif->full_name); This message is only useful for users who already use dpctl. And they will see the error while trying to use it. I don't think we need this message in the log file. > +} > + > +bool > +ct_dpif_is_zone_limit_protected(struct dpif *dpif) > +{ > + return sset_contains(&ct_limit_protection, dpif->full_name); > +} > diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h > index c8a7c155e..c3786d5ae 100644 > --- a/lib/ct-dpif.h > +++ b/lib/ct-dpif.h > @@ -350,5 +350,7 @@ int ct_dpif_get_timeout_policy_name(struct dpif *dpif, uint32_t tp_id, > uint16_t dl_type, uint8_t nw_proto, > char **tp_name, bool *is_generic); > int ct_dpif_get_features(struct dpif *dpif, enum ct_features *features); > +void ct_dpif_set_zone_limit_protection(struct dpif *, bool protected); > +bool ct_dpif_is_zone_limit_protected(struct dpif *); > > #endif /* CT_DPIF_H */ > diff --git a/lib/dpctl.c b/lib/dpctl.c > index a8c654747..8c87ff9e8 100644 > --- a/lib/dpctl.c > +++ b/lib/dpctl.c > @@ -2234,6 +2234,12 @@ dpctl_ct_set_limits(int argc, const char *argv[], > ct_dpif_push_zone_limit(&zone_limits, zone, limit, 0); > } > > + if (ct_dpif_is_zone_limit_protected(dpif)) { > + ds_put_cstr(&ds, "the zone limits are set via DB"); I'd suggest something more user-friendly, e.g. "The zone limits are set via database, use 'ovs-vsctl set-zone-limit <...>' instead." > + error = EPERM; > + goto error; > + } > + > error = ct_dpif_set_limits(dpif, &zone_limits); > if (!error) { > ct_dpif_free_zone_limits(&zone_limits); > @@ -2310,6 +2316,12 @@ dpctl_ct_del_limits(int argc, const char *argv[], > } > } > > + if (ct_dpif_is_zone_limit_protected(dpif)) { > + ds_put_cstr(&ds, "the zone limits are set via DB"); The same here, but with 'del-zone-limit'. > + error = EPERM; > + goto error; > + } > + > error = ct_dpif_del_limits(dpif, &zone_limits); > if (!error) { > goto out; > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > index 6a931a806..7c5360b67 100644 > --- a/ofproto/ofproto-dpif.c > +++ b/ofproto/ofproto-dpif.c > @@ -5663,6 +5663,19 @@ ct_zone_limits_commit(struct dpif_backer *backer) > } > } > > +static void > +ct_zone_limit_protection_update(const char *datapath_type, bool protected) > +{ > + struct dpif_backer *backer = shash_find_data(&all_dpif_backers, > + datapath_type); > + if (!backer) { > + return; > + } > + > + ct_dpif_set_zone_limit_protection(backer->dpif, protected); > +} > + > + Too many empty lines. > static void > get_datapath_cap(const char *datapath_type, struct smap *cap) > { > @@ -6953,4 +6966,5 @@ const struct ofproto_class ofproto_dpif_class = { > ct_set_zone_timeout_policy, > ct_del_zone_timeout_policy, > ct_zone_limit_update, > + ct_zone_limit_protection_update, > }; > diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h > index 33fb99280..e1d72b6df 100644 > --- a/ofproto/ofproto-provider.h > +++ b/ofproto/ofproto-provider.h > @@ -1925,6 +1925,11 @@ struct ofproto_class { > /* Updates the CT zone limit for specified zone. */ > void (*ct_zone_limit_update)(const char *dp_type, int32_t zone, > int64_t *limit); > + > + /* Sets the CT zone limit protection to "protected" for the specified > + * datapath type. */ > + void (*ct_zone_limit_protection_update)(const char *dp_type, > + bool protected); > }; > > extern const struct ofproto_class ofproto_dpif_class; > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c > index 649add089..122a06f30 100644 > --- a/ofproto/ofproto.c > +++ b/ofproto/ofproto.c > @@ -1038,6 +1038,17 @@ ofproto_ct_zone_limit_update(const char *datapath_type, int32_t zone_id, > } > } > > +void > +ofproto_ct_zone_limit_protection_update(const char *datapath_type, > + bool protected) > +{ > + datapath_type = ofproto_normalize_type(datapath_type); > + const struct ofproto_class *class = ofproto_class_find__(datapath_type); > + > + if (class && class->ct_zone_limit_protection_update) { > + class->ct_zone_limit_protection_update(datapath_type, protected); > + } > +} > > /* Spanning Tree Protocol (STP) configuration. */ > > diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h > index 7ce6a65e1..1c07df275 100644 > --- a/ofproto/ofproto.h > +++ b/ofproto/ofproto.h > @@ -386,6 +386,8 @@ void ofproto_ct_del_zone_timeout_policy(const char *datapath_type, > uint16_t zone); > void ofproto_ct_zone_limit_update(const char *datapath_type, int32_t zone_id, > int64_t *limit); > +void ofproto_ct_zone_limit_protection_update(const char *datapath_type, > + bool protected); > void ofproto_get_datapath_cap(const char *datapath_type, > struct smap *dp_cap); > > diff --git a/tests/system-traffic.at b/tests/system-traffic.at > index 445a9ffbd..df5c7f3d7 100644 > --- a/tests/system-traffic.at > +++ b/tests/system-traffic.at > @@ -5275,6 +5275,42 @@ OVS_WAIT_UNTIL_EQUAL([ovs-appctl dpctl/ct-get-limits], [dnl > default limit=0 > zone=0,limit=3,count=0]) > > +dnl Try to overwrite the zone limit via dpctl command. > +AT_CHECK([ovs-appctl dpctl/ct-set-limits default=15 zone=3,limit=5 zone=0,limit=5], [2], [ignore], [ignore]) Would be better to not ignore the error message. Same for tests below. > + > +AT_CHECK([ovs-appctl dpctl/ct-get-limits], [0], [dnl > +default limit=0 > +zone=0,limit=3,count=0 > +]) > + > +AT_CHECK([ovs-appctl dpctl/ct-del-limits zone=0], [2], [ignore], [ignore]) > +AT_CHECK([ovs-appctl dpctl/ct-get-limits], [0], [dnl > +default limit=0 > +zone=0,limit=3,count=0 > +]) > + > +AT_CHECK([ovs-vsctl del-zone-limit $DP_TYPE zone=0]) > +AT_CHECK([ovs-vsctl set-zone-limit $DP_TYPE default limit=10]) > +AT_CHECK([ovs-appctl dpctl/ct-get-limits], [0], [dnl > +default limit=10 > +]) > + > +AT_CHECK([ovs-appctl dpctl/ct-set-limits default=15 zone=1,limit=5], [2], [ignore], [ignore]) > + > +dnl Delete all zones from DB, that should remove the protection. > +AT_CHECK([ovs-vsctl del-zone-limit $DP_TYPE default]) > + > +AT_CHECK([ovs-appctl dpctl/ct-set-limits default=15 zone=1,limit=5]) > +AT_CHECK([ovs-appctl dpctl/ct-get-limits], [0], [dnl > +default limit=15 > +zone=1,limit=5,count=0 > +]) > + > +AT_CHECK([ovs-appctl dpctl/ct-del-limits zone=1]) > +AT_CHECK([ovs-appctl dpctl/ct-get-limits], [0], [dnl > +default limit=15 > +]) > + > OVS_TRAFFIC_VSWITCHD_STOP(["dnl > /could not create datapath/d > /(Cannot allocate memory) on packet/d"]) > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c > index 4545556b5..0fe348a77 100644 > --- a/vswitchd/bridge.c > +++ b/vswitchd/bridge.c > @@ -739,6 +739,7 @@ datapath_destroy(struct datapath *dp) > NULL); > } > > + ofproto_ct_zone_limit_protection_update(dp->type, false); > hmap_remove(&all_datapaths, &dp->node); > hmap_destroy(&dp->ct_zones); > free(dp->type); > @@ -751,6 +752,7 @@ static void > ct_zones_reconfigure(struct datapath *dp, struct ovsrec_datapath *dp_cfg) > { > struct ct_zone *ct_zone; > + bool protected = false; > > /* Add new 'ct_zone's or update existing 'ct_zone's based on the database > * state. */ > @@ -784,6 +786,8 @@ ct_zones_reconfigure(struct datapath *dp, struct ovsrec_datapath *dp_cfg) > } > > ct_zone->last_used = idl_seqno; > + > + protected = protected || !!zone_cfg->limit; > } > > /* Purge 'ct_zone's no longer found in the database. */ > @@ -804,6 +808,9 @@ ct_zones_reconfigure(struct datapath *dp, struct ovsrec_datapath *dp_cfg) > dp->ct_default_limit = default_limit; > } > > + protected = protected || !!dp_cfg->ct_zone_default_limit; > + > + ofproto_ct_zone_limit_protection_update(dp->type, protected); > } > > static void
On Wed, Oct 25, 2023 at 2:52 PM Ilya Maximets <i.maximets@ovn.org> wrote: > On 10/18/23 09:56, Ales Musil wrote: > > Make sure that if any zone limit was set via DB > > all zones are forced to be set there also. This > > is done by tracking which datapath has zone limit > > protection and it is reflected in the dpctl command. > > > > If the datapath is protected the dpctl command will > > return permission error. > > > > Signed-off-by: Ales Musil <amusil@redhat.com> > > --- > > v5: Rebase on top of current master. > > Address comments from Ilya: > > - Add more user friendly error message to the dpctl. > > - Fix style related problems. > > v4: Rebase on top of current master. > > Make the protection datapath wide. > > --- > > lib/ct-dpif.c | 27 +++++++++++++++++++++++++++ > > lib/ct-dpif.h | 2 ++ > > lib/dpctl.c | 12 ++++++++++++ > > ofproto/ofproto-dpif.c | 14 ++++++++++++++ > > ofproto/ofproto-provider.h | 5 +++++ > > ofproto/ofproto.c | 11 +++++++++++ > > ofproto/ofproto.h | 2 ++ > > tests/system-traffic.at | 36 ++++++++++++++++++++++++++++++++++++ > > vswitchd/bridge.c | 7 +++++++ > > 9 files changed, 116 insertions(+) > > > > diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c > > index 2ee045164..41d2dc4d7 100644 > > --- a/lib/ct-dpif.c > > +++ b/lib/ct-dpif.c > > @@ -23,6 +23,7 @@ > > #include "openvswitch/ofp-ct.h" > > #include "openvswitch/ofp-parse.h" > > #include "openvswitch/vlog.h" > > +#include "sset.h" > > > > VLOG_DEFINE_THIS_MODULE(ct_dpif); > > > > @@ -32,6 +33,10 @@ struct flags { > > const char *name; > > }; > > > > +/* Protection for CT zone limit per datapath. */ > > +static struct sset ct_limit_protection = > > + SSET_INITIALIZER(&ct_limit_protection); > > + > > static void ct_dpif_format_counters(struct ds *, > > const struct ct_dpif_counters *); > > static void ct_dpif_format_timestamp(struct ds *, > > @@ -1064,3 +1069,25 @@ ct_dpif_get_features(struct dpif *dpif, enum > ct_features *features) > > ? dpif->dpif_class->ct_get_features(dpif, features) > > : EOPNOTSUPP); > > } > > + > > +void > > +ct_dpif_set_zone_limit_protection(struct dpif *dpif, bool protected) > > +{ > > + if (sset_contains(&ct_limit_protection, dpif->full_name) == > protected) { > > + return; > > + } > > + > > + if (protected) { > > + sset_add(&ct_limit_protection, dpif->full_name); > > + } else { > > + sset_find_and_delete(&ct_limit_protection, dpif->full_name); > > + } > > + VLOG_INFO("The CT zone limit protection is %s for \"%s\".", > > + protected ? "enabled" : "disabled", dpif->full_name); > > This message is only useful for users who already use dpctl. > And they will see the error while trying to use it. I don't > think we need this message in the log file. > > > +} > > + > > +bool > > +ct_dpif_is_zone_limit_protected(struct dpif *dpif) > > +{ > > + return sset_contains(&ct_limit_protection, dpif->full_name); > > +} > > diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h > > index c8a7c155e..c3786d5ae 100644 > > --- a/lib/ct-dpif.h > > +++ b/lib/ct-dpif.h > > @@ -350,5 +350,7 @@ int ct_dpif_get_timeout_policy_name(struct dpif > *dpif, uint32_t tp_id, > > uint16_t dl_type, uint8_t nw_proto, > > char **tp_name, bool *is_generic); > > int ct_dpif_get_features(struct dpif *dpif, enum ct_features *features); > > +void ct_dpif_set_zone_limit_protection(struct dpif *, bool protected); > > +bool ct_dpif_is_zone_limit_protected(struct dpif *); > > > > #endif /* CT_DPIF_H */ > > diff --git a/lib/dpctl.c b/lib/dpctl.c > > index a8c654747..8c87ff9e8 100644 > > --- a/lib/dpctl.c > > +++ b/lib/dpctl.c > > @@ -2234,6 +2234,12 @@ dpctl_ct_set_limits(int argc, const char *argv[], > > ct_dpif_push_zone_limit(&zone_limits, zone, limit, 0); > > } > > > > + if (ct_dpif_is_zone_limit_protected(dpif)) { > > + ds_put_cstr(&ds, "the zone limits are set via DB"); > > I'd suggest something more user-friendly, e.g. "The zone limits are set > via database, use 'ovs-vsctl set-zone-limit <...>' instead." > > > + error = EPERM; > > + goto error; > > + } > > + > > error = ct_dpif_set_limits(dpif, &zone_limits); > > if (!error) { > > ct_dpif_free_zone_limits(&zone_limits); > > @@ -2310,6 +2316,12 @@ dpctl_ct_del_limits(int argc, const char *argv[], > > } > > } > > > > + if (ct_dpif_is_zone_limit_protected(dpif)) { > > + ds_put_cstr(&ds, "the zone limits are set via DB"); > > The same here, but with 'del-zone-limit'. > > > + error = EPERM; > > + goto error; > > + } > > + > > error = ct_dpif_del_limits(dpif, &zone_limits); > > if (!error) { > > goto out; > > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > > index 6a931a806..7c5360b67 100644 > > --- a/ofproto/ofproto-dpif.c > > +++ b/ofproto/ofproto-dpif.c > > @@ -5663,6 +5663,19 @@ ct_zone_limits_commit(struct dpif_backer *backer) > > } > > } > > > > +static void > > +ct_zone_limit_protection_update(const char *datapath_type, bool > protected) > > +{ > > + struct dpif_backer *backer = shash_find_data(&all_dpif_backers, > > + datapath_type); > > + if (!backer) { > > + return; > > + } > > + > > + ct_dpif_set_zone_limit_protection(backer->dpif, protected); > > +} > > + > > + > > Too many empty lines. > > > static void > > get_datapath_cap(const char *datapath_type, struct smap *cap) > > { > > @@ -6953,4 +6966,5 @@ const struct ofproto_class ofproto_dpif_class = { > > ct_set_zone_timeout_policy, > > ct_del_zone_timeout_policy, > > ct_zone_limit_update, > > + ct_zone_limit_protection_update, > > }; > > diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h > > index 33fb99280..e1d72b6df 100644 > > --- a/ofproto/ofproto-provider.h > > +++ b/ofproto/ofproto-provider.h > > @@ -1925,6 +1925,11 @@ struct ofproto_class { > > /* Updates the CT zone limit for specified zone. */ > > void (*ct_zone_limit_update)(const char *dp_type, int32_t zone, > > int64_t *limit); > > + > > + /* Sets the CT zone limit protection to "protected" for the > specified > > + * datapath type. */ > > + void (*ct_zone_limit_protection_update)(const char *dp_type, > > + bool protected); > > }; > > > > extern const struct ofproto_class ofproto_dpif_class; > > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c > > index 649add089..122a06f30 100644 > > --- a/ofproto/ofproto.c > > +++ b/ofproto/ofproto.c > > @@ -1038,6 +1038,17 @@ ofproto_ct_zone_limit_update(const char > *datapath_type, int32_t zone_id, > > } > > } > > > > +void > > +ofproto_ct_zone_limit_protection_update(const char *datapath_type, > > + bool protected) > > +{ > > + datapath_type = ofproto_normalize_type(datapath_type); > > + const struct ofproto_class *class = > ofproto_class_find__(datapath_type); > > + > > + if (class && class->ct_zone_limit_protection_update) { > > + class->ct_zone_limit_protection_update(datapath_type, > protected); > > + } > > +} > > > > /* Spanning Tree Protocol (STP) configuration. */ > > > > diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h > > index 7ce6a65e1..1c07df275 100644 > > --- a/ofproto/ofproto.h > > +++ b/ofproto/ofproto.h > > @@ -386,6 +386,8 @@ void ofproto_ct_del_zone_timeout_policy(const char > *datapath_type, > > uint16_t zone); > > void ofproto_ct_zone_limit_update(const char *datapath_type, int32_t > zone_id, > > int64_t *limit); > > +void ofproto_ct_zone_limit_protection_update(const char *datapath_type, > > + bool protected); > > void ofproto_get_datapath_cap(const char *datapath_type, > > struct smap *dp_cap); > > > > diff --git a/tests/system-traffic.at b/tests/system-traffic.at > > index 445a9ffbd..df5c7f3d7 100644 > > --- a/tests/system-traffic.at > > +++ b/tests/system-traffic.at > > @@ -5275,6 +5275,42 @@ OVS_WAIT_UNTIL_EQUAL([ovs-appctl > dpctl/ct-get-limits], [dnl > > default limit=0 > > zone=0,limit=3,count=0]) > > > > +dnl Try to overwrite the zone limit via dpctl command. > > +AT_CHECK([ovs-appctl dpctl/ct-set-limits default=15 zone=3,limit=5 > zone=0,limit=5], [2], [ignore], [ignore]) > > Would be better to not ignore the error message. > Same for tests below. > > > + > > +AT_CHECK([ovs-appctl dpctl/ct-get-limits], [0], [dnl > > +default limit=0 > > +zone=0,limit=3,count=0 > > +]) > > + > > +AT_CHECK([ovs-appctl dpctl/ct-del-limits zone=0], [2], [ignore], > [ignore]) > > +AT_CHECK([ovs-appctl dpctl/ct-get-limits], [0], [dnl > > +default limit=0 > > +zone=0,limit=3,count=0 > > +]) > > + > > +AT_CHECK([ovs-vsctl del-zone-limit $DP_TYPE zone=0]) > > +AT_CHECK([ovs-vsctl set-zone-limit $DP_TYPE default limit=10]) > > +AT_CHECK([ovs-appctl dpctl/ct-get-limits], [0], [dnl > > +default limit=10 > > +]) > > + > > +AT_CHECK([ovs-appctl dpctl/ct-set-limits default=15 zone=1,limit=5], > [2], [ignore], [ignore]) > > + > > +dnl Delete all zones from DB, that should remove the protection. > > +AT_CHECK([ovs-vsctl del-zone-limit $DP_TYPE default]) > > + > > +AT_CHECK([ovs-appctl dpctl/ct-set-limits default=15 zone=1,limit=5]) > > +AT_CHECK([ovs-appctl dpctl/ct-get-limits], [0], [dnl > > +default limit=15 > > +zone=1,limit=5,count=0 > > +]) > > + > > +AT_CHECK([ovs-appctl dpctl/ct-del-limits zone=1]) > > +AT_CHECK([ovs-appctl dpctl/ct-get-limits], [0], [dnl > > +default limit=15 > > +]) > > + > > OVS_TRAFFIC_VSWITCHD_STOP(["dnl > > /could not create datapath/d > > /(Cannot allocate memory) on packet/d"]) > > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c > > index 4545556b5..0fe348a77 100644 > > --- a/vswitchd/bridge.c > > +++ b/vswitchd/bridge.c > > @@ -739,6 +739,7 @@ datapath_destroy(struct datapath *dp) > > NULL); > > } > > > > + ofproto_ct_zone_limit_protection_update(dp->type, false); > > hmap_remove(&all_datapaths, &dp->node); > > hmap_destroy(&dp->ct_zones); > > free(dp->type); > > @@ -751,6 +752,7 @@ static void > > ct_zones_reconfigure(struct datapath *dp, struct ovsrec_datapath > *dp_cfg) > > { > > struct ct_zone *ct_zone; > > + bool protected = false; > > > > /* Add new 'ct_zone's or update existing 'ct_zone's based on the > database > > * state. */ > > @@ -784,6 +786,8 @@ ct_zones_reconfigure(struct datapath *dp, struct > ovsrec_datapath *dp_cfg) > > } > > > > ct_zone->last_used = idl_seqno; > > + > > + protected = protected || !!zone_cfg->limit; > > } > > > > /* Purge 'ct_zone's no longer found in the database. */ > > @@ -804,6 +808,9 @@ ct_zones_reconfigure(struct datapath *dp, struct > ovsrec_datapath *dp_cfg) > > dp->ct_default_limit = default_limit; > > } > > > > + protected = protected || !!dp_cfg->ct_zone_default_limit; > > + > > + ofproto_ct_zone_limit_protection_update(dp->type, protected); > > } > > > > static void > > Thank you for the review, all should be addressed in v6. Thanks, Ales
diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c index 2ee045164..41d2dc4d7 100644 --- a/lib/ct-dpif.c +++ b/lib/ct-dpif.c @@ -23,6 +23,7 @@ #include "openvswitch/ofp-ct.h" #include "openvswitch/ofp-parse.h" #include "openvswitch/vlog.h" +#include "sset.h" VLOG_DEFINE_THIS_MODULE(ct_dpif); @@ -32,6 +33,10 @@ struct flags { const char *name; }; +/* Protection for CT zone limit per datapath. */ +static struct sset ct_limit_protection = + SSET_INITIALIZER(&ct_limit_protection); + static void ct_dpif_format_counters(struct ds *, const struct ct_dpif_counters *); static void ct_dpif_format_timestamp(struct ds *, @@ -1064,3 +1069,25 @@ ct_dpif_get_features(struct dpif *dpif, enum ct_features *features) ? dpif->dpif_class->ct_get_features(dpif, features) : EOPNOTSUPP); } + +void +ct_dpif_set_zone_limit_protection(struct dpif *dpif, bool protected) +{ + if (sset_contains(&ct_limit_protection, dpif->full_name) == protected) { + return; + } + + if (protected) { + sset_add(&ct_limit_protection, dpif->full_name); + } else { + sset_find_and_delete(&ct_limit_protection, dpif->full_name); + } + VLOG_INFO("The CT zone limit protection is %s for \"%s\".", + protected ? "enabled" : "disabled", dpif->full_name); +} + +bool +ct_dpif_is_zone_limit_protected(struct dpif *dpif) +{ + return sset_contains(&ct_limit_protection, dpif->full_name); +} diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h index c8a7c155e..c3786d5ae 100644 --- a/lib/ct-dpif.h +++ b/lib/ct-dpif.h @@ -350,5 +350,7 @@ int ct_dpif_get_timeout_policy_name(struct dpif *dpif, uint32_t tp_id, uint16_t dl_type, uint8_t nw_proto, char **tp_name, bool *is_generic); int ct_dpif_get_features(struct dpif *dpif, enum ct_features *features); +void ct_dpif_set_zone_limit_protection(struct dpif *, bool protected); +bool ct_dpif_is_zone_limit_protected(struct dpif *); #endif /* CT_DPIF_H */ diff --git a/lib/dpctl.c b/lib/dpctl.c index a8c654747..8c87ff9e8 100644 --- a/lib/dpctl.c +++ b/lib/dpctl.c @@ -2234,6 +2234,12 @@ dpctl_ct_set_limits(int argc, const char *argv[], ct_dpif_push_zone_limit(&zone_limits, zone, limit, 0); } + if (ct_dpif_is_zone_limit_protected(dpif)) { + ds_put_cstr(&ds, "the zone limits are set via DB"); + error = EPERM; + goto error; + } + error = ct_dpif_set_limits(dpif, &zone_limits); if (!error) { ct_dpif_free_zone_limits(&zone_limits); @@ -2310,6 +2316,12 @@ dpctl_ct_del_limits(int argc, const char *argv[], } } + if (ct_dpif_is_zone_limit_protected(dpif)) { + ds_put_cstr(&ds, "the zone limits are set via DB"); + error = EPERM; + goto error; + } + error = ct_dpif_del_limits(dpif, &zone_limits); if (!error) { goto out; diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index 6a931a806..7c5360b67 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -5663,6 +5663,19 @@ ct_zone_limits_commit(struct dpif_backer *backer) } } +static void +ct_zone_limit_protection_update(const char *datapath_type, bool protected) +{ + struct dpif_backer *backer = shash_find_data(&all_dpif_backers, + datapath_type); + if (!backer) { + return; + } + + ct_dpif_set_zone_limit_protection(backer->dpif, protected); +} + + static void get_datapath_cap(const char *datapath_type, struct smap *cap) { @@ -6953,4 +6966,5 @@ const struct ofproto_class ofproto_dpif_class = { ct_set_zone_timeout_policy, ct_del_zone_timeout_policy, ct_zone_limit_update, + ct_zone_limit_protection_update, }; diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h index 33fb99280..e1d72b6df 100644 --- a/ofproto/ofproto-provider.h +++ b/ofproto/ofproto-provider.h @@ -1925,6 +1925,11 @@ struct ofproto_class { /* Updates the CT zone limit for specified zone. */ void (*ct_zone_limit_update)(const char *dp_type, int32_t zone, int64_t *limit); + + /* Sets the CT zone limit protection to "protected" for the specified + * datapath type. */ + void (*ct_zone_limit_protection_update)(const char *dp_type, + bool protected); }; extern const struct ofproto_class ofproto_dpif_class; diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index 649add089..122a06f30 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -1038,6 +1038,17 @@ ofproto_ct_zone_limit_update(const char *datapath_type, int32_t zone_id, } } +void +ofproto_ct_zone_limit_protection_update(const char *datapath_type, + bool protected) +{ + datapath_type = ofproto_normalize_type(datapath_type); + const struct ofproto_class *class = ofproto_class_find__(datapath_type); + + if (class && class->ct_zone_limit_protection_update) { + class->ct_zone_limit_protection_update(datapath_type, protected); + } +} /* Spanning Tree Protocol (STP) configuration. */ diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h index 7ce6a65e1..1c07df275 100644 --- a/ofproto/ofproto.h +++ b/ofproto/ofproto.h @@ -386,6 +386,8 @@ void ofproto_ct_del_zone_timeout_policy(const char *datapath_type, uint16_t zone); void ofproto_ct_zone_limit_update(const char *datapath_type, int32_t zone_id, int64_t *limit); +void ofproto_ct_zone_limit_protection_update(const char *datapath_type, + bool protected); void ofproto_get_datapath_cap(const char *datapath_type, struct smap *dp_cap); diff --git a/tests/system-traffic.at b/tests/system-traffic.at index 445a9ffbd..df5c7f3d7 100644 --- a/tests/system-traffic.at +++ b/tests/system-traffic.at @@ -5275,6 +5275,42 @@ OVS_WAIT_UNTIL_EQUAL([ovs-appctl dpctl/ct-get-limits], [dnl default limit=0 zone=0,limit=3,count=0]) +dnl Try to overwrite the zone limit via dpctl command. +AT_CHECK([ovs-appctl dpctl/ct-set-limits default=15 zone=3,limit=5 zone=0,limit=5], [2], [ignore], [ignore]) + +AT_CHECK([ovs-appctl dpctl/ct-get-limits], [0], [dnl +default limit=0 +zone=0,limit=3,count=0 +]) + +AT_CHECK([ovs-appctl dpctl/ct-del-limits zone=0], [2], [ignore], [ignore]) +AT_CHECK([ovs-appctl dpctl/ct-get-limits], [0], [dnl +default limit=0 +zone=0,limit=3,count=0 +]) + +AT_CHECK([ovs-vsctl del-zone-limit $DP_TYPE zone=0]) +AT_CHECK([ovs-vsctl set-zone-limit $DP_TYPE default limit=10]) +AT_CHECK([ovs-appctl dpctl/ct-get-limits], [0], [dnl +default limit=10 +]) + +AT_CHECK([ovs-appctl dpctl/ct-set-limits default=15 zone=1,limit=5], [2], [ignore], [ignore]) + +dnl Delete all zones from DB, that should remove the protection. +AT_CHECK([ovs-vsctl del-zone-limit $DP_TYPE default]) + +AT_CHECK([ovs-appctl dpctl/ct-set-limits default=15 zone=1,limit=5]) +AT_CHECK([ovs-appctl dpctl/ct-get-limits], [0], [dnl +default limit=15 +zone=1,limit=5,count=0 +]) + +AT_CHECK([ovs-appctl dpctl/ct-del-limits zone=1]) +AT_CHECK([ovs-appctl dpctl/ct-get-limits], [0], [dnl +default limit=15 +]) + OVS_TRAFFIC_VSWITCHD_STOP(["dnl /could not create datapath/d /(Cannot allocate memory) on packet/d"]) diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c index 4545556b5..0fe348a77 100644 --- a/vswitchd/bridge.c +++ b/vswitchd/bridge.c @@ -739,6 +739,7 @@ datapath_destroy(struct datapath *dp) NULL); } + ofproto_ct_zone_limit_protection_update(dp->type, false); hmap_remove(&all_datapaths, &dp->node); hmap_destroy(&dp->ct_zones); free(dp->type); @@ -751,6 +752,7 @@ static void ct_zones_reconfigure(struct datapath *dp, struct ovsrec_datapath *dp_cfg) { struct ct_zone *ct_zone; + bool protected = false; /* Add new 'ct_zone's or update existing 'ct_zone's based on the database * state. */ @@ -784,6 +786,8 @@ ct_zones_reconfigure(struct datapath *dp, struct ovsrec_datapath *dp_cfg) } ct_zone->last_used = idl_seqno; + + protected = protected || !!zone_cfg->limit; } /* Purge 'ct_zone's no longer found in the database. */ @@ -804,6 +808,9 @@ ct_zones_reconfigure(struct datapath *dp, struct ovsrec_datapath *dp_cfg) dp->ct_default_limit = default_limit; } + protected = protected || !!dp_cfg->ct_zone_default_limit; + + ofproto_ct_zone_limit_protection_update(dp->type, protected); } static void
Make sure that if any zone limit was set via DB all zones are forced to be set there also. This is done by tracking which datapath has zone limit protection and it is reflected in the dpctl command. If the datapath is protected the dpctl command will return permission error. Signed-off-by: Ales Musil <amusil@redhat.com> --- v5: Rebase on top of current master. Address comments from Ilya: - Add more user friendly error message to the dpctl. - Fix style related problems. v4: Rebase on top of current master. Make the protection datapath wide. --- lib/ct-dpif.c | 27 +++++++++++++++++++++++++++ lib/ct-dpif.h | 2 ++ lib/dpctl.c | 12 ++++++++++++ ofproto/ofproto-dpif.c | 14 ++++++++++++++ ofproto/ofproto-provider.h | 5 +++++ ofproto/ofproto.c | 11 +++++++++++ ofproto/ofproto.h | 2 ++ tests/system-traffic.at | 36 ++++++++++++++++++++++++++++++++++++ vswitchd/bridge.c | 7 +++++++ 9 files changed, 116 insertions(+)