[gjkim42]

What type of PR is this?

/kind bug

What this PR does / why we need it:

helm install -n <ns> only moved part of Kelos into <ns>. The controller manager and its supporting resources were hardcoded to kelos-system, and the chart always created Namespace/kelos-system.

This PR makes every namespaced resource follow the release namespace:

• Hand-written templates: the controller Deployment, kelos-controller ServiceAccount, kelos-webhook Service, Certificate (including dnsNames), Issuer, leader-election Role/RoleBinding, kelos-controller-rolebinding subject, and the telemetry CronJob use {{ .Release.Namespace }}. The control-plane PodMonitor selects only the release namespace.
• Generated files: hack/update-install-manifest.sh templates the webhook Service namespace and cert-manager.io/inject-ca-from in validating-webhook.yaml and in the subchart CRDs that carry conversion webhooks. The static install-crd.yaml (applied by kelos install) keeps kelos-system.
• Namespace object: rendered only when the release namespace is kelos-system, so kelos install and existing kelos-system releases keep owning it. Other namespaces come from --create-namespace or the user.
• Telemetry: the installation ID ConfigMap namespace is injected through a new --telemetry-namespace controller flag, set by the CronJob to the release namespace. It is required with --telemetry-report.
• kelos install / kelos uninstall: no behavior change; they still render into kelos-system.
• Conversion guard: with crds.install=false, the chart looks up tasks.kelos.dev and fails if its conversion webhook names a namespace other than the release namespace. This stops an upgrade from moving the kelos-webhook Service away from CRDs the release does not render (CRDs from kelos install, or CRDs kept by crds.keep during phase 1 of a two-phase upgrade). lookup finds nothing under helm template and kelos install, so offline rendering is unaffected; tools that render with helm template (such as Argo CD) skip the check, and crds.install=false releases need get on customresourcedefinitions. The failure message points to "Release Namespace" in the chart README.
• Docs: chart README ("Release Namespace" section with the guard and a migration guide for releases that own kelos-system from another namespace; the Option 2 CRD adoption loop now lists taskpipelines.kelos.dev and webhookgateways.kelos.dev, which the chart also renders), the podMonitor.enabled row in docs/reference.md, and the troubleshooting skill.
• Integration suite: test/integration/suite_test.go renders the chart's ValidatingWebhookConfiguration for envtest instead of reading templates/validating-webhook.yaml, which is no longer plain YAML.

Which issue(s) this PR is related to:

Fixes #1816

Special notes for your reviewer:

Not waiting on #1725. The issue marks this as blocked on #1725 because of the CRD conversion webhook. This PR templates the conversion clientConfig.service.namespace and CA annotation in the subchart CRDs, so Helm-rendered CRDs (crds.install=true) work in any namespace. For CRDs the release does not render, the conversion guard refuses an install or upgrade that would leave them pointing at a namespace without the webhook, and the README migration guide covers the one-phase crds.install=true upgrade (adopting outside-managed CRDs first). Once #1725 removes conversion, the guard becomes a no-op (CRDs without a conversion webhook pass) and can be dropped along with the README conversion notes. #1725 will need a small rebase over hack/update-install-manifest.sh, the subchart CRDs, webhook.yaml, render_test.go, and the README.

Decision needed: kelos-system deletion on upgrade. Existing releases installed with -n <other> own Namespace/kelos-system. Upgrading them stops rendering it, so Helm deletes kelos-system and anything in it, and their telemetry installation ID is regenerated. This follows the approach proposed in the issue. Operators who want to keep the namespace can run kubectl annotate namespace kelos-system helm.sh/resource-policy=keep before upgrading; Helm checks the live object's annotation before deleting it (pkg/kube/client.go in Helm v3.20.1). The README and release note document this opt-out.

Verification

