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

provider: Enable request/response logging #212

Merged
merged 2 commits into from
Feb 23, 2019

Conversation

radeksimko
Copy link
Contributor

This makes debugging for developers & users of this provider easier by exposing all requests & responses in the log (when >DEBUG severity is set).

I didn't have chance to explore the api_client_logging option on the provider, but it became a common convention in other major providers to just respect logging level of Terraform (typically controlled via TF_LOG) and leverage the global logging settings, which also allows us to respect TF_LOG_PATH and send all these useful logs somewhere safe, instead of flooding stderr.

Keeping providers aligned may help users going forward and reduce confusion as we can document debugging/troubleshooting guides in one place for all providers and just point people there.

Whether you decide to keep that option in the provider block or deprecate it is ultimately up to you though. I don't want you to feel like I'm pressing you into deprecating it. 😉

Example:

2019/02/22 17:24:29 [DEBUG] CloudFlare API Request Details:
---[ REQUEST ]---------------------------------------
GET /client/v4/zones/25afd8e9b39af234c001b657a2eb2c5c/pagerules/81932b0afeb25157322318a4398a1d91 HTTP/1.1
Host: api.cloudflare.com
User-Agent: Terraform/0.11.8 terraform-provider-cloudflare/dev
Content-Type: application/json
X-Auth-Email: <redacted>
X-Auth-Key: <redacted>
Accept-Encoding: gzip


-----------------------------------------------------
2019/02/22 17:24:29 [DEBUG] CloudFlare API Response Details:
---[ RESPONSE ]--------------------------------------
HTTP/2.0 200 OK
Cache-Control: no-store, no-cache, must-revalidate, post-check=0, pre-check=0
Cf-Ray: 4ad325865d64c99f-SEA
Content-Type: application/json
Date: Fri, 22 Feb 2019 17:24:29 GMT
Expect-Ct: max-age=604800, report-uri="https://report-uri.cloudflare.com/cdn-cgi/beacon/expect-ct"
Expires: Sun, 25 Jan 1981 05:00:00 GMT
Pragma: no-cache
Served-In-Seconds: 0.076
Server: cloudflare
Set-Cookie: __cfduid=dd589b51e35be510cb67896a359ac31f21550856269; expires=Sat, 22-Feb-20 17:24:29 GMT; path=/; domain=.cloudflare.com; HttpOnly
Set-Cookie: __cf_bm=eaa413494bbc211b6e125bec5eef183a064db294-1550856269-1800-AdKEAnqZ7d6UhqYeMM0SFMqvu+72epIWfqlSPkhTtECmqiANMO/hDxe9Ez58RP/wD6tXNYuyhQwC4/x2WN7CmJc=; path=/; expires=Fri, 22-Feb-19 17:54:29 GMT; domain=.cloudflare.com; HttpOnly
Strict-Transport-Security: max-age=15780000; includeSubDomains
Vary: Accept-Encoding
X-Content-Type-Options: nosniff
X-Frame-Options: SAMEORIGIN
X-Ssl-Protocol: TLSv1.2

{
 "result": {
  "id": "81932b0afeb25157322318a4398a1d91",
  "targets": [
   {
    "target": "url",
    "constraint": {
     "operator": "matches",
     "value": "test-basic.hashicorptest.com\/"
    }
   }
  ],
  "actions": [
   {
    "id": "ssl",
    "value": "flexible"
   },
   {
    "id": "always_online",
    "value": "on"
   },
   {
    "id": "disable_apps"
   }
  ],
  "priority": 1,
  "status": "active",
  "created_on": "2018-03-13T04:18:44.000000Z",
  "modified_on": "2019-02-22T17:24:29.000000Z"
 },
 "success": true,
 "errors": [],
 "messages": []
}
-----------------------------------------------------

@radeksimko radeksimko added the kind/enhancement Categorizes issue or PR as related to improving an existing feature. label Feb 22, 2019
@ghost ghost added the size/XS label Feb 22, 2019
@jacobbednarz
Copy link
Member

Big 💯 from me on this one. I've actually got a combination of something similar to this that I've hacked into the provider when building a dev version. 😛

My only question here is whether we should attempt to scrub some known sensitive data (such as the API key credential) that will show up in the output. Maybe replace everything except the first and last 4 characters of the key with asterisks? It will end up looking like a1b2**********c3d4 so you can still diagnose if it's the wrong key but not enough to do anything else with. Another option would be to replace the whole string with asterisks but then it may be harder to debug if you'd want to check which key is being picked up (still possible with something like a MITM proxy, just harder).

Thoughts?

@radeksimko
Copy link
Contributor Author

radeksimko commented Feb 22, 2019

@jacobbednarz I understand your concerns, but as I mentioned above the debug log is only available to those who are capable of enabling the debug mode, which are in all (?) cases the same people who provide these (sensitive) credentials through configs or ENV variables in the first place.

We generally expect users to treat debug logs with at least the same care as state files: https://www.terraform.io/docs/state/sensitive-data.html

We had some early discussions about scrubbing sensitive data internally (also in the context of state) and might introduce a solution for that at some point in the future.

@radeksimko
Copy link
Contributor Author

That said I guess we could explicitly mention this expectation in https://www.terraform.io/docs/internals/debugging.html

@jacobbednarz
Copy link
Member

Totally agree with your points and if it's something that has already been considered and we're happy proceeding, 👍 with me.

@jacobbednarz jacobbednarz merged commit 794247a into master Feb 23, 2019
@jacobbednarz jacobbednarz deleted the provider-req-resp-logging branch February 23, 2019 02:09
@jacobbednarz
Copy link
Member

Thanks again for this @radeksimko! 🤘

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement Categorizes issue or PR as related to improving an existing feature. size/XS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants