fix(helm): Include option to use Redis with SSL #26663

shakeelansari63 · 2024-01-18T14:42:47Z

SUMMARY

Updated Helm Chart to allow connection to Redis Server which enforce SSL connection. For example, Azure managed Redis Cache enforce connection through ssl only.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Has associated issue: Installing latest superset via helm, cant connect to azure redis service via ssl #25375
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

github-actions

Congrats on making your first PR and thank you for contributing to Superset! 🎉 ❤️

We hope to see you in our Slack community too! Not signed up? Use our Slack App to self-register.

helm/superset/templates/_helpers.tpl

shakeelansari63 · 2024-01-18T17:38:12Z

I have user CACHE_REDIS_URL instead of CACHE_REDIS_HOST

Here are some sample examples of rendered templates..

1. No Password and No SSL

values.yaml

Rendered Template

2. With Password and No SSL

values.yaml

Rendered Template

3. No Password and With SSL (ssl_cert_reqs = CERT_NONE)

values.yaml

Rendered Template

4. With Password and With SSL (ssl_cert_reqs = CERT_NONE)

values.yaml

Rendered Template

5. With Password and With SSL (ssl_cert_reqs = CERT_OPTIONAL)

values.yaml

Rendered Template

helm/superset/templates/_helpers.tpl

dpgaspar · 2024-01-22T13:39:27Z

can you please rebase if bump the chart version again to fix the conflict

shakeelansari63 · 2024-01-22T13:40:33Z

can you please rebase if bump the chart version again to fix the conflict

Sure, will do it shortly

shakeelansari63 · 2024-01-22T13:52:50Z

@dpgaspar , Rebased and bumped the chart version to 0.12.1

helm/superset/templates/_helpers.tpl

shakeelansari63 · 2024-01-26T10:28:15Z

So I have added 2 new Parameters in Values which will translate to 2 new Environment variables.
Also bumped up the chart version.

With Default Config

values.yaml

Generated secret-env.yaml

Generated secret-superset-config.yaml

With modified config

values.yaml

Generated secret-env.yaml

Generated secret-superset-config.yaml

helm/superset/templates/secret-env.yaml

shakeelansari63 · 2024-01-26T14:20:58Z

Default Config without Redis SSL and Redis Password

values.yaml

Rendered secret-env.yaml

Rendered superset-config.py under secret-superset-config.yaml

Updated Config with Redis SSL and Redis Password

values.yaml

Rendered secret-env.yaml

Rendered superset-config.py under secret-superset-config.yaml

shakeelansari63 · 2024-02-06T15:15:39Z

Hey guys, can you please review the code and tell me if you feel anything else should be changed...

helm/superset/templates/_helpers.tpl

craig-rueda

Looking good. I just left one more comment and I think @dpgaspar has one as well

helm/superset/templates/_helpers.tpl

shakeelansari63 · 2024-02-08T10:13:53Z

Hey guys, can you please suggest if any further change is needed?

rusackas · 2024-02-08T15:45:26Z

Re-running CI, while @craig-rueda and @dpgaspar review the requested changes. 🤞

dpgaspar

Almost there!

you need to update the README.md

 | supersetNode.connections.db_pass | string | `"superset"` |  |
 | supersetNode.connections.db_port | string | `"5432"` |  |
 | supersetNode.connections.db_user | string | `"superset"` |  |
+| supersetNode.connections.redis_cache_db | string | `"1"` |  |
+| supersetNode.connections.redis_celery_db | string | `"0"` |  |
 | supersetNode.connections.redis_host | string | `"{{ .Release.Name }}-redis-headless"` | Change in case of bringing your own redis and then also set redis.enabled:false |
 | supersetNode.connections.redis_port | string | `"6379"` |  |
+| supersetNode.connections.redis_ssl.enabled | bool | `false` |  |
+| supersetNode.connections.redis_ssl.ssl_cert_reqs | string | `"CERT_NONE"` |  |
+| supersetNode.connections.redis_user | string | `""` |  |
 | supersetNode.containerSecurityContext | object | `{}` |  |
 | supersetNode.deploymentAnnotations | object | `{}` | Annotations to be added to supersetNode deployment |
 | supersetNode.deploymentLabels | object | `{}` | Labels to be added to supersetNode deployment |

shakeelansari63 · 2024-02-08T16:23:48Z

Almost there!

you need to update the README.md

 | supersetNode.connections.db_pass | string | `"superset"` |  |
 | supersetNode.connections.db_port | string | `"5432"` |  |
 | supersetNode.connections.db_user | string | `"superset"` |  |
