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: use url from annotation value for webhook-notification #46
base: master
Are you sure you want to change the base?
Conversation
52b45bd
to
a7e192d
Compare
Codecov Report
@@ Coverage Diff @@
## master #46 +/- ##
==========================================
+ Coverage 48.21% 48.33% +0.12%
==========================================
Files 29 29
Lines 1680 1684 +4
==========================================
+ Hits 810 814 +4
Misses 687 687
Partials 183 183
Continue to review full report at Codecov.
|
@jimtoniq |
@ryota-sakamoto |
Signed-off-by: Thomas Münzl <thomas@jimtoniq.de>
Signed-off-by: Thomas Münzl <thomas@jimtoniq.de>
5c6fa48
to
3f48875
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The notification engine already passing the recipient as a "context" variable:
notifications-engine/pkg/api/api.go
Line 64 in e8ac53e
in[serviceTypeVarName] = dest.Service |
The variable is available in the notification template and can be referenced in the request body or path template. Changes in this PR will break this use case.
As I understand you need to be able to get the full URL from the recipient value, correct? We can achieve this use case in a different way: if "url" field of "webhook" setting is optional then webook service should assume that path template return full url. We would have to make changes here:
notifications-engine/pkg/services/webhook.go
Line 130 in 0e1f1ed
if notification.Path != "" { |
WDYT?
Hi Alex, With our webhook-service configuration, we have something like this service.webhook.mattermost_webhook: |
url: https://{mattermost-host}/hooks/{hook-id}
headers:
- name: Content-Type
value: application/json We want our users to configure their webook-url on their own (via UI with app-annotations), but with the above configuration, we would need to create several webhook-service-configs for each new webhook on many mattermost-channel. With the email-service, its possible (or even required) to set the recipient from annotation, as its read from
Thats why I tried making use of the dest.Recipient in the webhook-service, as its already read from the annotation and consider it as a overwrite.
I see it as a flexible solution to have the notification-template be the same, with less manual yaml-configuration but custom webhook-urls. Do you think we would need to make changes to not break other usages? Let me know |
@jimtoniq , in your use case, is it required to override mattermost host for each recipient? If not then you already can use
So the request will be sent to https://{mattermost-host}/hooks/{{recipient}} If you need to override whole URL then we need to make a code change. I just realized we can support |
We would like to set the entire url from the annotation. Then annotations on the
With the current state, we would create two services for the upper example: service.webhook.mymattermost_webhook: |
url: https://mymattermost.domain.tld/hooks/abcd1234
headers:
- name: Content-Type
value: application/json service.webhook.anothermattermost_webhook: |
url: https://anothermattermost.domain.tld/hooks/bcd0123
headers:
- name: Content-Type
value: application/json and two redundant webhook-templates template.app-health-degraded: |
email:
subject: Application {{.app.metadata.name}} has degraded.
message: |
{{if eq .serviceType "slack"}}:exclamation:{{end}} Application {{.app.metadata.name}} has degraded.
Application details: {{.context.argocdUrl}}/applications/{{.app.metadata.name}}.
webhook:
mymattermost_webhook:
method: POST
body: |
{
"attachments": [{
"title": "{{.app.metadata.name}}",
"title_link": "{{.context.argocdUrl}}/applications/{{.app.metadata.name}}",
"color": "#18be52",
"fields": [{
"title": "Sync Status",
"value": "{{.app.status.sync.status}}",
"short": true
}, {
"title": "Repository",
"value": "{{.app.spec.source.repoURL}}",
"short": true
}]
}]
}
anothermattermost_webhook:
method: POST
body: |
{
"attachments": [{
"title": "{{.app.metadata.name}}",
"title_link": "{{.context.argocdUrl}}/applications/{{.app.metadata.name}}",
"color": "#18be52",
"fields": [{
"title": "Sync Status",
"value": "{{.app.status.sync.status}}",
"short": true
}, {
"title": "Repository",
"value": "{{.app.spec.source.repoURL}}",
"short": true
}]
}]
} to use them with annotations: |
Sorry for the long response @jimtoniq . Got it . In this case I would recommend adding the service.webhook.mymattermost_webhook: |
headers:
- name: Content-Type
value: application/json
template.app-health-degraded: |
webhook:
mymattermost_webhook:
method: POST
url: "{{recipient}}"
body: |
{
"attachments": [{
"title": "{{.app.metadata.name}}",
"title_link": "{{.context.argocdUrl}}/applications/{{.app.metadata.name}}",
"color": "#18be52",
"fields": [{
"title": "Sync Status",
"value": "{{.app.status.sync.status}}",
"short": true
}, {
"title": "Repository",
"value": "{{.app.spec.source.repoURL}}",
"short": true
}]
}]
} Does it sounds good? |
@@ -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) |
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
This feature is enabling to set an Webhook-URL within the annotation. The value is already written into
Destination.Recipient
and now used when it's an valid URL.Can therefore be set from ArgoCD UI.