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.

Sign up for GitHub

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

Jump to bottom

Migrate client secret auth to Grafana Azure SDK #358

Merged
merged 6 commits into from
Apr 27, 2022
Merged

Migrate client secret auth to Grafana Azure SDK #358

merged 6 commits into from
Apr 27, 2022

Conversation

kostrse
Copy link
Contributor

@kostrse kostrse commented Apr 13, 2022

Introduced dependency on the external module github.com/grafana/grafana-azure-sdk-go.

App Registration (client secret) authentication was migrated to the library. OBO still implemented in ADX because it's not supported by the library.

@kostrse kostrse requested a review from a team as a code owner April 13, 2022 22:09
@kostrse kostrse requested review from iwysiu and vickyyyyyyy and removed request for a team April 13, 2022 22:09
@kostrse
Copy link
Contributor Author

kostrse commented Apr 13, 2022

cc @sunker @andresmgot

@fridgepoet
Copy link
Member

(Asked a couple of questions about scope since I saw #356. Sorry if you might have discussed these with someone else on the team or if these are obvious for you.)

fridgepoet
fridgepoet reviewed Apr 15, 2022
View reviewed changes
pkg/azuredx/azureauth/auth.go Outdated Show resolved Hide resolved
pkg/azuredx/azureauth/auth.go Outdated Show resolved Hide resolved
pkg/azuredx/azureauth/scopes_test.go Show resolved Hide resolved
pkg/azuredx/azureauth/auth_test.go Outdated Show resolved Hide resolved
@sunker
Copy link
Contributor

sunker commented Apr 19, 2022

Nice - this is much cleaner than before. Tested with app registration and OBO and it's working as expected. I'll leave the approval of this PR for @fridgepoet.

sunker
sunker requested changes Apr 20, 2022
View reviewed changes
Copy link
Contributor

@sunker sunker left a comment

Choose a reason for hiding this comment

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

Actually I'm getting invalid Azure credentials: the cloud '' not supported. Seems like ADX was not strict about setting azureCloud in the json data, so empty string needs to be treated as public cloud. Can you do that in auth.go?

@kostrse kostrse mentioned this pull request Apr 25, 2022
Closed
10 tasks
@kostrse
Copy link
Contributor Author

kostrse commented Apr 27, 2022

Actually I'm getting invalid Azure credentials: the cloud '' not supported. Seems like ADX was not strict about setting azureCloud in the json data, so empty string needs to be treated as public cloud. Can you do that in auth.go?

Thanks for pointing at this. Fixed. Unset cloud defaults to public now, any other unknown values fail.

@kostrse kostrse requested a review from sunker April 27, 2022 09:01
sunker
sunker approved these changes Apr 27, 2022
View reviewed changes
Copy link
Contributor

@sunker sunker left a comment

Choose a reason for hiding this comment

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

Thanks for applying changes! Tested and it's working as expected.

@fridgepoet looks like all of your comments have been addressed. Is it okay to merge?

fridgepoet
fridgepoet approved these changes Apr 27, 2022
View reviewed changes
Copy link
Member

@fridgepoet fridgepoet left a comment

Choose a reason for hiding this comment

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

Yes!

@andresmgot andresmgot merged commit 5f4167e into grafana:main Apr 27, 2022
@kostrse kostrse deleted the migrate-to-grafana-azure-sdk branch April 27, 2022 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fridgepoet fridgepoet fridgepoet approved these changes

@sunker sunker sunker approved these changes

@iwysiu iwysiu Awaiting requested review from iwysiu iwysiu was automatically assigned from grafana/aws-datasources

@vickyyyyyyy vickyyyyyyy Awaiting requested review from vickyyyyyyy vickyyyyyyy was automatically assigned from grafana/aws-datasources

Assignees
No one assigned
Labels
type/chore
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kostrse @fridgepoet @sunker @andresmgot