Skip to content

Commit deb58f1

Browse files
Merge pull request #1301 from ecordell/depspec-hash
Bug 1804812: fix(deployment): deployment spec hash
2 parents e7b6616 + 3b3b18a commit deb58f1

File tree

14 files changed

+362
-134
lines changed

14 files changed

+362
-134
lines changed

Makefile

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,12 +40,12 @@ all: test build
4040
test: clean cover.out
4141

4242
unit:
43-
go test $(MOD_FLAGS) $(SPECIFIC_UNIT_TEST) -v -race -tags=json1 -count=1 ./pkg/...
43+
go test $(MOD_FLAGS) $(SPECIFIC_UNIT_TEST) -v -race -count=1 ./pkg/...
4444

4545
schema-check:
4646

4747
cover.out: schema-check
48-
go test $(MOD_FLAGS) -v -race -tags=json1 -coverprofile=cover.out -covermode=atomic \
48+
go test $(MOD_FLAGS) -v -race -coverprofile=cover.out -covermode=atomic \
4949
-coverpkg ./pkg/controller/... ./pkg/...
5050

5151
coverage: cover.out

pkg/controller/install/deployment.go

Lines changed: 31 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -2,18 +2,21 @@ package install
22

33
import (
44
"fmt"
5+
"hash/fnv"
56

67
log "github.com/sirupsen/logrus"
78
appsv1 "k8s.io/api/apps/v1"
8-
"k8s.io/apimachinery/pkg/api/equality"
99
k8serrors "k8s.io/apimachinery/pkg/api/errors"
10-
"k8s.io/apimachinery/pkg/util/diff"
10+
"k8s.io/apimachinery/pkg/util/rand"
11+
hashutil "k8s.io/kubernetes/pkg/util/hash"
1112

1213
"github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/operators/v1alpha1"
1314
"github.com/operator-framework/operator-lifecycle-manager/pkg/api/wrappers"
1415
"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/ownerutil"
1516
)
1617

18+
const DeploymentSpecHashLabelKey = "olm.deployment-spec-hash"
19+
1720
type StrategyDeploymentInstaller struct {
1821
strategyClient wrappers.InstallStrategyDeploymentInterface
1922
owner ownerutil.Owner
@@ -68,7 +71,7 @@ func NewStrategyDeploymentInstaller(strategyClient wrappers.InstallStrategyDeplo
6871

6972
func (i *StrategyDeploymentInstaller) installDeployments(deps []v1alpha1.StrategyDeploymentSpec) error {
7073
for _, d := range deps {
71-
deployment, err := i.deploymentForSpec(d.Name, d.Spec)
74+
deployment, _, err := i.deploymentForSpec(d.Name, d.Spec)
7275
if err != nil {
7376
return err
7477
}
@@ -81,7 +84,7 @@ func (i *StrategyDeploymentInstaller) installDeployments(deps []v1alpha1.Strateg
8184
return nil
8285
}
8386

84-
func (i *StrategyDeploymentInstaller) deploymentForSpec(name string, spec appsv1.DeploymentSpec) (deployment *appsv1.Deployment, err error) {
87+
func (i *StrategyDeploymentInstaller) deploymentForSpec(name string, spec appsv1.DeploymentSpec) (deployment *appsv1.Deployment, hash string, err error) {
8588
dep := &appsv1.Deployment{Spec: spec}
8689
dep.SetName(name)
8790
dep.SetNamespace(i.owner.GetNamespace())
@@ -104,6 +107,8 @@ func (i *StrategyDeploymentInstaller) deploymentForSpec(name string, spec appsv1
104107
return
105108
}
106109

110+
hash = HashDeploymentSpec(dep.Spec)
111+
dep.Labels[DeploymentSpecHashLabelKey] = hash
107112
deployment = dep
108113
return
109114
}
@@ -204,45 +209,26 @@ func (i *StrategyDeploymentInstaller) checkForDeployments(deploymentSpecs []v1al
204209
}
205210
}
206211

207-
// check equality
208-
calculated, err := i.deploymentForSpec(spec.Name, spec.Spec)
209-
if err != nil {
210-
return err
212+
// check that the deployment spec hasn't changed since it was created
213+
labels := dep.GetLabels()
214+
if len(labels) == 0 {
215+
return StrategyError{Reason: StrategyErrDeploymentUpdated, Message: fmt.Sprintf("deployment doesn't have a spec hash, update it")}
211216
}
212-
213-
if !i.equalDeployments(&calculated.Spec, &dep.Spec) {
214-
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)}
217+
existingDeploymentSpecHash, ok := labels[DeploymentSpecHashLabelKey]
218+
if !ok {
219+
return StrategyError{Reason: StrategyErrDeploymentUpdated, Message: fmt.Sprintf("deployment doesn't have a spec hash, update it")}
215220
}
216-
}
217-
return nil
218-
}
219221

220-
func (i *StrategyDeploymentInstaller) equalDeployments(calculated, onCluster *appsv1.DeploymentSpec) bool {
221-
// ignore template annotations, OLM injects these elsewhere
222-
calculated.Template.Annotations = nil
223-
224-
// DeepDerivative doesn't treat `0` ints as unset. Stripping them here means we miss changes to these values,
225-
// but we don't end up getting bitten by the defaulter for deployments.
226-
for i, c := range onCluster.Template.Spec.Containers {
227-
o := calculated.Template.Spec.Containers[i]
228-
if o.ReadinessProbe != nil {
229-
o.ReadinessProbe.InitialDelaySeconds = c.ReadinessProbe.InitialDelaySeconds
230-
o.ReadinessProbe.TimeoutSeconds = c.ReadinessProbe.TimeoutSeconds
231-
o.ReadinessProbe.PeriodSeconds = c.ReadinessProbe.PeriodSeconds
232-
o.ReadinessProbe.SuccessThreshold = c.ReadinessProbe.SuccessThreshold
233-
o.ReadinessProbe.FailureThreshold = c.ReadinessProbe.FailureThreshold
222+
_, calculatedDeploymentHash, err := i.deploymentForSpec(spec.Name, spec.Spec)
223+
if err != nil {
224+
return StrategyError{Reason: StrategyErrDeploymentUpdated, Message: fmt.Sprintf("couldn't calculate deployment spec hash: %v", err)}
234225
}
235-
if o.LivenessProbe != nil {
236-
o.LivenessProbe.InitialDelaySeconds = c.LivenessProbe.InitialDelaySeconds
237-
o.LivenessProbe.TimeoutSeconds = c.LivenessProbe.TimeoutSeconds
238-
o.LivenessProbe.PeriodSeconds = c.LivenessProbe.PeriodSeconds
239-
o.LivenessProbe.SuccessThreshold = c.LivenessProbe.SuccessThreshold
240-
o.LivenessProbe.FailureThreshold = c.LivenessProbe.FailureThreshold
226+
227+
if existingDeploymentSpecHash != calculatedDeploymentHash {
228+
return StrategyError{Reason: StrategyErrDeploymentUpdated, Message: fmt.Sprintf("deployment changed old hash=%s, new hash=%s", existingDeploymentSpecHash, calculatedDeploymentHash)}
241229
}
242230
}
243-
244-
// DeepDerivative ensures that, for any non-nil, non-empty value in A, the corresponding value is set in B
245-
return equality.Semantic.DeepDerivative(calculated, onCluster)
231+
return nil
246232
}
247233

248234
// Clean up orphaned deployments after reinstalling deployments process
@@ -280,3 +266,11 @@ func (i *StrategyDeploymentInstaller) cleanupOrphanedDeployments(deploymentSpecs
280266

281267
return nil
282268
}
269+
270+
// HashDeploymentSpec calculates a hash given a copy of the deployment spec from a CSV, stripping any
271+
// operatorgroup annotations.
272+
func HashDeploymentSpec(spec appsv1.DeploymentSpec) string {
273+
hasher := fnv.New32a()
274+
hashutil.DeepHashObject(hasher, &spec)
275+
return rand.SafeEncodeString(fmt.Sprint(hasher.Sum32()))
276+
}

pkg/controller/install/deployment_test.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010
corev1 "k8s.io/api/core/v1"
1111
rbacv1 "k8s.io/api/rbac/v1"
1212
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
13+
"k8s.io/kubernetes/pkg/util/labels"
1314

1415
"github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/operators/v1alpha1"
1516
clientfakes "github.com/operator-framework/operator-lifecycle-manager/pkg/api/wrappers/wrappersfakes"
@@ -306,6 +307,7 @@ func TestInstallStrategyDeploymentCheckInstallErrors(t *testing.T) {
306307

307308
dep := testDeployment("olm-dep-1", namespace, &mockOwner)
308309
dep.Spec.Template.SetAnnotations(map[string]string{"test": "annotation"})
310+
dep.SetLabels(labels.CloneAndAddLabel(dep.ObjectMeta.GetLabels(), DeploymentSpecHashLabelKey, HashDeploymentSpec(dep.Spec)))
309311
fakeClient.FindAnyDeploymentsMatchingLabelsReturns(
310312
[]*appsv1.Deployment{
311313
&dep,
@@ -321,6 +323,7 @@ func TestInstallStrategyDeploymentCheckInstallErrors(t *testing.T) {
321323

322324
deployment := testDeployment("olm-dep-1", namespace, &mockOwner)
323325
deployment.Spec.Template.SetAnnotations(map[string]string{"test": "annotation"})
326+
deployment.SetLabels(labels.CloneAndAddLabel(dep.ObjectMeta.GetLabels(), DeploymentSpecHashLabelKey, HashDeploymentSpec(deployment.Spec)))
324327
fakeClient.CreateOrUpdateDeploymentReturns(&deployment, tt.createDeploymentErr)
325328
defer func() {
326329
require.Equal(t, &deployment, fakeClient.CreateOrUpdateDeploymentArgsForCall(0))

pkg/controller/install/errors.go

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,5 @@
11
package install
22

3-
import "fmt"
4-
53
const (
64
StrategyErrReasonComponentMissing = "ComponentMissing"
75
StrategyErrReasonAnnotationsMissing = "AnnotationsMissing"
@@ -32,7 +30,7 @@ var _ error = StrategyError{}
3230

3331
// Error implements the Error interface.
3432
func (e StrategyError) Error() string {
35-
return fmt.Sprintf("%s: %s", e.Reason, e.Message)
33+
return e.Message
3634
}
3735

3836
// IsErrorUnrecoverable reports if a given strategy error is one of the predefined unrecoverable types

pkg/controller/operators/olm/apiservices.go

Lines changed: 119 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,10 @@ const (
3636
PackageserverName = "v1.packages.operators.coreos.com"
3737
)
3838

39+
func secretName(apiServiceName string) string {
40+
return apiServiceName + "-cert"
41+
}
42+
3943
func (a *Operator) shouldRotateCerts(csv *v1alpha1.ClusterServiceVersion) bool {
4044
now := metav1.Now()
4145
if !csv.Status.CertsRotateAt.IsZero() && csv.Status.CertsRotateAt.Before(&now) {
@@ -339,6 +343,119 @@ func (a *Operator) installOwnedAPIServiceRequirements(csv *v1alpha1.ClusterServi
339343
return strategyDetailsDeployment, nil
340344
}
341345

346+
// updateDeploymentSpecsWithApiServiceData transforms an install strategy to include information about apiservices
347+
// it is used in generating hashes for deployment specs to know when something in the spec has changed,
348+
// but duplicates a lot of installAPIServiceRequirements and should be refactored.
349+
func (a *Operator) updateDeploymentSpecsWithApiServiceData(csv *v1alpha1.ClusterServiceVersion, strategy install.Strategy) (install.Strategy, error) {
350+
// Assume the strategy is for a deployment
351+
strategyDetailsDeployment, ok := strategy.(*v1alpha1.StrategyDetailsDeployment)
352+
if !ok {
353+
return nil, fmt.Errorf("unsupported InstallStrategy type")
354+
}
355+
356+
apiDescs := csv.GetOwnedAPIServiceDescriptions()
357+
358+
// Return early if there are no owned APIServices
359+
if len(apiDescs) == 0 {
360+
return strategyDetailsDeployment, nil
361+
}
362+
363+
depSpecs := make(map[string]appsv1.DeploymentSpec)
364+
for _, sddSpec := range strategyDetailsDeployment.DeploymentSpecs {
365+
depSpecs[sddSpec.Name] = sddSpec.Spec
366+
}
367+
368+
for _, desc := range apiDescs {
369+
apiServiceName := desc.GetName()
370+
apiService, err := a.lister.APIRegistrationV1().APIServiceLister().Get(apiServiceName)
371+
if err != nil {
372+
return nil, fmt.Errorf("could not retrieve generated APIService: %v", err)
373+
}
374+
375+
caBundle := apiService.Spec.CABundle
376+
caHash := certs.PEMSHA256(caBundle)
377+
378+
depSpec, ok := depSpecs[desc.DeploymentName]
379+
if !ok {
380+
return nil, fmt.Errorf("StrategyDetailsDeployment missing deployment %s for owned APIService %s", desc.DeploymentName, fmt.Sprintf("%s.%s", desc.Version, desc.Group))
381+
}
382+
383+
if depSpec.Template.Spec.ServiceAccountName == "" {
384+
depSpec.Template.Spec.ServiceAccountName = "default"
385+
}
386+
387+
// Update deployment with secret volume mount.
388+
secret, err := a.lister.CoreV1().SecretLister().Secrets(csv.GetNamespace()).Get(secretName(apiServiceName))
389+
390+
volume := corev1.Volume{
391+
Name: "apiservice-cert",
392+
VolumeSource: corev1.VolumeSource{
393+
Secret: &corev1.SecretVolumeSource{
394+
SecretName: secret.GetName(),
395+
Items: []corev1.KeyToPath{
396+
{
397+
Key: "tls.crt",
398+
Path: "apiserver.crt",
399+
},
400+
{
401+
Key: "tls.key",
402+
Path: "apiserver.key",
403+
},
404+
},
405+
},
406+
},
407+
}
408+
409+
replaced := false
410+
for i, v := range depSpec.Template.Spec.Volumes {
411+
if v.Name == volume.Name {
412+
depSpec.Template.Spec.Volumes[i] = volume
413+
replaced = true
414+
break
415+
}
416+
}
417+
if !replaced {
418+
depSpec.Template.Spec.Volumes = append(depSpec.Template.Spec.Volumes, volume)
419+
}
420+
421+
mount := corev1.VolumeMount{
422+
Name: volume.Name,
423+
MountPath: "/apiserver.local.config/certificates",
424+
}
425+
for i, container := range depSpec.Template.Spec.Containers {
426+
found := false
427+
for j, m := range container.VolumeMounts {
428+
if m.Name == mount.Name {
429+
found = true
430+
break
431+
}
432+
433+
// Replace if mounting to the same location.
434+
if m.MountPath == mount.MountPath {
435+
container.VolumeMounts[j] = mount
436+
found = true
437+
break
438+
}
439+
}
440+
if !found {
441+
container.VolumeMounts = append(container.VolumeMounts, mount)
442+
}
443+
444+
depSpec.Template.Spec.Containers[i] = container
445+
}
446+
depSpec.Template.ObjectMeta.SetAnnotations(map[string]string{OLMCAHashAnnotationKey: caHash})
447+
depSpecs[desc.DeploymentName] = depSpec
448+
}
449+
450+
// Replace all matching DeploymentSpecs in the strategy
451+
for i, sddSpec := range strategyDetailsDeployment.DeploymentSpecs {
452+
if depSpec, ok := depSpecs[sddSpec.Name]; ok {
453+
strategyDetailsDeployment.DeploymentSpecs[i].Spec = depSpec
454+
}
455+
}
456+
return strategyDetailsDeployment, nil
457+
}
458+
342459
func (a *Operator) installAPIServiceRequirements(desc v1alpha1.APIServiceDescription, ca *certs.KeyPair, rotateAt time.Time, depSpec appsv1.DeploymentSpec, csv *v1alpha1.ClusterServiceVersion) (*appsv1.DeploymentSpec, error) {
343460
apiServiceName := fmt.Sprintf("%s.%s", desc.Version, desc.Group)
344461
logger := log.WithFields(log.Fields{
@@ -413,7 +530,7 @@ func (a *Operator) installAPIServiceRequirements(desc v1alpha1.APIServiceDescrip
413530
},
414531
Type: corev1.SecretTypeTLS,
415532
}
416-
secret.SetName(apiServiceName + "-cert")
533+
secret.SetName(secretName(apiServiceName))
417534
secret.SetNamespace(csv.GetNamespace())
418535

419536
// Add olmcasha hash as a label to the
@@ -526,7 +643,7 @@ func (a *Operator) installAPIServiceRequirements(desc v1alpha1.APIServiceDescrip
526643
ownerutil.AddNonBlockingOwner(secretRoleBinding, csv)
527644
_, err = a.opClient.CreateRoleBinding(secretRoleBinding)
528645
if err != nil {
529-
log.Warnf("could not create secret rolebinding with dep spec: %+v", depSpec)
646+
log.Warnf("could not create secret rolebinding with dep spec: %#v", depSpec)
530647
return nil, err
531648
}
532649
} else {

0 commit comments

Comments
 (0)