From cb5d207333cee7253becba69329f4023aa224479 Mon Sep 17 00:00:00 2001 From: Ash Berlin-Taylor Date: Sun, 7 Jun 2020 22:03:05 +0100 Subject: [PATCH 1/3] Don't use the term "whitelist" - language matters It's fairly common to say whitelisting and blacklisting to describe desirable and undesirable things in cyber security. However just because it is common doesn't mean it's right. However, there's an issue with the terminology. It only makes sense if you equate white with 'good, permitted, safe' and black with 'bad, dangerous, forbidden'. There are some obvious problems with this. You may not see why this matters. If you're not adversely affected by racial stereotyping yourself, then please count yourself lucky. For some of your friends and colleagues (and potential future colleagues), this really is a change worth making. From now on, we will use 'allow list' and 'deny list' in place of 'whitelist' and 'blacklist' wherever possible. Which, in fact, is clearer and less ambiguous. So as well as being more inclusive of all, this is a net benefit to our understandability. (Words mostly borrowed from ) --- UPDATING.md | 2 +- airflow/jobs/scheduler_job.py | 20 ++++++++--------- .../apache/hive/operators/hive_stats.py | 22 ++++++++++++++----- pylintrc | 4 ++-- .../apache/cassandra/hooks/test_cassandra.py | 2 +- .../apache/hive/operators/test_hive_stats.py | 6 ++--- tests/test_stats.py | 4 ++-- tests/utils/test_dag_processing.py | 4 ++-- 8 files changed, 37 insertions(+), 27 deletions(-) diff --git a/UPDATING.md b/UPDATING.md index 1df6a986a92ee..07662fae9b0ed 100644 --- a/UPDATING.md +++ b/UPDATING.md @@ -2293,7 +2293,7 @@ dags_are_paused_at_creation = False If you specify a hive conf to the run_cli command of the HiveHook, Airflow add some convenience variables to the config. In case you run a secure Hadoop setup it might be -required to whitelist these variables by adding the following to your configuration: +required to allow these variables by adding the following to your configuration: ``` diff --git a/airflow/jobs/scheduler_job.py b/airflow/jobs/scheduler_job.py index 99dcf50677ba0..b5d5549ee4d0c 100644 --- a/airflow/jobs/scheduler_job.py +++ b/airflow/jobs/scheduler_job.py @@ -70,8 +70,8 @@ class DagFileProcessorProcess(AbstractDagFileProcessorProcess, LoggingMixin, Mul :type file_path: str :param pickle_dags: whether to serialize the DAG objects to the DB :type pickle_dags: bool - :param dag_id_white_list: If specified, only look at these DAG ID's - :type dag_id_white_list: List[str] + :param dag_ids: If specified, only look at these DAG ID's + :type dag_ids: List[str] :param failure_callback_requests: failure callback to execute :type failure_callback_requests: List[airflow.utils.dag_processing.FailureCallbackRequest] """ @@ -83,13 +83,13 @@ def __init__( self, file_path: str, pickle_dags: bool, - dag_id_white_list: Optional[List[str]], + dag_ids: Optional[List[str]], failure_callback_requests: List[FailureCallbackRequest] ): super().__init__() self._file_path = file_path self._pickle_dags = pickle_dags - self._dag_id_white_list = dag_id_white_list + self._dag_ids = dag_ids self._failure_callback_requests = failure_callback_requests # The process that was launched to process the given . @@ -116,7 +116,7 @@ def file_path(self): def _run_file_processor(result_channel, file_path, pickle_dags, - dag_id_white_list, + dag_ids, thread_name, failure_callback_requests): """ @@ -129,9 +129,9 @@ def _run_file_processor(result_channel, :param pickle_dags: whether to pickle the DAGs found in the file and save them to the DB :type pickle_dags: bool - :param dag_id_white_list: if specified, only examine DAG ID's that are + :param dag_ids: if specified, only examine DAG ID's that are in this list - :type dag_id_white_list: list[str] + :type dag_ids: list[str] :param thread_name: the name to use for the process that is launched :type thread_name: str :param failure_callback_requests: failure callback to execute @@ -160,7 +160,7 @@ def _run_file_processor(result_channel, start_time = time.time() log.info("Started process (PID=%s) to work on %s", os.getpid(), file_path) - dag_file_processor = DagFileProcessor(dag_ids=dag_id_white_list, log=log) + dag_file_processor = DagFileProcessor(dag_ids=dag_ids, log=log) result = dag_file_processor.process_file( file_path=file_path, pickle_dags=pickle_dags, @@ -195,7 +195,7 @@ def start(self): _child_channel, self.file_path, self._pickle_dags, - self._dag_id_white_list, + self._dag_ids, "DagFileProcessor{}".format(self._instance_id), self._failure_callback_requests ), @@ -1583,7 +1583,7 @@ def _create_dag_file_processor(file_path, failure_callback_requests, dag_ids, pi return DagFileProcessorProcess( file_path=file_path, pickle_dags=pickle_dags, - dag_id_white_list=dag_ids, + dag_ids=dag_ids, failure_callback_requests=failure_callback_requests ) diff --git a/airflow/providers/apache/hive/operators/hive_stats.py b/airflow/providers/apache/hive/operators/hive_stats.py index 2b0f968249459..96504c9451dad 100644 --- a/airflow/providers/apache/hive/operators/hive_stats.py +++ b/airflow/providers/apache/hive/operators/hive_stats.py @@ -17,6 +17,7 @@ # under the License. import json +import warnings from collections import OrderedDict from typing import Callable, Dict, List, Optional @@ -48,9 +49,9 @@ class HiveStatsCollectionOperator(BaseOperator): :param extra_exprs: dict of expression to run against the table where keys are metric names and values are Presto compatible expressions :type extra_exprs: dict - :param col_blacklist: list of columns to blacklist, consider - blacklisting blobs, large json columns, ... - :type col_blacklist: list + :param excluded_columns: list of columns to exclude, consider + excluding blobs, large json columns, ... + :type excluded_columns: list :param assignment_func: a function that receives a column name and a type, and returns a dict of metric names and an Presto expressions. If None is returned, the global defaults are applied. If an @@ -67,17 +68,26 @@ def __init__(self, table: str, partition: str, extra_exprs: Optional[Dict] = None, - col_blacklist: Optional[List] = None, + excluded_columns: Optional[List] = None, assignment_func: Optional[Callable[[str, str], Optional[Dict]]] = None, metastore_conn_id: str = 'metastore_default', presto_conn_id: str = 'presto_default', mysql_conn_id: str = 'airflow_db', *args, **kwargs) -> None: + if 'col_blacklist' in kwargs: + warnings.warn( + 'col_blacklist kwarg passed to {c} (task_id: {t}) is deprecated, please rename it to ' + 'excluded_columns instead'.format( + c=self.__class__.__name__, t=kwargs.get('task_id')), + category=FutureWarning, + stacklevel=2 + ) + excluded_columns = kwargs.pop('col_blacklist') super().__init__(*args, **kwargs) self.table = table self.partition = partition self.extra_exprs = extra_exprs or {} - self.col_blacklist = col_blacklist or [] # type: List + self.excluded_columns = excluded_columns or [] # type: List self.metastore_conn_id = metastore_conn_id self.presto_conn_id = presto_conn_id self.mysql_conn_id = mysql_conn_id @@ -89,7 +99,7 @@ def get_default_exprs(self, col, col_type): """ Get default expressions """ - if col in self.col_blacklist: + if col in self.excluded_columns: return {} exp = {(col, 'non_null'): f"COUNT({col})"} if col_type in ['double', 'int', 'bigint', 'float']: diff --git a/pylintrc b/pylintrc index cfeef0d889f64..f0275ed2a0c44 100644 --- a/pylintrc +++ b/pylintrc @@ -20,11 +20,11 @@ # run arbitrary code. extension-pkg-whitelist=setproctitle -# Add files or directories to the blacklist. They should be base names, not +# Add files or directories to the ignore list. They should be base names, not # paths. ignore=CVS -# Add files or directories matching the regex patterns to the blacklist. The +# Add files or directories matching the regex patterns to the ignore list. The # regex matches against base names, not paths. ignore-patterns= diff --git a/tests/providers/apache/cassandra/hooks/test_cassandra.py b/tests/providers/apache/cassandra/hooks/test_cassandra.py index e0192c801e3f4..a519c67d5156a 100644 --- a/tests/providers/apache/cassandra/hooks/test_cassandra.py +++ b/tests/providers/apache/cassandra/hooks/test_cassandra.py @@ -113,7 +113,7 @@ def test_get_lb_policy_invalid_policy(self): TokenAwarePolicy, expected_child_policy_type=RoundRobinPolicy) - def test_get_lb_policy_no_host_for_white_list(self): + def test_get_lb_policy_no_host_for_allow_list(self): # test host not specified for WhiteListRoundRobinPolicy should throw exception self._assert_get_lb_policy('WhiteListRoundRobinPolicy', {}, diff --git a/tests/providers/apache/hive/operators/test_hive_stats.py b/tests/providers/apache/hive/operators/test_hive_stats.py index f2e1e93227d41..5c2b33717bbe8 100644 --- a/tests/providers/apache/hive/operators/test_hive_stats.py +++ b/tests/providers/apache/hive/operators/test_hive_stats.py @@ -59,9 +59,9 @@ def test_get_default_exprs(self): (col, 'non_null'): 'COUNT({})'.format(col) }) - def test_get_default_exprs_blacklist(self): - col = 'blacklisted_col' - self.kwargs.update(dict(col_blacklist=[col])) + def test_get_default_exprs_excluded_cols(self): + col = 'excluded_col' + self.kwargs.update(dict(excluded_columns=[col])) default_exprs = HiveStatsCollectionOperator(**self.kwargs).get_default_exprs(col, None) diff --git a/tests/test_stats.py b/tests/test_stats.py index c226a951e0297..57e2f6ba230da 100644 --- a/tests/test_stats.py +++ b/tests/test_stats.py @@ -81,7 +81,7 @@ def test_stat_name_must_not_exceed_max_length(self): self.stats.incr('X' * 300) self.statsd_client.assert_not_called() - def test_stat_name_must_only_include_whitelisted_characters(self): + def test_stat_name_must_only_include_allowed_characters(self): self.stats.incr('test/$tats') self.statsd_client.assert_not_called() @@ -159,7 +159,7 @@ def test_stat_name_must_not_exceed_max_length_with_dogstatsd(self): self.dogstatsd.incr('X' * 300) self.dogstatsd_client.assert_not_called() - def test_stat_name_must_only_include_whitelisted_characters_with_dogstatsd(self): + def test_stat_name_must_only_include_allowed_characters_with_dogstatsd(self): self.dogstatsd.incr('test/$tats') self.dogstatsd_client.assert_not_called() diff --git a/tests/utils/test_dag_processing.py b/tests/utils/test_dag_processing.py index fc7c274dd74be..cffc25bfbef77 100644 --- a/tests/utils/test_dag_processing.py +++ b/tests/utils/test_dag_processing.py @@ -51,8 +51,8 @@ class FakeDagFileProcessorRunner(DagFileProcessorProcess): # This fake processor will return the zombies it received in constructor # as its processing result w/o actually parsing anything. - def __init__(self, file_path, pickle_dags, dag_id_white_list, zombies): - super().__init__(file_path, pickle_dags, dag_id_white_list, zombies) + def __init__(self, file_path, pickle_dags, dag_ids, zombies): + super().__init__(file_path, pickle_dags, dag_ids, zombies) # We need a "real" selectable handle for waitable_handle to work readable, writable = multiprocessing.Pipe(duplex=False) writable.send('abc') From 07e11a049a55a8e85e9d5d52055cdd70d018c32c Mon Sep 17 00:00:00 2001 From: Ash Berlin-Taylor Date: Sun, 7 Jun 2020 22:39:47 +0100 Subject: [PATCH 2/3] fixup! Don't use the term "whitelist" - language matters --- UPDATING.md | 11 ++++------- airflow/contrib/plugins/metastore_browser/main.py | 12 ++++++------ 2 files changed, 10 insertions(+), 13 deletions(-) diff --git a/UPDATING.md b/UPDATING.md index 07662fae9b0ed..18daca5c87ed4 100644 --- a/UPDATING.md +++ b/UPDATING.md @@ -2293,14 +2293,11 @@ dags_are_paused_at_creation = False If you specify a hive conf to the run_cli command of the HiveHook, Airflow add some convenience variables to the config. In case you run a secure Hadoop setup it might be -required to allow these variables by adding the following to your configuration: +required to allow these variables by adjusting you hive configuration to add `airflow\.ctx\..*` to the regex +of user-editable configuration properties. See +[the Hive docs on Configuration Properties][hive.security.authorization.sqlstd] for more info. -``` - - hive.security.authorization.sqlstd.confwhitelist.append - airflow\.ctx\..* - -``` +[hive.security.authorization.sqlstd]: https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=82903061#ConfigurationProperties-SQLStandardBasedAuthorization.1 ### Google Cloud Operator and Hook alignment diff --git a/airflow/contrib/plugins/metastore_browser/main.py b/airflow/contrib/plugins/metastore_browser/main.py index 1defa0e126007..b134cf94f902c 100644 --- a/airflow/contrib/plugins/metastore_browser/main.py +++ b/airflow/contrib/plugins/metastore_browser/main.py @@ -37,8 +37,8 @@ PRESTO_CONN_ID = 'presto_default' HIVE_CLI_CONN_ID = 'hive_default' DEFAULT_DB = 'default' -DB_WHITELIST = [] # type: List[str] -DB_BLACKLIST = ['tmp'] # type: List[str] +DB_ALLOW_LIST = [] # type: List[str] +DB_DENY_LIST = ['tmp'] # type: List[str] TABLE_SELECTOR_LIMIT = 2000 # Keeping pandas from truncating long strings @@ -139,11 +139,11 @@ def objects(self): Retrieve objects from TBLS and DBS """ where_clause = '' - if DB_WHITELIST: - dbs = ",".join(["'" + db + "'" for db in DB_WHITELIST]) + if DB_ALLOW_LIST: + dbs = ",".join(["'" + db + "'" for db in DB_ALLOW_LIST]) where_clause = "AND b.name IN ({})".format(dbs) - if DB_BLACKLIST: - dbs = ",".join(["'" + db + "'" for db in DB_BLACKLIST]) + if DB_DENY_LIST: + dbs = ",".join(["'" + db + "'" for db in DB_DENY_LIST]) where_clause = "AND b.name NOT IN ({})".format(dbs) sql = """ SELECT CONCAT(b.NAME, '.', a.TBL_NAME), TBL_TYPE From bd9d1d82974c177eb95be1d5d32d039d8fcfbe36 Mon Sep 17 00:00:00 2001 From: Jarek Potiuk Date: Mon, 8 Jun 2020 06:25:03 +0200 Subject: [PATCH 3/3] Automate language-matters check --- .pre-commit-config.yaml | 10 ++++++++++ BREEZE.rst | 20 +++++++++---------- STATIC_CODE_CHECKS.rst | 2 ++ .../default_webserver_config.py | 1 - airflow/jobs/scheduler_job.py | 1 + breeze-complete | 1 + pylintrc | 5 ----- 7 files changed, 24 insertions(+), 16 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 26189a7e10e8d..399fff7fa272c 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -246,6 +246,16 @@ metastore_browser/templates/.*\\.html$|.*\\.jinja2" entry: "pydevd.*settrace\\(" pass_filenames: true files: \.py$ + - id: language-matters + language: pygrep + name: Check for language that we do not accept as community + entry: "(?i)(black|white)[_-]?list" + pass_filenames: true + exclude: > + (?x) + ^airflow/providers/apache/cassandra/hooks/cassandra.py$| + ^airflow/providers/apache/hive/operators/hive_stats.py$| + ^tests/providers/apache/cassandra/hooks/test_cassandra.py - id: consistent-pylint language: pygrep name: Check for inconsitent pylint disable/enable without space diff --git a/BREEZE.rst b/BREEZE.rst index aee3f4c00fa8f..73eca9aade08c 100644 --- a/BREEZE.rst +++ b/BREEZE.rst @@ -1392,11 +1392,11 @@ This is the current syntax for `./breeze <./breeze>`_: check-hooks-apply check-integrations check-merge-conflict check-xml consistent-pylint daysago-import-check debug-statements detect-private-key doctoc end-of-file-fixer fix-encoding-pragma flake8 forbid-tabs - incorrect-use-of-LoggingMixin insert-license isort lint-dockerfile mixed-line-ending - mypy provide-create-sessions pydevd pylint pylint-tests python-no-log-warn - rst-backticks setup-order shellcheck stylelint trailing-whitespace - update-breeze-file update-extras update-local-yml-file update-setup-cfg-file - yamllint + incorrect-use-of-LoggingMixin insert-license isort language-matters lint-dockerfile + mixed-line-ending mypy provide-create-sessions pydevd pylint pylint-tests + python-no-log-warn rst-backticks setup-order shellcheck stylelint + trailing-whitespace update-breeze-file update-extras update-local-yml-file + update-setup-cfg-file yamllint You can pass extra arguments including options to to the pre-commit framework as passed after --. For example: @@ -1425,11 +1425,11 @@ This is the current syntax for `./breeze <./breeze>`_: check-hooks-apply check-integrations check-merge-conflict check-xml consistent-pylint daysago-import-check debug-statements detect-private-key doctoc end-of-file-fixer fix-encoding-pragma flake8 forbid-tabs - incorrect-use-of-LoggingMixin insert-license isort lint-dockerfile mixed-line-ending - mypy provide-create-sessions pydevd pylint pylint-tests python-no-log-warn - rst-backticks setup-order shellcheck stylelint trailing-whitespace - update-breeze-file update-extras update-local-yml-file update-setup-cfg-file - yamllint + incorrect-use-of-LoggingMixin insert-license isort language-matters lint-dockerfile + mixed-line-ending mypy provide-create-sessions pydevd pylint pylint-tests + python-no-log-warn rst-backticks setup-order shellcheck stylelint + trailing-whitespace update-breeze-file update-extras update-local-yml-file + update-setup-cfg-file yamllint You can pass extra arguments including options to the pre-commit framework as passed after --. For example: diff --git a/STATIC_CODE_CHECKS.rst b/STATIC_CODE_CHECKS.rst index 980f521c3b791..3d809b781c8c7 100644 --- a/STATIC_CODE_CHECKS.rst +++ b/STATIC_CODE_CHECKS.rst @@ -78,6 +78,8 @@ require Breeze Docker images to be installed locally: ----------------------------------- ---------------------------------------------------------------- ------------ ``isort`` Sorts imports in python files. ----------------------------------- ---------------------------------------------------------------- ------------ +``language-matters`` Check for language that we do not accept as community +----------------------------------- ---------------------------------------------------------------- ------------ ``lint-dockerfile`` Lints a dockerfile. ----------------------------------- ---------------------------------------------------------------- ------------ ``mixed-line-ending`` Detects if mixed line ending is used (\r vs. \r\n). diff --git a/airflow/config_templates/default_webserver_config.py b/airflow/config_templates/default_webserver_config.py index b2401f6c317d5..661b230c520f9 100644 --- a/airflow/config_templates/default_webserver_config.py +++ b/airflow/config_templates/default_webserver_config.py @@ -67,7 +67,6 @@ # Google OAuth example: # OAUTH_PROVIDERS = [{ # 'name':'google', -# 'whitelist': ['@YOU_COMPANY_DOMAIN'], # optional # 'token_key':'access_token', # 'icon':'fa-google', # 'remote_app': { diff --git a/airflow/jobs/scheduler_job.py b/airflow/jobs/scheduler_job.py index b5d5549ee4d0c..60e7d98982147 100644 --- a/airflow/jobs/scheduler_job.py +++ b/airflow/jobs/scheduler_job.py @@ -1,3 +1,4 @@ +# pylint: disable=no-name-in-module # # Licensed to the Apache Software Foundation (ASF) under one # or more contributor license agreements. See the NOTICE file diff --git a/breeze-complete b/breeze-complete index 9238f4fd5d490..8480aa55f2eb0 100644 --- a/breeze-complete +++ b/breeze-complete @@ -67,6 +67,7 @@ forbid-tabs incorrect-use-of-LoggingMixin insert-license isort +language-matters lint-dockerfile mixed-line-ending mypy diff --git a/pylintrc b/pylintrc index f0275ed2a0c44..938c4fdd34d74 100644 --- a/pylintrc +++ b/pylintrc @@ -15,11 +15,6 @@ [MASTER] -# A comma-separated list of package or module names from where C extensions may -# be loaded. Extensions are loading into the active Python interpreter and may -# run arbitrary code. -extension-pkg-whitelist=setproctitle - # Add files or directories to the ignore list. They should be base names, not # paths. ignore=CVS