AIRFLOW-45: Support Hidden Airflow Variables #1530

mattuuh7 · 2016-05-20T17:07:15Z

Dear Airflow Maintainers,

Please accept this PR that addresses the following issues:

https://issues.apache.org/jira/browse/AIRFLOW-45

Reminders for contributors:

Your PR's title must reference an issue on Airflow's JIRA. For example, a PR called "[AIRFLOW-1] My Amazing PR" would close JIRA issue Improving the search functionality in the graph view #1. Please open a new issue if required!
You must add an Apache License header to all new files.
Please squash your commits when possible and follow the 7 rules of good Git commits.

landscape-bot · 2016-05-20T19:40:27Z

Repository health decreased by 0.07% when pulling 6392cbd on mattuuh7:hidden-fields into 8bf335b on apache:master.

9 new problems were found (including 0 errors and 4 code smells).
16 problems were fixed (including 0 errors and 5 code smells).

criccomini · 2016-05-20T21:09:46Z

airflow/configuration.py

@@ -372,6 +375,9 @@ def run_command(command):
 # default_principal = admin
 # default_secret = admin

+[admin]
+hide_confidential_ui_fields = true


Should be True just to be more Python-esque, e.g.

# statsd_on = False

So, why True as default and not False as default? False preserves backward compatibility with respect to UI experience.

The issue is if we default to false, then passwords are visible by default.

landscape-bot · 2016-05-20T22:04:07Z

Repository health decreased by 0.05% when pulling fdc0c18 on mattuuh7:hidden-fields into 8bf335b on apache:master.

6 new problems were found (including 0 errors and 4 code smells).
16 problems were fixed (including 0 errors and 5 code smells).

criccomini · 2016-05-20T22:28:40Z

Wondering whether we want to include extras in the filtering. It's annoying, and I don't think that there is any secret stuff in there for existing connections. Thoughts?

criccomini · 2016-05-20T22:29:09Z

Note: installed locally, and this did appear to work.

criccomini · 2016-05-20T22:30:52Z

Actually, I take that back. It worked for the hooks. For variables, it only works for very specific fields:

    test    test    
    password    ********    
    secret_password test

I would have expected either all to be encrypted, or anything with 'password'/'secret' in it.

r39132 · 2016-05-20T22:45:38Z

airflow/configuration.py

+    },
+    'admin': {
+        'hide_confidential_ui_fields': True,
+    },
 }


Make sure you have specified the config value everywhere it is needed. I recall there are at least 3 places where new config needs to be specified.

For example, look at dags_are_paused_at_creation :

(venv) Sid-As-MacBook-Pro:airflow siddharth$ grep -ri dags_are_paused_at_creation . | grep -v Binary | grep -v static ./configuration.py: 'dags_are_paused_at_creation': True, ./configuration.py:dags_are_paused_at_creation = True ./configuration.py:dags_are_paused_at_creation = False ./models.py: is_paused_at_creation = configuration.getboolean('core', 'dags_are_paused_at_creation')

@criccomini updated the logic, it should check if any of the key_name contains any words in the confidential_fields list that models after sentry's list

landscape-bot · 2016-05-20T22:57:43Z

Repository health decreased by 0.05% when pulling c0a2fee on mattuuh7:hidden-fields into 8bf335b on apache:master.

6 new problems were found (including 0 errors and 4 code smells).
16 problems were fixed (including 0 errors and 5 code smells).

r39132 · 2016-05-20T23:09:48Z

Thx for providing a screen shot.. generally a requirement for all UI-changing PRs, though you can attach the screen shot here or in the JIRA. Both work.

criccomini · 2016-05-21T21:56:36Z

@mattuuh7, this is looking good.

Can you remove extras from being masked? I think the cons outweigh the pros of masking it. (I know this is annoying, considering how much time you spent trying to figure out how to mask it. Sorry. :()
Can you rename hide_encrypted_ui_fields to hide_sensitive_variable_fields?
Could you add some docs either to the .rst or to the config comments saying which variables get masked (password, secret, etc).
Could you squash your commit, and rename both the commit message and title of the PR as: 'AIRFLOW-45: Support Hidden Airflow Variables'

landscape-bot · 2016-05-24T06:42:34Z

Repository health decreased by 0.12% when pulling 1363dde on mattuuh7:hidden-fields into ee24855 on apache:master.

5 new problems were found (including 0 errors and 3 code smells).
No problems were fixed.

criccomini · 2016-05-25T00:39:43Z

When running this off of master, I get:

  File "/Users/chrisr/Code/airflow/airflow/www/views.py", line 2038, in hidden_field_formatter
    return getattr(model, name)
  File "build/bdist.macosx-10.10-intel/egg/sqlalchemy/orm/attributes.py", line 293, in __get__
    return self.descriptor.__get__(instance, owner)
  File "/Users/chrisr/Code/airflow/airflow/models.py", line 3200, in get_val
    "Can't decrypt _val, configuration is missing")
