search

dsccommunity / xRemoteDesktopSessionHost

This module contains DSC resources for the management and configuration of Microsoft Remote Desktop Session Host (RDSH).
MIT License
35 stars 47 forks source link

Fixed Collection Name Max Length (Fixes #60) #61

Closed bendunne closed 4 years ago

bendunne commented 4 years ago

Pull Request (PR) description

Corrects Collection Name variable max length from 15 characters to 80 characters in the xRDSessionCollection and xRDSessionCollectionConfiguration resources

This Pull Request (PR) fixes the following issues

Task list

This change is Reviewable

msftclas commented 4 years ago

CLA assistant check
All CLA requirements met.

codecov-io commented 4 years ago

Codecov Report

Merging #61 into dev will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##             dev     #61   +/-   ##
=====================================
  Coverage   87.4%   87.4%           
=====================================
  Files          8       8           
  Lines        381     381           
  Branches       9       9           
=====================================
  Hits         333     333           
  Misses        39      39           
  Partials       9       9
Impacted Files Coverage Δ
...RDSessionCollection/MSFT_xRDSessionCollection.psm1 94.73% <ø> (ø) :arrow_up:
...ration/MSFT_xRDSessionCollectionConfiguration.psm1 96.36% <ø> (ø) :arrow_up:
...Resources/MSFT_xRDRemoteApp/MSFT_xRDRemoteApp.psm1 97.77% <ø> (ø) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update b1e3a65...473951b. Read the comment docs.

bendunne commented 4 years ago

@johlju @ld0614 Hi, Would you mind merging this one?

johlju commented 4 years ago

@bendunne This was set to 15 char limit because of the issue https://github.com/PowerShell/xRemoteDesktopSessionHost/issues/10, is this not an issue anymore?

johlju commented 4 years ago

Is/was this a limitation in an older operating system?

bendunne commented 4 years ago

Is/was this a limitation in an older operating system?

I don't know if there was but I've tested on 2012 R2 with WMF 5.1 and 2016 and both work. I didn't test on 2008R2 as the module 'RemoteDesktop' doesn't exist nor are there 'Collections' in 2008R2

bendunne commented 4 years ago

Is/was this a limitation in an older operating system?

I don't know if there was but I've tested on 2012 R2 with WMF 5.1 and 2016 and both work. I didn't test on 2008R2 as the module 'RemoteDesktop' doesn't exist nor are there 'Collections' in 2008R2

I've also updated the App CollectionName max length which isn't an issue either

johlju commented 4 years ago

Then I suggest we remove the length validation altogether from all the resources. That is remove the line [ValidateLength(1,80)] throughout.

I will review after that.

I don't know if there was but I've tested on 2012 R2 with WMF 5.1 and 2016 and both work. I didn't test on 2008R2 as the module 'RemoteDesktop' doesn't exist nor are there 'Collections' in 2008R2

Thank you for testing! 2008 R2 is soon unsupported OS too.

bendunne commented 4 years ago

Then I suggest we remove the length validation altogether from all the resources. That is remove the line [ValidateLength(1,80)] throughout.

I will review after that.

I don't know if there was but I've tested on 2012 R2 with WMF 5.1 and 2016 and both work. I didn't test on 2008R2 as the module 'RemoteDesktop' doesn't exist nor are there 'Collections' in 2008R2

Thank you for testing! 2008 R2 is soon unsupported OS too.

I suggest that we keep it as the max length of 80 characters. As this is what is supported.

johlju commented 4 years ago

Aha, so there’s a max length. Then I agree we keep it. Is there documentation around that, if so could you add a link to that documentation in the README.md, or add a note for it to the README.me for each resource as appropriate?

danielboth commented 4 years ago

I had a look at this in my test environment. What happens in the background when we create a new RD Session Collection is that it creates a new instance of the Win32_RDSHCollection which can be found in the root\cimv2\rdms namespace.

This is also where the limitation for 15 characters comes from. Let's run a test on a session collection with the name "TestCollectionWithVeryLongName"

Get-CimInstance -Namespace root\cimv2\rdms -ClassName Win32_RDSHCollection

Alias          : TestCollectionWi
Description    : Test DSC
Name           : TestCollectionWithVeryLongName

As we can see in the documentation, the alias property here is the actual key: https://docs.microsoft.com/en-us/windows/win32/termserv/win32-rdshcollection#properties. Not the name property.

So to identify a session collection, we need to use the alias property (which internally, New-RDSessionCollection calls for you). So 15 chars is the limit here.

Edit: So when will this become an issue? I believe that this becomes an issue as soon as you have two or more collection where the first 15 characters in the name are the same. From that moment on, the code won't be able to uniquely identify a collection.

For example, Get-RDSessionCollection runs the following code;

$wmiQuery = "SELECT * FROM Win32_RDSHServer WHERE CollectionAlias='" + $rdSessionCollection.Alias + "'"
            $rdSessionServerCount = [int]( ([array](Get-WmiObject -Namespace $wmiNamespace -Query $wmiQuery -ComputerName $ConnectionBroker -Authentication PacketPrivacy -ErrorAction Stop)).Count)

As you can see, here it will no longer use the name property, but the Alias instead (which has this limit of 15 chars).

I did not test it, since my lab has one RD Session Host, but this is why I think the limit was here in the first place.

gaelcolas commented 4 years ago

@bendunne, as per @danielboth comment, can you confirm this is a potential problem? I'd rather not make this change if it's not safe to use in the long run. /cc @johlju

bendunne commented 4 years ago

@bendunne, as per @danielboth comment, can you confirm this is a potential problem? I'd rather not make this change if it's not safe to use in the long run. /cc @johlju

Hey so,

I've just tested in a fresh 2016 RDS deployment, if the Collection Name is the same for the first 15 characters. Windows will just add a 1: image

I've also added the comments to the README file

johlju commented 4 years ago

@bendunne Can you confirm that adding a third one will add a '2' to the alias property?

bendunne commented 4 years ago

I can but I've already deleted the deployment in Azure and will take another hour of my time to verify. Is not two enough?

Kind Regards,

Ben

On 26 Aug 2019, at 15:20, Johan Ljunggren notifications@github.com<mailto:notifications@github.com> wrote:

@bendunnehttps://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fbendunne&data=02%7C01%7C%7C2d7faef32c3d4388e32408d72a309189%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637024260389977883&sdata=%2FvktFcJFYTyMesfCFxoEuR%2BebCexGvjvEC2F1gGvk2Y%3D&reserved=0 Can you confirm that adding a third one will add a '2' to the alias property?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FPowerShell%2FxRemoteDesktopSessionHost%2Fpull%2F61%3Femail_source%3Dnotifications%26email_token%3DAHDCAFQZHG2NU4UIK2USXV3QGPRDLA5CNFSM4IMQDKJ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5EQOMQ%23issuecomment-524879666&data=02%7C01%7C%7C2d7faef32c3d4388e32408d72a309189%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637024260389987882&sdata=rRUFhWnt8QKREB4aR6xXAKxOGjZweCLABQcy4JE%2B0k0%3D&reserved=0, or mute the threadhttps://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAHDCAFUQYSFZYE72OLGTRHDQGPRDLANCNFSM4IMQDKJQ&data=02%7C01%7C%7C2d7faef32c3d4388e32408d72a309189%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637024260389997905&sdata=0vuJofyMK5uAHCG3ccV%2F3V4Xw0f8Mt4ePcly6AGBks0%3D&reserved=0.

johlju commented 4 years ago

Ah, sorry about that, but I would really like to know that it actually counts up when there are more than two with the same name.

Or if there is official documentation explaining that.

bendunne commented 4 years ago

Ah, sorry about that, but I would really like to know that it actually counts up when there are more than two with the same name.

Or if there is official documentation explaining that.

I couldn't find documentation explaining this. Here is with the third server:

PS C:\Users\BDTEST> Get-CimInstance -Namespace root\cimv2\rdms -ClassName Win32_RDSHCollection

Alias : 1234555555555555 Description : Name : 12345555555555555555555555555555555555555555555555555555556664421414173419210878 Type : 0 PSComputerName :

Alias : 12345555555555551 Description : Name : 12345555555555555555555555555555555555555555555555555555556664421414173419210879 Type : 0 PSComputerName :

Alias : 123455555555555512 Description : Name : 12345555555555555555555555555555555555555555555555555555556664421414173419210863 Type : 0 PSComputerName

I'll keep the test environment active until this is resolved.

danielboth commented 4 years ago

Hi @bendunne, thank you for all your tests! I verified this also in my test environment on W2016 machines. Also, I created a DSC configuration that deploys 2 collections with long collection names (using a local copy of your branch). That also went fine, I could successfully create two collections with the same name and also test them properly afterwards, where they both reported InDesiredState.

So it seems that the limitation for this 15 characters is no longer there, so I would be fine with merging this one, as we have now proven that the key identifier on Win32_RDSHCollection (Alias) will stay unique.

johlju commented 4 years ago

@bendunne and @danielboth awesome! Thank you so much for testing this. Just one more question, I ask this before, but since you tested again. Do we need to keep a max limit at all, or is 80 chars a hard limit for a collection name? If a collection name can be longer than 80 chars then I don't see the need to limit this at all. What are your thoughts?

bendunne commented 4 years ago

@bendunne and @danielboth awesome! Thank you so much for testing this. Just one more question, I ask this before, but since you tested again. Do we need to keep a max limit at all, or is 80 chars a hard limit for a collection name? If a collection name can be longer than 80 chars then I don't see the need to limit this at all. What are your thoughts?

So the GUI has a limit of 80 Characters but the New-RDSessionCollection command has a limit of 256. Shall I change the validation to 256 characters or just remove it altogether? I guess if we are building a DSC resource we want it to be reliable even in the unlikely use cases.

johlju commented 4 years ago

If viewing a collection name that is longer than 80 chars in the GUI, will it be cut? If it will be cut in the GUI then let's keep as-is, but if the GUI can show the known hard-limit of 256 chars then I suggest to change it to that. 🙂:thinking:

bendunne commented 4 years ago

If viewing a collection name that is longer than 80 chars in the GUI, will it be cut? If it will be cut in the GUI then let's keep as-is, but if the GUI can show the known hard-limit of 256 chars then I suggest to change it to that. 🙂🤔

So the GUI limit to enter is 80 however if I set the length of the collection name to 256 chars it will still display this in the GUI. I have updated the files to reflect this.

johlju commented 4 years ago

@bendunne thank you for your patience and work on this change! 😃

@gaelcolas can you merge this as I don't have permission.