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.

Sign up for GitHub

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

Jump to bottom

[flare] Remove proxy credentials from collected datadog.conf #1942

Merged
merged 2 commits into from
Nov 24, 2015
Merged

[flare] Remove proxy credentials from collected datadog.conf #1942

merged 2 commits into from
Nov 24, 2015

Conversation

olivielpeau
Copy link
Member

Also added a message in the output to make credentials removal from datadog.conf explicit. The message is similar to what is already displayed for the yaml files in conf.d, for instance:

2015-09-22 19:05:08,842 | INFO | dd.collector | utils.flare(flare.py:340) |   * /etc/dd-agent/datadog.conf - this file contains credentials which have been removed in the version collected: api_key, proxy credentials
2015-09-22 19:05:08,844 | INFO | dd.collector | utils.flare(flare.py:340) |   * /etc/dd-agent/supervisor.conf
2015-09-22 19:05:08,849 | INFO | dd.collector | utils.flare(flare.py:340) |   * /etc/dd-agent/conf.d/elastic.yaml - this file contains a password which has been removed in the version collected
2015-09-22 19:05:08,851 | INFO | dd.collector | utils.flare(flare.py:340) |   * /etc/dd-agent/conf.d/nginx.yaml

@olivielpeau olivielpeau added this to the 5.6.0 milestone Sep 22, 2015
@remh
Copy link

remh commented Oct 16, 2015

@JohnLZeller can you review this one ?

@remh remh modified the milestones: 5.7.0, 5.6.0 Nov 2, 2015
@remh
Copy link

remh commented Nov 2, 2015

This needs to be rebased

@olivielpeau olivielpeau force-pushed the olivielpeau/flare-rm-proxy-creds branch 2 times, most recently from 2045510 to e4a3a2f Compare November 10, 2015 22:22
@olivielpeau
Copy link
Member Author

I've rebased and refactored this PR, and also changed a little bit the output of flare when credentials are removed, it looks like this now:

2015-11-10 22:24:32,980 | INFO | dd.collector | utils.flare(flare.py:362) |   * /etc/dd-agent/datadog.conf - this file contains credentials (api_key, proxy credentials) which have been removed in the collected version
2015-11-10 22:24:32,981 | INFO | dd.collector | utils.flare(flare.py:362) |   * /etc/dd-agent/supervisor.conf
2015-11-10 22:24:32,983 | INFO | dd.collector | utils.flare(flare.py:362) |   * /etc/dd-agent/conf.d/nginx.yaml
2015-11-10 22:24:32,984 | INFO | dd.collector | utils.flare(flare.py:362) |   * /etc/dd-agent/conf.d/http_check.yaml
2015-11-10 22:24:32,986 | INFO | dd.collector | utils.flare(flare.py:362) |   * /etc/dd-agent/conf.d/sawtooth.yaml
2015-11-10 22:24:32,988 | INFO | dd.collector | utils.flare(flare.py:362) |   * /etc/dd-agent/conf.d/jmx.yaml - this file contains a credential (password) which has been removed in the collected version

@olivielpeau olivielpeau force-pushed the olivielpeau/flare-rm-proxy-creds branch from e4a3a2f to 5b14cb9 Compare November 17, 2015 18:37
JohnLZeller
JohnLZeller reviewed Nov 17, 2015
View reviewed changes
utils/flare.py
def _strip_password(self, file_path):
# Return path to a temp file without comments on which the credential patterns have been applied
def _strip_credentials(self, file_path, credential_patterns=None):
if not credential_patterns:
Copy link
Contributor

Choose a reason for hiding this comment

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

Could just use credential_patterns=[] in the function definition, instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

In Python I prefer avoiding using mutable objects as default parameter values because of this: http://effbot.org/zone/default-values.htm

It can be pretty annoying to debug this kind of behaviors...

Copy link
Contributor

Choose a reason for hiding this comment

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

Bah mutable, my mistake! Carry on...

@JohnLZeller
Copy link
Contributor

LGTM after those comments :) 👍

@JohnLZeller
Copy link
Contributor

Everything is g2g

@olivielpeau
Copy link
Member Author

Thanks @JohnLZeller! :)

olivielpeau added a commit that referenced this pull request Nov 24, 2015
@olivielpeau
Merge pull request #1942 from DataDog/olivielpeau/flare-rm-proxy-creds
78d53b3 
[flare] Remove proxy credentials from collected datadog.conf
@olivielpeau olivielpeau merged commit 78d53b3 into master Nov 24, 2015
@olivielpeau olivielpeau deleted the olivielpeau/flare-rm-proxy-creds branch November 24, 2015 17:00
@olivielpeau olivielpeau modified the milestones: 5.6.3, 5.7.0 Dec 4, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@JohnLZeller JohnLZeller

Labels
None yet
Projects
None yet
Milestone
5.6.3
Development

Successfully merging this pull request may close these issues.
3 participants
@olivielpeau @remh @JohnLZeller