Add Password To Redis #4642

Alirexaa · 2024-06-25T13:12:15Z

Microsoft Reviewers: Open in CodeFlow

src/Aspire.Hosting.Redis/PublicAPI.Shipped.txt

eerhardt · 2024-06-25T15:39:37Z

src/Aspire.Hosting.Redis/RedisBuilderExtensions.cs

+                              .WithImage(RedisContainerImageTags.Image, RedisContainerImageTags.Tag)
+                              .WithImageRegistry(RedisContainerImageTags.Registry);
+            }
+            throw new InvalidOperationException();


Can we add a message to this exception to explain what went wrong?

It is a shame that we have to do these parameters in this order (its different to AddPostgres).

src/Aspire.Hosting.Redis/RedisBuilderExtensions.cs

mitchdenny · 2024-06-26T11:36:53Z

I'm comfortable with this change pending @eerhardt 's comments about the API txt files and some incremental improvements to the XML docs since we are in the neighbhorhood.

I'm a little disappointed that about the parameter ordering but I can't think of a way that isn't a binary/source breaking change that isn't also disruptive.

One more iteration and we should be able to stamp an approval on this. Thanks once again for your efforts @Alirexaa.

…pire into RedisPasswordOptions

src/Aspire.Hosting.Redis/RedisBuilderExtensions.cs

tests/Aspire.Hosting.Tests/Redis/AddRedisTests.cs

src/Aspire.Hosting.Redis/RedisBuilderExtensions.cs

tests/Aspire.Hosting.Tests/Redis/AddRedisTests.cs

radical · 2024-06-28T01:37:09Z

Some of my comments might be non-sensical, or could be addressed in a follow-up PR. I'll let others decide that, since I'm very new to this code:)

playground/Redis/Redis.AppHost/Program.cs

DamianEdwards · 2024-07-01T23:46:57Z

src/Aspire.Hosting.Redis/RedisResource.cs

        ReferenceExpression.Create(
-            $"{PrimaryEndpoint.Property(EndpointProperty.Host)}:{PrimaryEndpoint.Property(EndpointProperty.Port)}");
+            $"{PrimaryEndpoint.Property(EndpointProperty.Host)}:{PrimaryEndpoint.Property(EndpointProperty.Port)},password={PasswordParameter}")


~~Does the password need to be encoded here, e.g. URI encoded?~~
It likely does but we shouldn't fix it here in isolation as this is a larger issue #3117

It isn't URL encoded. Looks like SE.Redis doesn't support = or , in their passwords:
https://github.com/StackExchange/StackExchange.Redis/blob/1c6b59e6c7751f03aec74679bb2f0d9123b74e3d/src/StackExchange.Redis/ConfigurationOptions.cs#L946-L959

FYI - @mgravell @NickCraver

Co-authored-by: Ankit Jain <[email protected]>

src/Aspire.Hosting.Redis/RedisBuilderExtensions.cs

eerhardt · 2024-07-02T02:30:40Z

src/Aspire.Hosting.Redis/RedisResource.cs

        ReferenceExpression.Create(
-            $"{PrimaryEndpoint.Property(EndpointProperty.Host)}:{PrimaryEndpoint.Property(EndpointProperty.Port)}");
+            $"{PrimaryEndpoint.Property(EndpointProperty.Host)}:{PrimaryEndpoint.Property(EndpointProperty.Port)},password={PasswordParameter}")


It isn't URL encoded. Looks like SE.Redis doesn't support = or , in their passwords:
https://github.com/StackExchange/StackExchange.Redis/blob/1c6b59e6c7751f03aec74679bb2f0d9123b74e3d/src/StackExchange.Redis/ConfigurationOptions.cs#L946-L959

FYI - @mgravell @NickCraver

mgravell · 2024-07-08T17:27:54Z

@eerhardt We've discussed this for the client a few times but it has never hit the top of the pile. If it is a blocker, we can prioritize. I would suggest looking at ado-net quoted values as the template, but obvs we'd need to think a little about how to do it without impacting existing use of any new token

