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

feature: Pause application webhooks #7247

Merged
merged 4 commits into from
Sep 5, 2024

Conversation

vlasebian
Copy link
Contributor

@vlasebian vlasebian commented Aug 19, 2024

Summary

References #7223

Changes

  • Add a paused boolean field to the ApplicationWebhook object
  • Skip processing webhooks with the paused field set to True

Testing

Steps
  • Create an application
  • Create an end device
  • Create a webhook (webhook.site can be used to set up one) using:
curl -H "Authorization: $AUTH_TOKEN" 'http://localhost:1885/api/v3/as/webhooks/app1' -d '{"webhook":{"base_url":"https://webhook.site/1e8cc8cf-1b46-4da5-92ea-711b818ba6ec","paused": false,"field_mask":{"paths":[]},"format":"json","ids":{"webhook_id":"wh4"}},"field_mask":{"paths":["base_url","field_mask","format","ids.webhook_id","paused"]}}'
  • Simulate an uplink message and check if it's received by webhook.site
  • Update the webhook to pause it using:
curl -H "Authorization: $AUTH_TOKEN" 'http://localhost:1885/api/v3/as/webhooks/app1/wh4' -X PUT -d '{"webhook":{"base_url":"https://webhook.site/1e8cc8cf-1b46-4da5-92ea-711b818ba6ec","paused": true,"field_mask":{"paths":[]},"format":"json","ids":{"webhook_id":"wh4"}},"field_mask":{"paths":["base_url","field_mask","format","ids.webhook_id","paused"]}}'
  • Simulate another uplink message, no webhook should be received this time
Regressions

I checked if an existing webhook created in a previous version is affected by this change. This are the steps that I did:

  • Checked out on the v3.32 branch
  • Launched the stack and created an app, a device and a custom webhook
  • Sent a simulated uplink and checked if it was sent to the webhooks site
  • Shut down the stack
  • Checked out on the feature branch
  • Launched the stack
  • Sent a curl request to pause the webhook
  • Sent a simulated uplink, it wasn't sent anymore
  • Sent a curl request to unpause the webhook
  • Sent a simulated uplink and checked if it was sent to the webhooks site

Notes for Reviewers

Both uplink and downlink webhooks are paused if the flag is set.

Checklist

  • Scope: The referenced issue is addressed, there are no unrelated changes.
  • Compatibility: The changes are backwards compatible with existing API, storage, configuration and CLI, according to the compatibility commitments in README.md for the chosen target branch.
  • Documentation: Relevant documentation is added or updated.
  • Testing: The steps/process to test this feature are clearly explained including testing for regressions.
  • Infrastructure: If infrastructural changes (e.g., new RPC, configuration) are needed, a separate issue is created in the infrastructural repositories.
  • Changelog: Significant features, behavior changes, deprecations and fixes are added to CHANGELOG.md.
  • Commits: Commit messages follow guidelines in CONTRIBUTING.md, there are no fixup commits left.

@github-actions github-actions bot added the c/application server This is related to the Application Server label Aug 19, 2024
@vlasebian vlasebian self-assigned this Aug 20, 2024
@vlasebian vlasebian added this to the Backlog milestone Aug 22, 2024
@vlasebian vlasebian force-pushed the feature/pause-application-webhooks branch from bbdd872 to 9cc125d Compare August 23, 2024 09:22
@vlasebian vlasebian marked this pull request as ready for review August 23, 2024 11:02
@vlasebian vlasebian requested review from a team as code owners August 23, 2024 11:02
@johanstokking
Copy link
Member

Code looks good to me.

I guess we want Console support for this as well?

Do we need to add an end-to-end test?

Backlog is not a good milestone for a PR.

@vlasebian
Copy link
Contributor Author

For now I placed it in the Backlog milestone as the issue is also in Backlog.

Copy link
Member

@KrishnaIyer KrishnaIyer left a comment

Choose a reason for hiding this comment

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

Base functionality looks great.

A few comments

  1. Please run tools/bin/mage jsSDK:definitions which should fix some of the CI errors.
  2. Indeed this needs Console support, I'll comment in the source issue and check with the frontend team
  3. Please add CLI Support, look for applicationsWebhooksSetCommand in the cmd/commands/applications_webhooks.go
  4. This needs a CHANGELOG entry.
  5. I think we should trigger a notification to the users when a Webhook is paused or unpaused. I don't think we currently have a mechanism for TTS components to trigger a notification in the IS since CreateNotificationRequest is currently only used internally by the IS. @nicholaspcr is this correct?

As a side note:

curl -H "Authorization: $AUTH_TOKEN" 'http://localhost:1885/api/v3/as/webhooks/app1' -d '{"webhook":{"base_url":"https://webhook.site/1e8cc8cf-1b46-4da5-92ea-711b818ba6ec","paused": false,"field_mask":{"paths":[]},"format":"json","ids":{"webhook_id":"wh4"}},"field_mask":{"paths":["base_url","field_mask","format","ids.webhook_id","paused"]}}'

Does this call actually work? I think we require bearer tokens so Authorization: Bearer <token> https://github.com/TheThingsNetwork/lorawan-stack/blob/v3.32/pkg/webmiddleware/log.go#L76-L83

api/ttn/lorawan/v3/applicationserver_web.proto Outdated Show resolved Hide resolved
pkg/applicationserver/io/web/webhooks.go Outdated Show resolved Hide resolved
@KrishnaIyer KrishnaIyer modified the milestones: Backlog, v3.32.1 Aug 28, 2024
@KrishnaIyer
Copy link
Member

Regarding milestones, backlog is indeed sort of a bucket for issues waiting to be picked up. Once you decide to pick up something, always move the issue to the proper version milestone and set the in progress label.

@KrishnaIyer
Copy link
Member

I think we should trigger a notification to the users when a Webhook is paused or unpaused. I don't think we currently have a mechanism for TTS components to trigger a notification in the IS since CreateNotificationRequest is currently only used internally by the IS. @nicholaspcr is this correct?

@vlasebian: On second thought, let's skip this. This would make this PR more complex than it is. I would however like to emit an event when the user pauses/unpauses a webhook.

@vlasebian
Copy link
Contributor Author

vlasebian commented Aug 29, 2024

Does this call actually work? I think we require bearer tokens so Authorization: Bearer <token>

It is there, yes, in the curl -H "Authorization: $AUTH_TOKEN" part. To get the token I usually log in the console and check the network tab in the dev tools to get the Bearer token. Then I do export AUTH_TOKEN=Bearer ... so I have the token for all the requests in my terminal.

@vlasebian vlasebian force-pushed the feature/pause-application-webhooks branch from 9cc125d to d2e1c82 Compare August 29, 2024 10:44
@vlasebian vlasebian requested a review from KrishnaIyer August 29, 2024 10:45
Copy link
Member

@KrishnaIyer KrishnaIyer left a comment

Choose a reason for hiding this comment

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

Code looks good. Please fix CI using the following mage commands

mage proto:all
mage go:messages
mage jsSDK:definitions

@vlasebian vlasebian force-pushed the feature/pause-application-webhooks branch from 579bbe1 to 9564b13 Compare September 2, 2024 13:45
Copy link
Member

@johanstokking johanstokking left a comment

Choose a reason for hiding this comment

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

One comment, otherwise LGTM

pkg/applicationserver/io/web/webhooks_test.go Show resolved Hide resolved
@vlasebian vlasebian force-pushed the feature/pause-application-webhooks branch from 00a8663 to 9343bd2 Compare September 4, 2024 15:39
@vlasebian vlasebian force-pushed the feature/pause-application-webhooks branch from 9343bd2 to a0c23c0 Compare September 5, 2024 08:25
@vlasebian vlasebian merged commit d259f3f into v3.32 Sep 5, 2024
14 of 15 checks passed
@vlasebian vlasebian deleted the feature/pause-application-webhooks branch September 5, 2024 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c/application server This is related to the Application Server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants