Skip to content

CP-53477 Update API/CLI to Support Dom0 SSH Control #6385

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ocaml/idl/datamodel_common.ml
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ open Datamodel_roles
to leave a gap for potential hotfixes needing to increment the schema version.*)
let schema_major_vsn = 5

let schema_minor_vsn = 786
let schema_minor_vsn = 787

(* Historical schema versions just in case this is useful later *)
let rio_schema_major_vsn = 5
Expand Down
3 changes: 3 additions & 0 deletions ocaml/idl/datamodel_errors.ml
Original file line number Diff line number Diff line change
Expand Up @@ -2043,6 +2043,9 @@ let _ =
error Api_errors.host_driver_no_hardware ["driver variant"]
~doc:"No hardware present for this host driver variant" () ;

error Api_errors.set_console_timeout_failed ["timeout"]
~doc:"Failed to set SSH&VNC idle session timeout." () ;

error Api_errors.tls_verification_not_enabled_in_pool []
~doc:
"TLS verification has not been enabled in the pool successfully, please \
Expand Down
39 changes: 39 additions & 0 deletions ocaml/idl/datamodel_host.ml
Original file line number Diff line number Diff line change
Expand Up @@ -2368,6 +2368,29 @@ let disable_ssh =
~params:[(Ref _host, "self", "The host")]
~allowed_roles:_R_POOL_ADMIN ()

let set_ssh_enable_timeout =
call ~name:"set_ssh_enable_timeout " ~lifecycle:[]
Copy link
Contributor

@BengangY BengangY Mar 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the SDK failure may be caused by the blank space after set_ssh_enable_timeout ...

~doc:"Set the SSH service enabled timeout for the host"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the enable or enabled timeout? There is a contradiciton between the documentation and the name of the function.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

set_ssh_enabled_timeout

Copy link
Contributor Author

@LunfanZhang LunfanZhang Mar 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original definition is set_ssh_enable_timeout, but it is conflict with the db setter for new fields ssh_enabled_timeout.
How about set_enabled_ssh_timeout or set_ssh_timeout?

~params:
[
(Ref _host, "self", "The host")
; ( Int
, "timeout"
, "The SSH enabled timeout in minutes (0 means no timeout, max 2880)"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most time values are seconds in the API. So this is likely to cause errors and there is no hint in the names that this is minutes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sense, I will update to sec

)
]
~allowed_roles:_R_POOL_ADMIN ()

let set_console_timeout =
call ~name:"set_console_timeout" ~lifecycle:[]
~doc:"Set the idle SSH/VNC session timeout for the host"
~params:
[
(Ref _host, "self", "The host")
; (Int, "console_timeout", "The idle console timeout in seconds")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Case in point: here we use seconds.

]
~allowed_roles:_R_POOL_ADMIN ()

let latest_synced_updates_applied_state =
Enum
( "latest_synced_updates_applied_state"
Expand Down Expand Up @@ -2527,6 +2550,8 @@ let t =
; emergency_clear_mandatory_guidance
; enable_ssh
; disable_ssh
; set_ssh_enable_timeout
; set_console_timeout
]
~contents:
([
Expand Down Expand Up @@ -2964,6 +2989,20 @@ let t =
~default_value:(Some (VString "")) "last_update_hash"
"The SHA256 checksum of updateinfo of the most recently applied \
update on the host"
; field ~qualifier:RW ~lifecycle:[] ~ty:Bool
~default_value:(Some (VBool true)) "ssh_enabled"
"True if SSH access is enabled for the host"
; field ~qualifier:RW ~lifecycle:[] ~ty:Int
~default_value:(Some (VInt 0L)) "ssh_enabled_timeout"
"The timeout in minutes after which SSH access will be \
automatically disabled (0 means never)"
; field ~qualifier:DynamicRO ~lifecycle:[] ~ty:DateTime
~default_value:(Some (VDateTime Date.epoch)) "ssh_expiry"
"The time when SSH access will expire"
; field ~qualifier:RW ~lifecycle:[] ~ty:Int
~default_value:(Some (VInt 0L)) "console_idle_timeout"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do ssh_enabled_timeout and console_idle_timeout have different unit?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a user perspective, the SSH service timeout should be longer than the idle session timeout. Generally, users might set ssh_enabled_timeout to 30 minutes and console_idle_timeout to 600 seconds, as defined in the REQ. I assume this is reasonable.

"The timeout in seconds after which idle console will be \
automatically terminated (0 means never)"
]
)
()
29 changes: 29 additions & 0 deletions ocaml/idl/datamodel_pool.ml
Original file line number Diff line number Diff line change
Expand Up @@ -1571,6 +1571,33 @@ let disable_ssh =
~params:[(Ref _pool, "self", "The pool")]
~allowed_roles:_R_POOL_ADMIN ()

let set_ssh_enable_timeout =
call ~name:"set_ssh_enable_timeout " ~lifecycle:[]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the space in tail.

~doc:"Set the SSH enabled timeout for the hosts in the pool"
~params:
[
(Ref _pool, "self", "The pool")
; ( Int
, "timeout"
, "The SSH enabled timeout in minutes. (0 means no timeout, max 2880)"
)
]
~allowed_roles:_R_POOL_ADMIN ()

let set_console_timeout =
call ~name:"set_console_timeout" ~lifecycle:[]
~doc:"Set the idle SSH/VNC session timeout for the pool"
~params:
[
(Ref _pool, "self", "The pool")
; ( Int
, "console_timeout"
, "The idle SSH/VNC session timeout in seconds. A value of 0 means no \
timeout."
)
]
~allowed_roles:_R_POOL_ADMIN ()

(** A pool class *)
let t =
create_obj ~in_db:true
Expand Down Expand Up @@ -1667,6 +1694,8 @@ let t =
; get_guest_secureboot_readiness
; enable_ssh
; disable_ssh
; set_ssh_enable_timeout
; set_console_timeout
]
~contents:
([
Expand Down
2 changes: 1 addition & 1 deletion ocaml/idl/schematest.ml
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ let hash x = Digest.string x |> Digest.to_hex
(* BEWARE: if this changes, check that schema has been bumped accordingly in
ocaml/idl/datamodel_common.ml, usually schema_minor_vsn *)

let last_known_schema_hash = "ad67a64cd47cdea32085518c1fb38d27"
let last_known_schema_hash = "42fd0bbdc613092390c32e04e35a24d2"

let current_schema_hash : string =
let open Datamodel_types in
Expand Down
3 changes: 2 additions & 1 deletion ocaml/tests/common/test_common.ml
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,8 @@ let make_host2 ~__context ?(ref = Ref.make ()) ?(uuid = make_uuid ())
~last_software_update:(Xapi_host.get_servertime ~__context ~host:ref)
~recommended_guidances:[] ~latest_synced_updates_applied:`unknown
~pending_guidances_recommended:[] ~pending_guidances_full:[]
~last_update_hash:"" ;
~last_update_hash:"" ~ssh_enabled:true ~ssh_enabled_timeout:0L
~ssh_expiry:Date.epoch ~console_idle_timeout:0L ;
ref

let make_pif ~__context ~network ~host ?(device = "eth0")
Expand Down
70 changes: 70 additions & 0 deletions ocaml/xapi-cli-server/records.ml
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,20 @@ let get_pbds_host rpc session_id pbds =
let get_sr_host rpc session_id record =
get_pbds_host rpc session_id record.API.sR_PBDs

let get_unified_field ~rpc ~session_id ~getter ~transform ~default =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of the function? Its semantics seem unusual. Please add documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function iterates over specific fields retrieved from all the hosts in a pool. If all the fields have the same value, it prints that value; otherwise, it prints the default value.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is about reporting, I believe the maximum should be reported. If this is about using a value, the minimum should be used. This is a security feature and I can't see the purpose of a default if values don't agree.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be better to change it to a more specified name, how about: get_common_value_from_all_hosts

match Client.Host.get_all ~rpc ~session_id with
| [] ->
default (* No hosts - return default *)
| h :: hs ->
let first_value = getter ~rpc ~session_id ~self:h |> transform in
let all_values =
List.map (fun h -> getter ~rpc ~session_id ~self:h |> transform) hs
in
if List.for_all (( = ) first_value) all_values then
first_value
else
default
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Print a log for the detailed differences in the hosts.


let bond_record rpc session_id bond =
let _ref = ref bond in
let empty_record =
Expand Down Expand Up @@ -1506,6 +1520,42 @@ let pool_record rpc session_id pool =
)
~get_map:(fun () -> (x ()).API.pool_license_server)
()
; make_field ~name:"ssh-enabled"
~get:(fun () ->
get_unified_field ~rpc ~session_id
~getter:Client.Host.get_ssh_enabled ~transform:string_of_bool
~default:""
)
()
; make_field ~name:"ssh-enabled-timeout"
~get:(fun () ->
get_unified_field ~rpc ~session_id
~getter:Client.Host.get_ssh_enabled_timeout
~transform:Int64.to_string ~default:""
)
~set:(fun value ->
Client.Pool.set_ssh_enable_timeout ~rpc ~session_id ~self:pool
~timeout:(Int64.of_string value)
)
()
; make_field ~name:"ssh-expiry"
~get:(fun () ->
get_unified_field ~rpc ~session_id
~getter:Client.Host.get_ssh_expiry ~transform:Date.to_rfc3339
~default:""
)
()
; make_field ~name:"console-idle-timeout"
~get:(fun () ->
get_unified_field ~rpc ~session_id
~getter:Client.Host.get_console_idle_timeout
~transform:Int64.to_string ~default:""
)
~set:(fun value ->
Client.Pool.set_console_timeout ~rpc ~session_id ~self:pool
~console_timeout:(Int64.of_string value)
)
()
]
}

Expand Down Expand Up @@ -3265,6 +3315,26 @@ let host_record rpc session_id host =
; make_field ~name:"last-update-hash"
~get:(fun () -> (x ()).API.host_last_update_hash)
()
; make_field ~name:"ssh-enabled"
~get:(fun () -> string_of_bool (x ()).API.host_ssh_enabled)
()
; make_field ~name:"ssh-enabled-timeout"
~get:(fun () -> Int64.to_string (x ()).API.host_ssh_enabled_timeout)
~set:(fun value ->
Client.Host.set_ssh_enable_timeout ~rpc ~session_id ~self:host
~timeout:(safe_i64_of_string "ssh-enabled-timeout" value)
)
()
; make_field ~name:"ssh-expiry"
~get:(fun () -> Date.to_rfc3339 (x ()).API.host_ssh_expiry)
()
; make_field ~name:"console-idle-timeout"
~get:(fun () -> Int64.to_string (x ()).API.host_console_idle_timeout)
~set:(fun value ->
Client.Host.set_console_timeout ~rpc ~session_id ~self:host
~console_timeout:(safe_i64_of_string "console-idle-timeout" value)
)
()
]
}

Expand Down
2 changes: 2 additions & 0 deletions ocaml/xapi-consts/api_errors.ml
Original file line number Diff line number Diff line change
Expand Up @@ -1424,3 +1424,5 @@ let host_driver_no_hardware = add_error "HOST_DRIVER_NO_HARDWARE"

let tls_verification_not_enabled_in_pool =
add_error "TLS_VERIFICATION_NOT_ENABLED_IN_POOL"

let set_console_timeout_failed = add_error "SET_CONSOLE_TIMEOUT_FAILED"
24 changes: 24 additions & 0 deletions ocaml/xapi/message_forwarding.ml
Original file line number Diff line number Diff line change
Expand Up @@ -1185,6 +1185,14 @@ functor
let disable_ssh ~__context ~self =
info "%s: pool = '%s'" __FUNCTION__ (pool_uuid ~__context self) ;
Local.Pool.disable_ssh ~__context ~self

let set_ssh_enable_timeout ~__context ~self ~timeout =
info "%s: pool = '%s'" __FUNCTION__ (pool_uuid ~__context self) ;
Local.Pool.set_ssh_enable_timeout ~__context ~self ~timeout

let set_console_timeout ~__context ~self ~console_timeout =
info "%s: pool = '%s'" __FUNCTION__ (pool_uuid ~__context self) ;
Local.Pool.set_console_timeout ~__context ~self ~console_timeout
end

module VM = struct
Expand Down Expand Up @@ -4035,6 +4043,22 @@ functor
let local_fn = Local.Host.disable_ssh ~self in
let remote_fn = Client.Host.disable_ssh ~self in
do_op_on ~local_fn ~__context ~host:self ~remote_fn

let set_ssh_enable_timeout ~__context ~self ~timeout =
let uuid = host_uuid ~__context self in
info "Host.set_ssh_enable_timeout : host = '%s'" uuid ;
Copy link
Contributor

@BengangY BengangY Mar 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can use __FUNCTION__ to replace Host.set_ssh_enable_timeout.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, for these ones in message_forwarding, please leave it as it, as it is meant to show the API call that was made (not the OCaml function). But please add the timeout value.

let local_fn = Local.Host.set_ssh_enable_timeout ~self ~timeout in
let remote_fn = Client.Host.set_ssh_enable_timeout ~self ~timeout in
do_op_on ~local_fn ~__context ~host:self ~remote_fn

let set_console_timeout ~__context ~self ~console_timeout =
let uuid = host_uuid ~__context self in
info "Host.set_console_timeout: host = '%s'" uuid ;
let local_fn = Local.Host.set_console_timeout ~self ~console_timeout in
let remote_fn =
Client.Host.set_console_timeout ~self ~console_timeout
in
do_op_on ~local_fn ~__context ~host:self ~remote_fn
end

module Host_crashdump = struct
Expand Down
Loading
Loading