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

Design proposal for implementing helm support in OpenShift Console #175

Merged

Conversation

sbose78
Copy link
Contributor

@sbose78 sbose78 commented Jan 9, 2020

This is a design follow-up to the requirements put out in #153 .

Already implemented

What's new

  • Representation of Helm chart repositories in Kubernetes, a new HelmChartRepository CRD is being proposed to be added to openshift/api. Parallel work is being with ACM team ( Josh ) and in Helm upstream to drive consensus.

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 9, 2020
@sbose78
Copy link
Contributor Author

sbose78 commented Jan 9, 2020


![Helm Charts Repo Service](../helm3/assets/charts-repo.png)

## How would disconnected installs work

1. The user would need to 'clone' the public Github repository into her inside-the-network Github or Gitlab instance, and configure the same to serve static content ( "Pages" ).
Copy link

@pedjak pedjak Jan 10, 2020

Choose a reason for hiding this comment

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

In general, it is required to serve chart repo content via a HTTP server, and GitHub/GitLab do that very well, directly from a given git repo. Another easy option would be to clone the content over the fence, and serve it using a containerized http server, e.g. nginx - or even provide OpenShift BuildConfig inside the git repo, so that users can build their own chart repo service, running inside the cluster.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good to me.


Here's how the control flow would look like:

1. The Console UI will invoke the `/api/console/helm/install` endpoint on the Console Backend.
Copy link

Choose a reason for hiding this comment

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

I would suggest that we drop console from the endpoints, and make them more REST. In that sense, how about:

  • POST, PUT, DELETE /api/helm/release with a JSON payload should install a given Helm release out of selected chart or update/delete a given Helm release
  • GET /api/helm/releases returns the list of available releases
  • /api/helm/charts proxies requests for the declared Helm chart repository

Copy link
Contributor Author

@sbose78 sbose78 Jan 15, 2020

Choose a reason for hiding this comment

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

I'm good. @spadgett @benjaminapetersen ?

My concern is that POST, PUT, DELETE /api/helm/release doesn't make the action look too obvious.

There might be multiple helm actions which modify a release, hence PUT might be a difficult way to express it.

Copy link

Choose a reason for hiding this comment

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

using single path with different http methods is a standard restful design.


## How would the UI discover the charts

1. The UI would invoke `/api/console/helm/charts/index.yaml` to populate the available charts in the developer catalog.
Copy link

Choose a reason for hiding this comment

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

the implemented endpoint is /api/helm/charts/index.yaml


1. The user would need to 'clone' the public Github repository into her inside-the-network Github or Gitlab instance, and configure the same to serve static content ( "Pages" ).

2. The URL serving the above static content would need to be configured in the cluster's Console Settings. The value would be propagated to the console backend through an environment variable.
Copy link

Choose a reason for hiding this comment

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

Configuring Helm repository location can be modelled similar to OperatorSource. The resource needs to contain the following field:

url: http://my.chart-repo.org/stable

# optional and only needed for UI purposes
displayName: myChartRepo

# optional and only needed for UI purposes
description: my private chart repo

# set to true if need to be disabled temporarly
disabled: true

