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

[Lukas Wagner]

On Tue Aug 19, 2025 at 2:52 PM CEST, Dominik Csapak wrote:
>
>
> 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).
>

Mhmmm, given the security implications I think the dialog is actually
the best choice for now (as it actively seeks the user's attention), so
IMO this can stay as is for now.

>> 
>> 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)

Fine for me :)

>
>> 
>> - 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.

Could you just add the accepted fingerprint into the "fingerprint" field
in the first page? I think this would be a quite simple fix for this.

>
>> 
>> - '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
>>