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.

Author: ecordell

Makefile

@@ -40,12 +40,12 @@ all: test build
 test: clean cover.out
 
 unit:
-	go test $(MOD_FLAGS) $(SPECIFIC_UNIT_TEST) -v -race -tags=json1 -count=1 ./pkg/...
+	go test $(MOD_FLAGS) $(SPECIFIC_UNIT_TEST) -v -race -count=1 ./pkg/...
 
 schema-check:
 
 cover.out: schema-check
-	go test $(MOD_FLAGS) -v -race -tags=json1 -coverprofile=cover.out -covermode=atomic \
+	go test $(MOD_FLAGS) -v -race -coverprofile=cover.out -covermode=atomic \
 		-coverpkg ./pkg/controller/... ./pkg/...
 
 coverage: cover.out

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"
 
+	v1 "github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/operators/v1"
 	"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
@@ -68,7 +72,7 @@ func NewStrategyDeploymentInstaller(strategyClient wrappers.InstallStrategyDeplo
 
 func (i *StrategyDeploymentInstaller) installDeployments(deps []v1alpha1.StrategyDeploymentSpec) error {
 	for _, d := range deps {
-		deployment, err := i.deploymentForSpec(d.Name, d.Spec)
+		deployment, _, err := i.deploymentForSpec(d.Name, d.Spec)
 		if err != nil {
 			return err
 		}
@@ -81,7 +85,7 @@ func (i *StrategyDeploymentInstaller) installDeployments(deps []v1alpha1.Strateg
 	return nil
 }
 
-func (i *StrategyDeploymentInstaller) deploymentForSpec(name string, spec appsv1.DeploymentSpec) (deployment *appsv1.Deployment, err error) {
+func (i *StrategyDeploymentInstaller) deploymentForSpec(name string, spec appsv1.DeploymentSpec) (deployment *appsv1.Deployment, hash string, err error) {
 	dep := &appsv1.Deployment{Spec: spec}
 	dep.SetName(name)
 	dep.SetNamespace(i.owner.GetNamespace())
@@ -104,6 +108,8 @@ func (i *StrategyDeploymentInstaller) deploymentForSpec(name string, spec appsv1
 		return
 	}
 
+	hash = HashDeploymentSpec(dep.Spec)
+	dep.Labels[DeploymentSpecHashLabelKey] = hash
 	deployment = dep
 	return
 }
@@ -204,45 +210,26 @@ func (i *StrategyDeploymentInstaller) checkForDeployments(deploymentSpecs []v1al
 			}
 		}
 
-		// check equality
-		calculated, err := i.deploymentForSpec(spec.Name, spec.Spec)
-		if err != nil {
-			return err
+		// check that the deployment spec hasn't changed since it was created
+		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)}
+		existingDeploymentSpecHash, ok := labels[DeploymentSpecHashLabelKey]
+		if !ok {
+			return StrategyError{Reason: StrategyErrDeploymentUpdated, Message: fmt.Sprintf("deployment doesn't have a spec hash, update it")}
 		}
-	}
-	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
+		_, calculatedDeploymentHash, err := i.deploymentForSpec(spec.Name, spec.Spec)
+		if err != nil {
+			return StrategyError{Reason: StrategyErrDeploymentUpdated, Message: fmt.Sprintf("couldn't calculate deployment spec hash: %v", err)}
 		}
-		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
+
+		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 +267,27 @@ func (i *StrategyDeploymentInstaller) cleanupOrphanedDeployments(deploymentSpecs
 
 	return nil
 }
+
+// HashDeploymentSpec calculates a hash given a copy of the deployment spec from a CSV, stripping any
+// operatorgroup annotations.
+func HashDeploymentSpec(spec appsv1.DeploymentSpec) string {
+	// TODO: the deployment installer should know about operatorgroups and ca hashes so that it can calculate these
+
+	// Remove operatorgroup annotations for deployment hashing - this simplifies calculation of the deployment hash
+	// because we don't need to worry about the current state of the operatorgroup
+	annotations := spec.Template.GetAnnotations()
+	if annotations != nil {
+		delete(annotations, v1.OperatorGroupAnnotationKey)
+		delete(annotations, v1.OperatorGroupNamespaceAnnotationKey)
+		delete(annotations, v1.OperatorGroupTargetsAnnotationKey)
+	}
+
+	if len(annotations) == 0 {
+		annotations = nil
+	}
+	spec.Template.SetAnnotations(annotations)
+
+	hasher := fnv.New32a()
+	hashutil.DeepHashObject(hasher, &spec)
+	return rand.SafeEncodeString(fmt.Sprint(hasher.Sum32()))
+}

pkg/controller/install/deployment_test.go

@@ -10,6 +10,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/kubernetes/pkg/util/labels"
 
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/operators/v1alpha1"
 	clientfakes "github.com/operator-framework/operator-lifecycle-manager/pkg/api/wrappers/wrappersfakes"
@@ -306,6 +307,7 @@ func TestInstallStrategyDeploymentCheckInstallErrors(t *testing.T) {
 
 			dep := testDeployment("olm-dep-1", namespace, &mockOwner)
 			dep.Spec.Template.SetAnnotations(map[string]string{"test": "annotation"})
+			dep.SetLabels(labels.CloneAndAddLabel(dep.ObjectMeta.GetLabels(), DeploymentSpecHashLabelKey, HashDeploymentSpec(dep.Spec)))
 			fakeClient.FindAnyDeploymentsMatchingLabelsReturns(
 				[]*appsv1.Deployment{
 					&dep,
@@ -321,6 +323,7 @@ func TestInstallStrategyDeploymentCheckInstallErrors(t *testing.T) {
 
 			deployment := testDeployment("olm-dep-1", namespace, &mockOwner)
 			deployment.Spec.Template.SetAnnotations(map[string]string{"test": "annotation"})
+			deployment.SetLabels(labels.CloneAndAddLabel(dep.ObjectMeta.GetLabels(), DeploymentSpecHashLabelKey, HashDeploymentSpec(deployment.Spec)))
 			fakeClient.CreateOrUpdateDeploymentReturns(&deployment, tt.createDeploymentErr)
 			defer func() {
 				require.Equal(t, &deployment, fakeClient.CreateOrUpdateDeploymentArgsForCall(0))

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/apiservices.go

@@ -36,6 +36,10 @@ const (
 	PackageserverName = "v1.packages.operators.coreos.com"
 )
 
+func secretName(apiServiceName string) string {
+	return apiServiceName + "-cert"
+}
+
 func (a *Operator) shouldRotateCerts(csv *v1alpha1.ClusterServiceVersion) bool {
 	now := metav1.Now()
 	if !csv.Status.CertsRotateAt.IsZero() && csv.Status.CertsRotateAt.Before(&now) {
@@ -339,6 +343,119 @@ func (a *Operator) installOwnedAPIServiceRequirements(csv *v1alpha1.ClusterServi
 	return strategyDetailsDeployment, nil
 }
 
+// updateDeploymentSpecsWithApiServiceData transforms an install strategy to include information about apiservices
+// it is used in generating hashes for deployment specs to know when something in the spec has changed,
+// but duplicates a lot of installAPIServiceRequirements and should be refactored.
+func (a *Operator) updateDeploymentSpecsWithApiServiceData(csv *v1alpha1.ClusterServiceVersion, strategy install.Strategy) (install.Strategy, error) {
+	// Assume the strategy is for a deployment
+	strategyDetailsDeployment, ok := strategy.(*v1alpha1.StrategyDetailsDeployment)
+	if !ok {
+		return nil, fmt.Errorf("unsupported InstallStrategy type")
+	}
+
+	apiDescs := csv.GetOwnedAPIServiceDescriptions()
+
+	// Return early if there are no owned APIServices
+	if len(apiDescs) == 0 {
+		return strategyDetailsDeployment, nil
+	}
+
+	depSpecs := make(map[string]appsv1.DeploymentSpec)
+	for _, sddSpec := range strategyDetailsDeployment.DeploymentSpecs {
+		depSpecs[sddSpec.Name] = sddSpec.Spec
+	}
+
+	for _, desc := range apiDescs {
+		apiServiceName := desc.GetName()
+		apiService, err := a.lister.APIRegistrationV1().APIServiceLister().Get(apiServiceName)
+		if err != nil {
+			return nil, fmt.Errorf("could not retrieve generated APIService: %v", err)
+		}
+
+		caBundle := apiService.Spec.CABundle
+		caHash := certs.PEMSHA256(caBundle)
+
+		depSpec, ok := depSpecs[desc.DeploymentName]
+		if !ok {
+			return nil, fmt.Errorf("StrategyDetailsDeployment missing deployment %s for owned APIService %s", desc.DeploymentName, fmt.Sprintf("%s.%s", desc.Version, desc.Group))
+		}
+
+		if depSpec.Template.Spec.ServiceAccountName == "" {
+			depSpec.Template.Spec.ServiceAccountName = "default"
+		}
+
+		// Update deployment with secret volume mount.
+		secret, err := a.lister.CoreV1().SecretLister().Secrets(csv.GetNamespace()).Get(secretName(apiServiceName))
+
+		volume := corev1.Volume{
+			Name: "apiservice-cert",
+			VolumeSource: corev1.VolumeSource{
+				Secret: &corev1.SecretVolumeSource{
+					SecretName: secret.GetName(),
+					Items: []corev1.KeyToPath{
+						{
+							Key:  "tls.crt",
+							Path: "apiserver.crt",
+						},
+						{
+							Key:  "tls.key",
+							Path: "apiserver.key",
+						},
+					},
+				},
+			},
+		}
+
+		replaced := false
+		for i, v := range depSpec.Template.Spec.Volumes {
+			if v.Name == volume.Name {
+				depSpec.Template.Spec.Volumes[i] = volume
+				replaced = true
+				break
+			}
+		}
+		if !replaced {
+			depSpec.Template.Spec.Volumes = append(depSpec.Template.Spec.Volumes, volume)
+		}
+
+		mount := corev1.VolumeMount{
+			Name:      volume.Name,
+			MountPath: "/apiserver.local.config/certificates",
+		}
+		for i, container := range depSpec.Template.Spec.Containers {
+			found := false
+			for j, m := range container.VolumeMounts {
+				if m.Name == mount.Name {
+					found = true
+					break
+				}
+
+				// Replace if mounting to the same location.
+				if m.MountPath == mount.MountPath {
+					container.VolumeMounts[j] = mount
+					found = true
+					break
+				}
+			}
+			if !found {
+				container.VolumeMounts = append(container.VolumeMounts, mount)
+			}
+
+			depSpec.Template.Spec.Containers[i] = container
+		}
+		depSpec.Template.ObjectMeta.SetAnnotations(map[string]string{OLMCAHashAnnotationKey: caHash})
+		depSpecs[desc.DeploymentName] = depSpec
+	}
+
+	// Replace all matching DeploymentSpecs in the strategy
+	for i, sddSpec := range strategyDetailsDeployment.DeploymentSpecs {
+		if depSpec, ok := depSpecs[sddSpec.Name]; ok {
+			strategyDetailsDeployment.DeploymentSpecs[i].Spec = depSpec
+		}
+	}
+	return strategyDetailsDeployment, nil
+}
+
 func (a *Operator) installAPIServiceRequirements(desc v1alpha1.APIServiceDescription, ca *certs.KeyPair, rotateAt time.Time, depSpec appsv1.DeploymentSpec, csv *v1alpha1.ClusterServiceVersion) (*appsv1.DeploymentSpec, error) {
 	apiServiceName := fmt.Sprintf("%s.%s", desc.Version, desc.Group)
 	logger := log.WithFields(log.Fields{
@@ -413,7 +530,7 @@ func (a *Operator) installAPIServiceRequirements(desc v1alpha1.APIServiceDescrip
 		},
 		Type: corev1.SecretTypeTLS,
 	}
-	secret.SetName(apiServiceName + "-cert")
+	secret.SetName(secretName(apiServiceName))
 	secret.SetNamespace(csv.GetNamespace())
 
 	// Add olmcasha hash as a label to the
@@ -526,7 +643,7 @@ func (a *Operator) installAPIServiceRequirements(desc v1alpha1.APIServiceDescrip
 		ownerutil.AddNonBlockingOwner(secretRoleBinding, csv)
 		_, err = a.opClient.CreateRoleBinding(secretRoleBinding)
 		if err != nil {
-			log.Warnf("could not create secret rolebinding with dep spec: %+v", depSpec)
+			log.Warnf("could not create secret rolebinding with dep spec: %#v", depSpec)
 			return nil, err
 		}
 	} else {

pkg/controller/operators/olm/operator.go

@@ -1382,11 +1382,16 @@ func (a *Operator) transitionCSVState(in v1alpha1.ClusterServiceVersion) (out *v
 			return
 		}
 
+		strategy, err := a.updateDeploymentSpecsWithApiServiceData(out, strategy)
+		if err != nil {
+			out.SetPhaseWithEvent(v1alpha1.CSVPhasePending, v1alpha1.CSVReasonNeedsReinstall, "calculated deployment install is bad", now, a.recorder)
+			return
+		}
 		if installErr := a.updateInstallStatus(out, installer, strategy, v1alpha1.CSVPhaseInstalling, v1alpha1.CSVReasonWaiting); installErr == nil {
 			logger.WithField("strategy", out.Spec.InstallStrategy.StrategyName).Infof("install strategy successful")
 		} else {
 			// Set phase to failed if it's been a long time since the last transition (5 minutes)
-			if metav1.Now().Sub(out.Status.LastTransitionTime.Time) >= 5*time.Minute {
+			if out.Status.LastTransitionTime != nil && metav1.Now().Sub(out.Status.LastTransitionTime.Time) >= 5*time.Minute {
 				out.SetPhaseWithEvent(v1alpha1.CSVPhaseFailed, v1alpha1.CSVReasonInstallCheckFailed, fmt.Sprintf("install timeout"), now, a.recorder)
 			}
 		}
@@ -1428,6 +1433,11 @@ func (a *Operator) transitionCSVState(in v1alpha1.ClusterServiceVersion) (out *v
 		}
 
 		// Check install status
+		strategy, err = a.updateDeploymentSpecsWithApiServiceData(out, strategy)
+		if err != nil {
+			out.SetPhaseWithEvent(v1alpha1.CSVPhasePending, v1alpha1.CSVReasonNeedsReinstall, "calculated deployment install is bad", now, a.recorder)
+			return
+		}
 		if installErr := a.updateInstallStatus(out, installer, strategy, v1alpha1.CSVPhaseFailed, v1alpha1.CSVReasonComponentUnhealthy); installErr != nil {
 			logger.WithField("strategy", out.Spec.InstallStrategy.StrategyName).Warnf("unhealthy component: %s", installErr)
 			return
@@ -1501,6 +1511,11 @@ func (a *Operator) transitionCSVState(in v1alpha1.ClusterServiceVersion) (out *v
 		}
 
 		// Check install status
+		strategy, err = a.updateDeploymentSpecsWithApiServiceData(out, strategy)
+		if err != nil {
+			out.SetPhaseWithEvent(v1alpha1.CSVPhasePending, v1alpha1.CSVReasonNeedsReinstall, "calculated deployment install is bad", now, a.recorder)
+			return
+		}
 		if installErr := a.updateInstallStatus(out, installer, strategy, v1alpha1.CSVPhasePending, v1alpha1.CSVReasonNeedsReinstall); installErr != nil {
 			logger.WithField("strategy", out.Spec.InstallStrategy.StrategyName).Warnf("needs reinstall: %s", installErr)
 		}
@@ -1924,7 +1939,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)