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

[receiver/azuremonitor] Add support for azure workload identity auth #24543

Merged
merged 8 commits into from
Sep 27, 2023
Merged

[receiver/azuremonitor] Add support for azure workload identity auth #24543

merged 8 commits into from
Sep 27, 2023

Conversation

cmergenthaler
Copy link
Contributor

Description:
Enhances azuremonitor receiver authentication by using azure workload identity. I have added a new config parameter auth to specify the used authentication method (default set to service-principal) in order to possibly add more methods in the future.

Link to tracking Issue: Fixes #24451

Testing: Tested on my AKS

Documentation: Added to README.md

@cmergenthaler cmergenthaler requested a review from a team July 25, 2023 12:40
@cmergenthaler cmergenthaler changed the title Azure monitor workload identity [receiver/azuremonitor] Add support for azure workload identity auth Jul 25, 2023
@cmergenthaler cmergenthaler force-pushed the azure_monitor_workload_identity branch from 8b2482e to 5cae9c8 Compare July 25, 2023 12:40
@github-actions github-actions bot requested a review from altuner July 25, 2023 12:41
fatsheep9146
fatsheep9146 reviewed Jul 26, 2023
View reviewed changes
Copy link
Contributor

@fatsheep9146 fatsheep9146 left a comment

Choose a reason for hiding this comment

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

since the service_principal and workload_identity are both used in multiple places, how about convert them into consts?

@cmergenthaler
Copy link
Contributor Author

since the service_principal and workload_identity are both used in multiple places, how about convert them into consts?

Good point! I have converted them to be consts

@cmergenthaler cmergenthaler force-pushed the azure_monitor_workload_identity branch from 3dfa18b to 40a26b2 Compare July 26, 2023 12:30
altuner
altuner suggested changes Aug 1, 2023
View reviewed changes
Copy link
Contributor

@altuner altuner left a comment

Choose a reason for hiding this comment

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

Good job with improving the receiver, I've added few small comments

receiver/azuremonitorreceiver/README.md Outdated Show resolved Hide resolved
receiver/azuremonitorreceiver/scraper.go Outdated Show resolved Hide resolved
@cmergenthaler cmergenthaler force-pushed the azure_monitor_workload_identity branch from 96a980b to f5adfd9 Compare August 2, 2023 06:18
@fatsheep9146
Copy link
Contributor

@cmergenthaler please fix the failed test, thanks

@cmergenthaler cmergenthaler force-pushed the azure_monitor_workload_identity branch from a4cea7d to f593724 Compare August 2, 2023 09:27
@cmergenthaler cmergenthaler force-pushed the azure_monitor_workload_identity branch from f593724 to 02a8cbb Compare August 14, 2023 07:53
@cmergenthaler
Copy link
Contributor Author

@cmergenthaler please fix the failed test, thanks

@fatsheep9146 done

@cmergenthaler cmergenthaler force-pushed the azure_monitor_workload_identity branch from 02a8cbb to 17eae82 Compare August 14, 2023 10:09
@fatsheep9146
Copy link
Contributor

fatsheep9146 commented Aug 14, 2023 •
edited
Loading

ping @codeboten @altuner for review again, thanks

@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Aug 29, 2023
@github-actions github-actions bot removed the Stale label Aug 30, 2023
@fatsheep9146 fatsheep9146 added the ready to merge Code review completed; ready to merge by maintainers label Aug 30, 2023
@cmergenthaler cmergenthaler force-pushed the azure_monitor_workload_identity branch from 17eae82 to 0c3e038 Compare August 31, 2023 10:57
This was referenced Sep 3, 2023
Open
Open
@cmergenthaler
Copy link
Contributor Author

Anything missing here? Would be nice to have this merged so that I don't have to rebase and resolve conflicts again.

@cmergenthaler cmergenthaler force-pushed the azure_monitor_workload_identity branch 2 times, most recently from 034e656 to 80b6aed Compare September 6, 2023 06:04
@github-actions github-actions bot mentioned this pull request Sep 6, 2023
Open
codeboten
codeboten reviewed Sep 8, 2023
View reviewed changes
Copy link
Contributor

@codeboten codeboten 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 the contribution, just a comment re. constants that don't need to be public. Otherwise it looks good

receiver/azuremonitorreceiver/config.go Outdated Show resolved Hide resolved
@github-actions github-actions bot mentioned this pull request Sep 12, 2023
Open
This was referenced Sep 12, 2023
Open
Open
@github-actions github-actions bot mentioned this pull request Sep 19, 2023
Open
codeboten
codeboten reviewed Sep 19, 2023
View reviewed changes
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

@cmergenthaler please resolve the last conflict and we can get this merged

@github-actions github-actions bot mentioned this pull request Sep 26, 2023
Open
@cmergenthaler
Copy link
Contributor Author

@cmergenthaler please resolve the last conflict and we can get this merged

@codeboten done, please merge

@codeboten
Copy link
Contributor

Thanks @cmergenthaler!

@codeboten codeboten merged commit 0f3d0b3 into open-telemetry:main Sep 27, 2023
92 checks passed
@github-actions github-actions bot added this to the next release milestone Sep 27, 2023
jmsnll pushed a commit to jmsnll/opentelemetry-collector-contrib that referenced this pull request Nov 12, 2023
@cmergenthaler @jmsnll
[receiver/azuremonitor] Add support for azure workload identity auth (o…
7831cd1 
…pen-telemetry#24543)

Enhances azuremonitor receiver authentication by using [azure workload
identity](https://azure.github.io/azure-workload-identity/docs/). I have
added a new config parameter `auth` to specify the used authentication
method (default set to service-principal) in order to possibly add more
methods in the future.

Fixes open-telemetry#24451 

**Testing:** Tested on my AKS
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codeboten codeboten codeboten approved these changes

@altuner altuner altuner approved these changes

@fatsheep9146 fatsheep9146 fatsheep9146 approved these changes

@matej-g matej-g Awaiting requested review from matej-g

@povilasv povilasv Awaiting requested review from povilasv

Assignees

@fatsheep9146 fatsheep9146

Labels
exporter/coralogix ready to merge Code review completed; ready to merge by maintainers receiver/azuremonitor
Projects
None yet
Milestone
v0.87.0
Development

Successfully merging this pull request may close these issues.

[receiver/azuremonitorreceiver] Add support for authenticating using AD workload identity
4 participants
@cmergenthaler @fatsheep9146 @codeboten @altuner