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.

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

Add init_command parameter to MySqlHook #33359

Merged
merged 1 commit into from
Aug 18, 2023

Conversation

alexbegg
Copy link
Contributor

@alexbegg alexbegg commented Aug 13, 2023

This allows the addition of init_command as a MySqlHook parameter.

init_command is a connection argument to set an initial command to issue to the MySQL server upon connection. It is available as both a mysqlclient connection attribute (https://mysqlclient.readthedocs.io/user_guide.html?highlight=init_command#functions-and-attributes) and a mysql-connector-python connection attribute (https://dev.mysql.com/doc/connector-python/en/connector-python-connectargs.html).

For example, to set the session's time_zone to UTC you can set in a MySqlHook an init_command parameter as shown:

MySqlHook(init_command="SET time_zone = '+00:00';")

or in a SQL operator shown using hook_params (which should return "+00:00"):

SQLExecuteQueryOperator(
    task_id="check_time_zone",
    conn_id="mysql_default",
    hook_params={"init_command": "SET time_zone = '+00:00';"},
    sql=r"""SELECT @@SESSION.time_zone;""",
    autocommit=True,
    show_return_value_in_logs=True
)

closes: #33300


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@alexbegg
Copy link
Contributor Author

alexbegg commented Aug 13, 2023

I have ran the tests and have tested a DAG locally with a MySQL connection created with the extra set to

{
  "init_command": "SET time_zone = '+00:00';"
}

and a task to output the @@SESSION.time_zone as shown:

SQLExecuteQueryOperator(
    task_id='check_time_zone',
    conn_id='mysql_default',
    sql=r"""SELECT @@SESSION.time_zone;""",
    autocommit=True,
    show_return_value_in_logs=True
)

with the init_command set in the connection as shown the task returned +00:00 as expected, without it the task returned the default time_zone of SYSTEM.

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

Main reason we are not adding all the potential options to extras is potential problem with security.

Allowing to execute arbitrary command via "extra" is potential security risk. This gives the user who can edit connections more priviliedges than needed. We had a number of security vulnerabilities, where we had to remove features like that from connection extras.

While we have modified our security model description https://airflow.apache.org/docs/apache-airflow/stable/security/index.html and hopefully informed our users that users who have access to edit connection can have more priviledges, adding a new option for that knowingly is a bad idea.

The "extra" parameters in ideal world should only be limited to declarative parameters, that merrely configure options of the underlying driver. passing arbitrary code to execute is a no-go.

It's quite ok to add an init_command when you initilalize the Hook - and with sql operator you can pass such parameter via hook_params, so that DAG authors could modify it. so if you can chang e it so that init command canot be passed by extra, but by hook parameter, then I'd be perfectly fine with accepting such PR.

@alexbegg
Copy link
Contributor Author

Thank you for the explanation. I see your point and will revise this PR to turn this into a hook parameter.

@alexbegg alexbegg force-pushed the mysql-init_command branch 2 times, most recently from b8bccd6 to 8fcc169 Compare August 14, 2023 23:29
@alexbegg
Copy link
Contributor Author

Ok, this is changed to a MySqlHook param now, and the PR description is updated

@alexbegg alexbegg requested a review from potiuk August 14, 2023 23:35
@alexbegg
Copy link
Contributor Author

It's quite ok to add an init_command when you initilalize the Hook - and with sql operator you can pass such parameter via hook_params, so that DAG authors could modify it.

Oh, I didn't even know about the hook_params sql operator parameter, that will be exactly what I need, a way to use this in a SQLExecuteQueryOperator. Thanks for the heads up!

@alexbegg alexbegg force-pushed the mysql-init_command branch from 8fcc169 to ea17f4d Compare August 14, 2023 23:50
@alexbegg alexbegg marked this pull request as draft August 15, 2023 08:39
@alexbegg alexbegg force-pushed the mysql-init_command branch 4 times, most recently from 8fdf187 to 43bd7e1 Compare August 15, 2023 15:36
@alexbegg alexbegg marked this pull request as ready for review August 15, 2023 15:37
@alexbegg alexbegg changed the title Allow init_command in MySQL provider Add init_command param to MySqlHook Aug 15, 2023
@alexbegg alexbegg force-pushed the mysql-init_command branch from 43bd7e1 to f388dbe Compare August 15, 2023 15:53
@alexbegg alexbegg changed the title Add init_command param to MySqlHook Add init_command parameter to MySqlHook Aug 15, 2023
@alexbegg
Copy link
Contributor Author

@potiuk this is now ready for review again

@alexbegg alexbegg force-pushed the mysql-init_command branch from f388dbe to a0ee2b3 Compare August 15, 2023 21:22
@alexbegg alexbegg changed the title Add init_command parameter to MySqlHook Add init_command parameter to MySqlHook Aug 16, 2023
This allows the addition of `init_command` as a `MySqlHook` parameter.

For example, to set the MySQL session's `time_zone` to UTC you can set the `init_command` to `SET time_zone = '+00:00';`.
@alexbegg alexbegg force-pushed the mysql-init_command branch from a0ee2b3 to 749f322 Compare August 16, 2023 17:41
@alexbegg
Copy link
Contributor Author

@potiuk it still says "change requested" although I already made the requested change, can you review it again?

@potiuk potiuk merged commit dce9796 into apache:main Aug 18, 2023
@potiuk
Copy link
Member

potiuk commented Aug 18, 2023

Cool. Just got to it now. Please exercise a bit of patience next time. No need to hurry things like that.

@alexbegg alexbegg deleted the mysql-init_command branch August 19, 2023 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MySqlHook add support for init_command
2 participants