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

VAULT-9427: Add read support to sys/loggers endpoints #17979

Merged
merged 11 commits into from
Nov 28, 2022

Conversation

ccapurso
Copy link
Contributor

@ccapurso ccapurso commented Nov 16, 2022

PR #16111 added support to modify server logging verbosity both globally and for individual loggers. It did not, however, add the ability to read the current state of the logger verbosity levels. This PR introduces read handlers for the sys/loggers and sys/loggers/:name endpoints.

Additional more low-level changes:

  • Modified Core.SetLogLevelByName to account for the potential of Core.allLoggers including loggers that share the same name
  • Modified existing sys/loggers handlers to use ParseLogLevel from the logging helper package to remove redundant code
  • Added checks to ensure sys/loggers endpoints do not handle base logger (name == "")
  • Added a TranslateLoggerLevel func to the logging helper package to determine the logging verbosity of an hclog.Logger

Example output on a dev server (vault server -dev):

# Set policy logger to trace level
❯ curl -XPUT -H "X-Vault-Token: $VAULT_TOKEN" -d '{"level":"trace"}' "$VAULT_ADDR/v1/sys/loggers/policy"

# Fetch verbosity level of all loggers
❯ curl -s -XGET -H "X-Vault-Token: $VAULT_TOKEN" "$VAULT_ADDR/v1/sys/loggers" | jq .data
{
  "activity": "info",
  "audit": "info",
  "auth.token.auth_token_cf840257": "info",
  "core": "info",
  "core.cluster-listener": "info",
  "core.cluster-listener.tcp": "info",
  "core.router": "info",
  "expiration": "info",
  "expiration.job-manager": "info",
  "grpclogfaker": "info",
  "identity": "info",
  "identity.storagepacker.entities": "info",
  "identity.storagepacker.groups": "info",
  "identity.storagepacker.local-aliases": "info",
  "mfa": "info",
  "policy": "trace",
  "quotas": "info",
  "rollback": "info",
  "seal.shamir": "info",
  "secrets.cubbyhole.cubbyhole_079cd5ab": "info",
  "secrets.identity.identity_7b1c0163": "info",
  "secrets.kv.kv_efb8658b": "info",
  "secrets.system.system_3ae59f37": "info",
  "storage.cache": "info",
  "storage.inmem": "info",
  "storage.sealunwrapper": "info",
  "system": "info",
  "token": "info"
}

# Get verbosity level of just the policy logger
❯ curl -s -XGET -H "X-Vault-Token: $VAULT_TOKEN" "$VAULT_ADDR/v1/sys/loggers/policy" | jq .data
{
  "policy": "trace"
}

# Revert verbosity level of just the policy logger to what is provided in config
❯ curl -s -XDELETE -H "X-Vault-Token: $VAULT_TOKEN" "$VAULT_ADDR/v1/sys/loggers/policy"

# Get verbosity level of just the policy logger
❯ curl -s -XGET -H "X-Vault-Token: $VAULT_TOKEN" "$VAULT_ADDR/v1/sys/loggers/policy" | jq .data
{
  "policy": "info"
}

@ccapurso ccapurso marked this pull request as ready for review November 21, 2022 15:03
Copy link
Contributor

@hghaf099 hghaf099 left a comment

Choose a reason for hiding this comment

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

Just a nit. Otherwise, looks good to me!

vault/core.go Show resolved Hide resolved
err = b.Core.SetLogLevelByName(nameRaw.(string), level)
if err != nil {
return logical.ErrorResponse(fmt.Sprintf("invalid params: %s", err.Error())), nil
name := nameRaw.(string)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Do we need to check if name is an empty string here the same as above functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we should. Thanks!

Copy link

@peteski22 peteski22 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for adding some of the godoc/comments etc. much appreciated.

@@ -112,6 +112,8 @@ func ParseLogFormat(format string) (LogFormat, error) {
}
}

// ParseLogLevel returns the hclog.Level that corresponds with the provided level string.
// This differs hclog.LevelFromString in that it supports additional level strings.
Copy link

@peteski22 peteski22 Nov 25, 2022

Choose a reason for hiding this comment

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

I think I took this code and refactored it into a function when adding the log file feature, so this is my bad and thanks for adding comments!

However, I think that the func should probably not include notice or warning as that hasn't (to my knowledge) been documented on our site.

There's also differing info depending on whether you look at the docs for the server command or Vault configuration.

Trace, Debug, Error, Warn, Info. vs trace, debug, info, warn, *err*.

My PR I have open at the moment to add rotation to log files has updates to the docs to make them all standardised, and I will also update ParseLogLevel to not handle notice, warning or "" which defaults to info.

ccapurso added a commit that referenced this pull request Nov 30, 2022
* add logger->log-level str func

* ensure SetLogLevelByName accounts for duplicates

* add read handlers for sys/loggers endpoints

* add changelog entry

* update docs

* ignore base logger

* fix docs formatting issue

* add ReadOperation support to TestSystemBackend_Loggers

* add more robust checks to TestSystemBackend_Loggers

* add more robust checks to TestSystemBackend_LoggersByName

* check for empty name in delete handler
ccapurso added a commit that referenced this pull request Dec 6, 2022
* add initial logging helper package

* VAULT-9427: Add read support to `sys/loggers` endpoints (#17979)

* add logger->log-level str func

* ensure SetLogLevelByName accounts for duplicates

* add read handlers for sys/loggers endpoints

* add changelog entry

* update docs

* ignore base logger

* fix docs formatting issue

* add ReadOperation support to TestSystemBackend_Loggers

* add more robust checks to TestSystemBackend_Loggers

* add more robust checks to TestSystemBackend_LoggersByName

* check for empty name in delete handler

* add logfile

* remove doc changes
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