Proposal for Helm Chart Registry Adapter #830
Conversation
openshift-ci-robot
added
the
size/L
docs/proposals/helm-charts.md
Outdated
| #### Creating a Helm Adapter | ||
|
|
||
| The `HelmAdapter` struct will look something like, the addition of `Charts` | ||
| makes it possible to save our work and only ready the registries index file |
docs/proposals/helm-charts.md
Outdated
| ## Introduction | ||
|
|
||
| Add a new registry adapter type, `helm`, that supports Helm chart | ||
| repositories. With this change, an admin, could configure the broker by adding |
There was a problem hiding this comment.
punctuation: remove the comma after admin. It should read "With this change, an admin could..."
docs/proposals/helm-charts.md
Outdated
| ``` | ||
|
|
||
| Most of the data required to create a useful `Spec` object is contained in | ||
| the Chart object. However, in order to retrieve the default values we will use |
There was a problem hiding this comment.
punctuation: add a comma after values. It should read "in order to retrieve the default values, we will use"
docs/proposals/helm-charts.md
Outdated
|
|
||
| Add a new registry adapter type, `helm`, that supports Helm chart | ||
| repositories. With this change, an admin, could configure the broker by adding | ||
| a `helm` type registry and have the Helm charts hosted in that Helm chart |
There was a problem hiding this comment.
I think it would sound better flipped: "a helm registry type"
docs/proposals/helm-charts.md
Outdated
| Currently, our broker only supports docker container image type registries. | ||
| However, with [Helm](https://helm.sh) being the "package manager for Kubernetes", | ||
| there is already a strong collection of applications defined in the form of | ||
| Helm charts. Adding the ability to the broker to consume these Helm charts as |
There was a problem hiding this comment.
shuffle the words around a bit:
Modifying the broker to consume these Helm charts as Service Bundles will allow work already done in Helm charts to be made available in a cluster.
docs/proposals/helm-charts.md
Outdated
| repository | ||
| structure](https://github.com/kubernetes/helm/blob/master/docs/chart_repository.md#the-chart-repository-structure) | ||
| and add the latest version of each chart to the `Charts` field as well as | ||
| its name to the list of `imageNames` to be returned. |
There was a problem hiding this comment.
break up the sentence:
...repository structure](https://github.com/kubernetes/helm/blob/master/docs/chart_repository.md#the-chart-repository-structure). The broker will add the latest version of each chart to the...
docs/proposals/helm-charts.md
Outdated
|
|
||
| After these image names are filtered, the subsequent call to `FetchSpecs()` | ||
| will find the chart by name in the `Charts` field and create a `Spec` object | ||
| for it. For example: |
There was a problem hiding this comment.
So will a call to bootstrap ever regenerate this Charts cache? How will we pick up changes?
There was a problem hiding this comment.
To my understanding, yes. Every call to bootstrap should create a new instance of a HelmAdapter for each of the helm type config entries, calling GetImageNames() and FetchSpecs() again.
There was a problem hiding this comment.
I don't think that is correct. Registries are made with an adapter as an unexposed field and then is used for subsequent calls. I do not think that bootstrap by itself is creating new adapters. Are you planning on adding some other behavior?
There was a problem hiding this comment.
Not intending to change the behavior. This concern should be overcome by rebuilding the list of image names and charts on each call to GetImageNames(), for example:
// GetImageNames - retrieve the images
func (r *HelmAdapter) GetImageNames() ([]string, error) {
var imageNames []string
r.Charts = map[string][]*repo.ChartVersion{}
Then, the subsequent call to FetchSpecs(image_names) will be operating on the newly built list of image names and charts.
docs/proposals/helm-charts.md
Outdated
| the Chart object. However, in order to retrieve the default values we will use | ||
| [Helm's Chartutil | ||
| pkg](https://github.com/kubernetes/helm/blob/master/pkg/chartutil/load.go) to | ||
| load the archive specified at `Chart.URLs[0]` and read the values. |
There was a problem hiding this comment.
Can you elaborate on this process a bit more? What sort of archive is it? Are we downloading something and cracking it open? Where is it being stored? etc.
docs/proposals/helm-charts.md
Outdated
|
|
||
| Helm Chart Repositories can be authenticated, [this | ||
| issue](https://github.com/kubernetes/helm/issues/1038) provides more | ||
| information. The broker should support authenticated chart repositories. |
There was a problem hiding this comment.
Should we support all of the authentication formats? How will we accomplish this? Or would we do another proposal for handling phase 2 at a later date?
There was a problem hiding this comment.
I'm making it clear that a separate proposal should be made for this work. Handling basic repos is a sufficient first step and the work required to support authenticated repos would require investigation.
docs/proposals/helm-charts.md
Outdated
| In phase 1, only the latest chart version will be used. The broker should | ||
| support all versions of a chart and allow the consumer to specify which version | ||
| of a chart they wish to install. The base image would need to be updated to | ||
| handle specifying a specific version at provision time. |
There was a problem hiding this comment.
Where is the version information stored? What does a versioned chart url look like? Or more specifically how would someone get the specific version of a chart? What changes do you anticipate being done to the base image? Or is this another proposal for when we do phase 3?
There was a problem hiding this comment.
I went ahead and made this part of the initial proposal, it's easy to get all of the chart's versions.
|
@eriknelson this is waiting on your review. |
docs/proposals/helm-charts.md
Outdated
|
|
||
| - Add the ability to read the `base_image` from the broker config. | ||
| - Pass the URL and BaseImage to the registry adapter. | ||
| - Instantiate a Helm registry adapter to handle Helm Chart registries in the |
There was a problem hiding this comment.
Language-wise, helm seems to use the word "repository" instead of "registry" to refer to a collection of charts. It may be a good idea to reflect that here and elsewhere.
docs/proposals/helm-charts.md
Outdated
| repository | ||
| structure](https://github.com/kubernetes/helm/blob/master/docs/chart_repository.md#the-chart-repository-structure). | ||
| The broker will create an entry in the `Charts` field to hold a list of | ||
| `ChartVersion` objects and add the chart name to the list of `imageNames` |
There was a problem hiding this comment.
two spaces between "and" and "add".
|
|
||
| ``` | ||
| // Use the latest chart for creating the bundle | ||
| chart := charts[0] |
There was a problem hiding this comment.
I assume in the real implementation there would be a more data-driven way of determining the latest version.
There was a problem hiding this comment.
what would that look like? https://github.com/kubernetes/helm/blob/master/pkg/repo/index.go#L141 sorts so we can reliably grab the latest version.
There was a problem hiding this comment.
Ah, nice! That'll do then if they're making the guarantee.
| "displayName": fmt.Sprintf("%s (Helm)", chart.Name), | ||
| "documentationUrl": chart.Home, | ||
| "dependencies": chart.Sources, | ||
| "imageUrl": chart.Icon, |
There was a problem hiding this comment.
I still haven't been able to get this to work. Is there documentation somewhere that we can cite as the authority that this is the right key to use?
There was a problem hiding this comment.
This is strange. I have had no issues getting this to work.
| Description: "Default plan for running helm charts", | ||
| Parameters: []apb.ParameterDescriptor{ | ||
| apb.ParameterDescriptor{ | ||
| Name: "repo", |
There was a problem hiding this comment.
What is the use case for changing this at provision time?
There was a problem hiding this comment.
There isn't one, but there is not currently a good way to hide it. That's why I use the Pattern to prevent you from changing it.
There was a problem hiding this comment.
Is it possible to just strip these from the ParameterDescriptor list in some post-processing if there's no reason to have them there?
There was a problem hiding this comment.
There is no case where a user should be changing these. However, at runtime we need the repo url. We can't hide these and still provide them at runtime. My solution was to make it the default and set the pattern to the exact string to prevent the user from changing it.
There was a problem hiding this comment.
Gotcha, so they have to be passed through somehow and we can't take a helm specific approach. I wonder if the UI could mark these fields as immutable somehow?
There was a problem hiding this comment.
Gotcha! Maybe we need an RFE for the broker to provide a way for a registry adapter to add hidden metadata that gets passed through to the executor and into the running bundle? It would be interesting to brainstorm about other possible use cases for that.
| Required: false, | ||
| }, | ||
| apb.ParameterDescriptor{ | ||
| Name: "chart", |
There was a problem hiding this comment.
What is the use case for changing this at provision time? Hasn't the user already selected a chart by name through the catalog at this point?
docs/proposals/helm-charts.md
Outdated
|
|
||
| ## Creating a Base Image for Executing Helm Charts | ||
|
|
||
| Ansible Playbook Bundles (APBs) have already been described as an instance of |
There was a problem hiding this comment.
I think the lanuage I would use here is:"Ansible Playbook Bundles (APBs) have served as an initial implementation of the more generic Service Bundle Specification"
docs/proposals/helm-charts.md
Outdated
| Service Bundle, Helm Bundles, that respect the Service Bundle contract but use | ||
| Helm as the runtime. We already have a [Helm Bundle | ||
| Base](https://github.com/ansibleplaybookbundle/helm-bundle-base) and this | ||
| [PR](https://github.com/ansibleplaybookbundle/helm-bundle-base/pull/2) |
There was a problem hiding this comment.
Thinking this project is maybe more appropriate in the automationbroker org once we're ready to merge it. No need for that to block this proposal.
| ``` | ||
| registry: | ||
| - type: helm | ||
| name: stable |
There was a problem hiding this comment.
Does stable signify something in the helm world? Is an unstable variant that would live at another url?
There was a problem hiding this comment.
Yes. The unstable variant is called incubator and the public URL is https://kubernetes-charts-incubator.storage.googleapis.com/.
docs/proposals/helm-charts.md
Outdated
| 1. Type: refers to the registry adapter to handle this registry | ||
| 1. Name: gives a name to this registry item | ||
| 1. URL: the URL for the Helm chart repository | ||
| 1. Base Image: the container image to use when provisioning/deprovisioning a |
There was a problem hiding this comment.
I'm confused with the term "Base Image" here. In the APB world, we called it apb-base because it was literally the base layer of the image that APBs then layered on top of. Is the helm-bundle-base here a generic runner that I execute directly with differing arguments? That feels like more of a "runner" or a "runtime" to me, than a "base", if I'm not expected to layer anything on top of it and instead call run on it directly.
There was a problem hiding this comment.
You are correct. It is not really a base image. Should probably make the project name and config item reflect that. runner or runner_image better reflects intent.
docs/proposals/helm-charts.md
Outdated
| 1. URL: the URL for the Helm chart repository | ||
| 1. Base Image: the container image to use when provisioning/deprovisioning a | ||
| Helm chart | ||
| 1. White List/Black List: allows the Admin to filter out Helm charts |
There was a problem hiding this comment.
As for the base_image, would this be a const in the HelmAdapter? Would we have a different one for a downstream broker?
+1, do we need to allow users to change this? Regardless of the answer, I don't think it's a good idea to bake this into the broker as a const. That would require rebuilds and releases to change.
docs/proposals/helm-charts.md
Outdated
| ``` | ||
|
|
||
| The addition of the `Charts` field makes it possible to save our work and only | ||
| read the registries' index file once. |
There was a problem hiding this comment.
So if we're caching the index file in-memory once, how do we expect folks to be able to refresh this cache if the chart repository is updated? Would it require a complete broker restart (not ideal)? How often do we expect repositories to change?
There was a problem hiding this comment.
We are rebuilding this cache every time GetImageNames() is called. I'm not totally familiar with how often the registry adapters are loaded and GetImageNames() and then FetchSpecs(imageNames) are called.
docs/proposals/helm-charts.md
Outdated
| `index.yaml` found at the Helm Chart Repository URL based on the [chart | ||
| repository | ||
| structure](https://github.com/kubernetes/helm/blob/master/docs/chart_repository.md#the-chart-repository-structure). | ||
| The broker will create an entry in the `Charts` field to hold a list of |
There was a problem hiding this comment.
Just want to make sure "The broker" here isn't referring to the registry. We definitely don't want generic registry code doing helm specific stuff like writing to the chart cache, that would break the adapter abstraction. I'm thinking it should be the adapter before returning the image name list to the registry.
Also, these adapter methods are going to be called on every bootstrap. What's the condition that we're checking to prevent doing all the "work" involved with each bootstrap to load the cache? i.e.
if cacheNotLoaded() { // What's this?
adapter.loadCache()
}There was a problem hiding this comment.
Also, does the value of the chart's entry start out as an empty list prior to the FetchSpecs call?
There was a problem hiding this comment.
Updating to make it clear that the "the broker" is actually the registry. The Charts field will be on the HelmAdapter struct. Calls to GetImageNames() will create the Charts map and FetchSpecs will use that map to create the Specs.
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| helmChart, err := chartutil.LoadArchive(resp.Body) |
| Description: "Default plan for running helm charts", | ||
| Parameters: []apb.ParameterDescriptor{ | ||
| apb.ParameterDescriptor{ | ||
| Name: "repo", |
There was a problem hiding this comment.
Is it possible to just strip these from the ParameterDescriptor list in some post-processing if there's no reason to have them there?
| Required: false, | ||
| }, | ||
| apb.ParameterDescriptor{ | ||
| Name: "chart", |
|
LGTM! |
This describes a plan for supporting Helm Chart Repositories.