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

Jump to bottom

feat: adding redis SSL and username authentification #21287

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

feat: adding redis SSL and username authentification #21287

wants to merge 4 commits into from

Conversation

Yoann-tildev
Copy link

SUMMARY

Added compatibility with Redis 6 new authentification (SSL + ACL)

TESTING INSTRUCTIONS

Deploying Superset with Redis 6 (we use managed instance from OVH Cloud)

ADDITIONAL INFORMATION

  • Has associated issue:
  • 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[bot]
github-actions bot reviewed Sep 1, 2022
View reviewed changes
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Congrats on making your first PR and thank you for contributing to Superset! 🎉 ❤️
We hope to see you in our Slack community too!

dpgaspar
dpgaspar reviewed Sep 1, 2022
View reviewed changes
Copy link
Member

@dpgaspar dpgaspar left a comment

Choose a reason for hiding this comment

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

Superset's base dependencies is loose by default on setup.py we don't pin or restrict Redis there, but our test suit and base requirements are pinned to 3.5.3 this could be a breaking change. I think that helm should be in sync with the rest of the repo.

helm/superset/templates/_helpers.tpl
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"
{{- if .Values.supersetNode.connections.redis_ssl }}
BROKER_URL = f"rediss://{env('REDIS_USERNAME')}:{env('REDIS_PASSWORD')}@{env('REDIS_HOST')}:{env('REDIS_PORT')}/0"
Copy link
Member

Choose a reason for hiding this comment

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

Can you add REDIS_SCHEME here to make it possible for an admin to choose between redis and rediss

Copy link
Author

Choose a reason for hiding this comment

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

I am unsure of what you're asking. I introduced "redis_ssl" and I'm checking if the option is set in order to be able to choose between redis and rediss (ssl mode) already.

What's REDIS_SCHEME ?

Copy link
Member

Choose a reason for hiding this comment

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

following this convention: https://www.rfc-editor.org/rfc/rfc3986.html#section-3 for scheme
optional but wouldn't be simpler to just create REDIS_SCHEME?

helm/superset/values.yaml
@@ -35,7 +35,7 @@ bootstrapScript: |
rm -rf /var/lib/apt/lists/* && \
pip install \
psycopg2-binary==2.9.1 \
redis==3.5.3 && \
redis==4.3.4 && \
Copy link
Member

Choose a reason for hiding this comment

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

our current base requirements is: https://github.com/apache/superset/blob/master/requirements/base.txt#L242

helm/superset/templates/_helpers.tpl
@@ -69,14 +69,21 @@ def env(key, default=None):

MAPBOX_API_KEY = env('MAPBOX_API_KEY', '')
CACHE_CONFIG = {
'CACHE_TYPE': 'redis',
'CACHE_TYPE': 'RedisCache',
Copy link
Member

Choose a reason for hiding this comment

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

we probably should change: https://github.com/apache/superset/blob/04dd8d414db6a3cddcd073ad74acb2a4b7a53b0b/docker/pythonpath_dev/superset_config.py

Copy link
Author

Choose a reason for hiding this comment

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

this change was introduced in version 1.10.0 of flask-caching and is mandatory in version 2.X

as I'm using helm I didn't edit the docket-compose setup but it should be safe to do it I guess

@Yoann-tildev
Copy link
Author

Superset's base dependencies is loose by default on setup.py we don't pin or restrict Redis there, but our test suit and base requirements are pinned to 3.5.3 this could be a breaking change. I think that helm should be in sync with the rest of the repo.

3.5.3 is more than 2 years old and doesn't support Redis 6 SSL authentication. Shouldn't it be the time to upgrade this dependency in the test suit ?

@Yoann-tildev
Copy link
Author

Superset's base dependencies is loose by default on setup.py we don't pin or restrict Redis there, but our test suit and base requirements are pinned to 3.5.3 this could be a breaking change. I think that helm should be in sync with the rest of the repo.

3.5.3 is more than 2 years old and doesn't support Redis 6 SSL authentication. Shouldn't it be the time to upgrade this dependency in the test suit ?

I investigated : neither Flask-caching nor CacheLib are specifying a specific version of py-redis package. Actually Flask-caching wasn't even specifying a version for CacheLib before v2.0 which specify 0.9.0 https://github.com/pallets-eco/flask-caching/blob/v2.0.0/setup.py#L6

@dpgaspar
Copy link
Member

dpgaspar commented Sep 2, 2022

Superset's base dependencies is loose by default on setup.py we don't pin or restrict Redis there, but our test suit and base requirements are pinned to 3.5.3 this could be a breaking change. I think that helm should be in sync with the rest of the repo.

3.5.3 is more than 2 years old and doesn't support Redis 6 SSL authentication. Shouldn't it be the time to upgrade this dependency in the test suit ?

Totally agree, can you please upgrade the versions on all the places I mentioned?

@moreiramarti
Copy link

👋
I'm interested about that.
Are there any news ?

@villebro
Copy link
Member

@Yoann-tildev is this PR still relevant? I see the config properties are based on the old version e.g. BROKER_URL vs broker_url which is the new contvention etc. Happy to help review and get this merged if this is still needed, otherwise I propose closing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dpgaspar dpgaspar dpgaspar left review comments

@github-actions github-actions[bot] github-actions left review comments

@craig-rueda craig-rueda Awaiting requested review from craig-rueda craig-rueda is a code owner

@villebro villebro Awaiting requested review from villebro villebro is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Yoann-tildev @dpgaspar @moreiramarti @villebro