The resource could be standalone, a part of Console config, or a part of Console operator config

  • If standalone, its could be named as HelmChartRepository and be placed in (cluster-wide config)[https://github.com/openshift/api/tree/master/config/v1] or console operator namespace. The console operator would watch for changes on instances and reconfigure the console deployment.
  • If a part of existing resource, it should be possible to specify a list of chart repositories, so that future use cases (e.g. ODC-2994) could be implemented.

The console backend already implements a few helm endpoints (including the chart proxy), but the future might require to extract them into a separate service (e.g. to make openshift more modular). Hence, it would be good to define the configuration as the standalone resource (possibly cluster-wide or in a dedicated namespace, e.g. openshift-helm) detaching API from the implementation.

2. The API handler for `/api/console/helm/install` will, in turn talk to the API server ( no tiller! ) using the user's authentication, while leveraging the Helm Golang API.

This is in-line with the "Console is a pretty kubectl" philosophy since Helm itself is a thin layer on top of kubectl.

Choose a reason for hiding this comment

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

You should add an Alternatives section here (see the template). Several things have been discussed in other conversations:
A. Additions to console-operator config. (the PR you have open in openshift/api).
B. A dedicated top-level Helm config (that console-operator or another operator would read).
C. An OLM based approach for Helm config.

And pro/con these to clarify the reason for the current approach.


2. The configuration could be embedded into [`Console` operator config](https://github.com/openshift/api/blob/master/operator/v1/types_console.go#L26)

Such approach shares the similar issues with the previous alternative. Discovering the config by cluster admins is even harder, it would live in `openshift-console-operator` namespace.

Choose a reason for hiding this comment

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

Console operator config is cluster scoped, just like console config.

Such approach shares the similar issues with the previous alternative. Discovering the config by cluster admins is even harder, it would live in `openshift-console-operator` namespace.

3. OLM operator for Helm endpoints. Helm configuration would move to the top-level resource inside `openshift-helm` namespace, along with the service implementing the endpoints.

Copy link

@benjaminapetersen benjaminapetersen Mar 23, 2020

Choose a reason for hiding this comment

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

I think all operators from OLM are installed into -n openshift-operators, though perhaps this is configurable.

Copy link
Contributor

Choose a reason for hiding this comment

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

they are not. global operators, by default, are installed there, but even that is just a default not a requirement.

single namespace operators are installed in the namespace the admin wants them in.

Due to future federated usecases ([ODC-2994](https://issues.redhat.com/browse/ODC-2994))), a cluster admin should be able to declare multiple chart repositories.
Such list could be embedded (cluster-wide, top-level `Helm` resource)[https://github.com/openshift/api/tree/master/config/v1]:

```yaml

Choose a reason for hiding this comment

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

Ok, so advocating a top level Helm resource, with alternatives below on console config, console operator config, or via OLM.

@sbose78
Copy link
Contributor Author

sbose78 commented Mar 23, 2020

Updated the proposal with more details on how the OLM operator approach could look like.

pedjak added a commit to pedjak/openshift-api that referenced this pull request Mar 30, 2020
* Introduced `HelmChartRepository` top-level CR
* The corresponding enchancement: openshift/enhancements/pull/175

1. The UI would invoke `/api/helm/charts/index.yaml` endpoint to get [the list of all available charts](https://helm.sh/docs/topics/chart_repository/#the-index-file) so that the available charts can be rendered in the developer catalog.

2. The above endpoint would proxy requests to the configured charts repository
Copy link
Contributor

Choose a reason for hiding this comment

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

a proxy surprises me. Why wouldn't you just directly access the charts repo?

Copy link
Contributor Author

@sbose78 sbose78 Mar 31, 2020

Choose a reason for hiding this comment

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

The charts repo URL is supposed to be configurable. Console UI therefore always talks to one console backend URL which proxies to the configured charts repo URL.

The UI doesn't have to be aware as long as the Console Backend knows what the chart repo URL is.

Note, this change is already in master and is being shipped in 4.4.

Copy link
Member

Choose a reason for hiding this comment

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

and that interaction with external URL off cluster will go through configured ocp cluster egress proxy?

Copy link
Contributor

Choose a reason for hiding this comment

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

so there's only one chart repository per cluster? or is the proxy aggregating multiple chart repositories?

Copy link

Choose a reason for hiding this comment

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

so there's only one chart repository per cluster? or is the proxy aggregating multiple chart repositories?

@bparees at the moment, yes, but we already have uses cases that would require aggregation from multiple repos.

and that interaction with external URL off cluster will go through configured ocp cluster egress proxy?

The enhancement does not enforce that, but it could be added by cluster admins orthogonally, if needed, correct @derekwaynecarr ?

Copy link
Contributor

Choose a reason for hiding this comment

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

The enhancement does not enforce that, but it could be added by cluster admins orthogonally, if needed, correct @derekwaynecarr ?

Seems like the enhancement has to enforce that. If this EP is defining a new component which has requirements to make external requests, the new component must respect the global cluster proxy configuration.

Copy link
Contributor

Choose a reason for hiding this comment

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

@bparees at the moment, yes, but we already have uses cases that would require aggregation from multiple repos.

it doesn't seem like this EP defines an api that allows for that (the api seems to be "single instance of a cluster scoped resource, and the resource instance only has a field for a single chart repository url")

Copy link

Choose a reason for hiding this comment

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

If this EP is defining a new component which has requirements to make external requests,

This EP defines a CR and a few endpoints that could be implemented either by existing or a new component.

The reality is that the endpoints got implemented by console backend already.

Copy link

Choose a reason for hiding this comment

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

it doesn't seem like this EP defines an api that allows for that (the api seems to be "single instance of a cluster scoped resource, and the resource instance only has a field for a single chart repository url")

We are proposing a new top-level CR (HelmChartRepository) that could be instantiated multiple-times. If cluster-scoped resources could be instantiated only once, we can introduce a CR that manages a list of repos (see #175 (comment)) or move CR to helm.openshift.io group (as suggested/asked by @derekwaynecarr )

Copy link
Contributor

Choose a reason for hiding this comment

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

If cluster-scoped resources could be instantiated only once

they can be instantiated multiple times, but since your example named the resource cluster and i thought there was some discussion around only having one instance, i drew the conclusion you intended to only consume that single special instance. If you're going to consume all instances, then i guess that's fine.

A cluster-scoped, top-level `HelmRepository` resource)[https://github.com/openshift/api/tree/master/config/v1] should be added:

```yaml
apiVersion: config.openshift.io/v1
Copy link
Contributor

Choose a reason for hiding this comment

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

I would not expect it in this group. If this is only for consumption by the console, I would expect it localized in some way. Does any other team need to interact with this?

Copy link
Contributor Author

@sbose78 sbose78 Mar 31, 2020

Choose a reason for hiding this comment

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

What do you suggest having instead as the group?

At the moment, it is only the Console operator controller which would consume the information to configure the console Deployment.

Conceptually, it is not part of Console. In future, there might be other consumers too.

Copy link

Choose a reason for hiding this comment

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

We are aiming at cluster wide configuration, as we agreed with @derekwaynecarr the last week.

Copy link
Contributor

@deads2k deads2k Apr 7, 2020

Choose a reason for hiding this comment

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

We are aiming at cluster wide configuration, as we agreed with @derekwaynecarr the last week.

A cluster-wide configuration that is later accessed by a proxy in in the console reads really unusually.

Unless the proxy is just a single way of consuming this, it sounds like you're creating a console API as opposed to the peer of an OperatorHub source.

Similarly, since only one instance is allowed, I would expect multiple possible sources for charts. This API appears to be built to have multiple instances instead of a single cluster instance.

If it can be consumed outside of a console proxy and if is designed to have multiple chart sources referenced in a single cluster instance (basically if it's a peer to OperatorHub) then it makes more sense

Copy link
Member

Choose a reason for hiding this comment

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

@pedjak I know we met prior, and I am now caught up on the enhancement.

instead of config.openshift.io, should we instead just a dedicated helm.openshift.io group?

if its in config.openshift.io, i agree that w/ @deads2k that it needs to look more like OperatorHub (singular).

do you foresee other helm related APIs getting added in the future that would warrant a helm.openshift.io group?

Copy link

Choose a reason for hiding this comment

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

instead of config.openshift.io, should we instead just a dedicated helm.openshift.io group?

@derekwaynecarr Is the implicit restriction of config.openshit.io group that we can have single CR instance? I am not against moving it to helm.openshift.io, but then we are going to loose the ability to edit/configure it via UI, i.e. Administration/Cluster Settings/Global Configuration, correct?

do you foresee other helm related APIs getting added in the future that would warrant a helm.openshift.io group?

Not really - we have already added IMHO the majority of needed API. However, if we would like to keep a few placeholders for future config options, and if config.openshift.io can handle only singletons, we could define then Helm top-level CR, that would contain for the moment a list of chart repos only, something like this:

apiVersion: config.openshift.io/v1
kind: Helm
metadata:
  name: cluster
spec:
  chartRepositories:
    - url: http://my.chart-repo.org/stable

      # optional and only needed for UI purposes
      displayName: myChartRepo

      # optional and only needed for UI purposes
      description: my private chart repo

      # set to true if need to be disabled temporarly
      disabled: true

Does it make sense @deads2k ?

Copy link
Contributor

@deads2k deads2k Apr 8, 2020

Choose a reason for hiding this comment

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

@derekwaynecarr Is the implicit restriction of config.openshit.io group that we can have single CR instance?

Yes. excepting clusteroperators and clusterversion, everything else is a single instance named cluster.

I am not against moving it to helm.openshift.io, but then we are going to loose the ability to edit/configure it via UI, i.e. Administration/Cluster Settings/Global Configuration, correct?

Seems like you'd just update the glass to edit them in the same spot. This will eventually happen to you when someone external makes an awesome optional component.

Note, the helm charts' repository configuration today exists as a console configuration, which enables Console to proxy to the Helm chart repository URL. Moving it out of Console is outside the scope of this section.

* The default helm chart repository URL remains unchanged in the Console configuration.
* Admin installs an OLM operator which only provides a `HelmConfig` cluster-scoped CRD
Copy link
Contributor

Choose a reason for hiding this comment

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

so OLM already has a peer concept or you're saying you could create such a peer concept? I can't tell if this is an existing thing or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an alternative path we don't plan on pursuing.

I'm sorry, what do you mean by the peer concept - operator X's controller watching operator Y's CRD ?


* The default helm chart repository URL remains unchanged in the Console configuration.
* Admin installs an OLM operator which only provides a `HelmConfig` cluster-scoped CRD
* Admin creates a cluster-scoped CR. Note, this isn't very intuitive for the Admin.
Copy link
Contributor

Choose a reason for hiding this comment

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

this looks spurious. It's equivalent to what you're trying to do here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here the admin configures a cluster-wide helm chart repository by creating a CR with the details.

Copy link
Member

Choose a reason for hiding this comment

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

i dont understand how this is not intuitive. this is equivalent to config.openshift.io use case.

Copy link

Choose a reason for hiding this comment

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

@derekwaynecarr , an admin need to understand that changing the default chart repo location requires installing an OLM operator.

pedjak added a commit to pedjak/openshift-api that referenced this pull request Apr 1, 2020
…uster

* Introduced `HelmChartRepository` top-level CR
* The corresponding enchancement: openshift/enhancements/pull/175
Copy link
Member

@derekwaynecarr derekwaynecarr left a comment

Choose a reason for hiding this comment

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

I would like to understand how the function would work when not tied to the console, but where we want to provide helm related functionality. We can then understand if we can make short term decisions that do not conflict with longer term goals.


1. The UI would invoke `/api/helm/charts/index.yaml` endpoint to get [the list of all available charts](https://helm.sh/docs/topics/chart_repository/#the-index-file) so that the available charts can be rendered in the developer catalog.

2. The above endpoint would proxy requests to the configured charts repository
Copy link
Member

Choose a reason for hiding this comment

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

and that interaction with external URL off cluster will go through configured ocp cluster egress proxy?

A cluster-scoped, top-level `HelmRepository` resource)[https://github.com/openshift/api/tree/master/config/v1] should be added:

```yaml
apiVersion: config.openshift.io/v1
Copy link
Member

Choose a reason for hiding this comment

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

@pedjak I know we met prior, and I am now caught up on the enhancement.

instead of config.openshift.io, should we instead just a dedicated helm.openshift.io group?

if its in config.openshift.io, i agree that w/ @deads2k that it needs to look more like OperatorHub (singular).

do you foresee other helm related APIs getting added in the future that would warrant a helm.openshift.io group?

disabled: true
```

The console operator would watch for changes on it and reconfigure the chart repository proxy. The console backend already implements a few helm endpoints (including the chart proxy), but the future might require to extract them into a separate service (e.g. to make openshift more modular). Hence, it would be good to define the configuration as the top-level resource detaching API from the implementation. Cluster admin would easily discover the new resource and manage it either via CLI or through UI.
Copy link
Member

Choose a reason for hiding this comment

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

the modularity approach is fine with just a helm.openshift.io group similar to template.openshift.io.

Copy link
Contributor

Choose a reason for hiding this comment

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

seems odd to me that the console operator would be responsible for configuring the chart repository proxy.

Is the chart repository proxy not going to have its own operator that manages its config?

Copy link

Choose a reason for hiding this comment

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

the modularity approach is fine with just a helm.openshift.io group similar to template.openshift.io.

Not against, but do we then loose ability to configure/manage chart repos from UI?

seems odd to me that the console operator would be responsible for configuring the chart repository proxy.

@bparees the statement was written after we already added the endpoints to the console backend.

Is the chart repository proxy not going to have its own operator that manages its config?

If we ever decide to extract the introduced Helm endpoints from the console backend into a standalone service, we should also accompanied it with its own operator.


#### 1. The configuration could be embedded into cluster-wide [`Console` config](https://github.com/openshift/api/blob/master/config/v1/types_console.go#L26)

Admins wouldn't be able to intuitively discover the operator config as a way to configure the Helm repository URLs. It becomes closely coupled with the console. Extracting Helm endpoints into a separate service would require moving the config as well.
Copy link
Member

Choose a reason for hiding this comment

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

i agree this unnecessarily couples things that need not be forever coupled.

Conceptually, the Helm repository URL isn't really an operator configuration, hence this doesn't feel like the right place.
This approach would have similar issues with the previous alternative - admins wouldn't be able to intuitively discover the operator config as a way to configure the Helm repository URLs.

#### 3. OLM operator for Helm Configuration.
Copy link
Member

Choose a reason for hiding this comment

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

i view this as trying to determine who installs the helm related crd.

i feel like there is an option that wasn't discussed, but this is very similar to templates.

we have template.openshift.io, and we have a set of templates installed ootb.

can you add some language contrasting your use case with existing patterns around templates?

Copy link

Choose a reason for hiding this comment

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

I was not aware of that option.

We could add Helm chart repo config to a dedicated helm.openshift.io group and watch on changes there, but are we able to to manage the instance within UI? I could not find that option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but are we able to to manage the instance within UI?

We can make an explicit change in the UI code to do that.


* The default helm chart repository URL remains unchanged in the Console configuration.
* Admin installs an OLM operator which only provides a `HelmConfig` cluster-scoped CRD
* Admin creates a cluster-scoped CR. Note, this isn't very intuitive for the Admin.
Copy link
Member

Choose a reason for hiding this comment

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

i dont understand how this is not intuitive. this is equivalent to config.openshift.io use case.

* Creation of the cluster-scoped `HelmConfig` CR may not be very intuitive for the admin unless we show it in the Cluster Configuration UI in Console.
* Ideally, the operator should have been pre-installed in the cluster, but that isn't supported.

## How would the UI install charts
Copy link
Member

Choose a reason for hiding this comment

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

as i read more of this enhancement, i am wondering why we feel obliged to couple our implementation to the console operator at all. rather than continue to couple things that do not need to be coupled, can we at least specify how it would look when uncoupled?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are coupled related at the moment because we are bringing the Helm experience to Console.

The helm CLI way of listing repos is different than the Console way of listing repos:

  • The helm CLI looks into the file system for figuring out the available helm chart repositories. ( We'll update the enhancement how the CLI could eventually make use of in-cluster representations ie CRs )
  • The UI would have to look at a REST API to figure that out.

Console Operator reconciling the information isn't coupling per se, it's more of Console being aware of the presence of a new Helm repository.

At the moment, the chart repo url is just an env var which the console proxy uses. We needed this env var because we didn't have a CR to read that information from.

Once we decide on the HelmChartRepository CR, I'll consider using the UI equivalent of oc get helmchartrepositories to have the UI rendered.

reviewers:
- TBD
approvers:
- TBD
Copy link
Member

Choose a reason for hiding this comment

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

if the change is to core openshift api, please add an openshift api approver here.

recommend @deads2k @bparees and myself.

Copy link
Contributor

@bparees bparees Apr 7, 2020

Choose a reason for hiding this comment

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

@derekwaynecarr
Copy link
Member

/assign

@sbose78
Copy link
Contributor Author

sbose78 commented Apr 13, 2020

Minutes of the meeting: @deads2k @pmorie Michael Elder, @akashshinde

  • Shoubhik showed the current Helm support in Console.
  • Michael showed the current channel/subscription concepts in ACM.
  • The current representation of a helm chart repository in the cluster is the Channel API in ACM.
  • The proposed representation ( in this proposal ) is the HelmChartRepository CRD/API.

Next steps:

  • Move the HelmChartRepository out of the config.opensift.io group. Of course, Console Cluster Settings page needs to be able to adapt accordingly.
  • Update proposal to reflect details of how multiple chart repos configured would look like.
  • Update proposal to specify upstream alternatives to HelmChartRepository and why they aren't a right fit at the moment, if any. This isn't a blocking requirement, rather the proposal needs to reflect that options have been considered.
  • Update proposal to specify what the future CLI experience could look like if it were to honor the presence of CRs in the cluster which have helm repository metadata.
  • Work with Michael and upstream to align on as single representation of a helm chart repository in-cluster ( Channel v/s HelmChartRepository ). This is a mid-term goal.

cc @derekwaynecarr @pedjak

@deads2k
Copy link
Contributor

deads2k commented Apr 13, 2020

  • Work with Michael and upstream to align on as single representation of a helm chart repository in-cluster ( Channel v/s HelmChartRepository ). This is a mid-term goal.

I think we should have a short-term outcome that identifies where upstream to engage with helm (not other parts of redhat/ibm) to find a way to produce comparable CLI/UI experiences that matches the direction we're going with this API. We can (and should) lead in the space, but we need to help the upstream space move with us. I think we'd have some good luck describing the forces that lead to conclusions like "find the helm chart repos that your kube cluster recommends using".

@sbose78
Copy link
Contributor Author

sbose78 commented Apr 16, 2020

Hi David, could you please take a look a tell me if I should make improvements?

@pedjak and I have tried to keep it balanced to be able to

  • provide details on how we plan to implement multiple chart repositories in the Console Developer Catalog,
  • and at the same time, I've laid out a summary of our plan to propose the new API upstream in the "Taking the HelmChartRepository API upstream" section.

Let me know!

Copy link
Member

@derekwaynecarr derekwaynecarr left a comment

Choose a reason for hiding this comment

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

the API namespace proposed is under Kubernetes.io where I suspect you intended to use openshift.io, please update and clarify.

The `HelmChartRepository` is a simple cluster-scoped API for letting users/admins define Helm Chart Repository configurations in a cluster.

```yaml
apiVersion: helm.kubernetes.io/v1
Copy link
Member

Choose a reason for hiding this comment

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

I am confused. We don’t control the Kubernetes.io namespace, was this meant to be openshift.io?


```shell
$ cat <<EOF | oc apply -f -
apiVersion: helm.kubernetes.io/v1
Copy link
Member

Choose a reason for hiding this comment

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

same comment applies here.


2) Given that the `Helm` CLI manages chart repository configurations in the local filesystem only, we shall propose upstream changes in the [Helm CLI Repository](https://github.com/helm/helm) to optionally make the CLI aware of `HelmChartRepository` CRs in the current cluster context.

As a consequence, `helm repo list` would not only list local chart repositories from the local file system, it would also list chart repositories represented as Kubernetes objects in the cluster.
Copy link
Member

Choose a reason for hiding this comment

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

this sounds good, but I would expect that future API to exist in the helm API namespace and not the Kubernetes.io namespace.

@sbose78
Copy link
Contributor Author

sbose78 commented Apr 27, 2020

Thanks, I will fix that.

I would avoid using the openshift.io namespace as well since I plan to propose this API to helm upstream, and therefore, I was thinking of using the *.kubernetes.io group.

Since *.Kubernetes.io would be problematic, I would go for a more helm upstream friendly apps.helm.io ( wouldn't mention OpenShift to keep our upstream prospects open ).

@derekwaynecarr
Copy link
Member

@sbose78 thank you for the updates, I reviewed the upstream discussion. Is there a time frame for closure on that discussion? It looks like it had some positive feedback. How would you want to proceed?

@sbose78
Copy link
Contributor Author

sbose78 commented Jun 2, 2020

@derekwaynecarr Right, upstream response has been promising, we are working on getting a timeframe.

I would like to go ahead with changes on the OpenShift side with our downstream API after this proposal is merged and accepted. If within a reasonable timeframe before 4.6 GA code freeze, the upstream API is ready and can be source'd in, we'll do so.

apiVersion: helm.openshift.io/v1alpha1
kind: HelmChartRepository
metadata:
name: cluster
Copy link
Contributor

Choose a reason for hiding this comment

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

If I've read correctly to here, this name can be anything. Please choose a descriptive name for the example so it's clear there is no "special" name

metadata:
name: cluster
spec:
url: http://my.chart-repo.org/stable
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm -1 on allowing http. The stance of our product is secure by default. We can restrict this to https and then provide a way to provide a CA bundle either by reference in a configmap in openshift-config namespace or inline (make a union type).

Copy link

Choose a reason for hiding this comment

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

sure, this was actually typo.

description: my private chart repo

# set to true if need to be disabled temporarly
disabled: true
Copy link
Contributor

Choose a reason for hiding this comment

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

You probably explain this later, but by have a disabled==true instead of simply delete? We don't have a disable value on workload resources or instance. Or APIServices.

Copy link

Choose a reason for hiding this comment

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

ok, fine with me.

disabled: true
```

An operator would watch for changes on them and reconfigure the chart repository proxy which is used to power the UI experience in OpenShift today..
Copy link
Contributor

Choose a reason for hiding this comment

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

If we developed a matching command line tool, would it expect to use this proxy?

Copy link

Choose a reason for hiding this comment

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

we can use Helm CLI out of the box by configuring our proxy as one of its repo via helm repo add cmd. In addition that - we are have a proposal for upstream: helm/helm#8175

@sbose78
Copy link
Contributor Author

sbose78 commented Jun 16, 2020

An update on delivering the CRD - we are making things simpler:

  1. We plan to add the CRD to https://github.com/openshift/console-operator/tree/master/manifests ( we need it to go in because the console-operator will be watching this CRD )

  2. The golang types / api would not be maintained in openshift/api . It would go to somewhere like github.com/redhat-developer/helm

@spadgett , are you Ok with this?

@sbose78
Copy link
Contributor Author

sbose78 commented Jun 29, 2020

Update: After discussion with @deads2k on #forum-arch , we'll go with adding the API to openshift/api .

pedjak added a commit to pedjak/openshift-api that referenced this pull request Jul 3, 2020
…uster

* Introduced `HelmChartRepository` top-level CR modelled according to chartrepo.Entry
  from https://github.com/helm/helm/blob/master/pkg/repo/chartrepo.go#L42
* The corresponding enchancement: openshift/enhancements/pull/175
@sbose78
Copy link
Contributor Author

sbose78 commented Jul 23, 2020

@deads2k
Copy link
Contributor

deads2k commented Jul 23, 2020

I'm ready to take the rest of the API comments to the api PR.

/approve

approving the API and the upgrade/migration path looks workable.

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 23, 2020
pedjak added a commit to pedjak/openshift-api that referenced this pull request Jul 24, 2020
…uster

* Introduced `HelmChartRepository` top-level CR modelled according to chartrepo.Entry
  from https://github.com/helm/helm/blob/master/pkg/repo/chartrepo.go#L42
* The corresponding enchancement: openshift/enhancements/pull/175
@openshift-ci-robot
Copy link

@sbose78: you cannot LGTM your own PR.

In response to this:

/lgtm
/approve

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.

Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

The approach lgtm, one question on the API version

The `HelmChartRepository` is a simple cluster-scoped API for letting users/admins define Helm Chart Repository configurations in a cluster.

```yaml
apiVersion: helm.openshift.io/v1alpha1
Copy link
Member

Choose a reason for hiding this comment

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

Sorry if this was discussed above and I missed it, but is this meant to be an alpha API? What is the intended support level?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was discussion in the API PR and we went for Beta
https://github.com/openshift/api/pull/598/files#diff-ed4a5a3bba15a3929122e018a72df324R15

Good catch, this part needs an update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@sbose78 sbose78 force-pushed the helm-3-implementation-console branch from 33a5ad1 to 3dbad51 Compare July 28, 2020 18:51
@sbose78
Copy link
Contributor Author

sbose78 commented Jul 28, 2020

Commits have been squashed.

/assign spadgett

Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 29, 2020
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, sbose78, spadgett

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:

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

@openshift-merge-robot openshift-merge-robot merged commit 0cacb62 into openshift:master Jul 29, 2020
wking added a commit to wking/console-operator that referenced this pull request Aug 1, 2023
It's unclear what the motivation was for their existing runlevels,
with the API filenames landing without much context:

  openshift/api $ git --no-pager log --oneline operator/v1/0000_70_console-operator.crd.yaml | tail -n1
  862dd2fe4 Add console-operator CRD and generate from types
  openshift/api $ git --no-pager log -1 --format='%h %B' --stat=80 862dd2fe4
  862dd2fe4 Add console-operator CRD and generate from types

   operator/v1/0000_70_console-operator.crd.yaml | 195 ++++++++++++++++++++++++++
   1 file changed, 195 insertions(+)
  openshift/api $ git --no-pager log --oneline helm/*.crd.yaml | tail -n1
  a4be43c2b Added ability to configure Helm chart repository accessible within cluster
  openshift/api $ git --no-pager log -1 --format='%h %B' --stat=80 a4be43c2b
  a4be43c2b Added ability to configure Helm chart repository accessible within cluster

  * Introduced `HelmChartRepository` top-level CR modelled according to chartrepo.Entry
    from https://github.com/helm/helm/blob/master/pkg/repo/chartrepo.go#L42
  * The corresponding enchancement: openshift/enhancements/pull/175

   Makefile                                           |   3 +-
   hack/update-deepcopy.sh                            |   2 +-
   hack/verify-crds.sh                                |   1 +
   .../0000_10-helm-chart-repository.crd.yaml         | 138 +++++++++++++++++++
   helm/v1alpha1/doc.go                               |   8 ++
   helm/v1alpha1/register.go                          |  38 ++++++
   helm/v1alpha1/types_helm.go                        | 108 +++++++++++++++
   helm/v1alpha1/zz_generated.deepcopy.go             | 152 +++++++++++++++++++++
   8 files changed, 448 insertions(+), 2 deletions(-)
  openshift/api $ git --no-pager log --oneline helm/v1beta1/0000_10-project-helm-chart-repository.crd.yaml | tail -n1
  8fe639fe2 helm: add a new namespaced CRD for helm
  $ git --no-pager log -1 --format='%h %B' --stat=80 8fe639fe2
  8fe639fe2 helm: add a new namespaced CRD for helm

  As part of the process to support namespace-scoped Helm repositories for
  non-admin users, this PR adds a new namespace-scoped CRD definition
  named `projecthelmchartrepositories.helm.openshift.io`.

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

   .../0000_10-project-helm-chart-repository.crd.yaml | 130 +++++++++++++++++++++
   helm/v1beta1/register.go                           |   2 +
   ...ypes_helm.go => types_helm_chart_repository.go} |   0
   .../v1beta1/types_project_helm_chart_repository.go |  92 +++++++++++++++
   helm/v1beta1/zz_generated.deepcopy.go              | 119 +++++++++++++++++++
   5 files changed, 343 insertions(+)

Removing the explicit runlevels will allow these resources to go in
with the other console resources in the default runlevel 50 [1,2].

Significantly, the outgoing 0000_70_console-operator.crd.yaml hadn't
matched the
0000_<runlevel>_<dash-separated-component>_<manifest_filename>
template [1], and by sorting lexically between other level-70
manifests:

  $ oc adm release extract --to manifests quay.io/openshift-release-dev/ocp-release:4.14.0-ec.4-x86_64
  $ ls manifests | grep -1 0000_70_console-operator.crd.yaml
  0000_70_cluster-network-operator_05_clusteroperator.yaml
  0000_70_console-operator.crd.yaml
  0000_70_dns-operator_00-cluster-role.yaml

it had been pushing the network and DNS manifests together into one
large task-node.  With this change removing the unparsable attempt at
a runlevel, the network and DNS manifests will be able to apply on
updates via two parallel task-nodes.

[1]: https://github.com/openshift/enhancements/blob/cafeb5c3cba7f8c9e261b2aabffa92e34dd76be6/dev-guide/cluster-version-operator/dev/operators.md#what-is-the-order-that-resources-get-createdupdated-in
[2]: https://github.com/openshift/oc/blob/13225e00caf1ad2d3603e1d1cc8651833f2effcb/pkg/cli/admin/release/new.go#L1412-L1416
wking added a commit to wking/console-operator that referenced this pull request Aug 1, 2023
It's unclear what the motivation was for their existing runlevels,
with the API filenames landing without much context:

  openshift/api $ git --no-pager log --oneline operator/v1/0000_70_console-operator.crd.yaml | tail -n1
  862dd2fe4 Add console-operator CRD and generate from types
  openshift/api $ git --no-pager log -1 --format='%h %B' --stat=80 862dd2fe4
  862dd2fe4 Add console-operator CRD and generate from types

   operator/v1/0000_70_console-operator.crd.yaml | 195 ++++++++++++++++++++++++++
   1 file changed, 195 insertions(+)
  openshift/api $ git --no-pager log --oneline helm/*.crd.yaml | tail -n1
  a4be43c2b Added ability to configure Helm chart repository accessible within cluster
  openshift/api $ git --no-pager log -1 --format='%h %B' --stat=80 a4be43c2b
  a4be43c2b Added ability to configure Helm chart repository accessible within cluster

  * Introduced `HelmChartRepository` top-level CR modelled according to chartrepo.Entry
    from https://github.com/helm/helm/blob/master/pkg/repo/chartrepo.go#L42
  * The corresponding enchancement: openshift/enhancements/pull/175

   Makefile                                           |   3 +-
   hack/update-deepcopy.sh                            |   2 +-
   hack/verify-crds.sh                                |   1 +
   .../0000_10-helm-chart-repository.crd.yaml         | 138 +++++++++++++++++++
   helm/v1alpha1/doc.go                               |   8 ++
   helm/v1alpha1/register.go                          |  38 ++++++
   helm/v1alpha1/types_helm.go                        | 108 +++++++++++++++
   helm/v1alpha1/zz_generated.deepcopy.go             | 152 +++++++++++++++++++++
   8 files changed, 448 insertions(+), 2 deletions(-)
  openshift/api $ git --no-pager log --oneline helm/v1beta1/0000_10-project-helm-chart-repository.crd.yaml | tail -n1
  8fe639fe2 helm: add a new namespaced CRD for helm
  $ git --no-pager log -1 --format='%h %B' --stat=80 8fe639fe2
  8fe639fe2 helm: add a new namespaced CRD for helm

  As part of the process to support namespace-scoped Helm repositories for
  non-admin users, this PR adds a new namespace-scoped CRD definition
  named `projecthelmchartrepositories.helm.openshift.io`.

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

   .../0000_10-project-helm-chart-repository.crd.yaml | 130 +++++++++++++++++++++
   helm/v1beta1/register.go                           |   2 +
   ...ypes_helm.go => types_helm_chart_repository.go} |   0
   .../v1beta1/types_project_helm_chart_repository.go |  92 +++++++++++++++
   helm/v1beta1/zz_generated.deepcopy.go              | 119 +++++++++++++++++++
   5 files changed, 343 insertions(+)

Removing the explicit runlevels will allow these resources to go in
with the other console resources in the default runlevel 50 [1,2].

Significantly, the outgoing 0000_70_console-operator.crd.yaml hadn't
matched the
0000_<runlevel>_<dash-separated-component>_<manifest_filename>
template [1], and by sorting lexically between other level-70
manifests:

  $ oc adm release extract --to manifests quay.io/openshift-release-dev/ocp-release:4.14.0-ec.4-x86_64
  $ ls manifests | grep -1 0000_70_console-operator.crd.yaml
  0000_70_cluster-network-operator_05_clusteroperator.yaml
  0000_70_console-operator.crd.yaml
  0000_70_dns-operator_00-cluster-role.yaml

it had been pushing the network and DNS manifests together into one
large task-node.  With this change removing the unparsable attempt at
a runlevel, the network and DNS manifests will be able to apply on
updates via two parallel task-nodes.

I'm prefixing them with '00_' now to get them near the front of the
other level-50 console manifests.  For example, manifests/01-helm.yaml
is declaring a HelmChartRepository resource, so we want it to sort
after the HelmChartRepository CustomResourceDefinition within the
console task-node.

[1]: https://github.com/openshift/enhancements/blob/cafeb5c3cba7f8c9e261b2aabffa92e34dd76be6/dev-guide/cluster-version-operator/dev/operators.md#what-is-the-order-that-resources-get-createdupdated-in
[2]: https://github.com/openshift/oc/blob/13225e00caf1ad2d3603e1d1cc8651833f2effcb/pkg/cli/admin/release/new.go#L1412-L1416
wking added a commit to wking/openshift-api that referenced this pull request Aug 2, 2023
It's unclear what the motivation was for their existing runlevels,
with the API filenames landing without much context:

  $ git --no-pager log --oneline operator/v1/0000_70_console-operator.crd.yaml | tail -n1
  862dd2f Add console-operator CRD and generate from types
  $ git --no-pager log -1 --format='%h %B' --stat=80 862dd2f
  862dd2f Add console-operator CRD and generate from types

   operator/v1/0000_70_console-operator.crd.yaml | 195 ++++++++++++++++++++++++++
   1 file changed, 195 insertions(+)
  $ git --no-pager log --oneline helm/*.crd.yaml | tail -n1
  a4be43c Added ability to configure Helm chart repository accessible within cluster
  $ git --no-pager log -1 --format='%h %B' --stat=80 a4be43c
  a4be43c Added ability to configure Helm chart repository accessible within cluster

  * Introduced `HelmChartRepository` top-level CR modelled according to chartrepo.Entry
    from https://github.com/helm/helm/blob/master/pkg/repo/chartrepo.go#L42
  * The corresponding enchancement: openshift/enhancements/pull/175

   Makefile                                           |   3 +-
   hack/update-deepcopy.sh                            |   2 +-
   hack/verify-crds.sh                                |   1 +
   .../0000_10-helm-chart-repository.crd.yaml         | 138 +++++++++++++++++++
   helm/v1alpha1/doc.go                               |   8 ++
   helm/v1alpha1/register.go                          |  38 ++++++
   helm/v1alpha1/types_helm.go                        | 108 +++++++++++++++
   helm/v1alpha1/zz_generated.deepcopy.go             | 152 +++++++++++++++++++++
   8 files changed, 448 insertions(+), 2 deletions(-)
  $ git --no-pager log --oneline helm/v1beta1/0000_10-project-helm-chart-repository.crd.yaml | tail -n1
  8fe639f helm: add a new namespaced CRD for helm
  $ git --no-pager log -1 --format='%h %B' --stat=80 8fe639f
  8fe639f helm: add a new namespaced CRD for helm

  As part of the process to support namespace-scoped Helm repositories for
  non-admin users, this PR adds a new namespace-scoped CRD definition
  named `projecthelmchartrepositories.helm.openshift.io`.

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

   .../0000_10-project-helm-chart-repository.crd.yaml | 130 +++++++++++++++++++++
   helm/v1beta1/register.go                           |   2 +
   ...ypes_helm.go => types_helm_chart_repository.go} |   0
   .../v1beta1/types_project_helm_chart_repository.go |  92 +++++++++++++++
   helm/v1beta1/zz_generated.deepcopy.go              | 119 +++++++++++++++++++
   5 files changed, 343 insertions(+)

Removing the explicit runlevels will allow these resources to go in
with the other console resources in the default runlevel 50 [1,2].

Significantly, the outgoing 0000_70_console-operator.crd.yaml hadn't
matched the
0000_<runlevel>_<dash-separated-component>_<manifest_filename>
template [1], and by sorting lexically between other level-70
manifests:

  $ oc adm release extract --to manifests quay.io/openshift-release-dev/ocp-release:4.14.0-ec.4-x86_64
  $ ls manifests | grep -1 0000_70_console-operator.crd.yaml
  0000_70_cluster-network-operator_05_clusteroperator.yaml
  0000_70_console-operator.crd.yaml
  0000_70_dns-operator_00-cluster-role.yaml

it had been pushing the network and DNS manifests together into one
large task-node.  With this change removing the unparsable attempt at
a runlevel, the network and DNS manifests will be able to apply on
updates via two parallel task-nodes.

I'm prefixing them with '00_' now to get them near the front of the
other level-50 console manifests.  For example, manifests/01-helm.yaml
is declaring a HelmChartRepository resource [3], so we want it to sort
after the HelmChartRepository CustomResourceDefinition within the
console task-node.

Generated with:

  $ rename 's/0000_[0-9][0-9]_/00_/' console/*/*.yaml
  $ rename 's/0000_[0-9][0-9]_/00_/' helm/*/*.yaml
  $ rename 's/0000_[0-9][0-9]_/00_/' operator/*/*console-operator*.yaml
  $ git add -A console helm operator

[1]: https://github.com/openshift/enhancements/blob/cafeb5c3cba7f8c9e261b2aabffa92e34dd76be6/dev-guide/cluster-version-operator/dev/operators.md#what-is-the-order-that-resources-get-createdupdated-in
[2]: https://github.com/openshift/oc/blob/13225e00caf1ad2d3603e1d1cc8651833f2effcb/pkg/cli/admin/release/new.go#L1412-L1416
[3]: https://github.com/openshift/console-operator/blob/cb08a319271b2f4676d1faae96a5ab10440facb0/manifests/01-helm.yaml
wking added a commit to wking/openshift-api that referenced this pull request Aug 2, 2023
It's unclear what the motivation was for their existing runlevels,
with the API filenames landing without much context:

  $ git --no-pager log --oneline operator/v1/0000_70_console-operator.crd.yaml | tail -n1
  862dd2f Add console-operator CRD and generate from types
  $ git --no-pager log -1 --format='%h %B' --stat=80 862dd2f
  862dd2f Add console-operator CRD and generate from types

   operator/v1/0000_70_console-operator.crd.yaml | 195 ++++++++++++++++++++++++++
   1 file changed, 195 insertions(+)
  $ git --no-pager log --oneline helm/*.crd.yaml | tail -n1
  a4be43c Added ability to configure Helm chart repository accessible within cluster
  $ git --no-pager log -1 --format='%h %B' --stat=80 a4be43c
  a4be43c Added ability to configure Helm chart repository accessible within cluster

  * Introduced `HelmChartRepository` top-level CR modelled according to chartrepo.Entry
    from https://github.com/helm/helm/blob/master/pkg/repo/chartrepo.go#L42
  * The corresponding enchancement: openshift/enhancements/pull/175

   Makefile                                           |   3 +-
   hack/update-deepcopy.sh                            |   2 +-
   hack/verify-crds.sh                                |   1 +
   .../0000_10-helm-chart-repository.crd.yaml         | 138 +++++++++++++++++++
   helm/v1alpha1/doc.go                               |   8 ++
   helm/v1alpha1/register.go                          |  38 ++++++
   helm/v1alpha1/types_helm.go                        | 108 +++++++++++++++
   helm/v1alpha1/zz_generated.deepcopy.go             | 152 +++++++++++++++++++++
   8 files changed, 448 insertions(+), 2 deletions(-)
  $ git --no-pager log --oneline helm/v1beta1/0000_10-project-helm-chart-repository.crd.yaml | tail -n1
  8fe639f helm: add a new namespaced CRD for helm
  $ git --no-pager log -1 --format='%h %B' --stat=80 8fe639f
  8fe639f helm: add a new namespaced CRD for helm

  As part of the process to support namespace-scoped Helm repositories for
  non-admin users, this PR adds a new namespace-scoped CRD definition
  named `projecthelmchartrepositories.helm.openshift.io`.

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

   .../0000_10-project-helm-chart-repository.crd.yaml | 130 +++++++++++++++++++++
   helm/v1beta1/register.go                           |   2 +
   ...ypes_helm.go => types_helm_chart_repository.go} |   0
   .../v1beta1/types_project_helm_chart_repository.go |  92 +++++++++++++++
   helm/v1beta1/zz_generated.deepcopy.go              | 119 +++++++++++++++++++
   5 files changed, 343 insertions(+)

Removing the explicit runlevels will allow these resources to go in
with the other console resources in the default runlevel 50 [1,2].

Significantly, the outgoing 0000_70_console-operator.crd.yaml hadn't
matched the
0000_<runlevel>_<dash-separated-component>_<manifest_filename>
template [1], and by sorting lexically between other level-70
manifests:

  $ oc adm release extract --to manifests quay.io/openshift-release-dev/ocp-release:4.14.0-ec.4-x86_64
  $ ls manifests | grep -1 0000_70_console-operator.crd.yaml
  0000_70_cluster-network-operator_05_clusteroperator.yaml
  0000_70_console-operator.crd.yaml
  0000_70_dns-operator_00-cluster-role.yaml

it had been pushing the network and DNS manifests together into one
large task-node.  With this change removing the unparsable attempt at
a runlevel, the network and DNS manifests will be able to apply on
updates via two parallel task-nodes.

I'm prefixing them with '00_' now to get them near the front of the
other level-50 console manifests.  For example, manifests/01-helm.yaml
is declaring a HelmChartRepository resource [3], so we want it to sort
after the HelmChartRepository CustomResourceDefinition within the
console task-node.

Generated with:

  $ rename 's/0000_[0-9][0-9]_/00_/' console/*/*.yaml helm/*/*.yaml operator/*/*console-operator*.yaml
  $ sed -i 's/0000_[0-9][0-9]_/00_/' console/*/*testsuite.yaml helm/*/*.testsuite.yaml
  $ sed -i 's/0000_[0-9][0-9]_\(.*console-operator\)/00_\1/' operator/*/*testsuite.yaml
  $ git add -A console helm operator

[1]: https://github.com/openshift/enhancements/blob/cafeb5c3cba7f8c9e261b2aabffa92e34dd76be6/dev-guide/cluster-version-operator/dev/operators.md#what-is-the-order-that-resources-get-createdupdated-in
[2]: https://github.com/openshift/oc/blob/13225e00caf1ad2d3603e1d1cc8651833f2effcb/pkg/cli/admin/release/new.go#L1412-L1416
[3]: https://github.com/openshift/console-operator/blob/cb08a319271b2f4676d1faae96a5ab10440facb0/manifests/01-helm.yaml
wking added a commit to wking/openshift-api that referenced this pull request Aug 2, 2023
It's unclear what the motivation was for their existing runlevels,
with the API filenames landing without much context:

  $ git --no-pager log --oneline operator/v1/0000_70_console-operator.crd.yaml | tail -n1
  862dd2f Add console-operator CRD and generate from types
  $ git --no-pager log -1 --format='%h %B' --stat=80 862dd2f
  862dd2f Add console-operator CRD and generate from types

   operator/v1/0000_70_console-operator.crd.yaml | 195 ++++++++++++++++++++++++++
   1 file changed, 195 insertions(+)
  $ git --no-pager log --oneline helm/*.crd.yaml | tail -n1
  a4be43c Added ability to configure Helm chart repository accessible within cluster
  $ git --no-pager log -1 --format='%h %B' --stat=80 a4be43c
  a4be43c Added ability to configure Helm chart repository accessible within cluster

  * Introduced `HelmChartRepository` top-level CR modelled according to chartrepo.Entry
    from https://github.com/helm/helm/blob/master/pkg/repo/chartrepo.go#L42
  * The corresponding enchancement: openshift/enhancements/pull/175

   Makefile                                           |   3 +-
   hack/update-deepcopy.sh                            |   2 +-
   hack/verify-crds.sh                                |   1 +
   .../0000_10-helm-chart-repository.crd.yaml         | 138 +++++++++++++++++++
   helm/v1alpha1/doc.go                               |   8 ++
   helm/v1alpha1/register.go                          |  38 ++++++
   helm/v1alpha1/types_helm.go                        | 108 +++++++++++++++
   helm/v1alpha1/zz_generated.deepcopy.go             | 152 +++++++++++++++++++++
   8 files changed, 448 insertions(+), 2 deletions(-)
  $ git --no-pager log --oneline helm/v1beta1/0000_10-project-helm-chart-repository.crd.yaml | tail -n1
  8fe639f helm: add a new namespaced CRD for helm
  $ git --no-pager log -1 --format='%h %B' --stat=80 8fe639f
  8fe639f helm: add a new namespaced CRD for helm

  As part of the process to support namespace-scoped Helm repositories for
  non-admin users, this PR adds a new namespace-scoped CRD definition
  named `projecthelmchartrepositories.helm.openshift.io`.

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

   .../0000_10-project-helm-chart-repository.crd.yaml | 130 +++++++++++++++++++++
   helm/v1beta1/register.go                           |   2 +
   ...ypes_helm.go => types_helm_chart_repository.go} |   0
   .../v1beta1/types_project_helm_chart_repository.go |  92 +++++++++++++++
   helm/v1beta1/zz_generated.deepcopy.go              | 119 +++++++++++++++++++
   5 files changed, 343 insertions(+)

Removing the explicit runlevels will allow these resources to go in
with the other console resources in the default runlevel 50 [1,2].

Significantly, the outgoing 0000_70_console-operator.crd.yaml hadn't
matched the
0000_<runlevel>_<dash-separated-component>_<manifest_filename>
template [1], and by sorting lexically between other level-70
manifests:

  $ oc adm release extract --to manifests quay.io/openshift-release-dev/ocp-release:4.14.0-ec.4-x86_64
  $ ls manifests | grep -1 0000_70_console-operator.crd.yaml
  0000_70_cluster-network-operator_05_clusteroperator.yaml
  0000_70_console-operator.crd.yaml
  0000_70_dns-operator_00-cluster-role.yaml

it had been pushing the network and DNS manifests together into one
large task-node.  With this change removing the unparsable attempt at
a runlevel, the network and DNS manifests will be able to apply on
updates via two parallel task-nodes.

I'm prefixing most of them with '00_' now to get them near the front
of the other level-50 console manifests.  For example,
manifests/01-helm.yaml is declaring a HelmChartRepository resource
[3], so we want it to sort after the HelmChartRepository
CustomResourceDefinition within the console task-node.

But I'm prefixing ConsolePlugin with '90_' to sort it towards the
back, after 07-operator.yaml [4], because the operator is what serves
the /crdconvert webhook defined in that CRD [5].

Generated with:

  $ rename 's/0000_[0-9][0-9]_/00_/' console/*/*.yaml helm/*/*.yaml operator/*/*console-operator*.yaml
  $ sed -i 's/0000_[0-9][0-9]_/00_/' console/*/*testsuite.yaml helm/*/*.testsuite.yaml
  $ sed -i 's/0000_[0-9][0-9]_\(.*console-operator\)/00_\1/' operator/*/*testsuite.yaml
  $ rename 's/00_/90_/' console/*/*consoleplugin*.yaml
  $ sed -i 's/00_/90_/' console/*/*consoleplugin*testsuite.yaml
  $ git add -A console helm operator

[1]: https://github.com/openshift/enhancements/blob/cafeb5c3cba7f8c9e261b2aabffa92e34dd76be6/dev-guide/cluster-version-operator/dev/operators.md#what-is-the-order-that-resources-get-createdupdated-in
[2]: https://github.com/openshift/oc/blob/13225e00caf1ad2d3603e1d1cc8651833f2effcb/pkg/cli/admin/release/new.go#L1412-L1416
[3]: https://github.com/openshift/console-operator/blob/cb08a319271b2f4676d1faae96a5ab10440facb0/manifests/01-helm.yaml
[4]: https://github.com/openshift/console-operator/blob/cb08a319271b2f4676d1faae96a5ab10440facb0/manifests/07-operator.yaml
[5]: https://issues.redhat.com//browse/OCPBUGS-15834
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants