Skip to content

feat: use url from annotation value for webhook-notification #46

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.

Sign up for GitHub

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

Open
jimtoniq wants to merge 3 commits into
base: master
Choose a base branch
from
Open

feat: use url from annotation value for webhook-notification #46

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d2b4890
feat: use url from annotation value for webhook-notification
jimtoniq Sep 16, 2021
3f48875
docs: optional webhook-url override
jimtoniq Sep 23, 2021
5218c34
Merge branch 'master' into feature/webhook-recipientUrl
ryota-sakamoto Nov 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/services/webhook.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ apiVersion: argoproj.io/v1alpha1
kind: Application
metadata:
annotations:
notifications.argoproj.io/subscribe.<trigger-name>.<webhook-name>: ""
notifications.argoproj.io/subscribe.<trigger-name>.<webhook-name>: <optional-url-override>
```

## Examples
Expand Down
9 changes: 8 additions & 1 deletion pkg/services/webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"fmt"
"io/ioutil"
"net/http"
"net/url"
"strings"
texttemplate "text/template"

Expand Down Expand Up @@ -91,10 +92,16 @@ type webhookService struct {
}

func (s webhookService) Send(notification Notification, dest Destination) error {
webhookUrl := s.opts.URL
_, urlParseError := url.ParseRequestURI(dest.Recipient)
Copy link
Copy Markdown

@ekbduffy ekbduffy Dec 15, 2021

Choose a reason for hiding this comment

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

Looks like I was doing completely the same in #60 but for MS Teams.
Could be useful to move check of a URL validity to some outside function and re-use everywhere, where an URL might appear :)

Copy link
Copy Markdown
Author

@jimtoniq jimtoniq Dec 15, 2021

Choose a reason for hiding this comment

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

yep, your real-world scenario is exactly the same!
i will have a look into your PR and try to find a place where this url-check can be put.

Copy link
Copy Markdown

@ekbduffy ekbduffy Dec 15, 2021

Choose a reason for hiding this comment

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

@alexmt looks like we can merge two PRs into something one combined with the same ideas. What do you think? :)

Actually, looks like only these two services can be upgraded with such a feature, but still moving out the check if the annotation is a URL somewhere outside of the service itself looks quite useful.

Copy link
Copy Markdown

@ekbduffy ekbduffy Dec 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Looks like pkg/util/misc/misc.go might be a good place for the function

Copy link
Copy Markdown

@ekbduffy ekbduffy Dec 17, 2021

Choose a reason for hiding this comment

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

I've just updated my PR with the switch to enable/disable allowed URLs in the annotation. Might be useful to implement such an option here as well, @jimtoniq.

if isRecipientUrlValid := len(strings.TrimSpace(dest.Recipient)) > 0 && urlParseError == nil; isRecipientUrlValid {
webhookUrl = strings.TrimSpace(dest.Recipient)
}

request := request{
body: notification.Message,
method: http.MethodGet,
url: s.opts.URL,
url: webhookUrl,
destService: dest.Service,
}

Expand Down
58 changes: 56 additions & 2 deletions pkg/services/webhook_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,14 +32,38 @@ func TestWebhook_SuccessfullySendsNotification(t *testing.T) {
Webhook: map[string]WebhookNotification{
"test": {Body: "hello world", Method: http.MethodPost},
},
}, Destination{Recipient: "test", Service: "test"})
}, Destination{Recipient: server.URL, Service: "test"})
assert.NoError(t, err)

assert.Equal(t, "hello world", receivedBody)
assert.Equal(t, receivedHeaders.Get("testHeader"), "testHeaderValue")
assert.Contains(t, receivedHeaders.Get("Authorization"), "Basic")
}

func TestWebhook_WithNoOverrides_OverrideRecipientWithUrl_SuccessfullySendsNotification(t *testing.T) {
var receivedHeaders http.Header
var receivedBody string
server := httptest.NewServer(http.HandlerFunc(func(writer http.ResponseWriter, request *http.Request) {
receivedHeaders = request.Header
data, err := ioutil.ReadAll(request.Body)
assert.NoError(t, err)
receivedBody = string(data)
}))
defer server.Close()

service := NewWebhookService(WebhookOptions{
BasicAuth: &BasicAuth{Username: "testUsername", Password: "testPassword"},
URL: server.URL,
Headers: []Header{{Name: "testHeader", Value: "testHeaderValue"}},
})
err := service.Send(Notification{}, Destination{Recipient: server.URL, Service: "test"})
assert.NoError(t, err)

assert.Equal(t, "", receivedBody)
assert.Equal(t, receivedHeaders.Get("testHeader"), "testHeaderValue")
assert.Contains(t, receivedHeaders.Get("Authorization"), "Basic")
}

func TestWebhook_WithNoOverrides_SuccessfullySendsNotification(t *testing.T) {
var receivedHeaders http.Header
var receivedBody string
Expand All @@ -64,7 +88,7 @@ func TestWebhook_WithNoOverrides_SuccessfullySendsNotification(t *testing.T) {
assert.Contains(t, receivedHeaders.Get("Authorization"), "Basic")
}

func TestWebhook_SubPath(t *testing.T) {
func TestWebhook_SubPath_SuccessfullySendsNotification(t *testing.T) {
var receivedPath string
server := httptest.NewServer(http.HandlerFunc(func(writer http.ResponseWriter, request *http.Request) {
receivedPath = request.URL.Path
Expand Down Expand Up @@ -92,6 +116,36 @@ func TestWebhook_SubPath(t *testing.T) {
assert.Equal(t, "/subpath1/subpath2", receivedPath)
}

func TestWebhook_SubPath_OverrideRecipientWithUrl_SuccessfullySendsNotification(t *testing.T) {
var receivedPath string
server := httptest.NewServer(http.HandlerFunc(func(writer http.ResponseWriter, request *http.Request) {
receivedPath = request.URL.Path
}))
defer server.Close()

recipientUrl := fmt.Sprintf("%s/subpath1", server.URL)

service := NewWebhookService(WebhookOptions{
URL: recipientUrl,
})

err := service.Send(Notification{
Webhook: map[string]WebhookNotification{
"test": {Body: "hello world", Method: http.MethodPost},
},
}, Destination{Recipient: recipientUrl, Service: "test"})
assert.NoError(t, err)
assert.Equal(t, "/subpath1", receivedPath)

err = service.Send(Notification{
Webhook: map[string]WebhookNotification{
"test": {Body: "hello world", Method: http.MethodPost, Path: "/subpath2"},
},
}, Destination{Recipient: recipientUrl, Service: "test"})
assert.NoError(t, err)
assert.Equal(t, "/subpath1/subpath2", receivedPath)
}

func TestGetTemplater_Webhook(t *testing.T) {
n := Notification{
Webhook: WebhookNotifications{
Expand Down