Re: [pdm-devel] [PATCH datacenter-manager 00/21] improve remote wizard

[Dominik Csapak <d.csapak@proxmox.com>]

On 8/19/25 14:09, Lukas Wagner wrote:
> On Fri May 16, 2025 at 3:35 PM CEST, Dominik Csapak wrote:
>> # Summary
>>
>> This series improves the remote wizard in various points:
>> * improved wording and texts
>> * moved seperate buttons on pages into the next button
>> * probing of entered nodes
>> * confirmation dialog for missing fingerprints
>> * better realm selector
>>
>> # Notes
>>
>> I'm not super sure about the API call structure. I think two seperate
>> API calls (one for tls probing, one for scanning) might make more sense.
>> If that's wanted, I'll do so in a v2 of the series.
> 
> Yeah, I think two separate API endpoints make a lot of sense, the `scan`
> endpoint is a bit weird right now.
> 
thanks, i also thought so, but only after i did this implementation 
already. will change the v3 to include a separate api call for tls
probing. Doing 2 api calls from the ui should be fine

>>
>> ## Probing on the server side
>>
>> I tried to determine if we need a fingerprint for nodes inside the
>> API call, by probing each node. Since we currently only get the
>> nodenames and not FQDNs in the nodelist, this will currently not
>> result in a valid connection in most cases and return the fingerprint.
>>
>> My plan would be to include FQDNs of the nodes on the PVE side API call,
>> so we can return here a list of nodename, ip and FQDNs, which the user
>> then can select from. (which of those I'd probe on first check is yet
>> to be determined)
>>
>> ## Fingerprint confirmation dialogs
>>
>> Not sure if we want to be able to let the user confirm the fingerprint
>> so easily. On one hand it's very convenient, but maybe leads to users
>> simply clicking yes without understanding what's happening.
>>
>> If it's deemed too dangerous, I'd rework the series without this.
> 
> Good question. IMO something like this can be okay for a bit more
> convenience, but maybe some other people have an opinion on this as
> well.
> 
> One thing I'm not sure about is the fact that we spawn a dialog from a
> dialog here, which (I think) is a first for us.
> 

yes it's a first indeed, but especially on the nodes part of the wizard
i found no good way to show that info otherwise. I can try to integrate
the info in the panel itself, but since it requires confirmation
i would change that to a checkbox ("Trust these/this fingerprint(s)") ?

on the nodes part, it would involve scrolling to possibly show all
the fingerprints of the nodes, (which also can happen on the dialog if
there are many).

> 
> Some other minor things I've noticed while testing this:
> 
> - Entering the remote name on the second stage of the wizard seemed a
>    bit odd to me (can't really explain why, just a gut feeling I had when
>    the second stage was shown). But I guess it could make sense in case
>    we want to autofill the clustername there.

i get what you mean, but yes exactly, the idea initially was to autofill
the cluster name here (so the first panel would not work).

For now I'd leave it on the second page (it's already that way) if
that's ok for you. (We can still move it around later)

> 
> - When you go from "Summary" from "Probe Remote" and then to "Settings"
>    (all via clicking the tab bar), the 'Trust fingerprint' dialog is
>    shown again. Maybe we can detect that the IP/host has not been changed
>    and/or cache which fingerprints we already marked as trusted?

Should be possible, I'll see that I do it that way.

> 
> - 'Realm' field should be greyed out when "Use existing Token" has been
>    selected

oops, that's a bug^^

> 
> All just minor things, all it all these changes looked quite solid to
> me!
> 
>>
>> # Future work
>>
>> The next step for the wizard is to have some kind of quick copy&paste
>> info. After discussing off-list with Fabian a bit, I think it would be
>> best for this to contain the hostname (FQDN?) + fingerprint (if just a
>> self-signed certificate) + a list of nodes with their respective
>> nodename + FQDNs (maybe requires api change on PVE side to generate
>> this). The user would then still have to do most of the steps currently
>> necessary in the wizard, except the manual copy & pasting of
>> fingerprints and maybe entering of FQDNs.
>>
>>
>> Dominik Csapak (21):
>>    server/ui: pve: change 'realm list' api call to GET
>>    api types: RemoteType: put default port info to the type
>>    server: connection: add probe_tls_connection helper
>>    server/ui: pve api: extend 'scan' so it can probe the tls connection
>>    pdm-client: add scan_remote and probe_tls methods
>>    ui: remotes: node url list: add placeholder and clear trigger
>>    ui: rmeotes: node url list: make column header clearer
>>    ui: remotes: node url list: handle changing default
>>    ui: pve wizard: rename 'realm' variable to 'info'
>>    ui: pve wizard: summary: add default text for fingerprint
>>    ui: pve wizard: nodes: improve info text
>>    ui: pve wizard: nodes: probe hosts to verify fingerprint settings
>>    ui: pve wizard: info: use pdm_client for scanning
>>    ui: pve wizard: info: detect hostname and fingerprint
>>    ui: pve wizard: info: remove manual scan button
>>    ui: widget: add pve realm selector
>>    ui: pve wizard: info: use pve realm selector
>>    ui: pve wizard: connect: factor out normalize_hostname
>>    ui: pve wizard: connect: move connection logic to next button
>>    ui: pve wizard: connect: use scan api endpoint instead of realms
>>    ui: pve wizard: connect: add certificate confirmation dialog
>>
>>   lib/pdm-api-types/Cargo.toml          |   1 +
>>   lib/pdm-api-types/src/lib.rs          |   2 +
>>   lib/pdm-api-types/src/remotes.rs      |  16 ++
>>   lib/pdm-client/src/lib.rs             |  45 ++++
>>   server/src/api/pve/mod.rs             |  62 ++++--
>>   server/src/connection.rs              |  87 +++++++-
>>   ui/Cargo.toml                         |   1 +
>>   ui/src/remotes/add_wizard.rs          |   8 +-
>>   ui/src/remotes/node_url_list.rs       |  33 ++-
>>   ui/src/remotes/wizard_page_connect.rs | 308 ++++++++++++++++----------
>>   ui/src/remotes/wizard_page_info.rs    | 127 ++++++-----
>>   ui/src/remotes/wizard_page_nodes.rs   | 241 +++++++++++++++++++-
>>   ui/src/remotes/wizard_page_summary.rs |   5 +-
>>   ui/src/widget/mod.rs                  |   3 +
>>   ui/src/widget/pve_realm_selector.rs   | 125 +++++++++++
>>   15 files changed, 853 insertions(+), 211 deletions(-)
>>   create mode 100644 ui/src/widget/pve_realm_selector.rs
> 



_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel