Merge pull request #1272 from ncabatoff/1235-charts-with-deps-continu…

…ously-upgraded

Avoid constant re-deployment of charts with multiple inline dependencies

chart/flux/README.md

@@ -113,6 +113,7 @@ The following tables lists the configurable parameters of the Weave Flux chart a
 | `helmOperator.repository` | Helm operator image repository | `quay.io/weaveworks/helm-operator`
 | `helmOperator.tag` | Helm operator image tag | `0.1.0-alpha`
 | `helmOperator.pullPolicy` | Helm operator image pull policy | `IfNotPresent`
+| `helmOperator.logReleaseDiffs` | Helm operator should log the diff when a chart release diverges (possibly insecure) | `false`
 | `helmOperator.tillerNamespace` | Namespace in which the Tiller server can be found | `kube-system`
 | `helmOperator.tls.enable` | Enable TLS for communicating with Tiller | `false`
 | `helmOperator.tls.verify` | Verify the Tiller certificate, also enables TLS when set to true | `false`

chart/flux/templates/helm-operator-deployment.yaml

@@ -70,6 +70,7 @@ spec:
         - --git-url={{ .Values.git.url }}
         - --git-branch={{ .Values.git.branch }}
         - --git-charts-path={{ .Values.git.chartsPath }}
+        - --log-release-diffs={{ .Values.helmOperator.logReleaseDiffs }}
         - --tiller-namespace={{ .Values.helmOperator.tillerNamespace }}
         {{- if .Values.helmOperator.tls.enable }}
         - --tiller-tls-enable={{ .Values.helmOperator.tls.enable }}

chart/flux/values.yaml

@@ -20,6 +20,7 @@ helmOperator:
   tag: 0.1.1-alpha
   pullPolicy: IfNotPresent
   tillerNamespace: kube-system
+  logReleaseDiffs: false
   tls:
     secretName: 'helm-client-certs'
     verify: false

cmd/helm-operator/main.go