+| supersetNode.connections.redis_cache_db | string | `"1"` |  |
+| supersetNode.connections.redis_celery_db | string | `"0"` |  |
 | supersetNode.connections.redis_host | string | `"{{ .Release.Name }}-redis-headless"` | Change in case of bringing your own redis and then also set redis.enabled:false |
 | supersetNode.connections.redis_port | string | `"6379"` |  |
+| supersetNode.connections.redis_ssl.enabled | bool | `false` |  |
+| supersetNode.connections.redis_ssl.ssl_cert_reqs | string | `"CERT_NONE"` |  |
+| supersetNode.connections.redis_user | string | `""` |  |
 | supersetNode.containerSecurityContext | object | `{}` |  |
 | supersetNode.deploymentAnnotations | object | `{}` | Annotations to be added to supersetNode deployment |
 | supersetNode.deploymentLabels | object | `{}` | Labels to be added to supersetNode deployment |

Yes, Just saw the failed CI task and updated this.

rusackas · 2024-02-08T16:26:45Z

Thanks for the quick turnaround. Go, CI, go!!!

shakeelansari63 · 2024-02-08T16:33:22Z

@rusackas , please trigger ci again , I had missed to update helm version in readme

shakeelansari63 · 2024-02-08T16:53:31Z

Yay .. All CI Checks finally completed 🥳

craig-rueda

LGTM!

fengsi · 2024-02-12T07:32:17Z

helm/superset/templates/secret-env.yaml

    {{- if .Values.supersetNode.connections.redis_password }}
    REDIS_PASSWORD: {{ .Values.supersetNode.connections.redis_password | quote }}
    {{- end }}
    REDIS_PORT: {{ .Values.supersetNode.connections.redis_port | quote }}
+    REDIS_PROTO: {{ if .Values.supersetNode.connections.redis_ssl.enabled }}"rediss"{{ else }}"redis"{{ end }}


This has broken those w/o generating the default secret, which is a very common use case to manage secrets separately (rather than relying on the Helm chart).

This kind of backward compatibility should have been implemented somewhere else, and it's not a good practice not to even document/mention this feature/behavior change at all.

The solution was to add the new REDIS_PROTO in the managed secret. However, it was a hassle to root cause it, due to lacking of documentation.

Update Superset config for redis ssl connection

12455be

shakeelansari63 requested review from craig-rueda, dpgaspar and villebro as code owners January 18, 2024 14:42

pull-request-size bot added the size/M label Jan 18, 2024

github-actions bot reviewed Jan 18, 2024

View reviewed changes

Use CERT_NONE instead of none in ssl_cert_reqs

0519149

dpgaspar reviewed Jan 18, 2024

View reviewed changes

helm/superset/templates/_helpers.tpl Outdated Show resolved Hide resolved

Use Cache_Redis_Url instead of Cache_Redis_Host

dbe4043

Bumped up Chart Version

f385766

shakeelansari63 requested a review from dpgaspar January 19, 2024 14:41

dpgaspar reviewed Jan 19, 2024

View reviewed changes

helm/superset/templates/_helpers.tpl Outdated Show resolved Hide resolved

Cleaned Up Trailing Spaces

09e1453

shakeelansari63 requested a review from dpgaspar January 19, 2024 16:27

Merge branch 'master' into master

f5d18fd

craig-rueda reviewed Jan 22, 2024

View reviewed changes

helm/superset/templates/_helpers.tpl Outdated Show resolved Hide resolved

shakeelansari63 added 2 commits January 23, 2024 17:07

Create envvars for Redis_proto, Redis_db and celery_db

a36984a

Bump up chart version

7d09d93

shakeelansari63 requested a review from craig-rueda January 26, 2024 10:39

shakeelansari63 changed the title ~~Fix(Helm): Include option to use Redis with SSL~~ fix(Helm): Include option to use Redis with SSL Jan 26, 2024

shakeelansari63 changed the title ~~fix(Helm): Include option to use Redis with SSL~~ fix(helm): Include option to use Redis with SSL Jan 26, 2024

Merge branch 'master' into master

4ba119a

dpgaspar reviewed Jan 26, 2024

View reviewed changes

helm/superset/templates/secret-env.yaml Outdated Show resolved Hide resolved

Build Redis URL Params based REDIS_SSL_CERT_REQS envvar

4ffe609