• make verify, make test, and make test-integration pass.
• New TestRender_ReleaseNamespace renders into kelos-system and kelos and checks namespaced objects, ServiceAccount binding subjects, Certificate dnsNames, webhook/CRD conversion service namespaces, CA annotations, the telemetry flag, the PodMonitor selector, and that the Namespace object is rendered only for kelos-system. Reverting the template changes makes it fail.
• Helm v3.20 against an envtest API server (Kubernetes 1.31) with cert-manager CRDs:
  • fresh helm install -n team-b --create-namespace --set crds.install=true: deployed; all 11 namespaced objects in team-b; no kelos-system namespace.
  • -n kelos-system release on the previous chart upgraded to this chart: Namespace/kelos-system kept (same UID, Active).
  • -n team-a release on the previous chart upgraded to this chart: controller, Service, and Certificate move to team-a; kelos-system is deleted, as described above.
  • -n team-a release on the previous chart with Helm-owned CRDs: phase 1 (crds.install=false) is refused by the conversion guard with nothing applied; after kubectl annotate namespace kelos-system helm.sh/resource-policy=keep, a single crds.install=true upgrade moves the CRD conversion to team-a and keeps kelos-system; a later phase 1 upgrade passes the guard.
• New TestRender_CRDConversionNamespaceGuard renders through a fake lookup client and covers the rejection plus no live CRD, a CRD without conversion, matching namespaces, a kelos-system release, and crds.install=true.

Pre-existing, not changed here: on main, a first helm install -n kelos-system --create-namespace fails with Helm v3.20 (namespaces "kelos-system" already exists) because both Helm and the chart create the namespace; a retry succeeds as an upgrade. The behavior for kelos-system releases is unchanged by this PR.

Does this PR introduce a user-facing change?

The Helm chart now installs the controller manager, its webhook Service, Certificate, Issuer, leader-election RBAC, and the telemetry CronJob into the Helm release namespace instead of always using `kelos-system`. The chart renders `Namespace/kelos-system` only when the release namespace is `kelos-system`; other release namespaces must already exist or be created with `--create-namespace`. Releases installed into `kelos-system` and `kelos install` are unaffected.

Action required for releases installed into a namespace other than `kelos-system`: the upgrade moves these resources into the release namespace and deletes the `kelos-system` namespace, including anything else in it, unless you first run `kubectl annotate namespace kelos-system helm.sh/resource-policy=keep`. The telemetry installation ID is regenerated. Upgrade in one step with `--set crds.install=true` instead of the two-phase flow; if the CRDs are managed outside Helm, adopt them into the release first. With `crds.install=false`, installs and upgrades now fail when the existing `tasks.kelos.dev` CRD sends conversion requests to a namespace other than the release namespace. See "Release Namespace" in the chart README.

🤖 Generated with Claude Code

---

Summary by cubic

Every namespaced Helm chart resource now follows the release namespace instead of hardcoding kelos-system, so helm install -n <ns> no longer splits Kelos across two namespaces.

• The controller Deployment, ServiceAccount, webhook Service, Certificate, Issuer, leader-election RBAC, and telemetry CronJob use {{ .Release.Namespace }}, as do the webhook Service and CA annotations in the validating webhook and in the subchart CRDs.
• Namespace/kelos-system is rendered only for kelos-system releases, so kelos install keeps owning it; other namespaces come from --create-namespace or the user.
• The telemetry CronJob passes its namespace through the new required --telemetry-namespace flag.
• With crds.install=false, the chart looks up tasks.kelos.dev and fails if its conversion webhook names a namespace other than the release namespace; lookup finds nothing under helm template, so offline rendering is unaffected.
• The README documents the migration path and the CRD adoption loop now covers every CRD the chart renders.
• The integration suite renders the chart and hands envtest the resulting ValidatingWebhookConfiguration, since the template is no longer plain YAML.
• kelos install and kelos uninstall are unchanged.

Migration

• Upgrading a release in another namespace moves these resources into the release namespace and deletes kelos-system, including anything else in it; the telemetry installation ID is regenerated. Annotate the namespace with helm.sh/resource-policy=keep to retain it.
• Upgrade such a release once with crds.install=true, adopting outside-managed CRDs into Helm first; the two-phase flow is not supported for this upgrade.

^{Written for commit 9cbd4c1. Summary will update on new commits.}

[gjkim42]

/kelos api-review

[kelos-bot]

🤖 Kelos Task Status

Task kelos-api-reviewer-issue-comment-c5f83e658279 has succeeded. ✅

[gjkim42]

/kelos review

[kelos-bot]

🤖 Kelos Task Status

Task kelos-reviewer-issue-comment-537d61ccc7c7 has succeeded. ✅

[cubic-dev-ai]

All reported issues were addressed across 21 files

<sub>Reply with feedback, questions, or to request a fix.

View guided diff | Turn on auto-fix | Re-trigger cubic</sub>

[kelos-bot]

🤖 Kelos API Reviewer Agent @gjkim42

API Design Review

Verdict: APPROVE
Scope: No api/ type, CRD schema, or chart values changes. This review covers the release-namespace contract for chart resources, the CRD conversion clientConfig / inject-ca-from templating, the new crds.install=false conversion-namespace guard, the Namespace/kelos-system ownership rule, and the --telemetry-namespace flag. Reviewed at 12871eb.

Findings

Compatibility

• [Resolved] The earlier P1, where existing non-kelos-system releases lost CRD conversion, is fixed - internal/manifests/charts/kelos/templates/webhook.yaml:8-14, internal/manifests/charts/kelos/README.md:37-64
  The guard runs only with crds.install=false. It compares the live tasks.kelos.dev conversion Service namespace with .Release.Namespace. It passes when the CRD is missing, has no conversion webhook, or under helm template, so after Remove v1alpha1 API #1725 it becomes a no-op. It is not a blanket namespace check, so phase 1 of a later two-phase upgrade still works once a release has migrated. TestRender_CRDConversionNamespaceGuard covers the rejection and each pass case. The README migration section and the release note both describe a single crds.install=true upgrade, with adoption first for CRDs managed outside Helm. No further change needed.

• [P2] Maintainer sign-off: by default, an upgrade deletes Namespace/kelos-system - internal/manifests/charts/kelos/templates/namespace.yaml:7, internal/manifests/charts/kelos/README.md:52
  From an API standpoint, the contract is right: the chart owns only the namespace that kelos install expects. The documented opt-out works. Helm v3.20.1 pkg/kube/client.go:436-450 fetches the live object and skips the delete when it carries helm.sh/resource-policy=keep. The PR asks for an explicit decision on this default, so a maintainer should record that sign-off before merge.

Validation / Docs (guard contract)

• [P3] The guard's error message names a remedy that fails for some users - internal/manifests/charts/kelos/templates/webhook.yaml:12
  The message only says "set crds.install=true". If the CRDs came from kelos install, that upgrade then fails on Helm's ownership metadata check (missing app.kubernetes.io/managed-by and meta.helm.sh/*). For a new release, the simpler fix is to install into kelos-system. Suggestion: end the message with a pointer such as ; see "Release Namespace" in the chart README, which covers both cases.

• [P3] The README describes the guard as broader than it is - internal/manifests/charts/kelos/README.md:32-34
  "Helm installs and upgrades fail if ..." needs two qualifiers:

  1. Tools that render with helm template, such as Argo CD, never reach lookup, so they get no protection.
  2. Helm's lookup treats only NotFound as empty (pkg/engine/lookup_func.go:74-81). A Helm identity without get on customresourcedefinitions now fails every crds.install=false install or upgrade, including kelos-system releases.

  Suggestion: add one sentence that helm template-based tools skip the check, and one that crds.install=false releases need read access to customresourcedefinitions.

Naming / API surface

• [P3] --telemetry-namespace is unchanged since the last review and still well shaped - cmd/kelos-controller/main.go:136, internal/manifests/charts/kelos/templates/cronjob.yaml:34
  It is injected by its only caller and fails fast when missing. No change needed.

Suggestions (optional)

