-
Notifications
You must be signed in to change notification settings - Fork 760
Spike: Set the client id for functions configuration #8868
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
Conversation
| // Always inject the connection string associated with the top-level resource | ||
| // for the Azure Functions host. | ||
| target[$"{connectionName}__accountEndpoint"] = ConnectionStringExpression; | ||
| target[$"{connectionName}__clientId"] = default!; |
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.
This is a little hacky. Unclear if this signal is needed. We might be able to do something else like always set it in WithReference if it is harmless (we already always set AZURE_CLIENT_ID today anyways?)
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.
It should generally be harmless to just set it in WithReference() today, but it's probably worth building some defensiveness in here. I actually realize we forgot to prescribe something for this scenario: I believe we still need {connectionName}__credential: managedidentity, as that's the signal used to engage this when AZURE_CLIENT_ID is missing. I don't know if the KEDA logic cares there, but that is how Microsoft.Extensions.Azure expects things.
I'm imagining a situation in which someone themselves specifies {connectionName}__credential, set to something that gets added in the future. AZURE_CLIENT_ID likely would be mapped the same, but I don't know that it's guaranteed (and I'm thinking of cross-tenant with MI+FIC, specifically). I think the safest approach would be to check the credential type. If it's not unspecified or "managedidentity", we wouldn't want to assume {connectionName}__clientId has these semantics, or that it should be set at all. If the user goes custom here, they become responsible for the full suite of config for that custom approach.
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.
I don't know if the KEDA logic cares there, but that is how Microsoft.Extensions.Azure expects things.
Are you pretty sure the KEDA doesn't support AZURE_CLIENT_ID?
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.
That was the case when we opened the issue, at least. I've not been made aware of any updates on that front if they did happen.
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.
We should fix keda and make it respect AZURE_CLIENT_ID
Description
This pull request introduces changes to enhance Azure Functions integration by adding support for the
clientIdenvironment variable. The updates ensure that theclientIdis correctly injected into the environment variables for Azure Functions projects when applicable.Fixes #8116
Checklist
<remarks />and<code />elements on your triple slash comments?doc-ideatemplatebreaking-changetemplatediagnostictemplate