Skip to content
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

Support pool.sync_updates from remote_pool repo #6108

Open
wants to merge 4 commits into
base: feature/easier-pool-join
Choose a base branch
from

Conversation

gangj
Copy link
Contributor

@gangj gangj commented Nov 8, 2024

CP-50787 CP-51347: Support pool.sync_updates from remote_pool repo

When a remote_pool type repository, which points to the enabled
repository in the remote pool coordinator, is set as the enabled
repository of the pool, updates can be synced from it with API
pool.sync_updates.

The username password of the remote pool coordinator is required as
parameters for pool.sync_updates to login the remote pool.

And the remote pool coordinator's host server certificate needs to be
configured in the remote_pool repository, it will be used to verify the
remote end when sending out username passwords.

A new yum/dnf plugin "xapitoken" is introduced to set xapi token as HTTP
cookie: "session_id" for each HTTP request which downloads files from the
remote_pool repository.

CP-52245: Temp disable repo_gpgcheck when syncing from remote_pool repo

Will re-enable repo_gpgcheck after CP-51429 is done by reverting this
commit.

@gangj gangj force-pushed the private/gangj/CP-50787_CP-51347 branch 2 times, most recently from ebb463a to 841c232 Compare November 8, 2024 09:10
python3/dnf_plugins/xapitoken.py Outdated Show resolved Hide resolved
python3/dnf_plugins/xapitoken.py Show resolved Hide resolved
@gangj gangj force-pushed the private/gangj/CP-50787_CP-51347 branch 5 times, most recently from 3ff9d75 to a2f74fd Compare November 11, 2024 08:40
ocaml/xapi/repository.ml Outdated Show resolved Hide resolved
ocaml/xapi/repository_helpers.ml Outdated Show resolved Hide resolved
ocaml/idl/datamodel_pool.ml Outdated Show resolved Hide resolved
ocaml/xapi/helpers.ml Outdated Show resolved Hide resolved
ocaml/xapi/repository.ml Outdated Show resolved Hide resolved
ocaml/xapi/repository_helpers.ml Outdated Show resolved Hide resolved
ocaml/xapi/repository.ml Outdated Show resolved Hide resolved
ocaml/xapi/xapi_pool.ml Outdated Show resolved Hide resolved
let sync ~__context ~self ~token ~token_id =
let ext_host_verified_rpc ~__context ~cert host_address xml =
try Helpers.make_external_host_verified_rpc ~__context host_address cert xml
with Xmlrpc_client.Connection_reset ->
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now when the certificate is invalid, the expected ssl_verify_error is not raised, instead this Xmlrpc_client.Connection_reset is caught here. I think this problem can be resolved with another ticket later.

@@ -1919,6 +1919,16 @@ let _ =
"If the bundle repository or remote_pool repository is enabled, it \
should be the only one enabled repository of the pool."
() ;
error Api_errors.update_syncing_remote_pool_coordinator_connection_failed []
Copy link
Member

Choose a reason for hiding this comment

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

I think the context of the error is clear as it would happen when a pool.sync_updates. It could be as simple as "CANNOT_CONTACT_HOST".
But the authentication (incorrect password) may fail with this error also. So the possible causes should be mentioned in the message.

Copy link
Contributor Author

@gangj gangj Nov 13, 2024

Choose a reason for hiding this comment

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

The authentication (incorrect password) failure will fail with another error: update_syncing_remote_pool_coordinator_service_failed.
And we have a corresponding error pool_joining_host_connection_failed.
If we change it to a general one, I think we will need to provide more info like the address and port of the host which failed to be contacted, like the other existing one: error Api_errors.tls_connection_failed ["address"; "port"]

ocaml/xapi/xapi_pool.ml Outdated Show resolved Hide resolved
ocaml/xapi/repository_helpers.ml Outdated Show resolved Hide resolved
ocaml/xapi/repository.ml Outdated Show resolved Hide resolved
ocaml/xapi/repository.ml Outdated Show resolved Hide resolved
ocaml/xapi/repository.ml Outdated Show resolved Hide resolved
ocaml/idl/datamodel_repository.ml Outdated Show resolved Hide resolved
ocaml/xapi/repository.ml Outdated Show resolved Hide resolved
ocaml/xapi/helpers.ml Outdated Show resolved Hide resolved
"There was an error connecting to the remote pool coordinator while \
syncing updates from it."
() ;
error Api_errors.update_syncing_remote_pool_coordinator_service_failed []
Copy link
Member

Choose a reason for hiding this comment

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

To be useful to the user, how about to use "HOST_OFFLINE"?

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 error corresponds to the error during pool join: pool_joining_host_service_failed, I think it is not host offline, it is the HTTP service failed to respond.

