Skip to content

fix(audit): logs not getting deleted after N days #7567

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

Merged
merged 3 commits into from
Mar 15, 2021

Conversation

ajeetdsouza
Copy link
Contributor

@ajeetdsouza ajeetdsouza commented Mar 15, 2021

Fixes DGRAPH-3139.

There were two bugs fixed here:

  • The wrong parameter was being passed to processOldLogFiles (l.MaxSize instead of l.MaxAge).
  • When formatting the closing timestamp of an audit log file, we use the local timezone. However, when parsing it back, we were assuming a UTC timezone. I changed this to assume local timezone.

This change is Reviewable

@github-actions github-actions bot added the area/enterprise Related to proprietary features label Mar 15, 2021
x/log_writer.go Outdated
@@ -404,7 +406,8 @@ func processOldLogFiles(fp string, maxAge int64) ([]string, []string, error) {
}

_, e := prefixAndExt(fp)
ts, err := time.Parse(backupTimeFormat, f.Name()[len(defPrefix):len(f.Name())-len(e)])
tsString := f.Name()[len(defPrefix) : len(f.Name())-len(e)]
ts, err := time.ParseInLocation(backupTimeFormat, tsString, time.Local)
Copy link
Contributor

Choose a reason for hiding this comment

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

@ajeetdsouza can you please point me to the code that creates the file? I can't find that code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jarifibrahim time.Now() returns time using the local timezone, but backupTimeFormat does not include timezone information, which is what is causing the problem.

Sample code:

package main

import (
	"fmt"
	"time"
)

func main() {
	const backupTimeFormat = "2006-01-02T15-04-05.000"
	now := time.Now()
	tsLocal := now.Format(backupTimeFormat)
	tsUTC := now.UTC().Format(backupTimeFormat)
	fmt.Printf("tsLocal: %s\ntsUTC: %s\n", tsLocal, tsUTC) // different values
}

There are three alternate approaches here:

  • Change backupTimeFormat to include timezone.
  • Use UTC when formatting to backupTimeFormat.
  • Both of the above.

I think these would be better than the current approach, since there is no ambiguity that way.

Copy link
Contributor

@jarifibrahim jarifibrahim Mar 15, 2021

Choose a reason for hiding this comment

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

Change backupTimeFormat to include timezone.
Use UTC when formatting to backupTimeFormat.
Both of the above.

This would change the file name and we might not be able to parse already existing files. Is this backupTimeFormat used only in audit logs? @aman-bansal why is it called backupTimeFormat?

If this is used only in audit logs, we can use time.UTC() everywhere for simplicity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jarifibrahim backupTimeFormat is only used within LogWriter, so I think it's safe to switch over to UTC timestamps. I just changed it to use UTC everywhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

UTC is fine. We are only using this for audit logs. So this will be fine.

@ajeetdsouza ajeetdsouza merged commit 2b61b62 into release/v21.03 Mar 15, 2021
@ajeetdsouza ajeetdsouza deleted the ajeet/audit branch March 15, 2021 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/enterprise Related to proprietary features
Development

Successfully merging this pull request may close these issues.

3 participants