[chart/redis-ha] issue-161: Resolve template issues with values.sentinel.auth

DandyDeveloper / charts

Various helm charts migrated from [helm/stable] due to deprecation

https://dandydeveloper.github.io/charts

Apache License 2.0

156 stars 143 forks source link

[chart/redis-ha] issue-161: Resolve template issues with values.sentinel.auth #194

Closed sonrai-doyle closed 2 years ago

sonrai-doyle commented 2 years ago

What this PR does / why we need it:

Resolves logic issues in the templating related to Values.sentinel.auth

Which issue this PR fixes

fixes #161

Special notes for your reviewer:

I have not tested these changes myself

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

[x] DCO signed
[x] Chart Version bumped
[x] Title of the PR starts with chart name (e.g. [stable/mychartname])

DandyDeveloper commented 2 years ago

@sonrai-doyle Thank you for the contribution. Getting this merged in now.

LongLiveCHIEF commented 2 years ago

@DandyDeveloper should this have made the default image tag (and appVersion) to 6.2.6-alpine? I notice it is still 6.2.5-alpine, and #161 doesn't occur when using redis:6.2.5 images.

DandyDeveloper commented 2 years ago

@LongLiveCHIEF Worth noting this bug only occurs when auth is disabled and sentinel auth is enabled.

I don't think the Redis image had any particular merit to this having impact. Unless I'm misunderstanding?

sonrai-doyle commented 2 years ago

Sorry, I may have misunderstood #161. As @DandyDeveloper stated, this does fix a particular use case, although it may not be the one outlined in the issue.

LongLiveCHIEF commented 2 years ago

Initially, i had the same issue as the language @bootcc used in the original report, where 6.2.6 with auth enabled (but not sentinel auth) failed, but using tag 6.2.5 did not fail. The code he called out though doesn't appear to me to affect the language of the report, rather it affects a bug with sentinel auth configuration.

So in the #161 original report it was a bug being reported when redis auth was set but sentinel auth was not.

If I'm reading this correctly, this PR fixes a bug where sentinel auth is enabled but redis is not.

sonrai-doyle commented 2 years ago

I reviewed issue #161 and I think this PR would resolve both use-cases.

(Unconfirmed by me) It appears that 6.2.6 would fail the liveness check for sentinel because the logic in the template was broken and it was attempting to use a sentinel password when it was disabled. I assume 6.2.5 sentinel just ignored the faulty password because it wasn't expecting one and that 6.2.6 failed because it received a password and wasn't expecting one.

I fixed the template logic and sentinel should now only receives a password if it's enabled. Someone should re-try 6.2.6 with only a redis password and I'd bet it would now work.

DandyDeveloper commented 2 years ago

I did not explicitly look at this use case because the code seemed solid but for the sake of thoroughness I'll try and test tomorrow