HELM-258: add enhancement for new namespace-scoped helm repo crd #949

zonggen · 2021-11-03T19:54:06Z

Defines and proposes a new namespace-scoped CRD for helm repositories.
See more details in https://issues.redhat.com/browse/HELM-258.

Ref: https://issues.redhat.com/browse/HELM-258
Signed-off-by: Allen Bai abai@redhat.com

zonggen · 2021-11-03T19:58:46Z

cc @sbose78 @pedjak @dperaza4dustbit @mmulholla @tisutisu

Defines and proposes a new namespace-scoped CRD for helm repositories. See more details in https://issues.redhat.com/browse/HELM-258. Ref: https://issues.redhat.com/browse/HELM-258 Signed-off-by: Allen Bai <abai@redhat.com>

slemeur · 2021-11-18T10:19:37Z

Hi @bparees and @runcom . Could we have a review for this please?
We've seen a lot of users asking for this capability and are looking at addressing it with 4.10
Thanks.

bparees

the new api itself doesn't seem controversial, but i'd like to see the rest of the EP fleshed out a little around the implications of introducing it (implications on tooling, user experience, console, upgrades/downgrades, testing scenarios, etc)

bparees · 2021-11-18T15:49:42Z

enhancements/helm3/namespace-scoped-helm-repository-crd.md

+
+## Summary
+
+Right now we support [`cluster-scoped` Helm repository CRD](https://github.com/openshift/api/blob/master/helm/v1beta1/0000_10-helm-chart-repository.crd.yaml#L17). This requires admin-level permissions when developers want to add a Helm repository in the OpenShift Console. We would like to add the ability to install Helm repository as a custom resource but with `scope: namespaced`.


are there any console, or other tooling integrations with the existing Cluster scoped resource, that will need to be updated to also look at the namespaced resources?

If so, i'd expect those to be called out in this EP and those teams to be tagged as reviewers.

I think Console team for sure @zonggen I would invite @christianvogt and @rohitkrai03 to review

Added more details to implementation details section.

they should be added as reviewers in the reviewer section w/ a blurb making it clear what aspect of this you want their review of.

Yes the console developer catalog would need to be updated to handle namespace scoped helm chart repositories.

bparees · 2021-11-18T15:50:58Z

enhancements/helm3/namespace-scoped-helm-repository-crd.md

+
+#### Removing a deprecated feature
+
+### Upgrade / Downgrade Strategy


presumably there's nothing that needs to be done here, but you should at least "N/A" the section to show us you've thought about it (same for all the other sections you left blank).

and there is at least the slight implication that if they downgrade, the namespaced instances will still exist, but nothing will be consuming them, presumably.

Thanks for the review! That is correct, if a user downgrades then the instances are not affected but the console will not be consuming/rendering them. Will update this and other blank fields.

Signed-off-by: Allen Bai <abai@redhat.com>

zonggen · 2021-11-18T19:01:14Z

enhancements/helm3/namespace-scoped-helm-repository-crd.md

+
+N/A
+
+#### Failure Modes


@dperaza4dustbit Not sure I understand your request on the failure mode for /api/helm/charts/index.yaml?namespace=foo endpoint, can you clarify so I can add to this section?

zonggen · 2021-11-18T19:02:07Z

Updated according to reveiws :)

bparees · 2021-11-18T19:10:57Z

enhancements/helm3/namespace-scoped-helm-repository-crd.md

+
+#### Story 1
+
+As an OpenShift developer, I would like to be able to add Helm repositories within my project without asking for an administrator's permissions


presumably only people w/ specific permission will be able to see this repository, it doesn't become globally visible, right?

What is the expected RBAC permission that will determine who can see this repository? project viewer? project editor? explicit role being granted to a user in that namespace?

presumably only people w/ specific permission will be able to see this repository, it doesn't become globally visible, right?

Yes, only users that have at least read access to that that namespace/project will be able to see.

bparees · 2021-11-18T19:12:53Z

