Skip to content

[AIRFLOW-3268] Cannot pass SSL dictionary to mysql connection via URL#4113

Merged
kaxil merged 1 commit intoapache:masterfrom
PolideaInternal:AIRFLOW-3268-pass-ssl-as-dictionary-mysql
Nov 5, 2018
Merged

[AIRFLOW-3268] Cannot pass SSL dictionary to mysql connection via URL#4113
kaxil merged 1 commit intoapache:masterfrom
PolideaInternal:AIRFLOW-3268-pass-ssl-as-dictionary-mysql

Conversation

@potiuk
Copy link
Member

@potiuk potiuk commented Oct 29, 2018

Make sure you have checked all steps below.

Jira

Description

  • Here are some details about my PR, including screenshots of any UI changes:
    It is impossible to pass 'ssl' dictionary to MySql hook as an extrai
    param via URL connection. While there is a code to pass the 'ssl'
    extra query parameter, MySqldb requires this parameter to be
    dictionary. When you want to create a connection via URL,
    you can at most have ?ssl= url-encoded string rather than
    dictionary and this is how it is passed (as string). What happens
    then in MySqldb, is that all SSL parameters are ignored
    and MySQL establishes a non-SSL connection silently.

This is fixed by detecting if 'ssl' is a string and converting
it to json dictionary in such case.

Tests

  • My PR adds the following unit tests
    TestMySqlHookConn:
    test_get_conn_ssl_as_dictionary
    test_get_conn_ssl_as_string

Commits

  • My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters (not including Jira issue reference)
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Documentation

  • In case of new functionality, my PR adds documentation that describes how to use it.
    No new functionality

Code Quality

  • Passes flake8

@potiuk potiuk force-pushed the AIRFLOW-3268-pass-ssl-as-dictionary-mysql branch from 0e344d2 to d9e399f Compare October 29, 2018 20:57
@codecov-io
Copy link

codecov-io commented Oct 29, 2018

Codecov Report

Merging #4113 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4113      +/-   ##
==========================================
+ Coverage   76.69%   76.71%   +0.01%     
==========================================
  Files         199      199              
  Lines       16233    16238       +5     
==========================================
+ Hits        12450    12457       +7     
+ Misses       3783     3781       -2
Impacted Files Coverage Δ
airflow/hooks/mysql_hook.py 92.98% <100%> (+2.59%) ⬆️
airflow/models.py 92.15% <0%> (+0.04%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e53182c...d856af3. Read the comment docs.

@kaxil
Copy link
Member

kaxil commented Oct 31, 2018

Can this be documented somewhere i.e. what can be passed into Extras field?

Currently, a user needs to check the source code to see what can be passed which isn't ideal.

@potiuk potiuk force-pushed the AIRFLOW-3268-pass-ssl-as-dictionary-mysql branch 2 times, most recently from f32b23e to 6d0eff9 Compare November 5, 2018 14:51
@potiuk
Copy link
Member Author

potiuk commented Nov 5, 2018

Hey @kaxil - Why not :). I added it now, reverse engineering it from the code. It's good to leave the world a bit better than you found. I added useful documentation now to MySQL connection. I will soon submit CloudSQL query one which I will add similar info about new cloudsql:// connection and about underlying Postgres connection as well.

@kaxil
Copy link
Member

kaxil commented Nov 5, 2018

@potiuk Thanks, appreciate that :) . Can you update the commit message as well to something along the line of "Fix Issue with Extras field in MySQL connection" or something that help users identify what issue was solved.

The current commit message is more of a "Jira issue".

It is impossible to pass 'ssl' dictionary to MySql hook as an extrai
param via URL connection. While there is a code to pass the 'ssl'
extra query parameter, MySqldb requires this parameter to be
dictionary. When you want to create a connection via URL,
you can at most have ?ssl= url-encoded string rather than
dictionary and this is how it is passed (as string). What happens
then in MySqldb, is that all SSL parameters are ignored
and MySQL establishes a non-SSL connection silently.

This is fixed by detecting if 'ssl' is a string and converting
it to json dictionary in such case.

Also documentation is updated to show examples of MySQL extras field
and how URI for MySQL connection shoudl be constructed.
@potiuk potiuk force-pushed the AIRFLOW-3268-pass-ssl-as-dictionary-mysql branch from 6d0eff9 to d856af3 Compare November 5, 2018 15:20
@potiuk
Copy link
Member Author

potiuk commented Nov 5, 2018

Sure :). Done.

@kaxil kaxil merged commit b5ecb8a into apache:master Nov 5, 2018
@kaxil
Copy link
Member

kaxil commented Nov 5, 2018

Awesome. Thanks @potiuk

wyndhblb pushed a commit to asappinc/incubator-airflow that referenced this pull request Nov 9, 2018
galak75 pushed a commit to VilledeMontreal/incubator-airflow that referenced this pull request Nov 23, 2018
tmiller-msft pushed a commit to cse-airflow/incubator-airflow that referenced this pull request Nov 27, 2018
elizabethhalper pushed a commit to cse-airflow/incubator-airflow that referenced this pull request Dec 7, 2018
aliceabe pushed a commit to aliceabe/incubator-airflow that referenced this pull request Jan 3, 2019
@potiuk potiuk deleted the AIRFLOW-3268-pass-ssl-as-dictionary-mysql branch January 11, 2019 16:03
cfei18 pushed a commit to cfei18/incubator-airflow that referenced this pull request Jan 23, 2019
wmorris75 pushed a commit to modmed-external/incubator-airflow that referenced this pull request Jul 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants