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

Support additional default values from a package #5692

Closed
3 tasks done
absoludity opened this issue Nov 21, 2022 · 2 comments · Fixed by #5885
Closed
3 tasks done

Support additional default values from a package #5692

absoludity opened this issue Nov 21, 2022 · 2 comments · Fixed by #5885
Assignees
Labels
component/apis-server Issue related to kubeapps api-server component/plugin-helm Issue related to kubeapps plugin to manage Helm packages component/ui Issue related to kubeapps UI kind/feature An issue that reports a feature (approved) to be implemented
Milestone

Comments

@absoludity
Copy link
Contributor

absoludity commented Nov 21, 2022

The first chunk of work that we can implement from the Kubeapps Custom Package Data is to support custom default value files when they are included within a package.

Use case 1: A single customized default values file

As a Kubeapps operator,
I want to repackage my packageA with a custom default values file (e.g. production-values.yaml)
So that any user who installs package A will be presented with my custom default values.

Use case 2: Multiple customized default values files

As a Kubeapps operator,
I want to provide my package A with multiple default values files (production, staging etc.),
So that any user who installs package A will be able to select one of these default value files as a base for the deployment.

Or, from the user point-of-view:

As a Kubeapps user,
I want to install my company's custom packageA,
So that I can choose from the defaults provided by my company for my situation.

Task breakdown

This task will focus enable the functionality for the Helm plugin only, though the

  • Define a format for including data in the packages themselves. This will be plugin-specific, so for Helm (and flux) it can be just matching on values-*.yaml file in the root. If we later decide to support this functionality with Carvel, it will similarly be an extra file in the bundle's root.
  • Design and implement support for the in-package custom defaults in the Kubeapps-APIs service, so that the Helm plugin is able to populate additionally a custom_defaults_values map, which, together with the existing “default_values”, allows the package’s original defaults to be overlaid with custom values.
  • Implement the dashboard changes so that, when present, custom defaults can be selected by the user. By default, the first custom defaults will be selected (sorted alpha-numerically). When only one custom default is present, it will be used without giving the user a selection at all.
@absoludity absoludity self-assigned this Nov 21, 2022
@absoludity absoludity added component/ui Issue related to kubeapps UI component/apis-server Issue related to kubeapps api-server component/plugin-helm Issue related to kubeapps plugin to manage Helm packages labels Nov 21, 2022
@ppbaena ppbaena added the kind/feature An issue that reports a feature (approved) to be implemented label Nov 22, 2022
@ppbaena ppbaena added this to the Custom values milestone Nov 22, 2022
absoludity added a commit that referenced this issue Nov 30, 2022
<!--
Before you open the request please review the following guidelines and
tips to help it be more easily integrated:

 - Describe the scope of your change - i.e. what the change does.
 - Describe any known limitations with your change.
- Please run any tests or examples that can exercise your modified code.

 Thank you for contributing!
 -->

### Description of the change

<!-- Describe the scope of your change - i.e. what the change does. -->
This PR begins the work specified in #5692 by updating the
`AvailablePackageDetail` message to include custom default values and
updating the tar utils so that they are fetched and returned (if
present) when the other files are fetched from a tarball.

### Benefits

<!-- What benefits will be realized by the code change? -->

### Possible drawbacks

<!-- Describe any known limitations with your change -->

### Applicable issues

