Add namespaces to different levels of deployable kustomizations#1475
Open
friegger wants to merge 1 commit into
Open
Add namespaces to different levels of deployable kustomizations#1475friegger wants to merge 1 commit into
friegger wants to merge 1 commit into
Conversation
apiserver and controller components now have:
- config/<component>/default/ - namespace+namePrefix only, emits no
Namespace (used as a base)
- config/<component>/standalone/ - wraps default/ and adds the
ironcore-system Namespace; use this for
single-component deploys
Shared Namespace lives in config/namespaces/ironcore-system/. The combined
config/default and config/etcdless installs reference the bases and the
namespace kustomization directly, eliminating the previous
remove-namespace.yaml patch dance. Build output for both is byte-identical
to main (only kustomize deprecation-warning lines disappear, since the
patches that triggered them are gone).
The namespaces folder can be extended with additional namespaces, e.g. for
the pool leases.
Behavioral change for downstream consumers: users who previously ran
`kustomize build config/controller/default` for a complete deploy must
migrate to `config/controller/standalone`; same for
`config/apiserver/default` -> `config/apiserver/standalone` (or
`config/apiserver/standalone-etcdless` for the external-etcd variant).
Consumers must not apply namespace transformers in overlays, these
will fail once we have multiple namespaces.
The Makefile install/uninstall/deploy/undeploy targets are retargeted at
the standalone variants accordingly. hack/validate-kustomize.sh is also
made portable (GNU realpath --relative-to is unavailable on macOS).
Signed-off-by: Felix Riegger <felix.riegger@sap.com>
friegger
changed the title
📝 WalkthroughWalkthroughThis PR refactors Kustomize namespace management by centralizing the ChangesKustomize Namespace Centralization Refactoring
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@hack/validate-kustomize.sh`:
- Line 15: The prefix-removal pattern dir="${path#$BASEDIR/../}" can misbehave
if BASEDIR contains glob meta-characters; update the pattern to quote BASEDIR
inside the parameter expansion so the prefix is treated literally (i.e. use the
variable name BASEDIR within the ${path#...} expression in a quoted form) —
modify the occurrence of ${path#$BASEDIR/../} to use a quoted BASEDIR in the
pattern so dir assignment is robust against globbing.
In `@Makefile`:
- Line 235: The uninstall/undeploy Makefile targets currently run "kubectl
delete -k config/apiserver/standalone" (and a similar line at the other
occurrence) which deletes the shared ironcore-system Namespace; change these
targets to avoid removing the Namespace by either pointing to a kustomize
overlay that excludes the Namespace resource or by splitting namespace lifecycle
into a separate explicit target (e.g., "uninstall-namespace") and having
uninstall/undeploy call only component-specific deletions; update the lines
containing "kubectl delete -k config/apiserver/standalone" (and the second
similar delete) to use the non-namespace overlay or remove the Namespace
deletion so removing one component won’t tear down the other.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 22fdf563-639d-4a15-b423-ecfe90aed72e
📒 Files selected for processing (23)
Makefileconfig/apiserver/kind/kustomization.yamlconfig/apiserver/server/server.yamlconfig/apiserver/standalone-etcdless/kustomization.yamlconfig/apiserver/standalone/kustomization.yamlconfig/controller/kind/kustomization.yamlconfig/controller/manager/manager.yamlconfig/controller/standalone/kustomization.yamlconfig/default/apiserver/kustomization.yamlconfig/default/apiserver/remove-namespace.yamlconfig/default/controller/kustomization.yamlconfig/default/controller/remove-namespace.yamlconfig/default/kustomization.yamlconfig/default/namespace.yamlconfig/etcdless/apiserver/kustomization.yamlconfig/etcdless/apiserver/remove-namespace.yamlconfig/etcdless/controller/kustomization.yamlconfig/etcdless/controller/remove-namespace.yamlconfig/etcdless/kustomization.yamlconfig/etcdless/namespace.yamlconfig/namespaces/ironcore-system/kustomization.yamlconfig/namespaces/ironcore-system/namespace.yamlhack/validate-kustomize.sh
💤 Files with no reviewable changes (12)
- config/etcdless/controller/remove-namespace.yaml
- config/default/namespace.yaml
- config/etcdless/apiserver/remove-namespace.yaml
- config/default/controller/remove-namespace.yaml
- config/etcdless/namespace.yaml
- config/etcdless/apiserver/kustomization.yaml
- config/default/apiserver/remove-namespace.yaml
- config/default/apiserver/kustomization.yaml
- config/default/controller/kustomization.yaml
- config/etcdless/controller/kustomization.yaml
- config/controller/manager/manager.yaml
- config/apiserver/server/server.yaml
| for kustomization in "$BASEDIR"/../config/*/**/kustomization.yaml; do | ||
| path="$(dirname "$kustomization")" | ||
| dir="$(realpath --relative-to "$BASEDIR"/.. "$path")" | ||
| dir="${path#$BASEDIR/../}" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant lines (including neighbors) around the referenced line
echo "== hack/validate-kustomize.sh (lines 1-40) =="
nl -ba hack/validate-kustomize.sh | sed -n '1,40p'
echo "== hack/validate-kustomize.sh: occurrences of BASEDIR and path#"$'\n'"BASEDIR =="
rg -n 'BASEDIR|path#\$\{?BASEDIR\}?|path#\$\BASEDIR|dir="\$\{path#\$\BASEDIR/.*\}"' hack/validate-kustomize.sh || trueRepository: ironcore-dev/ironcore
Length of output: 172
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== hack/validate-kustomize.sh (lines 1-60) =="
awk 'NR>=1 && NR<=60 {printf "%4d\t%s\n", NR, $0}' hack/validate-kustomize.sh
echo "== BASEDIR definitions/usages =="
rg -n 'BASEDIR|path#\$\{?BASEDIR\}?|dir="\$\{path#\$\{?BASEDIR\}?/.*"' hack/validate-kustomize.sh || trueRepository: ironcore-dev/ironcore
Length of output: 1059
Quote BASEDIR in the ${path#...} pattern to avoid globbing edge cases.
At Line 15, dir="${path#$BASEDIR/../}" uses prefix removal with a pattern; since BASEDIR is derived from filesystem paths (pwd), it could contain glob metacharacters, changing how stripping behaves. Quote the BASEDIR portion so it’s treated literally.
Proposed fix
- dir="${path#$BASEDIR/../}"
+ dir="${path#"$BASEDIR"/../}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| dir="${path#$BASEDIR/../}" | |
| dir="${path#"$BASEDIR"/../}" |
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 15-15: Expansions inside ${..} need to be quoted separately, otherwise they match as patterns.
(SC2295)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@hack/validate-kustomize.sh` at line 15, The prefix-removal pattern
dir="${path#$BASEDIR/../}" can misbehave if BASEDIR contains glob
meta-characters; update the pattern to quote BASEDIR inside the parameter
expansion so the prefix is treated literally (i.e. use the variable name BASEDIR
within the ${path#...} expression in a quoted form) — modify the occurrence of
${path#$BASEDIR/../} to use a quoted BASEDIR in the pattern so dir assignment is
robust against globbing.
| .PHONY: uninstall | ||
| uninstall: manifests kustomize ## Uninstall API server & API services from the K8s cluster specified in ~/.kube/config. | ||
| kubectl delete -k config/apiserver/default | ||
| kubectl delete -k config/apiserver/standalone |
There was a problem hiding this comment.
Avoid deleting the shared namespace from single-component teardown targets.
uninstall and undeploy now delete standalone kustomizations that include ironcore-system. If both components are installed independently, removing one can delete the namespace and tear down the other component unexpectedly.
Use delete targets that exclude the Namespace (or split namespace lifecycle into a separate explicit target).
Also applies to: 244-244
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Makefile` at line 235, The uninstall/undeploy Makefile targets currently run
"kubectl delete -k config/apiserver/standalone" (and a similar line at the other
occurrence) which deletes the shared ironcore-system Namespace; change these
targets to avoid removing the Namespace by either pointing to a kustomize
overlay that excludes the Namespace resource or by splitting namespace lifecycle
into a separate explicit target (e.g., "uninstall-namespace") and having
uninstall/undeploy call only component-specific deletions; update the lines
containing "kubectl delete -k config/apiserver/standalone" (and the second
similar delete) to use the non-namespace overlay or remove the Namespace
deletion so removing one component won’t tear down the other.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Deal with namespaces more explicitly by adding them to kustomizations on different levels directly, eliminating the previous remove-namespace.yaml patch dance.
apiserverandcontrollercomponents now have:Namespace (used as a base)
ironcore-system Namespace; use this for
single-component deploys
Shared Namespace lives in config/namespaces/ironcore-system/. The combined config/default and config/etcdless installs reference the bases and the Build output for both is byte-identical to main (only kustomize deprecation-warning lines disappear, since the patches that triggered them are gone).
The namespaces folder can be extended with additional namespaces, e.g. for the pool leases.
Behavioral change for downstream consumers of the low level kustomizations: users who previously ran
kustomize build config/controller/defaultfor a complete deploy must migrate toconfig/controller/standalone; same forconfig/apiserver/default->config/apiserver/standalone(orconfig/apiserver/standalone-etcdlessfor the external-etcd variant).Consumers must not apply namespace transformers in overlays, these will fail once we have multiple namespaces. This is true, independent of this change, once base kustomizations provide multiple namespaces.
The Makefile install/uninstall/deploy/undeploy targets are retargeted at the standalone variants accordingly. hack/validate-kustomize.sh is also made portable (GNU realpath --relative-to is unavailable on macOS).
Contributes to #1472
Summary by CodeRabbit