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

Add reconcile strategy for HelmCharts #308

Merged
merged 1 commit into from
Oct 8, 2021
Merged

Add reconcile strategy for HelmCharts #308

merged 1 commit into from
Oct 8, 2021

Conversation

arbourd
Copy link
Contributor

@arbourd arbourd commented Mar 13, 2021

Adds ReconcileStrategy option allowing the HelmChart CRD to use the source's revision (ex: 0.1.0+a96eff73a282d408a960086d978ca97a0a1fb2ef) in place of the Chart version (ex: 0.1.0), by using the Revision option. ReconcileStrategy defaults to ChartVersion.

This is only applicable to GitRepository and Bucket sources as HelmRepository has a clearly defined contract that Charts are (should be) immutable.

This change would allow Flux users to determine if they want to install/upgrade on each change to the source. A more detailed workflow and reasoning as to why is below:

Use Case

We manage a single repository which houses a single chart that manages all microservices. Beside the ./templates lives values-<environment> for staging, test, dev, etc. Some developers keep their own values-name.yaml lying around elsewhere. We utilize Helm to its templating-fullest for production workloads and local develop (like bringing up databases in containers).

We chose not to package the Helm chart and ship it to a Helm repository. This would serve no purpose and just requires another step. We also chose to place the values files beside the values.yaml similar to how Bitnami once did. Currently, we use ArgoCD which uses a similar-to GitRepository source, templates on each change, and deploys when differences occur.

I feel like our use of Helm is optimal for GitOps:

  1. We store the base (templates, values.yaml) in Git.
  2. We store the overrides or patches beside the base (values-production.yaml) in the same Git repository.
  3. We run CI on the base and overrides ensuring that the generated manifests comply with the Kubernetes API (against a KIND cluster).
  4. We run CI to diff the local changed state against the remote state of the desired deployment target on pull-requests (similar to a terraform plan)
  5. We update the chart many times a day mostly patching the values-env.yaml for changes to image tags.
  6. By not versioning the Chart.yaml, changes to the base templates affect all environments immediately, meaning that all base changes must be backwards compatible.
  7. We don't need to store the desired state (values) of our deployment in ConfigMaps, Secrets, Flux CRDs, ArgoCD CRDs, etc. It exists in a single place controlled, managed and audited by Git.

Without this change, Flux requires us to move values out of the Git repository and into other objects (mentioned above) or version the Chart.yaml with every change.

What happens with this change:

  1. Set ReconcileStrategy to Revision
  2. GitRepository changes reconcile with a new revision 0.1.0+f0faacd5164a875ebdbd9e3fab778f49c5aadbbc
  3. Triggers a HelmChart reconciliation with the revision 0.1.0+f0faacd5164a875ebdbd9e3fab778f49c5aadbbc
  4. Triggers a HelmRelease reconciliation with revision 0.1.0+f0faacd5164a875ebdbd9e3fab778f49c5aadbbc

This has been tested in an environment with a patched helm-controller. Changes to the HelmRelease and HelmChart CRD look like:

spec:
  chart:
    spec:
      chart: chart
      ignoreVersion: true
      sourceRef:
        kind: GitRepository
        name: flux-system
        namespace: flux-system
      valuesFile: chart/values-production.yaml
      version: '*'
  interval: 30s
  releaseName: prod
status:
  conditions:
  - lastTransitionTime: "2021-03-13T00:50:11Z"
    message: Release reconciliation succeeded
    reason: ReconciliationSucceeded
    status: "True"
    type: Ready
  - lastTransitionTime: "2021-03-13T00:50:11Z"
    message: Helm upgrade succeeded
    reason: UpgradeSucceeded
    status: "True"
    type: Released
  helmChart: flux-system/default-example
  lastAppliedRevision: 0.1.0+f0faacd5164a875ebdbd9e3fab778f49c5aadbbc
  lastAttemptedRevision: 0.1.0+f0faacd5164a875ebdbd9e3fab778f49c5aadbbc
  lastAttemptedValuesChecksum: da39a3ee5e6b4b0d3255bfef95601890afd80709
  lastReleaseRevision: 4
  observedGeneration: 1

Copy link
Member

@hiddeco hiddeco left a comment

Choose a reason for hiding this comment

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

Helm has a strict SemVer requirement for charts since v3.5.2.

Instead of using the revision of the source, we should probably create a valid SemVer using the allowed build metadata, e.g v0.1.0-alpha+<commit SHA>. Using the full revision is not possible as:

