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

feat: Add webhook configuration for sending event payloads #2087

Merged
merged 13 commits into from
Sep 11, 2023

Conversation

yquansah
Copy link
Contributor

@yquansah yquansah commented Sep 6, 2023

Adds support for receiving Flipt audit event payloads to a configured server. This PR does the following:

  • Adds Audit configuration for supporting sending a webhook
  • Adds webhook client with configurable backoff retries
  • Adds tests for the new entries to the Audit configuration

Completes FLI-591

@codecov
Copy link

codecov bot commented Sep 7, 2023

Codecov Report

Merging #2087 (86ac309) into main (3c8d12f) will increase coverage by 0.14%.
Report is 6 commits behind head on main.
The diff coverage is 76.92%.

@@            Coverage Diff             @@
##             main    #2087      +/-   ##
==========================================
+ Coverage   70.61%   70.75%   +0.14%     
==========================================
  Files          70       73       +3     
  Lines        6823     6997     +174     
==========================================
+ Hits         4818     4951     +133     
- Misses       1727     1761      +34     
- Partials      278      285       +7     
Files Changed Coverage Δ
internal/cmd/grpc.go 0.00% <0.00%> (ø)
internal/server/audit/checker.go 68.96% <68.96%> (ø)
internal/server/audit/webhook/webhook.go 78.94% <78.94%> (ø)
internal/server/audit/webhook/client.go 91.42% <91.42%> (ø)
internal/config/audit.go 94.28% <100.00%> (+1.42%) ⬆️
internal/config/config.go 87.04% <100.00%> (+0.03%) ⬆️
internal/server/audit/audit.go 86.46% <100.00%> (ø)
internal/server/middleware/grpc/middleware.go 62.97% <100.00%> (+0.78%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@yquansah yquansah marked this pull request as ready for review September 7, 2023 20:30
@yquansah yquansah requested a review from a team as a code owner September 7, 2023 20:30
Comment on lines 71 to 86
body, err := json.Marshal(e)
if err != nil {
return err
}

req, err := http.NewRequestWithContext(context.Background(), http.MethodPost, w.url, bytes.NewBuffer(body))
if err != nil {
return err
}

// If the signing secret is configured, add the specific header to it.
if w.signingSecret != "" {
signedPayload := w.signPayload(body)

req.Header.Add(fliptSignatureHeader, fmt.Sprintf("%x", signedPayload))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should probably also be adding the URL to this payload.
Just wondering, given we're concerned with signing event payload, if we should also be concerned about where they're being signed. So that folks couldn't replay signed events to different sources and them still be deemed valid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@GeorgeMac That is a great idea. So basically adding the URL in the event payload?

Copy link
Contributor

@GeorgeMac GeorgeMac left a comment

Choose a reason for hiding this comment

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

One thought around client body reading. Otherwise, I think this looks great.

internal/server/audit/webhook/client.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@markphelps markphelps left a comment

Choose a reason for hiding this comment

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

looking good, one comment about the name of the maxBackoff configuration field, also would you mind adding some tests for the webhook/webhook.go and webhook/client.go files?

internal/server/audit/webhook/client.go Outdated Show resolved Hide resolved
internal/server/audit/webhook/client.go Outdated Show resolved Hide resolved
internal/config/audit.go Outdated Show resolved Hide resolved
@yquansah yquansah dismissed stale reviews from markphelps and GeorgeMac September 9, 2023 03:41

Addressed.

Copy link
Contributor

@GeorgeMac GeorgeMac left a comment

Choose a reason for hiding this comment

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

Looking great, just one piece of debugging left in there.

internal/server/audit/webhook/client.go Outdated Show resolved Hide resolved
}

// SendAudit will send an audit event to a configured server at a URL.
func (w *HTTPClient) SendAudit(e audit.Event) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

was thinking we might want to modify the signature of this method to add ctx as the first parameter, that way we could pass it down to http.NewRequestWithContext below, and cancel any inflight requests if Flipt were to shutdown potentially

return err
}

req, err := http.NewRequestWithContext(context.Background(), http.MethodPost, w.url, bytes.NewBuffer(body))
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should probably set the request headers for content-type: application/json

Copy link
Collaborator

@markphelps markphelps left a comment

Choose a reason for hiding this comment

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

looks great! great work @yquansah !! 🪝

@yquansah yquansah merged commit 56a620b into main Sep 11, 2023
25 checks passed
@yquansah yquansah deleted the webhook-handler branch September 11, 2023 16:07
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