AirflowException: Can't decrypt _val, configuration is missing

This can be reproduced by going to a fresh airflow install, running airflow initdb, airflow webserver, and opening the variables UI.

criccomini · 2016-05-25T03:11:52Z

Removing val from:

column_list = ('key', 'val', 'is_encrypted',)

Made the exception go away. The patch appears to work without it, but val isn't shown in the table view anymore.

criccomini · 2016-05-25T03:16:22Z

I've determined the issue was with some rows that somehow persisted even after I ran airflow initdb. Running airflow resetdb fixed the issue.

criccomini · 2016-05-25T03:18:24Z

Ready to merge. Could you change the commit message to:

AIRFLOW-45: Support Hidden Airflow Variables

mattuuh7 · 2016-05-25T15:46:26Z

@criccomini done with the change. please review again. thx

landscape-bot · 2016-05-25T15:48:58Z

Repository health decreased by 0.12% when pulling 3e30941 on mattuuh7:hidden-fields into 7332c40 on apache:master.

5 new problems were found (including 0 errors and 3 code smells).
No problems were fixed.

mattuuh7 · 2016-05-25T23:18:18Z

thanks @criccomini

criccomini reviewed May 20, 2016
View reviewed changes

r39132 reviewed May 20, 2016
View reviewed changes

mattuuh7 changed the title ~~Support Hidden Airflow Variables~~ AIRFLOW-45: Support Hidden Airflow Variables May 24, 2016

mattuuh7 force-pushed the hidden-fields branch from c0a2fee to 1363dde Compare May 24, 2016 06:38

AIRFLOW-45: Support Hidden Airflow Variables

3e30941

mattuuh7 force-pushed the hidden-fields branch from 1363dde to 3e30941 Compare May 25, 2016 15:46

asfgit merged commit 3e30941 into apache:master May 25, 2016

asfgit pushed a commit that referenced this pull request May 25, 2016

Merge pull request #1530 from mattuuh7/hidden-fields

456dada

kaxil mentioned this pull request Aug 14, 2018

[AIRFLOW-2899] Hide sensitive data when Exporting Variables #3748

Closed

6 tasks

marcusianlevine mentioned this pull request Aug 10, 2020

Hide sensitive data in UI #8421

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AIRFLOW-45: Support Hidden Airflow Variables #1530

AIRFLOW-45: Support Hidden Airflow Variables #1530

mattuuh7 commented May 20, 2016

landscape-bot commented May 20, 2016

criccomini May 20, 2016

r39132 May 20, 2016 •

edited

criccomini May 20, 2016 •

edited

landscape-bot commented May 20, 2016

criccomini commented May 20, 2016

criccomini commented May 20, 2016

criccomini commented May 20, 2016

r39132 May 20, 2016

mattuuh7 May 20, 2016 •

edited

landscape-bot commented May 20, 2016

r39132 commented May 20, 2016

criccomini commented May 21, 2016

landscape-bot commented May 24, 2016

criccomini commented May 25, 2016 •

edited

criccomini commented May 25, 2016

criccomini commented May 25, 2016

criccomini commented May 25, 2016

mattuuh7 commented May 25, 2016

landscape-bot commented May 25, 2016

mattuuh7 commented May 25, 2016

AIRFLOW-45: Support Hidden Airflow Variables #1530

AIRFLOW-45: Support Hidden Airflow Variables #1530

Conversation

mattuuh7 commented May 20, 2016

landscape-bot commented May 20, 2016

criccomini May 20, 2016

Choose a reason for hiding this comment

r39132 May 20, 2016 • edited

Choose a reason for hiding this comment

criccomini May 20, 2016 • edited

Choose a reason for hiding this comment

landscape-bot commented May 20, 2016

criccomini commented May 20, 2016

criccomini commented May 20, 2016

criccomini commented May 20, 2016

r39132 May 20, 2016

Choose a reason for hiding this comment

mattuuh7 May 20, 2016 • edited

Choose a reason for hiding this comment

landscape-bot commented May 20, 2016

r39132 commented May 20, 2016

criccomini commented May 21, 2016

landscape-bot commented May 24, 2016

criccomini commented May 25, 2016 • edited

criccomini commented May 25, 2016

criccomini commented May 25, 2016

criccomini commented May 25, 2016

mattuuh7 commented May 25, 2016

landscape-bot commented May 25, 2016

mattuuh7 commented May 25, 2016

r39132 May 20, 2016 •

edited

criccomini May 20, 2016 •

edited

mattuuh7 May 20, 2016 •

edited

criccomini commented May 25, 2016 •

edited