Skip to content
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.

Sign up for GitHub

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

feat(Helm): Redis with password supported in helm charts and redis chart version updated #18642

Merged
merged 3 commits into from
Feb 10, 2022
Merged

feat(Helm): Redis with password supported in helm charts and redis chart version updated #18642

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a72fc71
fix import_datasources documentation
wiktor2200 Feb 9, 2022
8ed08e5
Redis with password supported in helm chart
wiktor2200 Feb 9, 2022
12dffad
fix conditionals format and redisURL
wiktor2200 Feb 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/docs/installation/running-on-kubernetes.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ Data source definitions can be automatically declared by providing key/value yam

```yaml
extraConfigs:
datasources-init.yaml: |
import_datasources.yaml: |
databases:
- allow_file_upload: true
allow_ctas: true
Expand Down
4 changes: 2 additions & 2 deletions helm/superset/Chart.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,13 @@ maintainers:
- name: craig-rueda
email: craig@craigrueda.com
url: https://github.com/craig-rueda
version: 0.5.7
version: 0.5.8
dependencies:
- name: postgresql
version: 10.2.0
repository: https://charts.bitnami.com/bitnami
condition: postgresql.enabled
- name: redis
version: 12.3.3
version: 16.3.1
repository: https://charts.bitnami.com/bitnami
condition: redis.enabled
12 changes: 10 additions & 2 deletions helm/superset/templates/_helpers.tpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,14 +90,22 @@ WTF_CSRF_EXEMPT_LIST = []
# A CSRF token that expires in 1 year
WTF_CSRF_TIME_LIMIT = 60 * 60 * 24 * 365
class CeleryConfig(object):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some lines are duplicated in both condition situations. I wonder if we want to have two ifs or if we want duplicate code. I think it's better not to have two ifs than duplicate code, because then it's easier to maintain consistency when a new parameter is added in the middle.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought the same thing, but drying that up will be a little tricky and will likely make the code less readable in this case.

Copy link
Contributor

@ad-m ad-m Feb 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have in mind something like:

class CeleryConfig(object):
  CELERY_ANNOTATIONS = {'tasks.add': {'rate_limit': '10/s'}}
  CELERY_IMPORTS = ('superset.sql_lab', )
{{- if .Values.supersetNode.connections.redis_password }}
  CELERY_RESULT_BACKEND = f"redis://{env('REDIS_PASSWORD')}@{env('REDIS_HOST')}:{env('REDIS_PORT')}/0"
  BROKER_URL = f"redis://{env('REDIS_PASSWORD')}@{env('REDIS_HOST')}:{env('REDIS_PORT')}/0"
{{- else }}
  CELERY_RESULT_BACKEND = f"redis://{env('REDIS_HOST')}:{env('REDIS_PORT')}/0"
  BROKER_URL = f"redis://{env('REDIS_HOST')}:{env('REDIS_PORT')}/0"
{{- end -}}

CELERY_CONFIG = CeleryConfig
RESULTS_BACKEND = RedisCache(
      host=env('REDIS_HOST'),
{{- if .Values.supersetNode.connections.redis_password }}
      password=env('REDIS_PASSWORD'),
{{- end -}}
      port=env('REDIS_PORT'),
      key_prefix='superset_results'
)

Do you think readability has been maintained?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes sense. I've tried it this way before but I wanted diff to be minimal, so I didn't want to change order od parameters. I changed it, because I agree that when it will become bigger one of if/else/end could be omitted and cause lots of troubleshooting.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your feedback! And please take a look once again. @craig-rueda @ad-m

BROKER_URL = f"redis://{env('REDIS_HOST')}:{env('REDIS_PORT')}/0"
CELERY_IMPORTS = ('superset.sql_lab', )
CELERY_RESULT_BACKEND = f"redis://{env('REDIS_HOST')}:{env('REDIS_PORT')}/0"
CELERY_ANNOTATIONS = {'tasks.add': {'rate_limit': '10/s'}}
{{- if .Values.supersetNode.connections.redis_password }}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the check of auth.enabled is needed here. Otherwise, the redis without password is connected with an incorrect url because the default value in value.yaml is set.

BROKER_URL = f"redis://:{env('REDIS_PASSWORD')}@{env('REDIS_HOST')}:{env('REDIS_PORT')}/0"
CELERY_RESULT_BACKEND = f"redis://:{env('REDIS_PASSWORD')}@{env('REDIS_HOST')}:{env('REDIS_PORT')}/0"
{{- else }}
BROKER_URL = f"redis://{env('REDIS_HOST')}:{env('REDIS_PORT')}/0"
CELERY_RESULT_BACKEND = f"redis://{env('REDIS_HOST')}:{env('REDIS_PORT')}/0"
{{- end }}

CELERY_CONFIG = CeleryConfig
RESULTS_BACKEND = RedisCache(
host=env('REDIS_HOST'),
{{- if .Values.supersetNode.connections.redis_password }}
password=env('REDIS_PASSWORD'),
{{- end }}
port=env('REDIS_PORT'),
key_prefix='superset_results'
)
Expand Down
3 changes: 3 additions & 0 deletions helm/superset/templates/secret-env.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@ metadata:
type: Opaque
stringData:
REDIS_HOST: {{ tpl .Values.supersetNode.connections.redis_host . | quote }}
{{- if .Values.supersetNode.connections.redis_password }}
REDIS_PASSWORD: {{ .Values.supersetNode.connections.redis_password | quote }}
{{- end }}
REDIS_PORT: {{ .Values.supersetNode.connections.redis_port | quote }}
DB_HOST: {{ tpl .Values.supersetNode.connections.db_host . | quote }}
DB_PORT: {{ .Values.supersetNode.connections.db_port | quote }}
Expand Down
43 changes: 25 additions & 18 deletions helm/superset/values.schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,9 @@
"redis_host": {
"type": "string"
},
"redis_password": {
"type": "string"
},
"redis_port": {
"type": "string"
},
Expand Down Expand Up @@ -456,24 +459,29 @@
"enabled": {
"type": "boolean"
},
"usePassword": {
"type": "boolean"
},
"existingSecret": {
"type": [
"string",
"null"
]
"architecture": {
"type": "string"
},
"existingSecretKey": {
"type": [
"string",
"null"
"auth": {
"type": "object",
"properties": {
"enabled": {
"type": "boolean"
},
"existingSecret": {
"type": "string"
},
"existingSecretKey": {
"type": "string"
},
"password": {
"type": "string"
}
},
"required": [
"enabled"
]
},
"password": {
"type": "string"
},
"master": {
"type": "object",
"additionalProperties": true,
Expand Down Expand Up @@ -519,9 +527,8 @@
},
"required": [
"enabled",
"usePassword",
"master",
"cluster"
"architecture",
"master"
]
},
"nodeSelector": {
Expand Down
38 changes: 22 additions & 16 deletions helm/superset/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ extraSecretEnv: {}
# GOOGLE_SECRET: ...

extraConfigs: {}
# datasources-init.yaml: |
# import_datasources.yaml: |
# databases:
# - allow_file_upload: true
# allow_ctas: true
Expand Down Expand Up @@ -220,8 +220,11 @@ supersetNode:
- "-c"
- ". {{ .Values.configMountPath }}/superset_bootstrap.sh; /usr/bin/run-server.sh"
connections:
# Change in case of bringing your own redis and then also set redis.enabled:false
redis_host: '{{ template "superset.fullname" . }}-redis-headless'
# redis_password: superset
redis_port: "6379"
# You need to change below configuration incase bringing own PostgresSQL instance and also set postgresql.enabled:false
db_host: '{{ template "superset.fullname" . }}-postgresql'
db_port: "5432"
db_user: superset
Expand Down Expand Up @@ -393,27 +396,34 @@ postgresql:
- ReadWriteOnce

## Configuration values for the Redis dependency.
## ref: https://github.com/kubernetes/charts/blob/master/stable/redis/README.md
## ref: https://github.com/bitnami/charts/blob/master/bitnami/redis
## More documentation can be found here: https://artifacthub.io/packages/helm/bitnami/redis
redis:
##
## Use the redis chart dependency.
##
## If you are bringing your own redis, you can set the host in supersetNode.connections.redis_host
##
## Set to false if bringing your own redis.
enabled: true
usePassword: false
##
## The name of an existing secret that contains the redis password.
existingSecret:
## Name of the key containing the secret.
existingSecretKey: redis-password
##
## If you are bringing your own redis, you can set the host in redisHost.
## redisHost:
## Set architecture to standalone/replication
architecture: standalone
##
## Redis password
## Auth configuration:
##
password: superset
auth:
## Enable password authentication
enabled: false
## The name of an existing secret that contains the redis password.
existingSecret: ""
## Name of the key containing the secret.
existingSecretKey: ""
## Redis password
password: superset
##
## Master configuration
##
master:
##
## Image configuration
Expand All @@ -434,10 +444,6 @@ redis:
## Access mode:
accessModes:
- ReadWriteOnce
##
## Disable cluster management by default.
cluster:
enabled: false

nodeSelector: {}

Expand Down