Loki: Add support for Azure Workload Identity authentication #7540

Tolsto · 2022-10-28T00:22:10Z

What this PR does / why we need it:

Adds support for Azure Workload Identity authentication.

CLAassistant · 2022-10-28T00:22:14Z

All committers have signed the CLA.

jkroepke · 2022-10-28T06:03:39Z

Please validate, if it re-reads the token after a hour, since the token file is rotating each hour.

kavirajk

Thanks for PR @Tolsto. Added one minor feedback. Can you please add small test to check if adal.NewServicePrincipalTokenFromFederatedToken works correctly with this additional flag and configs?

kavirajk · 2022-11-01T09:07:53Z

pkg/storage/chunk/client/azure/blob_storage_client.go

+		var environmentName string
+		if b.cfg.Environment == azureGlobal {
+			environmentName = "AzurePublicCloud"
+		} else {
+			environmentName = b.cfg.Environment
+		}


Can you add AzurePublicCloud as part of the list of const Environments top of this file.

Also you could avoid else here.

environmentName := azurePublicCloud if b.cfg.Environment != azureGlobal { environmentName = b.cfg.Environment }

looks more readable IMHO.

Q: Also curious what's the difference between AzureGlobal vs AzurePublicCloud environments?

I don't know where AzureGlobal is coming from but it doesn't exist in the Azure Go SDKs. It's AzurePublicCloud there, i.e. you cannot do an environment lookup via EnvironmentFromName with AzureGlobal

The workload identity webhook is exposing a authority host AZURE_AUTHORITY_HOST which is the env.ActiveDirectoryEndpoint. Consider to prefer the environment variable instead a own cloud env lookup

I thought about that but decided to go with the reference implementation outlined by the person who added the code to adal. I also looked at the new azidentity module where AZURE_AUTHORITY_HOST gets only used as a fallback option. It will only honor it if no cloud configuration has been set. However, the current implementation of Azure storage in Loki requires the cloud environment name to be set, i.e. adding fallback logic is imho futile.

grafanabot · 2022-11-02T01:58:16Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

jkroepke · 2022-11-03T14:30:46Z

FYI: Workload Identity will be official supported jan 2023 - Azure/azure-sdk-for-go#15615

Edit:

ref: #5413 - why discontinued azure-storage-blob is used.

Tolsto · 2022-11-03T17:06:49Z

FYI: Workload Identity will be official supported jan 2023 - Azure/azure-sdk-for-go#15615

Doesn't help us much as Loki doesn't use azure-sdk-for-go for the Azure storage integration but the discontinued azure-storage-blob SDK.

grafanabot · 2022-11-04T05:40:30Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

pkg/storage/chunk/client/azure/blob_storage_client.go

grafanabot · 2022-11-08T14:15:06Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

pkg/storage/chunk/client/azure/blob_storage_client.go

grafanabot · 2022-12-01T06:25:57Z

./tools/diff_coverage.sh ../loki-target-branch/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

Tolsto · 2022-12-13T14:29:20Z

@kavirajk Anything missing for this PR to move forward?

keisari-ch · 2022-12-20T13:15:32Z

Would be a nice addition, because actual workaround using sidecar requires enabling privilege escalation.

Tolsto · 2023-01-10T17:32:57Z

ping @kavirajk

jeschkies

Unfortunately I don't know Azure well enough to accept this change. I does look good to me though.

jeschkies · 2023-01-11T13:25:33Z

pkg/storage/chunk/client/azure/blob_storage_client_test.go

