fix(deployment): use deployment spec hash for determining if change

 is needed on cluster

DeepDerivative will miss cases where values are removed, because an
empty value is treated as unchanged.

pkg/controller/install/deployment.go

@@ -2,18 +2,22 @@ package install
 
 import (
 	"fmt"
+	"hash/fnv"
+
+	"k8s.io/apimachinery/pkg/util/rand"
 
 	log "github.com/sirupsen/logrus"
 	appsv1 "k8s.io/api/apps/v1"
-	"k8s.io/apimachinery/pkg/api/equality"
 	k8serrors "k8s.io/apimachinery/pkg/api/errors"
-	"k8s.io/apimachinery/pkg/util/diff"
+	hashutil "k8s.io/kubernetes/pkg/util/hash"
 
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/operators/v1alpha1"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/api/wrappers"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/ownerutil"
 )
 
+const DeploymentSpecHashLabelKey = "olm.deployment-spec-hash"
+
 type StrategyDeploymentInstaller struct {
 	strategyClient      wrappers.InstallStrategyDeploymentInterface
 	owner               ownerutil.Owner
@@ -98,6 +102,7 @@ func (i *StrategyDeploymentInstaller) deploymentForSpec(name string, spec appsv1
 
 	ownerutil.AddNonBlockingOwner(dep, i.owner)
 	ownerutil.AddOwnerLabelsForKind(dep, i.owner, v1alpha1.ClusterServiceVersionKind)
+	dep.Labels[DeploymentSpecHashLabelKey] = HashDeploymentSpec(&spec)
 
 	if applyErr := i.initializers.Apply(dep); applyErr != nil {
 		err = applyErr
@@ -205,44 +210,20 @@ func (i *StrategyDeploymentInstaller) checkForDeployments(deploymentSpecs []v1al
 		}
 
 		// check equality
-		calculated, err := i.deploymentForSpec(spec.Name, spec.Spec)
-		if err != nil {
-			return err
+		labels := dep.GetLabels()
+		if len(labels) == 0 {
+			return StrategyError{Reason: StrategyErrDeploymentUpdated, Message: fmt.Sprintf("deployment doesn't have a spec hash, update it")}
 		}
-
-		if !i.equalDeployments(&calculated.Spec, &dep.Spec) {
-			return StrategyError{Reason: StrategyErrDeploymentUpdated, Message: fmt.Sprintf("deployment changed, rolling update with patch: %s\n%#v\n%#v", diff.ObjectDiff(dep.Spec.Template.Spec, calculated.Spec.Template.Spec), calculated.Spec.Template.Spec, dep.Spec.Template.Spec)}
-		}
-	}
-	return nil
-}
-
-func (i *StrategyDeploymentInstaller) equalDeployments(calculated, onCluster *appsv1.DeploymentSpec) bool {
-	// ignore template annotations, OLM injects these elsewhere
-	calculated.Template.Annotations = nil
-
-	// DeepDerivative doesn't treat `0` ints as unset. Stripping them here means we miss changes to these values,
-	// but we don't end up getting bitten by the defaulter for deployments.
-	for i, c := range onCluster.Template.Spec.Containers {
-		o := calculated.Template.Spec.Containers[i]
-		if o.ReadinessProbe != nil {
-			o.ReadinessProbe.InitialDelaySeconds = c.ReadinessProbe.InitialDelaySeconds
-			o.ReadinessProbe.TimeoutSeconds = c.ReadinessProbe.TimeoutSeconds
-			o.ReadinessProbe.PeriodSeconds = c.ReadinessProbe.PeriodSeconds
-			o.ReadinessProbe.SuccessThreshold = c.ReadinessProbe.SuccessThreshold
-			o.ReadinessProbe.FailureThreshold = c.ReadinessProbe.FailureThreshold
+		existingDeploymentSpecHash, ok := labels[DeploymentSpecHashLabelKey]
+		if !ok {
+			return StrategyError{Reason: StrategyErrDeploymentUpdated, Message: fmt.Sprintf("deployment doesn't have a spec hash, update it")}
 		}
-		if o.LivenessProbe != nil {
-			o.LivenessProbe.InitialDelaySeconds = c.LivenessProbe.InitialDelaySeconds
-			o.LivenessProbe.TimeoutSeconds = c.LivenessProbe.TimeoutSeconds
-			o.LivenessProbe.PeriodSeconds = c.LivenessProbe.PeriodSeconds
-			o.LivenessProbe.SuccessThreshold = c.LivenessProbe.SuccessThreshold
-			o.LivenessProbe.FailureThreshold = c.LivenessProbe.FailureThreshold
+		calculatedDeploymentHash := HashDeploymentSpec(&spec.Spec)
+		if existingDeploymentSpecHash != calculatedDeploymentHash {
+			return StrategyError{Reason: StrategyErrDeploymentUpdated, Message: fmt.Sprintf("deployment changed old hash=%s, new hash=%s", existingDeploymentSpecHash, calculatedDeploymentHash)}
 		}
 	}
-
-	// DeepDerivative ensures that, for any non-nil, non-empty value in A, the corresponding value is set in B
-	return equality.Semantic.DeepDerivative(calculated, onCluster)
+	return nil
 }
 
 // Clean up orphaned deployments after reinstalling deployments process
@@ -280,3 +261,9 @@ func (i *StrategyDeploymentInstaller) cleanupOrphanedDeployments(deploymentSpecs
 
 	return nil
 }
+
+func HashDeploymentSpec(spec *appsv1.DeploymentSpec) string {
+	hasher := fnv.New32a()
+	hashutil.DeepHashObject(hasher, *spec)
+	return rand.SafeEncodeString(fmt.Sprint(hasher.Sum32()))
+}

pkg/controller/install/deployment_test.go

@@ -4,6 +4,8 @@ import (
 	"fmt"
 	"testing"
 
+	"k8s.io/kubernetes/pkg/util/labels"
+
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	appsv1 "k8s.io/api/apps/v1"
@@ -306,6 +308,7 @@ func TestInstallStrategyDeploymentCheckInstallErrors(t *testing.T) {
 
 			dep := testDeployment("olm-dep-1", namespace, &mockOwner)
 			dep.Spec.Template.SetAnnotations(map[string]string{"test": "annotation"})
+			dep.ObjectMeta.SetLabels(labels.CloneAndAddLabel(dep.ObjectMeta.GetLabels(), DeploymentSpecHashLabelKey, HashDeploymentSpec(&strategy.DeploymentSpecs[0].Spec)))
 			fakeClient.FindAnyDeploymentsMatchingLabelsReturns(
 				[]*appsv1.Deployment{
 					&dep,

pkg/controller/install/errors.go

@@ -1,7 +1,5 @@
 package install
 
-import "fmt"
-
 const (
 	StrategyErrReasonComponentMissing   = "ComponentMissing"
 	StrategyErrReasonAnnotationsMissing = "AnnotationsMissing"
@@ -32,7 +30,7 @@ var _ error = StrategyError{}
 
 // Error implements the Error interface.
 func (e StrategyError) Error() string {
-	return fmt.Sprintf("%s: %s", e.Reason, e.Message)
+	return e.Message
 }
 
 // IsErrorUnrecoverable reports if a given strategy error is one of the predefined unrecoverable types

pkg/controller/operators/olm/operator.go

@@ -1924,7 +1924,8 @@ func (a *Operator) ensureLabels(in *v1alpha1.ClusterServiceVersion, labelSets ..
 		return in, nil
 	}
 
-	a.logger.WithField("labels", merged).Info("Labels updated!")
+	logger := a.logger.WithFields(logrus.Fields{"labels": merged, "csv": in.GetName(), "ns": in.GetNamespace()})
+	logger.Info("updated labels")
 
 	out := in.DeepCopy()
 	out.SetLabels(merged)

pkg/controller/operators/olm/operator_test.go

@@ -99,6 +99,11 @@ func ownerLabelFromCSV(name, namespace string) map[string]string {
 	}
 }
 
+func addDepSpecHashLabel(labels map[string]string, strat v1alpha1.NamedInstallStrategy) map[string]string {
+	labels[install.DeploymentSpecHashLabelKey] = install.HashDeploymentSpec(&strat.StrategySpec.DeploymentSpecs[0].Spec)
+	return labels
+}
+
 func apiResourcesForObjects(objs []runtime.Object) []*metav1.APIResourceList {
 	apis := []*metav1.APIResourceList{}
 	for _, o := range objs {
@@ -1116,7 +1121,7 @@ func TestTransitionCSV(t *testing.T) {
 				objs: []runtime.Object{
 					withLabels(
 						deployment("csv1-dep1", namespace, "sa", defaultTemplateAnnotations),
-						ownerLabelFromCSV("csv1", namespace),
+						addDepSpecHashLabel(ownerLabelFromCSV("csv1", namespace), installStrategy("csv1-dep1", nil, nil)),
 					),
 				},
 			},
@@ -1162,7 +1167,7 @@ func TestTransitionCSV(t *testing.T) {
 						deployment("a1", namespace, "sa", addAnnotations(defaultTemplateAnnotations, map[string]string{
 							OLMCAHashAnnotationKey: validCAHash,
 						})),
-						ownerLabelFromCSV("csv1", namespace),
+						addDepSpecHashLabel(ownerLabelFromCSV("csv1", namespace), installStrategy("a1", nil, nil)),
 					),
 					withAnnotations(keyPairToTLSSecret("v1.a1-cert", namespace, signedServingPair(time.Now().Add(24*time.Hour), validCA, []string{"v1-a1.ns", "v1-a1.ns.svc"})), map[string]string{
 						OLMCAHashAnnotationKey: validCAHash,
@@ -2186,7 +2191,7 @@ func TestTransitionCSV(t *testing.T) {
 				objs: []runtime.Object{
 					withLabels(
 						deployment("csv1-dep1", namespace, "sa", defaultTemplateAnnotations),
-						ownerLabelFromCSV("csv1", namespace),
+						addDepSpecHashLabel(ownerLabelFromCSV("csv1", namespace), installStrategy("csv1-dep1", nil, nil)),
 					),
 					deployment("extra-dep", namespace, "sa", nil),
 				},
@@ -2221,11 +2226,11 @@ func TestTransitionCSV(t *testing.T) {
 				objs: []runtime.Object{
 					withLabels(
 						deployment("csv1-dep1", namespace, "sa", defaultTemplateAnnotations),
-						map[string]string{
+						addDepSpecHashLabel(map[string]string{
 							ownerutil.OwnerKey:          "csv1",
 							ownerutil.OwnerNamespaceKey: namespace,
 							ownerutil.OwnerKind:         "ClusterServiceVersion",
-						},
+						}, installStrategy("csv1-dep1", nil, nil)),
 					),
 					deployment("extra-dep", namespace, "sa", nil),
 				},
@@ -2329,11 +2334,11 @@ func TestTransitionCSV(t *testing.T) {
 				objs: []runtime.Object{
 					withLabels(
 						deployment("csv1-dep1", namespace, "sa", defaultTemplateAnnotations),
-						ownerLabelFromCSV("csv1", namespace),
+						addDepSpecHashLabel(ownerLabelFromCSV("csv1", namespace), installStrategy("csv1-dep1", nil, nil)),
 					),
 					withLabels(
 						deployment("csv2-dep1", namespace, "sa", defaultTemplateAnnotations),
-						ownerLabelFromCSV("csv2", namespace),
+						addDepSpecHashLabel(ownerLabelFromCSV("csv2", namespace), installStrategy("csv2-dep1", nil, nil)),
 					),
 				},
 			},
@@ -2374,11 +2379,11 @@ func TestTransitionCSV(t *testing.T) {
 				objs: []runtime.Object{
 					withLabels(
 						deployment("csv1-dep1", namespace, "sa", defaultTemplateAnnotations),
-						ownerLabelFromCSV("csv1", namespace),
+						addDepSpecHashLabel(ownerLabelFromCSV("csv1", namespace), installStrategy("csv1-dep1", nil, nil)),
 					),
 					withLabels(
 						deployment("csv2-dep1", namespace, "sa", defaultTemplateAnnotations),
-						ownerLabelFromCSV("csv2", namespace),
+						addDepSpecHashLabel(ownerLabelFromCSV("csv2", namespace), installStrategy("csv2-dep1", nil, nil)),
 					),
 				},
 			},
@@ -2435,15 +2440,15 @@ func TestTransitionCSV(t *testing.T) {
 				objs: []runtime.Object{
 					withLabels(
 						deployment("csv1-dep1", namespace, "sa", defaultTemplateAnnotations),
-						ownerLabelFromCSV("csv1", namespace),
+						addDepSpecHashLabel(ownerLabelFromCSV("csv1", namespace), installStrategy("csv1-dep1", nil, nil)),
 					),
 					withLabels(
 						deployment("csv2-dep1", namespace, "sa", defaultTemplateAnnotations),
-						ownerLabelFromCSV("csv2", namespace),
+						addDepSpecHashLabel(ownerLabelFromCSV("csv2", namespace), installStrategy("csv2-dep1", nil, nil)),
 					),
 					withLabels(
 						deployment("csv3-dep1", namespace, "sa", defaultTemplateAnnotations),
-						ownerLabelFromCSV("csv3", namespace),
+						addDepSpecHashLabel(ownerLabelFromCSV("csv3", namespace), installStrategy("csv3-dep1", nil, nil)),
 					),
 				},
 			},
@@ -2494,15 +2499,15 @@ func TestTransitionCSV(t *testing.T) {
 				objs: []runtime.Object{
 					withLabels(
 						deployment("csv1-dep1", namespace, "sa", defaultTemplateAnnotations),
-						ownerLabelFromCSV("csv1", namespace),
+						addDepSpecHashLabel(ownerLabelFromCSV("csv1", namespace), installStrategy("csv1-dep1", nil, nil)),
 					),
 					withLabels(
 						deployment("csv2-dep1", namespace, "sa", defaultTemplateAnnotations),
-						ownerLabelFromCSV("csv2", namespace),
+						addDepSpecHashLabel(ownerLabelFromCSV("csv2", namespace), installStrategy("csv2-dep1", nil, nil)),
 					),
 					withLabels(
 						deployment("csv3-dep1", namespace, "sa", defaultTemplateAnnotations),
-						ownerLabelFromCSV("csv3", namespace),
+						addDepSpecHashLabel(ownerLabelFromCSV("csv3", namespace), installStrategy("csv3-dep1", nil, nil)),
 					),
 				},
 			},
@@ -2544,11 +2549,11 @@ func TestTransitionCSV(t *testing.T) {
 				objs: []runtime.Object{
 					withLabels(
 						deployment("csv2-dep1", namespace, "sa", defaultTemplateAnnotations),
-						ownerLabelFromCSV("csv2", namespace),
+						addDepSpecHashLabel(ownerLabelFromCSV("csv2", namespace), installStrategy("csv2-dep1", nil, nil)),
 					),
 					withLabels(
 						deployment("csv3-dep1", namespace, "sa", defaultTemplateAnnotations),
-						ownerLabelFromCSV("csv3", namespace),
+						addDepSpecHashLabel(ownerLabelFromCSV("csv3", namespace), installStrategy("csv3-dep1", nil, nil)),
 					),
 				},
 			},
@@ -2591,11 +2596,11 @@ func TestTransitionCSV(t *testing.T) {
 				objs: []runtime.Object{
 					withLabels(
 						deployment("csv2-dep1", namespace, "sa", defaultTemplateAnnotations),
-						ownerLabelFromCSV("csv2", namespace),
+						addDepSpecHashLabel(ownerLabelFromCSV("csv2", namespace), installStrategy("csv2-dep1", nil, nil)),
 					),
 					withLabels(
 						deployment("csv3-dep1", namespace, "sa", defaultTemplateAnnotations),
-						ownerLabelFromCSV("csv3", namespace),
+						addDepSpecHashLabel(ownerLabelFromCSV("csv3", namespace), installStrategy("csv3-dep1", nil, nil)),
 					),
 				},
 			},
@@ -3388,13 +3393,17 @@ func TestSyncOperatorGroups(t *testing.T) {
 
 	ownedDeployment := deployment(deploymentName, operatorNamespace, serviceAccount.GetName(), nil)
 	ownerutil.AddNonBlockingOwner(ownedDeployment, operatorCSV)
+	ownedDeployment.SetLabels(map[string]string{
+		install.DeploymentSpecHashLabelKey: install.HashDeploymentSpec(&installStrategy(deploymentName, permissions, nil).StrategySpec.DeploymentSpecs[0].Spec),
+	})
 
 	annotatedDeployment := ownedDeployment.DeepCopy()
 	annotatedDeployment.Spec.Template.SetAnnotations(map[string]string{v1.OperatorGroupTargetsAnnotationKey: operatorNamespace + "," + targetNamespace, v1.OperatorGroupAnnotationKey: "operator-group-1", v1.OperatorGroupNamespaceAnnotationKey: operatorNamespace})
 	annotatedDeployment.SetLabels(map[string]string{
 		"olm.owner":           "csv1",
 		"olm.owner.namespace": "operator-ns",
 		"olm.owner.kind":      "ClusterServiceVersion",
+		install.DeploymentSpecHashLabelKey: install.HashDeploymentSpec(&installStrategy(deploymentName, permissions, nil).StrategySpec.DeploymentSpecs[0].Spec),
 	})
 
 	annotatedGlobalDeployment := ownedDeployment.DeepCopy()
@@ -3403,6 +3412,7 @@ func TestSyncOperatorGroups(t *testing.T) {
 		"olm.owner":           "csv1",
 		"olm.owner.namespace": "operator-ns",
 		"olm.owner.kind":      "ClusterServiceVersion",
+		install.DeploymentSpecHashLabelKey: install.HashDeploymentSpec(&installStrategy(deploymentName, permissions, nil).StrategySpec.DeploymentSpecs[0].Spec),
 	})
 
 	role := &rbacv1.Role{

pkg/controller/registry/grpc/source_test.go

@@ -22,7 +22,7 @@ import (
 )
 
 func server(store registry.Query, port int) (func(), func()) {
-	lis, err := net.Listen("tcp", fmt.Sprintf(":%d", port))
+	lis, err := net.Listen("tcp", fmt.Sprintf("localhost:%d", port))
 	if err != nil {
 		logrus.Fatalf("failed to listen: %v", err)
 	}

pkg/lib/filemonitor/cert_updater_test.go

@@ -59,7 +59,7 @@ func TestOLMGetCertRotationFn(t *testing.T) {
 	require.NoError(t, err)
 
 	// find a free port to listen on and start server
-	listener, err := net.Listen("tcp", ":0")
+	listener, err := net.Listen("tcp", "localhost:0")
 	require.NoError(t, err)
 	freePort := listener.Addr().(*net.TCPAddr).Port
 	http.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {

pkg/package-server/provider/registry_test.go

@@ -41,7 +41,7 @@ const (
 
 func server() {
 	_ = os.Remove(dbName)
-	lis, err := net.Listen("tcp", ":"+port)
+	lis, err := net.Listen("tcp", "localhost:"+port)
 	if err != nil {
 		logrus.Fatalf("failed to listen: %v", err)
 	}