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

Webhook documentation #2150

Merged
merged 4 commits into from
Nov 9, 2020
Merged

Webhook documentation #2150

merged 4 commits into from
Nov 9, 2020

Conversation

antgamdia
Copy link
Contributor

Description of the change

Documented the process of configuring a webhook (exemplified using Harbor) and the subsequent creation/granting of the service account.

Benefits

Users will now be able to configure their platforms to use the /refresh endpoint implemented in #2138.

Possible drawbacks

Despite trying to be platform-agnostic, the only real example documented is Harbor. Perhaps other platforms should be included, don't know.

Applicable issues

Additional information

This PR is only intended for documenting the process implemented in #2138.

Copy link
Contributor

@absoludity absoludity left a comment

Choose a reason for hiding this comment

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

Excellent @antgamdia . I'm +1'ing with a bunch of text suggestions inline that are all optional - see what you think. If you do decide to add the ClusterRole to the chart itself, perhaps don't land, but otherwise fire away :)

subjects:
- kind: ServiceAccount
name: apprepositories-webhook
namespace: default
Copy link
Contributor

Choose a reason for hiding this comment

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

Just pop a new line at the end here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, after reading the rest of the docs, I don't think you refer to this file at all? You may have added it because you saw a bunch of other kubeapps-local-dev-*.yaml files, but they're actually used for the local env or in CI. I think you can remove this file, unless you think it's beneficial for some other reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I just supposed the whole files should be there regardless of any existing mention in the docs. I'll delete this file since it is actually unnecessary. Thanks for letting me know!

kind: RoleBinding
metadata:
name: apprepositories-webhook
namespace: kubeapps
Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't checked if you mention this later, but the namespace may not be for kubeapps? Not sure whether it may be better for the example to do a namespaced app repo (as it's less of a security concern) and mention that if it's for a global repo then the rolebinding will need to be for the kubeapps namespace. See what you think - I'm OK either way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually forgot to mention it in the docs. Thank you for bringing it to my attention!

But, yes, I was thinking about what's might be the best approach for the example. Whereas using the kubeapps namespace will work seamlessly for users who just deploy the default kubeapps chart with the Bitnami repo, it has, as you mention, security concerns. Given that the general docs also include the cluster-admin for the sake of simplicity, I opted to exemplify using the non-namespaced repo.

Nevertheless, since the target user using webhooks may be the one who is installing a new custom repo (i.e., Harbor), I believe we should choose the namespaced repo approach in the docs. I'll modify the docs accordingly.


Nevertheless, this default approach might not be useful for environments with highly frequent changes. Moreover, if there are a few App Repositories with numerous changes while others hardly are modified, therefore, increasing the default global syncing periodicity is not a good approach.

Kubeapps now support a method for manually trigger a sync process over a given App Repository intended to be used as a webhook endpoint.
Copy link
Contributor

Choose a reason for hiding this comment

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

Small typo: s/support/supports/ , s/trigger/triggering/, s/over/for/ . I'd also break the second phrase to a separate sentence to simplify it. So, as a suggestion:

"Kubeapps now supports an API endpoint for manually triggering a sync process for a given App Repository. This endpoint is intended to be used as a webhook from external applications."

Kubeapps now support a method for manually trigger a sync process over a given App Repository intended to be used as a webhook endpoint.
A number of platforms do use webhooks for triggering actions when something occurs. For instance, [Harbor notifies the webhook endpoint of certain events that occur in the project]
(https://goharbor.io/docs/1.10/working-with-projects/project-configuration/configure-webhooks/).
Webhook notifications provide information about events in JSON format and are usually delivered by HTTP(s) POST to an existing webhook endpoint URL.
Copy link
Contributor

Choose a reason for hiding this comment

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

"by an HTTP(s) POST request to ..."

(https://goharbor.io/docs/1.10/working-with-projects/project-configuration/configure-webhooks/).
Webhook notifications provide information about events in JSON format and are usually delivered by HTTP(s) POST to an existing webhook endpoint URL.

This document will use Harbor as a running example for explaining how a webhook is configured for triggering an App Repository syncing process.
Copy link
Contributor

Choose a reason for hiding this comment

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

The example below will use Harbor ... triggering an App Repository sync process.

metadata:
name: apprepositories-webhook
---
kind: ClusterRole
Copy link
Contributor

Choose a reason for hiding this comment

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

A couple of related thoughts: I assume you're using a ClusterRole so it can be re-used in other namespaces, but the person creating the webhook might not have cluster-admin. I wonder if the Kubeapps chart should be updated with this PR to include the creation of this cluster role when installing Kubeapps, so that any Kubeapps user can setup a webhook for an app repository in their namespace? See what you think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mmmm, it really makes sense! I'm going to test this idea out and then I'll do another PR for the chart thing so we can continue the discussion there.
Thank you for the valuable comments!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Follow up PR here: #2151

EOF
```

Next, the `ClusterRole` *apprepositories-webhook* (with `get` and `update` permissions over `apprepositories`) has to be associated to a certain (or multiple) namespace(s) by means of a `RoleBinding`:
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you give an example of how to bind it in multiple namespaces below, this sentence might be simpler as "...to be bound to the relevant namespace(s) by means of a RoleBinding:"


Next, the `ClusterRole` *apprepositories-webhook* (with `get` and `update` permissions over `apprepositories`) has to be associated to a certain (or multiple) namespace(s) by means of a `RoleBinding`:

> To bind it in multiple namespaces, create as RoleBinding (changing `namespace: my-namespace` to match your needs) as you need.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/as you need/for each namespace/


## Configuring a webhook in Harbor

A high-level description of the main steps is presented herein; please refer to the [official Harbor documentation](https://goharbor.io/docs/1.10/working-with-projects/project-configuration/configure-webhooks/) for further information.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/herein/below/ (simple :) )

4. As authentication header enter `Bearer <TOKEN>`
> Note `<TOKEN>`is the value retrieved [before](#retrieving-the-serviceaccount-token).

5. Select as many events as you need to trigger notifications.
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps you could append to step 5: " The image below shows the common events that you would want for triggering a resync." and then updating the image to just select the relevant ones? (I could be missing something, but I think it'd just be "Chart Uploaded" and "Chart Deleted"? (possibly also Artifact pushed / deleted for future OCI stuff?)

PR comments have been added.  Modify image. Delete unnecessary manifest file
Copy link
Contributor

@absoludity absoludity left a comment

Choose a reason for hiding this comment

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

Great, thanks for the updates!

@antgamdia antgamdia merged commit 6837847 into vmware-tanzu:master Nov 9, 2020
@antgamdia antgamdia deleted the webhookDocs branch November 9, 2020 08:30

### Retrieving the ServiceAccount token

The `<TOEKN>` for the ServiceAccount *apprepositories-webhook* is retrieved:
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: <TOKEN> :)

antgamdia added a commit that referenced this pull request Nov 16, 2020
* Add apprepositories-webhook ClusterRole in kupeapps chart

Rel PR #2150

* Renamed apprepositories-webhook to apprepositories-refresh
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