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

Improve the registry reader #4153

Closed
Tracked by #4608
ericpromislow opened this issue Mar 10, 2023 · 5 comments · Fixed by #4251
Closed
Tracked by #4608

Improve the registry reader #4153

ericpromislow opened this issue Mar 10, 2023 · 5 comments · Fixed by #4251
Assignees
Labels
kind/bug Something isn't working
Milestone

Comments

@ericpromislow
Copy link
Contributor

ericpromislow commented Mar 10, 2023

A list of TODO items (from code review on #4148)

[x] Separate walking the subkeys from walking the values
[x ] Remove the comment 'Alternatively, if the keys work, we could break, even if both hives are empty....'
[x ] Give readRegistryUsingSchema a consistent return type

  • Can't do much better than RecursivePartial<settings.Settings>|null
    [x] Report unrecognized registry keys and values
    [x] Clarify array processing, like do that before walking sub-objects
  • That's being done now when reading MULTI_SZ values
    [x] See if I can remove the cast multiSzValue as nativeReg.Value
  • This is done by using nativeReg.queryValueRaw to get a (type, value) pair of the registry value, and then casing on the type field to parse out the correct kind of value.
    [x] line 167: Is regValue ever not an object? - This code has changed too much to point to what this comment was referring to, or determine if it's still valid.
    [x] Pass in the current registry path so we can report the full path in error-messages, not just the current key
    [x] More type-checking is needed when reading in registry values, e.g. when reading a string into a number, etc.
@jandubois
Copy link
Member

  • Report unrecognized registry keys and values

This should not be done (see #4148 (comment))!

We should only fail on incorrect types. In which case RD should refuse to start.

@mook-as
Copy link
Contributor

mook-as commented Mar 10, 2023

There's also the corner case of if the user supplies a DWORD to a thing that expects a string[], how do we cast it (if we do at all; we also have the option of not accepting it).

@ericpromislow ericpromislow added this to the 1.9 milestone Mar 10, 2023
@jandubois
Copy link
Member

the user supplies a DWORD to a thing that expects a string[]

This should be a fatal error. I think supplying a DWORD for just a plain string should be an error too.

I expect the same things to be an error in the other formats too, e.g.

{ "experimental": 42 }

should fail because experimental must be an object and not a number.

We also need to make sure the settings pass the validator; if they don't, we should also refuse to start.

@IsaSih
Copy link
Contributor

IsaSih commented May 25, 2023

Tested on windows 11 providing settings with incorrect registry types. Rancher Desktop is just ignoring the deployment profiles and showing the first-run dialog. It is NOT throwing a fatal error.

Reg file used:


Windows Registry Editor Version 5.00
[HKEY_CURRENT_USER\Software\Rancher Desktop\Profile]
[HKEY_CURRENT_USER\Software\Rancher Desktop\Profile\Defaults]
[HKEY_CURRENT_USER\Software\Rancher Desktop\Profile\Defaults\kubernetes]
"enabled"=dword:00000001
[HKEY_CURRENT_USER\Software\Rancher Desktop\Profile\Locked]
[HKEY_CURRENT_USER\Software\Rancher Desktop\Profile\Locked\containerEngine]
[HKEY_CURRENT_USER\Software\Rancher Desktop\Profile\Locked\containerEngine\allowedImages]
"disabled"=dword:00000000
[HKEY_LOCAL_MACHINE\Software\Rancher Desktop\Profile]
[HKEY_LOCAL_MACHINE\Software\Rancher Desktop\Profile\Defaults]
[HKEY_LOCAL_MACHINE\Software\Rancher Desktop\Profile\Defaults\containerEngine]
"name"=dword:00000000
[HKEY_LOCAL_MACHINE\Software\Rancher Desktop\Profile\Defaults\kubernetes]
"enabled"=42
[HKEY_LOCAL_MACHINE\Software\Rancher Desktop\Profile\Locked]
[HKEY_LOCAL_MACHINE\Software\Rancher Desktop\Profile\Locked\containerEngine]
[HKEY_LOCAL_MACHINE\Software\Rancher Desktop\Profile\Locked\containerEngine\allowedImages]
"enabled"=dword:00000001
"patterns"=hex(7):62,00,75,00,73,00,79,00,62,00,6f,00,78,00,00,00,6e,00,67,00,\
  69,00,6e,00,78,00,00,00,00,00

Note that the values with incorrect reg types are:

[HKEY_LOCAL_MACHINE\Software\Rancher Desktop\Profile\Defaults\containerEngine]
"name"=dword:00000000
[HKEY_LOCAL_MACHINE\Software\Rancher Desktop\Profile\Defaults\kubernetes]
"enabled"=42

@IsaSih
Copy link
Contributor

IsaSih commented May 26, 2023

Tried again and this time application failed to start. Moving issue to done.

Image

@IsaSih IsaSih closed this as completed May 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants