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

tailscale/logtail: log-filtering #10531

Merged
merged 1 commit into from
Dec 15, 2023
Merged

tailscale/logtail: log-filtering #10531

merged 1 commit into from
Dec 15, 2023

Conversation

as2643
Copy link
Contributor

@as2643 as2643 commented Dec 8, 2023

In order to provide more visibility into the operation of tailnets for a specific customer, redact public IP addresses such as employee homes from the log content before the upload.

Updates #15664

@DentonGentry
Copy link
Contributor

For this to make it into 1.56 it would need to be finished tomorrow. However the consumer, who shall remain nameless because this is a public PR, wouldn't be likely to deploy it in December anyway. If it slips into 1.58 instead that would be fine.

@DentonGentry
Copy link
Contributor

At this point I'd request this go in after release 1.56 branches, to be included in 1.58.

@as2643 as2643 force-pushed the anishka/coral_log_filtering branch 2 times, most recently from d003d42 to f789d98 Compare December 13, 2023 22:57
@as2643 as2643 changed the title tailscale/logtail: coral-log-filtering tailscale/logtail: log-filtering Dec 13, 2023
@as2643 as2643 force-pushed the anishka/coral_log_filtering branch 3 times, most recently from 15d10aa to ffc924b Compare December 15, 2023 17:11
@as2643 as2643 marked this pull request as ready for review December 15, 2023 17:11
Copy link
Member

@creachadair creachadair left a comment

Choose a reason for hiding this comment

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

A couple small notes in passing.

Comment on lines 770 to 772
func redactIPs(buf []byte) []byte {
regexMatchesIPv6 := regexp.MustCompile(`([0-9a-fA-F]{1,4}):([0-9a-fA-F]{1,4}):([0-9a-fA-F:]{1,4})*`)
regexMatchesIPv4 := regexp.MustCompile(`(\d{1,3})\.(\d{1,3})\.\d{1,3}\.\d{1,3}`)
Copy link
Member

Choose a reason for hiding this comment

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

We should bump these variables up to the package level so we don't re-compile the patterns each time it's called.

Suggested change
func redactIPs(buf []byte) []byte {
regexMatchesIPv6 := regexp.MustCompile(`([0-9a-fA-F]{1,4}):([0-9a-fA-F]{1,4}):([0-9a-fA-F:]{1,4})*`)
regexMatchesIPv4 := regexp.MustCompile(`(\d{1,3})\.(\d{1,3})\.\d{1,3}\.\d{1,3}`)
var (
regexMatchesIPv6 = regexp.MustCompile(`([0-9a-fA-F]{1,4}):([0-9a-fA-F]{1,4}):([0-9a-fA-F:]{1,4})*`)
regexMatchesIPv4 = regexp.MustCompile(`(\d{1,3})\.(\d{1,3})\.\d{1,3}\.\d{1,3}`)
)
func redactIPs(buf []byte) []byte {

@@ -757,6 +767,33 @@ func (l *Logger) Write(buf []byte) (int, error) {
return inLen, err
}

func redactIPs(buf []byte) []byte {
Copy link
Member

Choose a reason for hiding this comment

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

Although it's unexported, this function is doing enough interesting work that it probably deserves a doc comment. In particular it'd be good to say what the input is and to summarize how the function transforms it.

regexMatchesIPv6 := regexp.MustCompile(`([0-9a-fA-F]{1,4}):([0-9a-fA-F]{1,4}):([0-9a-fA-F:]{1,4})*`)
regexMatchesIPv4 := regexp.MustCompile(`(\d{1,3})\.(\d{1,3})\.\d{1,3}\.\d{1,3}`)

out := regexMatchesIPv6.ReplaceAllStringFunc(string(buf), func(s string) string {
Copy link
Member

Choose a reason for hiding this comment

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

I would guess we can save some allocation and copying if we use the byte forms:

regexMatchesIPv6.ReplaceAllFunc(buf, func(b []byte) []byte {
   // ...
   prefix := bytes.Split(b, []byte(":"))
   return bytes.Join(append(prefix[:2], []byte("x")), []byte(":"))
})

That way we don't need to convert to string and back, since we wanted the output as a byte slice anyway. But, YMMV.

…ilscaled.

Updates #15664

Signed-off-by: Anishka Singh <anishkasingh66@gmail.com>
Copy link
Member

@creachadair creachadair left a comment

Choose a reason for hiding this comment

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

LGTM for the code, though before merging we should make sure the release branch Denton noted has been cut.

@as2643 as2643 merged commit 3fb6ee7 into main Dec 15, 2023
46 checks passed
@as2643 as2643 deleted the anishka/coral_log_filtering branch December 15, 2023 23:15
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.

None yet

3 participants