[Helm plugin] Fix for listing repos without cluster-wide permissions #5453

castelblanque · 2022-10-11T16:50:58Z

Signed-off-by: Rafa Castelblanque rcastelblanq@vmware.com

Description of the change

Before the PR, when a user with non-admin invoked:

GetPackageRepositorySummaries passing as context e.g. the namespace: kubeapps-user-namespace -> Returned a valid list of accesible repositories in that namespace
GetPackageRepositorySummaries passing as context no namespace (or namespace: "") -> Returned an error Forbidden to get the AppRepository 'all' due to.... This happened even when the user had permissions for AppRepository and get/Secret in some namespaces.

This is because when using non-admin RBACs, the Apprepos List over the whole cluster action requires a ClusterRole (since we are passing "" namespace)

After the PR:

Is unchanged.
When user performs GetPackageRepositorySummaries passing as context no namespace (or namespace: "") -> Gets a valid list of repositories from the namespaces in which the user has access.

Warning: This logic is not applying the kubeappsapis.pluginConfig.resources.packages.v1alpha1.trustedNamespaces.headerName and headerPattern, so it could be time-consuming for scenarios with many namespaces.

Benefits

No need for a ClusterRoleBinding in order to get the list of repositories cluster-wide. Logic now checks namespace by namespace.

Possible drawbacks

Slow with high number of namespaces.

Applicable issues

fixes Listing all repositories an unpriviledged users can access to #5215 for Helm plugin

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

netlify · 2022-10-11T16:51:06Z

✅ Deploy Preview for kubeapps-dev canceled.

Name	Link
🔨 Latest commit	`905899b`
🔍 Latest deploy log	https://app.netlify.com/sites/kubeapps-dev/deploys/634e7c6b3e26f500083828a4

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

antgamdia

Thanks! I guess you haven't had a chance to actually measure the overhead added by this solution, but I assume it is worth it (much better than an error 😅 )
Perhaps we can post it on slack, to double-check with some of our adopters using a large number of namespaces.

antgamdia · 2022-10-17T15:52:17Z

cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/repositories_test.go

+				}
+			}
+
+			s := newServerWithReactors(unstructuredObjects, nil, typedObjects, nil, tc.reactors)


antgamdia · 2022-10-17T15:56:42Z

cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/server_test.go

