Move HelmRelease mutation prevention to operator

Before this change there was a ticker in the ChartChangeSync loop
which 'rereleased' all HelmReleases on the set interval, to prevent
mutations due to e.g. manual chart releases. This commit moves this
logic to the operator by utilizing the resync interval that can be set
on the shared informer.

On every resync interval, all objects known at that time to the
informer are redelivered as 'updates', causing the same effect as the
ticker process described above, but with several improvements.

1. The syncHandler validates if a HelmRelease still exists before
   attempting to run the release. This is an improvement compared to
   the blind run we did before, where in case a user had many
   HelmReleases, the Helm release could be removed at the time the
   release attempt was made.
2. We now act on information from one source (except for the
   `reconcileReleaseDef()` call on repository changes, see added
   comment).

cmd/helm-operator/main.go

@@ -171,17 +171,15 @@ func main() {
 	rel := release.New(log.With(logger, "component", "release"), helmClient)
 	chartSync := chartsync.New(
 		log.With(logger, "component", "chartsync"),
-		chartsync.Polling{Interval: *chartsSyncInterval},
 		chartsync.Clients{KubeClient: *kubeClient, IfClient: *ifClient},
 		rel,
 		chartsync.Config{LogDiffs: *logReleaseDiffs, UpdateDeps: *updateDependencies, GitTimeout: *gitTimeout},
 		*namespace,
-		statusUpdater,
 	)
 	chartSync.Run(shutdown, errc, shutdownWg)
 
 	nsOpt := ifinformers.WithNamespace(*namespace)
-	ifInformerFactory := ifinformers.NewSharedInformerFactoryWithOptions(ifClient, 30*time.Second, nsOpt)
+	ifInformerFactory := ifinformers.NewSharedInformerFactoryWithOptions(ifClient, *chartsSyncInterval, nsOpt)
 	fhrInformer := ifInformerFactory.Flux().V1beta1().HelmReleases()
 
 	// start FluxRelease informer

integrations/helm/chartsync/chartsync.go

@@ -75,10 +75,6 @@ const (
 	ReasonSuccess          = "HelmSuccess"
 )
 
-type Polling struct {
-	Interval time.Duration
-}
-
 type Clients struct {
 	KubeClient kubernetes.Clientset
 	IfClient   ifclientset.Clientset
@@ -106,7 +102,6 @@ type clone struct {
 }
 
 type ChartChangeSync struct {
-	Polling
 	logger     log.Logger
 	kubeClient kubernetes.Clientset
 	ifClient   ifclientset.Clientset
@@ -121,10 +116,9 @@ type ChartChangeSync struct {
 	namespace string
 }
 
-func New(logger log.Logger, polling Polling, clients Clients, release *release.Release, config Config, namespace string, statusUpdater *status.Updater) *ChartChangeSync {
+func New(logger log.Logger, clients Clients, release *release.Release, config Config, namespace string) *ChartChangeSync {
 	return &ChartChangeSync{
 		logger:     logger,
-		Polling:    polling,
 		kubeClient: clients.KubeClient,
 		ifClient:   clients.IfClient,
 		release:    release,
@@ -148,9 +142,6 @@ func (chs *ChartChangeSync) Run(stopCh <-chan struct{}, errc chan error, wg *syn
 			wg.Done()
 		}()
 
-		ticker := time.NewTicker(chs.Polling.Interval)
-		defer ticker.Stop()
-
 		for {
 			select {
 			case reposChanged := <-chs.mirrors.Changes():
@@ -239,18 +230,14 @@ func (chs *ChartChangeSync) Run(stopCh <-chan struct{}, errc chan error, wg *syn
 						}
 					}
 
+					// TODO(hidde): if we could somehow signal the
+					// operator an 'update' has happened we can control
+					// everything through the queue it maintains...
+					// Annotating the FHR could be an option as those
+					// count as updates but I am not sure if Flux would
+					// see those as in-cluster mutations.
 					chs.reconcileReleaseDef(fhr)
 				}
-			case <-ticker.C:
-				// Re-release any chart releases that have apparently
-				// changed in the cluster.
-				chs.logger.Log("info", fmt.Sprint("Start of releasesync"))
-				err := chs.reapplyReleaseDefs()
-				if err != nil {
-					chs.logger.Log("error", fmt.Sprintf("Failure to do manual release sync: %s", err))
-				}
-				chs.logger.Log("info", fmt.Sprint("End of releasesync"))
-
 			case <-stopCh:
 				chs.logger.Log("stopping", "true")
 				return
@@ -383,22 +370,6 @@ func (chs *ChartChangeSync) reconcileReleaseDef(fhr fluxv1beta1.HelmRelease) {
 	}
 }
 
-// reapplyReleaseDefs goes through the resource definitions and
-// reconciles them with Helm releases. This is a "backstop" for the
-// other sync processes, to cover the case of a release being changed
-// out-of-band (e.g., by someone using `helm upgrade`).
-func (chs *ChartChangeSync) reapplyReleaseDefs() error {
-	resources, err := chs.getCustomResources()
-	if err != nil {
-		return fmt.Errorf("failed to get HelmRelease resources from the API server: %s", err.Error())
-	}
-
-	for _, fhr := range resources {
-		chs.reconcileReleaseDef(fhr)
-	}
-	return nil
-}
-
 // DeleteRelease deletes the helm release associated with a
 // HelmRelease. This exists mainly so that the operator code can
 // call it when it is handling a resource deletion.

integrations/helm/operator/operator.go

@@ -297,15 +297,13 @@ func (c *Controller) enqueueUpdateJob(old, new interface{}) {
 		return
 	}
 
-	if diff := cmp.Diff(oldFhr.Spec, newFhr.Spec); diff != "" {
-		c.logger.Log("info", "UPGRADING release")
-		if c.logDiffs {
-			c.logger.Log("info", "Custom Resource driven release upgrade", "diff", diff)
-		} else {
-			c.logger.Log("info", "Custom Resource driven release upgrade")
-		}
-		c.enqueueJob(new)
+	c.logger.Log("info", "UPGRADING release")
+	if diff := cmp.Diff(oldFhr.Spec, newFhr.Spec); diff != "" && c.logDiffs {
+		c.logger.Log("info", "Custom Resource driven release upgrade", "diff", diff)
+	} else {
+		c.logger.Log("info", "Custom Resource driven release upgrade")
 	}
+	c.enqueueJob(new)
 }
 
 func (c *Controller) deleteRelease(fhr flux_v1beta1.HelmRelease) {