Skip to content

Conversation

@gledis69
Copy link
Contributor

@gledis69 gledis69 commented Nov 29, 2022

Why make this change?

  • Add extra support for Azure Managed Identity for Pg. I say extra, because even in the repo's current state, the user can just specify the host and pass the access token as the password in the connection string to authenticate to their managed PG with a managed identity.
  • The extra part is:
    • Now the user can alternatively specify the access token in the config to authenticate with managed identity.
    • Alternatively, now the user can just not specify the password in the connection string and the runtime will attempt to fetch the default managed identity token. If this fails, connection will be attempted without a password in the connection string.

What is this change?

  • Introduces a class PostgreSqlQueryExecutor inspired by how managed identity was supported for Sql Server.

How was this tested?

  • Unit Tests

Copy link
Contributor

@seantleonard seantleonard left a comment

Choose a reason for hiding this comment

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

one nit, otherwise looks good :)

@seantleonard
Copy link
Contributor

I think it's fair to denote USE DEFAULT for deferring to system assigned managed identity. The behavior is needed, though if we want to enhance in the future, we could consider updating the runtime configuration schema to ask a user to use a different JSON value like true

Copy link
Collaborator

@Aniruddh25 Aniruddh25 left a comment

Choose a reason for hiding this comment

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

Looks good for hosted scenario, but unclear waiting on how "USE DEFAULT" will be set by user and identified by RuntimeConfigProvider

@Aniruddh25
Copy link
Collaborator

Through offline discussion, it was decided to tackle this as follows:
If password is absent in the connection string, can we imply in decreasing order of preference:

  1. Managed Identity scenario - attempt to get the token
  2. Passwordless Pg User
    eventually, fail startup of DAB if none of the scenarios succeed auth

@Aniruddh25 Aniruddh25 dismissed their stale review December 8, 2022 14:03

I will again be OOF starting 12/12, dont want my review to block the merge of this PR

Copy link
Collaborator

@Aniruddh25 Aniruddh25 left a comment

Choose a reason for hiding this comment

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

Thank you for pushing through this change for the Nov2022 release, really appreciate it!
Left 1 optimization for passwordless scenarios, otherwise LGTM.

@gledis69 gledis69 force-pushed the managed-identity-support-pg branch from 544a141 to 835975f Compare December 9, 2022 18:06
@gledis69 gledis69 enabled auto-merge (squash) December 9, 2022 19:06
@gledis69 gledis69 disabled auto-merge December 9, 2022 19:06
@gledis69 gledis69 merged commit 75faf0b into main Dec 9, 2022
@gledis69 gledis69 deleted the managed-identity-support-pg branch December 9, 2022 19:10
@Aniruddh25 Aniruddh25 linked an issue Dec 9, 2022 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PgSql: Support for Managed Identity Access

4 participants