Add support for workload identity in the Azure CosmosDB Key/Value impl #2566

devigned · 2024-06-15T23:55:44Z

This PR adds the ability for the Azure CosmosDB KV store implementation to use ambient authentication (managed identity, workflow identity, azure cli). The PR should not break existing users of the key authentication mechanism; it should only be additive. For more information about the Azure Rust SDK identity flows, check out: https://github.com/Azure/azure-sdk-for-rust/blob/main/sdk/identity/README.md.

Here is the associated infrastructure and sample application to demo this identity flow: https://github.com/devigned/spin-workload-id.

I'm opening this now to start getting feedback. I (or the identity SDK) also likely have a bug related to the authentication scope being passed to Entra (previously Azure Active Directory). I will sort that issue out and notify via the PR.

This work is related to spinkube/spin-operator#252.

If you have Azure CLI installed, you can test this locally doing the following:

# Create an Azure CosmosDB
az cosmosdb create -n cosmosdb-spin-test -g test --locations regionName=southcentralus
# Create a database in our account
az cosmosdb database create -a cosmosdb-spin-test -g test -n spin
# Create a container (table) in our database
az cosmosdb container create -a cosmosdb-spin-test -g test -n  -d spin -p "/id"
# Get your subscription ID
SUB_ID=$(az account show --query "id" -o tsv)
# Get your user principal ID
ID=$(az ad user show --id your_user@your_domain.com --query "id" -o tsv)
# Add the role assignment to enable data mutations for your user
az cosmosdb sql role assignment create -a cosmosdb-spin-test -g test \
  --role-definition-name "00000000-0000-0000-0000-000000000002" \
  -s "/subscriptions/${SUB_ID}/resourceGroups/test/providers/Microsoft.DocumentDB/databaseAccounts/cosmosdb-spin-test" \
  -p ${ID}

Your corresponding runtime config should look like the following. Note there is no key used.

[key_value_store.default]
type = "azure_cosmos"
account = "cosmosdb-spin-test"
database = "spin"
container = "keys-and-values"

devigned · 2024-06-16T14:22:08Z

Also to note on this PR is that the changes will also allow users to run locally using the Azure CLI to authenticate to Azure CosmosDB. This could be a convenient way of testing / developing with Azure resources without needing to deploy to Azure.

devigned · 2024-06-16T14:23:41Z

