Add a /refresh endpoint for forcing an update in an AppRepository #2138

antgamdia · 2020-11-03T17:31:56Z

Description of the change

As discussed in #2062 , an endpoint to trigger a refresh in a given repository was requested. Internally, a refresh is being triggered when resyncRequests property changes. This PR creates an endpoint for doing so.
It also has implications on the frontend side, since it is no longer need to use de old logic (GET the appRepo, increase the resyncRequests and PUT it back).

Benefits

This PR enables third-party integrations (i.e., Harbor webhooks) to manually trigger an update in a given AppRepository whenever needed.
Furthermore, on the frontend side, we no longer need to perform two requests, one is enough.

Possible drawbacks

I can't see any negative implications in the short term. Perhaps we should re-think webhook and subscription mechanisms in the future and revisit this /refresh approach

Applicable issues

fixes [FEATURE REQUEST] refresh repository by webhook #2062

Additional information

For example, a POST /api/v1/clusters/default/namespaces/kubeapps/apprepositories/bitnami/refresh will trigger a refresh in the bitnami appRepository in the namespace kubeapps.

Rel vmware-tanzu#2062 For instance, "POST /api/v1/clusters/default/namespaces/kubeapps/apprepositories/bitnami/refresh" will trigger a refresh in the "bitnami" appRepository in the namespace "kubeapps"

Rel vmware-tanzu#2062 Now it calls the new POST .../refresh endpoint implemented in the backend.

absoludity

This PR enables third-party integrations (i.e., Harbor webhooks) to manually trigger an update in a given AppRepository whenever needed.

Although I like the generals changes here (simplifying refresh), I just want to check: how does this work as a third-party webhook integration, when it depends on the user's authentication? Is the harbor webhook only ever triggered via the users browser session as opposed to a background process?

That is, the new endpoint assumes the user is logged in to kubeapps (so that their cookie is being used by oauth2-proxy to include their id_token as the bearer, etc.). I assume if you hit this URL via curl or a private browser window, it'd not work (at least I hope it wouldn't currently :) ). So it may work if the webhook is triggered from the users' browser in harbor and they're already auth'd with Kubeapps, but I'm guessing otherwise it'd fail (ie. logout of kubeapps and then try from harbor).

We could enable the refresh to happen without auth creds with specific conditions (since it's idempotent and non-destructive) - interested to hear your thoughts, or where I've misunderstood :)

absoludity · 2020-11-03T19:55:30Z

dashboard/src/shared/url.ts

@@ -82,6 +82,8 @@ export const backend = {
    validate: (cluster: string) => `${backend.apprepositories.base(cluster, "kubeapps")}/validate`,
    delete: (cluster: string, name: string, namespace: string) =>
      `${backend.apprepositories.base(cluster, namespace)}/${name}`,
+    refresh: (cluster: string, name: string, namespace: string) =>


I was going to say that the ordering is odd - I've been trying to standardise on cluster, namespace, name in other places - but I see you're just following the existing code here. Perhaps in a followup PR we can standardise these ones to match?

Yes, below the update method is properly cluster, namespace, name so I would use that order.

+1 the ordering is pretty odd. I considered changing to cluster, namespace, name, but I opted for preserving the existing order for the sake of uniformity.
I'll send another PR to solve it when this one becomes merged, or do you prefer to fix it directly in this PR?

both options work for me

absoludity · 2020-11-03T19:57:28Z

pkg/http-handler/http-handler.go

@@ -274,12 +305,14 @@ func SetupDefaultRoutes(r *mux.Router, clustersConfig kube.ClustersConfig) error
 	r.Methods("PUT").Path("/namespaces/{namespace}/apprepositories/{name}").Handler(http.HandlerFunc(UpdateAppRepository(backendHandler)))
 	r.Methods("DELETE").Path("/namespaces/{namespace}/apprepositories/{name}").Handler(http.HandlerFunc(DeleteAppRepository(backendHandler)))
 	r.Methods("GET").Path("/namespaces/{namespace}/operator/{name}/logo").Handler(http.HandlerFunc(GetOperatorLogo(backendHandler)))
+


:) I'm liking your attention both to detail and clarity.

andresmgot

Thanks @antgamdia! I have a few comments.

andresmgot · 2020-11-04T09:50:40Z

dashboard/src/shared/AppRepository.ts

-    const { data } = await axiosWithAuth.put(
-      AppRepository.getSelfLink(cluster, namespace, name),
-      repo,
+    const { data } = await axiosWithAuth.post(


You can do a get request, right?

Yes, technically it is perfectly feasible. However, webhooks (almost) always use POST method for sending requests mainly for: I) semantically, "refresh" is an action and it is something you want to get executed, not simply retrieve a value; II) it will trigger a subsequent operation that may not be idempotent.

At the moment, this POST method will ignore any payload sent in the body, that's the reason for sending null at the frontend.

🤔 okay, makes sense

andresmgot · 2020-11-04T09:51:50Z

dashboard/src/shared/url.ts

@@ -82,6 +82,8 @@ export const backend = {
    validate: (cluster: string) => `${backend.apprepositories.base(cluster, "kubeapps")}/validate`,
    delete: (cluster: string, name: string, namespace: string) =>
      `${backend.apprepositories.base(cluster, namespace)}/${name}`,
+    refresh: (cluster: string, name: string, namespace: string) =>


Yes, below the update method is properly cluster, namespace, name so I would use that order.

andresmgot · 2020-11-04T09:56:41Z

pkg/kube/kube_handler.go

+	if a.kubeappsNamespace == "" {
+		log.Errorf("attempt to use app repositories handler without kubeappsNamespace configured")
+		return nil, fmt.Errorf("kubeappsNamespace must be configured to enable app repository handler")
+	}


you don't need this check for this method (you don't need a.kubeappsNamespace here).

andresmgot · 2020-11-04T10:02:16Z

pkg/http-handler/http-handler.go

 	r.Methods("GET").Path("/clusters/{cluster}/namespaces").Handler(http.HandlerFunc(GetNamespaces(backendHandler)))
 	r.Methods("GET").Path("/clusters/{cluster}/apprepositories").Handler(http.HandlerFunc(ListAppRepositories(backendHandler)))
 	r.Methods("GET").Path("/clusters/{cluster}/namespaces/{namespace}/apprepositories").Handler(http.HandlerFunc(ListAppRepositories(backendHandler)))
 	r.Methods("POST").Path("/clusters/{cluster}/namespaces/{namespace}/apprepositories").Handler(http.HandlerFunc(CreateAppRepository(backendHandler)))
 	r.Methods("POST").Path("/clusters/{cluster}/namespaces/{namespace}/apprepositories/validate").Handler(http.HandlerFunc(ValidateAppRepository(backendHandler)))
 	r.Methods("PUT").Path("/clusters/{cluster}/namespaces/{namespace}/apprepositories/{name}").Handler(http.HandlerFunc(UpdateAppRepository(backendHandler)))
+	r.Methods("POST").Path("/clusters/{cluster}/namespaces/{namespace}/apprepositories/{name}/refresh").Handler(http.HandlerFunc(RefreshAppRepository(backendHandler)))


I would change this for a GET request, the body is not relevant.

See my comments above

andresmgot · 2020-11-04T10:06:49Z

Although I like the generals changes here (simplifying refresh), I just want to check: how does this work as a third-party webhook integration, when it depends on the user's authentication? Is the harbor webhook only ever triggered via the users browser session as opposed to a background process?

That is, the new endpoint assumes the user is logged in to kubeapps (so that their cookie is being used by oauth2-proxy to include their id_token as the bearer, etc.). I assume if you hit this URL via curl or a private browser window, it'd not work (at least I hope it wouldn't currently :) ). So it may work if the webhook is triggered from the users' browser in harbor and they're already auth'd with Kubeapps, but I'm guessing otherwise it'd fail (ie. logout of kubeapps and then try from harbor).

We could enable the refresh to happen without auth creds with specific conditions (since it's idempotent and non-destructive) - interested to hear your thoughts, or where I've misunderstood :)