@gangj gangj force-pushed the private/gangj/CP-50787_CP-51347 branch from 04f1cec to 9a9fa40 Compare December 10, 2024 17:19
@gangj gangj force-pushed the private/gangj/CP-50787_CP-51347 branch 3 times, most recently from 61c948b to cf7af87 Compare December 12, 2024 05:11
@gangj gangj requested a review from minglumlu December 12, 2024 05:55
@gangj gangj force-pushed the private/gangj/CP-50787_CP-51347 branch from cf7af87 to 63c06fe Compare December 12, 2024 10:22
@@ -149,26 +149,69 @@ let get_proxy_params ~__context repo_name =
| _ ->
("", "", "")

let sync ~__context ~self ~token ~token_id =
type client_auth_conf =
| CdnTokenAuthConf (* remote *) of {token: string; token_id: string}
Copy link
Member

@minglumlu minglumlu Dec 12, 2024

Choose a reason for hiding this comment

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

The location of the comment looks odd. It is auto-formatted like this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added there and it had been formatted, or is there a better suggested place to add the comment?

the remote pool. *)
("", None)
let uri =
Uri.make ~scheme:"http" ~host:local_host
Copy link
Member

Choose a reason for hiding this comment

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

I think it's too early exposing the localhost here.

Copy link
Contributor Author

@gangj gangj Dec 12, 2024

Choose a reason for hiding this comment

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

It is used to setup the binary_url which is pointing to the endpoint proxied by stunnel, so it is the localhost.

Copy link
Member

Choose a reason for hiding this comment

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

The binary_url is determined by the ServerAuthConf. It should be determined by the handling of ServerAuthConf.

@gangj gangj requested a review from minglumlu December 12, 2024 10:56
Comment on lines 234 to 262
let config_repo yum_conf =
match yum_conf with
| Some (token_path, yum_plugin) ->
(* Configure proxy and token *)
let token_param =
match token_path with
| "" ->
""
| p ->
Printf.sprintf "--setopt=%s.%s=%s" repo_name yum_plugin
(Uri.make ~scheme:"file" ~path:p () |> Uri.to_string)
in
let proxy_url_param, proxy_username_param, proxy_password_param =
get_proxy_params ~__context repo_name
in
let Pkg_mgr.{cmd; params} =
[
"--save"
; proxy_url_param
; proxy_username_param
; proxy_password_param
; token_param
]
|> fun config -> Pkgs.config_repo ~repo_name ~config
in
ignore
(Helpers.call_script ~log_output:Helpers.On_failure cmd params)
| None ->
""
()
Copy link
Member

@minglumlu minglumlu Dec 13, 2024

Choose a reason for hiding this comment

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

Separating the function config_repo would look better:

let proxy_params =
  let proxy_url_param, proxy_username_param, proxy_password_param =
    get_proxy_params ~__context repo_name
  in
  [
  ; proxy_url_param
  ; proxy_username_param
  ; proxy_password_param
  ]
in

let config_repo params =
  let Pkg_mgr.{cmd; params'} =
    "--save" :: params
    |> fun config -> Pkgs.config_repo ~repo_name ~config
  in
  ignore
    (Helpers.call_script ~log_output:Helpers.On_failure cmd params')
in

And with the above, it can be like:

config_repo (List.rev_append proxy_params auth_params)

ignore (Helpers.call_script cmd params)
in

let server_auth conf f =
Copy link
Member

Choose a reason for hiding this comment

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

Would suggest keeping consistency with with_sync_client_auth. E.g. with_sync_server_auth.

(* Import YUM repository GPG key to check metadata in reposync *)
let Pkg_mgr.{cmd; params} = Pkgs.make_cache ~repo_name in
ignore (Helpers.call_script cmd params) ;
let auth =
Copy link
Member

Choose a reason for hiding this comment

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

To be clear, it could be client_auth.

Copy link
Member

Choose a reason for hiding this comment

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

And If the login_with_password bit is handled before constructing PoolExtHostAuthConf, it will not be necessary to map PoolExtHostAuthConf to ExtHostAuth.

Comment on lines 300 to 284
let verified_rpc =
try
Helpers.make_external_host_verified_rpc ~__context remote_addr
cert
with Xmlrpc_client.Connection_reset ->
raise
(Api_errors.Server_error
( Api_errors
.update_syncing_remote_pool_coordinator_connection_failed
, []
)
)
in
let session_id =
try
Client.Client.Session.login_with_password ~rpc:verified_rpc
~uname:username ~pwd:password
~version:Datamodel_common.api_version_string
~originator:Xapi_version.xapi_user_agent
with
| Http_client.Http_request_rejected _ | Http_client.Http_error _
->
raise
(Api_errors.Server_error
( Api_errors
.update_syncing_remote_pool_coordinator_service_failed
, []
)
)
in
Copy link
Member

Choose a reason for hiding this comment

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

This part can be generalized as a function being like Helpers.login_after_server_check.

in
f (Some (temp_file, yum_plugin))
)
| None ->
Copy link
Member

Choose a reason for hiding this comment

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

This function can be like:

let with_sync_client_auth auth f =
  let go_with_client_plugin cred plugin_name =
    let ( let@ ) g x = g x in
    let@ temp_file =
      Helpers.with_temp_file_of_content ~mode:[Open_text] "token-" ".json"
        (Yojson.Basic.to_string cred)
    in
    f [temp_file]
  in
  match auth with
  | NoClientAuth ->
      f []
  | ExtHostAuth xapi_token, plugin ->
      let cred = `Assoc [("xapitoken", `String xapi_token)] in
      go_with_client_plugin cred plugin
  | CdnTokenAuth (token_id, token), plugin ->
      (* The validity of the token/token_id can be verified at the beginning of the API call. *)
      let cred = `Assoc [("token", `String token); ("token_id", `String token_id)] in
      go_with_client_plugin cred plugin

| `remote ->
( Db.Repository.get_binary_url ~__context ~self
, Some (Db.Repository.get_source_url ~__context ~self)
, true
, (CdnTokenAuth {token_id; token}, "accesstoken")
Copy link
Member

@minglumlu minglumlu Dec 13, 2024

Choose a reason for hiding this comment

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

If both the token_id and token are empty string, it could return NoAuth here. This avoids the empty token_path and checking against it later.
And the inconsistency like non-empty token_id and empty token will result in other error (like auth failure) raised from the CDN server side. This is expected. So we could rely on that instead of checking it in xapi. This will make the with_sync_client_auth and generating plugin parameter simpler as it will not be necessary to handle something like:

match token_path with
| "" ->
    ""

and

  match auth with
  | CdnTokenAuth {token_id; token}, plugin when token_id = "" && token = "" ->
      f (Some ("", plugin))
  | CdnTokenAuth {token_id; token}, plugin when token_id <> "" && token <> "" ->

Comment on lines 1291 to 1294
type client_auth =
| CdnTokenAuth (* remote *) of {token_id: string; token: string}
| NoAuth (* bundle *)
| PoolExtHostAuth (* remote_pool *) of {xapi_token: string}
Copy link
Member

@minglumlu minglumlu Dec 13, 2024

Choose a reason for hiding this comment

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

I think the plugin name should be part of the type. This can avoid the empty plugin name for NoAuth following in line 171:

(Uri.to_string uri, None, true, (NoAuth, ""), NoAuth)

with_sync_server_auth server_auth @@ fun binary_url' ->
write_initial_yum_config
~binary_url:(Option.value binary_url' ~default:binary_url) ;
config_repo yum_conf ;
Copy link
Member

Choose a reason for hiding this comment

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

The config_repo is working as accept a client auth tuple and add proxy under the hood.
Instead, the yum_conf can be just a prepared sync client params, e.g. with name client_auth_params. And proxy can be another sync client params prepared outside of the config_repo, e.g. with name client_proxy_params. Then the config_repo is just to accept these params to run the command with all the parameters.

@gangj gangj force-pushed the private/gangj/CP-50787_CP-51347 branch 2 times, most recently from 675308f to 1298e2b Compare December 13, 2024 11:03
When a remote_pool type repository, which points to the enabled
repository in the remote pool coordinator, is set as the enabled
repository of the pool, updates can be synced from it with API
pool.sync_updates.

The username password of the remote pool coordinator is required as
parameters for pool.sync_updates to login the remote pool.

And the remote pool coordinator's host server certificate needs to be
configured in the remote_pool repository, it will be used to verify the
remote end when sending out username passwords and syncing updates from
it.

A new yum/dnf plugin "xapitoken" is introduced to set xapi token as HTTP
cookie: "session_id" for each HTTP request which downloads files from the
remote_pool repository.

Signed-off-by: Gang Ji <[email protected]>
Will re-enable repo_gpgcheck by reverting this commit after CP-51429 is done.

Signed-off-by: Gang Ji <[email protected]>
@gangj gangj force-pushed the private/gangj/CP-50787_CP-51347 branch from 1298e2b to c710e8f Compare December 13, 2024 11:31
@gangj gangj requested a review from minglumlu December 13, 2024 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants