Skip to content

Commit cd8350e

Browse files
lmicciniclaude
lmiccini
and
claude
committed
Fix webhook config merge to match by name instead of index
MergeWebhookConfigurationForUpdate was copying the entire clientConfig from current to updated webhooks by array index. When the webhook arrays have different ordering (e.g. Kubernetes sorts alphabetically by name, but the template YAML has a different order), each webhook gets the clientConfig from the wrong entry. This scrambles the service paths, causing admission requests to be routed to the wrong handler. For example, mrabbitmq-v1beta1.kb.io would get the path for /mutate-network-openstack-org-v1beta1-reservation instead of /mutate-rabbitmq-openstack-org-v1beta1-rabbitmq, resulting in: "unable to decode rabbitmq.openstack.org/v1beta1, Kind=RabbitMq into *v1beta1.Reservation" Fix by matching webhooks by name and only copying the caBundle field (injected by cert-manager) rather than the entire clientConfig. This preserves the correct service path from the updated template. Jira: https://redhat.atlassian.net/browse/OSPRH-29026 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 6fd66d8 commit cd8350e

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)