Skip to content

Commit e620d61

Browse files
stuggiclaude
stuggi
and
claude
committed
Rewrite CRD namespace fields and preserve cert-manager caBundle
At CRD install time, rewrite the cert-manager.io/inject-ca-from annotation namespace and spec.conversion.webhook.clientConfig.service.namespace to the install namespace (instance.Namespace). CRDs shipped in operator bundles encode the operator's own namespace; openstack-operator installs into openstack-operators, so without this rewrite cert-manager cannot locate the Certificate and never injects the caBundle, breaking conversion webhook TLS. - applyCRDs: inline render loop with rewriteCRDNamespaces() called before each ApplyObject; generic — applies to any CRD carrying the annotation or conversion webhook service reference, no per-operator name checks - MergeCRDForUpdate: preserve cert-manager-injected caBundle on re-apply so each reconcile does not clobber it back to empty - Unit tests: 5 cases for rewriteCRDNamespaces (both fields, idempotent, annotation-only, no annotation, non-CRD skip); 3 cases for MergeCRDForUpdate - kuttl skeleton: joint acceptance gate for OSPRH-30208/30209/30210 asserting annotation namespace, service namespace, and non-empty caBundle on the installed keystone CRD (timeout: 120s for async cert-manager injection) Jira: OSPRH-30208 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Martin Schuppert <mschuppert@redhat.com>
1 parent f784e54 commit e620d61

7 files changed

Lines changed: 385 additions & 1 deletion

File tree

‎internal/controller/operator/crd_namespace_rewrite_test.go‎

Lines changed: 149 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,149 @@
1+
/*
2+
Copyright 2024.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
17+
package operator
18+
19+
import (
20+
"testing"
21+
22+
uns "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
23+
"k8s.io/apimachinery/pkg/runtime/schema"
24+
)
25+
26+
func makeKeystoneCRD(annotationNS, serviceNS string) *uns.Unstructured {
27+
obj := &uns.Unstructured{Object: map[string]interface{}{
28+
"apiVersion": "apiextensions.k8s.io/v1",
29+
"kind": "CustomResourceDefinition",
30+
"metadata": map[string]interface{}{
31+
"name": "keystoneapis.keystone.openstack.org",
32+
"annotations": map[string]interface{}{
33+
"cert-manager.io/inject-ca-from": annotationNS + "/keystone-operator-serving-cert",
34+
},
35+
},
36+
"spec": map[string]interface{}{
37+
"conversion": map[string]interface{}{
38+
"strategy": "Webhook",
39+
"webhook": map[string]interface{}{
40+
"clientConfig": map[string]interface{}{
41+
"service": map[string]interface{}{
42+
"name": "keystone-operator-webhook-service",
43+
"namespace": serviceNS,
44+
"path": "/convert",
45+
},
46+
},
47+
},
48+
},
49+
},
50+
}}
51+
obj.SetGroupVersionKind(schema.GroupVersionKind{
52+
Group: "apiextensions.k8s.io", Version: "v1", Kind: "CustomResourceDefinition",
53+
})
54+
return obj
55+
}
56+
57+
func TestRewriteCRDNamespaces_RewritesBothFields(t *testing.T) {
58+
obj := makeKeystoneCRD("keystone-operator-system", "keystone-operator-system")
59+
if err := rewriteCRDNamespaces(obj, "openstack-operators"); err != nil {
60+
t.Fatalf("unexpected error: %v", err)
61+
}
62+
63+
annos := obj.GetAnnotations()
64+
got := annos["cert-manager.io/inject-ca-from"]
65+
want := "openstack-operators/keystone-operator-serving-cert"
66+
if got != want {
67+
t.Errorf("inject-ca-from: got %q, want %q", got, want)
68+
}
69+
70+
svcNs, _, _ := uns.NestedString(obj.Object, "spec", "conversion", "webhook", "clientConfig", "service", "namespace")
71+
if svcNs != "openstack-operators" {
72+
t.Errorf("service.namespace: got %q, want %q", svcNs, "openstack-operators")
73+
}
74+
}
75+
76+
func TestRewriteCRDNamespaces_AlreadyCorrect(t *testing.T) {
77+
obj := makeKeystoneCRD("openstack-operators", "openstack-operators")
78+
if err := rewriteCRDNamespaces(obj, "openstack-operators"); err != nil {
79+
t.Fatalf("unexpected error: %v", err)
80+
}
81+
82+
annos := obj.GetAnnotations()
83+
if got := annos["cert-manager.io/inject-ca-from"]; got != "openstack-operators/keystone-operator-serving-cert" {
84+
t.Errorf("annotation changed unexpectedly: %q", got)
85+
}
86+
}
87+
88+
func TestRewriteCRDNamespaces_AnnotationOnlyNoConversionSection(t *testing.T) {
89+
// CRD has the inject-ca-from annotation but no spec.conversion.webhook.clientConfig.service
90+
// (e.g. a CRD that uses cainjection for a non-conversion purpose). Service rewrite is a no-op.
91+
obj := &uns.Unstructured{Object: map[string]interface{}{
92+
"apiVersion": "apiextensions.k8s.io/v1",
93+
"kind": "CustomResourceDefinition",
94+
"metadata": map[string]interface{}{
95+
"name": "somecrd.example.org",
96+
"annotations": map[string]interface{}{
97+
"cert-manager.io/inject-ca-from": "source-ns/some-cert",
98+
},
99+
},
100+
"spec": map[string]interface{}{},
101+
}}
102+
obj.SetGroupVersionKind(schema.GroupVersionKind{
103+
Group: "apiextensions.k8s.io", Version: "v1", Kind: "CustomResourceDefinition",
104+
})
105+
if err := rewriteCRDNamespaces(obj, "openstack-operators"); err != nil {
106+
t.Fatalf("unexpected error: %v", err)
107+
}
108+
annos := obj.GetAnnotations()
109+
if got := annos["cert-manager.io/inject-ca-from"]; got != "openstack-operators/some-cert" {
110+
t.Errorf("inject-ca-from not rewritten: got %q", got)
111+
}
112+
}
113+
114+
func TestRewriteCRDNamespaces_NoAnnotation(t *testing.T) {
115+
obj := &uns.Unstructured{Object: map[string]interface{}{
116+
"apiVersion": "apiextensions.k8s.io/v1",
117+
"kind": "CustomResourceDefinition",
118+
"metadata": map[string]interface{}{"name": "no-annotation.example.org"},
119+
"spec": map[string]interface{}{},
120+
}}
121+
obj.SetGroupVersionKind(schema.GroupVersionKind{
122+
Group: "apiextensions.k8s.io", Version: "v1", Kind: "CustomResourceDefinition",
123+
})
124+
if err := rewriteCRDNamespaces(obj, "openstack-operators"); err != nil {
125+
t.Fatalf("unexpected error: %v", err)
126+
}
127+
}
128+
129+
func TestRewriteCRDNamespaces_SkipsNonCRD(t *testing.T) {
130+
obj := &uns.Unstructured{Object: map[string]interface{}{
131+
"apiVersion": "apps/v1",
132+
"kind": "Deployment",
133+
"metadata": map[string]interface{}{
134+
"name": "test",
135+
"annotations": map[string]interface{}{
136+
"cert-manager.io/inject-ca-from": "wrong-ns/some-cert",
137+
},
138+
},
139+
}}
140+
obj.SetGroupVersionKind(schema.GroupVersionKind{Group: "apps", Version: "v1", Kind: "Deployment"})
141+
if err := rewriteCRDNamespaces(obj, "openstack-operators"); err != nil {
142+
t.Fatalf("unexpected error: %v", err)
143+
}
144+
145+
// Non-CRD: annotation must NOT be touched
146+
if got := obj.GetAnnotations()["cert-manager.io/inject-ca-from"]; got != "wrong-ns/some-cert" {
147+
t.Errorf("non-CRD annotation was rewritten: %q", got)
148+
}
149+
}

‎internal/controller/operator/openstack_controller.go‎

Lines changed: 54 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -692,7 +692,60 @@ func (r *OpenStackReconciler) applyManifests(ctx context.Context, instance *oper
692692
func (r *OpenStackReconciler) applyCRDs(ctx context.Context, instance *operatorv1beta1.OpenStack) error {
693693
data := bindata.MakeRenderData()
694694
data.Data["OperatorNamespace"] = instance.Namespace
695-
return r.renderAndApply(ctx, instance, data, "crds", false)
695+
696+
bindir := util.GetEnvVar("BASE_BINDATA", "/bindata")
697+
objs, err := bindata.RenderDir(filepath.Join(bindir, "crds"), &data)
698+
if err != nil {
699+
return errors.Wrapf(err, "failed to render openstack-operator - crds")
700+
}
701+
if len(objs) == 0 {
702+
return fmt.Errorf("no manifests rendered from %s/crds", bindir)
703+
}
704+
for _, obj := range objs {
705+
if obj.GetName() == "" {
706+
continue
707+
}
708+
if err := rewriteCRDNamespaces(obj, instance.Namespace); err != nil {
709+
return err
710+
}
711+
if err := bindata.ApplyObject(ctx, r.Client, obj); err != nil {
712+
return errors.Wrapf(err, "failed to apply object %v", obj)
713+
}
714+
}
715+
return nil
716+
}
717+
718+
// rewriteCRDNamespaces rewrites namespace-sensitive fields on CRDs at install time:
719+
// - cert-manager.io/inject-ca-from annotation ("<source-ns>/<cert-name>" → "<ns>/<cert-name>")
720+
// - spec.conversion.webhook.clientConfig.service.namespace
721+
//
722+
// CRDs shipped in operator bundles encode the operator's own install namespace;
723+
// openstack-operator installs into a different namespace, so both must be rewritten.
724+
func rewriteCRDNamespaces(obj *uns.Unstructured, ns string) error {
725+
gvk := obj.GroupVersionKind()
726+
if gvk.Group != "apiextensions.k8s.io" || gvk.Kind != "CustomResourceDefinition" {
727+
return nil
728+
}
729+
const caInjectAnno = "cert-manager.io/inject-ca-from"
730+
annotations := obj.GetAnnotations()
731+
if v, ok := annotations[caInjectAnno]; ok {
732+
if idx := strings.LastIndex(v, "/"); idx >= 0 {
733+
annotations[caInjectAnno] = ns + v[idx:]
734+
obj.SetAnnotations(annotations)
735+
}
736+
}
737+
svcNs, found, err := uns.NestedString(obj.Object,
738+
"spec", "conversion", "webhook", "clientConfig", "service", "namespace")
739+
if err != nil {
740+
return fmt.Errorf("failed to read service namespace on CRD %s: %w", obj.GetName(), err)
741+
}
742+
if found && svcNs != "" {
743+
if err := uns.SetNestedField(obj.Object, ns,
744+
"spec", "conversion", "webhook", "clientConfig", "service", "namespace"); err != nil {
745+
return fmt.Errorf("failed to rewrite service namespace on CRD %s: %w", obj.GetName(), err)
746+
}
747+
}
748+
return nil
696749
}
697750

698751
func (r *OpenStackReconciler) applyServices(ctx context.Context, instance *operatorv1beta1.OpenStack) error {

‎internal/operator/bindata/merge.go‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,10 @@ func MergeMetadataForUpdate(current, updated *uns.Unstructured) error {
2929
// Some objects, such as Deployments and Services require
3030
// some semantic-aware updates
3131
func MergeObjectForUpdate(current, updated *uns.Unstructured) error {
32+
if err := MergeCRDForUpdate(current, updated); err != nil {
33+
return err
34+
}
35+
3236
if err := MergeWebhookConfigurationForUpdate(current, updated); err != nil {
3337
return err
3438
}
@@ -55,6 +59,24 @@ func MergeObjectForUpdate(current, updated *uns.Unstructured) error {
5559
return nil
5660
}
5761

62+
// MergeCRDForUpdate preserves the cert-manager-injected caBundle in the CRD's
63+
// conversion webhook clientConfig on re-apply. Without this, each reconcile
64+
// would clear the caBundle that cert-manager wrote, breaking conversion TLS.
65+
func MergeCRDForUpdate(current, updated *uns.Unstructured) error {
66+
gvk := updated.GroupVersionKind()
67+
if gvk.Group != "apiextensions.k8s.io" || gvk.Kind != "CustomResourceDefinition" {
68+
return nil
69+
}
70+
caBundle, found, err := uns.NestedString(current.Object, "spec", "conversion", "webhook", "clientConfig", "caBundle")
71+
if err != nil {
72+
return errors.Wrapf(err, "failed to read caBundle from CRD %s", updated.GetName())
73+
}
74+
if !found || caBundle == "" {
75+
return nil
76+
}
77+
return uns.SetNestedField(updated.Object, caBundle, "spec", "conversion", "webhook", "clientConfig", "caBundle")
78+
}
79+
5880
// MergeWebhookConfigurationForUpdate merges the caBundle from the current
5981
// webhook configuration into the updated one, matching webhooks by name.
6082
// This preserves the CA certificate injected by cert-manager while keeping

‎internal/operator/bindata/merge_test.go‎

Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -212,6 +212,116 @@ func TestMergeWebhookSkipsNonWebhookResources(t *testing.T) {
212212
}
213213
}
214214

215+
func makeCRD(caBundle string) *uns.Unstructured {
216+
svc := map[string]interface{}{
217+
"name": "keystone-operator-webhook-service",
218+
"namespace": "openstack-operators",
219+
"path": "/convert",
220+
}
221+
cc := map[string]interface{}{"service": svc}
222+
if caBundle != "" {
223+
cc["caBundle"] = caBundle
224+
}
225+
obj := &uns.Unstructured{Object: map[string]interface{}{
226+
"apiVersion": "apiextensions.k8s.io/v1",
227+
"kind": "CustomResourceDefinition",
228+
"metadata": map[string]interface{}{"name": "keystoneapis.keystone.openstack.org"},
229+
"spec": map[string]interface{}{
230+
"conversion": map[string]interface{}{
231+
"strategy": "Webhook",
232+
"webhook": map[string]interface{}{"clientConfig": cc},
233+
},
234+
},
235+
}}
236+
obj.SetGroupVersionKind(schema.GroupVersionKind{
237+
Group: "apiextensions.k8s.io", Version: "v1", Kind: "CustomResourceDefinition",
238+
})
239+
return obj
240+
}
241+
242+
func TestMergeCRDPreservesCaBundle(t *testing.T) {
243+
current := makeCRD("CERT-MANAGER-INJECTED-CA")
244+
updated := makeCRD("")
245+
246+
if err := MergeCRDForUpdate(current, updated); err != nil {
247+
t.Fatalf("MergeCRDForUpdate failed: %v", err)
248+
}
249+
250+
got, found, err := uns.NestedString(updated.Object, "spec", "conversion", "webhook", "clientConfig", "caBundle")
251+
if err != nil || !found {
252+
t.Fatalf("caBundle not found after merge: err=%v found=%v", err, found)
253+
}
254+
if got != "CERT-MANAGER-INJECTED-CA" {
255+
t.Errorf("expected caBundle CERT-MANAGER-INJECTED-CA, got %q", got)
256+
}
257+
}
258+
259+
func TestMergeCRDNoCaBundleInCurrent(t *testing.T) {
260+
current := makeCRD("")
261+
updated := makeCRD("")
262+
263+
if err := MergeCRDForUpdate(current, updated); err != nil {
264+
t.Fatalf("unexpected error: %v", err)
265+
}
266+
267+
_, found, _ := uns.NestedString(updated.Object, "spec", "conversion", "webhook", "clientConfig", "caBundle")
268+
if found {
269+
t.Error("expected no caBundle when current has none")
270+
}
271+
}
272+
273+
func TestMergeCRDSkipsNonCRDs(t *testing.T) {
274+
current := makeWebhookConfig(makeWebhook("mfoo.kb.io", "/mutate-foo", "CABUNDLE"))
275+
updated := makeWebhookConfig(makeWebhook("mfoo.kb.io", "/mutate-foo", nil))
276+
277+
if err := MergeCRDForUpdate(current, updated); err != nil {
278+
t.Fatalf("unexpected error: %v", err)
279+
}
280+
// caBundle should NOT be copied — wrong object type
281+
if bundle := getWebhookCABundle(updated, 0); bundle != nil {
282+
t.Errorf("MergeCRDForUpdate should not touch non-CRD objects, but caBundle=%v", bundle)
283+
}
284+
}
285+
286+
func TestMergeCRDInvalidCaBundleType(t *testing.T) {
287+
// When caBundle is not a string (e.g., malformed CRD with caBundle as a map),
288+
// MergeCRDForUpdate should return an error instead of silently ignoring it.
289+
current := &uns.Unstructured{Object: map[string]interface{}{
290+
"apiVersion": "apiextensions.k8s.io/v1",
291+
"kind": "CustomResourceDefinition",
292+
"metadata": map[string]interface{}{"name": "keystoneapis.keystone.openstack.org"},
293+
"spec": map[string]interface{}{
294+
"conversion": map[string]interface{}{
295+
"strategy": "Webhook",
296+
"webhook": map[string]interface{}{
297+
"clientConfig": map[string]interface{}{
298+
"service": map[string]interface{}{
299+
"name": "keystone-operator-webhook-service",
300+
"namespace": "openstack-operators",
301+
"path": "/convert",
302+
},
303+
// Invalid: caBundle should be a string, but it's a map
304+
"caBundle": map[string]interface{}{"invalid": "structure"},
305+
},
306+
},
307+
},
308+
},
309+
}}
310+
current.SetGroupVersionKind(schema.GroupVersionKind{
311+
Group: "apiextensions.k8s.io", Version: "v1", Kind: "CustomResourceDefinition",
312+
})
313+
314+
updated := makeCRD("")
315+
316+
err := MergeCRDForUpdate(current, updated)
317+
if err == nil {
318+
t.Fatal("expected error for invalid caBundle type, got nil")
319+
}
320+
if err.Error() == "" {
321+
t.Error("error message should not be empty")
322+
}
323+
}
324+
215325
func TestMergeWebhookValidatingConfig(t *testing.T) {
216326
// The fix should also work for ValidatingWebhookConfiguration.
217327
current := &uns.Unstructured{Object: map[string]interface{}{

‎test/kuttl/tests/ctlplane-conversion-webhook/00-assert-deploy-openstack.yaml‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
apiVersion: core.openstack.org/v1beta1
2+
kind: OpenStackControlPlane
3+
metadata:
4+
name: openstack
5+
status:
6+
conditions:
7+
- type: Ready
8+
status: "True"

‎test/kuttl/tests/ctlplane-conversion-webhook/00-deploy-openstack.yaml‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
apiVersion: kuttl.dev/v1beta1
2+
kind: TestStep
3+
commands:
4+
- script: |
5+
oc kustomize ../../../../config/samples/core | oc apply -n $NAMESPACE -f -

0 commit comments

Comments
 (0)