Identifiers MUST comprise only ASCII alphanumerics and hyphens [0-9A-Za-z-].
SemVer spec

This change also requires a documentation update, as the migration guide currently states that this Helm Operator-like feature is no longer supported.

@arbourd
Copy link
Contributor Author

arbourd commented Mar 13, 2021

Instead of using the revision of the source

Have I misunderstood how revision works? I'm not attempting to change the HelmChart CRD's Chart.Spec.Version, simply the revision metadata for tracking the differences between HelmChart CRDs. In my example, the Chart.Spec.Version is still 0.1.0 and never changes, satisfying Helm 3.5.2. My change versions the artifact differently and does not interact with the version of the chart at all.

The tarball from a HelmChart with revision: main/f0faacd5164a875ebdbd9e3fab778f49c5aadbbc yields this Chart.yaml:

apiVersion: v2
dependencies:
- condition: postgresql.enabled
  name: postgresql
  repository: https://charts.bitnami.com/bitnami
  version: 10.3.13
description: A Helm chart for an app
home: https://github.com/example/charts/tree/main/my-app
name: my-app
type: application
version: 0.1.0

The release looks like:

NAME      		NAMESPACE	REVISION	UPDATED                                	STATUS  	CHART     	APP VERSION
release-name	default  	4       	2021-03-13 00:50:05.404387943 +0000 UTC	deployed	my-app-0.1.0

This change also requires a documentation update

Willing to do whatever is necessary if you're willing to accept the feature. 😁

@hiddeco
Copy link
Member

hiddeco commented Mar 18, 2021

You are correct, but I think it would be better to change the SemVer to include the build number than to copy the revision from the GitRepository. Reason being that it would not require any changes on the helm-controller side, and would be reflected in the Helm state as-well (as the chart version is persisted to the Helm storage).

@kingdonb
Copy link
Member

kingdonb commented Apr 7, 2021

I am still excited about this contribution, I think the compromise sounds fairly well fleshed out.

This will enable many uses of Helm Controller that have been problematic since changing from Helm Operator.

@Legion2
Copy link

Legion2 commented Apr 12, 2021

Would this also support the "ignore version" feature for local chart dependencies? For example, we maintain multiple helm charts in a git repository and they depend on each other using local file references. If we change the values.yaml of a dependency without updating the Chart.yaml currently the changes are not applied to the cluster.

@kadzielawa
Copy link

kadzielawa commented Apr 12, 2021

does it will work also for fluxcd in version 2 ?

@hiddeco
Copy link
Member

hiddeco commented Apr 12, 2021

does it will work also for fluxcd in version 2 ?

