Reapply Redis password #7599
Reapply Redis password #7599
Conversation
dotnet-policy-service
bot
added
the
community-contribution
danmoseley
added
area-integrations
SE.Redis doesn't support commas in the password when parsing the connection string.
There was a problem hiding this comment.
Thanks again for this contribution @Alirexaa! Nice work here.
I merged main and refactored the code a little bit to simplify it a bit.
But when deploying the app to ACA I found an issue that while Aspire doesn't use commas , in its generated passwords, azd will. So I updated the code to not allow special characters in the password.
This change LGTM - we should be able to finally fix this long standing security issue. Thanks again!
|
|
||
| if (builder.Resource.PasswordParameter is { } password) | ||
| { | ||
| args.Add("--requirepass"); |
There was a problem hiding this comment.
When I run I see the command line args in the Dashboard:
I wonder if we want this experience by default. Do we really want/need to show these? Is there a way to disable them for certain resources?
Part of my reasoning is that we don't display all the environment variables here, why are command line args of the container important enough to show front and center here?
| @@ -608,6 +608,7 @@ BicepValue<string> GetHostValue(string? prefix = null, string? suffix = null) | |||
| EndpointProperty.Url => GetHostValue($"{scheme}://", suffix: isHttpIngress ? null : $":{port}"), | |||
| EndpointProperty.Host or EndpointProperty.IPV4Host => GetHostValue(), | |||
| EndpointProperty.Port => port.ToString(CultureInfo.InvariantCulture), | |||
| EndpointProperty.HostAndPort=> GetHostValue(suffix: isHttpIngress ? null : $":{port}"), | |||
There was a problem hiding this comment.
FYI - @davidfowl - I found a bug that we missed here and fixed it so all the generate-manifests work again.
There was a problem hiding this comment.
I didn't know if I should switch off isHttpIngress or not. If you explicitly ask for HostAndPort and this is the httpIngress, should we still tack on :80?
There was a problem hiding this comment.
Rethinking this, I think it should always put the port. The reasoning the URL can drop the port is because it contains the scheme at the begining. http:// indicates what the port is. If you say HostAndPort we should always give you the host and the port.
There was a problem hiding this comment.
I agree, it should include host an port
src/Aspire.Hosting.Azure.AppContainers/AzureContainerAppsInfrastructure.cs
Outdated
Show resolved
Hide resolved
Description
Add Password To Redis (dotnet/aspire#464) has been reverted because of [WebToolsE2E][Aspire] Using ‘azd up’ to deploy aspire starter with redis project fails with error: generating bicep from manifest: argument 1 cannot contain connection strings, secured parameters, or secret outputs. Use environment variables instead. (dotnet/aspire#7429).
This PR addresses the mentioned issue by changing the container entrypoint and providing the password from an environment variable.
For more information see #3838 (comment)
Fixes #3838
@eerhardt @davidfowl
Checklist
<remarks />and<code />elements on your triple slash comments?breaking-changetemplate):doc-ideatemplate):