Include chart annotations as event metadata

[eljulians]

This PR extends the Helm release controller to include Chart annotations in the registered event as metadata, along with the Chart revision already included.

This is my first Go code, so feel free to give opinionated feedback :)

Rationale

Use case

With the current notifications, is hard to be aware of what exactly was deployed, as just the Helm chart revision is included in the payload. If I wanted to know what specific change (or changeset) has been rolled out, it wouldn't be possible with the current setup. A workaround could be to abuse the chart version semver, but of course with several drawbacks, like needing to keep a 1-1 relationship between the char and app versions, having to come up with some specific encoding, having it to decode on the other end if a generic webhook receiver has been configured, and just probably being a bad practice.

It's probably reasonable to be able to plug some arbitrary data into the event delivered by Flux, specially considering that the Helm charts already provide annotations for this. I had a look into the open issues and it's something that was mentioned several times (#412 and #256 just to mention a couple of examples).

By including the chart annotations as part of the metadata, users can enrich their notifications as they wish by including the data they consider necessary for their own use cases.

Doing it with the chart annotations, the user experience doesn't change, as the chart needs to be updated for making a release anyways, and the data can be set at that point; or just left it empty otherwise if it's not needed, as they're optional.

Changes

The calls to the event function from reconcile are done passing nil as the chart is not loaded yet at that point.

The annotations are merged in the root of the event map instead of having a nested structure, for simplicity, e.g. the payload looks like:

{
    "revision": "1.0.2",
    "annotation_key1": "annotation_key2"
}

Instead of

{
    "revision": "1.0.2",
    "annotations": {
        "annotation_key1": "annotation_key2"
    }
}

It's assumed that the annotations is a plain string:string map, with no nested structures, according to the Helm specification. Setting annotations in a different format will make the Helm release fail itself (ArtifactFailed error), with no registered event by the Helm release controller, so there's no point on checking for these.

Summary of changes

• Add metadata *chart.Metadata parameter to event function in controllers/helmrelease_controller.go
• In reconcile function, modify the calls to event in to pass a nil value for this new param
• In reconcileRelease function, modify the calls to event in passing chart.Metadata
• In event function, initialize empty meta map at the beginning
• In event function, iterate through the newly added chart.Metadata parameter if not null, and merge the data to the meta map that gets send as part of the event

Testing

Manual testing

Below are the steps I followed for testing it with a local k8s cluster. The infrastructure and source code repositores I've used are private, but they're just the minimal for configuring helm releases and alerts, and a simple API server to make test releases for, respectively. I can provide more details if needed.

The source code contains an endpoint for which the HelmRelease notifications are configured for, which logs the request payload, so that there's no need for another service for the notifications in order to check the payload.

The following versions of the tools were used:

• minikube v1.25.2
• kubectl client v1.24.2
• flux v0.31.1

Setting the local cluster up

Start minikube:

minikube start

(Once the cluster is up, the source code docker image needs to be built loaded to minikube).

On the infra repository, bootstrap Flux:

flux bootstrap gitlab \
  --owner=julenpardo \
  --hostname=gitlab.hostname.com \
  --repository=fleet-fastapi-example \
  --branch=main \
  --path=./clusters/my-cluster \
  --personal

Running the Helm controller

Scale the already running Helm controller down:

kubectl -n flux-system scale deployment/helm-controller --replicas=0

Forward the source and notification controllers:

kubectl -n flux-system port-forward svc/source-controller 8081:80
kubectl -n flux-system port-forward svc/notification-controller 8082:80

Run the controller specifying the event address. Apparently the controller doesn't look for an environment variable for it unlike with the source controller, so I had to figure out how to do it, and I manually tweaked the Makefile the following way for testing purposes (I'm not sure of what the best workflow for this is):

diff --git a/Makefile b/Makefile
index f4bfbf3..18d5306 100644
--- a/Makefile
+++ b/Makefile
@@ -37,7 +37,7 @@ manager: generate fmt vet
 
 # Run against the configured Kubernetes cluster in ~/.kube/config
 run: generate fmt vet manifests
-	go run ./main.go
+	go run ./main.go --events-addr=http://localhost:8082
 
 # Install CRDs into a cluster
 install: manifests

Finally, run the controller:

export SOURCE_CONTROLLER_LOCALHOST=localhost:8081 
make run

Test: watch logs for Helm releases

Watch the logs of the fastapi-example pod:

kubectl logs -n fastapi -f $(kubectl get pods -n fastapi -o=name)

In the fastapi-example project directory, define a new Helm release by bumping version in charts/Chart.yaml. Also add some annotations, e.g.:

annotations:
    env: dev
    commit_sha: eb93c4033dc402de9d97c3d345c6ef1bce7d125f
    repo_host: https://github.com
    repo_owner: julenpardo
    repo_name: fastapi-exapmle

Commit, push and wait for the Helm controller to make the upgrade. Once done, the notification endpoint should have been hit, and the fastapi-example log output should show the payload including the annotations within metadata:

{
    "involvedObject": {
        "kind": "HelmRelease",
        "namespace": "flux-system",
        "name": "fastapi",
        "uid": "e92d963d-3c45-4a53-a2c5-11df902fe1f5",
        "apiVersion": "helm.toolkit.fluxcd.io/v2beta1",
        "resourceVersion": "3551"
    },
    "severity": "info",
    "timestamp": "2022-08-04T17:23:50Z",
    "message": "Helm upgrade succeeded",
    "reason": "info",
    "metadata": {
        "commit_sha": "eb93c4033dc402de9d97c3d345c6ef1bce7d125f",
        "env": "dev",
        "repo_host": "https://github.com",
        "repo_name": "fastapi-exapmle",
        "repo_owner": "julenpardo",
        "revision": "1.0.2"
    },
    "reportingController": "helm-controller",
    "reportingInstance": "jpardo-le-XXXXX"
}

(The Helm upgrade has started has been omitted for convenience, but the same metadata is included in the payload).

Make another release this time without any annotations, to check that everything is still okay without them, and check the logs:

{                                                                                                                                 
    "involvedObject": {                                                                                                           
        "kind": "HelmRelease",                                                                                                    
        "namespace": "flux-system",                                                                                               
        "name": "fastapi",                                                                                                        
        "uid": "e92d963d-3c45-4a53-a2c5-11df902fe1f5", 
        "apiVersion": "helm.toolkit.fluxcd.io/v2beta1",
        "resourceVersion": "5850"
    },                 
    "severity": "info",                                          
    "timestamp": "2022-08-04T17:40:01Z",
    "message": "Helm upgrade succeeded",
    "reason": "info",
    "metadata": {          
        "revision": "1.0.3"
    },                                                           
    "reportingController": "helm-controller",
    "reportingInstance": "jpardo-le-XXXXX"
}  

Unit and regression testing

I didn't add any automated tests mainly because currently there's no coverage for the flow for reconcileRelease, so I'm not sure of what would the best approach be to tackle this, and not end up being a pull request of adding tests rather than a feature. But suggestions are welcome.

[stefanprodan]

Thanks @julenpardo for this contribution.

I think we need to prefix all keys with helm to avoid collisions in notification-controller. This PR will have to wait till end of August as Hidde is in vacation and he will probably want this to be merged into the dev branch.

[eljulians]

@stefanprodan thanks a lot for such a quick response!

Should we then wait for Hidde to discuss how to prefix the keys?

[hiddeco]

Thank you for your contribution @julenpardo.

I am interested in adding this to the current refactoring work which lives in dev, but not all events have materialized there yet (which is something I am working on in addition to extensive test coverage now that I have returned). Once my PR is up which adds the majority of the events, I will circle back to this and see how we can properly pass on and/or prefix the data.

[eljulians]

Thanks a lot @hiddeco for checking it and the heads up!

[Delorien84]

Hello, this MR has been sitting here for a long time. This feature would be great, as we have same use case as @julenpardo.

[matheuscscp]

Hi @julenpardo thanks for putting this together, it's gonna be very useful!

A question: Where exactly do these chart annotations come from? Do they come from the HelmRelease .spec, .metadata, or .spec.chart.spec, or somewhere else?

I would really appreciate a way in which I could specify metadata in my HelmRelease manifest, like this: #682

Is your intention to also cover something like that or very similar? Could you please provide an example on where would I put my annotations/metadata exactly?

Thanks one more time!!

[moritzschmitz-oviva]

A question: Where exactly do these chart annotations come from? Do they come from the HelmRelease .spec, .metadata, or .spec.chart.spec, or somewhere else?

Probably this PR revolves around the chart annotations in the Chart.yaml, like here.

I also made another comment in #682.