Skip to content

Commit

Permalink
Prevent sending empty patches in NetworkPolicy controller (gardener…
Browse files Browse the repository at this point in the history
…#8043)

* Introduce PatchOptions

* Add SkipEmptyPatch option

* Add SkipEmptyPatch when updating NetworkPolicies

* Add changes to CreateOrGetAndPatch functions

* Fix empty patch detection for optimistic locking

* Address review comment
  • Loading branch information
timuthy authored Jun 9, 2023
1 parent bb486a9 commit fb1a08d
Show file tree
Hide file tree
Showing 10 changed files with 302 additions and 51 deletions.
4 changes: 2 additions & 2 deletions cmd/gardenlet/app/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -358,7 +358,7 @@ func (g *garden) Start(ctx context.Context) error {
// and can ensure the required policies are created.
// TODO(timuthy, rfranzke): To be removed in a future release.
log.Info("Migrating all relevant shoot control plane services to create required network policies")
if err := g.migrateAllShootServicesForNetworkPolicies(ctx, log); err != nil {
if err := g.migrateAllShootServicesForNetworkPolicies(ctx); err != nil {
return err
}

Expand Down Expand Up @@ -453,7 +453,7 @@ func (g *garden) registerSeed(ctx context.Context, gardenClient client.Client) e
})
}

func (g *garden) migrateAllShootServicesForNetworkPolicies(ctx context.Context, log logr.Logger) error {
func (g *garden) migrateAllShootServicesForNetworkPolicies(ctx context.Context) error {
var taskFns []flow.TaskFn

// kube-apiserver services
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -413,7 +413,11 @@ webhooks:
vp.EXPECT().GetStorageClassesChartValues(ctx, cp, cluster).Return(storageClassesChartValues, nil)

// Handle shoot access secrets and legacy secret cleanup
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, shootAccessSecretsFunc(namespace)[0].Secret.Name), gomock.AssignableToTypeOf(&corev1.Secret{}))
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, shootAccessSecretsFunc(namespace)[0].Secret.Name), gomock.AssignableToTypeOf(&corev1.Secret{})).
Do(func(_ context.Context, _ client.ObjectKey, obj client.Object, _ ...client.GetOption) {
obj.SetResourceVersion("0")
})

c.EXPECT().Patch(ctx, gomock.AssignableToTypeOf(&corev1.Secret{}), gomock.Any()).
Do(func(ctx context.Context, obj client.Object, _ client.Patch, _ ...client.PatchOption) {
Expect(obj).To(DeepEqual(&corev1.Secret{
Expand All @@ -427,6 +431,7 @@ webhooks:
Labels: map[string]string{
"resources.gardener.cloud/purpose": "token-requestor",
},
ResourceVersion: "0",
},
Type: corev1.SecretTypeOpaque,
}))
Expand Down Expand Up @@ -539,7 +544,10 @@ webhooks:
vp.EXPECT().GetControlPlaneExposureChartValues(ctx, cpExposure, cluster, gomock.Any(), exposureChecksums).Return(controlPlaneExposureChartValues, nil)

// Handle shoot access secrets and legacy secret cleanup
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, exposureShootAccessSecretsFunc(namespace)[0].Secret.Name), gomock.AssignableToTypeOf(&corev1.Secret{}))
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, exposureShootAccessSecretsFunc(namespace)[0].Secret.Name), gomock.AssignableToTypeOf(&corev1.Secret{})).
Do(func(_ context.Context, _ client.ObjectKey, obj client.Object, _ ...client.GetOption) {
obj.SetResourceVersion("0")
})
c.EXPECT().Patch(ctx, gomock.AssignableToTypeOf(&corev1.Secret{}), gomock.Any()).
Do(func(ctx context.Context, obj client.Object, _ client.Patch, _ ...client.PatchOption) {
Expect(obj).To(DeepEqual(&corev1.Secret{
Expand All @@ -553,6 +561,7 @@ webhooks:
Labels: map[string]string{
"resources.gardener.cloud/purpose": "token-requestor",
},
ResourceVersion: "0",
},
Type: corev1.SecretTypeOpaque,
}))
Expand Down
36 changes: 29 additions & 7 deletions pkg/component/clusterautoscaler/cluster_autoscaler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,7 @@ var _ = Describe("ClusterAutoscaler", func() {
Labels: map[string]string{
"resources.gardener.cloud/purpose": "token-requestor",
},
ResourceVersion: "0",
},
Type: corev1.SecretTypeOpaque,
}
Expand Down Expand Up @@ -620,7 +621,10 @@ subjects:
c.EXPECT().Patch(ctx, gomock.AssignableToTypeOf(&rbacv1.ClusterRoleBinding{}), gomock.Any()),
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, serviceName), gomock.AssignableToTypeOf(&corev1.Service{})),
c.EXPECT().Patch(ctx, gomock.AssignableToTypeOf(&corev1.Service{}), gomock.Any()),
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, secret.Name), gomock.AssignableToTypeOf(&corev1.Secret{})),
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, secretName), gomock.AssignableToTypeOf(&corev1.Secret{})).
Do(func(_ context.Context, _ client.ObjectKey, obj client.Object, _ ...client.GetOption) {
obj.SetResourceVersion("0")
}),
c.EXPECT().Patch(ctx, gomock.AssignableToTypeOf(&corev1.Secret{}), gomock.Any()).Return(fakeErr),
)

