Inconsistent code-styles and potential nil-dereferences

[dholbach]

From Ada Logics

Flux is composed of projects across different repositories and there is often similar logic happening across the controllers but performed in quite different ways. This leads to a more complex overall codebase and can make it difficult to reason about properties of the code.

Event recording and checking status of similar elements in the controllers is performed differently. This came up as an issue through fuzzing. Each of the controllers rely on an EventRecoder, and the way these EventRecorder variables are used differs between the controllers. Some controllers check for nil-status and others do not. The HelmRelease reconciler and the Kustomization reconciler assume that the EventRecorder is not nil in their respective event() implementations, whereas the other controls do not:

Helm Release Reconciler

https://github.com/fluxcd/helm-controller/blob/e9d31e9f1f8df5149b10ce7719b2d272f617a44f/controllers/helmrelease_controller.go#L739-L745

Kustomize Reconciler

https://github.com/fluxcd/kustomize-controller/blob/72bc54477aa89aadc40d1444d0f30b1e9963806f/controllers/kustomization_controller.go#L788-L795

Image Update Automation Reconciler

https://github.com/fluxcd/image-automation-controller/blob/bc3d7b21121851ffe75ebb9c9dcd530c38db3d4e/controllers/imageupdateautomation_controller.go#L717-L720

Git Repository Reconciler

https://github.com/fluxcd/source-controller/blob/c4d7e46b90dc48aac7d5c74def2a82e7b7ea9333/controllers/gitrepository_controller.go#L427-L432

Recommendation
The same code pattern should be used across the controllers. Through our analysis we determined the EventRecorder cannot be nil using the current main.go files and thus the nil check should be removed.

[pjbgf]

The two open PRs should align the way controllers deal with EventRecorder properties. The approach taken was to add nil checks at both helm and kustomize controllers. The reason being:

• The majority of the controllers are checking for nil.
• The kuberecorder.EventRecorder is an interface, which can be nil.
• The controllers are public, and can therefore be used outside the project and initialised in different ways.

[pjbgf]

@dholbach I believe this can be moved to done.

[pjbgf]

@dholbach should we also close this issue given that the work has concluded?