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-8732: Add log-file to Vault Agent #17841

Merged
merged 25 commits into from
Nov 11, 2022
Merged

Conversation

peteski22
Copy link
Contributor

@peteski22 peteski22 commented Nov 7, 2022

This pull request will add support for Vault Agent to write log files to a specified path, it can be supplied using:

  • HCL config value (log_file)
  • Environment variable (VAULT_LOG_FILE)
  • CLI flag (-log-file)

The order of precedence follows that of other Vault parameters, CLI > Environment variable > HCL config.

The change was modelled heavily on Consul's approach to logging, and we have employed Consul changes to Vault (in a stripped down format) to support logging to files, in order to support logging from Vault Agent when running as a Windows Service.

Documentation has been updated, but suggest that it's reviewed for style guidance.

Some manual testing to be carried out in a Windows environment before this PR is marked 'ready for review'.

Related issues: #15393

changelog/17841.txt Outdated Show resolved Hide resolved
Name: flagNameLogFile,
Target: &c.flagLogFile,
EnvVar: EnvVaultLogFile,
Usage: "Path to the log file that Vault should use for logging",
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's more than this though right? It can be a log dir, or a prefix: https://developer.hashicorp.com/consul/docs/agent/config/cli-flags#_log_file

Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't we add the other log options (like the rotation options) as well as flags? Or alternatively, since unlike consul we have logs of config without associated flags, should we maybe not add the -log-file CLI option?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In Consul yes, as Consul does rotation of logs etc. so it fiddles with the name a lot more. In Vault I took the simple approach so it's no frills.

helper/logging/logfile_test.go Outdated Show resolved Hide resolved
helper/logging/logger_test.go Outdated Show resolved Hide resolved
helper/logging/logfile.go Outdated Show resolved Hide resolved
@peteski22 peteski22 marked this pull request as ready for review November 10, 2022 13:39
@peteski22 peteski22 requested a review from a team November 10, 2022 14:01
@@ -2,10 +2,10 @@
layout: docs
page_title: Commands (CLI)
description: |-
In addition to a verbose HTTP API, Vault features a command-line interface
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a lot of (potentially unnecessary?) changes - are all of these necessary? I'm wondering if it's possible to revert the (probably automatically done by your editor) changes and keep just the ones you've added. If nothing else, we should ensure that the formatting changes don't change the contents at all!

Copy link
Contributor

Choose a reason for hiding this comment

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

(It also makes it hard to see only the bits you've changed, so it would help for my review, too!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the catch @VioletHynes, I'm not sure why my IDE went wild with that. :(

Copy link
Contributor

@VioletHynes VioletHynes left a comment

Choose a reason for hiding this comment

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

Looks good! Approved. I'm finding it hard to review the docs changes with the formatting changes added (I assume automatically by your editor), but the bits I've seen have been good.

Nice work!

Edit: Actually, the vercel deployment failed here: https://vercel.com/hashicorp/vault/4HArMN49mEzH9tMzGVfhzu1H2e3F

We should make sure to re-deploy then ensure the docs look good before merging.

go.mod Outdated Show resolved Hide resolved
command/commands.go Show resolved Hide resolved
command/commands.go Show resolved Hide resolved
@peteski22 peteski22 enabled auto-merge (squash) November 11, 2022 10:41
@peteski22 peteski22 merged commit 7ae65df into main Nov 11, 2022
@peteski22 peteski22 deleted the VAULT-8732-log-file-parameter branch November 11, 2022 11:13
jayant07-yb pushed a commit to jayant07-yb/hashicorp-vault-integrations that referenced this pull request Mar 15, 2023
* Started work on adding log-file support to Agent
* Allow log file to be picked up and appended
* Use NewLogFile everywhere
* Tried to pull out the config aggregation from Agent.Run

Co-authored-by: Nick Cabatoff <ncabatoff@hashicorp.com>
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.

None yet

4 participants