| Message ID | 20220427123834.2693431-1-mheib@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,v3] controller/binding: prevent claiming container lport to ovs bridge | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
On Wed, Apr 27, 2022 at 8:39 AM Mohammad Heib <mheib@redhat.com> wrote: > currently ovn-controller allow users to claim lport of type container > to ovs bridge which is invalid use-case. > > This patch will prevent such invalid use-cases by ignoring the claiming > requests for container lports and will throw a warning message to the > controller logs. > > Signed-off-by: Mohammad Heib <mheib@redhat.com> > Hi Mohammad, Thanks for v3. Looks like we don't need this patch anymore after your patch - https://github.com/ovn-org/ovn/commit/cd3b685043fa9758df3665bf3e3fc972048698a6 Also I do have a few concerns with this patch. lport_lookup_by_name() will be called one extra time for every port binding claim. This could be expensive. Presently when we have an ovs port with external_ids:iface-id set, we create a local_binding object for it. And this patch now skips it. Can you please test it out and see if the issue is still seen with the present main branch ? Thanks Numan --- > controller/binding.c | 34 ++++++++++++++++++++++++++++++++++ > tests/ovn.at | 11 +++++++++++ > 2 files changed, 45 insertions(+) > > diff --git a/controller/binding.c b/controller/binding.c > index 7281b0485..db0a6f118 100644 > --- a/controller/binding.c > +++ b/controller/binding.c > @@ -970,6 +970,32 @@ claim_lport(const struct sbrec_port_binding *pb, > return true; > } > > +/* Validate that "iface_rec:external_ids:iface-id" belongs to a lport > + * that can be claimed to OVS bridge. > + * > + * If the lport type is LP_CONTAINER this function will throw a warning > + * message to the logs and return "false". > + * > + * Otherwise, return "true". > + */ > +static bool > +valid_iface_claim(const char *iface_id, > + struct binding_ctx_in *b_ctx_in) > +{ > + const struct sbrec_port_binding *pb = NULL; > + pb = lport_lookup_by_name(b_ctx_in->sbrec_port_binding_by_name, > + iface_id); > + if (pb && (get_lport_type(pb) == LP_CONTAINER)) { > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > + VLOG_WARN_RL(&rl, "Can't claim lport %s of type container to" > + " OVS bridge,\nplease remove the lport parent_name" > + " before claiming it.", pb->logical_port); > + return false; > + } > + > + return true; > +} > + > /* Returns false if lport is not released due to 'sb_readonly'. > * Returns true otherwise. > * > @@ -1497,6 +1523,10 @@ build_local_bindings(struct binding_ctx_in > *b_ctx_in, > struct local_binding *lbinding = > local_binding_find(local_bindings, iface_id); > if (!lbinding) { > + if (!valid_iface_claim(iface_id, b_ctx_in)) { > + continue; > + } > + > lbinding = local_binding_create(iface_id, iface_rec); > local_binding_add(local_bindings, lbinding); > } else { > @@ -2022,6 +2052,10 @@ binding_handle_ovs_interface_changes(struct > binding_ctx_in *b_ctx_in, > int64_t ofport = iface_rec->n_ofport ? *iface_rec->ofport : 0; > if (iface_id && ofport > 0 && > is_iface_in_int_bridge(iface_rec, b_ctx_in->br_int)) { > + if (!valid_iface_claim(iface_id, b_ctx_in)) { > + continue; > + } > + > handled = consider_iface_claim(iface_rec, iface_id, b_ctx_in, > b_ctx_out, qos_map_ptr); > if (!handled) { > diff --git a/tests/ovn.at b/tests/ovn.at > index 34b6abfc0..bc59f3f13 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -28535,6 +28535,11 @@ check ovn-nbctl --wait=sb set logical_switch_port > vm1 parent_name=vm-cont1 > > wait_for_ports_up > > +# Try to claim container port to ovs > +check ovn-nbctl set logical_switch_port vm-cont2 parent_name=vm2 > +check as hv1 ovs-vsctl set Interface vm1 external_ids:iface-id=vm-cont2 > +AT_CHECK([test 1 = `cat hv1/ovn-controller.log |grep -c "claim lport > vm-cont2 of type container"`]) > + > # Delete vm1, vm-cont1 and vm-cont2 and recreate again. > check ovn-nbctl lsp-del vm1 > check ovn-nbctl lsp-del vm-cont1 > @@ -28546,6 +28551,12 @@ check ovn-nbctl lsp-add ls vm-cont1 vm1 1 > check ovn-nbctl lsp-add ls vm-cont2 vm1 2 > > wait_for_ports_up > +check as hv1 ovn-appctl -t ovn-controller debug/pause > +# Try to claim container port to ovs with recompute > +check ovn-nbctl set logical_switch_port vm-cont2 parent_name=vm2 > +check as hv1 ovs-vsctl set Interface vm1 external_ids:iface-id=vm-cont2 > +check as hv1 ovn-appctl -t ovn-controller debug/resume > +AT_CHECK([test 1 = `cat hv1/ovn-controller.log |grep -c "claim lport > vm-cont2 of type container"`]) > > # Make vm1 as a child port of some non existent lport - foo. vm1, > vm1-cont1 and > # vm1-cont2 should be released. > -- > 2.34.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
diff --git a/controller/binding.c b/controller/binding.c index 7281b0485..db0a6f118 100644 --- a/controller/binding.c +++ b/controller/binding.c @@ -970,6 +970,32 @@ claim_lport(const struct sbrec_port_binding *pb, return true; } +/* Validate that "iface_rec:external_ids:iface-id" belongs to a lport + * that can be claimed to OVS bridge. + * + * If the lport type is LP_CONTAINER this function will throw a warning + * message to the logs and return "false". + * + * Otherwise, return "true". + */ +static bool +valid_iface_claim(const char *iface_id, + struct binding_ctx_in *b_ctx_in) +{ + const struct sbrec_port_binding *pb = NULL; + pb = lport_lookup_by_name(b_ctx_in->sbrec_port_binding_by_name, + iface_id); + if (pb && (get_lport_type(pb) == LP_CONTAINER)) { + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); + VLOG_WARN_RL(&rl, "Can't claim lport %s of type container to" + " OVS bridge,\nplease remove the lport parent_name" + " before claiming it.", pb->logical_port); + return false; + } + + return true; +} + /* Returns false if lport is not released due to 'sb_readonly'. * Returns true otherwise. * @@ -1497,6 +1523,10 @@ build_local_bindings(struct binding_ctx_in *b_ctx_in, struct local_binding *lbinding = local_binding_find(local_bindings, iface_id); if (!lbinding) { + if (!valid_iface_claim(iface_id, b_ctx_in)) { + continue; + } + lbinding = local_binding_create(iface_id, iface_rec); local_binding_add(local_bindings, lbinding); } else { @@ -2022,6 +2052,10 @@ binding_handle_ovs_interface_changes(struct binding_ctx_in *b_ctx_in, int64_t ofport = iface_rec->n_ofport ? *iface_rec->ofport : 0; if (iface_id && ofport > 0 && is_iface_in_int_bridge(iface_rec, b_ctx_in->br_int)) { + if (!valid_iface_claim(iface_id, b_ctx_in)) { + continue; + } + handled = consider_iface_claim(iface_rec, iface_id, b_ctx_in, b_ctx_out, qos_map_ptr); if (!handled) { diff --git a/tests/ovn.at b/tests/ovn.at index 34b6abfc0..bc59f3f13 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -28535,6 +28535,11 @@ check ovn-nbctl --wait=sb set logical_switch_port vm1 parent_name=vm-cont1 wait_for_ports_up +# Try to claim container port to ovs +check ovn-nbctl set logical_switch_port vm-cont2 parent_name=vm2 +check as hv1 ovs-vsctl set Interface vm1 external_ids:iface-id=vm-cont2 +AT_CHECK([test 1 = `cat hv1/ovn-controller.log |grep -c "claim lport vm-cont2 of type container"`]) + # Delete vm1, vm-cont1 and vm-cont2 and recreate again. check ovn-nbctl lsp-del vm1 check ovn-nbctl lsp-del vm-cont1 @@ -28546,6 +28551,12 @@ check ovn-nbctl lsp-add ls vm-cont1 vm1 1 check ovn-nbctl lsp-add ls vm-cont2 vm1 2 wait_for_ports_up +check as hv1 ovn-appctl -t ovn-controller debug/pause +# Try to claim container port to ovs with recompute +check ovn-nbctl set logical_switch_port vm-cont2 parent_name=vm2 +check as hv1 ovs-vsctl set Interface vm1 external_ids:iface-id=vm-cont2 +check as hv1 ovn-appctl -t ovn-controller debug/resume +AT_CHECK([test 1 = `cat hv1/ovn-controller.log |grep -c "claim lport vm-cont2 of type container"`]) # Make vm1 as a child port of some non existent lport - foo. vm1, vm1-cont1 and # vm1-cont2 should be released.
currently ovn-controller allow users to claim lport of type container to ovs bridge which is invalid use-case. This patch will prevent such invalid use-cases by ignoring the claiming requests for container lports and will throw a warning message to the controller logs. Signed-off-by: Mohammad Heib <mheib@redhat.com> --- controller/binding.c | 34 ++++++++++++++++++++++++++++++++++ tests/ovn.at | 11 +++++++++++ 2 files changed, 45 insertions(+)