Expand All @@ -635,7 +639,10 @@ subjects:
c.EXPECT().Patch(ctx, gomock.AssignableToTypeOf(&rbacv1.ClusterRoleBinding{}), gomock.Any()),
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, serviceName), gomock.AssignableToTypeOf(&corev1.Service{})),
c.EXPECT().Patch(ctx, gomock.AssignableToTypeOf(&corev1.Service{}), gomock.Any()),
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, secret.Name), gomock.AssignableToTypeOf(&corev1.Secret{})),
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, secretName), gomock.AssignableToTypeOf(&corev1.Secret{})).
Do(func(_ context.Context, _ client.ObjectKey, obj client.Object, _ ...client.GetOption) {
obj.SetResourceVersion("0")
}),
c.EXPECT().Patch(ctx, gomock.AssignableToTypeOf(&corev1.Secret{}), gomock.Any()),
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, deploymentName), gomock.AssignableToTypeOf(&appsv1.Deployment{})),
c.EXPECT().Patch(ctx, gomock.AssignableToTypeOf(&appsv1.Deployment{}), gomock.Any()).Return(fakeErr),
Expand All @@ -652,7 +659,10 @@ subjects:
c.EXPECT().Patch(ctx, gomock.AssignableToTypeOf(&rbacv1.ClusterRoleBinding{}), gomock.Any()),
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, serviceName), gomock.AssignableToTypeOf(&corev1.Service{})),
c.EXPECT().Patch(ctx, gomock.AssignableToTypeOf(&corev1.Service{}), gomock.Any()),
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, secret.Name), gomock.AssignableToTypeOf(&corev1.Secret{})),
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, secretName), gomock.AssignableToTypeOf(&corev1.Secret{})).
Do(func(_ context.Context, _ client.ObjectKey, obj client.Object, _ ...client.GetOption) {
obj.SetResourceVersion("0")
}),
c.EXPECT().Patch(ctx, gomock.AssignableToTypeOf(&corev1.Secret{}), gomock.Any()),
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, deploymentName), gomock.AssignableToTypeOf(&appsv1.Deployment{})),
c.EXPECT().Patch(ctx, gomock.AssignableToTypeOf(&appsv1.Deployment{}), gomock.Any()),
Expand All @@ -671,7 +681,10 @@ subjects:
c.EXPECT().Patch(ctx, gomock.AssignableToTypeOf(&rbacv1.ClusterRoleBinding{}), gomock.Any()),
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, serviceName), gomock.AssignableToTypeOf(&corev1.Service{})),
c.EXPECT().Patch(ctx, gomock.AssignableToTypeOf(&corev1.Service{}), gomock.Any()),
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, secret.Name), gomock.AssignableToTypeOf(&corev1.Secret{})),
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, secretName), gomock.AssignableToTypeOf(&corev1.Secret{})).
Do(func(_ context.Context, _ client.ObjectKey, obj client.Object, _ ...client.GetOption) {
obj.SetResourceVersion("0")
}),
c.EXPECT().Patch(ctx, gomock.AssignableToTypeOf(&corev1.Secret{}), gomock.Any()),
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, deploymentName), gomock.AssignableToTypeOf(&appsv1.Deployment{})),
c.EXPECT().Patch(ctx, gomock.AssignableToTypeOf(&appsv1.Deployment{}), gomock.Any()),
Expand All @@ -692,7 +705,10 @@ subjects:
c.EXPECT().Patch(ctx, gomock.AssignableToTypeOf(&rbacv1.ClusterRoleBinding{}), gomock.Any()),
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, serviceName), gomock.AssignableToTypeOf(&corev1.Service{})),
c.EXPECT().Patch(ctx, gomock.AssignableToTypeOf(&corev1.Service{}), gomock.Any()),
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, secret.Name), gomock.AssignableToTypeOf(&corev1.Secret{})),
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, secretName), gomock.AssignableToTypeOf(&corev1.Secret{})).
Do(func(_ context.Context, _ client.ObjectKey, obj client.Object, _ ...client.GetOption) {
obj.SetResourceVersion("0")
}),
c.EXPECT().Patch(ctx, gomock.AssignableToTypeOf(&corev1.Secret{}), gomock.Any()),
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, deploymentName), gomock.AssignableToTypeOf(&appsv1.Deployment{})),
c.EXPECT().Patch(ctx, gomock.AssignableToTypeOf(&appsv1.Deployment{}), gomock.Any()),
Expand All @@ -715,7 +731,10 @@ subjects:
c.EXPECT().Patch(ctx, gomock.AssignableToTypeOf(&rbacv1.ClusterRoleBinding{}), gomock.Any()),
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, serviceName), gomock.AssignableToTypeOf(&corev1.Service{})),
c.EXPECT().Patch(ctx, gomock.AssignableToTypeOf(&corev1.Service{}), gomock.Any()),
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, secret.Name), gomock.AssignableToTypeOf(&corev1.Secret{})),
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, secretName), gomock.AssignableToTypeOf(&corev1.Secret{})).
Do(func(_ context.Context, _ client.ObjectKey, obj client.Object, _ ...client.GetOption) {
obj.SetResourceVersion("0")
}),
c.EXPECT().Patch(ctx, gomock.AssignableToTypeOf(&corev1.Secret{}), gomock.Any()),
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, deploymentName), gomock.AssignableToTypeOf(&appsv1.Deployment{})),
c.EXPECT().Patch(ctx, gomock.AssignableToTypeOf(&appsv1.Deployment{}), gomock.Any()),
Expand Down Expand Up @@ -759,7 +778,10 @@ subjects:
Do(func(ctx context.Context, obj client.Object, _ client.Patch, _ ...client.PatchOption) {
Expect(obj).To(DeepEqual(service))
}),
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, secretName), gomock.AssignableToTypeOf(&corev1.Secret{})),
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, secretName), gomock.AssignableToTypeOf(&corev1.Secret{})).
Do(func(_ context.Context, _ client.ObjectKey, obj client.Object, _ ...client.GetOption) {
obj.SetResourceVersion("0")
}),
c.EXPECT().Patch(ctx, gomock.AssignableToTypeOf(&corev1.Secret{}), gomock.Any()).
Do(func(ctx context.Context, obj client.Object, _ client.Patch, _ ...client.PatchOption) {
Expect(obj).To(DeepEqual(secret))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,10 @@ var _ = Describe("KubeControllerManager", func() {
gomock.InOrder(
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, serviceName), gomock.AssignableToTypeOf(&corev1.Service{})),
c.EXPECT().Patch(ctx, gomock.AssignableToTypeOf(&corev1.Service{}), gomock.Any()),
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, secretName), gomock.AssignableToTypeOf(&corev1.Secret{})),
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, secretName), gomock.AssignableToTypeOf(&corev1.Secret{})).
Do(func(_ context.Context, _ client.ObjectKey, obj client.Object, _ ...client.GetOption) {
obj.SetResourceVersion("0")
}),
c.EXPECT().Patch(ctx, gomock.AssignableToTypeOf(&corev1.Secret{}), gomock.Any()).Return(fakeErr),
)

Expand All @@ -204,7 +207,10 @@ var _ = Describe("KubeControllerManager", func() {
gomock.InOrder(
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, serviceName), gomock.AssignableToTypeOf(&corev1.Service{})),
c.EXPECT().Patch(ctx, gomock.AssignableToTypeOf(&corev1.Service{}), gomock.Any()),
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, secretName), gomock.AssignableToTypeOf(&corev1.Secret{})),
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, secretName), gomock.AssignableToTypeOf(&corev1.Secret{})).
Do(func(_ context.Context, _ client.ObjectKey, obj client.Object, _ ...client.GetOption) {
obj.SetResourceVersion("0")
}),
c.EXPECT().Patch(ctx, gomock.AssignableToTypeOf(&corev1.Secret{}), gomock.Any()),
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, v1beta1constants.DeploymentNameKubeControllerManager), gomock.AssignableToTypeOf(&appsv1.Deployment{})),
c.EXPECT().Patch(ctx, gomock.AssignableToTypeOf(&appsv1.Deployment{}), gomock.Any()).Return(fakeErr),
Expand All @@ -217,7 +223,10 @@ var _ = Describe("KubeControllerManager", func() {
gomock.InOrder(
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, serviceName), gomock.AssignableToTypeOf(&corev1.Service{})),
c.EXPECT().Patch(ctx, gomock.AssignableToTypeOf(&corev1.Service{}), gomock.Any()),
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, secretName), gomock.AssignableToTypeOf(&corev1.Secret{})),
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, secretName), gomock.AssignableToTypeOf(&corev1.Secret{})).
Do(func(_ context.Context, _ client.ObjectKey, obj client.Object, _ ...client.GetOption) {
obj.SetResourceVersion("0")
}),
c.EXPECT().Patch(ctx, gomock.AssignableToTypeOf(&corev1.Secret{}), gomock.Any()),
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, v1beta1constants.DeploymentNameKubeControllerManager), gomock.AssignableToTypeOf(&appsv1.Deployment{})),
c.EXPECT().Patch(ctx, gomock.AssignableToTypeOf(&appsv1.Deployment{}), gomock.Any()),
Expand All @@ -232,7 +241,10 @@ var _ = Describe("KubeControllerManager", func() {
gomock.InOrder(
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, serviceName), gomock.AssignableToTypeOf(&corev1.Service{})),
c.EXPECT().Patch(ctx, gomock.AssignableToTypeOf(&corev1.Service{}), gomock.Any()),
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, secretName), gomock.AssignableToTypeOf(&corev1.Secret{})),
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, secretName), gomock.AssignableToTypeOf(&corev1.Secret{})).
Do(func(_ context.Context, _ client.ObjectKey, obj client.Object, _ ...client.GetOption) {
obj.SetResourceVersion("0")
}),
c.EXPECT().Patch(ctx, gomock.AssignableToTypeOf(&corev1.Secret{}), gomock.Any()),
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, v1beta1constants.DeploymentNameKubeControllerManager), gomock.AssignableToTypeOf(&appsv1.Deployment{})),
c.EXPECT().Patch(ctx, gomock.AssignableToTypeOf(&appsv1.Deployment{}), gomock.Any()),
Expand All @@ -248,7 +260,10 @@ var _ = Describe("KubeControllerManager", func() {
gomock.InOrder(
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, serviceName), gomock.AssignableToTypeOf(&corev1.Service{})),
c.EXPECT().Patch(ctx, gomock.AssignableToTypeOf(&corev1.Service{}), gomock.Any()),
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, secretName), gomock.AssignableToTypeOf(&corev1.Secret{})),
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, secretName), gomock.AssignableToTypeOf(&corev1.Secret{})).
Do(func(_ context.Context, _ client.ObjectKey, obj client.Object, _ ...client.GetOption) {
obj.SetResourceVersion("0")
}),
c.EXPECT().Patch(ctx, gomock.AssignableToTypeOf(&corev1.Secret{}), gomock.Any()),
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, v1beta1constants.DeploymentNameKubeControllerManager), gomock.AssignableToTypeOf(&appsv1.Deployment{})),
c.EXPECT().Patch(ctx, gomock.AssignableToTypeOf(&appsv1.Deployment{}), gomock.Any()),
Expand All @@ -266,7 +281,10 @@ var _ = Describe("KubeControllerManager", func() {
gomock.InOrder(
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, serviceName), gomock.AssignableToTypeOf(&corev1.Service{})),
c.EXPECT().Patch(ctx, gomock.AssignableToTypeOf(&corev1.Service{}), gomock.Any()),
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, secretName), gomock.AssignableToTypeOf(&corev1.Secret{})),
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, secretName), gomock.AssignableToTypeOf(&corev1.Secret{})).
Do(func(_ context.Context, _ client.ObjectKey, obj client.Object, _ ...client.GetOption) {
obj.SetResourceVersion("0")
}),
c.EXPECT().Patch(ctx, gomock.AssignableToTypeOf(&corev1.Secret{}), gomock.Any()),
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, v1beta1constants.DeploymentNameKubeControllerManager), gomock.AssignableToTypeOf(&appsv1.Deployment{})),
c.EXPECT().Patch(ctx, gomock.AssignableToTypeOf(&appsv1.Deployment{}), gomock.Any()),
Expand All @@ -286,7 +304,10 @@ var _ = Describe("KubeControllerManager", func() {
gomock.InOrder(
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, serviceName), gomock.AssignableToTypeOf(&corev1.Service{})),
c.EXPECT().Patch(ctx, gomock.AssignableToTypeOf(&corev1.Service{}), gomock.Any()),
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, secretName), gomock.AssignableToTypeOf(&corev1.Secret{})),
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, secretName), gomock.AssignableToTypeOf(&corev1.Secret{})).
Do(func(_ context.Context, _ client.ObjectKey, obj client.Object, _ ...client.GetOption) {
obj.SetResourceVersion("0")
}),
c.EXPECT().Patch(ctx, gomock.AssignableToTypeOf(&corev1.Secret{}), gomock.Any()),
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, v1beta1constants.DeploymentNameKubeControllerManager), gomock.AssignableToTypeOf(&appsv1.Deployment{})),
c.EXPECT().Patch(ctx, gomock.AssignableToTypeOf(&appsv1.Deployment{}), gomock.Any()),
Expand Down Expand Up @@ -347,6 +368,7 @@ var _ = Describe("KubeControllerManager", func() {
Labels: map[string]string{
"resources.gardener.cloud/purpose": "token-requestor",
},
ResourceVersion: "0",
},
Type: corev1.SecretTypeOpaque,
}
Expand Down Expand Up @@ -735,7 +757,10 @@ subjects:
Do(func(ctx context.Context, obj client.Object, _ client.Patch, _ ...client.PatchOption) {
Expect(obj).To(DeepEqual(serviceFor(version)))
}),
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, secretName), gomock.AssignableToTypeOf(&corev1.Secret{})),
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, secretName), gomock.AssignableToTypeOf(&corev1.Secret{})).
Do(func(_ context.Context, _ client.ObjectKey, obj client.Object, _ ...client.GetOption) {
obj.SetResourceVersion("0")
}),
c.EXPECT().Patch(ctx, gomock.AssignableToTypeOf(&corev1.Secret{}), gomock.Any()).
Do(func(ctx context.Context, obj client.Object, _ client.Patch, _ ...client.PatchOption) {
Expect(obj).To(DeepEqual(secret))
Expand Down Expand Up @@ -849,7 +874,10 @@ subjects:
Do(func(ctx context.Context, obj client.Object, _ client.Patch, _ ...client.PatchOption) {
Expect(obj).To(DeepEqual(serviceFor(version)))
}),
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, secretName), gomock.AssignableToTypeOf(&corev1.Secret{})),
c.EXPECT().Get(ctx, kubernetesutils.Key(namespace, secretName), gomock.AssignableToTypeOf(&corev1.Secret{})).
Do(func(_ context.Context, _ client.ObjectKey, obj client.Object, _ ...client.GetOption) {
obj.SetResourceVersion("0")
}),
c.EXPECT().Patch(ctx, gomock.AssignableToTypeOf(&corev1.Secret{}), gomock.Any()).
Do(func(ctx context.Context, obj client.Object, _ client.Patch, _ ...client.PatchOption) {
Expect(obj).To(DeepEqual(secret))
Expand Down
Loading

0 comments on commit fb1a08d

Please sign in to comment.