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

AIRFLOW-45: Support Hidden Airflow Variables #1530

Merged
merged 1 commit into from
May 25, 2016

Conversation

mattuuh7
Copy link

Dear Airflow Maintainers,

Please accept this PR that addresses the following issues:

Reminders for contributors:

@landscape-bot
Copy link

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

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

[admin]
hide_confidential_ui_fields = true
Copy link
Contributor

Choose a reason for hiding this comment

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

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

 # statsd_on =  False

Copy link
Contributor

@r39132 r39132 May 20, 2016

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@criccomini criccomini May 20, 2016

Choose a reason for hiding this comment

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

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

@landscape-bot
Copy link

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

@criccomini
Copy link
Contributor

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
Copy link
Contributor

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

@criccomini
Copy link
Contributor

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.

},
'admin': {
'hide_confidential_ui_fields': True,
},
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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')

Copy link
Author

@mattuuh7 mattuuh7 May 20, 2016

Choose a reason for hiding this comment

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

@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

screen shot 2016-05-20 at 3 57 19 pm

@landscape-bot
Copy link

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

@r39132
Copy link
Contributor

r39132 commented May 20, 2016

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
Copy link
Contributor

@mattuuh7, this is looking good.

  1. 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. :()
  2. Can you rename hide_encrypted_ui_fields to hide_sensitive_variable_fields?
  3. Could you add some docs either to the .rst or to the config comments saying which variables get masked (password, secret, etc).
  4. Could you squash your commit, and rename both the commit message and title of the PR as: 'AIRFLOW-45: Support Hidden Airflow Variables'

@mattuuh7 mattuuh7 changed the title Support Hidden Airflow Variables AIRFLOW-45: Support Hidden Airflow Variables May 24, 2016
@landscape-bot
Copy link

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

@criccomini
Copy link
Contributor

criccomini commented May 25, 2016

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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

AIRFLOW-45: Support Hidden Airflow Variables

@mattuuh7
Copy link
Author

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

@landscape-bot
Copy link

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

@asfgit asfgit merged commit 3e30941 into apache:master May 25, 2016
asfgit pushed a commit that referenced this pull request May 25, 2016
@mattuuh7
Copy link
Author

thanks @criccomini

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.

5 participants