SQL Azure - add support for AD admin

[gursharan001]

This PR closes Azure/bicep#1036

The changes in this PR are as follows:

• introduce new type ActiveDirectoryAdminSettings
• change to apply these settings to handle
  • sql admin with no AD admin (existing capability)
  • sql admin with AD admin (new)
  • AD admin only (new)

I have read the contributing guidelines and have completed the following:

• Tested my code end-to-end against a live Azure subscription.
• Updated the documentation in the docs folder for the affected changes.
• Written unit tests against the modified code that I have made.
• Updated the release notes with a new entry for this PR.
• Checked the coding standards outlined in the contributions guide and ensured my code adheres to them.

Below is a minimal example configuration that includes the new features, which can be used to deploy to Azure:

let activeDirectoryAdmin: ActiveDirectoryAdminSettings =
  {
    Login = "admingroup"
    Sid = "F9D49C34-01BA-4897-B7E2-3694BF3DE2CF"
    PrincipalType = ActiveDirectoryPrincipalType.Group
    AdOnlyAuth = false
  }

sqlServer {
     name "adtestserver"
     active_directory_admin (Some(activeDirectoryAdmin))
     admin_username "sqladminuser"
}

Issues

I have found that ARM templates need modification to switch SQL server configuration among following -

1. sql admin with no AD admin
2. sql admin with AD admin
3. AD admin only

Please refer this blog post - https://www.codez.one/azure-sql-with-managed-identities-part-2/

One possible way forward is to make AdOnlyAuth member optional and let the user write correct farmer template by detecting existence of SQL server out of band (using AZ cli or powershell etc)

[ninjarobot]

Thanks again for your continued work on this. I made some recommendations to try to reduce the code complexity a bit. The compiler can deal with it, but it's getting a bit difficult to follow some of the logic for building the various auth settings records.

[ninjarobot]

This will be in the next release - 1.7.25

[ninjarobot]

All the logic here has grown in complexity beyond just building a record - now the record is built then copies are modified a couple of times. This is too much to do in the implementation of the properties field, so if this level of complexity is needed, please move it to a separate function or a private member method.

[ninjarobot]

This particular check is in here many times so it would clarify the code to make this an active pattern. For example,

let (|ActiveDirAuth|AdOnlyAuth|StandardAuth|) activeDirAdmin =
    match activeDirAdmin with 
    | Some x when x.AdOnlyAuth -> AdOnlyAuth
    | Some x -> ActiveDirAuth
    | None -> StandardAuth

and then the code expresses the intent without having to read through the options and boolean logic:

match this.ActiveDirectoryAdmin with
| ActiveDirAuth -> // whatever happens for AD auth
| AdOnlyAuth -> // whatever happens when it's AD only
| StandardAuth -> // whatever happens when not using AD auth (not sure if it's called standard, this was just an example)

[gursharan001]

@ninjarobot I have made further changes to hopefully simplify the code. Please do let me know if it can be made better. Thanks again.

[ninjarobot]

Looks great, thanks for simplifying the code a bit!

[martin-bmc]

Maybe the wrong place to add this but I just tested this feature and found an issue when changing from an existing Azure AD admin to a new one. It seems like this is not supported in ARM/Bicep:
Azure/bicep-types-az#2320

[ninjarobot]

Does it work in json, just not in bicep? Or is this impacted by the bug regardless?

[martin-bmc]

As far as I understand the comments on the issue, the Microsoft SQL team allows the value to be set but not changed via the ARM template that is generated in this PR:

The initial (deployment) template has to have AAD admin set as a property of "Microsoft.Sql/servers" resource. The "update" template would have to have "Microsoft.Sql/servers/administrators" resource.

Azure/bicep-types-az#2320

It might be to complex for Farmer to handle this case but perhaps it should be mentioned in the documentation?