@@ -46,6 +46,7 @@ var (
 
 	chartsSyncInterval *time.Duration
 	chartsSyncTimeout  *time.Duration
+	logReleaseDiffs    *bool
 
 	gitURL          *string
 	gitBranch       *string
@@ -91,6 +92,7 @@ func init() {
 
 	chartsSyncInterval = fs.Duration("charts-sync-interval", 3*time.Minute, "Interval at which to check for changed charts")
 	chartsSyncTimeout = fs.Duration("charts-sync-timeout", 1*time.Minute, "Timeout when checking for changed charts")
+	logReleaseDiffs = fs.Bool("log-release-diffs", false, "Log the diff when a chart release diverges; potentially insecure")
 
 	gitURL = fs.String("git-url", "", "URL of git repo with Helm Charts; e.g., git@github.com:weaveworks/flux-example")
 	gitBranch = fs.String("git-branch", "master", "branch of git repo")
@@ -207,7 +209,7 @@ func main() {
 	chartSync := chartsync.New(log.With(logger, "component", "chartsync"),
 		chartsync.Polling{Interval: *chartsSyncInterval, Timeout: *chartsSyncTimeout},
 		chartsync.Clients{KubeClient: *kubeClient, IfClient: *ifClient},
-		rel, repoConfig)
+		rel, repoConfig, *logReleaseDiffs)
 	chartSync.Run(shutdown, errc, shutdownWg)
 
 	// OPERATOR - CUSTOM RESOURCE CHANGE SYNC -----------------------------------------------
@@ -218,7 +220,8 @@ func main() {
 	// Reference to shared index informers for the FluxHelmRelease
 	fhrInformer := ifInformerFactory.Helm().V1alpha2().FluxHelmReleases()
 
-	opr := operator.New(log.With(logger, "component", "operator"), kubeClient, fhrInformer, rel, repoConfig)
+	opr := operator.New(log.With(logger, "component", "operator"), *logReleaseDiffs,
+		kubeClient, fhrInformer, rel, repoConfig)
 	// Starts handling k8s events related to the given resource kind
 	go ifInformerFactory.Start(shutdown)
 

integrations/helm/chartsync/chartsync.go

@@ -40,13 +40,18 @@ import (
 	"context"
 	"fmt"
 	"path/filepath"
+	"sort"
 	"sync"
 	"time"
 
 	"github.com/go-kit/kit/log"
+	google_protobuf "github.com/golang/protobuf/ptypes/any"
+	"github.com/google/go-cmp/cmp"
+	"github.com/ncabatoff/go-seq/seq"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/runtime"
 	"k8s.io/client-go/kubernetes"
+	hapi_chart "k8s.io/helm/pkg/proto/hapi/chart"
 	hapi_release "k8s.io/helm/pkg/proto/hapi/release"
 
 	ifv1 "github.com/weaveworks/flux/apis/helm.integrations.flux.weave.works/v1alpha2"
@@ -73,16 +78,18 @@ type ChartChangeSync struct {
 	ifClient   ifclientset.Clientset
 	release    *release.Release
 	config     helmop.RepoConfig
+	logDiffs   bool
 }
 
-func New(logger log.Logger, polling Polling, clients Clients, release *release.Release, config helmop.RepoConfig) *ChartChangeSync {
+func New(logger log.Logger, polling Polling, clients Clients, release *release.Release, config helmop.RepoConfig, logReleaseDiffs bool) *ChartChangeSync {
 	return &ChartChangeSync{
 		logger:     logger,
 		Polling:    polling,
 		kubeClient: clients.KubeClient,
 		ifClient:   clients.IfClient,
 		release:    release,
 		config:     config,
+		logDiffs:   logReleaseDiffs,
 	}
 }
 
@@ -309,6 +316,48 @@ func (chs *ChartChangeSync) getCustomResources() ([]ifv1.FluxHelmRelease, error)
 	return fhrs, nil
 }
 
+func sortStrings(ss []string) []string {
+	ret := append([]string{}, ss...)
+	sort.Strings(ret)
+	return ret
+}
+
+func sortChartFields(c *hapi_chart.Chart) *hapi_chart.Chart {
+	nc := hapi_chart.Chart{
+		Metadata:  &(*c.Metadata),
+		Templates: append([]*hapi_chart.Template{}, c.Templates...),
+		Files:     append([]*google_protobuf.Any{}, c.Files...),
+	}
+
+	if c.Values != nil {
+		nc.Values = &(*c.Values)
+	}
+
+	sort.SliceStable(nc.Files, func(i, j int) bool {
+		return seq.Compare(nc.Files[i], nc.Files[j]) < 0
+	})
+	sort.SliceStable(nc.Templates, func(i, j int) bool {
+		return seq.Compare(nc.Templates[i], nc.Templates[j]) < 0
+	})
+
+	nc.Metadata.Sources = sortStrings(nc.Metadata.Sources)
+	nc.Metadata.Keywords = sortStrings(nc.Metadata.Keywords)
+	nc.Metadata.Maintainers = append([]*hapi_chart.Maintainer{}, nc.Metadata.Maintainers...)
+	sort.SliceStable(nc.Metadata.Maintainers, func(i, j int) bool {
+		return seq.Compare(nc.Metadata.Maintainers[i], nc.Metadata.Maintainers[j]) < 0
+	})
+
+	nc.Dependencies = make([]*hapi_chart.Chart, len(c.Dependencies))
+	for i := range c.Dependencies {
+		nc.Dependencies[i] = sortChartFields(c.Dependencies[i])
+	}
+	sort.SliceStable(nc.Dependencies, func(i, j int) bool {
+		return seq.Compare(nc.Dependencies[i], nc.Dependencies[j]) < 0
+	})
+
+	return &nc
+}
+
 // shouldUpgrade returns true if the current running values or chart
 // don't match what the repo says we ought to be running, based on
 // doing a dry run install from the chart in the git repo.
@@ -317,8 +366,8 @@ func (chs *ChartChangeSync) shouldUpgrade(chartsRepo string, currRel *hapi_relea
 		return false, fmt.Errorf("No Chart release provided for %v", fhr.GetName())
 	}
 
-	currVals := currRel.GetConfig().GetRaw()
-	currChart := currRel.GetChart().String()
+	currVals := currRel.GetConfig()
+	currChart := currRel.GetChart()
 
 	// Get the desired release state
 	opts := release.InstallOptions{DryRun: true}
@@ -327,16 +376,25 @@ func (chs *ChartChangeSync) shouldUpgrade(chartsRepo string, currRel *hapi_relea
 	if err != nil {
 		return false, err
 	}
-	desVals := desRel.GetConfig().GetRaw()
-	desChart := desRel.GetChart().String()
+	desVals := desRel.GetConfig()
+	desChart := desRel.GetChart()
 
 	// compare values && Chart
-	if currVals != desVals {
-		chs.logger.Log("error", fmt.Sprintf("Release %s: values have diverged due to manual Chart release", currRel.GetName()))
+	if diff := cmp.Diff(currVals, desVals); diff != "" {
+		if chs.logDiffs {
+			chs.logger.Log("error", fmt.Sprintf("Release %s: values have diverged due to manual Chart release", currRel.GetName()), "diff", diff)
+		} else {
+			chs.logger.Log("error", fmt.Sprintf("Release %s: values have diverged due to manual Chart release", currRel.GetName()))
+		}
 		return true, nil
 	}
-	if currChart != desChart {
-		chs.logger.Log("error", fmt.Sprintf("Release %s: Chart has diverged due to manual Chart release", currRel.GetName()))
+
+	if diff := cmp.Diff(sortChartFields(currChart), sortChartFields(desChart)); diff != "" {
+		if chs.logDiffs {
+			chs.logger.Log("error", fmt.Sprintf("Release %s: Chart has diverged due to manual Chart release", currRel.GetName()), "diff", diff)
+		} else {
+			chs.logger.Log("error", fmt.Sprintf("Release %s: Chart has diverged due to manual Chart release", currRel.GetName()))
+		}
 		return true, nil
 	}
 

integrations/helm/operator/operator.go

@@ -9,6 +9,7 @@ import (
 
 	"github.com/go-kit/kit/log"
 	"github.com/golang/glog"
+	"github.com/google/go-cmp/cmp"
 	corev1 "k8s.io/api/core/v1"
 	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/util/runtime"
@@ -51,7 +52,8 @@ const (
 
 // Controller is the operator implementation for FluxHelmRelease resources
 type Controller struct {
-	logger log.Logger
+	logger   log.Logger
+	logDiffs bool
 
 	fhrLister iflister.FluxHelmReleaseLister
 	fhrSynced cache.InformerSynced
@@ -74,6 +76,7 @@ type Controller struct {
 // New returns a new helm-operator
 func New(
 	logger log.Logger,
+	logReleaseDiffs bool,
 	kubeclientset kubernetes.Interface,
 	fhrInformer fhrv1.FluxHelmReleaseInformer,
 	release *chartrelease.Release,
@@ -89,6 +92,7 @@ func New(
 
 	controller := &Controller{
 		logger:           logger,
+		logDiffs:         logReleaseDiffs,
 		fhrLister:        fhrInformer.Lister(),
 		fhrSynced:        fhrInformer.Informer().HasSynced,
 		release:          release,
@@ -327,9 +331,13 @@ func (c *Controller) enqueueUpateJob(old, new interface{}) {
 		return
 	}
 
-	if needsUpdate(oldFhr, newFhr) {
+	if diff := cmp.Diff(oldFhr.Spec, newFhr.Spec); diff != "" {
 		c.logger.Log("info", "UPGRADING release")
-		c.logger.Log("info", "Custom Resource driven release upgrade")
+		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)
 	}
 }
@@ -344,30 +352,3 @@ func (c *Controller) deleteRelease(fhr ifv1.FluxHelmRelease) {
 	}
 	return
 }
-
-// needsUpdate compares two FluxHelmRelease and determines if any changes occurred
-func needsUpdate(old, new ifv1.FluxHelmRelease) bool {
-	oldValues, err := old.Spec.Values.YAML()
-	if err != nil {
-		return false
-	}
-
-	newValues, err := new.Spec.Values.YAML()
-	if err != nil {
-		return false
-	}
-
-	if oldValues != newValues {
-		return true
-	}
-
-	if old.Spec.ReleaseName != new.Spec.ReleaseName {
-		return true
-	}
-
-	if old.Spec.ChartGitPath != new.Spec.ChartGitPath {
-		return true
-	}
-
-	return false
-}