feat: use url from annotation value for webhook-notification

[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.

[codecov]

Codecov Report

Merging #46 (5218c34) into master (0e1f1ed) will increase coverage by 0.12%.
The diff coverage is 100.00%.

@@            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              

Impacted Files	Coverage Δ
pkg/services/webhook.go	69.35% <100.00%> (+2.11%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0e1f1ed...5218c34. Read the comment docs.

[ryota-sakamoto]

@jimtoniq
Would you add docs for this features?

[jimtoniq]

@ryota-sakamoto
Sure! hope that one is enough

[alexmt]

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?

[jimtoniq]

Hi Alex,
thanks for looking into my PR.

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.
The url above shouldn't be optional but a fallback.

With the email-service, its possible (or even required) to set the recipient from annotation, as its read from dest.Recipient

notifications-engine/pkg/services/email.go

Line 84 in 0e1f1ed

}).WithSubject(subject).WithBody(body).To(dest.Recipient)

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

[alexmt]

@jimtoniq , in your use case, is it required to override mattermost host for each recipient? If not then you already can use {{recipient}} in template:

  service.webhook.mattermost_webhook: |
    url: https://{mattermost-host}/hooks/
    headers:
    - name: Content-Type
      value: application/json

  template.<templateName>: |
    webhook:
      <webhook-name>:
        method: POST
        path: /{{recipient}}
        body: |
          <optional-body-template>

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 url field in template - probably this is the cleanest solution and won't break existing behavior

[jimtoniq]

We would like to set the entire url from the annotation.
So that we could have one service and one template for each webhook-service.

Then annotations on the Application can be

notifications.argoproj.io/subscribe.app-health-degraded.mattermost_webhook: https://mymattermost.domain.tld/hooks/abcd1234
for one app,
for another one
notifications.argoproj.io/subscribe.app-health-degraded.mattermost_webhook: https://anothermattermost.domain.tld/hooks/bcd0123
without having to configure multiple services or templates in our argocd-notifications configuration.

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:
notifications.argoproj.io/subscribe.app-health-degraded.mymattermost_webhook: ""
notifications.argoproj.io/subscribe.app-health-degraded.anothermattermost_webhook: ""

[alexmt]

Sorry for the long response @jimtoniq .

Got it . In this case I would recommend adding the url field in webhook template. So you could define mattermost service once and use url: "{{recipient}}" in template to support customizing the full URL.

  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?

[ekbduffy]

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 :)

[jimtoniq]

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.

[ekbduffy]

@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.

[ekbduffy]

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

[ekbduffy]

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.