Skip to content

Commit 2b6be63

Browse files
tcp13equals2tcp13equals2
authored
Fix issue of observed webhook object changing prior to CA patch (#273)
* Fix issue of observed webhook object changing prior to CA patch * Remove retry * Remove safety for onEvent handler not being registered * changelog
1 parent e9aa5cc commit 2b6be63

File tree

4 files changed

+77
-15
lines changed

4 files changed

+77
-15
lines changed

‎CHANGELOG.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@
1515
* `k8s.io/api` from `v0.33.1` to `v0.33.3`
1616
* `k8s.io/apimachinery` from `v0.33.1` to `v0.33.3`
1717
* `k8s.io/client-go` from `v0.33.1` to `v0.33.3`
18-
* [ENHANCEMENT] Automatically patch new validating and mutating rollout-operator webhooks with the self-signed CA if they are created after rollout-operator starts. #262
18+
* [ENHANCEMENT] Automatically patch validating and mutating rollout-operator webhooks with the self-signed CA if they are created or modified after the rollout-operator starts. #262, #273
1919
* [ENHANCEMENT] Support for zone and partition aware pod disruption budgets, enabling finer control over pod eviction policies. #253
2020
* [BUGFIX] Always configure HTTP client with a timeout. #240
2121
* [BUGFIX] Use a StatefulSet's `.spec.serviceName` when constructing the delayed downscale endpoint for a pod. #258

‎integration/e2e_test.go

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,13 +4,15 @@ package integration
44

55
import (
66
"context"
7+
"encoding/json"
78
"testing"
89
"time"
910

1011
"github.com/stretchr/testify/require"
1112
corev1 "k8s.io/api/core/v1"
1213
policyv1beta1 "k8s.io/api/policy/v1beta1"
1314
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
15+
"k8s.io/apimachinery/pkg/types"
1416
"k8s.io/client-go/kubernetes"
1517
_ "k8s.io/client-go/kubernetes/scheme"
1618

@@ -149,6 +151,20 @@ func TestWebHookInformer(t *testing.T) {
149151

150152
t.Log("Await CABundle assignment")
151153
require.Eventually(t, awaitCABundleAssignment(3, ctx, api), time.Second*30, time.Millisecond*10, "New webhooks have CABundle added")
154+
155+
wh, err = api.AdmissionregistrationV1().ValidatingWebhookConfigurations().Get(ctx, wh.GetName(), metav1.GetOptions{})
156+
require.NoError(t, err)
157+
158+
t.Log("Update a webhook")
159+
wh.Webhooks[0].ClientConfig.CABundle = nil
160+
data, err := json.Marshal(wh)
161+
require.NoError(t, err)
162+
wh, err = api.AdmissionregistrationV1().ValidatingWebhookConfigurations().Patch(context.Background(), wh.GetName(), types.MergePatchType, data, metav1.PatchOptions{})
163+
require.NoError(t, err)
164+
require.Nil(t, wh.Webhooks[0].ClientConfig.CABundle)
165+
166+
t.Log("Await CABundle assignment after update")
167+
require.Eventually(t, awaitCABundleAssignment(3, ctx, api), time.Second*30, time.Millisecond*10, "New webhooks have CABundle added")
152168
}
153169
}
154170

‎pkg/tlscert/webhook_observer.go

Lines changed: 28 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package tlscert
22

33
import (
44
"fmt"
5+
"sync"
56
"time"
67

78
"github.com/go-kit/log"
@@ -32,6 +33,8 @@ type WebhookObserver struct {
3233
informerMutatingWebhooks cache.SharedIndexInformer
3334
log log.Logger
3435
stopCh chan struct{}
36+
lock sync.Mutex
37+
onEvent *WebhookConfigurationListener
3538
}
3639

3740
func NewWebhookObserver(kubeClient kubernetes.Interface, namespace string, logger log.Logger) *WebhookObserver {
@@ -45,31 +48,44 @@ func NewWebhookObserver(kubeClient kubernetes.Interface, namespace string, logge
4548
informerMutatingWebhooks: factory.Admissionregistration().V1().MutatingWebhookConfigurations().Informer(),
4649
log: logger,
4750
stopCh: make(chan struct{}),
51+
lock: sync.Mutex{},
4852
}
4953

5054
return c
5155
}
5256

57+
func (c *WebhookObserver) onWebHookConfigurationObserved(obj interface{}) {
58+
// avoid any concurrent modifications where the same webhook is passed in
59+
c.lock.Lock()
60+
defer c.lock.Unlock()
61+
62+
var err error
63+
switch obj := obj.(type) {
64+
case *admissionregistrationv1.ValidatingWebhookConfiguration:
65+
err = c.onEvent.OnValidatingWebhookConfiguration(obj)
66+
case *admissionregistrationv1.MutatingWebhookConfiguration:
67+
err = c.onEvent.OnMutatingWebhookConfiguration(obj)
68+
default:
69+
level.Error(c.log).Log("msg", "unknown object", "type", fmt.Sprintf("%T", obj))
70+
}
71+
if err != nil {
72+
level.Error(c.log).Log("msg", "unknown to call configuration listener", "err", err)
73+
}
74+
}
75+
5376
// Init starts watching for validating and mutating webhook configurations being added.
5477
// The given WebhookConfigurationListener is called when any webhook configurations is added.
5578
func (c *WebhookObserver) Init(onEvent *WebhookConfigurationListener) error {
79+
c.onEvent = onEvent
5680

5781
informers := []cache.SharedIndexInformer{c.informerValidatingWebhooks, c.informerMutatingWebhooks}
5882
for _, informer := range informers {
5983
if _, err := informer.AddEventHandler(cache.ResourceEventHandlerFuncs{
6084
AddFunc: func(obj interface{}) {
61-
var err error
62-
switch obj := obj.(type) {
63-
case *admissionregistrationv1.ValidatingWebhookConfiguration:
64-
err = onEvent.OnValidatingWebhookConfiguration(obj)
65-
case *admissionregistrationv1.MutatingWebhookConfiguration:
66-
err = onEvent.OnMutatingWebhookConfiguration(obj)
67-
default:
68-
level.Error(c.log).Log("msg", "Unknown object", "type", fmt.Sprintf("%T", obj))
69-
}
70-
if err != nil {
71-
level.Error(c.log).Log("msg", "Unable to call webhook configuration listener", "err", err)
72-
}
85+
c.onWebHookConfigurationObserved(obj)
86+
},
87+
UpdateFunc: func(_, new interface{}) {
88+
c.onWebHookConfigurationObserved(new)
7389
},
7490
}); err != nil {
7591
return errors.Wrap(err, "failed to add webhook listener")

‎pkg/tlscert/webhook_observer_test.go

Lines changed: 32 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,14 @@ package tlscert
22

33
import (
44
"context"
5+
"encoding/json"
56
"testing"
67
"time"
78

89
"github.com/stretchr/testify/require"
910
admissionregistrationv1 "k8s.io/api/admissionregistration/v1"
1011
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
12+
"k8s.io/apimachinery/pkg/types"
1113
k8sfake "k8s.io/client-go/kubernetes/fake"
1214
)
1315

@@ -69,8 +71,9 @@ func TestWebhookObserver_ListenerInvoked(t *testing.T) {
6971
require.Empty(t, observedValidatingWebhooks)
7072
require.Empty(t, observedMutatingWebhooks)
7173

72-
_, err := client.AdmissionregistrationV1().ValidatingWebhookConfigurations().Create(context.Background(), validatingWebhookConfiguration("validating-webhook"), metav1.CreateOptions{})
74+
validatingWebhook, err := client.AdmissionregistrationV1().ValidatingWebhookConfigurations().Create(context.Background(), validatingWebhookConfiguration("validating-webhook"), metav1.CreateOptions{})
7375
require.NoError(t, err)
76+
require.NotEmpty(t, validatingWebhook)
7477

7578
// wait for the informer to be aware of this create
7679
task := func() bool {
@@ -80,15 +83,42 @@ func TestWebhookObserver_ListenerInvoked(t *testing.T) {
8083
require.Equal(t, "validating-webhook", observedValidatingWebhooks[0].Name)
8184
require.Empty(t, observedMutatingWebhooks)
8285

83-
_, err = client.AdmissionregistrationV1().MutatingWebhookConfigurations().Create(context.Background(), mutatingWebhookConfiguration("mutating-webhook"), metav1.CreateOptions{})
86+
mutatingWebhook, err := client.AdmissionregistrationV1().MutatingWebhookConfigurations().Create(context.Background(), mutatingWebhookConfiguration("mutating-webhook"), metav1.CreateOptions{})
8487
require.NoError(t, err)
88+
require.NotEmpty(t, mutatingWebhook)
8589

8690
// wait for the informer to be aware of this create
8791
task = func() bool {
8892
return len(observedMutatingWebhooks) == 1
8993
}
9094
require.Eventually(t, task, time.Second*5, time.Millisecond*10, "MutatingWebhookConfiguration should have 1 MutatingWebhookConfiguration")
9195
require.Equal(t, "mutating-webhook", observedMutatingWebhooks[0].Name)
96+
97+
// modify our validating webhook
98+
validatingWebhook.SetLabels(map[string]string{"foo": "bar"})
99+
data, err := json.Marshal(validatingWebhook)
100+
require.NoError(t, err)
101+
_, err = client.AdmissionregistrationV1().ValidatingWebhookConfigurations().Patch(context.Background(), validatingWebhook.GetName(), types.MergePatchType, data, metav1.PatchOptions{})
102+
require.NoError(t, err)
103+
104+
// wait for the informer to be aware of this create
105+
task = func() bool {
106+
return len(observedValidatingWebhooks) == 2
107+
}
108+
require.Eventually(t, task, time.Second*5, time.Millisecond*10, "ValidatingWebhookConfiguration should have 2 ValidatingWebhookConfigurations")
109+
110+
// modify our mutating webhook
111+
mutatingWebhook.SetLabels(map[string]string{"foo": "bar"})
112+
data, err = json.Marshal(mutatingWebhook)
113+
require.NoError(t, err)
114+
_, err = client.AdmissionregistrationV1().MutatingWebhookConfigurations().Patch(context.Background(), mutatingWebhook.GetName(), types.MergePatchType, data, metav1.PatchOptions{})
115+
require.NoError(t, err)
116+
117+
// wait for the informer to be aware of this create
118+
task = func() bool {
119+
return len(observedMutatingWebhooks) == 2
120+
}
121+
require.Eventually(t, task, time.Second*5, time.Millisecond*10, "MutatingWebhookConfiguration should have 2 MutatingWebhookConfigurations")
92122
}
93123

94124
// validatingWebhookConfiguration returns a new ValidatingWebhookConfiguration with only it's name set

0 commit comments

Comments
 (0)