• None beyond the P3 wording fixes above. Once Remove v1alpha1 API #1725 removes conversion, drop the guard, the README conversion notes, and the "upgrade in one phase" carve-out together, so the chart doesn't keep a check that can no longer fire.

/kelos needs-input

[kelos-bot]

🤖 Kelos Reviewer Agent @gjkim42

Review Summary

Verdict: APPROVE
Overall correctness: patch is correct
Scope: Moves the controller manager, its webhook Service, Certificate, Issuer, leader-election RBAC, and the telemetry CronJob into the Helm release namespace. Templates the webhook and CRD conversion namespaces, renders Namespace/kelos-system only for kelos-system releases, adds --telemetry-namespace, and adds a crds.install=false conversion-namespace guard. Reviewed at 12871eb.

Findings Overview

Priority	Count	File:Line	Summary
P0	0	—	none
P1	0	—	none
P2	0	—	none
P3	0	—	none

Findings

Correctness / Upgrade compatibility

• [Resolved] The P1 from the 5bc664b review is fixed: existing non-kelos-system releases with outside-managed CRDs no longer lose CRD conversion. internal/manifests/charts/kelos/templates/webhook.yaml:8-14 now blocks any crds.install=false render whose live tasks.kelos.dev conversion Service is in a namespace other than the release namespace. I checked the cases below against the code:

  • Helm v3.20.1 lookup turns NotFound into an empty map, and engine.Render without a client provider never installs a live lookup. So helm template, helm lint, and kelos install / kelos uninstall (via helmchart.Render) are unaffected.
  • A CRD without a conversion webhook passes, so the guard becomes a no-op after Remove v1alpha1 API #1725.
  • A migrated release can still run phase 1 of a later two-phase upgrade.
  • TestRender_CRDConversionNamespaceGuard covers the rejection and each pass case through a fake lookup client.

  Other paths I checked, with no issues found:

  • kelos install still renders into kelos-system and applies the unchanged install-crd.yaml.
  • The generator runs escape_helm_template_placeholders before template_release_namespace, so the inserted {{ .Release.Namespace }} is not escaped. verify_kelos_conversion still checks the static kelos-system annotation in install-crd.yaml.
  • Leader election uses the in-cluster namespace, and no Go runtime code hardcodes kelos-system outside kelos install.
  • The telemetry ConfigMap RBAC is in a ClusterRole, so the CronJob can create it in any release namespace.
  • /readyz is healthz.Ping, so the one-phase crds.install=true migration does not deadlock on conversion. It only has the transient gap the README documents.

Suggestions (optional)

• [P3] (pre-existing, not counted) internal/manifests/charts/kelos/README.md:137-151: the Option 2 adoption loop omits taskpipelines.kelos.dev and webhookgateways.kelos.dev. Both ship in install-crd.yaml and render unconditionally in the crds subchart. The new migration section sends releases with outside-managed CRDs through this loop and then crds.install=true, so that upgrade fails Helm's ownership pre-check on those two CRDs. Nothing is applied, but the user still has to fix it by hand. The same gap already exists on main for plain Option 2 adoption. Adding both CRDs to the loop would make the documented migration work as written.

Key takeaways

• No blocking issues at 12871eb. Fresh installs, existing kelos-system releases, kelos install, and offline rendering all behave as before or as documented. TestRender_ReleaseNamespace checks both namespaces and asserts that no kelos-system reference remains outside a kelos-system release.
• The PR still needs a maintainer decision: by default, upgrading a non-kelos-system release deletes Namespace/kelos-system. The README and release note document the helm.sh/resource-policy=keep opt-out.

[gjkim42]

Addressed the Kelos reviewer and API reviewer findings (both at 5bc664b) in 9cbd4c1 (squashed).

[P1] Existing non-kelos-system releases lose CRD conversion when they upgrade before #1725 (both reviews). Valid: main has no storage-version migration, so older clusters can still hold objects stored as v1alpha1 that need conversion. Instead of holding the PR, I took the "if this PR must land first" path:

• Guard: with crds.install=false, templates/webhook.yaml looks up tasks.kelos.dev and fails if its conversion webhook names a namespace other than .Release.Namespace. Under helm template and kelos install, and when the CRD is absent, lookup returns nothing, so offline rendering is unaffected. A CRD with no conversion webhook also passes, so after Remove v1alpha1 API #1725 the guard becomes a no-op. The check is not a blanket crds.install=false && ns != kelos-system rule, so phase 1 of later two-phase upgrades still works once a release has migrated.
• README: "Release Namespace" documents the guard and adds a migration section for releases that own kelos-system from another namespace. It covers how to detect one, upgrading once with crds.install=true instead of the two-phase flow, labeling and annotating outside-managed CRDs first, and the transient conversion gap. The unfollowable "must be installed into kelos-system" note now applies only to Option 1. The release note matches.
• Tests: TestRender_CRDConversionNamespaceGuard renders through a fake lookup client. It covers the rejection, plus no live CRD, a CRD without conversion, matching namespaces, a kelos-system release, and crds.install=true, including crds.install=false outside kelos-system as suggested. With Helm v3.20 against envtest, phase 1 of a two-phase upgrade of an old-chart -n team-a release is refused with nothing applied, a single crds.install=true upgrade moves the CRDs to team-a, and a later phase 1 passes.

The maintainer can still choose to land this after #1725 instead. The guard and README notes are written so they can be removed then.

[P2]/[P3] kelos-system deletion on upgrade: add a one-line opt-out. Done. Helm reads the live object's helm.sh/resource-policy before deleting a resource that is no longer rendered. I confirmed this in pkg/kube/client.go in Helm v3.20.1 and with a real upgrade that kept the namespace. The README and release note now give kubectl annotate namespace kelos-system helm.sh/resource-policy=keep. Deleting the namespace by default still needs maintainer sign-off.

[P3] --telemetry-namespace shape: no change requested.

[gjkim42]

/kelos api-review

[gjkim42]

/kelos review

[cubic-dev-ai]

All reported issues were addressed across 4 files (changes from recent commits).

<sub>Tip: Review your code locally with the cubic CLI to iterate faster.

View guided diff | Turn on auto-fix | Re-trigger cubic</sub>

[gjkim42]

Follow-up on the Kelos reviewer and API reviewer findings at 12871eb, addressed in 9cbd4c1. The branch is now squashed into that single commit.

• [P3] The guard's error names a remedy that fails for CRDs from kelos install (API reviewer). Fixed. The message now ends with see "Release Namespace" in the chart README. That section covers both cases: adopting outside-managed CRDs before the crds.install=true upgrade, and Option 1's requirement that the release namespace be kelos-system. TestRender_CRDConversionNamespaceGuard now pins the full message.
• [P3] The README describes the guard as broader than it is (API reviewer). Fixed. I confirmed both limits in Helm v3.20.1 pkg/engine/lookup_func.go: only NotFound is treated as empty. "Release Namespace" now states that the check reads the CRD, so the Helm identity needs get on customresourcedefinitions, and that tools that render with helm template, such as Argo CD, skip it.
• [P3] The Option 2 adoption loop omits taskpipelines.kelos.dev and webhookgateways.kelos.dev (Kelos reviewer, also in cubic's thread). Fixed. The loop now lists all 11 CRDs the chart renders.
• [P2] Maintainer sign-off on deleting Namespace/kelos-system by default (API reviewer). Not something I can resolve. It still needs an explicit maintainer decision before merge. The helm.sh/resource-policy=keep opt-out is documented in the README and the release note.
• Optional: drop the guard and the README conversion notes after Remove v1alpha1 API #1725. Agreed, and already noted in the PR description. They are written to be removed together.

Also in this commit: the test-integration CI failure is fixed. The suite now renders the chart's ValidatingWebhookConfiguration for envtest instead of reading the templated file. make verify, make test, and make test-integration pass locally.