+	mockedServicePrincipalToken *adal.ServicePrincipalToken
+}
+
+func (suite *FederatedTokenTestSuite) SetupTest() {


Interesting. Are you using the suite to get a test setup code?

I use the suite to set up an isolated environment with mocks for testing the Azure federated token functionality. There might be other ways to do it, but I come from the Ruby/Rspec world, and using a suite was the most familiar approach for me.

jeschkies · 2023-01-11T13:27:15Z

docs/sources/configuration/_index.md

@@ -829,6 +829,13 @@ The `azure_storage_config` configures Azure as a general storage for different d
 # CLI flag: -<prefix>.azure.use-managed-identity
 [use_managed_identity: <boolean> | default = false]

+# Use a Federated Token to authenticate to the Azure storage account.


Would Azure users understand what a federated token is? Do you have a link to the documentation?

I'd suppose so.

https://azure.github.io/azure-workload-identity/docs/topics/federated-identity-credential.html
https://learn.microsoft.com/en-us/azure/active-directory/develop/workload-identity-federation

Could you add the second link in the documentation? I think that would be helpful.

Also, I know this is a big ask but could you provide documentation similar to https://grafana.com/docs/loki/next/storage/#aws-deployment-s3-single-store

I don't think that the second link is very helpful as it describes the federated token architecture in general. >90% of Loki users will use it with the reference implementation (Azure Workload Identity) in a Kubernetes cluster.
I'm willing to provide documentation if this PR can then get merged in the very near future.

Sounds good to me 😊

jeschkies · 2023-01-12T08:26:00Z

@Tolsto once you've fixed the merge conflicts we can land the change.

Tolsto requested a review from a team as a code owner October 28, 2022 00:22

pull-request-size bot added the size/M label Oct 28, 2022

github-actions bot added the type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories label Oct 28, 2022

Tolsto force-pushed the azure-workload-identity branch 2 times, most recently from 0ecbde2 to 4134182 Compare October 28, 2022 00:36

Tolsto mentioned this pull request Oct 28, 2022

[Helm] Add support for azure blob storage #7500

Merged

5 tasks

Tolsto force-pushed the azure-workload-identity branch from 4134182 to 93e30f9 Compare October 28, 2022 01:21

kavirajk reviewed Nov 1, 2022

View reviewed changes

Tolsto force-pushed the azure-workload-identity branch from 93e30f9 to 3f40965 Compare November 2, 2022 01:09

pull-request-size bot added size/L and removed size/M labels Nov 2, 2022

Tolsto force-pushed the azure-workload-identity branch from 3f40965 to 6120ef4 Compare November 2, 2022 01:53

Tolsto requested a review from kavirajk November 2, 2022 02:04

Tolsto force-pushed the azure-workload-identity branch 2 times, most recently from 30a571a to 32d4d5c Compare November 4, 2022 05:35

weisdd approved these changes Nov 7, 2022

View reviewed changes

weisdd suggested changes Nov 7, 2022

View reviewed changes

pkg/storage/chunk/client/azure/blob_storage_client.go Show resolved Hide resolved

Tolsto force-pushed the azure-workload-identity branch from 32d4d5c to 853e577 Compare November 8, 2022 14:08

Tolsto requested review from weisdd and removed request for kavirajk November 9, 2022 06:31

weisdd suggested changes Nov 9, 2022

View reviewed changes

pkg/storage/chunk/client/azure/blob_storage_client.go Show resolved Hide resolved

weisdd approved these changes Nov 9, 2022

View reviewed changes

Tolsto requested a review from kavirajk November 15, 2022 08:50

Add support for Azure Workload Identity

230d0b0

Tolsto force-pushed the azure-workload-identity branch from 853e577 to 230d0b0 Compare December 1, 2022 06:20

Add Helm config for Azure Workload Identity

9d27bc0

github-actions bot added the area/helm label Dec 1, 2022

jeschkies reviewed Jan 11, 2023

View reviewed changes

jeschkies approved these changes Jan 12, 2023

View reviewed changes

jeschkies enabled auto-merge (squash) January 12, 2023 08:25

Merge branch 'main' into azure-workload-identity

4bcd3d3

jeschkies merged commit e4fbd3c into grafana:main Jan 13, 2023

LambArchie mentioned this pull request Mar 13, 2023

Add Azure Workload Identity authentication support grafana/tempo#2195

Merged

3 tasks

Tolsto mentioned this pull request Apr 18, 2023

Re-add helm configuration for Azure OIDC federation #9168

Merged

5 tasks

rowanmoul mentioned this pull request May 8, 2024

Azure storage with federated workload identity #12614

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Loki: Add support for Azure Workload Identity authentication #7540

Loki: Add support for Azure Workload Identity authentication #7540

Tolsto commented Oct 28, 2022

CLAassistant commented Oct 28, 2022 •

edited

jkroepke commented Oct 28, 2022

kavirajk left a comment

kavirajk Nov 1, 2022

Tolsto Nov 1, 2022

jkroepke Nov 1, 2022

Tolsto Nov 1, 2022 •

edited

grafanabot commented Nov 2, 2022

jkroepke commented Nov 3, 2022 •

edited

Tolsto commented Nov 3, 2022

grafanabot commented Nov 4, 2022

grafanabot commented Nov 8, 2022

grafanabot commented Dec 1, 2022

Tolsto commented Dec 13, 2022

keisari-ch commented Dec 20, 2022

Tolsto commented Jan 10, 2023

jeschkies left a comment

jeschkies Jan 11, 2023

Tolsto Jan 11, 2023

jeschkies Jan 11, 2023

Tolsto Jan 11, 2023

jeschkies Jan 11, 2023

Tolsto Jan 11, 2023

jeschkies Jan 12, 2023

jeschkies commented Jan 12, 2023

Loki: Add support for Azure Workload Identity authentication #7540

Loki: Add support for Azure Workload Identity authentication #7540

Conversation

Tolsto commented Oct 28, 2022

CLAassistant commented Oct 28, 2022 • edited

jkroepke commented Oct 28, 2022

kavirajk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tolsto Nov 1, 2022 • edited

Choose a reason for hiding this comment

grafanabot commented Nov 2, 2022

jkroepke commented Nov 3, 2022 • edited

Tolsto commented Nov 3, 2022

grafanabot commented Nov 4, 2022

grafanabot commented Nov 8, 2022

grafanabot commented Dec 1, 2022

Tolsto commented Dec 13, 2022

keisari-ch commented Dec 20, 2022

Tolsto commented Jan 10, 2023

jeschkies left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeschkies commented Jan 12, 2023

CLAassistant commented Oct 28, 2022 •

edited

Tolsto Nov 1, 2022 •

edited

jkroepke commented Nov 3, 2022 •

edited