eerhardt · 2024-07-08T18:10:21Z

@eerhardt We've discussed this for the client a few times but it has never hit the top of the pile. If it is a blocker, we can prioritize. I would suggest looking at ado-net quoted values as the template, but obvs we'd need to think a little about how to do it without impacting existing use of any new token

I don't think it is blocking this work. I just wanted to point out that we can't URL encode the connection string here.

drewnoakes · 2024-08-22T06:32:59Z

@drewnoakes @adamint - is it possible to mark command line arguments as "secret"? So they get obfuscated in the dashboard.

#5065 tracks marking resource properties as secrets, however that has to be specified by the resource service. We could hard-code some known properties as sensitive. If we did that, which would you think should be included? The current list is https://github.com/dotnet/aspire/blob/0f2931a94873d09e5a76f72485000e5388bf08dd/src/Shared/Model/KnownProperties.cs

This is in review in #5380

mitchdenny · 2024-08-22T13:40:12Z

Yeah, I was wondering if it would make sense for us to switch from library/redis to redis/redis-stack-server instead. And then Add WithRedisInsight (dotnet/aspire#5227) would switch the image from redis/redis-stack-server to redis/redis-stack and add the appropriate endpoints.

I think this is an option we should seriously consider. I think we would just need to look through where there are any licensing implications for folks deploying that image into production. There might be extra licensing requirements that we wouldn't want to silently swap people into.

This needs to work around RedisInsight/RedisInsight#3452, which requires the agreements encryption property to be initialized.

eerhardt · 2025-01-29T18:28:39Z

I've revived this PR with the latest code in main. The intent is to get this change into 9.1. The initial concerns that blocked this change should no longer be a problem. The command line args in the dashboard are hidden/masked by default now.

One complication is that Redis Insight needs to be initialized before we add a database with a password. I'll add a comment to the code to start a conversation about it there.

eerhardt · 2025-01-29T18:37:05Z

src/Aspire.Hosting.Redis/RedisBuilderExtensions.cs

+            agreements = new
+            {
+                // all 4 are required to be set
+                eula = false,


This is unfortunate, but it is the only way I could get it to work. See RedisInsight/RedisInsight#3452 for more information on why this is needed.

Note that this isn't accepting the EULA (it is sending false). But the consequences of initializing this is the pop up no longer happens on first run of Redis Insight, which is a nice user experience.

Thoughts? @sebastienros @DamianEdwards @Alirexaa

What are the options?

Seems they're suggesting we wait until they implement support for adding databases via environment variables instead.

I've opened [Bug]: Posting eula: false to /api/settings suppresses the first run dialog (RedisInsight/RedisInsight#4340) for the first run dialog issue.

Let's move forward with this PR for now. We can readjust later.

- Update manifests

mgravell · 2025-01-29T23:00:15Z

Question: is it possible to specify the user for Redis? Or is that unnecessary in this scenario, with Aspire just using the default user? Similarly, there is increasing support for azure etc identity tokens for auth.

davidfowl · 2025-01-30T07:33:19Z

src/Aspire.Hosting.Redis/RedisResource.cs

+
+    /// <summary>
+    /// Gets the parameter that contains the Redis server username.
+    /// </summary>
    private ReferenceExpression ConnectionString =>


nit: Prefer this pattern

aspire/src/Aspire.Hosting.MongoDB/MongoDBServerResource.cs

Line 56 in 9e2a2df

internal ReferenceExpression BuildConnectionString(string? databaseName = null)

davidfowl

Do we mark this as a breaking change?

eerhardt · 2025-01-31T20:40:21Z

is it possible to specify the user for Redis? Or is that unnecessary in this scenario, with Aspire just using the default user?

Right now, no. We are just adding password protection for now. If necessary, we can also add support for user in the future.

Similarly, there is increasing support for azure etc identity tokens for auth.

We support Azure Redis and Entra ID already in 9.0. See https://learn.microsoft.com/dotnet/aspire/caching/stackexchange-redis-integration#add-azure-cache-for-redis-client

eerhardt · 2025-01-31T20:42:39Z

Do we mark this as a breaking change?

I don't think this is a breaking change. We can log it in the changelog, but I'm not sure exactly what it would break. The only thing I can think of is if someone is explicitly cutting the password out of the connection string getting flown to the app (or passing the connection string a custom way - not using our Hosting ConnectionString).

eerhardt · 2025-01-31T22:22:13Z

Thank you, @Alirexaa for your continued contributions!

Repond to feedback from dotnet#4642. Refactor how the connection string reference expression is built.

Repond to feedback from #4642. Refactor how the connection string reference expression is built.

This reverts commit 7d221ef.

* Revert "Follow up from Redis Password change (#7356)" This reverts commit ac76241. * Revert "Add Password To Redis (#4642)" This reverts commit 7d221ef. * Remove unnecessary using * Revert unnecessary changes

add password to redis

0a10f76

Alirexaa requested a review from mitchdenny as a code owner June 25, 2024 13:12

dotnet-issue-labeler bot added the area-integrations Issues pertaining to Aspire Integrations packages label Jun 25, 2024

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jun 25, 2024

Merge branch 'main' into RedisPasswordOptions

d03adfa

eerhardt reviewed Jun 25, 2024

View reviewed changes

src/Aspire.Hosting.Redis/PublicAPI.Shipped.txt Outdated Show resolved Hide resolved

eerhardt reviewed Jun 25, 2024

View reviewed changes

mitchdenny added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label Jun 26, 2024

mitchdenny reviewed Jun 26, 2024

View reviewed changes

src/Aspire.Hosting.Redis/RedisBuilderExtensions.cs Outdated Show resolved Hide resolved

mitchdenny self-assigned this Jun 26, 2024

mitchdenny added this to the 8.1 milestone Jun 26, 2024

Alirexaa added 2 commits June 26, 2024 23:20

add xml docs,fix public api text files

6952f0b

Merge branch 'RedisPasswordOptions' of https://github.com/Alirexaa/as…

b753630

…pire into RedisPasswordOptions

Alirexaa requested a review from mitchdenny June 27, 2024 20:21

radical reviewed Jun 28, 2024

View reviewed changes

eerhardt reviewed Jul 1, 2024

View reviewed changes

playground/Redis/Redis.AppHost/Program.cs Outdated Show resolved Hide resolved

DamianEdwards reviewed Jul 1, 2024

View reviewed changes

Alirexaa and others added 7 commits July 2, 2024 21:07

Update src/Aspire.Hosting.Redis/RedisBuilderExtensions.cs

98c791d

Co-authored-by: Ankit Jain <[email protected]>

Update src/Aspire.Hosting.Redis/RedisBuilderExtensions.cs

d8c1dcb

Co-authored-by: Ankit Jain <[email protected]>

Update src/Aspire.Hosting.Redis/RedisBuilderExtensions.cs

cb1f013

Co-authored-by: Ankit Jain <[email protected]>

Apply suggestions from code review

e167d26

Co-authored-by: Ankit Jain <[email protected]>

apply suggested changes

02378f0

add more tests

a7eec1f

add password to redis commander REDIS_HOSTS env variable

44eff5a

eerhardt reviewed Jul 8, 2024

View reviewed changes

Minor PR feedback

b39cf07

Alirexaa mentioned this pull request Sep 5, 2024

Add RedisStack Hosting #5479

Closed

16 tasks

davidfowl added the redis Issues related to redis integrations label Sep 14, 2024

eerhardt added 3 commits January 27, 2025 19:12

Merge remote-tracking branch 'upstream/main' into RedisPasswordOptions

b680c4a

Fix up merge for the latest code

ad6f111

Fix Redis Insights with a Redis connection with a password.

c3eba43

This needs to work around RedisInsight/RedisInsight#3452, which requires the agreements encryption property to be initialized.

eerhardt reviewed Jan 29, 2025

View reviewed changes

eerhardt added 3 commits January 29, 2025 13:49

- Fix tests

b80feb3

- Update manifests

Merge remote-tracking branch 'upstream/main' into RedisPasswordOptions

6d4e8d1

Fix new test.

2184d13

davidfowl reviewed Jan 30, 2025

View reviewed changes

davidfowl approved these changes Jan 30, 2025

View reviewed changes

Merge branch 'main' into RedisPasswordOptions

76c7609

eerhardt merged commit 7d221ef into dotnet:main Jan 31, 2025
9 checks passed

Alirexaa deleted the RedisPasswordOptions branch January 31, 2025 21:31

eerhardt added a commit to eerhardt/aspire that referenced this pull request Jan 31, 2025

Follow up from Redis Password change

7c4df1f

Repond to feedback from dotnet#4642. Refactor how the connection string reference expression is built.

eerhardt mentioned this pull request Jan 31, 2025

Follow up from Redis Password change #7356

Merged

eerhardt added a commit that referenced this pull request Feb 1, 2025

Follow up from Redis Password change (#7356)

ac76241

Repond to feedback from #4642. Refactor how the connection string reference expression is built.

eerhardt added a commit to eerhardt/aspire that referenced this pull request Feb 10, 2025

Revert "Add Password To Redis (dotnet#4642)"

2d2d13e

This reverts commit 7d221ef.

eerhardt mentioned this pull request Feb 10, 2025

Revert redis password change #7518

Merged

5 tasks

eerhardt mentioned this pull request Feb 13, 2025

Revisit Redis password options #3838

Open

Alirexaa mentioned this pull request Feb 13, 2025

Reapply Redis password #7599

Open

18 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Password To Redis #4642

Add Password To Redis #4642

Alirexaa commented Jun 25, 2024 •

edited by dotnet-policy-service bot

Loading

eerhardt Jun 25, 2024

mitchdenny Jun 26, 2024

mitchdenny commented Jun 26, 2024

radical commented Jun 28, 2024

DamianEdwards Jul 1, 2024 •

edited

Loading

eerhardt Jul 2, 2024

eerhardt Jul 2, 2024

mgravell commented Jul 8, 2024

eerhardt commented Jul 8, 2024

drewnoakes commented Aug 22, 2024

mitchdenny commented Aug 22, 2024

eerhardt commented Jan 29, 2025

eerhardt Jan 29, 2025

davidfowl Jan 29, 2025

DamianEdwards Jan 31, 2025

eerhardt Jan 31, 2025

mgravell commented Jan 29, 2025 •

edited

Loading

davidfowl Jan 30, 2025 •

edited

Loading

davidfowl left a comment

eerhardt commented Jan 31, 2025

eerhardt commented Jan 31, 2025

eerhardt commented Jan 31, 2025

Add Password To Redis #4642

Add Password To Redis #4642

Conversation

Alirexaa commented Jun 25, 2024 • edited by dotnet-policy-service bot Loading

Microsoft Reviewers: Open in CodeFlow

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitchdenny commented Jun 26, 2024

radical commented Jun 28, 2024

DamianEdwards Jul 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgravell commented Jul 8, 2024

eerhardt commented Jul 8, 2024

drewnoakes commented Aug 22, 2024

mitchdenny commented Aug 22, 2024

eerhardt commented Jan 29, 2025

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

mgravell commented Jan 29, 2025 • edited Loading

davidfowl Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

davidfowl left a comment

Choose a reason for hiding this comment

eerhardt commented Jan 31, 2025

eerhardt commented Jan 31, 2025

eerhardt commented Jan 31, 2025

Alirexaa commented Jun 25, 2024 •

edited by dotnet-policy-service bot

Loading

DamianEdwards Jul 1, 2024 •

edited

Loading

mgravell commented Jan 29, 2025 •

edited

Loading

davidfowl Jan 30, 2025 •

edited

Loading