The issue linked (az/rust#1593) is what needs to be resolved & verified prior to completing this PR.

devigned · 2024-06-17T16:05:29Z

Ok, with Azure/azure-sdk-for-rust#1678 this works as expected. Currently, I am pointing at my fixed branch of the Azure SDK for Rust. Do we want to delay this getting merged until the Azure Rust SDK merges the fix?

devigned · 2024-06-18T12:49:31Z

The fix to the Azure SDK for Rust has been merged and I've updated this PR to point at that the ref of that change in main. I believe this PR is ready for review.

devigned · 2024-06-18T12:54:17Z

crates/trigger/src/runtime_config/key_value.rs

@@ -122,19 +124,33 @@ impl RedisKeyValueStoreOpts {

 #[derive(Clone, Debug, Deserialize)]
 pub struct AzureCosmosConfig {


Should this struct contain an untagged enum (https://serde.rs/enum-representations.html#untagged) rather than defaulted values?

Hmm, maybe. It seems like a user should have environmental_auth or key but not both. But I'm not sure the two cases are different enough to justify the extra complexity and the weird unhelpful errors that untagged enums give us. I'm actually wondering, is environmental_auth ever needed? That is, is the presence of key enough to signal "use this key instead of the one in the environment"? If so, would it make sense to make key an Option<String> and ditch environmental_auth altogether?

itowlson · 2024-06-20T22:34:31Z

@devigned I think you have some clippies (and autoformats maybe?) that are unrelated to the changes. I believe these are already picked up in #2569 so you'll probably need to rebase.

itowlson · 2024-06-20T22:42:32Z

crates/trigger/src/runtime_config/key_value.rs

@@ -122,19 +124,33 @@ impl RedisKeyValueStoreOpts {

 #[derive(Clone, Debug, Deserialize)]
 pub struct AzureCosmosConfig {


Hmm, maybe. It seems like a user should have environmental_auth or key but not both. But I'm not sure the two cases are different enough to justify the extra complexity and the weird unhelpful errors that untagged enums give us. I'm actually wondering, is environmental_auth ever needed? That is, is the presence of key enough to signal "use this key instead of the one in the environment"? If so, would it make sense to make key an Option<String> and ditch environmental_auth altogether?

crates/trigger/src/runtime_config/key_value.rs

radu-matei · 2024-06-24T08:19:50Z

crates/key-value-azure/Cargo.toml

@@ -6,7 +6,8 @@ edition = { workspace = true }

 [dependencies]
 anyhow = "1"
-azure_data_cosmos = "0.11.0"
+azure_data_cosmos = { git = "https://github.com/azure/azure-sdk-for-rust.git", rev = "8c4caa251c3903d5eae848b41bb1d02a4d65231c" }
+azure_identity = { git = "https://github.com/azure/azure-sdk-for-rust.git", rev = "8c4caa251c3903d5eae848b41bb1d02a4d65231c" }


These are not yet part of a release, right?

They are not. Also, I'm not clear on when they would be.

radu-matei · 2024-06-24T08:26:26Z

crates/key-value-azure/src/lib.rs

+    /// DefaultCredentialChain to derive the TokenCredential based on what environment variables
+    /// have been set.
+    /// See also: https://github.com/Azure/azure-sdk-for-rust/blob/main/sdk/identity/README.md
+    Environmental,


It would be very helpful to explicitly state the expected environment variables that should be set for this authentication mechanism to work.

Yes, of course!

@radu-matei please take a look at the documentation when you have a moment.

crates/trigger/src/runtime_config/key_value.rs

radu-matei · 2024-06-24T08:31:26Z

This is looking great, @devigned! Thanks for the contribution!

itowlson · 2024-07-19T01:14:20Z

@devigned What's the status of this? I believe we're keen for it if you still have bandwidth - is it just the review nits and the lints?

devigned · 2024-07-19T15:20:01Z

Sorry, I got pulled away. I'll address the feedback and get it updated today. Thank you for your patience.

devigned · 2024-07-19T20:06:08Z

@itowlson I believe this is ready for review. I'm going to start work on the Key Vault integration to do the same thing.

itowlson

LGTM - thanks David! I really appreciated the detailed documentation on the environment variables. There were a couple of format/clippy changes that seem unrelated to the functional changes, possibly due to different Rust versions? That's not a blocker though.

itowlson · 2024-07-21T20:42:20Z

crates/oci/src/client.rs

-            annotations
-                .get(oci_distribution::annotations::ORG_OPENCONTAINERS_IMAGE_DESCRIPTION)
-                .is_none(),
+            !annotations


This and the next diff seem unrelated to all things Azure. Maybe your clippy is more recent than the one in CI? (I also noticed some format changes on the OTel attributes which suggested a toolchain having new opinions about existing code.)

Happy to revert the attribute changes. Let me know. Pretty sure it was just RustRover doing its auto format thing.

As for the annotations, IIRC that was a clippy error with the Rust version I am using.

You and your fancy "up-to-date" Rust versions. Yeah I'm not too worried. Thanks - I'll merge once Radu has had a chance to look.

itowlson · 2024-07-21T20:48:07Z

@devigned You'll need to run make update-cargo-locks to get CI to pass. (This happens because new/changed dependencies in the trigger crate need to feed through into the lockfile for the custom trigger example.)

itowlson · 2024-07-22T00:10:19Z

Integration test failure seems unrelated (it's hitting one of my PRs too).

Signed-off-by: David Justice <[email protected]>

devigned mentioned this pull request Jun 16, 2024

[help] - Using the Default Azure Credential to authenticate to CosmosDB Azure/azure-sdk-for-rust#1593

Closed

itowlson added the do-not-merge label Jun 16, 2024

devigned force-pushed the az-cosmos-wid branch from d46b28f to deb9f62 Compare June 17, 2024 16:01

devigned force-pushed the az-cosmos-wid branch from deb9f62 to a5a6197 Compare June 18, 2024 12:47

devigned marked this pull request as ready for review June 18, 2024 12:48

devigned force-pushed the az-cosmos-wid branch from a5a6197 to 94e8958 Compare June 18, 2024 12:52

devigned commented Jun 18, 2024

View reviewed changes

itowlson removed the do-not-merge label Jun 18, 2024

itowlson requested a review from ThorstenHans June 20, 2024 22:30

itowlson reviewed Jun 20, 2024

View reviewed changes

radu-matei reviewed Jun 24, 2024

View reviewed changes

devigned force-pushed the az-cosmos-wid branch 2 times, most recently from 062d792 to 4b7b969 Compare July 19, 2024 19:49

devigned force-pushed the az-cosmos-wid branch from 4b7b969 to 33e2ffe Compare July 19, 2024 20:35

itowlson approved these changes Jul 21, 2024

View reviewed changes

devigned force-pushed the az-cosmos-wid branch from 33e2ffe to 144142c Compare July 21, 2024 21:42

add support for workload identity in azure cosmosdb kv

6656e68

Signed-off-by: David Justice <[email protected]>

devigned force-pushed the az-cosmos-wid branch from 144142c to 6656e68 Compare July 22, 2024 16:47

itowlson merged commit 08eac60 into fermyon:main Jul 23, 2024
16 of 17 checks passed

This was referenced Jul 23, 2024

Enable environmental authentication for Azure Key Vault var provider #2678

Merged

Skip 004 - Cloud Identity and Access Management spinkube/skips#9

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for workload identity in the Azure CosmosDB Key/Value impl #2566

Add support for workload identity in the Azure CosmosDB Key/Value impl #2566

devigned commented Jun 15, 2024 •

edited

Loading

devigned commented Jun 16, 2024

devigned commented Jun 16, 2024

devigned commented Jun 17, 2024

devigned commented Jun 18, 2024

devigned Jun 18, 2024

itowlson Jun 20, 2024

itowlson commented Jun 20, 2024

itowlson Jun 20, 2024

radu-matei Jun 24, 2024

devigned Jun 24, 2024

radu-matei Jun 24, 2024

devigned Jun 24, 2024

devigned Jul 19, 2024

radu-matei commented Jun 24, 2024

itowlson commented Jul 19, 2024

devigned commented Jul 19, 2024

devigned commented Jul 19, 2024

itowlson left a comment

itowlson Jul 21, 2024

devigned Jul 21, 2024

itowlson Jul 21, 2024

itowlson commented Jul 21, 2024

itowlson commented Jul 22, 2024

		@@ -122,19 +124,33 @@ impl RedisKeyValueStoreOpts {

		#[derive(Clone, Debug, Deserialize)]
		pub struct AzureCosmosConfig {

Add support for workload identity in the Azure CosmosDB Key/Value impl #2566

Add support for workload identity in the Azure CosmosDB Key/Value impl #2566

Conversation

devigned commented Jun 15, 2024 • edited Loading

devigned commented Jun 16, 2024

devigned commented Jun 16, 2024

devigned commented Jun 17, 2024

devigned commented Jun 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itowlson commented Jun 20, 2024

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

radu-matei commented Jun 24, 2024

itowlson commented Jul 19, 2024

devigned commented Jul 19, 2024

devigned commented Jul 19, 2024

itowlson 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

itowlson commented Jul 21, 2024

itowlson commented Jul 22, 2024

devigned commented Jun 15, 2024 •

edited

Loading