Skip to content

Commit b950208

Browse files
openshift-merge-bot[bot]openshift-merge-bot[bot]
authored
Merge pull request #1888 from lmiccini/fix-webhook-merge-by-name
Fix webhook config merge to match by name instead of index
2 parents 2531d2e + cd8350e commit b950208

2 files changed

Lines changed: 303 additions & 8 deletions

File tree

‎internal/operator/bindata/merge.go‎

Lines changed: 43 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,10 @@ func MergeObjectForUpdate(current, updated *uns.Unstructured) error {
5555
return nil
5656
}
5757

58-
// MergeWebhookConfigurationForUpdate merges certs and clientConfig from the current object
58+
// MergeWebhookConfigurationForUpdate merges the caBundle from the current
59+
// webhook configuration into the updated one, matching webhooks by name.
60+
// This preserves the CA certificate injected by cert-manager while keeping
61+
// the service path and other clientConfig fields from the updated template.
5962
func MergeWebhookConfigurationForUpdate(current, updated *uns.Unstructured) error {
6063
gvk := updated.GroupVersionKind()
6164

@@ -65,17 +68,49 @@ func MergeWebhookConfigurationForUpdate(current, updated *uns.Unstructured) erro
6568
return nil
6669
}
6770

68-
for i, webhook := range updated.Object["webhooks"].([]interface{}) {
69-
// Check if the index exists in the current webhooks list
70-
if i >= len(currentWebhooks) {
71+
// Build a lookup of current caBundle values keyed by webhook name
72+
caBundleByName := make(map[string]interface{})
73+
for _, cw := range currentWebhooks {
74+
cwMap, ok := cw.(map[string]interface{})
75+
if !ok {
7176
continue
7277
}
73-
74-
currentClientConfig := currentWebhooks[i].(map[string]interface{})["clientConfig"].(map[string]interface{})
75-
if currentClientConfig != nil {
76-
webhook.(map[string]interface{})["clientConfig"] = currentClientConfig
78+
name, _ := cwMap["name"].(string)
79+
if name == "" {
80+
continue
81+
}
82+
cc, ok := cwMap["clientConfig"].(map[string]interface{})
83+
if !ok {
84+
continue
85+
}
86+
if caBundle, exists := cc["caBundle"]; exists {
87+
caBundleByName[name] = caBundle
7788
}
89+
}
90+
91+
updatedWebhooks, updatedExists := updated.Object["webhooks"].([]interface{})
92+
if !updatedExists {
93+
return nil
94+
}
7895

96+
for _, webhook := range updatedWebhooks {
97+
whMap, ok := webhook.(map[string]interface{})
98+
if !ok {
99+
continue
100+
}
101+
name, _ := whMap["name"].(string)
102+
if name == "" {
103+
continue
104+
}
105+
caBundle, found := caBundleByName[name]
106+
if !found {
107+
continue
108+
}
109+
cc, ok := whMap["clientConfig"].(map[string]interface{})
110+
if !ok {
111+
continue
112+
}
113+
cc["caBundle"] = caBundle
79114
}
80115
}
81116
return nil

‎internal/operator/bindata/merge_test.go‎

Lines changed: 260 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,260 @@
1+
package bindata
2+
3+
import (
4+
"testing"
5+
6+
uns "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
7+
"k8s.io/apimachinery/pkg/runtime/schema"
8+
)
9+
10+
// makeWebhook builds a single webhook entry for use in test fixtures.
11+
func makeWebhook(name, path string, caBundle interface{}) map[string]interface{} {
12+
cc := map[string]interface{}{
13+
"service": map[string]interface{}{
14+
"name": "webhook-service",
15+
"namespace": "openstack-operators",
16+
"path": path,
17+
},
18+
}
19+
if caBundle != nil {
20+
cc["caBundle"] = caBundle
21+
}
22+
return map[string]interface{}{
23+
"name": name,
24+
"clientConfig": cc,
25+
"rules": []interface{}{
26+
map[string]interface{}{
27+
"apiGroups": []interface{}{"example.org"},
28+
"resources": []interface{}{"things"},
29+
},
30+
},
31+
}
32+
}
33+
34+
// makeWebhookConfig builds an Unstructured MutatingWebhookConfiguration.
35+
func makeWebhookConfig(webhooks ...map[string]interface{}) *uns.Unstructured {
36+
whList := make([]interface{}, len(webhooks))
37+
for i, w := range webhooks {
38+
whList[i] = w
39+
}
40+
obj := &uns.Unstructured{Object: map[string]interface{}{
41+
"apiVersion": "admissionregistration.k8s.io/v1",
42+
"kind": "MutatingWebhookConfiguration",
43+
"metadata": map[string]interface{}{"name": "test"},
44+
"webhooks": whList,
45+
}}
46+
obj.SetGroupVersionKind(schema.GroupVersionKind{
47+
Group: "admissionregistration.k8s.io",
48+
Version: "v1",
49+
Kind: "MutatingWebhookConfiguration",
50+
})
51+
return obj
52+
}
53+
54+
func getWebhookPath(obj *uns.Unstructured, index int) string {
55+
wh := obj.Object["webhooks"].([]interface{})[index].(map[string]interface{})
56+
cc := wh["clientConfig"].(map[string]interface{})
57+
svc := cc["service"].(map[string]interface{})
58+
return svc["path"].(string)
59+
}
60+
61+
func getWebhookName(obj *uns.Unstructured, index int) string {
62+
wh := obj.Object["webhooks"].([]interface{})[index].(map[string]interface{})
63+
return wh["name"].(string)
64+
}
65+
66+
func getWebhookCABundle(obj *uns.Unstructured, index int) interface{} {
67+
wh := obj.Object["webhooks"].([]interface{})[index].(map[string]interface{})
68+
cc := wh["clientConfig"].(map[string]interface{})
69+
return cc["caBundle"]
70+
}
71+
72+
func TestMergeWebhookPreservesPathsWhenOrderDiffers(t *testing.T) {
73+
// Simulate the bug scenario: Kubernetes sorts webhooks alphabetically by name,
74+
// but the template has a different order. The merge must match by name, not index.
75+
76+
// Current (on cluster): sorted alphabetically by name, with caBundle from cert-manager
77+
current := makeWebhookConfig(
78+
makeWebhook("mmemcached-v1beta1.kb.io", "/mutate-memcached", "CABUNDLE-memcached"),
79+
makeWebhook("mrabbitmq-v1beta1.kb.io", "/mutate-rabbitmq", "CABUNDLE-rabbitmq"),
80+
makeWebhook("mreservation-v1beta1.kb.io", "/mutate-reservation", "CABUNDLE-reservation"),
81+
)
82+
83+
// Updated (from template): different order, no caBundle
84+
updated := makeWebhookConfig(
85+
makeWebhook("mreservation-v1beta1.kb.io", "/mutate-reservation", nil),
86+
makeWebhook("mmemcached-v1beta1.kb.io", "/mutate-memcached", nil),
87+
makeWebhook("mrabbitmq-v1beta1.kb.io", "/mutate-rabbitmq", nil),
88+
)
89+
90+
if err := MergeWebhookConfigurationForUpdate(current, updated); err != nil {
91+
t.Fatalf("MergeWebhookConfigurationForUpdate failed: %v", err)
92+
}
93+
94+
// Verify each webhook kept its correct path and got the right caBundle
95+
tests := []struct {
96+
index int
97+
expectedName string
98+
expectedPath string
99+
expectedBundle string
100+
}{
101+
{0, "mreservation-v1beta1.kb.io", "/mutate-reservation", "CABUNDLE-reservation"},
102+
{1, "mmemcached-v1beta1.kb.io", "/mutate-memcached", "CABUNDLE-memcached"},
103+
{2, "mrabbitmq-v1beta1.kb.io", "/mutate-rabbitmq", "CABUNDLE-rabbitmq"},
104+
}
105+
106+
for _, tc := range tests {
107+
name := getWebhookName(updated, tc.index)
108+
path := getWebhookPath(updated, tc.index)
109+
bundle := getWebhookCABundle(updated, tc.index)
110+
111+
if name != tc.expectedName {
112+
t.Errorf("webhook[%d]: got name %q, want %q", tc.index, name, tc.expectedName)
113+
}
114+
if path != tc.expectedPath {
115+
t.Errorf("webhook[%d] (%s): got path %q, want %q", tc.index, name, path, tc.expectedPath)
116+
}
117+
if bundle != tc.expectedBundle {
118+
t.Errorf("webhook[%d] (%s): got caBundle %v, want %q", tc.index, name, bundle, tc.expectedBundle)
119+
}
120+
}
121+
}
122+
123+
func TestMergeWebhookNoCaBundleInCurrent(t *testing.T) {
124+
// When the current webhook has no caBundle (e.g. cert-manager hasn't injected yet),
125+
// the updated webhook should remain unchanged.
126+
current := makeWebhookConfig(
127+
makeWebhook("mfoo.kb.io", "/mutate-foo", nil),
128+
)
129+
updated := makeWebhookConfig(
130+
makeWebhook("mfoo.kb.io", "/mutate-foo", nil),
131+
)
132+
133+
if err := MergeWebhookConfigurationForUpdate(current, updated); err != nil {
134+
t.Fatalf("unexpected error: %v", err)
135+
}
136+
137+
if bundle := getWebhookCABundle(updated, 0); bundle != nil {
138+
t.Errorf("expected no caBundle, got %v", bundle)
139+
}
140+
if path := getWebhookPath(updated, 0); path != "/mutate-foo" {
141+
t.Errorf("expected path /mutate-foo, got %s", path)
142+
}
143+
}
144+
145+
func TestMergeWebhookNewWebhookInUpdated(t *testing.T) {
146+
// When the updated template has a new webhook that doesn't exist in current,
147+
// it should be left as-is (no caBundle).
148+
current := makeWebhookConfig(
149+
makeWebhook("mfoo.kb.io", "/mutate-foo", "CABUNDLE-foo"),
150+
)
151+
updated := makeWebhookConfig(
152+
makeWebhook("mfoo.kb.io", "/mutate-foo", nil),
153+
makeWebhook("mbar.kb.io", "/mutate-bar", nil),
154+
)
155+
156+
if err := MergeWebhookConfigurationForUpdate(current, updated); err != nil {
157+
t.Fatalf("unexpected error: %v", err)
158+
}
159+
160+
// Existing webhook gets its caBundle
161+
if bundle := getWebhookCABundle(updated, 0); bundle != "CABUNDLE-foo" {
162+
t.Errorf("webhook[0]: expected caBundle CABUNDLE-foo, got %v", bundle)
163+
}
164+
// New webhook has no caBundle
165+
if bundle := getWebhookCABundle(updated, 1); bundle != nil {
166+
t.Errorf("webhook[1]: expected no caBundle, got %v", bundle)
167+
}
168+
}
169+
170+
func TestMergeWebhookRemovedFromUpdated(t *testing.T) {
171+
// When a webhook exists in current but was removed from the updated template,
172+
// it should not appear in the result.
173+
current := makeWebhookConfig(
174+
makeWebhook("mfoo.kb.io", "/mutate-foo", "CABUNDLE-foo"),
175+
makeWebhook("mbar.kb.io", "/mutate-bar", "CABUNDLE-bar"),
176+
)
177+
updated := makeWebhookConfig(
178+
makeWebhook("mfoo.kb.io", "/mutate-foo", nil),
179+
)
180+
181+
if err := MergeWebhookConfigurationForUpdate(current, updated); err != nil {
182+
t.Fatalf("unexpected error: %v", err)
183+
}
184+
185+
webhooks := updated.Object["webhooks"].([]interface{})
186+
if len(webhooks) != 1 {
187+
t.Fatalf("expected 1 webhook, got %d", len(webhooks))
188+
}
189+
if name := getWebhookName(updated, 0); name != "mfoo.kb.io" {
190+
t.Errorf("expected mfoo.kb.io, got %s", name)
191+
}
192+
}
193+
194+
func TestMergeWebhookSkipsNonWebhookResources(t *testing.T) {
195+
// Non-webhook resources should pass through unchanged.
196+
current := &uns.Unstructured{Object: map[string]interface{}{
197+
"apiVersion": "apps/v1",
198+
"kind": "Deployment",
199+
"metadata": map[string]interface{}{"name": "test"},
200+
}}
201+
current.SetGroupVersionKind(schema.GroupVersionKind{Group: "apps", Version: "v1", Kind: "Deployment"})
202+
203+
updated := &uns.Unstructured{Object: map[string]interface{}{
204+
"apiVersion": "apps/v1",
205+
"kind": "Deployment",
206+
"metadata": map[string]interface{}{"name": "test"},
207+
}}
208+
updated.SetGroupVersionKind(schema.GroupVersionKind{Group: "apps", Version: "v1", Kind: "Deployment"})
209+
210+
if err := MergeWebhookConfigurationForUpdate(current, updated); err != nil {
211+
t.Fatalf("unexpected error: %v", err)
212+
}
213+
}
214+
215+
func TestMergeWebhookValidatingConfig(t *testing.T) {
216+
// The fix should also work for ValidatingWebhookConfiguration.
217+
current := &uns.Unstructured{Object: map[string]interface{}{
218+
"apiVersion": "admissionregistration.k8s.io/v1",
219+
"kind": "ValidatingWebhookConfiguration",
220+
"metadata": map[string]interface{}{"name": "test"},
221+
"webhooks": []interface{}{
222+
makeWebhook("vbar.kb.io", "/validate-bar", "CABUNDLE-bar"),
223+
makeWebhook("vfoo.kb.io", "/validate-foo", "CABUNDLE-foo"),
224+
},
225+
}}
226+
current.SetGroupVersionKind(schema.GroupVersionKind{
227+
Group: "admissionregistration.k8s.io", Version: "v1", Kind: "ValidatingWebhookConfiguration",
228+
})
229+
230+
updated := &uns.Unstructured{Object: map[string]interface{}{
231+
"apiVersion": "admissionregistration.k8s.io/v1",
232+
"kind": "ValidatingWebhookConfiguration",
233+
"metadata": map[string]interface{}{"name": "test"},
234+
"webhooks": []interface{}{
235+
makeWebhook("vfoo.kb.io", "/validate-foo", nil),
236+
makeWebhook("vbar.kb.io", "/validate-bar", nil),
237+
},
238+
}}
239+
updated.SetGroupVersionKind(schema.GroupVersionKind{
240+
Group: "admissionregistration.k8s.io", Version: "v1", Kind: "ValidatingWebhookConfiguration",
241+
})
242+
243+
if err := MergeWebhookConfigurationForUpdate(current, updated); err != nil {
244+
t.Fatalf("unexpected error: %v", err)
245+
}
246+
247+
// vfoo should get vfoo's caBundle, not vbar's
248+
if bundle := getWebhookCABundle(updated, 0); bundle != "CABUNDLE-foo" {
249+
t.Errorf("webhook[0] (vfoo): expected CABUNDLE-foo, got %v", bundle)
250+
}
251+
if bundle := getWebhookCABundle(updated, 1); bundle != "CABUNDLE-bar" {
252+
t.Errorf("webhook[1] (vbar): expected CABUNDLE-bar, got %v", bundle)
253+
}
254+
if path := getWebhookPath(updated, 0); path != "/validate-foo" {
255+
t.Errorf("webhook[0]: expected /validate-foo, got %s", path)
256+
}
257+
if path := getWebhookPath(updated, 1); path != "/validate-bar" {
258+
t.Errorf("webhook[1]: expected /validate-bar, got %s", path)
259+
}
260+
}

0 commit comments

Comments
 (0)