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

Add FOG_DEBUG env var and distinguish different providers #185

Merged
merged 1 commit into from
Jan 14, 2021

Conversation

WeiQuan0605
Copy link
Contributor

Thanks for contributing to the capi_release. To speed up the process of reviewing your pull request please provide us with:

  • A short explanation of the proposed change:
    Add FOG_DEBUG environment variable and add switch expression for different providers
  • An explanation of the use cases your change solves
    Add additional FOG_DEBUG environment variable for Azure blobstore(fog-azure-rm changes: add fog debug var fog/fog-azure-rm#431)
    Add switch expression to set different environment variables for different blobstore provider. Without this sensitive information could be leaked in the logs(i.g. when "DEBUG" is enabled on azure)

We also added a simple unit test for the bpm templating. Let us know if this can be done in a nicer way 🙃

  • I have viewed signed and have submitted the Contributor License Agreement

  • I have made this pull request to the develop branch

  • I have run CF Acceptance Tests on bosh lite

@cf-gitbot
Copy link

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/175393470

The labels on this github issue will be updated when the story is started.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 22, 2020

CLA Check
The committers are authorized under a signed CLA.

@stephanme
Copy link
Contributor

EXCON_DEBUG was removed because it leaks S3 credentials. As a consequence, there are no debug logs on AWS when setting log_fog_requests to true. Maybe this can be addressed with newer fog-aws versions based on the FOG_DEBUG value that was added to fog-core.

config["env"]["FOG_DEBUG"] = true
config["env"]["DEBUG"] = true
else
config["env"]["DEBUG"] = true
Copy link
Member

Choose a reason for hiding this comment

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

Is DEBUG redundant with the addition of the FOG_DEBUG variable? From looking at this commit, it seems like FOG_DEBUG doesn't actually introduce new debug information? Or does it depend on the specific blobstore provider gem?

Copy link
Contributor

Choose a reason for hiding this comment

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

Eventually, DEBUG will be redundant and can be removed. But FOG_DEBUG was introduced in fog-core just recently and is not yet released as far as I can see. Therefore I would stick with setting both FOG_DEBUG and DEBUG with the exception of Azure.
On Azure, setting DEBUG leads to full request/response logging including sensitive information and response bodies so we can't use DEBUG. FOG_DEBUG is handled in fog-azure-rm (branch fog-arm-cf) explicitly.

when "azurerm"
config["env"]["FOG_DEBUG"] = true
when "aliyun"
config["env"]["ALIYUN_OSS_SDK_LOG_LEVEL"] = "debug"
Copy link
Member

Choose a reason for hiding this comment

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

Can you provide where this env variable is documented? Not seeing anything about it in fog-aliyun.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The env var of fog-aliyun is not documented. We got it from:https://github.com/aliyun/aliyun-oss-ruby-sdk/blob/d0a02321bf53798525d17f1c53ed780c2fe7437b/lib/aliyun/common/logging.rb#L47
We've also tested it on our landscape, there were no problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants