BREAKING CHANGE: SqlServerNetwork: Add the possibility to use ListenAll and custom IPs

[claudiospizzi]

Pull Request (PR) description
Add the posibility to manage the TCP network with all aspects, including ListenAll and the custom IPs.

This Pull Request (PR) fixes the following issues:
Fixes #339

Task list:

• Change details added to Unreleased section of CHANGELOG.md?
• Added/updated documentation, comment-based help and descriptions in .schema.mof files where appropriate?
• Examples appropriately updated?
• New/changed code adheres to Style Guidelines?
• Unit and (optional) Integration tests created/updated where possible?

---

This change is 

[codecov-io]

Codecov Report

Merging #1045 into dev will decrease coverage by <1%.
The diff coverage is 73%.

@@          Coverage Diff          @@
##            dev   #1045    +/-   ##
=====================================
- Coverage    97%     97%    -1%     
=====================================
  Files        32      33     +1     
  Lines      3925    4575   +650     
=====================================
+ Hits       3839    4451   +612     
- Misses       86     124    +38

[johlju]

@claudiospizzi Thanks for sending in this PR! A quick glance at this I don't see this cover the ability to set a particular IP Address group. I think that if we do a breaking change, then we need to make sure that the change will not lead to a breaking change down the road.
Could you please look at the #339 (comment) and see if this PR covers that?

[claudiospizzi]

Hi @johlju
Yes, because you will never know on which IP address group which IP address IPv4/IPv6 is configured. Normally, when I want to setup an SQL server, I want to control on which IP the SQL server is listening. The IP group is just a management container. With my PR, I will automatically search for the valid IP group where the IP is configured.

[johlju]

@claudiospizzi But what if the IP address is not configured before, or the IP address in the configuration is not listed in any of the IP address groups? Can that happen?

[claudiospizzi]

@johlju You are right, this can happen if the IP address was not already configured on the system during the SQL setup. Maybe, in this case, we just add an IP address group with the target IP? Is there a supported way of adding IP address groups? I can't find any.

[johlju]

@claudiospizzi I will get back to your question as soon as I have time. It has been a long days at work recently. I haven't forgot. 😄

[johlju]

@claudiospizzi I will focus on this PR this weekend. It has been a hectic time at the day job recently.

[johlju]

@claudiospizzi went thru and tried to wrap my head around this. I think we need to make a design choice here. Please let me know what you think about the below.

IP Address groups are added depending on available network cards, see Adding or Removing IP Addresses. In my lab server I only have IP1-IP6 and IPAll. But in a test server with a cluster I have IP1-IP8 and IPAll.
So we cannot support adding and removing IP address groups, but I think we should support managing IP address groups with the SqlServerNetwork resource. For that reason I see a need for a IpAddressGroup parameter that is mentioned in the #339 (comment). The user can then choose which group should have which IP address, and if it should be enabled or not. This resource will most likely be used to change the IP addresses to different once that wasn't used during installation, or configure additional ones when network configuration changes.

If possible it would be good to be able to support all these properties (but that could be another PR).
The property Active seems to be read-only.

Also, I think the property ListenAll need to be in a separate resource (together with KeepAlive and Enabled which is protocol properties). Reason for this that these could otherwise be set differently if resource could be used multiple times in a configuration (depending on the schema after above change).

Maybe we should change this resource SqlServerNetwork to just handle protocol properties for each protocol 'Named Pipes', 'Shared Memory' and 'TCP/IP'. Another resource SqlServerNetworkTcp could handle managing IP address groups.
The names SqlServerNetwork and SqlServerNetworkTcp doesn't seem appropriate if we do this, maybe SqlServerProtocol and SqlServerProtocolTcp would be more appropriate? We could create those side-by-side and eventually make the old SqlServerNetwork resource obsolete.

PS > $managedComputerObject.ServerInstances['SQL2017'].ServerProtocols

Parent              : Microsoft.SqlServer.Management.Smo.Wmi.ServerInstance
DisplayName         : Named Pipes
HasMultiIPAddresses : False
IsEnabled           : False
IPAddresses         : {}
ProtocolProperties  : {Enabled, PipeName}
Urn                 : ManagedComputer[@Name='SQLTEST']/ServerInstance[@Name='SQL2017']/ServerProtocol[@Name='Np']
Name                : Np
Properties          : {Name=DisplayName/Type=System.String/Writable=False/Value=Named Pipes, Name=HasMultiIPAddresses/T
                      ype=System.Boolean/Writable=False/Value=False, Name=IsEnabled/Type=System.Boolean/Writable=True/V
                      alue=False}
UserData            :
State               : Existing

Parent              : Microsoft.SqlServer.Management.Smo.Wmi.ServerInstance
DisplayName         : Shared Memory
HasMultiIPAddresses : False
IsEnabled           : True
IPAddresses         : {}
ProtocolProperties  : {Enabled}
Urn                 : ManagedComputer[@Name='SQLTEST']/ServerInstance[@Name='SQL2017']/ServerProtocol[@Name='Sm']
Name                : Sm
Properties          : {Name=DisplayName/Type=System.String/Writable=False/Value=Shared Memory, Name=HasMultiIPAddresses
                      /Type=System.Boolean/Writable=False/Value=False, Name=IsEnabled/Type=System.Boolean/Writable=True
                      /Value=True}
