-
Notifications
You must be signed in to change notification settings - Fork 235
Constrained Runspace Support #1507
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
Constrained Runspace Support #1507
Conversation
I'm working on the test failures |
Hey @dkattan, sorry not to have read through this yet, it's on my radar. Since I've taken over maintainership of PSES from @TylerLeonhardt, and this work was started before I joined, I need more context. I'd also be happy to have a call with you. I've read through #1282, but could you summarize/explain the full context of what drove this work, what it solves, how you've gone about solving it, as well as the potential risks of these changes with respect to the extension and other users of editor services? I would really like to see this get in, as I know you've been working hard on it and so there are obviously good reasons for the work, I just want to understand those reasons myself. |
Absolutely! I'll start by defining the problem. I have a .net core web application that allows users to write PowerShell that executes in the backend. Our backend exposes various cmdlets that are implemented in C# that have complex parameter sets. To protect against users unintentionally, or intentionally breaking the backend by overwriting files, we run the PowerShell in a Constrained Runspace, in Constrained Language Mode. A Constrained Runspace is a runspace created with the InitialSessionState.Create() method (versus CreateDefault() or CreateDefault2()). The .Create() method creates an initialSessionState with nothing preloaded at all. From there I explicitly add the cmdlets, aliases, modules, functions, providers, etc that I want the user to have access to. CreateDefault() and CreateDefault2() will automatically load the FileSystem provider, so I am forced to create the runspace in this way if I don't want that provider loaded automatically. Where this gets tricky is ensuring that the PSES has access to the cmdlets and modules it requires, which is actually a pretty manageable list: Cmdlets
Modules
The Cmdlets were easy enough, the only complication is that we reference them by their fully qualified name which I worked around by adding an alias to the shortname that I loaded into the constrained runspace. The modules were slightly trickier since they were being loaded by Import-Module -Name $Path after the runspace was already open. Even if I added Import-Module to the runspace, this approach assumes a FileSystem provider will be present. @TylerLeonhardt had commented that he preferred doing it like this as the logging is better, which is true. The only way to load file based modules into a ConstrainedRunspace is via InitialSessionState.ImportPSModulesFromPath which returns void and throws no exceptions on failure. To work around that, I am now checking if the module path exists and verify the module is present in the initialSessionState object after calling ImportPSModulesFromPath, and logging as appropriate. A recent complication I had to overcome was the structure of the PowerShellEditorServices.Commands sub-module. Traditionally .psd1 files should have the same filename as their parent folder, and ImportPSModulesFromPath only takes folder paths, and will only load modules who have a .psd1 that match the parent folder name. However, our module violates this since the path is So as of yesterday I am making a copy of PowerShellEditorServices.Commands.psd1 and naming it Commands.psd1 and giving ImportPSModulesFromPath(".module\PowerShellEditorServices\Commands") and it loads successfully. The complications with Serilog are really a side-effect of me running Start-EditorServices.ps1 from a process that has overlapping dependencies. I'm able to work around that now with no code changes. I simply delete the extra .dll files that you guys are opportunistically loading. I could submit a merge request to check if it already exists in the loaded assemblies, but baby steps. The final and probably most risky change is ensuring that the other runspaces PSES may create, are created from the same initialSessionState object as the original runspace. To accomplish this, I simply pass the entire initialSessionState object in place of the LanguageMode property that was previously being passed. The initialSessionState object is where the language mode was coming from in the first place, and was down the line being used to create, you guessed it, a new initialSessionState object which was then being used to create out of process runspaces. Since these new runspaces were created using CreateDefault() This was causing PSES to leak FileSystem details to my ConstrainedRunspace. Passing along the initialSessionState from the calling runspace solved this problem. The meat of my change is only triggered when the FileSystem provider is absent, so I don't anticipate it affecting much outside of that circumstance. Now I just need to figure out what I broke that is causing the tests to fail in the pipeline. Those tests pass when run locally. |
Also, @andschwa I'd love to jump on a call. I don't know if you've got time today, but I emailed you a Teams link just in case. |
@@ -349,6 +349,8 @@ private EditorServicesConfig CreateConfigObject() | |||
var profile = (PSObject)GetVariableValue("profile"); | |||
|
|||
var hostInfo = new HostInfo(HostName, HostProfileId, HostVersion); | |||
var iss = Runspace.DefaultRunspace.InitialSessionState; | |||
iss.LanguageMode = Runspace.DefaultRunspace.SessionStateProxy.LanguageMode; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2685a37
to
37d1fe9
Compare
37d1fe9
to
f353d72
Compare
I was able to get everything working without changing the codebase with the changes in 3.0 closing this PR. |
@dkattan Does that mean PSES is fully working in constrained runspaces now? |
There is a lot I have to do to my constrained runspace to make PSES work, it doesn't just work out of the box. If I'm being honest, now that I'm looking at it with fresh eyes from the time I originally submitted this, I think I could have made it work all along, but the idea was to put the onus on PSES to do that instead of the calling code. |
Would love to know what all you're still having to do, as I want to support constrained language mode at some point. |
@andschwa I've got this boiled down with as few changes as possible, could I have you take a look?