You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2020/07/01 19:15:32 UTC
[GitHub] [airflow] Unit03 opened a new pull request #9615: Mask other forms of password arguments in SparkSubmitOperator
Unit03 opened a new pull request #9615:
URL: https://github.com/apache/airflow/pull/9615
This is a follow-up to #6917. Related: #9595.
---
Make sure to mark the boxes below before creating PR: [x]
- [x] Description above provides context of the change
- [x] Unit tests coverage for changes (not needed for documentation changes)
- [x] Target Github ISSUE in description if exists
- [x] Commits follow "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)"
- [x] Relevant documentation is updated including usage instructions.
- [x] I will engage committers as explained in [Contribution Workflow Example](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#contribution-workflow-example).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #9615: Mask other forms of password arguments in SparkSubmitOperator
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #9615:
URL: https://github.com/apache/airflow/pull/9615#discussion_r450034396
##########
File path: tests/providers/apache/spark/hooks/test_spark_submit.py
##########
@@ -748,3 +750,64 @@ def test_k8s_process_on_kill(self, mock_popen, mock_client_method):
client.delete_namespaced_pod.assert_called_once_with(
'spark-pi-edf2ace37be7353a958b38733a12f8e6-driver',
'mynamespace', **kwargs)
+
+
+@pytest.mark.parametrize(
+ ("command", "expected"),
+ (
+ (
+ ("spark-submit", "foo", "--bar", "baz", "--password='secret'"),
+ "spark-submit foo --bar baz --password='******'",
+ ),
+ (
+ ("spark-submit", "foo", "--bar", "baz", '--password="secret"'),
+ 'spark-submit foo --bar baz --password="******"',
+ ),
+ (
+ ("spark-submit", "foo", "--bar", "baz", "--password=secret"),
+ "spark-submit foo --bar baz --password=******",
+ ),
+ (
+ ("spark-submit", "foo", "--bar", "baz", "--password 'secret'"),
+ "spark-submit foo --bar baz --password '******'",
+ ),
+ (
+ ("spark-submit", "foo", "--bar", "baz", "--password secret"),
+ "spark-submit foo --bar baz --password ******",
+ ),
+ (
+ ("spark-submit", "foo", "--bar", "baz", '--password "secret"'),
+ 'spark-submit foo --bar baz --password "******"',
+ ),
+ (
+ ("spark-submit", "foo", "--bar", "baz", "--secret='secret'"),
+ "spark-submit foo --bar baz --secret='******'",
+ ),
+ (
+ ("spark-submit", "foo", "--bar", "baz", "--foo.password='secret'"),
+ "spark-submit foo --bar baz --foo.password='******'",
+ ),
+ (
+ ("spark-submit",),
+ "spark-submit",
+ ),
+
+ (
+ ("spark-submit", "foo", "--bar", "baz", "--password \"secret'"),
+ "spark-submit foo --bar baz --password \"secret'",
+ ),
+ (
+ ("spark-submit", "foo", "--bar", "baz", "--password 'secret\""),
+ "spark-submit foo --bar baz --password 'secret\"",
+ ),
+ ),
+)
+def test_masks_passwords(command: str, expected: str) -> None:
Review comment:
I would say that we are ok with single test case for each possibility without testing all combinations
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Unit03 commented on a change in pull request #9615: Mask other forms of password arguments in SparkSubmitOperator
Posted by GitBox <gi...@apache.org>.
Unit03 commented on a change in pull request #9615:
URL: https://github.com/apache/airflow/pull/9615#discussion_r449802853
##########
File path: airflow/providers/apache/spark/hooks/spark_submit.py
##########
@@ -237,8 +237,8 @@ def _mask_cmd(self, connection_cmd):
# Mask any password related fields in application args with key value pair
# where key contains password (case insensitive), e.g. HivePassword='abc'
connection_cmd_masked = re.sub(
- r"(\S*?(?:secret|password)\S*?\s*=\s*')[^']*(?=')",
- r'\1******', ' '.join(connection_cmd), flags=re.I)
+ r"(\S*?(?:secret|password)\S*?\s*(?:=|\s+)(['\"]?))[^'^\"]+(\2)",
Review comment:
Done (although I didn't use in-regexp comments). Let me know which parts aren't clear enough. :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #9615: Mask other forms of password arguments in SparkSubmitOperator
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #9615:
URL: https://github.com/apache/airflow/pull/9615#issuecomment-657041304
Awesome work, congrats on your first merged pull request!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #9615: Mask other forms of password arguments in SparkSubmitOperator
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #9615:
URL: https://github.com/apache/airflow/pull/9615#issuecomment-652599302
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst)
Here are some useful points:
- Pay attention to the quality of your code (flake8, pylint and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/master/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) will help you with that.
- In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/master/docs/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/master/BREEZE.rst) for testing locally, itβs a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#coding-style-and-best-practices).
Apache Airflow is a community-driven project and together we are making it better π.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://apache-airflow-slack.herokuapp.com/
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Unit03 commented on a change in pull request #9615: Mask other forms of password arguments in SparkSubmitOperator
Posted by GitBox <gi...@apache.org>.
Unit03 commented on a change in pull request #9615:
URL: https://github.com/apache/airflow/pull/9615#discussion_r448566919
##########
File path: airflow/providers/apache/spark/hooks/spark_submit.py
##########
@@ -237,8 +237,8 @@ def _mask_cmd(self, connection_cmd):
# Mask any password related fields in application args with key value pair
# where key contains password (case insensitive), e.g. HivePassword='abc'
connection_cmd_masked = re.sub(
- r"(\S*?(?:secret|password)\S*?\s*=\s*')[^']*(?=')",
- r'\1******', ' '.join(connection_cmd), flags=re.I)
+ r"(\S*?(?:secret|password)\S*?\s*(?:=|\s+)(['\"]?))[^'^\"]+(\2)",
Review comment:
Does this regular expression deserve some more explaining, possibly in the form of [inline comments](https://docs.python.org/3/library/re.html#re.VERBOSE)?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #9615: Mask other forms of password arguments in SparkSubmitOperator
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #9615:
URL: https://github.com/apache/airflow/pull/9615#discussion_r449012333
##########
File path: airflow/providers/apache/spark/hooks/spark_submit.py
##########
@@ -237,8 +237,8 @@ def _mask_cmd(self, connection_cmd):
# Mask any password related fields in application args with key value pair
# where key contains password (case insensitive), e.g. HivePassword='abc'
connection_cmd_masked = re.sub(
- r"(\S*?(?:secret|password)\S*?\s*=\s*')[^']*(?=')",
- r'\1******', ' '.join(connection_cmd), flags=re.I)
+ r"(\S*?(?:secret|password)\S*?\s*(?:=|\s+)(['\"]?))[^'^\"]+(\2)",
Review comment:
It would be nice π
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on pull request #9615: Mask other forms of password arguments in SparkSubmitOperator
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #9615:
URL: https://github.com/apache/airflow/pull/9615#issuecomment-661747284
@Unit03 Thank you for this contribution. What's your plan for your next contribution?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #9615: Mask other forms of password arguments in SparkSubmitOperator
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #9615:
URL: https://github.com/apache/airflow/pull/9615#discussion_r449011845
##########
File path: tests/providers/apache/spark/hooks/test_spark_submit.py
##########
@@ -748,3 +750,64 @@ def test_k8s_process_on_kill(self, mock_popen, mock_client_method):
client.delete_namespaced_pod.assert_called_once_with(
'spark-pi-edf2ace37be7353a958b38733a12f8e6-driver',
'mynamespace', **kwargs)
+
+
+@pytest.mark.parametrize(
+ ("command", "expected"),
+ (
+ (
+ ("spark-submit", "foo", "--bar", "baz", "--password='secret'"),
+ "spark-submit foo --bar baz --password='******'",
+ ),
+ (
+ ("spark-submit", "foo", "--bar", "baz", '--password="secret"'),
+ 'spark-submit foo --bar baz --password="******"',
+ ),
+ (
+ ("spark-submit", "foo", "--bar", "baz", "--password=secret"),
+ "spark-submit foo --bar baz --password=******",
+ ),
+ (
+ ("spark-submit", "foo", "--bar", "baz", "--password 'secret'"),
+ "spark-submit foo --bar baz --password '******'",
+ ),
+ (
+ ("spark-submit", "foo", "--bar", "baz", "--password secret"),
+ "spark-submit foo --bar baz --password ******",
+ ),
+ (
+ ("spark-submit", "foo", "--bar", "baz", '--password "secret"'),
+ 'spark-submit foo --bar baz --password "******"',
+ ),
+ (
+ ("spark-submit", "foo", "--bar", "baz", "--secret='secret'"),
+ "spark-submit foo --bar baz --secret='******'",
+ ),
+ (
+ ("spark-submit", "foo", "--bar", "baz", "--foo.password='secret'"),
+ "spark-submit foo --bar baz --foo.password='******'",
+ ),
+ (
+ ("spark-submit",),
+ "spark-submit",
+ ),
+
+ (
+ ("spark-submit", "foo", "--bar", "baz", "--password \"secret'"),
+ "spark-submit foo --bar baz --password \"secret'",
+ ),
+ (
+ ("spark-submit", "foo", "--bar", "baz", "--password 'secret\""),
+ "spark-submit foo --bar baz --password 'secret\"",
+ ),
+ ),
+)
+def test_masks_passwords(command: str, expected: str) -> None:
Review comment:
You can use https://pypi.org/project/parameterized/ in future we will probably migrate to pytest
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Unit03 commented on a change in pull request #9615: Mask other forms of password arguments in SparkSubmitOperator
Posted by GitBox <gi...@apache.org>.
Unit03 commented on a change in pull request #9615:
URL: https://github.com/apache/airflow/pull/9615#discussion_r449802853
##########
File path: airflow/providers/apache/spark/hooks/spark_submit.py
##########
@@ -237,8 +237,8 @@ def _mask_cmd(self, connection_cmd):
# Mask any password related fields in application args with key value pair
# where key contains password (case insensitive), e.g. HivePassword='abc'
connection_cmd_masked = re.sub(
- r"(\S*?(?:secret|password)\S*?\s*=\s*')[^']*(?=')",
- r'\1******', ' '.join(connection_cmd), flags=re.I)
+ r"(\S*?(?:secret|password)\S*?\s*(?:=|\s+)(['\"]?))[^'^\"]+(\2)",
Review comment:
Done. Let me know which parts aren't clear enough. :)
##########
File path: tests/providers/apache/spark/hooks/test_spark_submit.py
##########
@@ -748,3 +750,64 @@ def test_k8s_process_on_kill(self, mock_popen, mock_client_method):
client.delete_namespaced_pod.assert_called_once_with(
'spark-pi-edf2ace37be7353a958b38733a12f8e6-driver',
'mynamespace', **kwargs)
+
+
+@pytest.mark.parametrize(
+ ("command", "expected"),
+ (
+ (
+ ("spark-submit", "foo", "--bar", "baz", "--password='secret'"),
+ "spark-submit foo --bar baz --password='******'",
+ ),
+ (
+ ("spark-submit", "foo", "--bar", "baz", '--password="secret"'),
+ 'spark-submit foo --bar baz --password="******"',
+ ),
+ (
+ ("spark-submit", "foo", "--bar", "baz", "--password=secret"),
+ "spark-submit foo --bar baz --password=******",
+ ),
+ (
+ ("spark-submit", "foo", "--bar", "baz", "--password 'secret'"),
+ "spark-submit foo --bar baz --password '******'",
+ ),
+ (
+ ("spark-submit", "foo", "--bar", "baz", "--password secret"),
+ "spark-submit foo --bar baz --password ******",
+ ),
+ (
+ ("spark-submit", "foo", "--bar", "baz", '--password "secret"'),
+ 'spark-submit foo --bar baz --password "******"',
+ ),
+ (
+ ("spark-submit", "foo", "--bar", "baz", "--secret='secret'"),
+ "spark-submit foo --bar baz --secret='******'",
+ ),
+ (
+ ("spark-submit", "foo", "--bar", "baz", "--foo.password='secret'"),
+ "spark-submit foo --bar baz --foo.password='******'",
+ ),
+ (
+ ("spark-submit",),
+ "spark-submit",
+ ),
+
+ (
+ ("spark-submit", "foo", "--bar", "baz", "--password \"secret'"),
+ "spark-submit foo --bar baz --password \"secret'",
+ ),
+ (
+ ("spark-submit", "foo", "--bar", "baz", "--password 'secret\""),
+ "spark-submit foo --bar baz --password 'secret\"",
+ ),
+ ),
+)
+def test_masks_passwords(command: str, expected: str) -> None:
Review comment:
Done. :)
Btw when changing the regular expression, I considered these things happening in the string under masking to possibly be impacted by bugs in said expression:
* with and without the `=`
* with single quoted/double quoted/not quoted secret value
* with and without additional non-matching quotes in the middle of the value
* with and without flags/arguments after the secret value
I have added all of the combinations for now but that's quite a few - it may be better to get rid of some of them. What do you think?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Unit03 commented on a change in pull request #9615: Mask other forms of password arguments in SparkSubmitOperator
Posted by GitBox <gi...@apache.org>.
Unit03 commented on a change in pull request #9615:
URL: https://github.com/apache/airflow/pull/9615#discussion_r448568444
##########
File path: tests/providers/apache/spark/hooks/test_spark_submit.py
##########
@@ -748,3 +750,64 @@ def test_k8s_process_on_kill(self, mock_popen, mock_client_method):
client.delete_namespaced_pod.assert_called_once_with(
'spark-pi-edf2ace37be7353a958b38733a12f8e6-driver',
'mynamespace', **kwargs)
+
+
+@pytest.mark.parametrize(
+ ("command", "expected"),
+ (
+ (
+ ("spark-submit", "foo", "--bar", "baz", "--password='secret'"),
+ "spark-submit foo --bar baz --password='******'",
+ ),
+ (
+ ("spark-submit", "foo", "--bar", "baz", '--password="secret"'),
+ 'spark-submit foo --bar baz --password="******"',
+ ),
+ (
+ ("spark-submit", "foo", "--bar", "baz", "--password=secret"),
+ "spark-submit foo --bar baz --password=******",
+ ),
+ (
+ ("spark-submit", "foo", "--bar", "baz", "--password 'secret'"),
+ "spark-submit foo --bar baz --password '******'",
+ ),
+ (
+ ("spark-submit", "foo", "--bar", "baz", "--password secret"),
+ "spark-submit foo --bar baz --password ******",
+ ),
+ (
+ ("spark-submit", "foo", "--bar", "baz", '--password "secret"'),
+ 'spark-submit foo --bar baz --password "******"',
+ ),
+ (
+ ("spark-submit", "foo", "--bar", "baz", "--secret='secret'"),
+ "spark-submit foo --bar baz --secret='******'",
+ ),
+ (
+ ("spark-submit", "foo", "--bar", "baz", "--foo.password='secret'"),
+ "spark-submit foo --bar baz --foo.password='******'",
+ ),
+ (
+ ("spark-submit",),
+ "spark-submit",
+ ),
+
+ (
+ ("spark-submit", "foo", "--bar", "baz", "--password \"secret'"),
+ "spark-submit foo --bar baz --password \"secret'",
+ ),
+ (
+ ("spark-submit", "foo", "--bar", "baz", "--password 'secret\""),
+ "spark-submit foo --bar baz --password 'secret\"",
+ ),
+ ),
+)
+def test_masks_passwords(command: str, expected: str) -> None:
Review comment:
I figured it would be a good place to use `pytest.mark.parameterize` - [but it doesn't go along with `unittest.TestCase`](https://docs.pytest.org/en/stable/unittest.html#pytest-features-in-unittest-testcase-subclasses).
On the other hand, I get it that going with a separate function outside of the `TestSparkSubmitHook` class looks kind of awkward. Which way do we want this test to go?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Unit03 commented on a change in pull request #9615: Mask other forms of password arguments in SparkSubmitOperator
Posted by GitBox <gi...@apache.org>.
Unit03 commented on a change in pull request #9615:
URL: https://github.com/apache/airflow/pull/9615#discussion_r453174207
##########
File path: tests/providers/apache/spark/hooks/test_spark_submit.py
##########
@@ -748,3 +750,64 @@ def test_k8s_process_on_kill(self, mock_popen, mock_client_method):
client.delete_namespaced_pod.assert_called_once_with(
'spark-pi-edf2ace37be7353a958b38733a12f8e6-driver',
'mynamespace', **kwargs)
+
+
+@pytest.mark.parametrize(
+ ("command", "expected"),
+ (
+ (
+ ("spark-submit", "foo", "--bar", "baz", "--password='secret'"),
+ "spark-submit foo --bar baz --password='******'",
+ ),
+ (
+ ("spark-submit", "foo", "--bar", "baz", '--password="secret"'),
+ 'spark-submit foo --bar baz --password="******"',
+ ),
+ (
+ ("spark-submit", "foo", "--bar", "baz", "--password=secret"),
+ "spark-submit foo --bar baz --password=******",
+ ),
+ (
+ ("spark-submit", "foo", "--bar", "baz", "--password 'secret'"),
+ "spark-submit foo --bar baz --password '******'",
+ ),
+ (
+ ("spark-submit", "foo", "--bar", "baz", "--password secret"),
+ "spark-submit foo --bar baz --password ******",
+ ),
+ (
+ ("spark-submit", "foo", "--bar", "baz", '--password "secret"'),
+ 'spark-submit foo --bar baz --password "******"',
+ ),
+ (
+ ("spark-submit", "foo", "--bar", "baz", "--secret='secret'"),
+ "spark-submit foo --bar baz --secret='******'",
+ ),
+ (
+ ("spark-submit", "foo", "--bar", "baz", "--foo.password='secret'"),
+ "spark-submit foo --bar baz --foo.password='******'",
+ ),
+ (
+ ("spark-submit",),
+ "spark-submit",
+ ),
+
+ (
+ ("spark-submit", "foo", "--bar", "baz", "--password \"secret'"),
+ "spark-submit foo --bar baz --password \"secret'",
+ ),
+ (
+ ("spark-submit", "foo", "--bar", "baz", "--password 'secret\""),
+ "spark-submit foo --bar baz --password 'secret\"",
+ ),
+ ),
+)
+def test_masks_passwords(command: str, expected: str) -> None:
Review comment:
Sure. I tried to leave the most representative subset, does it look reasonable?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek merged pull request #9615: Mask other forms of password arguments in SparkSubmitOperator
Posted by GitBox <gi...@apache.org>.
turbaszek merged pull request #9615:
URL: https://github.com/apache/airflow/pull/9615
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj edited a comment on pull request #9615: Mask other forms of password arguments in SparkSubmitOperator
Posted by GitBox <gi...@apache.org>.
mik-laj edited a comment on pull request #9615:
URL: https://github.com/apache/airflow/pull/9615#issuecomment-661747284
@Unit03 Thank you for this contribution. What's your plan for your next contribution?
CC: @turbaszek
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org