+	reaction k8stesting.ReactionFunc
+}
+
+func newServerWithReactors(unstructuredObjs []k8sruntime.Object, repos []*v1alpha1.AppRepository, typedObjects []k8sruntime.Object, typedClientReactions []*ClientReaction, dynClientReactions []*ClientReaction) *Server {


Maybe renaming newServerWithReactors to newServerWithAppRepoReactors ?

antgamdia · 2022-10-17T16:03:15Z

cmd/kubeapps-apis/plugins/pkg/resources/namespaces.go

+	return allowedNamespaces, nil
+}
+
+func FilterActiveNamespaces(namespaces []corev1.Namespace) []corev1.Namespace {


I guess you're moving this logic as it will be re-used by the kapp plugin, great then.

Indeed, it will be used by Kapp, Helm and Resources plugins.

beni0888

LGTM! I've just added some minor suggestions

cmd/kubeapps-apis/plugins/pkg/resources/namespaces.go

beni0888 · 2022-10-17T20:39:52Z

cmd/kubeapps-apis/plugins/pkg/resources/namespaces.go

+
+func nsCheckerWorker(client kubernetes.Interface, nsJobs <-chan checkNSJob, resultChan chan checkNSResult) {
+	for j := range nsJobs {
+		res, err := client.AuthorizationV1().SelfSubjectAccessReviews().Create(context.TODO(), &authorizationapi.SelfSubjectAccessReview{


Suggested change

res, err := client.AuthorizationV1().SelfSubjectAccessReviews().Create(context.TODO(), &authorizationapi.SelfSubjectAccessReview{

res, err := client.AuthorizationV1().SelfSubjectAccessReviews().Create(context.Background(), &authorizationapi.SelfSubjectAccessReview{

I would use context.Background() here instead of TODO which according to its documentation is like something temporary. But an even better solution would be to receive a context.Context parameter, so the calling function can pass it's own context and allow for cancellation.

You are right. This is a copied-pasted piece of code, so the whole block was left as-is.
Changed to context.Background().

beni0888 · 2022-10-17T20:58:11Z

cmd/kubeapps-apis/plugins/pkg/resources/namespaces_test.go

+			namespaces, err := FindAccessibleNamespaces(clusterTypedClient, inClusterClient, 1)
+
+			if tc.expectedErr != nil && err != nil {
+				if got, want := err.Error(), tc.expectedErr.Error(); !cmp.Equal(want, got) {


I would use testify library for the assertions.

beni0888 · 2022-10-17T21:00:52Z

cmd/kubeapps-apis/plugins/resources/v1alpha1/namespaces_filtering.go

+			return s.clusterServiceAccountClientGetter.Typed(ctx, cluster)
+		}
+		inClusterTypedClientFunc := func() (kubernetes.Interface, error) {
+			return s.localServiceAccountClientGetter.Typed(context.Background())


Just for the sake of knowledge, I don't understand why in this line you use context.Background() while above you're passing the existing ctx variable. Could you please explain the reason? 🙏🏻

Most of the times, clusterServiceAccountClientGetter will use the context from the incoming request (ctx in this case), whereas localServiceAccountClientGetter does not need it, as it will provide a so called "in-cluster" client that will use the pod's service account. This local getter will have then different RBAC applied than the incoming context.

Co-authored-by: Jesús Miguel Benito Calzada <bjesus@vmware.com> Signed-off-by: Rafa Castelblanque <67455978+castelblanque@users.noreply.github.com>

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

…eapps into 5215-listing-repos-rbac Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

Co-authored-by: Jesús Miguel Benito Calzada <bjesus@vmware.com> Signed-off-by: Rafa Castelblanque <67455978+castelblanque@users.noreply.github.com>

castelblanque · 2022-10-18T09:54:42Z

Thanks @beni0888 and @antgamdia for the review!

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

…eapps into 5215-listing-repos-rbac Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

Bumps [axios](https://github.com/axios/axios) from 1.2.2 to 1.2.3. <details> <summary>Release notes</summary> <p><em>Sourced from <a href="https://github.com/axios/axios/releases">axios's releases</a>.</em></p> <blockquote> <h2>1.2.3</h2> <h2>Release notes:</h2> <h3>Bug Fixes</h3> <ul> <li><strong>types:</strong> fixed AxiosRequestConfig header interface by refactoring it to RawAxiosRequestConfig; (<a href="https://github-redirect.dependabot.com/axios/axios/issues/5420">#5420</a>) (<a href="https://github.com/axios/axios/commit/08119634a22f1d5b19f5c9ea0adccb6d3eebc3bc">0811963</a>)</li> </ul> <h3>Contributors to this release</h3> <ul> <li><img src="https://avatars.githubusercontent.com/u/12586868?v=4&s=16" alt="avatar" /> <a href="https://github.com/DigitalBrainJS" title="+938/-442 ([#5456](axios/axios#5456) [#5455](axios/axios#5455) [#5453](axios/axios#5453) [#5451](axios/axios#5451) [#5449](axios/axios#5449) [#5447](axios/axios#5447) [#5446](axios/axios#5446) [#5443](axios/axios#5443) [#5442](axios/axios#5442) [#5439](axios/axios#5439) [#5420](axios/axios#5420) )">Dmitriy Mozgovoy</a></li> </ul> </blockquote> </details> <details> <summary>Changelog</summary> <p><em>Sourced from <a href="https://github.com/axios/axios/blob/v1.x/CHANGELOG.md">axios's changelog</a>.</em></p> <blockquote> <h2><a href="https://github.com/axios/axios/compare/1.2.2...1.2.3">1.2.3</a> (2023-01-10)</h2> <h3>Bug Fixes</h3> <ul> <li><strong>types:</strong> fixed AxiosRequestConfig header interface by refactoring it to RawAxiosRequestConfig; (<a href="https://github-redirect.dependabot.com/axios/axios/issues/5420">#5420</a>) (<a href="https://github.com/axios/axios/commit/08119634a22f1d5b19f5c9ea0adccb6d3eebc3bc">0811963</a>)</li> </ul> <h3>Contributors to this release</h3> <ul> <li><img src="https://avatars.githubusercontent.com/u/12586868?v=4&s=16" alt="avatar" /> <a href="https://github.com/DigitalBrainJS" title="+938/-442 ([#5456](axios/axios#5456) [#5455](axios/axios#5455) [#5453](axios/axios#5453) [#5451](axios/axios#5451) [#5449](axios/axios#5449) [#5447](axios/axios#5447) [#5446](axios/axios#5446) [#5443](axios/axios#5443) [#5442](axios/axios#5442) [#5439](axios/axios#5439) [#5420](axios/axios#5420) )">Dmitriy Mozgovoy</a></li> </ul> </blockquote> </details> <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/axios/axios/commit/557ed0a7489b1bf62296ea34568eeea8975ff4f9"><code>557ed0a</code></a> chore(ci): fixed publish action; (<a href="https://github-redirect.dependabot.com/axios/axios/issues/5470">#5470</a>)</li> <li><a href="https://github.com/axios/axios/commit/484e0b6ed24745df9cadaacc0fbf129114e70d00"><code>484e0b6</code></a> chore(release): v1.2.3 (<a href="https://github-redirect.dependabot.com/axios/axios/issues/5459">#5459</a>)</li> <li><a href="https://github.com/axios/axios/commit/d83316db4a242252db3a2ed7728cb43f8f8f4189"><code>d83316d</code></a> chore(ci): enabled npm publishing; (<a href="https://github-redirect.dependabot.com/axios/axios/issues/5460">#5460</a>)</li> <li><a href="https://github.com/axios/axios/commit/d750901deda2994a2d89643e8f18723cfb6b2732"><code>d750901</code></a> chore(ci): added an action to make GitHub & NPM releases when merging a relea...</li> <li><a href="https://github.com/axios/axios/commit/477c71427dc1d03e0f3dced0d65bd7c1b99fd900"><code>477c714</code></a> chore(ci): fixed error in generating changelog with unnecessary spaces; (<a href="https://github-redirect.dependabot.com/axios/axios/issues/5455">#5455</a>)</li> <li><a href="https://github.com/axios/axios/commit/e2a1e280f6dfbb4f11ad541dec9541cdbf760ab1"><code>e2a1e28</code></a> chore(ci): improved contributors & PRs sections generator; (<a href="https://github-redirect.dependabot.com/axios/axios/issues/5453">#5453</a>)</li> <li><a href="https://github.com/axios/axios/commit/18772ed8fdcd0768a9b520737d81283c04a273f8"><code>18772ed</code></a> chore(ci): improved logging; (<a href="https://github-redirect.dependabot.com/axios/axios/issues/5451">#5451</a>)</li> <li><a href="https://github.com/axios/axios/commit/259f5f5aaadfcaf7f3a3fe462d8b0dbbc8004962"><code>259f5f5</code></a> chore(ci): added step of generating a list of contributors for CHANELOG.md; (...</li> <li><a href="https://github.com/axios/axios/commit/d33a3deb82b808f109b598abbf39fd2a1f8da998"><code>d33a3de</code></a> chore(ci): added commit message config; (<a href="https://github-redirect.dependabot.com/axios/axios/issues/5447">#5447</a>)</li> <li><a href="https://github.com/axios/axios/commit/ebb9e814436d2f6c7cc65ffecb6ff013539ce961"><code>ebb9e81</code></a> chore(deps): bump json5 from 1.0.1 to 1.0.2 (<a href="https://github-redirect.dependabot.com/axios/axios/issues/5438">#5438</a>)</li> <li>Additional commits viewable in <a href="https://github.com/axios/axios/compare/1.2.2...v1.2.3">compare view</a></li> </ul> </details> <br /> [![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=axios&package-manager=npm_and_yarn&previous-version=1.2.2&new-version=1.2.3)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Fix for Helm plugin

ddcc14b

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

vmwclabot added the cla-not-required label Oct 11, 2022

castelblanque mentioned this pull request Oct 11, 2022

Listing all repositories an unpriviledged users can access to #5215

Closed

Fixed missing returned value

47c0892

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

castelblanque requested review from antgamdia and beni0888 October 13, 2022 11:00

castelblanque mentioned this pull request Oct 13, 2022

[Carvel plugin] Fix for listing repos without cluster-wide permissions #5460

Merged

Merge branch 'main' into 5215-listing-repos-rbac

c579c77

antgamdia approved these changes Oct 17, 2022

View reviewed changes

beni0888 approved these changes Oct 17, 2022

View reviewed changes

castelblanque and others added 5 commits October 18, 2022 11:17

Update cmd/kubeapps-apis/plugins/pkg/resources/namespaces.go

bbcb596

Co-authored-by: Jesús Miguel Benito Calzada <bjesus@vmware.com> Signed-off-by: Rafa Castelblanque <67455978+castelblanque@users.noreply.github.com>

Update cmd/kubeapps-apis/plugins/pkg/resources/namespaces.go

7c12f47

Co-authored-by: Jesús Miguel Benito Calzada <bjesus@vmware.com> Signed-off-by: Rafa Castelblanque <67455978+castelblanque@users.noreply.github.com>

Rename test method

5a57395

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

Merge branch '5215-listing-repos-rbac' of github.com:vmware-tanzu/kub…

6ce0563

…eapps into 5215-listing-repos-rbac Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

Update cmd/kubeapps-apis/plugins/pkg/resources/namespaces.go

ae59baf

Co-authored-by: Jesús Miguel Benito Calzada <bjesus@vmware.com> Signed-off-by: Rafa Castelblanque <67455978+castelblanque@users.noreply.github.com>

Rafa Castelblanque added 4 commits October 18, 2022 11:56

Fixed some problems

1051856

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

Merge branch '5215-listing-repos-rbac' of github.com:vmware-tanzu/kub…

712f3e9

…eapps into 5215-listing-repos-rbac Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

Merge branch 'main' into 5215-listing-repos-rbac

d84fa05

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

Undo failed change from PR

905899b

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

castelblanque merged commit ae9ea7c into main Oct 18, 2022

castelblanque deleted the 5215-listing-repos-rbac branch October 18, 2022 11:16

castelblanque mentioned this pull request Oct 18, 2022

Package repository management UI for users with restricted permissions #5480

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Helm plugin] Fix for listing repos without cluster-wide permissions #5453

[Helm plugin] Fix for listing repos without cluster-wide permissions #5453

castelblanque commented Oct 11, 2022

netlify bot commented Oct 11, 2022 •

edited

Loading

antgamdia left a comment

antgamdia Oct 17, 2022

antgamdia Oct 17, 2022

antgamdia Oct 17, 2022

castelblanque Oct 18, 2022

beni0888 left a comment

beni0888 Oct 17, 2022

castelblanque Oct 18, 2022

beni0888 Oct 17, 2022

beni0888 Oct 17, 2022

castelblanque Oct 18, 2022

castelblanque commented Oct 18, 2022

	res, err := client.AuthorizationV1().SelfSubjectAccessReviews().Create(context.TODO(), &authorizationapi.SelfSubjectAccessReview{
	res, err := client.AuthorizationV1().SelfSubjectAccessReviews().Create(context.Background(), &authorizationapi.SelfSubjectAccessReview{

[Helm plugin] Fix for listing repos without cluster-wide permissions #5453

[Helm plugin] Fix for listing repos without cluster-wide permissions #5453

Conversation

castelblanque commented Oct 11, 2022

Description of the change

Benefits

Possible drawbacks

Applicable issues

netlify bot commented Oct 11, 2022 • edited Loading

✅ Deploy Preview for kubeapps-dev canceled.

antgamdia 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

beni0888 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

castelblanque commented Oct 18, 2022

netlify bot commented Oct 11, 2022 •

edited

Loading