enhancements/helm3/namespace-scoped-helm-repository-crd.md

+
+#### How would the UI render the namespace-scoped Helm repository
+
+As defined in [previous helm enhancement](https://github.com/openshift/enhancements/blob/master/enhancements/helm3/console.md#how-would-the-ui-discover-the-charts), the UI will invoke the `/api/helm/charts/index.yaml` endpoint and the endpoint will proxy requests to the configured chart repositories.


hm, so will /api/help/charts/index.yaml do any filtering/rbac? or everyone will see the charts provided by any ProjectHelmChartRepository resource defined in any namespace in the cluster?

The dev console backend will authenticate request with token: https://github.com/openshift/console/blob/64f95b6f5338950765a0b1b7dc6eb866757207eb/pkg/auth/auth_openshift.go#L176

However, you are correct on this assumption, the /api/help/charts/index.yaml API will fetch whatever the user has requested as long as the token is valid, including the Helm repositories in other namespaces.

However in implementation, the openshift-console will be the one sending this request. So the backend will not know the original user of the request (the human that is accessing the dev console). We might want to add another query parameter like ?user=user1 to return only the helm repositories that are available to this user1, not any helm repository user1 requested.

Need some insights from dev console team members as well and any suggestion is welcomed.

cc @dperaza4dustbit @christianvogt @rohitkrai03

I assumed we would provide namespace=<namespace query parameter to the api call and therefore only receive helm charts which are both cluster and namespace scoped. If no namespace supplied, then only cluster scoped resources are returned.

From the console backend you should have access to the user which invoked the console backend api.

cc @serenamarie125

I assumed we would provide namespace=<namespace query parameter to the api call and therefore only receive helm charts which are both cluster and namespace scoped. If no namespace supplied, then only cluster scoped resources are returned.

Agreed, this aligns with my understanding.

I thought twice about the RBAC, please correct me if I'm wrong:

For the user RBAC, no extra RBAC policy is added in this enhancement. I would assume the RBAC is done purely based on the bearer token in the HTTP request and if the token/user/role of user does not have access to the CRD in the requested namespace, then the resource will not be responsed back.

Yes, this makes sense.

@zonggen should it mentioned that /api/helm/charts/index.yaml needs to pass also namespace as the argument, for example:

/api/helm/charts/index.yaml?namespace=foo or

/api/helm/charts/foo/index.yaml

@pedjak Thanks for the comments. I've made further clarifications in this section on the URL parameter namespace.

bparees · 2021-11-18T19:14:31Z

enhancements/helm3/namespace-scoped-helm-repository-crd.md

+
+As defined in [previous helm enhancement](https://github.com/openshift/enhancements/blob/master/enhancements/helm3/console.md#how-would-the-ui-discover-the-charts), the UI will invoke the `/api/helm/charts/index.yaml` endpoint and the endpoint will proxy requests to the configured chart repositories.
+
+Then the chart repository proxy will use all configured chart repositories, including cluster-scoped and available namespace-scoped Helm repositories in the current namespace, and deliver to the UI an aggregated index file. The UI then renders the developer catalog with the aggregated index file.


seems like yes(everyone will see everyone else's charts/repositories), but that also seems like questionable behavior.

While it appears the helm chart repositories have no authentication mechanism anyway (since there's no way to provide creds on the resource), i still wouldn't want some malicious user to provide a repository of exploitative charts that i can then trick other users into installing.

or even just spamming the cluster with a million charts, or offensively named charts, or other possibilities.

Yes this seems exploitative and not what I expected when namespace scoped helm chart repos was first discussed. I'll defer to PM on intent.

( disclosure: coming in as a reviewer today )

Does this imply, that the aggregated view of available charts are composed of

cluster-scoped charts already available to the user as of today

namespace-scoped charts from the ProjectHelmChartRepository in the current namespace only?

I would say yes @sbose78 the main idea is to enable developers to create their own repo in the namespace they have access to and are scoped at the moment

This should work like any other namespace scope resource like for example ConfigMap. If I create in namespace A I should not be able to see it when scoped to namespace B and I will only be able see it if I have access to list/read from namespace A

i misread. If the a call to /api/helm/charts/index.yaml is made without a namespace scope, then as long as only the cluster scoped repos are returned and not all namespaced repos.

Signed-off-by: Allen Bai <abai@redhat.com>

sbose78 · 2021-11-23T13:02:48Z

@zonggen could you add details on whether you intend to aggregate any cluster role in order to enable users ootb ?

zonggen · 2021-11-23T14:39:12Z

could you add details on whether you intend to aggregate any cluster role in order to enable users ootb ?

No I don't intend to aggregate cluster roles in this feature, and I don't think we aggregated roles in the previous cluster-wise helm repository CRD either. Will make a note in the enhancement.

Signed-off-by: Allen Bai <abai@redhat.com>

zonggen · 2021-11-29T14:24:46Z

Ready for another round of review :)

bparees · 2021-11-29T19:03:41Z

enhancements/helm3/namespace-scoped-helm-repository-crd.md

+
+![Namespaced Helm Repo Console UI](../helm3/assets/namespace_helm_repo_console_ui.png)
+
+[HELM-244](https://issues.redhat.com/browse/HELM-244): Extend the `/api/helm/charts/index.yaml` API endpoint in OpenShift Dev Console backend to support query parameter `namespace`. For example, a GET request to `/api/helm/charts/index.yaml?namespace=foo` will respond an aggregated `index.yaml` file with entities extracted from both cluster scoped Helm repository and Helm repositories in `foo` namespace.


so if a user wants to see all the charts in all the repos they have access to, they'd have to keep switching namespaces? and/or the console would have to make multiple calls to this api(one for each NS the user has access to) and aggregate the results?

what permissions are required to invoke /api/helm/charts/index.yaml and /api/helm/charts/index.yaml?namespace=foo ?

Yes, like other namespaced CRD, a user needs to switch namespace or pass the --namespace flag to access the resource. The usecase for the console API is to display the corresponding helm repositories that user selected in the dev console (the value of dropdown "Project: xxx"). If we need to support displaying multiple namespaces at the same time, we need to return aggregated helm repo in all requested namespaces (this includes the "all NS" case). This enhancement is the first step of supporting namespaced helm repo CRD, which only allows a single namespace in the url parameter.

No extra RBAC handling is applied to the /api/helm/charts/index.yaml endpoint. So this endpoint authenticate user token just like other API dev console endpoints. However, if the user/token does not have permission to the namespace/CRD, the List CRD request is rejected by k8s go-client calls: https://github.com/openshift/console/pull/10467/files#diff-c8e8620a353bc6becb91e8b2f168e723cdc23d3c59976a93fd5813edbf8990e3R230

Don't think the idea is to aggregate all chart repos in all namespaces the user has access to, at least not in this first pass. It is simply the union of repos a cluster scope with all the repos under the current namespace. The feature is scoped to enable members of a project to be able to play with helm charts in the current project without needing admin access to k8 cluster. My assumption is that the user has a role that allows them to have read/list access to ProjectHelmChartRepositoy resource to use the repo and create access to be able to create the repo. A project admin will control that and the helm backend will enforce it.

If a user does not have rights to access requested namespace, what will be returned on /api/helm/charts/index.yaml?namespace=foo ?

@pedjak Only available resouce will be returned, e.g. if the user does not have permission for the namespace=foo, then the default cluster-wise Helm repos will be returned, and if no cluster-wise repo is available, an empty list will be returned. Of course we will log the error in the backend but will not explicitly return error to the frontend component. This behavior is captured in the implementation PR: openshift/console#10467.

Let's please include the above information in the proposal too.

Updated with more details.

zonggen · 2021-12-09T14:58:50Z

Please let me know if more changes are needed in order to move this forward :)

christianvogt · 2021-12-09T18:06:56Z

lgtm

slemeur · 2021-12-16T15:38:56Z

@pedjak Could you also review?
@sbose78 What do you think about the current state of the proposal?

slemeur · 2021-12-16T15:39:19Z

@bparees : Any chance we can get that reviewed with the latest changes/comments?

dperaza4dustbit

lgtm

bparees · 2021-12-16T16:47:22Z

my security concerns are addressed and you've pulled the console team in for their awareness, so i'm good w/ this at this point.

Signed-off-by: Allen Bai <abai@redhat.com>

sbose78

Looks good to me, made some minor recommendations on making the proposal a little more detailed.

sbose78 · 2021-12-20T01:36:54Z

enhancements/helm3/namespace-scoped-helm-repository-crd.md

+
+N/A
+
+#### Tech Preview -> GA


Is the API going to be GA right away? ( I'm fine with that - since we've had the Cluster-scoped API around for a while now ).

Either way, this section or the one above it deserves a mention.

Agreed, added a sentence stating it's fully backward compatible, therefore will be released immediately.

sbose78 · 2021-12-20T01:37:29Z

enhancements/helm3/namespace-scoped-helm-repository-crd.md

+
+![Namespaced Helm Repo Console UI](../helm3/assets/namespace_helm_repo_console_ui.png)
+
+[HELM-244](https://issues.redhat.com/browse/HELM-244): Extend the `/api/helm/charts/index.yaml` API endpoint in OpenShift Dev Console backend to support query parameter `namespace`. For example, a GET request to `/api/helm/charts/index.yaml?namespace=foo` will respond an aggregated `index.yaml` file with entities extracted from both cluster scoped Helm repository and Helm repositories in `foo` namespace.


Let's please include the above information in the proposal too.

slemeur · 2021-12-20T08:24:59Z

Thanks everyone for the review.
/assign @frobware

Signed-off-by: Allen Bai <abai@redhat.com>

zonggen · 2021-12-20T15:05:52Z

@sbose78 Thank you for the reviews, I've updated according to your comments.

zonggen · 2021-12-22T19:37:55Z

Again please let me know if anything else needs to be addressed :)

bparees · 2022-01-05T21:12:24Z

/approve

openshift-ci · 2022-01-05T21:12:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bparees, sbose78

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [bparees]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

bparees · 2022-01-05T21:13:33Z

/lgtm

openshift-ci · 2022-01-05T21:20:47Z

@zonggen: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

openshift-ci bot requested review from bparees and runcom November 3, 2021 19:54

zonggen force-pushed the helm-namespace-repo-crd branch from b5af2ed to 08d0977 Compare November 3, 2021 20:08

helm3: add enhancement for new namespace-scoped helm repo crd

3336863

Defines and proposes a new namespace-scoped CRD for helm repositories. See more details in https://issues.redhat.com/browse/HELM-258. Ref: https://issues.redhat.com/browse/HELM-258 Signed-off-by: Allen Bai <abai@redhat.com>

zonggen force-pushed the helm-namespace-repo-crd branch from 08d0977 to 3336863 Compare November 3, 2021 20:09

bparees reviewed Nov 18, 2021

View reviewed changes

helm: update according to reviews

19ef686

Signed-off-by: Allen Bai <abai@redhat.com>

zonggen force-pushed the helm-namespace-repo-crd branch from d3992b2 to 19ef686 Compare November 18, 2021 18:59

zonggen commented Nov 18, 2021

View reviewed changes

bparees reviewed Nov 18, 2021

View reviewed changes

helm: add reviewers in metadata

427d480

Signed-off-by: Allen Bai <abai@redhat.com>

helm3: note on the clusterrole aggregation

5e8ec91

Signed-off-by: Allen Bai <abai@redhat.com>

bparees reviewed Nov 29, 2021

View reviewed changes

zonggen mentioned this pull request Dec 13, 2021

helm: add a new namespaced CRD for helm openshift/api#1084

Merged

dperaza4dustbit reviewed Dec 16, 2021

View reviewed changes

helm3: clarification on namespace URL parameter

36bb5fd

Signed-off-by: Allen Bai <abai@redhat.com>

sbose78 approved these changes Dec 20, 2021

View reviewed changes

openshift-ci bot assigned frobware Dec 20, 2021

helm: add more details according to comments

622acc0

Signed-off-by: Allen Bai <abai@redhat.com>

frobware removed their assignment Dec 22, 2021

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 5, 2022

openshift-ci bot assigned bparees Jan 5, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 5, 2022

openshift-merge-robot merged commit 163c6c6 into openshift:master Jan 5, 2022


		## Summary

		Right now we support [`cluster-scoped` Helm repository CRD](https://github.com/openshift/api/blob/master/helm/v1beta1/0000_10-helm-chart-repository.crd.yaml#L17). This requires admin-level permissions when developers want to add a Helm repository in the OpenShift Console. We would like to add the ability to install Helm repository as a custom resource but with `scope: namespaced`.


		#### Removing a deprecated feature

		### Upgrade / Downgrade Strategy


		#### Story 1

		As an OpenShift developer, I would like to be able to add Helm repositories within my project without asking for an administrator's permissions


		#### How would the UI render the namespace-scoped Helm repository

		As defined in [previous helm enhancement](https://github.com/openshift/enhancements/blob/master/enhancements/helm3/console.md#how-would-the-ui-discover-the-charts), the UI will invoke the `/api/helm/charts/index.yaml` endpoint and the endpoint will proxy requests to the configured chart repositories.


		As defined in [previous helm enhancement](https://github.com/openshift/enhancements/blob/master/enhancements/helm3/console.md#how-would-the-ui-discover-the-charts), the UI will invoke the `/api/helm/charts/index.yaml` endpoint and the endpoint will proxy requests to the configured chart repositories.

		Then the chart repository proxy will use all configured chart repositories, including cluster-scoped and available namespace-scoped Helm repositories in the current namespace, and deliver to the UI an aggregated index file. The UI then renders the developer catalog with the aggregated index file.


		![Namespaced Helm Repo Console UI](../helm3/assets/namespace_helm_repo_console_ui.png)

		[HELM-244](https://issues.redhat.com/browse/HELM-244): Extend the `/api/helm/charts/index.yaml` API endpoint in OpenShift Dev Console backend to support query parameter `namespace`. For example, a GET request to `/api/helm/charts/index.yaml?namespace=foo` will respond an aggregated `index.yaml` file with entities extracted from both cluster scoped Helm repository and Helm repositories in `foo` namespace.

HELM-258: add enhancement for new namespace-scoped helm repo crd #949

HELM-258: add enhancement for new namespace-scoped helm repo crd #949

Conversation

zonggen commented Nov 3, 2021

zonggen commented Nov 3, 2021

slemeur commented Nov 18, 2021

bparees 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

Choose a reason for hiding this comment

zonggen commented Nov 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zonggen Nov 18, 2021 • edited Loading

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

bparees Nov 18, 2021 • edited Loading

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

sbose78 commented Nov 23, 2021

zonggen commented Nov 23, 2021

zonggen commented Nov 29, 2021

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

zonggen commented Dec 9, 2021

christianvogt commented Dec 9, 2021

slemeur commented Dec 16, 2021

slemeur commented Dec 16, 2021

dperaza4dustbit left a comment

Choose a reason for hiding this comment

bparees commented Dec 16, 2021

sbose78 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

slemeur commented Dec 20, 2021

zonggen commented Dec 20, 2021

zonggen commented Dec 22, 2021

bparees commented Jan 5, 2022

openshift-ci bot commented Jan 5, 2022

bparees commented Jan 5, 2022

openshift-ci bot commented Jan 5, 2022

zonggen Nov 18, 2021 •

edited

Loading

bparees Nov 18, 2021 •

edited

Loading