UserData            :
State               : Existing

Parent              : Microsoft.SqlServer.Management.Smo.Wmi.ServerInstance
DisplayName         : TCP/IP
HasMultiIPAddresses : True
IsEnabled           : True
IPAddresses         : {IP1, IP2, IP3, IP4...}
ProtocolProperties  : {Enabled, KeepAlive, ListenOnAllIPs}
Urn                 : ManagedComputer[@Name='SQLTEST']/ServerInstance[@Name='SQL2017']/ServerProtocol[@Name='Tcp']
Name                : Tcp
Properties          : {Name=DisplayName/Type=System.String/Writable=False/Value=TCP/IP, Name=HasMultiIPAddresses/Type=S
                      ystem.Boolean/Writable=False/Value=True, Name=IsEnabled/Type=System.Boolean/Writable=True/Value=T
                      rue}
UserData            :
State               : Existing

[claudiospizzi]

Hi @johlju

I agree with you, a redesign of the resource and spliting it up into two new resources is a good idea. The two resources will then be simpler and therefore easier to maintain. Creating the new resources side-by-side and deprecate the old resource is fine with me. I've used your proposal of the new resource name SqlServerProtocol for the following draft of the two new resources:

[ClassVersion("1.0.0.0"), FriendlyName("SqlServerProtocol")]
class MSFT_SqlServerProtocol : OMI_BaseResource
{
    [Write, Description("The host name of the SQL Server to be configured. Default value is $env:COMPUTERNAME.")] String ServerName;
    [Key, Description("The name of the SQLf instance to be configured.")] String InstanceName;
    [Key, Description("The name of protocol to be configured."), ValueMap{"SharedMemory", "NamedPipes", "TcpIp"}, Values{"SharedMemory", "NamedPipes", "TcpIp"}] String ProtocolName;
    [Write, Description("Enables or disables the protocol.")] Boolean Enabled;
    [Write, Description("Specifies to listen on all IPs. Only used for the TCP/IP protocol.")] Boolean ListenOnAllIPs;
    [Write, Description("Specifies the keep alive duration. Only used for the TCP/IP protocol.")] UInt16 KeepAlive;
    [Write, Description("Specifies the named pipe name. Only used for the Named Pipes protocol.")] String PipeName;
    [Write, Description("If set to $true then SQL Server and dependent services will be restarted if a change to the configuration is made. The default value is $false.")] Boolean RestartService;
    [Write, Description("Timeout value for restarting the SQL Server services. The default value is 120 seconds.")] UInt16 RestartTimeout;
}

[ClassVersion("1.0.0.0"), FriendlyName("SqlServerProtocolTcpIp")]
class MSFT_SqlServerProtocolTcpIp : OMI_BaseResource
{
    [Write, Description("The host name of the SQL Server to be configured. Default value is $env:COMPUTERNAME.")] String ServerName;
    [Key, Description("The name of the SQLf instance to be configured.")] String InstanceName;
    [Key, Description("The name of the IP address group in the TCP/IP protocol.")] String IpAddressGroup;
    [Write, Description("Enables or disables the IP address. Only used if the IP address group is not set to 'IPAll'. If not specified, the existing value will not be changed.")] Boolean Enabled;
    [Write, Description("The IP address. Only used if the IP address group is not set to 'IPAll'. If not specified, the existing value will not be changed.")] String IPAddress;
    [Write, Description("Specifies whether the SQL Server instance should use a dynamic port. Value will be ignored if TcpPort is set to a non-empty string. If not specified, the existing value will not be changed.")] Boolean TcpDynamicPort;
    [Write, Description("The TCP port(s) that SQL Server should be listening on. If the IP address should listen on more than one port, list all ports separated with a comma ('1433,1500,1501'). If not specified, the existing value will not be changed.")] String TcpPort;
    [Write, Description("If set to $true then SQL Server and dependent services will be restarted if a change to the configuration is made. The default value is $false.")] Boolean RestartService;
    [Write, Description("Timeout value for restarting the SQL Server services. The default value is 120 seconds.")] UInt16 RestartTimeout;
}

I hope this draft of the resource schema should give an idea, how we can use the new resource in a configuration in the future. What do you think about that?

[johlju]

@claudiospizzi Sorry I missed this one! I labeled it as 'needs review' so will get back to this next week.

[johlju]

@claudiospizzi I'm good with your proposal of two new resource! Could you create two new issues for them, so we can close them individually? Do you want to run with this?

[stale]

Labeling this pull request (PR) as abandoned since it has gone 14 days or more since the last update. An abandoned PR can be continued by another contributor. The abandoned label will be removed if work on this PR is taken up again.

[stale]

Labeling this pull request (PR) as abandoned since it has gone 14 days or more since the last update. An abandoned PR can be continued by another contributor. The abandoned label will be removed if work on this PR is taken up again.

[claudiospizzi]

Currently I don't have time to get onto this. Maybe in July.

[johlju]

I have added issue #1377 and issue #1378 to track the redesign. I'm closing this PR at this time.