Yes, with these changes, only authenticated users (with permissions to modify AppRepos) would be able to trigger the refresh. My suggestion is to write some documentation about how to create a serviceaccount with those permissions and use its token as a Bearer token when configuring, for example, the Harbor webhook (as explained in here: #2062 (comment)).

antgamdia · 2020-11-04T16:24:40Z

My suggestion is to write some documentation about how to create a serviceaccount with those permissions and use its token as a Bearer token when configuring,

+1 to this idea. Harbor indeed only supports authentication in webhooks by manually setting the HTTP Authorization header when configuring it. Therefore, I'll go for the idea of the serviceaccount for hooks.
I'll investigate and document it in this PR.

andresmgot · 2020-11-04T17:05:20Z

I'll investigate and document it in this PR.

You can do a separate PR for it if you want. Typically split PRs are better.

absoludity

My suggestion is to write some documentation about how to create a serviceaccount with those permissions and use its token as a Bearer token when configuring,

+1 to this idea. Harbor indeed only supports authentication in webhooks by manually setting the HTTP Authorization header when configuring it. Therefore, I'll go for the idea of the serviceaccount for hooks.
I'll investigate and document it in this PR.

Great - sounds good. Small concern that the permission required by an external webhook would be to edit AppRepository resources (not just the count field, so for example, the cred could be mis-used to change the source of the app repo), but not a big issue right now, and something that will be solved by open policy agent in the not-too-distant future (well, already, it's just not common to use it extensively yet).

Rel vmware-tanzu#2147

andresmgot · 2020-11-05T14:41:43Z

pkg/kube/kube_handler.go

 	appRepo, err := a.clientset.KubeappsV1alpha1().AppRepositories(requestNamespace).Get(context.TODO(), repoName, metav1.GetOptions{})
 	if err != nil {
 		return nil, err
 	}

-	// an updated is forced if the ResyncRequests property changes,
+	// An updated is forced if the ResyncRequests property changes,


An update

andresmgot · 2020-11-05T14:43:33Z

dashboard/src/shared/url.ts

@@ -80,8 +80,10 @@ export const backend = {
      backend.apprepositories.base(cluster, namespace),
    list: (cluster: string, namespace: string) => backend.apprepositories.base(cluster, namespace),
    validate: (cluster: string) => `${backend.apprepositories.base(cluster, "kubeapps")}/validate`,
-    delete: (cluster: string, name: string, namespace: string) =>
+    delete: (cluster: string, namespace: string, name: string) =>


you have changed the order of the parameters here in this delete but not in the places it's used, maybe you should leave this change out of this PR.

Totally agree, it was wrongly staged and committed while working on #2147. My fault!

Also fix minor typo

Since refreshing an apprepository no longer requires to get the repo to send an edition later, this test step (faulty) can be removed

Conflicts: dashboard/src/shared/AppRepository.ts

antgamdia · 2020-11-05T16:38:55Z

Comments from this PR addressed. Also edited a test step that is no longer required. Merged the latest master commits to integrate changes on #2147.

antgamdia added 2 commits November 3, 2020 17:54

Add a new API endpoint to force a refresh in a appRepository

8eebb11

Rel vmware-tanzu#2062 For instance, "POST /api/v1/clusters/default/namespaces/kubeapps/apprepositories/bitnami/refresh" will trigger a refresh in the "bitnami" appRepository in the namespace "kubeapps"

Change endpoint of appRepository refresh button

f3071f5

Rel vmware-tanzu#2062 Now it calls the new POST .../refresh endpoint implemented in the backend.

antgamdia requested a review from andresmgot November 3, 2020 17:31

absoludity reviewed Nov 3, 2020

View reviewed changes

andresmgot reviewed Nov 4, 2020

View reviewed changes

absoludity approved these changes Nov 5, 2020

View reviewed changes

antgamdia mentioned this pull request Nov 5, 2020

Change ordering in some arguments to "cluster, namespace, other args" #2147

Merged

antgamdia added 2 commits November 5, 2020 12:29

Change ordering in some arguments to "cluster, namespace, other args"

76f234d

Rel vmware-tanzu#2147

Remove unnecessary namespace check

3103533

andresmgot reviewed Nov 5, 2020

View reviewed changes

antgamdia added 3 commits November 5, 2020 16:28

Revert unnecessary change

7841872

Also fix minor typo

Remove unnecessary test step

34ea72e

Since refreshing an apprepository no longer requires to get the repo to send an edition later, this test step (faulty) can be removed

Merge branch 'master' into 2062-repoRefreshHook

45a4f80

Conflicts: dashboard/src/shared/AppRepository.ts

antgamdia mentioned this pull request Nov 6, 2020

Webhook documentation #2150

Merged

antgamdia merged commit 3c5fdf4 into vmware-tanzu:master Nov 9, 2020

antgamdia deleted the 2062-repoRefreshHook branch November 9, 2020 08:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a /refresh endpoint for forcing an update in an AppRepository #2138

Add a /refresh endpoint for forcing an update in an AppRepository #2138

antgamdia commented Nov 3, 2020

absoludity left a comment

absoludity Nov 3, 2020

andresmgot Nov 4, 2020

antgamdia Nov 4, 2020 •

edited

andresmgot Nov 4, 2020

absoludity Nov 3, 2020

andresmgot left a comment

andresmgot Nov 4, 2020

antgamdia Nov 4, 2020

andresmgot Nov 4, 2020

andresmgot Nov 4, 2020

andresmgot Nov 4, 2020

andresmgot Nov 4, 2020

antgamdia Nov 4, 2020

andresmgot commented Nov 4, 2020

antgamdia commented Nov 4, 2020

andresmgot commented Nov 4, 2020

absoludity left a comment

andresmgot Nov 5, 2020

andresmgot Nov 5, 2020

antgamdia Nov 5, 2020

antgamdia commented Nov 5, 2020

Add a /refresh endpoint for forcing an update in an AppRepository #2138

Add a /refresh endpoint for forcing an update in an AppRepository #2138

Conversation

antgamdia commented Nov 3, 2020

Description of the change

Benefits

Possible drawbacks

Applicable issues

Additional information

absoludity left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antgamdia Nov 4, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresmgot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresmgot commented Nov 4, 2020

antgamdia commented Nov 4, 2020

andresmgot commented Nov 4, 2020

absoludity left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antgamdia commented Nov 5, 2020

antgamdia Nov 4, 2020 •

edited