Skip to content

Commit 8bdd50d

Browse files
nader-ziadanader-ziada
committed
address copilot comments
Signed-off-by: Nader Ziada <nziada@redhat.com>
1 parent 14b722f commit 8bdd50d

5 files changed

Lines changed: 170 additions & 32 deletions

File tree

‎internal/operator-controller/applier/boxcutter.go‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,10 @@ func (r *SimpleRevisionGenerator) GenerateRevisionFromHelmRelease(
102102
}
103103

104104
if nsConfig != nil && nsConfig.Managed {
105-
nsObj := BuildNamespaceObject(nsConfig.Name, nsConfig.Template)
105+
nsObj, err := BuildNamespaceObject(nsConfig.Name, nsConfig.Template)
106+
if err != nil {
107+
return nil, err
108+
}
106109
nsObj.SetLabels(mergeStringMaps(nsObj.GetLabels(), objectLabels))
107110
objs = append(objs, *ocv1ac.ClusterObjectSetObject().WithObject(nsObj))
108111
}
@@ -202,7 +205,10 @@ func (r *SimpleRevisionGenerator) GenerateRevision(
202205
}
203206

204207
if nsConfig != nil && nsConfig.Managed {
205-
nsObj := BuildNamespaceObject(nsConfig.Name, nsConfig.Template)
208+
nsObj, err := BuildNamespaceObject(nsConfig.Name, nsConfig.Template)
209+
if err != nil {
210+
return nil, err
211+
}
206212
nsObj.SetLabels(mergeStringMaps(nsObj.GetLabels(), objectLabels))
207213
objs = append(objs, *ocv1ac.ClusterObjectSetObject().WithObject(nsObj))
208214
}

‎internal/operator-controller/applier/namespace.go‎

Lines changed: 32 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -3,13 +3,16 @@ package applier
33
import (
44
"encoding/json"
55
"fmt"
6+
"regexp"
67

78
corev1 "k8s.io/api/core/v1"
89
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
910
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
1011
"k8s.io/apimachinery/pkg/runtime"
1112
)
1213

14+
var dns1123LabelRegexp = regexp.MustCompile(`^[a-z0-9]([-a-z0-9]*[a-z0-9])?$`)
15+
1316
const (
1417
AnnotationSuggestedNamespaceTemplate = "operatorframework.io/suggested-namespace-template"
1518
AnnotationSuggestedNamespace = "operators.operatorframework.io/suggested-namespace"
@@ -43,26 +46,40 @@ func ResolveNamespaceName(csvAnnotations map[string]string, packageName string)
4346
return "", nil, err
4447
}
4548

46-
if template != nil {
47-
if template.Name != "" {
48-
return template.Name, template, nil
49+
var name string
50+
if template != nil && template.Name != "" {
51+
name = template.Name
52+
} else if csvAnnotations != nil {
53+
if n, ok := csvAnnotations[AnnotationSuggestedNamespace]; ok && n != "" {
54+
name = n
4955
}
50-
// Template exists but has no name — fall through to next option,
51-
// but keep the template for labels/annotations
5256
}
5357

54-
// Try suggested-namespace annotation
55-
if csvAnnotations != nil {
56-
if name, ok := csvAnnotations[AnnotationSuggestedNamespace]; ok && name != "" {
57-
return name, template, nil
58-
}
58+
if name == "" {
59+
name = fmt.Sprintf("%s-system", packageName)
60+
}
61+
62+
if err := validateNamespaceName(name); err != nil {
63+
return "", nil, err
5964
}
6065

61-
// Fallback: <packageName>-system
62-
return fmt.Sprintf("%s-system", packageName), template, nil
66+
return name, template, nil
67+
}
68+
69+
func validateNamespaceName(name string) error {
70+
if name == "" {
71+
return fmt.Errorf("resolved namespace name is empty")
72+
}
73+
if len(name) > 63 {
74+
return fmt.Errorf("resolved namespace name %q exceeds 63 characters", name)
75+
}
76+
if !dns1123LabelRegexp.MatchString(name) {
77+
return fmt.Errorf("resolved namespace name %q is not a valid DNS1123 label", name)
78+
}
79+
return nil
6380
}
6481

65-
func BuildNamespaceObject(name string, template *corev1.Namespace) unstructured.Unstructured {
82+
func BuildNamespaceObject(name string, template *corev1.Namespace) (unstructured.Unstructured, error) {
6683
ns := corev1.Namespace{
6784
TypeMeta: metav1.TypeMeta{
6885
APIVersion: "v1",
@@ -84,8 +101,8 @@ func BuildNamespaceObject(name string, template *corev1.Namespace) unstructured.
84101

85102
unstructuredObj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&ns)
86103
if err != nil {
87-
panic(fmt.Sprintf("failed to convert namespace to unstructured: %v", err))
104+
return unstructured.Unstructured{}, fmt.Errorf("failed to convert namespace to unstructured: %w", err)
88105
}
89106

90-
return unstructured.Unstructured{Object: unstructuredObj}
107+
return unstructured.Unstructured{Object: unstructuredObj}, nil
91108
}

‎internal/operator-controller/applier/namespace_test.go‎

Lines changed: 101 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -234,7 +234,8 @@ func TestBuildNamespaceObject(t *testing.T) {
234234

235235
for _, tt := range tests {
236236
t.Run(tt.name, func(t *testing.T) {
237-
result := BuildNamespaceObject(tt.nsName, tt.template)
237+
result, err := BuildNamespaceObject(tt.nsName, tt.template)
238+
require.NoError(t, err)
238239
tt.validate(t, result.Object)
239240
})
240241
}
@@ -332,3 +333,102 @@ func TestResolveNamespaceName_InvalidTemplate(t *testing.T) {
332333
require.Error(t, err)
333334
require.Contains(t, err.Error(), "failed to parse namespace template")
334335
}
336+
337+
func TestResolveNamespaceName_Validation(t *testing.T) {
338+
tests := []struct {
339+
name string
340+
annotations map[string]string
341+
packageName string
342+
expectErr bool
343+
errContains string
344+
}{
345+
{
346+
name: "rejects uppercase characters in suggested-namespace",
347+
annotations: map[string]string{
348+
AnnotationSuggestedNamespace: "Invalid-NS",
349+
},
350+
packageName: "pkg",
351+
expectErr: true,
352+
errContains: "not a valid DNS1123 label",
353+
},
354+
{
355+
name: "rejects name exceeding 63 characters",
356+
annotations: map[string]string{
357+
AnnotationSuggestedNamespace: "a234567890123456789012345678901234567890123456789012345678901234",
358+
},
359+
packageName: "pkg",
360+
expectErr: true,
361+
errContains: "exceeds 63 characters",
362+
},
363+
{
364+
name: "rejects name with dots",
365+
annotations: map[string]string{
366+
AnnotationSuggestedNamespace: "my.namespace",
367+
},
368+
packageName: "pkg",
369+
expectErr: true,
370+
errContains: "not a valid DNS1123 label",
371+
},
372+
{
373+
name: "rejects name starting with hyphen",
374+
annotations: map[string]string{
375+
AnnotationSuggestedNamespace: "-invalid",
376+
},
377+
packageName: "pkg",
378+
expectErr: true,
379+
errContains: "not a valid DNS1123 label",
380+
},
381+
{
382+
name: "accepts valid fallback name",
383+
annotations: nil,
384+
packageName: "my-package",
385+
expectErr: false,
386+
},
387+
{
388+
name: "accepts valid suggested-namespace",
389+
annotations: map[string]string{
390+
AnnotationSuggestedNamespace: "valid-ns-123",
391+
},
392+
packageName: "pkg",
393+
expectErr: false,
394+
},
395+
{
396+
name: "rejects invalid name from template",
397+
annotations: map[string]string{
398+
AnnotationSuggestedNamespaceTemplate: `{"metadata":{"name":"INVALID"}}`,
399+
},
400+
packageName: "pkg",
401+
expectErr: true,
402+
errContains: "not a valid DNS1123 label",
403+
},
404+
}
405+
406+
for _, tt := range tests {
407+
t.Run(tt.name, func(t *testing.T) {
408+
_, _, err := ResolveNamespaceName(tt.annotations, tt.packageName)
409+
if tt.expectErr {
410+
require.Error(t, err)
411+
require.Contains(t, err.Error(), tt.errContains)
412+
} else {
413+
require.NoError(t, err)
414+
}
415+
})
416+
}
417+
}
418+
419+
func TestBuildNamespaceObject_ReturnsError(t *testing.T) {
420+
// BuildNamespaceObject with valid input should not error
421+
_, err := BuildNamespaceObject("valid-ns", nil)
422+
require.NoError(t, err)
423+
424+
// With template
425+
template := &corev1.Namespace{
426+
ObjectMeta: metav1.ObjectMeta{
427+
Labels: map[string]string{"key": "value"},
428+
},
429+
}
430+
obj, err := BuildNamespaceObject("valid-ns", template)
431+
require.NoError(t, err)
432+
require.Equal(t, "valid-ns", obj.GetName())
433+
require.Equal(t, "value", obj.GetLabels()["key"])
434+
}

‎internal/operator-controller/controllers/clusterextension_reconcile_steps.go‎

Lines changed: 25 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -414,9 +414,9 @@ func ResolveNamespace(nsClient corev1client.NamespacesGetter) ReconcileStepFunc
414414
l.V(1).Info("validating user-provided namespace exists", "namespace", ext.Spec.Namespace)
415415
_, err := nsClient.Namespaces().Get(ctx, ext.Spec.Namespace, metav1.GetOptions{})
416416
if apierrors.IsNotFound(err) {
417-
err := fmt.Errorf("namespace %q not found; spec.namespace must reference an existing namespace", ext.Spec.Namespace)
418-
setStatusProgressing(ext, reconcile.TerminalError(err))
419-
return nil, err
417+
termErr := reconcile.TerminalError(fmt.Errorf("namespace %q not found; spec.namespace must reference an existing namespace", ext.Spec.Namespace))
418+
setStatusProgressing(ext, termErr)
419+
return nil, termErr
420420
}
421421
if err != nil {
422422
return nil, fmt.Errorf("error checking namespace %q: %w", ext.Spec.Namespace, err)
@@ -426,8 +426,14 @@ func ResolveNamespace(nsClient corev1client.NamespacesGetter) ReconcileStepFunc
426426
return nil, nil
427427
}
428428

429-
// Managed mode: resolve name from bundle annotations
429+
// Managed mode: resolve name from bundle annotations.
430+
// If imageFS is nil (fallback path), preserve the previously resolved namespace
431+
// from status to avoid wiping it in later steps.
430432
if state.imageFS == nil {
433+
if ext.Status.Namespace != "" {
434+
state.resolvedNamespace = ext.Status.Namespace
435+
state.namespaceManaged = true
436+
}
431437
return nil, nil
432438
}
433439

@@ -439,15 +445,15 @@ func ResolveNamespace(nsClient corev1client.NamespacesGetter) ReconcileStepFunc
439445
packageName := getPackageName(ext)
440446
resolvedName, template, err := applier.ResolveNamespaceName(bundleAnnotations, packageName)
441447
if err != nil {
442-
err := fmt.Errorf("error resolving namespace from bundle annotations: %w", err)
443-
setStatusProgressing(ext, reconcile.TerminalError(err))
444-
return nil, err
448+
termErr := reconcile.TerminalError(fmt.Errorf("error resolving namespace from bundle annotations: %w", err))
449+
setStatusProgressing(ext, termErr)
450+
return nil, termErr
445451
}
446452

447453
if ext.Status.Namespace != "" && ext.Status.Namespace != resolvedName {
448-
err := fmt.Errorf("bundle upgrade changes managed namespace from %q to %q; this is not supported", ext.Status.Namespace, resolvedName)
449-
setStatusProgressing(ext, reconcile.TerminalError(err))
450-
return nil, err
454+
termErr := reconcile.TerminalError(fmt.Errorf("bundle upgrade changes managed namespace from %q to %q; this is not supported", ext.Status.Namespace, resolvedName))
455+
setStatusProgressing(ext, termErr)
456+
return nil, termErr
451457
}
452458

453459
// On first install, verify managed namespace does not already exist.
@@ -456,9 +462,9 @@ func ResolveNamespace(nsClient corev1client.NamespacesGetter) ReconcileStepFunc
456462
l.V(1).Info("checking managed namespace does not already exist", "namespace", resolvedName)
457463
_, getErr := nsClient.Namespaces().Get(ctx, resolvedName, metav1.GetOptions{})
458464
if getErr == nil {
459-
err := fmt.Errorf("managed namespace %q already exists; use spec.namespace to install into an existing namespace", resolvedName)
460-
setStatusProgressing(ext, reconcile.TerminalError(err))
461-
return nil, err
465+
termErr := reconcile.TerminalError(fmt.Errorf("managed namespace %q already exists; use spec.namespace to install into an existing namespace", resolvedName))
466+
setStatusProgressing(ext, termErr)
467+
return nil, termErr
462468
}
463469
if !apierrors.IsNotFound(getErr) {
464470
return nil, fmt.Errorf("error checking namespace %q: %w", resolvedName, getErr)
@@ -490,6 +496,12 @@ func ApplyBundle(a Applier) ReconcileStepFunc {
490496
labels.OwnerNameKey: ext.GetName(),
491497
}
492498

499+
if state.namespaceManaged {
500+
termErr := reconcile.TerminalError(fmt.Errorf("managed namespace mode (omitting spec.namespace) requires the BoxcutterRuntime feature gate"))
501+
setStatusProgressing(ext, termErr)
502+
return nil, termErr
503+
}
504+
493505
l.Info("applying bundle contents")
494506
// NOTE: We need to be cautious of eating errors here.
495507
// We should always return any error that occurs during an

‎internal/operator-controller/controllers/clusterobjectset_controller.go‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -600,7 +600,10 @@ func collisionMessage(ores machinery.ObjectResult) string {
600600
if gvk.Kind == "Namespace" {
601601
return fmt.Sprintf("namespace %q is already managed by another controller", name)
602602
}
603-
return fmt.Sprintf("%s.%s %s/%s collision: %s", gvk.Kind, gvk.GroupVersion(), obj.GetNamespace(), name, ores.String())
603+
if ns := obj.GetNamespace(); ns != "" {
604+
return fmt.Sprintf("%s.%s %s/%s collision: %s", gvk.Kind, gvk.GroupVersion(), ns, name, ores.String())
605+
}
606+
return fmt.Sprintf("%s.%s %s collision: %s", gvk.Kind, gvk.GroupVersion(), name, ores.String())
604607
}
605608

606609
// EffectiveCollisionProtection resolves the collision protection value using

0 commit comments

Comments
 (0)