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

Move audit logs to a dedicated logs directory #116562

Merged
merged 8 commits into from
Nov 10, 2021
Merged

Conversation

watson
Copy link
Contributor

@watson watson commented Oct 28, 2021

Depends on #116282

The only new commit in this PR is the last one (13e27d6)

This PR adds a new folder to the root of the Kibana directory: logs.

The reason behind this change is to align more closely with where Elasticsearch stores logs. This was previously discussed here: #82578 (comment)

The new logs directory will currently only be used to store audit logs.

@watson watson added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting labels Oct 28, 2021
@watson watson self-assigned this Oct 28, 2021
@watson watson added auto-backport Deprecated - use backport:version if exact versions are needed v8.1.0 and removed backport:skip This commit does not require backporting labels Oct 28, 2021
@watson watson force-pushed the logs-directory branch 2 times, most recently from 6b6b928 to 13e27d6 Compare October 28, 2021 08:43
@watson watson marked this pull request as ready for review November 2, 2021 10:44
@watson watson requested review from a team as code owners November 2, 2021 10:44
@azasypkin
Copy link
Member

ACK: will review today

@azasypkin azasypkin self-requested a review November 3, 2021 11:42
Copy link
Member

@azasypkin azasypkin left a comment

Choose a reason for hiding this comment

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

Everything looks good, but I'm wondering if we should commit /logs directory (empty dir with .gitignore inside or something like this)? Otherwise if I just try to enable audit logs locally Kibana crashes with Error: ENOENT: no such file or directory, open '.../kibana/logs/audit.log' forcing me to manually create logs folder.

packages/kbn-utils/src/path/index.ts Outdated Show resolved Hide resolved
@watson
Copy link
Contributor Author

watson commented Nov 4, 2021

@elasticmachine merge upstream

@watson
Copy link
Contributor Author

watson commented Nov 4, 2021

I'm wondering if we should commit /logs directory (empty dir with .gitignore inside or something like this)? Otherwise if I just try to enable audit logs locally Kibana crashes with Error: ENOENT: no such file or directory, open '.../kibana/logs/audit.log' forcing me to manually create logs folder.

We used to have an empty data directory checked into git as well, but as far as I understand we went away from that approach to instead create when accessing it if it wasn't there. I guess this is more stable in case a user manually deletes it. @spalger can probably weigh in on the original thoughts around this.

If that's the case, I think it would make sense to change the audit code to create it if it doesn't exist already as well. I actually thought it worked like that all ready since the tests didn't fail. But I guess the tests doesn't fully test that. So I will update this PR to improve that if @spalger agrees this is the way to go.

@watson
Copy link
Contributor Author

watson commented Nov 5, 2021

The default settings uses a rolling log-file which in turn use this code to write the file to disk:

this.outputStream = createWriteStream(this.filePath, {
encoding: 'utf8',
flags: 'a',
});

So we can either update this code to create any missing directories first, though that would impact all rolling file appenders - which might be a nice thing, but is a bigger discussion.

Alternatively, we can update the audit setup code to create the dir, which seems like a smaller change.

@spalger
Copy link
Contributor

spalger commented Nov 5, 2021

We used to have an empty data directory checked into git as well, but as far as I understand we went away from that approach to instead create when accessing it if it wasn't there. I guess this is more stable in case a user manually deletes it. @spalger can probably weigh in on the original thoughts around this.

If I recall correctly we ran into a few issues with the .keep files not integrating with other processes... that said I don't exactly remember why we got rid of them. Maybe @tylersmalley knows.

So we can either update this code to create any missing directories first, though that would impact all rolling file appenders - which might be a nice thing, but is a bigger discussion.

Can you think of a reason this would be undesirable? I think we should do it across the board and include the appropriate release note.

@watson
Copy link
Contributor Author

watson commented Nov 5, 2021

I've created a PR to show how this could be done: #117666

If we decide to go down this path, I think we want to land it as a separate PR and then wait for that to land before continuing with this one. So let's move the related to discussion to that PR.

@tylersmalley
Copy link
Contributor

I don't know if we purposefully got rid of the data directory. Looks like it might have been accidentally removed in 7bac741. I am not opposed to trying it again if it simplifies things.

@watson
Copy link
Contributor Author

watson commented Nov 9, 2021

While a little more complex, I think it's more stable if Kibana could bootstrap its required empty directories on boot. This makes it easier in case you on purpose or accidentally delete them while cleaning up. As far as I can tell the two directories in question here are data and the new logs. However, I don't want this to be a blocker for this PR, so I'll go ahead and add logs/.empty in this PR and then we can follow up with another PR (for instance #117666) where we make them "dynamic".

@cla-checker-service
Copy link

cla-checker-service bot commented Nov 9, 2021

💚 CLA has been signed

@azasypkin azasypkin self-requested a review November 9, 2021 14:28
@azasypkin
Copy link
Member

ACK: will review today

Copy link
Member

@azasypkin azasypkin 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!

@@ -129,6 +129,9 @@ export async function runFpm(
// copy the data directory at /var/lib/kibana
`${resolveWithTrailingSlash(fromBuild('data'))}=/var/lib/kibana/`,

// copy the logs directory at /var/log/kibana
Copy link
Member

@jbudz jbudz Nov 9, 2021

Choose a reason for hiding this comment

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

Can we add an '--exclude', usr/share/kibana/logs, a few lines up so we don't copy the .gitempty/folder over twice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 👍

@watson watson enabled auto-merge (squash) November 9, 2021 19:49
@watson watson disabled auto-merge November 9, 2021 19:50
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @watson

@watson watson merged commit a861170 into elastic:main Nov 10, 2021
@watson watson deleted the logs-directory branch November 10, 2021 05:12
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Nov 10, 2021
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
8.0

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Nov 10, 2021
Co-authored-by: Aleh Zasypkin <[email protected]>

Co-authored-by: Thomas Watson <[email protected]>
Co-authored-by: Aleh Zasypkin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes v8.0.0 v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants