helm: factor out logic from controller into package #485
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hiddeco
added
area/helm
hiddeco
force-pushed
the
helmchart-reconciler-dev
branch
3 times, most recently
from
4f5d87b
to
4ed3efe
Compare
hiddeco
force-pushed
the
helmchart-reconciler-dev
branch
from
4ed3efe
to
d9694ed
Compare
relu
reviewed
Nov 8, 2021
darkowlzz
reviewed
Nov 8, 2021
darkowlzz
reviewed
Nov 8, 2021
darkowlzz
reviewed
Nov 9, 2021
darkowlzz
reviewed
Nov 10, 2021
darkowlzz
reviewed
Nov 10, 2021
relu
reviewed
Nov 11, 2021
hiddeco
force-pushed
the
helmchart-reconciler-dev
branch
7 times, most recently
from
9b10815
to
11cc6e2
Compare
hiddeco
force-pushed
the
helmchart-reconciler-dev
branch
8 times, most recently
from
01c459d
to
315fbd7
Compare
This commits adds `LoadChartMetadataFromArchive` and `LoadChartMetadataFromDir` helpers to the internal `helm` package to be able to make observations to the Helm metadata file without loading the chart in full. The helpers are compatible with charts of the v1 format (with a separate `requirements.yaml` file), and an additional `LoadChartMetadata` helper is available to automatically call the right `LoadChartMetadataFrom*` version by looking at the file description of the given path. Signed-off-by: Hidde Beydals <hello@hidde.co>
This commits adds simple caching capabilities to the `ChartRepository`, which makes it possible to load the `Index` from a defined `CachePath` using `LoadFromCache()`, and to download the index to a new `CachePath` using `CacheIndex()`. In addition, the repository tests have been updated to make use of Gomega, and some missing ones have been added. Signed-off-by: Hidde Beydals <hello@hidde.co>
This commit starts with the optimization of the `DepenendencyManager`, ensuring the chart indexes are lazy loaded, and replacing the (limitless) concurrency with a configurable number of workers with a default of 1. Signed-off-by: Hidde Beydals <hello@hidde.co>
This commit starts with the creation of a `ChartBuilder` to facilitate the (conditional) build of a chart outside of the reconciler logic. The builder can be configured with a set of (modifying) options, which define together with the type of chart source what steps are taken during the build. To better facilitate the builder's needs and attempt to be more efficient, changes have been made to the `DependencyBuilder` and `ChartRepository` around (order of) operations and/or lazy-load capabilities. Signed-off-by: Hidde Beydals <hello@hidde.co>
This wires the `ChartRepository` changes into the reconciler to ensure it works. Signed-off-by: Hidde Beydals <hello@hidde.co>
This commit refactors the `ChartBuilder` that used to be a do-it-all struct into an interace with two implementations: - `LocalChartBuilder`: to build charts from a source on the local filesystem, either from a directory or from a packaged chart. - `RemoteChartBuilder`: to build charts from a remote Helm repository index. The new logic within the builders validates the size of the Helm size it works with based on the `Max*Size` global variables in the internal `helm` package, to address the recommendation from the security audit. In addition, changes `ClientOptionsFromSecret` takes now a directory argument which temporary files are placed in, making it easier to perform a garbage collection of the whole directory at the end of a reconcile run. Signed-off-by: Hidde Beydals <hello@hidde.co>
This commit starts wiring the factored out Helm chart build logic into the reconciler to ensure, validating the API capabilities. Signed-off-by: Hidde Beydals <hello@hidde.co>
With all the logic that used to reside in the `controllers` package factored into this package, it became cluttered. This commit tries to bring a bit more structure in place. Signed-off-by: Hidde Beydals <hello@hidde.co>
Dealing with some loose ends around making observations, and code style. The loaded byes of a chart are used as a revision to ensure e.g. periodic builds with unstable ordering of items do not trigger a false positive. Signed-off-by: Hidde Beydals <hello@hidde.co>
Add more chart local builder and dependency manager tests. Signed-off-by: Sunny <darkowlzz@protonmail.com>
- For remote builds, if the build option has a version metadata, the chart should be repackaged with the provided version. - Update internal/helm/testdata/charts/helmchart-0.1.0.tgz to include value files for testing merge chart values. Signed-off-by: Sunny <darkowlzz@protonmail.com>
Cached chart build tests for both local and remote builder. Signed-off-by: Sunny <darkowlzz@protonmail.com>
This makes the string less verbose and deals with the safe handling of some edge-case build states. Signed-off-by: Hidde Beydals <hello@hidde.co>
Signed-off-by: Hidde Beydals <hello@hidde.co>
This commit introduces a typed `BuildError` to be returned by `Builder.Build` in case of a failure. The `Reason` field in combination with `BuildErrorReason` can be used to signal (or determine) the reason of a returned error within the context of the build process. At present this is used to determine the correct Condition Reason, but in a future iteration this can be used to determine the negative polarity condition that should be set to indicate a precise failure to the user. Signed-off-by: Hidde Beydals <hello@hidde.co>
This includes a change of the defaults to more acceptible (higher) values. Signed-off-by: Sunny <darkowlzz@protonmail.com>
Previous usage while consistent, was incorrect, and inconsitent with the field in the API spec. Signed-off-by: Hidde Beydals <hello@hidde.co>
This allows custom configuration of the Helm file read limits, allowing
a user to overwrite them to their likenings if the defaults are too
restrictive for their specific setup using arguments:
`--helm-{index,chart,chart-file}-max-size`
Signed-off-by: Hidde Beydals <hello@hidde.co>
- Add some more documentation around chart builders - Ensure correct indentation in some doc comments - Provide example of using `errors.Is` for typed `BuildError` - Mention "bytes" in file size limit errors - Add missing copyright header Signed-off-by: Hidde Beydals <hello@hidde.co>
This makes it possible to signal reference (validation) errors happening before the build process actually starts dealing with the chart. At present, this does not have a more specific counterpart in the API, but this is expected to change when the conditions logic is revised. Signed-off-by: Hidde Beydals <hello@hidde.co>
By providing the Generation of the object that is getting reconciled as version metadata to the builder if any custom values files are defined, the Artifact revision changes if the specification does, ensuring consumers of the Artifact are able to react to changes in values (and perform a release). Signed-off-by: Hidde Beydals <hello@hidde.co>
This helps detect e.g. path or chart name reference changes. Signed-off-by: Hidde Beydals <hello@hidde.co>
hiddeco
force-pushed
the
helmchart-reconciler-dev
branch
2 times, most recently
from
caa7665
to
3594188
Compare
Signed-off-by: Hidde Beydals <hello@hidde.co>
hiddeco
force-pushed
the
helmchart-reconciler-dev
branch
from
3594188
to
2392326
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes: #470
This PR factors the Helm logic which previously resided in the reconciler code into the
internal/helmpackage.It introduces a
ChartBuilderwith two implementations to pull and (conditionally) repackage charts from a Helm repository or local file and/or directory based on a set of options.Both implementations attempt to make observations where possible to determine if they should return early, to keep the build process as short as possible and prevent loading chart data into memory when this isn't strictly required.
A build failure returns a typed
BuildError, which can be used to determine the reason of a returned error within the context of the build process, allowing one to provide an informative reason in e.g. a Status Condition. This also prepares us for more informative negative polarity conditions in a future PR (e.g.ValuesFilesMergeFailed == True).In addition, the
DependencyManager(and itsChartRepository) have been rewritten to allow on-demand fetching of Helm chart repositories, allow sharing of the same loaded index bytes for dependencies of a single chart that have the same upstream repository, and lazy-loading of those same index bytes.To mitigate against the issue described in Reading large files can crash flux with an out-of-memory bug #470, file size limits have been defined for Helm related files:
The limits can be changed with
--helm-{index,chart,chart-file}-max-size.When a
HelmChartdefinesValuesFiles, theGenerationof the object is added to the SemVer metadata (and thereby, the Artifact revision), ensuring changes in values can be noticed by Artifact consumers.💡 Help testing
ghcr.io/fluxcd/source-controller:rc-2392326bimage as a replacement for your currentv0.18.0source-controller image, the RC is backwards compatible with this version.