Merge branch 'master' into master

d984761

dpgaspar reviewed Feb 6, 2024

View reviewed changes

helm/superset/templates/_helpers.tpl Outdated Show resolved Hide resolved

craig-rueda reviewed Feb 6, 2024

View reviewed changes

helm/superset/templates/_helpers.tpl Outdated Show resolved Hide resolved

shakeelansari63 added 2 commits February 6, 2024 21:17

Fix old use_redis_ssl

250948e

Add Redis User

7d79c70

dpgaspar reviewed Feb 8, 2024

View reviewed changes

Add Helm Documentation

1000b62

Fix helm version in readme

4d3249f

pull-request-size bot added size/XL and removed size/M labels Feb 8, 2024

Fix helm version in helm readme

29d7a7a

fix auto formatting caused by editor

cb3567b

pull-request-size bot added size/M and removed size/XL labels Feb 8, 2024

craig-rueda approved these changes Feb 9, 2024

View reviewed changes

craig-rueda merged commit f59498f into apache:master Feb 9, 2024
28 of 29 checks passed

fengsi reviewed Feb 12, 2024

View reviewed changes

villebro mentioned this pull request Feb 13, 2024

fix(helm): typo on ssl_cert_reqs variable #27094

Merged

9 tasks

sfirke pushed a commit to sfirke/superset that referenced this pull request Mar 22, 2024

fix(helm): Include option to use Redis with SSL (apache#26663)

a89d966

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 4.0.0 labels Apr 17, 2024

vinothkumar66 pushed a commit to vinothkumar66/superset that referenced this pull request Nov 11, 2024

fix(helm): Include option to use Redis with SSL (apache#26663)

c905980

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(helm): Include option to use Redis with SSL #26663

fix(helm): Include option to use Redis with SSL #26663

shakeelansari63 commented Jan 18, 2024 •

edited

Loading

github-actions bot left a comment

shakeelansari63 commented Jan 18, 2024

dpgaspar commented Jan 22, 2024

shakeelansari63 commented Jan 22, 2024

shakeelansari63 commented Jan 22, 2024 •

edited

Loading

shakeelansari63 commented Jan 26, 2024

shakeelansari63 commented Jan 26, 2024

shakeelansari63 commented Feb 6, 2024

craig-rueda left a comment

shakeelansari63 commented Feb 8, 2024

rusackas commented Feb 8, 2024

dpgaspar left a comment

shakeelansari63 commented Feb 8, 2024

rusackas commented Feb 8, 2024

shakeelansari63 commented Feb 8, 2024

shakeelansari63 commented Feb 8, 2024

craig-rueda left a comment

fengsi Feb 12, 2024

fengsi Feb 12, 2024

fix(helm): Include option to use Redis with SSL #26663

fix(helm): Include option to use Redis with SSL #26663

Conversation

shakeelansari63 commented Jan 18, 2024 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

github-actions bot left a comment

Choose a reason for hiding this comment

shakeelansari63 commented Jan 18, 2024

1. No Password and No SSL

2. With Password and No SSL

3. No Password and With SSL (ssl_cert_reqs = CERT_NONE)

4. With Password and With SSL (ssl_cert_reqs = CERT_NONE)

5. With Password and With SSL (ssl_cert_reqs = CERT_OPTIONAL)

dpgaspar commented Jan 22, 2024

shakeelansari63 commented Jan 22, 2024

shakeelansari63 commented Jan 22, 2024 • edited Loading

shakeelansari63 commented Jan 26, 2024

With Default Config

With modified config

shakeelansari63 commented Jan 26, 2024

Default Config without Redis SSL and Redis Password

Updated Config with Redis SSL and Redis Password

shakeelansari63 commented Feb 6, 2024

craig-rueda left a comment

Choose a reason for hiding this comment

shakeelansari63 commented Feb 8, 2024

rusackas commented Feb 8, 2024

dpgaspar left a comment

Choose a reason for hiding this comment

shakeelansari63 commented Feb 8, 2024

rusackas commented Feb 8, 2024

shakeelansari63 commented Feb 8, 2024

shakeelansari63 commented Feb 8, 2024

craig-rueda left a comment

Choose a reason for hiding this comment

fengsi Feb 12, 2024

Choose a reason for hiding this comment

fengsi Feb 12, 2024

Choose a reason for hiding this comment

shakeelansari63 commented Jan 18, 2024 •

edited

Loading

shakeelansari63 commented Jan 22, 2024 •

edited

Loading