From d08c6cb8a2581726690b1eb6f78951f9a5abf1a4 Mon Sep 17 00:00:00 2001 From: Rob Hoes Date: Mon, 9 Dec 2024 18:18:26 +0000 Subject: [PATCH 1/4] CA-402921: Relax VIF constraint for PVS proxy The current constraint is that the VIF used for PVS proxy must have device number 0. It turned out that this can be relaxed. It is sufficient to enforce that the VIF is the one with the lowest device number for the VM. Signed-off-by: Rob Hoes --- ocaml/idl/datamodel_errors.ml | 5 +++++ ocaml/xapi-consts/api_errors.ml | 2 ++ ocaml/xapi/xapi_pvs_proxy.ml | 15 ++++++++++++--- 3 files changed, 19 insertions(+), 3 deletions(-) diff --git a/ocaml/idl/datamodel_errors.ml b/ocaml/idl/datamodel_errors.ml index 80b36218f25..81f5ca9fdf3 100644 --- a/ocaml/idl/datamodel_errors.ml +++ b/ocaml/idl/datamodel_errors.ml @@ -1818,6 +1818,11 @@ let _ = "The address specified is already in use by an existing PVS_server object" () ; + error Api_errors.pvs_vif_must_be_first_device [] + ~doc: + "The VIF used by PVS proxy must be the one with the lowest device number" + () ; + error Api_errors.usb_group_contains_vusb ["vusbs"] ~doc:"The USB group contains active VUSBs and cannot be deleted." () ; error Api_errors.usb_group_contains_pusb ["pusbs"] diff --git a/ocaml/xapi-consts/api_errors.ml b/ocaml/xapi-consts/api_errors.ml index 53e9e06176b..0acf87020e3 100644 --- a/ocaml/xapi-consts/api_errors.ml +++ b/ocaml/xapi-consts/api_errors.ml @@ -1246,6 +1246,8 @@ let pvs_proxy_already_present = add_error "PVS_PROXY_ALREADY_PRESENT" let pvs_server_address_in_use = add_error "PVS_SERVER_ADDRESS_IN_USE" +let pvs_vif_must_be_first_device = add_error "PVS_VIF_MUST_BE_FIRST_DEVICE" + let extension_protocol_failure = add_error "EXTENSION_PROTOCOL_FAILURE" let usb_group_contains_vusb = add_error "USB_GROUP_CONTAINS_VUSB" diff --git a/ocaml/xapi/xapi_pvs_proxy.ml b/ocaml/xapi/xapi_pvs_proxy.ml index 3f81ffc783e..5c3545952a5 100644 --- a/ocaml/xapi/xapi_pvs_proxy.ml +++ b/ocaml/xapi/xapi_pvs_proxy.ml @@ -32,9 +32,18 @@ let create ~__context ~site ~vIF = ) ; Helpers.assert_is_valid_ref ~__context ~name:"site" ~ref:site ; Helpers.assert_is_valid_ref ~__context ~name:"VIF" ~ref:vIF ; - let device = Db.VIF.get_device ~__context ~self:vIF in - if device <> "0" then - raise Api_errors.(Server_error (invalid_device, [device])) ; + let device = Db.VIF.get_device ~__context ~self:vIF |> int_of_string in + let min_device = + let open Xapi_database.Db_filter_types in + let vm = Db.VIF.get_VM ~__context ~self:vIF in + Db.VIF.get_records_where ~__context + ~expr:(Eq (Field "VM", Literal (Ref.string_of vm))) + |> List.fold_left + (fun m (_, {API.vIF_device= d; _}) -> min m (int_of_string d)) + device + in + if device <> min_device then + raise Api_errors.(Server_error (pvs_vif_must_be_first_device, [])) ; let pvs_proxy = Ref.make () in let uuid = Uuidx.(to_string (make ())) in Db.PVS_proxy.create ~__context ~ref:pvs_proxy ~uuid ~site ~vIF From 2f2ee291641bb7e281533eeb0e6905c989bc7c00 Mon Sep 17 00:00:00 2001 From: Rob Hoes Date: Wed, 11 Dec 2024 16:54:52 +0000 Subject: [PATCH 2/4] CA-402921: Update PVS-proxy tests Signed-off-by: Rob Hoes --- ocaml/tests/test_pvs_proxy.ml | 31 +++++++++++++++++++++++++++---- 1 file changed, 27 insertions(+), 4 deletions(-) diff --git a/ocaml/tests/test_pvs_proxy.ml b/ocaml/tests/test_pvs_proxy.ml index 2c102c0ca32..47e41ab3c24 100644 --- a/ocaml/tests/test_pvs_proxy.ml +++ b/ocaml/tests/test_pvs_proxy.ml @@ -34,12 +34,28 @@ let test_create_ok () = "test_create_ok testing get_VIF" vIF (Db.PVS_proxy.get_VIF ~__context ~self:pvs_proxy) -let test_create_invalid_device () = +let test_create_ok_lowest_device_number () = let __context = T.make_test_database () in let site = T.make_pvs_site ~__context () in let vIF = T.make_vif ~__context ~device:"1" () in - Alcotest.check_raises "test_create_invalid_device should raise invalid_device" - Api_errors.(Server_error (invalid_device, ["1"])) + let _vIF' = T.make_vif ~__context ~device:"2" () in + let pvs_proxy = Xapi_pvs_proxy.create ~__context ~site ~vIF in + Alcotest.(check (Alcotest_comparators.ref ())) + "test_create_ok testing get_site" site + (Db.PVS_proxy.get_site ~__context ~self:pvs_proxy) ; + Alcotest.(check (Alcotest_comparators.ref ())) + "test_create_ok testing get_VIF" vIF + (Db.PVS_proxy.get_VIF ~__context ~self:pvs_proxy) + +let test_create_not_lowest_device_number () = + let __context = T.make_test_database () in + let site = T.make_pvs_site ~__context () in + let _vIF' = T.make_vif ~__context ~device:"0" () in + let vIF = T.make_vif ~__context ~device:"1" () in + Alcotest.check_raises + "test_create_not_lowest_device_number should raise \ + pvs_vif_must_be_first_device" + Api_errors.(Server_error (pvs_vif_must_be_first_device, [])) (fun () -> ignore (Xapi_pvs_proxy.create ~__context ~site ~vIF)) let test_create_invalid_site () = @@ -103,7 +119,14 @@ let test = [ ("test_unlicensed", `Quick, test_unlicensed) ; ("test_create_ok", `Quick, test_create_ok) - ; ("test_create_invalid_device", `Quick, test_create_invalid_device) + ; ( "test_create_ok_lowest_device_number" + , `Quick + , test_create_ok_lowest_device_number + ) + ; ( "test_create_not_lowest_device_number" + , `Quick + , test_create_not_lowest_device_number + ) ; ("test_create_invalid_site", `Quick, test_create_invalid_site) ; ("test_create_invalid_vif", `Quick, test_create_invalid_vif) ; ("test_destroy", `Quick, test_destroy) From 14406ba90312c595c2a75e7f07eb25b8dba66892 Mon Sep 17 00:00:00 2001 From: Rob Hoes Date: Wed, 18 Dec 2024 13:15:06 +0000 Subject: [PATCH 3/4] CA-402921: Restrict VIF.create When creating a new VIF and there is already a VIF with PVS_proxy, check that the new VIF does not have a lower device number than the PVS_proxy VIF. Signed-off-by: Rob Hoes --- ocaml/idl/datamodel_errors.ml | 7 +++++++ ocaml/xapi-consts/api_errors.ml | 3 +++ ocaml/xapi/xapi_vif_helpers.ml | 37 +++++++++++++++++++++++++++++---- 3 files changed, 43 insertions(+), 4 deletions(-) diff --git a/ocaml/idl/datamodel_errors.ml b/ocaml/idl/datamodel_errors.ml index 81f5ca9fdf3..fed2f830db1 100644 --- a/ocaml/idl/datamodel_errors.ml +++ b/ocaml/idl/datamodel_errors.ml @@ -1823,6 +1823,13 @@ let _ = "The VIF used by PVS proxy must be the one with the lowest device number" () ; + error Api_errors.pvs_proxy_present_on_higher_vif_device ["device"] + ~doc: + "The VM has a VIF, with a higher device number than the new VIF, that \ + uses a PVS proxy. The VIF used by PVS proxy must be the one with the \ + lowest device number." + () ; + error Api_errors.usb_group_contains_vusb ["vusbs"] ~doc:"The USB group contains active VUSBs and cannot be deleted." () ; error Api_errors.usb_group_contains_pusb ["pusbs"] diff --git a/ocaml/xapi-consts/api_errors.ml b/ocaml/xapi-consts/api_errors.ml index 0acf87020e3..54bdd6f6660 100644 --- a/ocaml/xapi-consts/api_errors.ml +++ b/ocaml/xapi-consts/api_errors.ml @@ -1248,6 +1248,9 @@ let pvs_server_address_in_use = add_error "PVS_SERVER_ADDRESS_IN_USE" let pvs_vif_must_be_first_device = add_error "PVS_VIF_MUST_BE_FIRST_DEVICE" +let pvs_proxy_present_on_higher_vif_device = + add_error "PVS_PROXY_PRESENT_ON_HIGHER_VIF_DEVICE" + let extension_protocol_failure = add_error "EXTENSION_PROTOCOL_FAILURE" let usb_group_contains_vusb = add_error "USB_GROUP_CONTAINS_VUSB" diff --git a/ocaml/xapi/xapi_vif_helpers.ml b/ocaml/xapi/xapi_vif_helpers.ml index 4a469b84368..da6ede482fa 100644 --- a/ocaml/xapi/xapi_vif_helpers.ml +++ b/ocaml/xapi/xapi_vif_helpers.ml @@ -287,13 +287,42 @@ let create ~__context ~device ~network ~vM ~mAC ~mTU ~other_config ) ; (* Check to make sure the device is unique *) Xapi_stdext_threads.Threadext.Mutex.execute m (fun () -> - let all = Db.VM.get_VIFs ~__context ~self:vM in - let all_devices = - List.map (fun self -> Db.VIF.get_device ~__context ~self) all + let all_vifs_with_devices = + Db.VM.get_VIFs ~__context ~self:vM + |> List.map (fun self -> + (self, int_of_string (Db.VIF.get_device ~__context ~self)) + ) in - if List.mem device all_devices then + let new_device = int_of_string device in + if List.exists (fun (_, d) -> d = new_device) all_vifs_with_devices then raise (Api_errors.Server_error (Api_errors.device_already_exists, [device])) ; + + (* If the VM uses a PVS_proxy, then the proxy _must_ be associated with + the VIF that has the lowest device number. Check that the new VIF + respects this. *) + ( match all_vifs_with_devices with + | [] -> + () + | hd :: tl -> + let min_vif, min_device = + List.fold_left + (fun ((_, d) as v) ((_, d') as v') -> if d' < d then v' else v) + hd tl + in + let vm_has_pvs_proxy = + Pvs_proxy_control.find_proxy_for_vif ~__context ~vif:min_vif <> None + in + if vm_has_pvs_proxy && new_device < min_device then + raise + Api_errors.( + Server_error + ( pvs_proxy_present_on_higher_vif_device + , [Printf.sprintf "%d" min_device] + ) + ) + ) ; + let metrics = Ref.make () and metrics_uuid = Uuidx.to_string (Uuidx.make ()) in Db.VIF_metrics.create ~__context ~ref:metrics ~uuid:metrics_uuid From 819279248403e034f8e9a9e5778e852d9c5bb58b Mon Sep 17 00:00:00 2001 From: Rob Hoes Date: Wed, 18 Dec 2024 13:16:07 +0000 Subject: [PATCH 4/4] CA-402921: Add some unit tests for Xapi_vif_helpers Signed-off-by: Rob Hoes --- ocaml/tests/suite_alcotest.ml | 1 + ocaml/tests/test_vif_helpers.ml | 90 +++++++++++++++++++++++++++++++++ 2 files changed, 91 insertions(+) create mode 100644 ocaml/tests/test_vif_helpers.ml diff --git a/ocaml/tests/suite_alcotest.ml b/ocaml/tests/suite_alcotest.ml index c2e422c2379..7c425e39639 100644 --- a/ocaml/tests/suite_alcotest.ml +++ b/ocaml/tests/suite_alcotest.ml @@ -37,6 +37,7 @@ let () = ; ("Test_pvs_site", Test_pvs_site.test) ; ("Test_pvs_proxy", Test_pvs_proxy.test) ; ("Test_pvs_server", Test_pvs_server.test) + ; ("Test_vif_helpers", Test_vif_helpers.test) ; ("Test_vm_memory_constraints", Test_vm_memory_constraints.test) ; ("Test_xapi_xenops", Test_xapi_xenops.test) ; ("Test_network_event_loop", Test_network_event_loop.test) diff --git a/ocaml/tests/test_vif_helpers.ml b/ocaml/tests/test_vif_helpers.ml new file mode 100644 index 00000000000..2e92a3d7f01 --- /dev/null +++ b/ocaml/tests/test_vif_helpers.ml @@ -0,0 +1,90 @@ +(* + * Copyright (C) Citrix Systems Inc. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU Lesser General Public License as published + * by the Free Software Foundation; version 2.1 only. with the special + * exception on linking described in file LICENSE. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Lesser General Public License for more details. + *) + +module T = Test_common + +(* Function under test *) +let create ~__context ~device ~network ~vM ?(mAC = "00:00:00:00:00:00") + ?(mTU = 1500L) ?(qos_algorithm_type = "") ?(qos_algorithm_params = []) + ?(currently_attached = false) ?(other_config = []) + ?(locking_mode = `unlocked) ?(ipv4_allowed = []) ?(ipv6_allowed = []) + ?(ipv4_configuration_mode = `None) ?(ipv4_addresses = []) + ?(ipv4_gateway = "") ?(ipv6_configuration_mode = `None) + ?(ipv6_addresses = []) ?(ipv6_gateway = "") () = + Xapi_vif_helpers.create ~__context ~device ~network ~vM ~mAC ~mTU + ~other_config ~qos_algorithm_type ~qos_algorithm_params ~currently_attached + ~locking_mode ~ipv4_allowed ~ipv6_allowed ~ipv4_configuration_mode + ~ipv4_addresses ~ipv4_gateway ~ipv6_configuration_mode ~ipv6_addresses + ~ipv6_gateway + +let test_create_ok () = + let __context = T.make_test_database () in + let vM = T.make_vm ~__context () in + let network = T.make_network ~__context () in + let vif = create ~__context ~device:"0" ~network ~vM () in + Alcotest.(check (Alcotest_comparators.ref ())) + "test_create_ok testing get_VM" vM + (Db.VIF.get_VM ~__context ~self:vif) + +let test_create_duplicate_device () = + let __context = T.make_test_database () in + let vM = T.make_vm ~__context () in + let network = T.make_network ~__context () in + let _vif0 = T.make_vif ~__context ~device:"0" ~vM ~network () in + Alcotest.check_raises + "test_create_duplicate_device should raise device_already_exists" + Api_errors.(Server_error (device_already_exists, ["0"])) + @@ fun () -> + let _ = create ~__context ~device:"0" ~network ~vM () in + () + +let test_create_with_pvs_proxy_ok () = + let __context = T.make_test_database () in + let vM = T.make_vm ~__context () in + let network = T.make_network ~__context () in + let vIF = T.make_vif ~__context ~device:"0" ~vM ~network () in + let _vIF2 = T.make_vif ~__context ~device:"2" ~vM ~network () in + let site = T.make_pvs_site ~__context () in + let _pvs_proxy = T.make_pvs_proxy ~__context ~site ~vIF () in + let vif1 = create ~__context ~device:"1" ~network ~vM () in + Alcotest.(check (Alcotest_comparators.ref ())) + "test_create_with_pvs_proxy_ok testing get_VM" vM + (Db.VIF.get_VM ~__context ~self:vif1) + +let test_create_with_pvs_proxy_not_ok () = + let __context = T.make_test_database () in + let vM = T.make_vm ~__context () in + let network = T.make_network ~__context () in + let vIF = T.make_vif ~__context ~device:"1" ~vM ~network () in + let _vIF2 = T.make_vif ~__context ~device:"2" ~vM ~network () in + let site = T.make_pvs_site ~__context () in + let _pvs_proxy = T.make_pvs_proxy ~__context ~site ~vIF () in + Alcotest.check_raises + "test_create_with_pvs_proxy_not_ok should raise \ + pvs_proxy_present_on_higher_vif_device" + Api_errors.(Server_error (pvs_proxy_present_on_higher_vif_device, ["1"])) + @@ fun () -> + let _ = create ~__context ~device:"0" ~network ~vM () in + () + +let test = + [ + ("test_create_ok", `Quick, test_create_ok) + ; ("test_create_duplicate_device", `Quick, test_create_duplicate_device) + ; ("test_create_with_pvs_proxy_ok", `Quick, test_create_with_pvs_proxy_ok) + ; ( "test_create_with_pvs_proxy_not_ok" + , `Quick + , test_create_with_pvs_proxy_not_ok + ) + ]