Skip to content

Commit d30a2bc

Browse files
nader-ziadanader-ziada
committed
address coderabbit comments
Signed-off-by: Nader Ziada <nziada@redhat.com>
1 parent 4843c7d commit d30a2bc

13 files changed

Lines changed: 192 additions & 64 deletions

File tree

‎Makefile‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -685,6 +685,10 @@ crd-ref-docs: $(CRD_REF_DOCS) #EXHELP Generate the API Reference Documents.
685685
$(CRD_REF_DOCS) --source-path=$(ROOT_DIR)/api/ \
686686
--config=$(API_REFERENCE_DIR)/crd-ref-docs-gen-config.yaml \
687687
--renderer=markdown --output-path=$(API_REFERENCE_DIR)/$(API_REFERENCE_FILENAME);
688+
# crd-ref-docs renders doc-comment text verbatim, including internal <opcon:...> generator
689+
# directives; strip them from the published reference (the per-channel contracts remain in prose).
690+
sed -E 's#</?opcon:[^>]*>##g' $(API_REFERENCE_DIR)/$(API_REFERENCE_FILENAME) > $(API_REFERENCE_DIR)/$(API_REFERENCE_FILENAME).tmp
691+
mv $(API_REFERENCE_DIR)/$(API_REFERENCE_FILENAME).tmp $(API_REFERENCE_DIR)/$(API_REFERENCE_FILENAME)
688692

689693
VENVDIR := $(abspath docs/.venv)
690694

‎api/v1/clusterextension_types.go‎

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -49,17 +49,18 @@ const (
4949

5050
// ClusterExtensionSpec defines the desired state of ClusterExtension
5151
type ClusterExtensionSpec struct {
52-
// namespace references an existing namespace where namespace-scoped resources
53-
// for the extension are applied. The namespace must already exist on the cluster.
52+
// namespace selects the namespace that namespace-scoped resources for the extension
53+
// are applied to.
5454
//
5555
// <opcon:standard:description>
56-
// namespace is required.
56+
// In the standard configuration, namespace is required and must reference an existing
57+
// namespace on the cluster.
5758
// </opcon:standard:description>
5859
// <opcon:experimental:description>
59-
// namespace is optional. When omitted, operator-controller resolves and creates a
60-
// managed namespace from bundle metadata. The mode (set vs omitted) is locked at
61-
// creation time and cannot be changed. Omitting namespace requires the experimental
62-
// feature set (BoxcutterRuntime).
60+
// BoxcutterRuntime feature set, namespace is optional.
61+
// When set, it must reference an existing namespace. When omitted, operator-controller
62+
// resolves and creates a managed namespace from bundle metadata. The mode (set vs omitted)
63+
// is locked at creation time and cannot be changed.
6364
// </opcon:experimental:description>
6465
//
6566
// The namespace field follows the DNS label standard as defined in [RFC 1123].
@@ -69,6 +70,7 @@ type ClusterExtensionSpec struct {
6970
// [RFC 1123]: https://tools.ietf.org/html/rfc1123
7071
//
7172
// <opcon:standard:validation:Required>
73+
// <opcon:standard:validation:XValidation:rule="self != ''",message="namespace is required">
7274
// <opcon:experimental:validation:XValidation:rule="oldSelf != '' || self == ''",message="namespace cannot be set after creation; mode is locked at creation time">
7375
//
7476
// +kubebuilder:validation:MaxLength:=63

‎applyconfigurations/api/v1/clusterextensionspec.go‎

Lines changed: 9 additions & 7 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎docs/api-reference/olmv1-api-reference.md‎

Lines changed: 19 additions & 19 deletions
Large diffs are not rendered by default.

‎helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterextensions.yaml‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -147,13 +147,13 @@ spec:
147147
rule: has(self.preflight)
148148
namespace:
149149
description: |-
150-
namespace references an existing namespace where namespace-scoped resources
151-
for the extension are applied. The namespace must already exist on the cluster.
150+
namespace selects the namespace that namespace-scoped resources for the extension
151+
are applied to.
152152
153-
namespace is optional. When omitted, operator-controller resolves and creates a
154-
managed namespace from bundle metadata. The mode (set vs omitted) is locked at
155-
creation time and cannot be changed. Omitting namespace requires the experimental
156-
feature set (BoxcutterRuntime).
153+
BoxcutterRuntime feature set, namespace is optional.
154+
When set, it must reference an existing namespace. When omitted, operator-controller
155+
resolves and creates a managed namespace from bundle metadata. The mode (set vs omitted)
156+
is locked at creation time and cannot be changed.
157157
158158
The namespace field follows the DNS label standard as defined in [RFC 1123].
159159
It must contain only lowercase alphanumeric characters or hyphens (-), start and end with an alphanumeric character,

‎helm/olmv1/base/operator-controller/crd/standard/olm.operatorframework.io_clusterextensions.yaml‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -109,10 +109,11 @@ spec:
109109
rule: has(self.preflight)
110110
namespace:
111111
description: |-
112-
namespace references an existing namespace where namespace-scoped resources
113-
for the extension are applied. The namespace must already exist on the cluster.
112+
namespace selects the namespace that namespace-scoped resources for the extension
113+
are applied to.
114114
115-
namespace is required.
115+
In the standard configuration, namespace is required and must reference an existing
116+
namespace on the cluster.
116117
117118
The namespace field follows the DNS label standard as defined in [RFC 1123].
118119
It must contain only lowercase alphanumeric characters or hyphens (-), start and end with an alphanumeric character,
@@ -126,6 +127,8 @@ spec:
126127
rule: self == '' || self.matches("^[a-z0-9]([-a-z0-9]*[a-z0-9])?$")
127128
- message: namespace is immutable once set
128129
rule: oldSelf == '' || self == oldSelf
130+
- message: namespace is required
131+
rule: self != ''
129132
serviceAccount:
130133
description: |-
131134
serviceAccount is a deprecated field and is completely ignored.

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -598,7 +598,7 @@ func collisionMessage(ores machinery.ObjectResult) string {
598598
}
599599

600600
if gvk.Kind == "Namespace" {
601-
return fmt.Sprintf("namespace %q is already managed by another controller", name)
601+
return fmt.Sprintf("namespace %q already exists and cannot be adopted", name)
602602
}
603603
if ns := obj.GetNamespace(); ns != "" {
604604
return fmt.Sprintf("%s.%s %s/%s collision: %s", gvk.Kind, gvk.GroupVersion(), ns, name, ores.String())

‎internal/operator-controller/rukpak/render/namespace.go‎

Lines changed: 55 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"encoding/json"
55
"fmt"
66
"regexp"
7+
"strings"
78

89
corev1 "k8s.io/api/core/v1"
910
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -12,6 +13,7 @@ import (
1213
"sigs.k8s.io/controller-runtime/pkg/client"
1314

1415
"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/bundle"
16+
hashutil "github.com/operator-framework/operator-controller/internal/shared/util/hash"
1517
)
1618

1719
const (
@@ -47,7 +49,9 @@ func resolveSystemManagedNamespace(rv1 *bundle.RegistryV1) (string, *corev1.Name
4749
case csvAnnotations[AnnotationSuggestedNamespace] != "":
4850
name = csvAnnotations[AnnotationSuggestedNamespace]
4951
default:
50-
name = fmt.Sprintf("%s-system", rv1.PackageName)
52+
// The auto-derived default must always be a valid namespace, even for package names
53+
// with disallowed characters (e.g. dots) or names that are too long.
54+
name = defaultInstallNamespace(rv1.PackageName)
5155
}
5256

5357
if err := validateNamespaceName(name); err != nil {
@@ -71,19 +75,67 @@ func parseNamespaceTemplate(csvAnnotations map[string]string) (*corev1.Namespace
7175
return &ns, nil
7276
}
7377

78+
const maxNamespaceNameLength = 63
79+
7480
func validateNamespaceName(name string) error {
7581
if name == "" {
7682
return fmt.Errorf("resolved namespace name is empty")
7783
}
78-
if len(name) > 63 {
79-
return fmt.Errorf("resolved namespace name %q exceeds 63 characters", name)
84+
if len(name) > maxNamespaceNameLength {
85+
return fmt.Errorf("resolved namespace name %q exceeds %d characters", name, maxNamespaceNameLength)
8086
}
8187
if !dns1123LabelRegexp.MatchString(name) {
8288
return fmt.Errorf("resolved namespace name %q is not a valid DNS1123 label", name)
8389
}
8490
return nil
8591
}
8692

93+
// defaultInstallNamespace derives a deterministic, DNS1123-label-valid namespace name for a
94+
// package when the bundle does not suggest one. It normalizes disallowed characters (e.g. dots)
95+
// and enforces the namespace length limit. When the normalized name must be truncated, a short
96+
// hash of the original package name is appended to preserve deterministic collision resistance.
97+
func defaultInstallNamespace(packageName string) string {
98+
const suffix = "system"
99+
100+
base := sanitizeDNS1123Label(packageName)
101+
102+
// Fast path: an already-valid, short base keeps the historical "<package>-system" name.
103+
if base != "" && len(base)+1+len(suffix) <= maxNamespaceNameLength {
104+
return base + "-" + suffix
105+
}
106+
107+
// Otherwise keep the name deterministic and collision-resistant: append a short hash of the
108+
// original package name and truncate the base to fit within the length limit.
109+
hash := hashutil.DeepHashObject(packageName)[:8]
110+
maxBase := maxNamespaceNameLength - len(suffix) - len(hash) - 2 // account for two '-' separators
111+
if len(base) > maxBase {
112+
base = base[:maxBase]
113+
}
114+
base = strings.Trim(base, "-")
115+
if base == "" {
116+
return hash + "-" + suffix
117+
}
118+
return base + "-" + hash + "-" + suffix
119+
}
120+
121+
// sanitizeDNS1123Label lowercases s, replaces each run of disallowed characters with a single
122+
// hyphen, and trims leading/trailing hyphens so the result is a valid DNS1123 label (or empty).
123+
func sanitizeDNS1123Label(s string) string {
124+
var b strings.Builder
125+
lastHyphen := false
126+
for _, r := range strings.ToLower(s) {
127+
switch {
128+
case (r >= 'a' && r <= 'z') || (r >= '0' && r <= '9'):
129+
b.WriteRune(r)
130+
lastHyphen = false
131+
case !lastHyphen:
132+
b.WriteByte('-')
133+
lastHyphen = true
134+
}
135+
}
136+
return strings.Trim(b.String(), "-")
137+
}
138+
87139
// BuildNamespaceObject returns the Namespace object to include in the rendered set,
88140
// seeding labels and annotations from the optional template. Empty spec/status are
89141
// stripped to avoid apply drift.

‎internal/operator-controller/rukpak/render/namespace_test.go‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package render
22

33
import (
4+
"strings"
45
"testing"
56

67
"github.com/stretchr/testify/assert"
@@ -301,3 +302,61 @@ func TestBuildNamespaceObject(t *testing.T) {
301302
})
302303
}
303304
}
305+
306+
func TestDefaultInstallNamespace(t *testing.T) {
307+
tests := []struct {
308+
name string
309+
packageName string
310+
want string // exact expected name; empty means only assert validity
311+
}{
312+
{
313+
name: "valid short name keeps <package>-system",
314+
packageName: "argocd-operator",
315+
want: "argocd-operator-system",
316+
},
317+
{
318+
name: "dotted package name is normalized",
319+
packageName: "my.operator",
320+
want: "my-operator-system",
321+
},
322+
{
323+
name: "uppercase and underscores are normalized",
324+
packageName: "My_Operator",
325+
want: "my-operator-system",
326+
},
327+
{
328+
name: "overlong package name is truncated to a valid label",
329+
packageName: strings.Repeat("a", 80),
330+
// no exact expectation; validated below
331+
},
332+
{
333+
name: "package name with no valid characters still yields a valid namespace",
334+
packageName: "...",
335+
// no exact expectation; validated below
336+
},
337+
}
338+
339+
for _, tt := range tests {
340+
t.Run(tt.name, func(t *testing.T) {
341+
got := defaultInstallNamespace(tt.packageName)
342+
343+
// The default must always be a valid, length-bounded namespace name.
344+
require.NoError(t, validateNamespaceName(got))
345+
346+
// It must be deterministic.
347+
require.Equal(t, got, defaultInstallNamespace(tt.packageName))
348+
349+
if tt.want != "" {
350+
require.Equal(t, tt.want, got)
351+
}
352+
})
353+
}
354+
355+
t.Run("distinct overlong names that share a prefix do not collide", func(t *testing.T) {
356+
a := defaultInstallNamespace(strings.Repeat("a", 70) + "-one")
357+
b := defaultInstallNamespace(strings.Repeat("a", 70) + "-two")
358+
require.NoError(t, validateNamespaceName(a))
359+
require.NoError(t, validateNamespaceName(b))
360+
require.NotEqual(t, a, b)
361+
})
362+
}

‎manifests/experimental-e2e.yaml‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -761,13 +761,13 @@ spec:
761761
rule: has(self.preflight)
762762
namespace:
763763
description: |-
764-
namespace references an existing namespace where namespace-scoped resources
765-
for the extension are applied. The namespace must already exist on the cluster.
764+
namespace selects the namespace that namespace-scoped resources for the extension
765+
are applied to.
766766
767-
namespace is optional. When omitted, operator-controller resolves and creates a
768-
managed namespace from bundle metadata. The mode (set vs omitted) is locked at
769-
creation time and cannot be changed. Omitting namespace requires the experimental
770-
feature set (BoxcutterRuntime).
767+
BoxcutterRuntime feature set, namespace is optional.
768+
When set, it must reference an existing namespace. When omitted, operator-controller
769+
resolves and creates a managed namespace from bundle metadata. The mode (set vs omitted)
770+
is locked at creation time and cannot be changed.
771771
772772
The namespace field follows the DNS label standard as defined in [RFC 1123].
773773
It must contain only lowercase alphanumeric characters or hyphens (-), start and end with an alphanumeric character,

0 commit comments

Comments
 (0)