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

stdout support for file backend via logger #3235

Merged
merged 4 commits into from
Aug 29, 2017
Merged

stdout support for file backend via logger #3235

merged 4 commits into from
Aug 29, 2017

Conversation

tallpauley
Copy link
Contributor

@tallpauley tallpauley commented Aug 24, 2017

Here is a working request based on @jefferai's input on #2195. For this one I just took the file backend and forked behavior if file_path is STDOUT.

What I did:

  • passed the logger into the audit backend
  • made a special log level LevelRaw (-2) that will dynamically select formatRaw.
  • fork audit file backend when file_path == "STDOUT", I first tried - like @jefferai suggested, but this behaved weirdly in the CLI-- even in quotes I think it must get interpreted as a flag, wasn't sure how to escape it properly. STDOUT is easier-- no escaping

Questions:

  • Should it be it's own backend?
  • Is there any better way to do raw logging? It seems like it would be cleaner to make a special logger just for raw logging, but then the semaphore wouldn't be shared w/ the main logger
  • Can my amateurish go code be improved anywhere?
  • Is -2 an ok log-level? Since you have to mount it as an audit backend, I think it shouldn't ever be silenced by less verbose log-levels.

Still to be done:

  • I need to update the docs, just figured I'd wait to get feedback first

@tallpauley
Copy link
Contributor Author

I think it might be actually a lot cleaner to just make it a separate audit backend, since it uses almost none of the file backend. What do you think @jefferai?

audit/audit.go Outdated

// auditing to stdout (file audit backend) requires the logger
// to prevent interleaving w/ the main log output
Logger log.Logger
Copy link
Member

Choose a reason for hiding this comment

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

Logging goes to stderr after startup, not stdout, so this isn't necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jefferai I'm confused then by your comments on #2195. What were you referring to when you said "I worry about interleaving issues here given that Vault also writes things to stdout at various points."? I guess I wrongly assumed you were concerned about interleaving between a stdout audit backend and vault's main logging.

Copy link
Member

Choose a reason for hiding this comment

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

When Vault starts it dumps a bunch of output to stdout. Then the server logs go to stderr. However, making stdout a separate audit backend doesn't make any difference. You'd basically just need to ignore any line that isn't JSON.

@@ -15,6 +15,10 @@ import (
const (
styledefault = iota
stylejson
// Setting a level for raw log output. While it's not technically a level, we need the ability
Copy link
Member

Choose a reason for hiding this comment

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

You don't need any of this -- just use os.Stdout as the writer (https://golang.org/pkg/os/#pkg-variables)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, that's what I did initially, but then I was reading your comments on #2195 and thought that output from the audit-to-stdout and the primary vault logger would need to be synchronized (thinking vault logged to stdout). What needs to be changed for in #2195 for it to be merged?

Copy link
Member

Choose a reason for hiding this comment

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

2195 will never be merged because it's a separate backend.

@tallpauley
Copy link
Contributor Author

tallpauley commented Aug 28, 2017

@jefferai Ok, made it super simple, can you take another look? Reused fileLock to prevent interleaving between request and response logs.

@jefferai
Copy link
Member

Feels like this should be sanitized to lower case since a lot of people might assume it should be valid.

@jefferai
Copy link
Member

Other than that, looking good!

@tallpauley
Copy link
Contributor Author

@jefferai Ok, lowercased!

@jefferai
Copy link
Member

Same problem, in reverse. To be clear I meant using a sanitizing function, like strings.ToLower.

@tallpauley
Copy link
Contributor Author

tallpauley commented Aug 28, 2017

@jefferai Got it, I should have understood by the fact you used the word sanitized. It is now sanitized! I kept the docs to lowercase, feels more natural. Technically this is a breaking change for anyone who was outputting to a file named stdout. I think it should be reported as a breaking change even though outputting to a file named stdout is questionable.

@jefferai
Copy link
Member

I wouldn't worry about it, we don't have defined behavior on relative paths currently. People should be using absolute paths!

@jefferai
Copy link
Member

Looks good!

@jefferai jefferai merged commit f2d452b into hashicorp:master Aug 29, 2017
@jefferai
Copy link
Member

Merged, thanks!

chrishoffman pushed a commit that referenced this pull request Aug 30, 2017
* oss/master:
  changelog++
  add support to use application default credentials to gcs storage backend (#3257)
  Remove fake news about custom plugins
  Fix travis build on go 1.9
  changelog++
  stdout support for file backend via logger (#3235)
  fix swallowed errors in pki package tests (#3215)
  Fix API/AUTH/AppRole doc issue concerning bound_cidr_list (#3205)
@lyndon160
Copy link

This is great.

Is this documented anywhere in the vault documentation? It feel like we need a note on this page https://www.vaultproject.io/docs/audit/index.html

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.

3 participants