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

Make writing env to disk configurable. #147

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@mheath
Contributor

mheath commented Aug 18, 2014

We have strict security requirements that prevent us from writing clear text passwords to disk. Unfortunately the DEA does this when it write the environment out to logs/env.log. We would like to make the persisting of the environment configurable so that we can disable it.

@cfdreddbot

This comment has been minimized.

cfdreddbot commented Aug 18, 2014

Hey mheath!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you've already signed the CLA.

@cf-gitbot

This comment has been minimized.

Collaborator

cf-gitbot commented Aug 18, 2014

We have created an issue in Pivotal Tracker to manage this. You can view the current status of your issue at: https://www.pivotaltracker.com/story/show/77151178.

@mkocher

This comment has been minimized.

Contributor

mkocher commented Aug 18, 2014

I'm not sure this writing the env.log "feature" is even necessary. I wrote the code for it initially when we were adding buildpacks to cf and were troubleshooting environment issues. I never intended to ship it.

@mheath

This comment has been minimized.

Contributor

mheath commented Aug 18, 2014

lol. That's awesome @mkocher.

I've had a couple of different conversations with people trying to get logging of the environment removed. (We removed it in our DEA fork.) There was always push back to having it removed though. Unfortunately, there are a lot of people that find this useful. Now that we can show the environment from the command line though, perhaps there wouldn't be any push back to removing this now.

I would MUCH rather remove it rather than make it configurable.

I'll even submit another PR that removes the offending tests. Just let me know how to proceed.

@mkocher

This comment has been minimized.

Contributor

mkocher commented Aug 18, 2014

@jbayer @MarkKropf @dieucao Can we just remove env.log? I'm in favor.

@dieucao

This comment has been minimized.

dieucao commented Aug 18, 2014

@mkocher Sounds good. Let's get rid of it.
@mheath Would you do the PR to remove it?

@mheath

This comment has been minimized.

Contributor

mheath commented Aug 18, 2014

@dieucao I would be very happy to submit the PR to remove it!

@mheath mheath closed this Aug 18, 2014

@mheath mheath referenced this pull request Aug 18, 2014

Merged

Remove log/env.log #148

@jbayer

This comment has been minimized.

jbayer commented Aug 19, 2014

@dieucao especially since this is available via API now i think it's fine...one thing we would be missing is that it does show you the env variables at the time the container was started vs what they are now, which you don't really get with the straight API currently (i believe that is rectified in diego with execution contexts).

can you please give the vcap-dev mailing list a head's up on this one as i've seen this documented in places for troubleshooting. eg. step 1: get the contents of your env variables with cf logs APPNAME logs/env.log

@dieucao

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment