Skip to content
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

Managed Identity dev experience improvements #1936

Merged
merged 18 commits into from
Oct 26, 2022
Merged

Managed Identity dev experience improvements #1936

merged 18 commits into from
Oct 26, 2022

Conversation

tkyc
Copy link
Member

@tkyc tkyc commented Oct 18, 2022

PR Resolves the following:

Deprecated the following:

New authentication property values:

  • authentication=ActiveDirectoryManagedIdentity
  • authentication=DefaultAzureCredential EDIT: Later changed to authentication=ActiveDirectoryDefault

New environment variables:

  • INTELLIJ_KEEPASS_PATH
  • ADDITIONALLY_ALLOWED_TENANTS (comma delimited list of additionally allowed tenant IDs, used with DefaultAzureCredential)

To use the IntellijCredential with the driver, supply the connection string property authentication=DefaultAzureCredential and set the INTELLIJ_KEEPASS_PATH environment variable to the path of the keepass database.

@lilgreenbird lilgreenbird added this to In progress in MSSQL JDBC via automation Oct 19, 2022
@lilgreenbird lilgreenbird added this to the 11.3.0 milestone Oct 19, 2022
@lilgreenbird lilgreenbird changed the title Jdbc msi improvements Managed Identity dev experience improvements Oct 20, 2022
Copy link
Contributor

@David-Engel David-Engel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is one other change we need to make to align the JDBC driver with other drivers. We should deprecate the msiClientId get/set methods (but they should continue to work. Comment: "Use the getUser/setUser method instead.") This will include changing the Authentication=ActiveDirectoryMSI validation to allow specifying User in the connection string. If both msiClientId and User are specified, User should override msiClientId.

@lilgreenbird lilgreenbird moved this from In progress to Under Peer Review in MSSQL JDBC Oct 24, 2022
Copy link
Member

@lilgreenbird lilgreenbird left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please run formatter looks like a lot of these files aren't formatted

MSSQL JDBC automation moved this from Under Peer Review to In progress Oct 24, 2022
boolean isAzureFunction = null != identityEndpoint && !identityEndpoint.isEmpty() && null != identityHeader
&& !identityHeader.isEmpty();
throw new SQLServerException(SQLServerException
.getErrString("R_ManagedIdentityTokenAcquisitionFail"), null);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add more info to the error message to make it easier to debug when there's a failure

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, do we have more info if the Optional task fails or doesn't return a result? I don't know the API well...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What additional info did you have in mind? I'll amend the error message to the following to be more clear as to the reason it is null.

Failed to acquire managed identity token. Request for the token succeeded, but no token was returned. The token is null.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure. From the sounds of it, we don't have anything more available. The current iteration looks good.

Copy link
Contributor

@David-Engel David-Engel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partial review

boolean isAzureFunction = null != identityEndpoint && !identityEndpoint.isEmpty() && null != identityHeader
&& !identityHeader.isEmpty();
throw new SQLServerException(SQLServerException
.getErrString("R_ManagedIdentityTokenAcquisitionFail"), null);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, do we have more info if the Optional task fails or doesn't return a result? I don't know the API well...

@tkyc
Copy link
Member Author

tkyc commented Oct 25, 2022

@David-Engel For some reason there's no "reply" button for your comment. I'll just quote you and reply here.

Similarly, do we have more info if the Optional task fails or doesn't return a result? I don't know the API well...

If the Optional task fails, it will throw its own error. The resulting exception, since we're using the Azure Identity credentials, will be a CredentialUnavailableException.

A CredentialUnavailableException indicates the following:

  1. Error in the provided information for the credential eg. wrong client ID of the user-assigned Managed Identity
  2. Connection was dropped mid query for the token

Howerver, there isn't any more available information other than a null token if the Optional succeeds (eg. request for token went through but a null token was returned). In this case, as you suggested (since we shouldn't return null), we'll throw our own error.

@lilgreenbird lilgreenbird moved this from In progress to Under Peer Review in MSSQL JDBC Oct 26, 2022
@tkyc tkyc merged commit 095c7ee into main Oct 26, 2022
MSSQL JDBC automation moved this from Under Peer Review to Closed/Merged PRs Oct 26, 2022
@tkyc tkyc deleted the jdbc-msi-improvements branch October 26, 2022 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
MSSQL JDBC
  
Closed/Merged PRs
Development

Successfully merging this pull request may close these issues.

None yet

4 participants