-
Notifications
You must be signed in to change notification settings - Fork 106
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
Add support for Azure authentication #785
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Jan-Otto Kröpke <[email protected]>
# Conflicts: # go.mod # go.sum # pkg/models/settings.go
I fixed the conflict. @yesoreyeram let me know If I can help/assist here. |
Hi @jkroepke - Thank you so much for the contribution. I am planning to take a look at this later this week. Sorry for the delay. Will keep you posted. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall looks good to me. Left some nit comments.
# Conflicts: # go.mod # go.sum
Signed-off-by: Jan-Otto Kröpke <[email protected]>
I may ask myself, how this may can work, if os.LookupEnv does not work. At the end, Ref: |
Hi.. Thanks for the pointers. We have discussed this lot internally on similar topics. As grafana plugins are proceeding towards multitenant/remote plugins, we are already removing such os level things in our plugins code already. Adding this now to infinity will make things harder. In future, yes we can re-evaluate this decision and come up with better alternatives. |
I figure out that Grafana admins needs to opt-in to allow such authentication. https://grafana.com/docs/grafana/latest/setup-grafana/configure-grafana/#azure Grafana pass the azure settings to plugins context, and the grafana-azure-sdk can be make aware of them
I had rewrite the plugin datasource config. The plugins will take care of the azure settings for now. It will allow only the specific authentications, if the Grafana admin has allowed it via grafana.ini. For an out of the box experience, I would recommend to add this plugin there: https://github.com/grafana/grafana/blob/cc87281d7153d4b6b0edd365efee94d199a4f79b/conf/defaults.ini#L968 Maybe you can take an eye here. |
Signed-off-by: Jan-Otto Kröpke <[email protected]>
Signed-off-by: Jan-Otto Kröpke <[email protected]>
Again sorry for the delay and thank you for the patience. I am still on top of this PR and hopefully can merge this week. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like something broke after a merge commit. I'm I still have a compiled version locally, but I did do a retest after the merge conflict |
Signed-off-by: Jan-Otto Kröpke <[email protected]>
@yesoreyeram It seems like the error you get is coming from Grafana SDK, if The errors comes from the Grafana SDK: https://github.com/grafana/grafana-azure-sdk-go/blob/c271284be0be9f85c64b6d2c8d00d22e458dc80f/azsettings/env.go#L86 |
Hi there.. again sorry for the delay. you can do yarn spellcheck locally and fix the missing keywords in cspell.json file |
Signed-off-by: Jan-Otto Kröpke <[email protected]>
great, guessing it is ready to merge now? |
Signed-off-by: Jan-Otto Kröpke <[email protected]>
Hey @yesoreyeram my motivation to resolve a merge conflict one more is quite low. Should I close this PR? It seems like that grafana-infinity-datasource has serious issues with external contributions |
Hi @jkroepke - Sorry for the annoyance. We are quite busy and couldn't focus much on this PR. also last time when I tried to merge, CI was failing. Let's home this goes through. Happy to merge once CI passed. |
seems CI tests failing again. Can you check? |
Done. It seems like additional check (lint) where introduced. If the CI would automatic start, I could fix the CI issues on my own. However I have to ask someone each time to trigger the CI which makes to process terrible slow. Maybe switch from CircleCI to GH Actions to solve such issues. |
CI is green now |
Hey @yesoreyeram can we merge this before an merge conflict is introduced again resulting into red CI pipelines? |
Hey @yesoreyeram what are the next steps here? tbh, it's also time waste for you to recurring check this. I recommend to merge this and any additional errors can be resolved than. |
Signed-off-by: Jan-Otto Kröpke <[email protected]>
Signed-off-by: Jan-Otto Kröpke <[email protected]>
Hi @jkroepke this is blocked on our side, due to internal discussions on authentication architecture, which may take a little while. We understand that you put a lot of effort in this PR and we are sorry it is taking so long. |
Hi @jkroepke @yesoreyeram @ivanahuckova Background information: |
Yep, within this PR, scopes can be configured. You can also use Managed Identities to gain access to KeyVault password-less. |
This would be awesome! Would solve a lot of problems and simplify several things, nowadays i would need to write custom prometheus exporter for doing this but which would lead to more effort instead of using alternatives like this one here <3 |
Not sure, if it helps: https://github.com/webdevops/azure-keyvault-exporter |
Thanks for the link, already familiar with it :) Markus Blaschke is a former colleague of mine :) This looks really good but the issue here this that you also would need to dockerize it, create a helm chart, deploy it, maintain it, patch it....and you have to think about where to deploy it later. Makes it pretty difficult if you do not have one single environment but several ones without any multi tenancy setup. All about processes: In enterprise companies you then have also to take care about OSS, license and vulnerability such as security stuff. Converting that into money would mean high costs. That is why it would be nice to use the plugin for doing that. You can automate the whole deployment via terraform, starting with Grafana, then the dashboards and last one the datasources (like this one here: https://registry.terraform.io/providers/grafana/grafana/latest/docs/resources/data_source.html) Since already mentioned above in Grafana would make the setup more attractive since i would then only need to take care about Grafana and not where i put also then the exporter -> there i would think about using Kubernetes or any other Container Solution with Prometheus integration. |
@mrmiyagietas we wrote a custom proxy between grafana-infinity-datasource and the upstream URL which injects the authentication and may do other things, e.g. pagination. In other words, I give up here. |
Fixes #768
This PR adds native Azure authentication, provided by
github.com/grafana/grafana-azure-sdk-go
.It supports: