234 lines
14 KiB
Markdown
234 lines
14 KiB
Markdown
# Highlight of major changes of note
|
|
|
|
The majority of the code review for SRE-12016 has a lot of touched files that may not be strictly relevant to the pull request. There are some files that were touched and some new files that I want to review in detail.
|
|
|
|
### General notes
|
|
|
|
#### Touched files
|
|
|
|
Several files were touched for reformatting while reading, reformatting per code-review request, removal of module-loading (changed the psd1 to match), TODO leavings so we can continue to refactor with targeted review of TODO blocks, etc. I also changed function parameters for things like `Position` or `Mandatory` flags in cases where it caused further issues during unit tests.
|
|
|
|
I added some build-solution changes that meant touching a lot of `.tests.ps1` files.
|
|
|
|
#### ServerManager
|
|
|
|
One of my goals of this refactoring was to get us off Microsoft.Web.Administration.ServerManager. While this does expose some excellent functionality that we can definitely use, one thing it isn't is super-future-proofed. While my replacements are also somewhat bound to the same concepts as the use of ServerManager, I am trying to move in the direction of functions-per-need. This allows for better testing by mockability. To mock ServerManager already requires something like this:
|
|
|
|
Function Mock-ServerManager {
|
|
Write-Host "Creating our custom mock object";
|
|
|
|
$applications = @(
|
|
@{
|
|
Path = "/Mockable";
|
|
PhysicalPath = "C:\";
|
|
ApplicationPoolName = "Mockable";
|
|
EnabledProtocols = "http";
|
|
Attributes = @{
|
|
preloadEnabled = @{
|
|
Value = "True"
|
|
}
|
|
}
|
|
},
|
|
@{
|
|
Path = "/Mockable2";
|
|
PhysicalPath = "C:\";
|
|
ApplicationPoolName = "Mockable";
|
|
EnabledProtocols = "http";
|
|
Attributes = @{
|
|
preloadEnabled = @{
|
|
Value = "True"
|
|
}
|
|
}
|
|
}
|
|
)
|
|
|
|
Add-Member -InputObject $applications -MemberType ScriptMethod -Name Add -Value {
|
|
Param($path, $physicalPath)
|
|
return @{
|
|
Path = $path;
|
|
PhysicalPath = $physicalPath;
|
|
ApplicationPoolName = "";
|
|
EnabledProtocols = "";
|
|
Attributes = @{
|
|
preloadEnabled = @{
|
|
Value = "False"
|
|
}
|
|
}
|
|
}
|
|
} -Force $mgr = New-Object PSObject -Property @{ Sites = @{"Default Web Site" = @{Applications = $applications}}}
|
|
|
|
Add-Member -InputObject $mgr -MemberType ScriptMethod -Name CommitChanges -Value {}
|
|
Add-Member -InputObject $mgr -MemberType ScriptMethod -Name Dispose -Value { param() Write-Host "Disposing";return;}
|
|
|
|
return $mgr
|
|
}
|
|
|
|
Maintaining something like this with more complexity for all functionality and testing is going to grow more painful as time goes on. Switching to simple-functions is a goal we should all strive for.
|
|
|
|
We should work to modify as many functions as use the ServerManager object to use these functions as possible for further testability work and eventual replacement/auditing:
|
|
|
|
1. Get-ServerManager
|
|
2. Save-IISServerManagerChange
|
|
|
|
|
|
|
|
## Get-MachineConfigAppSetting
|
|
|
|
[Get-MachineConfigAppSetting.ps1](https://bitbucket.corp.alkamitech.com/projects/DEVOPS/repos/alkami.powershell/pull-requests/336/diff#Modules/Alkami.PowerShell.Common/Public/Get-MachineConfigAppSetting.ps1) is being deprecated in favor of Get-AppSetting. The warnings here are for TeamCity so we can find, isolate and update those settings in other places. Nobody knows for sure why we can't move to this pattern, so we are doing so. This touches a few files (I can put together a list if needed) that aren't directly related to the requirement of the ticket, but code reviews indicated the discrepancy and added to a request for the change.
|
|
|
|
## Get-AppServiceAccountName
|
|
|
|
[Get-AppServiceAccountName.ps1](https://bitbucket.corp.alkamitech.com/projects/DEVOPS/repos/alkami.powershell/pull-requests/336/diff#Modules/Alkami.PowerShell.Configuration/Public/Get-AppServiceAccountName.ps1) This is to reduce our need for magic variables by environments. This replaces Set-AppTierGMSAAccounts when we get the bootstrap fully implemented on all servers. See [SRE-12325](https://jira.alkami.com/browse/SRE-12325) for more details here. We can use this in Set-AlkamiWebAppPoolConfiguration (see below for more discussion) to automatically put in a user credential without having to predefine it in a global array.
|
|
|
|
> This function is basically the linchpin of the auto-configuration of Set-AlkamiWebAppPoolConfiguration on the Legacy ORB applications.
|
|
|
|
## Set-AlkamiWebAppPoolConfiguration
|
|
|
|
[Set-AlkamiWebAppPoolConfiguration.ps1](https://bitbucket.corp.alkamitech.com/projects/DEVOPS/repos/alkami.powershell/pull-requests/336/diff#Modules/Alkami.PowerShell.IIS/Public/Set-AlkamiWebAppPoolConfiguration.ps1) for user configuration setting as everything uses AD GMSA credentials. This function does self-healing of application pool configurations, so while it may cause additional disk churn, it fixes any aberrant configurations of existing functionality and ensures consistency throughout the application.
|
|
|
|
Set-AllAppPoolDefaults can be converted to use this new functionality as well, reducing repeated complexity.
|
|
|
|
One of the setbacks of this function, especially when referring to general notes above, is that there is a lack of explicit testability around $appCmdPath. This could also be extracted into a function that would improve testability. In the interests of not-overloading the complexity of this pull request I left this inline, but a simple pair of functions would improve testability of these functions:
|
|
|
|
if (!(& $appCmdPath list apppool $property | Where-Object { $_ -match $AppPoolName })) {
|
|
|
|
could be replaced with
|
|
|
|
Function Get-AppCommandPropertyExistsOnAppPoool {
|
|
[CmdletBinding()]
|
|
Param(
|
|
[Parameter(Mandatory=$true, Position=0)]
|
|
[Alias("Name")]
|
|
[string]$property,
|
|
|
|
[Parameter(Mandatory=$true, Position=1)]
|
|
[ref]$AppPoolName
|
|
)
|
|
|
|
## Replace with a call to a function to find this in the right place
|
|
$appCmdPath = (Join-Path $env:systemroot "\system32\inetsrv\appcmd.exe")
|
|
|
|
$returnValue = $false
|
|
|
|
$propertiesFoundOn = @(& $appCmdPath list apppool $property)
|
|
|
|
foreach($propertyReturned in $propertiesFoundOn) {
|
|
if ($propertyReturned -match $AppPoolName) {
|
|
$returnValue = $false
|
|
}
|
|
}
|
|
|
|
return $returnValue
|
|
}
|
|
|
|
and this is the paired replacement:
|
|
|
|
(& $appCmdPath set apppool $AppPoolName $property) | Out-Null
|
|
|
|
Function Set-AppCommandPropertyOnAppPoool {
|
|
[CmdletBinding()]
|
|
Param(
|
|
[Parameter(Mandatory=$true, Position=0)]
|
|
[Alias("Name")]
|
|
[string]$property,
|
|
|
|
[Parameter(Mandatory=$true, Position=1)]
|
|
[ref]$AppPoolName
|
|
)
|
|
|
|
## Replace with a call to a function to find this in the right place
|
|
$appCmdPath = (Join-Path $env:systemroot "\system32\inetsrv\appcmd.exe")
|
|
|
|
(& $appCmdPath set apppool $AppPoolName $property) | Out-Null
|
|
}
|
|
|
|
> The main holdup on using this as a bulk replacement option is the lack of support for Get-AppServiceAccountName
|
|
> However, this can still be used today without that functionality, we just have to maintain setting the value of the service account information elsewhere.
|
|
|
|
## Set-CommonApplicationPoolOptions
|
|
|
|
[Set-CommonApplicationPoolOptions.ps1](https://bitbucket.corp.alkamitech.com/projects/DEVOPS/repos/alkami.powershell/pull-requests/336/diff#Modules/Alkami.PowerShell.IIS/Public/Set-CommonApplicationPoolOptions.ps1) Any use of this function should be replaced with a call to Set-AlkamiWebAppPoolConfiguration.ps1 as this function is now just a very thing wrapper around that.
|
|
|
|
## New-AppTierApplicationPools
|
|
|
|
[New-AppTierApplicationPools.ps1](https://bitbucket.corp.alkamitech.com/projects/DEVOPS/repos/alkami.powershell/pull-requests/336/diff#Modules/Alkami.PowerShell.IIS/Public/New-AppTierApplicationPools.ps1)
|
|
|
|
The guts of this function were moved to New-AppTierApplicationPool to help get Set-CommonApplicationPoolOptions and Set-AlkamiWebAppPoolConfiguration to the right place. This function should be removed if we adopt Install-AlkamiWebApplication overall.
|
|
|
|
> Implementing this also means drastically simplifying the global:AppTierApplications
|
|
|
|
## New-AppTierApplicationPool
|
|
|
|
[New-AppTierApplicationPool.ps1](https://bitbucket.corp.alkamitech.com/projects/DEVOPS/repos/alkami.powershell/pull-requests/336/diff#Modules/Alkami.PowerShell.IIS/Public/New-AppTierApplicationPool.ps1) This function was a rip from the middle of New-AppTierApplicationPools so that we could focus on replacing the internals and ensure we have consistent behavior on the replacement function. See also (links below) New-AppTierWebApplication/New-AppTierWebApplications and New-WebTierWebApplications which was just internals bulk-replaced with the call to Install-AlkamiWebApplication.
|
|
|
|
I didn't force-replace the AppTier changes like I did on the WebTier because I wanted more group buy-in before I force that.
|
|
|
|
If we push the changes for New-AppTierWebApplications to use Install-AlkamiWebApplication instead, then we can just get rid of all calls to New-AppTierApplicationPools entirely, as the Install-AlkamiWebApplication creates or updates (self-heals) the app pool for that service as it runs.
|
|
|
|
New-AppTierApplicationPool should be converted to use the new functionality in Set-AlkamiWebAppPoolConfiguration
|
|
|
|
> Note the conversion to a PSCredential parameter
|
|
|
|
## New-AppTierWebApplications
|
|
|
|
[New-AppTierWebApplications.ps1](https://bitbucket.corp.alkamitech.com/projects/DEVOPS/repos/alkami.powershell/pull-requests/336/diff#Modules/Alkami.PowerShell.IIS/Public/New-AppTierWebApplications.ps1)
|
|
|
|
The internals of this function should be moved to Install-AlkamiWebApplication. The holdup is the lack of configurable credentials.
|
|
|
|
## New-WebTierWebApplications
|
|
|
|
[New-WebTierWebApplications.ps1](https://bitbucket.corp.alkamitech.com/projects/DEVOPS/repos/alkami.powershell/pull-requests/336/diff#Modules/Alkami.PowerShell.IIS/Public/New-WebTierWebApplications.ps1)
|
|
|
|
This just wholesale calls Install-AlkamiWebApplication under the hood because there is no GMSA account associated with any web-tier applications, and that is the majority holdup on making the changes above.
|
|
|
|
## Install-AlkamiWebApplication
|
|
|
|
[Install-AlkamiWebApplication.ps1](https://bitbucket.corp.alkamitech.com/projects/DEVOPS/repos/alkami.powershell/pull-requests/336/diff#Modules/Alkami.PowerShell.IIS/Public/Install-AlkamiWebApplication.ps1) This is the brute of the story.
|
|
|
|
Basically, the intent is that we can install existing Legacy ORB applications (ex: BankService) with the same installer we use for standalone WebApplications (ex: CUFX, Iodine). This function will allow us to install some applications (ex: CUFX) to chocolatey and link into ORB shared as appropriate, and will allow us to register the Legacy ORB applications using C:\Orb with the same functionality. This reduces the cross-system complexity we've used in the past with multiple functions duplicating effort in part or in whole, and gives us a single point of failure for all installs. If we decide that we need to change a specific feature for AppPools or WebApplications, we can do so in these new consolidated files.
|
|
|
|
# Lesser function changes/additions
|
|
|
|
## New-DefaultWebsite
|
|
## Optimize-DefaultWebsite
|
|
|
|
[New-DefaultWebsite.ps1](https://bitbucket.corp.alkamitech.com/projects/DEVOPS/repos/alkami.powershell/pull-requests/336/diff#Modules/Alkami.PowerShell.IIS/Public/New-DefaultWebsite.ps1) This function serves to create a Default Web Site but it can be used to create any website. The use of New-Website conflicts with the Microsoft WebAdministration functions, so we should avoid using their names.
|
|
|
|
> We could rename this to New-AlkamiWebsite.
|
|
|
|
If we replace New-WebSite with New-DefaultWebsite (New-AlkamiWebsite?) we should copy over the certificate functionality (which is why this wasn't merged to that function).
|
|
|
|
See also Set-WebTierDefaultWebSite, New-IPSTSWebSite, etc
|
|
|
|
[Optimize-DefaultWebsite.ps1](https://bitbucket.corp.alkamitech.com/projects/DEVOPS/repos/alkami.powershell/pull-requests/336/diff#Modules/Alkami.PowerShell.IIS/Public/Optimize-DefaultWebsite.ps1)
|
|
|
|
> We could rename this to Optimize-AlkamiWebsite.
|
|
|
|
## Install-WebApplication
|
|
|
|
This name may be confusing with Install-AlkamiWebApplication but this name matches the naming convention of the other installers:
|
|
|
|
1. Install-Widget
|
|
2. Install-Provider
|
|
3. Install-WebExtension
|
|
|
|
## New-WebBinding
|
|
|
|
This can be drastically cleaned up as Get-AlkamiWebAppPool and New-AlkamiWebAppPool are just aliases to Set-AlkamiWebAppPoolConfiguration which is the new guts of Set-CommonApplicationPoolOptions. This triple-alias for Get/New/Set was that they all did precisely the same work (save new-when-not-exists, but the function this replaces does that too, so this is existing behavior)
|
|
|
|
# Potential removal of functions on final adoption wholesale
|
|
|
|
> This requires the presence of Environment.UserPrefix on all app tier servers in the fleet.
|
|
|
|
Remove:
|
|
|
|
1. New-AppTierApplicationPools
|
|
2. New-AppTierApplicationPool
|
|
3. New-AppTierWebApplication
|
|
4. Set-CommonApplicationPoolOptions
|
|
5. Get-MachineConfigAppSetting
|
|
6. Set-AppTierGMSAAccounts (just re-run Set-AlkamiWebAppPoolConfiguration)
|
|
7. New-WebSite
|
|
8. Set-AllAppPoolDefaults (just re-run Set-AlkamiWebAppPoolConfiguration/Optimize-DefaultWebsite)
|
|
9. Set-ApplicationPoolExecutionAccount
|