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

Make encryption default if Security is given arguments #3887

Merged
merged 1 commit into from
Jun 17, 2020
Merged

Make encryption default if Security is given arguments #3887

merged 1 commit into from
Jun 17, 2020

Conversation

mrocklin
Copy link
Member

Fixes #3886

@mrocklin mrocklin mentioned this pull request Jun 11, 2020
Closed
@mrocklin mrocklin force-pushed the encrypted-default branch from 07e612b to 2f458f5 Compare June 11, 2020 16:37
@mrocklin mrocklin force-pushed the encrypted-default branch from 2f458f5 to 4486684 Compare June 11, 2020 19:42
@mrocklin
Copy link
Member Author

Merging tomorrow if there are no comments. cc @jcrist

samaust
samaust reviewed Jun 12, 2020
View reviewed changes
distributed/security.py
if require_encryption is None:
require_encryption = dask.config.get("distributed.comm.require-encryption")
if require_encryption is None:
require_encryption = not not kwargs
Copy link

Choose a reason for hiding this comment

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

Can you explain what this line does?

Copy link
Member

Choose a reason for hiding this comment

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

I suspect this is a mistake ? not not ?

Copy link
Member Author

Choose a reason for hiding this comment

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

not not turns anything into True/False if it is truthy/falsey.

In [1]: not not {"x": 1}                                                                                                                                                                                          
Out[1]: True

In [2]: not not {}                                                                                                                                                                                                
Out[2]: False

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably calling bool is better

Copy link
Member

Choose a reason for hiding this comment

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

Oh! Cool! And we do that in other places throughout the dask ecosystem.

So @samaust this line is saying any time the user is setting security settings (in kwargs) dask will now enable require_encryption without the user explicitly setting it. Essentially the title of the PR.

@quasiben
Copy link
Member

merging in

@quasiben quasiben merged commit acb0f08 into dask:master Jun 17, 2020
@mrocklin mrocklin deleted the encrypted-default branch June 17, 2020 20:06
@pentschev pentschev mentioned this pull request Jun 19, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samaust samaust samaust left review comments

@quasiben quasiben quasiben left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Default value for Security(require_encryption=) keyword
3 participants
@mrocklin @quasiben @samaust