Skip to content

Conversation

@w0ut0
Copy link
Contributor

@w0ut0 w0ut0 commented Aug 12, 2024

Microsoft graph API operator: try to fetch Tenant Id from extra_dejson.tenantId instead of extra_dejson.tenant_id
fixes #41399


^ 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.

client_secret = connection.password
config = connection.extra_dejson if connection.extra else {}
tenant_id = config.get("tenant_id")
tenant_id = config.get("tenantId") or config.get("tenant_id")
Copy link
Contributor

Choose a reason for hiding this comment

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

If we want to change this then we should deprecate the old one.
But also, lets verify all settings. We should not mix two types of syntax's.

Copy link
Contributor

Choose a reason for hiding this comment

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

@w0ut0 thanks for your PR. I would do following:

tenant_id = config.get(“tenant_id”) or config.get(“tenantId”)

No need to check the connection type. I would like to also use the Azure connection type in the future for the MSGraphAsyncOperator as that would make more sense but then some changes would not to be done on the Azure connection type. This would also be interesting for the PR regarding the PowerBi dataset operator of @ambika-garg. But as I said that would require some changes in the Azure connection type, something I didn’t want to mess with yet when I introduced the MSGraphOperator.

@w0ut0
Copy link
Contributor Author

w0ut0 commented Aug 12, 2024

So it seems that the MS Graph operator does not expect an Azure Connection, but rather an HTTP connection, with extra_dejson tenantId.
Would it be ok if I do a check on conn.conn_type?

if conn.conn_type == 'http':
    tenant_id = config.get("tenant_id")
elif conn.conn_type == 'azure':
    tenant_id = config.get("tenantId")

My use case is that we use the same credentials (service principals) to authenticate the Airflow instance, both to the Azure Resource Manager, as well as to the MS Graph API.

Alternatively, we can raise an exception if the supplied conn_id is of the wrong type?

cc @dabla

@dabla
Copy link
Contributor

dabla commented Aug 12, 2024

So it seems that the MS Graph operator does not expect an Azure Connection, but rather an HTTP connection, with extra_dejson tenantId. Would it be ok if I do a check on conn.conn_type?

if conn.conn_type == 'http':
    tenant_id = config.get("tenant_id")
elif conn.conn_type == 'azure':
    tenant_id = config.get("tenantId")

My use case is that we use the same credentials (service principals) to authenticate the Airflow instance, both to the Azure Resource Manager, as well as to the MS Graph API.

Alternatively, we can raise an exception if the supplied conn_id is of the wrong type?

cc @dabla

We could raise an exception once the MSGraph operator would require an Azure connection type, something that would be nice once all parameters vould also be specified in the Azure connection type. At the moment I would keep the original solution and allow both cases with conn_type check. Maybe add a unit test which also test the other tenantId case.

@dabla
Copy link
Contributor

dabla commented Sep 5, 2024

@eladkal I think you can close this one, I've added the support in this PR.

@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Oct 23, 2024
@eladkal eladkal closed this Oct 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providers provider:microsoft-azure Azure-related issues stale Stale PRs per the .github/workflows/stale.yml policy file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Microsoft-Azure - ms graph hook - Tenant ID fetched from wrong extra_dejson key.

3 participants