tomkerkhove / promitor

Bringing Azure Monitor metrics where you need them.
https://promitor.io
MIT License
249 stars 91 forks source link

Provide support for specifying application ID through environment variable for managed identities #1830

Open PowerSurj opened 2 years ago

PowerSurj commented 2 years ago

Proposal

Make environment variables resolvable in runtime.yaml Currently only literal values are supported. Environment variables would facilitate secrets passing through CSI.

I tested this in promitor-agent-resource-discovery 0.6.0 image deployed through helm chart I define environment variable through CSI and reference it in runtime.yaml through values.yaml

azureAuthentication:
  mode: UserAssignedManagedIdentity
  identity:
    id: ${PR_AZURE_MANAGED_IDENTITY_CLIENT_ID}
    binding: promitor-pod-identity

In this example i reference clientID This results in exception:

System.AggregateException: One or more errors occurred. (ManagedIdentityCredential authentication failed: 'no azure identity found for request clientID ${PR##### REDACTED #####_ID} ' is an invalid JSON literal. Expected the literal 'null'. LineNumber: 0 | BytePositionInLine: 1.

Component

Resource Discovery

Contact Details

No response

tomkerkhove commented 2 years ago

While I do get the use-case, I'm wondering if this approach is the best.

Would it be OK for you if you could use an environment variable instead of passing the name of the environment variable inside the YAML?

PowerSurj commented 2 years ago

It would be great if we could keep the env var. I came across https://github.com/tomkerkhove/promitor/issues/1582 where the phase out of PROMITOR_AUTH_APPID is announced. Hence, i brought this request up.

Such approach would be in line with our deployment framework where we reference all authentication details and don't keep it static in code (we don't use credentials store integration at CI/CD platform level but hand it over to underlying platform, in this case AKV + CSI)

tomkerkhove commented 2 years ago

I'm happy to revert the deprecation for being more flexible. I'd rather have an environment variable than tokenization placeholders in the YAML that need to be resolved at runtime.

I hope this approach is OK to you?

PowerSurj commented 2 years ago

That'd be great! Thanks Tom!