Skip to content

Commit 658a6a6

Browse files
stevekuznetsovstevekuznetsov
committed
*: use a stable hash for client-side short-circuits
The previous approach here was to use go-spew, which, unfortunately, is not a stable and germane encoding for objects. We are encoding objects that we send to the Kubernetes API server, so features of go-spew like printing the capacity of slices are not germane. This led to hashes of otherwise identical (as far as the server was concerned) objects to be determined as hashing differently. Signed-off-by: Steve Kuznetsov <[email protected]>
1 parent 4655321 commit 658a6a6

File tree

18 files changed

+418
-230
lines changed

18 files changed

+418
-230
lines changed

‎.github/workflows/sanity.yaml

+1
Original file line numberDiff line numberDiff line change
@@ -27,3 +27,4 @@ jobs:
2727
uses: "golangci/golangci-lint-action@v3"
2828
with:
2929
version: "v1.51.1"
30+

‎go.mod

+1-1
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ go 1.20
55
require (
66
github.com/blang/semver/v4 v4.0.0
77
github.com/coreos/go-semver v0.3.0
8-
github.com/davecgh/go-spew v1.1.1
98
github.com/distribution/distribution v2.7.1+incompatible
109
github.com/evanphx/json-patch v5.6.0+incompatible
1110
github.com/fsnotify/fsnotify v1.6.0
@@ -89,6 +88,7 @@ require (
8988
github.com/coreos/go-systemd/v22 v22.5.0 // indirect
9089
github.com/cpuguy83/go-md2man/v2 v2.0.2 // indirect
9190
github.com/cyphar/filepath-securejoin v0.2.4 // indirect
91+
github.com/davecgh/go-spew v1.1.1 // indirect
9292
github.com/docker/cli v23.0.1+incompatible // indirect
9393
github.com/docker/distribution v2.8.2+incompatible // indirect
9494
github.com/docker/docker v23.0.3+incompatible // indirect

‎pkg/controller/install/deployment.go

+4-11
Original file line numberDiff line numberDiff line change
@@ -2,15 +2,13 @@ package install
22

33
import (
44
"fmt"
5-
"hash/fnv"
65
"time"
76

87
log "github.com/sirupsen/logrus"
98
appsv1 "k8s.io/api/apps/v1"
109
corev1 "k8s.io/api/core/v1"
1110
apierrors "k8s.io/apimachinery/pkg/api/errors"
1211
k8slabels "k8s.io/apimachinery/pkg/labels"
13-
"k8s.io/apimachinery/pkg/util/rand"
1412
"k8s.io/utils/pointer"
1513

1614
"github.com/operator-framework/api/pkg/operators/v1alpha1"
@@ -181,7 +179,10 @@ func (i *StrategyDeploymentInstaller) deploymentForSpec(name string, spec appsv1
181179
// to 2 ReplicaSets per deployment it manages, saving memory.
182180
dep.Spec.RevisionHistoryLimit = pointer.Int32(1)
183181

184-
hash = HashDeploymentSpec(dep.Spec)
182+
hash, err = hashutil.DeepHashObject(&dep.Spec)
183+
if err != nil {
184+
return nil, "", err
185+
}
185186
dep.Labels[DeploymentSpecHashLabelKey] = hash
186187

187188
deployment = dep
@@ -327,11 +328,3 @@ func (i *StrategyDeploymentInstaller) cleanupOrphanedDeployments(deploymentSpecs
327328

328329
return nil
329330
}
330-
331-
// HashDeploymentSpec calculates a hash given a copy of the deployment spec from a CSV, stripping any
332-
// operatorgroup annotations.
333-
func HashDeploymentSpec(spec appsv1.DeploymentSpec) string {
334-
hasher := fnv.New32a()
335-
hashutil.DeepHashObject(hasher, &spec)
336-
return rand.SafeEncodeString(fmt.Sprint(hasher.Sum32()))
337-
}

‎pkg/controller/install/deployment_test.go

+11-2
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"fmt"
55
"testing"
66

7+
hashutil "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/kubernetes/pkg/util/hash"
78
"github.com/stretchr/testify/assert"
89
"github.com/stretchr/testify/require"
910
appsv1 "k8s.io/api/apps/v1"
@@ -352,7 +353,11 @@ func TestInstallStrategyDeploymentCheckInstallErrors(t *testing.T) {
352353
dep := testDeployment("olm-dep-1", namespace, &mockOwner)
353354
dep.Spec.Template.SetAnnotations(map[string]string{"test": "annotation"})
354355
dep.Spec.RevisionHistoryLimit = &revisionHistoryLimit
355-
dep.SetLabels(labels.CloneAndAddLabel(dep.ObjectMeta.GetLabels(), DeploymentSpecHashLabelKey, HashDeploymentSpec(dep.Spec)))
356+
hash, err := hashutil.DeepHashObject(&dep.Spec)
357+
if err != nil {
358+
t.Fatal(err)
359+
}
360+
dep.SetLabels(labels.CloneAndAddLabel(dep.ObjectMeta.GetLabels(), DeploymentSpecHashLabelKey, hash))
356361
dep.Labels[OLMManagedLabelKey] = OLMManagedLabelValue
357362
dep.Status.Conditions = append(dep.Status.Conditions, appsv1.DeploymentCondition{
358363
Type: appsv1.DeploymentAvailable,
@@ -374,7 +379,11 @@ func TestInstallStrategyDeploymentCheckInstallErrors(t *testing.T) {
374379
deployment := testDeployment("olm-dep-1", namespace, &mockOwner)
375380
deployment.Spec.Template.SetAnnotations(map[string]string{"test": "annotation"})
376381
deployment.Spec.RevisionHistoryLimit = &revisionHistoryLimit
377-
deployment.SetLabels(labels.CloneAndAddLabel(dep.ObjectMeta.GetLabels(), DeploymentSpecHashLabelKey, HashDeploymentSpec(deployment.Spec)))
382+
hash, err = hashutil.DeepHashObject(&deployment.Spec)
383+
if err != nil {
384+
t.Fatal(err)
385+
}
386+
deployment.SetLabels(labels.CloneAndAddLabel(dep.ObjectMeta.GetLabels(), DeploymentSpecHashLabelKey, hash))
378387
fakeClient.CreateOrUpdateDeploymentReturns(&deployment, tt.createDeploymentErr)
379388
defer func() {
380389
require.Equal(t, &deployment, fakeClient.CreateOrUpdateDeploymentArgsForCall(0))

‎pkg/controller/install/webhook.go

+5-10
Original file line numberDiff line numberDiff line change
@@ -3,14 +3,12 @@ package install
33
import (
44
"context"
55
"fmt"
6-
"hash/fnv"
76

87
log "github.com/sirupsen/logrus"
98
admissionregistrationv1 "k8s.io/api/admissionregistration/v1"
109
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
1110
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
1211
"k8s.io/apimachinery/pkg/labels"
13-
"k8s.io/apimachinery/pkg/util/rand"
1412
"k8s.io/client-go/util/retry"
1513

1614
"github.com/operator-framework/api/pkg/operators/v1alpha1"
@@ -285,17 +283,14 @@ func addWebhookLabels(object metav1.Object, webhookDesc v1alpha1.WebhookDescript
285283
if labels == nil {
286284
labels = map[string]string{}
287285
}
286+
hash, err := hashutil.DeepHashObject(&webhookDesc)
287+
if err != nil {
288+
return err
289+
}
288290
labels[WebhookDescKey] = webhookDesc.GenerateName
289-
labels[WebhookHashKey] = HashWebhookDesc(webhookDesc)
291+
labels[WebhookHashKey] = hash
290292
labels[OLMManagedLabelKey] = OLMManagedLabelValue
291293
object.SetLabels(labels)
292294

293295
return nil
294296
}
295-
296-
// HashWebhookDesc calculates a hash given a webhookDescription
297-
func HashWebhookDesc(webhookDesc v1alpha1.WebhookDescription) string {
298-
hasher := fnv.New32a()
299-
hashutil.DeepHashObject(hasher, &webhookDesc)
300-
return rand.SafeEncodeString(fmt.Sprint(hasher.Sum32()))
301-
}

‎pkg/controller/operators/catalog/operator_test.go

+9-6
Original file line numberDiff line numberDiff line change
@@ -999,15 +999,15 @@ func TestSyncCatalogSources(t *testing.T) {
999999
},
10001000
expectedError: nil,
10011001
expectedObjs: []runtime.Object{
1002-
pod(*grpcCatalog),
1002+
pod(t, *grpcCatalog),
10031003
},
10041004
},
10051005
{
10061006
testName: "CatalogSourceWithGrpcImage/EnsuresCorrectImage",
10071007
namespace: "cool-namespace",
10081008
catalogSource: grpcCatalog,
10091009
k8sObjs: []runtime.Object{
1010-
pod(v1alpha1.CatalogSource{
1010+
pod(t, v1alpha1.CatalogSource{
10111011
ObjectMeta: metav1.ObjectMeta{
10121012
Name: "cool-catalog",
10131013
Namespace: "cool-namespace",
@@ -1031,7 +1031,7 @@ func TestSyncCatalogSources(t *testing.T) {
10311031
},
10321032
expectedError: nil,
10331033
expectedObjs: []runtime.Object{
1034-
pod(*grpcCatalog),
1034+
pod(t, *grpcCatalog),
10351035
},
10361036
},
10371037
{
@@ -1110,7 +1110,7 @@ func TestSyncCatalogSources(t *testing.T) {
11101110
},
11111111
}),
11121112
k8sObjs: []runtime.Object{
1113-
pod(*grpcCatalog),
1113+
pod(t, *grpcCatalog),
11141114
service(grpcCatalog.GetName(), grpcCatalog.GetNamespace()),
11151115
serviceAccount(grpcCatalog.GetName(), grpcCatalog.GetNamespace(), "", objectReference("init secret")),
11161116
},
@@ -2119,14 +2119,17 @@ func toManifest(t *testing.T, obj runtime.Object) string {
21192119
return string(raw)
21202120
}
21212121

2122-
func pod(s v1alpha1.CatalogSource) *corev1.Pod {
2122+
func pod(t *testing.T, s v1alpha1.CatalogSource) *corev1.Pod {
21232123
serviceAccount := &corev1.ServiceAccount{
21242124
ObjectMeta: metav1.ObjectMeta{
21252125
Namespace: s.GetNamespace(),
21262126
Name: s.GetName(),
21272127
},
21282128
}
2129-
pod := reconciler.Pod(&s, "registry-server", "central-opm", "central-util", s.Spec.Image, serviceAccount, s.GetLabels(), s.GetAnnotations(), 5, 10, 1001)
2129+
pod, err := reconciler.Pod(&s, "registry-server", "central-opm", "central-util", s.Spec.Image, serviceAccount, s.GetLabels(), s.GetAnnotations(), 5, 10, 1001)
2130+
if err != nil {
2131+
t.Fatal(err)
2132+
}
21302133
ownerutil.AddOwner(pod, &s, false, true)
21312134
return pod
21322135
}

‎pkg/controller/operators/olm/apiservices.go

+6-1
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"context"
55
"fmt"
66

7+
hashutil "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/kubernetes/pkg/util/hash"
78
log "github.com/sirupsen/logrus"
89
appsv1 "k8s.io/api/apps/v1"
910
apierrors "k8s.io/apimachinery/pkg/api/errors"
@@ -414,7 +415,11 @@ func (a *Operator) areWebhooksAvailable(csv *v1alpha1.ClusterServiceVersion) (bo
414415
// Create Webhook Label Selector
415416
webhookLabels := ownerutil.OwnerLabel(csv, v1alpha1.ClusterServiceVersionKind)
416417
webhookLabels[install.WebhookDescKey] = desc.GenerateName
417-
webhookLabels[install.WebhookHashKey] = install.HashWebhookDesc(desc)
418+
hash, err := hashutil.DeepHashObject(&desc)
419+
if err != nil {
420+
return false, err
421+
}
422+
webhookLabels[install.WebhookHashKey] = hash
418423
webhookSelector := labels.SelectorFromSet(webhookLabels).String()
419424

420425
webhookCount := 0

0 commit comments

Comments
 (0)