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

azuredns: provide the ability to select authentication methods #2026

Conversation

pchanvallon
Copy link
Contributor

@pchanvallon pchanvallon commented Sep 27, 2023

Hello,

After using azuredns new DNS provider, I figured out it could be really useful to manage which authentication methods to use, in order to configure more accurately the expected behavior.

Therefore, I have splited the DefaultAzureCredential by using the different Azure credential methods and added the possibility to disable some of them.

Note that for NewManagedIdentityCredential I added a timeout management as described in the azidentity package documentation

Let me know if you want me to add anything.
Thanks.

@pchanvallon pchanvallon force-pushed the feat/azuredns-configure-authentication-methods branch from f4ab2aa to e2a643b Compare September 27, 2023 19:51
@ldez ldez changed the title feat(azuredns): provide the ability to select authentication methods azuredns! provide the ability to select authentication methods Sep 27, 2023
@ldez ldez changed the title azuredns! provide the ability to select authentication methods azuredns: provide the ability to select authentication methods Sep 27, 2023
@ldez
Copy link
Member

ldez commented Sep 27, 2023

Hi, what is the value of disabling some auth methods instead of choosing only one?

@pchanvallon
Copy link
Contributor Author

Hello,
The purpose is to avoid breaking changes while introducing this feature.
The behavior of DefaultAzureCredentials is to provide a builtin fallback mechanism between the different authentication methods. By enabling all of them by default, we are keeping the behavior as is and provide the ability to configure which one should be used.
The other way round is also possible but it will introduce some changes in terms of configuration.

@ldez
Copy link
Member

ldez commented Sep 28, 2023

We can keep the default and choose one with only one option.

@pchanvallon
Copy link
Contributor Author

Hello, I have updated the PR according to your suggestion.

@ldez
Copy link
Member

ldez commented Sep 29, 2023

I think you did not understand my 2 previous messages: I was speaking about 1 option instead of 5 options, and not changing the default option values.

@pchanvallon
Copy link
Contributor Author

Hello,
The goal of using different variables was to mimic the azurerm terraform provider configuration.
I have no cons to use only one variable as we still have the default credential method as fallback.
Therefore I updated the PR accordingly.

@ldez
Copy link
Member

ldez commented Oct 2, 2023

You can mimic the terraform provider but I need to know if being able to use several auth methods is a real thing or not, it's not a pro/con.

I need more concrete arguments to justify if auth method fallback is a thing for Azure users.

@pchanvallon
Copy link
Contributor Author

pchanvallon commented Oct 3, 2023

The fallback mechanism can be useful during the development phase on a local machine. The DefaultAzureCredentials was made for that purpose as described in the Azure SDK documentation:

DefaultAzureCredential simplifies authentication by combining commonly used credential types. It chains credential types used to authenticate Azure-deployed applications with credential types used to authenticate in a development environment.

In a production environment for a workload or CI/CD usage, defining one and only one authentication method is viable because it is determined by the context of the workload or CI/CD environment.

@ldez ldez added this to the v4.15 milestone Oct 5, 2023
@ldez ldez enabled auto-merge (squash) October 5, 2023 11:04
@ldez ldez merged commit bf8c7ab into go-acme:master Oct 5, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

2 participants