All work done in fluxcd/*-controller repositories is for Flux v2 / GitOps Toolkit components.

Would this also support the "ignore version" feature for local chart dependencies?

If they live in the same repository, then yes.

@arbourd
Copy link
Contributor Author

arbourd commented Apr 19, 2021

Tests are failing so I think this has stopped working and I'll invest a bit more time on it this week.

@stealthybox
Copy link
Member

After we merge this, we should update the modified lines from fluxcd/flux2#1067

@hiddeco
Copy link
Member

hiddeco commented May 19, 2021

@arbourd are you still committed to finishing this PR?

@arbourd
Copy link
Contributor Author

arbourd commented May 19, 2021

@hiddeco Yup, just a little burnt out and taking a break.

@arbourd
Copy link
Contributor Author

arbourd commented Jun 10, 2021

Hey @hiddeco I will get back to this, this weekend or next week. Thank you for your patience everyone.

@hiddeco
Copy link
Member

hiddeco commented Jun 10, 2021

Awesome, and take your time! I have been chewing myself on things like #361 for ages :-)

@arbourd arbourd changed the title Allow HelmChart to use Source revision over version Add Reconcile Strategy Aug 4, 2021
@arbourd
Copy link
Contributor Author

arbourd commented Aug 4, 2021

Latest changes:

  • IgnoreVersion renamed to ReconcileStrategy
  • There are two strategies: ChartVersion and CommitSha. CommitSha is only functional with Git/Bucket based sources.
    • CommitSha is nice and descriptive, but this also should work for Bucket based revisions as well which are technically not commits.
  • The Helm version is ammended (when CommitSha strategy is used) to add the metadata to the version. For example, 0.2.0 becomes 0.2.0+44b98beaeced64a1ea2d4bc704d884c2498b27e7
    • I opted against a prerelease of alpha like the example above, so that Flux users can set their own prereleases without us impacting them

@arbourd arbourd marked this pull request as ready for review August 4, 2021 21:40
@hiddeco
Copy link
Member

hiddeco commented Aug 4, 2021

I will do a proper review of changes later, but would it be an idea to rename CommitSha to Revision so that it matches the terminology of the Artifact object?

@arbourd
Copy link
Contributor Author

arbourd commented Aug 4, 2021

Yup! That shoulda been obvious 😅

@arbourd
Copy link
Contributor Author

arbourd commented Aug 6, 2021

Got this running in our staging environment nicely 😁

@jack-evans
Copy link

Is there plans to get this merged in soon? Really keen to use this feature and it's one of the dependencies my team needs to move to flux2

api/v1beta1/helmchart_types.go Outdated Show resolved Hide resolved
controllers/helmchart_controller.go Outdated Show resolved Hide resolved
@hiddeco
Copy link
Member

hiddeco commented Sep 10, 2021

@jack-evans originally the plan was to get this merged after some refactor work (see reconcilers-dev branch), but some unexpected events happened that caused distractions for some time.

@arbourd if you take my review comments into consideration, update the documentation in the docs/folder, and smash (and sign-off) your commits. I'd be happy to get this landed. Please note that the libgit2 dependencies are currently in a less-optimal shape, and your build may fail, I am addressing this at the moment.

@hiddeco hiddeco added the area/git Git related issues and pull requests label Sep 10, 2021
@hiddeco hiddeco added area/helm Helm related issues and pull requests enhancement New feature or request labels Sep 10, 2021
@hiddeco hiddeco changed the title Add Reconcile Strategy Add reconcile strategy for HelmCharts Sep 10, 2021
@arbourd
Copy link
Contributor Author

arbourd commented Sep 13, 2021

Anything I'm missing from a docs point of view @hiddeco? I've documented the values (consts) similar to other examples, and I have a draft in the real docs which will be more structured.

@hiddeco
Copy link
Member

hiddeco commented Oct 1, 2021

Before the weekend update after a long period of waiting.

While I was debugging the issue that suddenly popped up in the CI, it became clear that we needed a more permanent, safe and controllable way of building against libgit2. Getting a proper solution for this took awhile, battling the various issues along the way, but I expect #437 to unblock everything next week.

Thanks for your patience 🌻

Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks @arbourd and @hiddeco

PS. I left a comment that's not blocking the merge.


version := v.String()
if chart.Spec.ReconcileStrategy == sourcev1.ReconcileStrategyRevision {
// Isolate the commit SHA from GitRepository type artifacts by removing the branch/ prefix.
Copy link
Member

Choose a reason for hiding this comment

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

We need to extract this in fluxcd/pkg as it's being used in notification-controller also.

This commit adds a `ReconcileStrategy` field to the `HelmChart` resource, which
allows defining when a new chart should be packaged and/or published if it
originates from a `Bucket` or `GitRepository` resource.

The two available strategies are:

- `ChartVersion`: creates a new artifact when the version of the Helm chart as
  defined in the `Chart.yaml` from the Source is different from the current
  version.
- `Revision`: creates a new artifact when the revision of the Source is
  different from the current revision.

For the `Revision` strategy, the (checksum part of the) revision of the
artifact the chart originatesfrom is added as SemVer metadata.

A chart from a `GitRepository` with Artifact revision
`main/f0faacd5164a875ebdbd9e3fab778f49c5aadbbc` and a chart with e.g. SemVer
`0.1.0` will be published as `0.1.0+f0faacd5164a875ebdbd9e3fab778f49c5aadbbc`.

A chart from a `Bucket` with Artifact revision
`f0faacd5164a875ebdbd9e3fab778f49c5aadbbc` and a chart with e.g. SemVer `0.1.0`
will be published as `0.1.0+f0faacd5164a875ebdbd9e3fab778f49c5aadbbc`.

Signed-off-by: Dylan Arbour <arbourd@users.noreply.github.com>
Copy link
Member

@hiddeco hiddeco left a comment

Choose a reason for hiding this comment

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

Made some tiny changes to the doc blocks and commit message, but the code was all looking OK @arbourd. Thank you very much 🙇

@hiddeco hiddeco merged commit d382eca into fluxcd:main Oct 8, 2021
@kadzielawa
Copy link

good job guys ! @hiddeco @stefanprodan

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/git Git related issues and pull requests area/helm Helm related issues and pull requests enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants