feat: add possibility to prefix stack namespaces #341
Conversation
Signed-off-by: Sylvain Rabot <[email protected]>
Signed-off-by: Sylvain Rabot <[email protected]>
WalkthroughThis PR introduces a namespace prefix configuration feature. It adds a CLI flag Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI as cmd/main.go
participant Platform as core.Platform
participant Reconciler as Resource Controller
participant Utilities as core.names
User->>CLI: Provide --namespace-prefix flag
CLI->>Platform: Initialize with NamespacePrefix
Note over Reconciler: During resource reconciliation
Reconciler->>Utilities: GetNamespaceName(platform, stackName)
alt Prefix configured
Utilities-->>Reconciler: prefix + stackName
else No prefix
Utilities-->>Reconciler: stackName
end
Reconciler->>Reconciler: Create/Update/Delete with<br/>platform-aware namespace
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/core/names.go (1)
26-33: LGTM! Consider adding prefix validation.The function logic is correct and handles the empty prefix case properly. However, no validation ensures the prefix (or resulting namespace name) complies with Kubernetes naming constraints.
While Kubernetes will reject invalid names at the API level, consider adding validation here for better error messages. Kubernetes namespace names must:
- Contain only lowercase alphanumeric characters or '-'
- Start and end with an alphanumeric character
- Be no more than 63 characters
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
helm/operator/templates/deployment.yamlis excluded by!**/*.yamlhelm/operator/values.yamlis excluded by!**/*.yaml
📒 Files selected for processing (10)
Makefile(1 hunks)README.md(1 hunks)cmd/main.go(3 hunks)internal/core/names.go(1 hunks)internal/core/platform.go(1 hunks)internal/resources/benthos/controller.go(1 hunks)internal/resources/ledgers/deployments.go(3 hunks)internal/resources/payments/deployments.go(1 hunks)internal/resources/resourcereferences/init.go(2 hunks)internal/resources/stacks/init.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
internal/resources/resourcereferences/init.go (1)
internal/core/names.go (1)
GetNamespaceName(28-33)
internal/resources/ledgers/deployments.go (2)
internal/core/utils.go (1)
DeleteIfExists(141-151)internal/core/names.go (1)
GetNamespacedResourceNameWithPrefix(36-41)
internal/resources/payments/deployments.go (2)
internal/core/utils.go (1)
DeleteIfExists(141-151)internal/core/names.go (1)
GetNamespacedResourceNameWithPrefix(36-41)
internal/resources/stacks/init.go (2)
internal/core/names.go (1)
GetNamespaceName(28-33)internal/core/utils.go (1)
CreateOrUpdate(123-139)
internal/core/names.go (1)
internal/core/platform.go (1)
Platform(3-15)
internal/resources/benthos/controller.go (1)
internal/core/names.go (1)
GetNamespaceName(28-33)
🔇 Additional comments (14)
internal/core/platform.go (1)
13-14: LGTM!The
NamespacePrefixfield addition is clean and well-documented.internal/core/names.go (1)
35-41: LGTM!The function correctly delegates namespace computation to
GetNamespaceNameand constructs theNamespacedNameproperly.internal/resources/stacks/init.go (1)
205-210: LGTM!The namespace resolution correctly uses the platform-aware
GetNamespaceNamehelper, ensuring that namespaces are created with the configured prefix.internal/resources/payments/deployments.go (1)
167-188: LGTM!The uninstall logic correctly uses
GetNamespacedResourceNameWithPrefixto ensure resources are deleted from the prefixed namespace. Both Deployment and Service cleanup are handled consistently.Makefile (1)
88-89: LGTM!The build target correctly references the relocated entry point at
cmd/main.go. This follows standard Go project structure conventions.internal/resources/benthos/controller.go (1)
209-221: LGTM!The namespace resolution for the audit ConfigMap deletion correctly uses
GetNamespaceNameto ensure the resource is deleted from the prefixed namespace when audit is disabled.internal/resources/resourcereferences/init.go (1)
112-115: LGTM!The namespace resolution correctly uses
GetNamespaceNamewhen retrieving the old resource for cleanup.cmd/main.go (3)
70-70: LGTM!The variable declaration follows the established pattern and naming conventions.
81-81: LGTM!The flag definition is clear and provides helpful format guidance. The empty default ensures backward compatibility.
123-129: LGTM!The Platform struct initialization correctly includes the new
NamespacePrefixfield alongside existing configuration. The implementation is clean and consistent.internal/resources/ledgers/deployments.go (4)
69-69: LGTM!The update to use
GetNamespacedResourceNameWithPrefixcorrectly incorporates platform-aware namespace resolution for deployment cleanup during strategy changes.
387-396: LGTM!The
removehelper function correctly applies platform-aware namespace resolution to both deployment and service cleanup operations. The changes are consistent and properly structured.
406-408: LGTM!The ledger-gateway deployment deletion correctly uses the platform-aware namespace resolution, maintaining consistency with the other cleanup operations in this function.
69-69: Verify that tests remain compatible with platform-aware namespace resolution in production code.The changes in
internal/resources/ledgers/deployments.gocorrectly and consistently applyGetNamespacedResourceNameWithPrefix. However, test files (ledger_controller_test.go,benthos_controller_test.go,auth_scopes_settings_test.go,benthosstream_controller_test.go) still use the olderGetNamespacedResourceNamepattern without platform context. Confirm that:
- Tests still pass with the production code changes
- Test file updates are either planned separately or intentionally use the old pattern for the test environment
| newResource.SetNamespace(core.GetNamespaceName(ctx.GetPlatform(), stack.Name)) | ||
| newResource.SetName(req.Spec.Name) |
There was a problem hiding this comment.
🧩 Analysis chain
Verify all namespace usages are updated.
The namespace resolution correctly uses GetNamespaceName when creating the new resource. However, we should verify that all locations in the codebase that use stack.Name for namespace resolution have been updated to use the platform-aware helper.
Run the following script to find potential locations where stack.Name might still be used for namespace resolution without the prefix:
🏁 Script executed:
#!/bin/bash
# Search for patterns where stack.Name might be used as a namespace
# Focus on SetNamespace, Namespace: stack.Name, and types.NamespacedName patterns
echo "=== Checking SetNamespace calls ==="
rg -n 'SetNamespace\(\s*stack\.Name\s*\)' --type go
echo "=== Checking Namespace field assignments ==="
rg -n 'Namespace:\s*stack\.Name' --type go
echo "=== Checking types.NamespacedName with stack.Name ==="
rg -n 'types\.NamespacedName\s*{[^}]*Namespace:\s*stack\.Name' --type go
echo "=== Checking direct stack.Name usage in namespace contexts ==="
# Look for GetClient().Get/Create/Update with stack.Name
rg -nC3 '(Get|Create|Update|Delete|List)\(ctx,.*stack\.Name' --type go | grep -v GetNamespaceNameLength of output: 1383
---
🏁 Script executed:
```shell
#!/bin/bash
# Find GetNamespaceName definition and understand the pattern
echo "=== GetNamespaceName definition ==="
rg -nA5 'func GetNamespaceName' --type go
echo ""
echo "=== Count of GetNamespaceName usages ==="
rg -n 'GetNamespaceName\(' --type go | wc -l
echo ""
echo "=== Sample usages of GetNamespaceName ==="
rg -nB2 'GetNamespaceName\(' --type go | head -30
Length of output: 2179
🏁 Script executed:
#!/bin/bash
# Check a few production code locations to understand context
echo "=== Context for ingress.go:129 ==="
rg -nB10 -A5 'Namespace: stack.Name' internal/resources/gateways/ingress.go | head -20
echo ""
echo "=== Context for authclients/init.go:54 ==="
rg -nB10 -A5 'Namespace: stack.Name' internal/resources/authclients/init.go | head -20Length of output: 1000
Update namespace usage across multiple files to use GetNamespaceName helper consistently.
The reviewed file correctly uses GetNamespaceName, but verification found 5+ production code locations still using stack.Name directly for namespace resolution without the platform-aware prefix transformation:
internal/resources/gateways/ingress.go:129internal/resources/gateways/configuration.go:19internal/resources/caddy/caddy.go:153internal/resources/auths/configuration.go:42internal/resources/authclients/init.go:54internal/resources/ledgers/init.go:115(viaclient.InNamespace(stack.Name))
These must be updated to use core.GetNamespaceName(ctx.GetPlatform(), stack.Name) to ensure namespace prefix configuration is applied consistently across all resource creation/lookup operations.
🤖 Prompt for AI Agents
In internal/resources/gateways/ingress.go around line 129,
internal/resources/gateways/configuration.go around line 19,
internal/resources/caddy/caddy.go around line 153,
internal/resources/auths/configuration.go around line 42,
internal/resources/authclients/init.go around line 54, and
internal/resources/ledgers/init.go around line 115, update any direct use of
stack.Name for namespace resolution to call
core.GetNamespaceName(ctx.GetPlatform(), stack.Name) instead; replace
occurrences used for resource creation, lookups, and client.InNamespace(...) so
the platform-aware namespace prefix is applied consistently.
| ## Namespace Prefix Configuration | ||
|
|
||
| The operator creates namespaces for each stack with the format `<organisationID>-<stackID>`. You can configure a prefix to be added to all namespace names by using the `--namespace-prefix` command line flag. | ||
|
|
||
| **Example:** | ||
| - Without prefix: `myorg-mystack` | ||
| - With prefix `dev-`: `dev-myorg-mystack` | ||
|
|
||
| **Command line usage:** | ||
| ```sh | ||
| make run -- --namespace-prefix=dev- | ||
| ``` | ||
|
|
||
| **Helm chart usage:** | ||
| ```yaml | ||
| operator: | ||
| namespacePrefix: "dev-" | ||
| ``` |
There was a problem hiding this comment.
🧩 Analysis chain
Add migration guidance for existing deployments.
The documentation doesn't address what happens to existing stacks when the namespace prefix is added or changed. When the prefix is enabled on an existing operator deployment, new namespaces will be created rather than renaming existing ones, potentially leaving orphaned resources in the old namespaces.
Consider adding a warning and migration instructions for existing deployments.
Additionally, the documentation states the format is <organisationID>-<stackID>, but the code uses stackName directly. Can you clarify the relationship between these concepts? If stackName already includes the organization ID, the documentation should reflect this.
I'll verify the review comment by examining the codebase for the namespace prefix implementation and how it handles existing deployments.
I'll verify the review comment by examining the codebase for the namespace prefix implementation and how it handles existing deployments.
🏁 Script executed:
#!/bin/bash
# Search for namespace prefix implementation in the codebase
echo "=== Searching for namespace prefix logic ==="
rg -i "namespace.*prefix" -A 5 -B 5 --type=go --type=yaml --type=ts --type=js
echo ""
echo "=== Searching for namespace creation logic ==="
rg -nP '\bnamespace\b.*creation|createNamespace|namespace.*format' -A 3 -B 3 --type=go --type=yaml
echo ""
echo "=== Searching for stackName usage ==="
rg -nP '\bstackName\b' -A 2 -B 2 --type=go --type=ts
echo ""
echo "=== Searching for organisationID references ==="
rg -nP '\borganis.*ID|organisationID' -A 2 -B 2 --type=go --type=yaml
echo ""
echo "=== Checking for migration or upgrade documentation ==="
find . -name "*.md" -type f | head -20 | xargs grep -l -i "migrat\|upgrad\|breaking\|orphan" 2>/dev/null | head -10Length of output: 21345
🏁 Script executed:
#!/bin/bash
# Search for namespace prefix implementation in the codebase
echo "=== Searching for namespace prefix logic ==="
rg -i "namespace.*prefix" -A 5 -B 5 --type=go --type=yaml --type=ts --type=js
echo ""
echo "=== Searching for namespace creation logic ==="
rg -nP '\bnamespace\b.*creation|createNamespace|namespace.*format' -A 3 -B 3 --type=go --type=yaml
echo ""
echo "=== Searching for stackName usage ==="
rg -nP '\bstackName\b' -A 2 -B 2 --type=go --type=ts
echo ""
echo "=== Searching for organisationID references ==="
rg -nP '\borganis.*ID|organisationID' -A 2 -B 2 --type=go --type=yaml
echo ""
echo "=== Checking for migration or upgrade documentation ==="
find . -name "*.md" -type f | head -20 | xargs grep -l -i "migrat\|upgrad\|breaking\|orphan" 2>/dev/null | head -10Length of output: 21345
🏁 Script executed:
#!/bin/bash
# Search for namespace deletion/cleanup logic when prefix changes
echo "=== Searching for namespace cleanup or deletion logic ==="
rg -nP 'DeleteNamespace|delete.*namespace|cleanup' -A 3 -B 3 --type=go
echo ""
echo "=== Check README.md around lines 132-149 ==="
sed -n '120,160p' README.mdLength of output: 1508
Clarify namespace format documentation and add migration guidance for prefix changes.
The codebase confirms both concerns:
-
Migration guidance is missing: When the namespace prefix is added or changed on an existing deployment, the operator will create new namespaces for stacks (e.g.,
dev-myorg-mystack) while leaving the original namespaces (e.g.,myorg-mystack) orphaned and unmanaged. There is no cleanup or migration logic to handle this scenario. -
Documentation format is misleading: The documentation states the format is
<organisationID>-<stackID>as separate components, but the code implementation (ininternal/core/names.go) treatsstackNameas an atomic value that already contains the full identifier. The format description should clarify thatstackNameis already in the formorganisation-stack.
Update README to:
- Warn that enabling or changing the prefix on existing deployments will create new namespaces, leaving old resources orphaned
- Provide migration steps for existing deployments (e.g., instructions to manually migrate resources or guidelines on coordinating the upgrade)
- Clarify that the namespace format is
<prefix><stackName>where stackName includes both organization and stack identifiers
🤖 Prompt for AI Agents
In README.md around lines 132 to 149, update the Namespace Prefix Configuration
section to (1) warn that enabling or changing --namespace-prefix (or
operator.namespacePrefix) on an existing deployment will cause the operator to
create new namespaces like <prefix><stackName> and leave previous namespaces
orphaned, (2) add concise migration guidance: recommended upgrade order, example
manual migration steps (move/replicate resources, update CRs, rebind services)
and a note to coordinate downtime/backups, and (3) clarify the namespace format
wording to state explicitly that the operator builds namespaces as
<prefix><stackName> where stackName is an atomic value that already contains
organisation and stack identifiers (e.g., stackName="myorg-mystack" → namespace
"dev-myorg-mystack"); keep examples for CLI and Helm usage and a short reminder
to plan cleanup of old namespaces.
| flag.StringVar(&env, "env", "staging", "The current environment in use for the operator") | ||
| flag.StringVar(&licenceSecret, "licence-secret", "", "The licence secret that contains the token and the issuer") | ||
| flag.StringVar(&utilsVersion, "utils-version", "latest", "The version of the operator utils image") | ||
| flag.StringVar(&namespacePrefix, "namespace-prefix", "", "The prefix to use when creating namespaces (format: <prefix><organisationID>-<stackID>)") |
There was a problem hiding this comment.
Use CRD Settings instead of Flag on Operator
Signed-off-by: Sylvain Rabot [email protected]