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

First crack at a fix to prevent clients from stomping on their headers #376

Merged
merged 3 commits into from
Nov 2, 2022

Conversation

bc-sb
Copy link
Contributor

@bc-sb bc-sb commented Oct 25, 2022

First crack at a fix to prevent clients from stomping on their own headers

What type of PR is this?

Uncomment one (or more) /kind <> lines:

/kind bug

/kind cleanup

/kind design

/kind documentation

/kind failing-test

/kind feature

Any specific area of the project related to this PR?

Uncomment one (or more) /area <> lines:

/area build

/area config

/area outputs

/area tests

What this PR does / why we need it:
Fixes a race condition that multiple calls to the same http Post causes headers to become unset
Which issue(s) this PR fixes:
#375

Fixes #375

Special notes for your reviewer:

@poiana
Copy link

poiana commented Oct 25, 2022

Welcome @bc-sb! It looks like this is your first PR to falcosecurity/falcosidekick 🎉

@poiana poiana added the size/M label Oct 25, 2022
@Issif Issif added this to the 2.27.0 milestone Oct 26, 2022
@Issif
Copy link
Member

Issif commented Oct 26, 2022

Thank you for this PR, I'm in holidays this week, I'll take a look asap

@Issif
Copy link
Member

Issif commented Oct 31, 2022

For all outputs that use the generic client.Post method, why are you not locking directly in the method (https://github.com/spyderbat/falcosidekick/blob/d80b603dddd6a2599f6691e48cf21d40f7f2df66/outputs/client.go#L139)? I see you set your c.m.Lock() in each output.

@bc-sb
Copy link
Contributor Author

bc-sb commented Oct 31, 2022

I actually tried that approach originally of just doing a:

Lock()
defer Unlock()

But was still seeing it...admittedly I got lazy and just crammed it into the clients that were touching the headers to confirm. But now that I looked at it again, I ran the race detector on it:

==================
WARNING: DATA RACE
Write at 0x00c000620110 by goroutine 42:
  github.com/falcosecurity/falcosidekick/outputs.(*Client).Post()
      /home/bc/spyderbat/falcosidekick/outputs/client.go:232 +0x1904
  github.com/falcosecurity/falcosidekick/outputs.(*Client).WebhookPost()
      /home/bc/spyderbat/falcosidekick/outputs/webhook.go:19 +0x1fd
  main.forwardEvent.func22()
      /home/bc/spyderbat/falcosidekick/handlers.go:246 +0xa8                                                                                                                                                                                                                                                                                                                            Previous write at 0x00c000620110 by goroutine 55:
  github.com/falcosecurity/falcosidekick/outputs.(*Client).AddHeader()                                                                                                                            /home/bc/spyderbat/falcosidekick/outputs/client.go:287 +0x7f5
  github.com/falcosecurity/falcosidekick/outputs.(*Client).WebhookPost()
      /home/bc/spyderbat/falcosidekick/outputs/webhook.go:15 +0x6ff                                                                                                                           main.forwardEvent.func22()
      /home/bc/spyderbat/falcosidekick/handlers.go:246 +0xa8  

So, I think I have a better option that I will push after seeing that.

@bc-sb
Copy link
Contributor Author

bc-sb commented Oct 31, 2022

@Issif I just pushed the more robust option.

Signed-off-by: Beau Croteau <[email protected]>
@Issif
Copy link
Member

Issif commented Oct 31, 2022

Can you a more explicit name than m for the mutex please, I try to keep things explicit for beginners in Go that would like to contribute

Signed-off-by: Beau Croteau <[email protected]>
@@ -103,6 +104,7 @@ type Client struct {
DogstatsdClient *statsd.Client
GCPTopicClient *pubsub.Topic
GCPCloudFunctionsClient *gcpfunctions.CloudFunctionsClient
m sync.Mutex
Copy link
Member

Choose a reason for hiding this comment

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

Can you use a longer name for clarity please.

@poiana poiana added the lgtm label Nov 2, 2022
@poiana
Copy link

poiana commented Nov 2, 2022

LGTM label has been added.

Git tree hash: 10d4035a83f8192a189c4931da6b547ce70519ec

@poiana
Copy link

poiana commented Nov 2, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bc-sb, Issif

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@poiana poiana added the approved label Nov 2, 2022
@poiana poiana merged commit 8d2c8ab into falcosecurity:master Nov 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

400 Headers Missing When Firing Multiple Alerts via HTTP Post
3 participants