Skip to content

Adding Managed Identity for Access Token#48

Open
hecflores wants to merge 2 commits into
RamonPage:mainfrom
hecflores:main
Open

Adding Managed Identity for Access Token#48
hecflores wants to merge 2 commits into
RamonPage:mainfrom
hecflores:main

Conversation

@hecflores

Copy link
Copy Markdown

I added code to support managed identity authentication.

I have not tested this! Dont complete right away - Can you walk me through the process for testing the changes and/or any patterns that I did that don't align to yours?

@RamonPage

Copy link
Copy Markdown
Owner

Hi! Thank you for your contribution! ✨

Comment thread lib/ex_azure_key_vault.ex
}

"""
@spec smart_connect() :: Client.t() | {:error, any}

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This looks out of scope for this feature. Could you move this piece to a separate PR for further discussion?

Comment thread lib/ex_azure_key_vault.ex
Client.t() | {:error, any}
def msi_connect(vault_name \\ nil) do
vault_name = get_env(:azure_vault_name, vault_name)
endpoint = System.get_env("IDENTITY_ENDPOINT")

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I'd suggest you move the IDENTITY_ENDPOINT and IDENTITY_HEADER env variables to the config files for each environment. This way you can leverage the get_env() function for them as well.

Comment thread lib/ex_azure_key_vault.ex
}

"""
@spec connect!() :: Client.t() | {:error, any}

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This seems out of scope as well. Also, although I understand the motivation for the ! function, the current connect() already raises exceptions.

Please open a PR or issue with this suggestion if you want to discuss more.

Comment thread lib/ex_azure_key_vault.ex
}

"""
@spec cert_connect!() :: Client.t() | {:error, any}

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Same as connect!(). I'm happy to discuss more on a separate issue/PR.

@RamonPage

Copy link
Copy Markdown
Owner

This project is configured to expect 100% coverage. Could you include some tests?

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.

3 participants