NR-428950: Added support for Entra ID (AD Service Principal) authentication. #242
Conversation
src/args/argument_list.go
Outdated
| @@ -17,8 +17,8 @@ const ( | |||
| // ArgumentList struct that holds all MSSQL arguments | |||
| type ArgumentList struct { | |||
| sdkArgs.DefaultArgumentList | |||
| Username string `default:"" help:"The Microsoft SQL Server connection user name"` | |||
| Password string `default:"" help:"The Microsoft SQL Server connection password"` | |||
| Username string `default:"" help:"The Microsoft SQL Server connection user name. For Azure AD Service Principal, use format: <client_id>@<tenant_id>"` | |||
There was a problem hiding this comment.
same here instead of using the same fields for Azure AD Service Principal. Adding new optional args will make identifying authtype more easy.
src/args/argument_list.go
Outdated
| Username string `default:"" help:"The Microsoft SQL Server connection user name. For Azure AD Service Principal, use format: <client_id>@<tenant_id>"` | ||
| Password string `default:"" help:"The Microsoft SQL Server connection password. For Azure AD Service Principal, use client secret"` |
There was a problem hiding this comment.
Just wondering why we are reusing username, password fields for SP values. Would it not be easier for the users to have clear field names for args: client_id, tenant_id, client_secret?
There was a problem hiding this comment.
Added changes, please review it.
| @@ -50,4 +50,54 @@ integrations: | |||
| # db_hostname: my-custom-hostname # useful to filter in dashboards, especially in multi-server environments. | |||
| inventory_source: config/mssql | |||
|
|
|||
| - name: nri-mssql | |||
There was a problem hiding this comment.
nit: Adding a comment about what this config represents will help when refered in future.
sairaj18
dismissed
their stale review
LGTM, I'll leave it to Rohan for final approval
src/connection/sql_connection.go
Outdated
| func determineAuthMethod(args *args.ArgumentList) (AuthConnector, error) { | ||
| azureFieldsProvided := 0 | ||
| if args.ClientID != "" { | ||
| azureFieldsProvided++ | ||
| } | ||
| if args.TenantID != "" { | ||
| azureFieldsProvided++ | ||
| } | ||
| if args.ClientSecret != "" { | ||
| azureFieldsProvided++ | ||
| } | ||
|
|
||
| if azureFieldsProvided == 3 { | ||
| log.Debug("Detected Azure AD Service Principal authentication - using ClientID, TenantID, and ClientSecret") | ||
| return AzureADAuthConnector{}, nil | ||
| } | ||
|
|
||
| if azureFieldsProvided > 0 && azureFieldsProvided < 3 { | ||
| return nil, fmt.Errorf("incomplete Azure AD Service Principal credentials: all three fields (ClientID, TenantID, ClientSecret) must be provided together") | ||
| } | ||
|
|
||
| return SQLAuthConnector{}, nil | ||
| } |
There was a problem hiding this comment.
I think this logic can be improved. It's not extendable in it's current state.
I'd just write helper functions to return true if certain auth type is satisfied. And have a switch to go through them. This means adding new method is as simple as adding a new helper and another condition in the switch. The default for the switch will be an error because that is only hit if we don't have sufficient args for any of the auth types.
There was a problem hiding this comment.
Added changes as per review, for default authentication we cant check username/password because for gMSA user both can be empty. So i added SQL authenticator in default case.
There was a problem hiding this comment.
SQL Server authentication can work with empty username and password in some cases (like Windows Authentication).
There was a problem hiding this comment.
Hmm, okay, so default case could be that, but I still don't understand why you need azureFieldsProvided, could you not put them in a single condition?
This pull request includes modifications to extend the authentication options to Entra ID in addition to the existing SQL authentication. The changes were tested using the appropriate tenant_id, client_id, and client_secret as username and password, confirming that queries execute successfully with a proper connection. Additionally, test cases have been added to verify the functionality of the CreateEntraIdConnectionURL function.
Documentation changes - newrelic/docs-website@01b3e49