Skip to content

Conversation

@Adaverse
Copy link
Contributor

@Adaverse Adaverse commented Jun 7, 2023

The option to pass extra configs is essential, for example - when working with a proxy (ex socks) to authenticate as a service principal using ClientSecretCredential. Currently, it isn't possible to do so. Hence added the option to pass extra options to ClientSecretCredential. Have added a few tests to validate it.


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

Copy link
Contributor

Choose a reason for hiding this comment

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

can you please improve the naming for this test_ad_connection

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing out. Changed it to test_azure_directory_connection.

Copy link
Contributor

@phanikumv phanikumv Jun 8, 2023

Choose a reason for hiding this comment

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

Did you verify that connection is being done correctly after this change? Can you please include a screenshot in the description.

May be you need to do that by running a ADF example DAG using an actual azure conn, and verify by running it using a service principal

Copy link
Contributor Author

@Adaverse Adaverse Jun 8, 2023

Choose a reason for hiding this comment

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

I tested it before (on real azure system using the resources which expose through a proxy to Azure) making this PR and just tested rn for double confirmation. I will give the scenario below -
So while passing the below config and using the ad_connection

extra: 
{
	"tenant_id": "xxx",
	"proxies": {"https": "xxx"},
}

produces below error -

ClientSecretCredential.get_token failed: Authentication failed: ('Cannot connect to proxy.', OSError('Tunnel connection failed: 502 Bad Gateway'))

the main reason is proxy config not being passed in the ClientSecretCredential here but instead it should be

token_credential = ClientSecretCredential(tenant, app_id, app_secret, proxies = {...}, authority="...")

But after applying the changes of this PR and setting the extra as below -

{
	"tenant_id": "xxx"
	"proxies": {"https": "xxx"},
	"client_secret_auth_config": {"proxies": {"https": "xxx"}, "authority": "xxxx"}
}

It works as expected without any issues.

And in the test, it is being ensured that client_secret_auth_config is being applied to ClientSecretCredential

@Adaverse Adaverse requested a review from phanikumv June 8, 2023 17:39
@Adaverse
Copy link
Contributor Author

Is anything else needed here?
cc - @phanikumv

@Adaverse
Copy link
Contributor Author

Hi @eladkal
Can we include this in 2.6.2 release by any chance?

@Adaverse
Copy link
Contributor Author

cc - @potiuk

@phanikumv
Copy link
Contributor

Hi @eladkal Can we include this in 2.6.2 release by any chance?

IMO this would be in azure provider release, not in 2.6.2

@Adaverse
Copy link
Contributor Author

Shall we merge this?
cc - @potiuk

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants