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

feat(inputs.sqlserver): Introduce user specified ID parameter for ADD logins #15424

Merged

Conversation

Jan747
Copy link
Contributor

@Jan747 Jan747 commented May 30, 2024

Summary

If an Azure VM has more than one user assigned identiy, the authentication with the Azure SQL DB does not work.

Another method to get a token has been introduced for this and a new parameter to select an identy.

Checklist

  • No AI generated code was used in this PR

Related issues

resolves #15404

…to get tokens for the specified UserAssignedID
@telegraf-tiger
Copy link
Contributor

Thanks so much for the pull request!
🤝 ✒️ Just a reminder that the CLA has not yet been signed, and we'll need it before merging. Please sign the CLA when you get a chance, then post a comment here saying !signed-cla

Copy link
Contributor

@powersj powersj 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 PR! I appreciate you taking the time to put this up.

I have added some comments. The only big one is what to call the option. You will also need to run make docs, or update the sqlserver README.md as well to ensure it has the new config option in the example. make docs does this for you.

plugins/inputs/sqlserver/sqlserver.go Outdated Show resolved Hide resolved
plugins/inputs/sqlserver/sqlserver.go Outdated Show resolved Hide resolved
plugins/inputs/sqlserver/sample.conf Outdated Show resolved Hide resolved
plugins/inputs/sqlserver/README.md Outdated Show resolved Hide resolved
@powersj powersj self-assigned this May 30, 2024
@powersj powersj added the waiting for response waiting for response from contributor label May 30, 2024
@powersj powersj changed the title Added parameter UserAssignedID and modified refreshToken() to enable … feat(inputs.sqlserver): Introduce user specified ID parameter for ADD logins May 30, 2024
@telegraf-tiger telegraf-tiger bot added area/sqlserver feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins labels May 30, 2024
@powersj
Copy link
Contributor

powersj commented May 30, 2024

Also need you to sign the CLA please.

@telegraf-tiger telegraf-tiger bot removed the waiting for response waiting for response from contributor label May 30, 2024
@powersj powersj added the waiting for response waiting for response from contributor label May 30, 2024
Jan747 and others added 2 commits May 31, 2024 11:35
remove unnessary vars

Co-authored-by: Joshua Powers <powersj@fastmail.com>
@Jan747
Copy link
Contributor Author

Jan747 commented May 31, 2024

Hello @powersj thank you very much for your comments and tips.

I improved the PR. Please have another look on it.

@telegraf-tiger telegraf-tiger bot removed the waiting for response waiting for response from contributor label May 31, 2024
@powersj
Copy link
Contributor

powersj commented May 31, 2024

!signed-cla

@powersj
Copy link
Contributor

powersj commented May 31, 2024

!signed-cla

@powersj
Copy link
Contributor

powersj commented May 31, 2024

@Jan747 - Thank you very much for the updates and signing the CLA. I pushed an update to ensure the linter and doc tests pass. Can you confirm you have been able to use your proposed fix and it resolves your issue?

@powersj powersj added the waiting for response waiting for response from contributor label May 31, 2024
@powersj
Copy link
Contributor

powersj commented Jun 3, 2024

@Jan747,

I need to land this early this week so we can make v1.31.0, can you confirm the PR artifacts were able to resolve your issue and you are happy with my updates?

Thanks again!

@telegraf-tiger telegraf-tiger bot removed the waiting for response waiting for response from contributor label Jun 3, 2024
@powersj powersj added the waiting for response waiting for response from contributor label Jun 3, 2024
@Jan747
Copy link
Contributor Author

Jan747 commented Jun 5, 2024

Hello @powersj sorry for the late response. I wasn't able to get the version running. Docker image gave error that entrypoint.sh is missing. So I can't say if the error is now fixed or not.

@telegraf-tiger telegraf-tiger bot removed the waiting for response waiting for response from contributor label Jun 5, 2024
@powersj
Copy link
Contributor

powersj commented Jun 5, 2024

Docker image gave error that entrypoint.sh is missing

Ah, if you share you how you are trying to build a docker image we might be able to help. Our official images use https://github.com/influxdata/influxdata-docker/tree/master/telegraf

@powersj powersj added the waiting for response waiting for response from contributor label Jun 5, 2024
@macxs
Copy link

macxs commented Jun 13, 2024

I can confirm this build is working.
@Jan747 @powersj Thank you!

The error message from before is gone now and logs are clean.

...
2024-06-13T08:23:20Z I! Starting Telegraf 1.31.0-c549b29c brought to you by InfluxData the makers of InfluxDB
...
2024-06-13T08:23:20Z I! [inputs.sqlserver] Config: database_type: AzureSQLDB , query_version:0 , azuredb: false
2024-06-13T08:23:20Z I! [inputs.sqlserver] Config: Effective Queries: []string{"AzureSQLDBOsWaitstats", "AzureSQLDBResourceStats", "AzureSQLDBWaitStats", "AzureSQLDBDatabaseIO", "AzureSQLDBResourceGovernance", "AzureSQLDBPerformanceCounters"}

@telegraf-tiger telegraf-tiger bot removed the waiting for response waiting for response from contributor label Jun 13, 2024
@powersj
Copy link
Contributor

powersj commented Jun 13, 2024

@Jan747 - can you rebase on master and resolve the one conflict? Then we can get this landed.

@Jan747
Copy link
Contributor Author

Jan747 commented Jun 14, 2024

@macxs thank you very much for testing.

@powersj confilct is solved. Anything else what I have todo ? And thank you very much again.

@telegraf-tiger
Copy link
Contributor

Download PR build artifacts for linux_amd64.tar.gz, darwin_arm64.tar.gz, and windows_amd64.zip.
Downloads for additional architectures and packages are available below.

⚠️ This pull request increases the Telegraf binary size by 1.60 % for linux amd64 (new size: 243.5 MB, nightly size 239.7 MB)

📦 Click here to get additional PR build artifacts

Artifact URLs

DEB RPM TAR GZ ZIP
amd64.deb aarch64.rpm darwin_amd64.tar.gz windows_amd64.zip
arm64.deb armel.rpm darwin_arm64.tar.gz windows_arm64.zip
armel.deb armv6hl.rpm freebsd_amd64.tar.gz windows_i386.zip
armhf.deb i386.rpm freebsd_armv7.tar.gz
i386.deb ppc64le.rpm freebsd_i386.tar.gz
mips.deb riscv64.rpm linux_amd64.tar.gz
mipsel.deb s390x.rpm linux_arm64.tar.gz
ppc64el.deb x86_64.rpm linux_armel.tar.gz
riscv64.deb linux_armhf.tar.gz
s390x.deb linux_i386.tar.gz
linux_mips.tar.gz
linux_mipsel.tar.gz
linux_ppc64le.tar.gz
linux_riscv64.tar.gz
linux_s390x.tar.gz

@powersj powersj added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Jun 14, 2024
@powersj powersj assigned srebhan and DStrand1 and unassigned powersj Jun 14, 2024
Copy link
Contributor

@srebhan srebhan 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 your contribution @Jan747!

@srebhan srebhan merged commit df78bc2 into influxdata:master Jun 17, 2024
27 checks passed
@github-actions github-actions bot added this to the v1.32.0 milestone Jun 17, 2024
@Jan747 Jan747 deleted the fix/input_sqlserver_aad_userassignedid branch June 17, 2024 10:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/sqlserver feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SQL Server AAD Azure Auth method doesn't work if multiple User assigned identies are existing.
5 participants