<!-- Enter any applicable Issues here (You can reference an issue using
#) -->

- ref #5692

### Additional information

<!-- If there's anything else that's important and relevant to your pull
request, mention that information here.-->

Signed-off-by: Michael Nelson <[email protected]>
@absoludity absoludity changed the title Support custom default values from a package Support additional default values from a package Nov 30, 2022
absoludity added a commit that referenced this issue Dec 20, 2022
Signed-off-by: Michael Nelson <[email protected]>

<!--
Before you open the request please review the following guidelines and
tips to help it be more easily integrated:

 - Describe the scope of your change - i.e. what the change does.
 - Describe any known limitations with your change.
- Please run any tests or examples that can exercise your modified code.

 Thank you for contributing!
 -->

### Description of the change

<!-- Describe the scope of your change - i.e. what the change does. -->

While working on the next part of #5692 and noticed that the OCI code
seemed to be repeating similar functionality that exists already in a
re-usable lib. This PR just updates to use the existing code, which
required a little refactoring as OCI tarballs don't have a root
directory like the chart ones do.

### Benefits

<!-- What benefits will be realized by the code change? -->

Less code.

### Possible drawbacks

<!-- Describe any known limitations with your change -->

### Applicable issues

<!-- Enter any applicable Issues here (You can reference an issue using
#) -->

- ref #5692 

### Additional information

<!-- If there's anything else that's important and relevant to your pull
request, mention that information here.-->

Signed-off-by: Michael Nelson <[email protected]>
absoludity added a commit that referenced this issue Dec 20, 2022
Signed-off-by: Michael Nelson <[email protected]>

<!--
Before you open the request please review the following guidelines and
tips to help it be more easily integrated:

 - Describe the scope of your change - i.e. what the change does.
 - Describe any known limitations with your change.
- Please run any tests or examples that can exercise your modified code.

 Thank you for contributing!
 -->

### Description of the change

<!-- Describe the scope of your change - i.e. what the change does. -->
Updates the asset-syncer to fetch additional values files from a chart
package and updates the helm plugin in kubeapps-apis to populate the
`AvailablePackageDetail` with the additional values files when present.

### Benefits

<!-- What benefits will be realized by the code change? -->
The dashboard can now be updated to use or display these when present.

### Possible drawbacks

<!-- Describe any known limitations with your change -->

### Applicable issues

<!-- Enter any applicable Issues here (You can reference an issue using
#) -->

- ref #5692 

### Additional information

<!-- If there's anything else that's important and relevant to your pull
request, mention that information here.-->

Signed-off-by: Michael Nelson <[email protected]>
@absoludity
Copy link
Contributor Author

So the first case is now done (well, in review). I'm looking at the case 2. The installation of a package seems straight forward enough (small UX change), but where I'm uncertain with case 2 is when viewing an installed appication: Kubeapps will (currently) not have a way to know which default value was used by the user during install.

Fundamentally, there's no way for Kubeapps to know without us storing some indication with the release itself, which seems like the obvious solution: a custom values.yaml can include a custom-defaults-name field, for example:

# Copyright 2022 the Kubeapps contributors.
# SPDX-License-Identifier: Apache-2.0

image:
  repository: bitnami/nginx
  tag: "1.23.3-debian-11"
  pullPolicy: IfNotPresent

custom-defaults-name: values-production.yaml

This field will not be set when the package defaults were used, but if a custom default values was used, the field will be set to the name of the file. This will allow Kubeapps to identify the file to use as the base overlay, presenting this (with the diff view) to the user. The cost is small, the behaviour on failure is simple and expected (uses main default).

Another option would be to use a custom kubeapps annotation on the helm release object (secret or config map). The benefit would be that it's not visible to the user and so less likely to change unexpectedly (though still possible), and it's not dependent on the chart author remembering to add the field etc.

For this reason, I'm leaning toward the annotation rather than an explicit value in the values file. Thoughts?

absoludity added a commit that referenced this issue Jan 13, 2023
Signed-off-by: Michael Nelson <[email protected]>

<!--
Before you open the request please review the following guidelines and
tips to help it be more easily integrated:

 - Describe the scope of your change - i.e. what the change does.
 - Describe any known limitations with your change.
- Please run any tests or examples that can exercise your modified code.

 Thank you for contributing!
 -->

### Description of the change

<!-- Describe the scope of your change - i.e. what the change does. -->
This change uses the new support for custom default values files so
that:
- if a single custom default values file is found in the chart, it is
presented to the user so that authors can control the default values
presented to users.

Note that, since it is just an overlay over the charts `values.yaml`, it
can be used both to present a simpler selection of options to users, or
to present a different set of default values.

### Benefits

<!-- What benefits will be realized by the code change? -->

Fulfills use-case 1 of #5692 .

### Possible drawbacks

<!-- Describe any known limitations with your change -->

### Applicable issues

<!-- Enter any applicable Issues here (You can reference an issue using
#) -->

- ref #5692 

### Additional information

<!-- If there's anything else that's important and relevant to your pull
request, mention that information here.-->
Using the [custom default values in the
simplechart](https://github.com/vmware-tanzu/kubeapps/blob/5692-dashboard/integration/charts/simplechart-customvalues/values-custom.yaml),
before the user makes any changes, they are presented with those values
(only):


![5692-simplechart-no-changes](https://user-images.githubusercontent.com/497518/211962126-9e6335fd-8d55-4416-9878-445f80161bf4.png)

Any changes made are diffed against the custom default:

![5692-simplechart-smallchange](https://user-images.githubusercontent.com/497518/211962180-ceed059f-86cc-45fc-bd2e-4a7ed30cfaaf.png)

Signed-off-by: Michael Nelson <[email protected]>
absoludity added a commit that referenced this issue Jan 17, 2023
Signed-off-by: Michael Nelson <[email protected]>

<!--
Before you open the request please review the following guidelines and
tips to help it be more easily integrated:

 - Describe the scope of your change - i.e. what the change does.
 - Describe any known limitations with your change.
- Please run any tests or examples that can exercise your modified code.

 Thank you for contributing!
 -->

### Description of the change

<!-- Describe the scope of your change - i.e. what the change does. -->
Adds an example chart (which can later be used in CI) and the background
state work to support switching the selected default values for charts
that support it.

### Benefits

<!-- What benefits will be realized by the code change? -->
Next PR can focus on the UI work.

### Possible drawbacks

<!-- Describe any known limitations with your change -->

### Applicable issues

<!-- Enter any applicable Issues here (You can reference an issue using
#) -->

- Ref #5692 

### Additional information

<!-- If there's anything else that's important and relevant to your pull
request, mention that information here.-->

Signed-off-by: Michael Nelson <[email protected]>
absoludity added a commit that referenced this issue Jan 20, 2023
Signed-off-by: Michael Nelson <[email protected]>

<!--
Before you open the request please review the following guidelines and
tips to help it be more easily integrated:

 - Describe the scope of your change - i.e. what the change does.
 - Describe any known limitations with your change.
- Please run any tests or examples that can exercise your modified code.

 Thank you for contributing!
 -->

### Description of the change

This PR adds a default values selector to the DeploymentForm which is
only displayed if a package has multiple additional values file in the
package. For packages with multiple additional values files, the deploy
initially uses the package values.yaml, but the form explains why the
selector is available:


![5692-01-default-values](https://user-images.githubusercontent.com/497518/213103171-957ee0c6-b242-439b-934c-91f47a4323f5.png)

If the user selects a different default values file, the form is updated
with that files content, ready for editing:


![5692-02-custom-values-selected](https://user-images.githubusercontent.com/497518/213103368-34a9354b-2d5f-4a09-9d6a-3f7b86af3e5b.png)

And finally, if the user edits the fields, the diff is against the
selected default, as expected:

![5692-03-with-diff](https://user-images.githubusercontent.com/497518/213103483-06642f00-bce1-4227-96e5-682b1d02f5ea.png)

Also fixes issue to enable hyphens in custom values files (hard to find
bug found while testing this PR).


<!-- Describe the scope of your change - i.e. what the change does. -->

### Benefits

<!-- What benefits will be realized by the code change? -->

### Possible drawbacks

<!-- Describe any known limitations with your change -->

### Applicable issues

<!-- Enter any applicable Issues here (You can reference an issue using
#) -->

- fixes #5692 

### Additional information

<!-- If there's anything else that's important and relevant to your pull
request, mention that information here.-->

Tomorrow (actually, Friday), I will verify the behaviour for installed
packages being upgraded.

Signed-off-by: Michael Nelson <[email protected]>
@absoludity
Copy link
Contributor Author

For this reason, I'm leaning toward the annotation rather than an explicit value in the values file. Thoughts?

Answering myself here, but while working on the multi-value option, I realised that actually, by default, we show the use the deployed values when they upgrade, so it works quite well out of the box. For example, this is after deploying the chart with one of the custom default values selected... when upgrading it just looks normal since it uses the deployed values for the diff:

upgrade-multi-defaults

You can instead choose to switch the diff to be against the package defaults if you want to:

diff-against-package

I think this is a great starting point for the feature. If we later get feedback and have resources, we could enable diffing against other defaults in the package, but not necessary at the moment, in my opinion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/apis-server Issue related to kubeapps api-server component/plugin-helm Issue related to kubeapps plugin to manage Helm packages component/ui Issue related to kubeapps UI kind/feature An issue that reports a feature (approved) to be implemented
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants