Repository navigation
fix(info): use root-host for Keycloak OIDC issuer URL in tenant kubeconfig - #2704
Conversation
…onfig The kubeconfig generated for non-root tenants pointed `--oidc-issuer-url` at `keycloak.<TENANT>.<ROOT-DOMAIN>` because `$host` was sourced from `_namespace.host` (tenant-specific subdomain). Keycloak's ingress and cert live at `keycloak.<ROOT-DOMAIN>` (built from `_cluster.root-host`), so `kubectl oidc-login` failed TLS verification against the nginx-ingress default fake cert. The existing lookup-based override that tried to read `tenant-root`'s `spec.values.host` no longer triggers since PR #1787 migrated tenant-root to `valuesFrom` (Secret-backed), leaving `spec.values` empty. Use `_cluster.root-host` directly, matching what the dashboard gatekeeper template already does for the same Keycloak realm. Assisted-By: Claude <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe kubeconfig Helm template is simplified to resolve the host exclusively from cluster-level configuration. The template removes fallback logic that previously defaulted to namespace values and eliminates a conditional lookup of Flux HelmRelease resources that could override the host. ChangesKubeconfig Host Resolution Simplification
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves an issue where auto-generated kubeconfigs for non-root tenants were failing TLS verification during OIDC login. By standardizing the OIDC issuer URL to use the cluster's root-host, the configuration now correctly points to the valid Keycloak ingress, and redundant, non-functional lookup logic has been cleaned up to simplify the template. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request simplifies the kubeconfig.yaml template by removing a FluxCD-specific lookup block and streamlining the $host variable assignment. The feedback suggests improving code clarity by moving the $host definition inside the OIDC conditional block and applying defensive programming practices to handle potentially missing keys in .Values._cluster.
| @@ -1,4 +1,4 @@ | |||
| {{- $host := .Values._namespace.host | default (index .Values._cluster "root-host") }} | |||
| {{- $host := index .Values._cluster "root-host" }} | |||
There was a problem hiding this comment.
The variable $host is now only used within the OIDC configuration block (line 38) and is no longer reassigned since the lookup block was removed. To improve code clarity and keep the scope tight, consider moving its definition inside the {{- if $oidcEnabled }} block, consistent with other OIDC-specific variables like $apiServerEndpoint and $k8sCa. Additionally, following the defensive programming guidelines in the style guide, it is safer to provide a default or check for the existence of _cluster (e.g., index (.Values._cluster | default dict) "root-host") to avoid template errors if the key is missing.
References
- Enforce defensive programming: ensure appropriate null/nil checks or other language-idiomatic guards exist before object property accesses. (link)
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
LGTM — surgical fix, +1/-7. The bug is real, the dead-code removal claim checks out, and the fix matches the established pattern elsewhere in the chart tree.
Verified:
- Bug is real: pre-PR,
$hostdefaulted to.Values._namespace.host(e.g.tenant1.example.org), so the rendered OIDC issuer URL washttps://keycloak.tenant1.example.org/realms/cozy. But Keycloak's ingress and TLS cert live atkeycloak.<root-host>(e.g.keycloak.example.org), sooidc-loginhit nginx-ingress's default fake cert and failed TLS verification on every non-root tenant. - Cross-reference accurate:
packages/system/dashboard/templates/gatekeeper.yamluses exactly this shape —{{- $host := index .Values._cluster "root-host" }}followed by--oidc-issuer-url=https://keycloak.{{ $host }}/realms/cozy. The info kubeconfig now matches the same realm in the same way, so a tenant user and the dashboard both authenticate against the same issuer URL. - Dead-code claim accurate:
tenant-root.yamlusesvaluesFrom: Secret name: cozystack-values(verified inpackages/system/cozystack-basics/templates/tenant-root.yaml), sotenantRoot.spec.valuesis the empty struct, theand $tenantRoot $tenantRoot.spec $tenantRoot.spec.values $tenantRoot.spec.values.hostchain always short-circuits, and the override has been a no-op since #1787 (merged 2026-01-05) migrated tenants to the Secret-backedvaluesFrommechanism. Removing the block matches the post-#1787 reality. - Codex finds no regression. CI is green (Build + E2E + pre-commit + Verify generated all pass).
Bot finding is cosmetic, not a blocker:
- Gemini (low): "
$hostis now only used inside the$oidcEnabledblock; consider moving it inside." Pure style nit, can be deferred — keeping the host at the top mirrors the dashboardgatekeeper.yamlshape, which is consistent.
Non-blocking follow-up (not gating this fix): packages/extra/info/ has no tests/ directory, so the contract "OIDC issuer URL renders from _cluster.root-host, not _namespace.host" isn't pinned. Neighbouring extra packages do ship helm-unittest (packages/extra/{ingress,etcd,gateway}/tests/). Adding a minimal info/tests/kubeconfig_test.yaml that asserts the rendered Secret contains https://keycloak.<root-host>/realms/cozy and explicitly not the namespace host would pin this regression vector for next time. Out of scope for a 1-line backport-tagged fix; worth a separate task once the info package gets its test scaffolding.
…#3527) ## What this PR does This adds `cmd/backport-audit`, a maintainer tool that checks, before a release or an rc is cut, whether everything labelled for backport actually landed on the release branch. It prints the URLs of the PRs that did not and exits non-zero, so the cut can gate on it. `docs/release.md` and `docs/agents/releasing.md` now send the patch-cut check through it. The docs it replaces inventoried backports with `gh pr list --search "is:merged label:kind/backport"`. That lists every PR ever labelled, not what reached the branch. Answering the real question by hand means reading the branch history per PR, and that is how backports go missing quietly. ```console $ go build ./cmd/backport-audit $ ./backport-audit release-1.4 === release-1.4 === 30 candidate PRs: MISSING=5 dropped=4 backported=21 MISSING -- labelled for this line, no trace of it here (5): #2704 #2704 fix(info): use root-host for Keycloak OIDC issuer URL in tenant kubeconfig label=kind/backport author=myasnikovdaniil merged=2026-05-26 -- no backport PR, nothing on branch ``` A `kind/backport` label does not name a branch. `backport.yaml` resolves the target when the PR merges, so the same label means `release-1.5` on a PR merged in June and `release-1.6` on one merged in August. The audit reproduces that rule instead of trusting the label text. The rule changed once, on 2026-08-03, when 53a7fc4 made cutting an rc freeze the line and 6ff5b98 pointed the bot at the branch list. Before that date a line exists from its first stable release, after it from the moment its `release-X.Y` branch is pushed. The audit estimates that moment from the earliest `vX.Y.0-rc.N` tagged commit. This is an estimate, because the commit timestamp is not the branch-push timestamp. The README has the full reasoning. Landing is checked per commit, because a commit is what a backport loses: the bot's conflict drafts stop at the first commit that does not apply. Every non-empty commit of the original needs its own evidence on the branch. That is reachability, an `-x` reference, or a commit with its subject among the branch commits that are not on main. Some but not all is PARTIAL. A multi-commit original with a merged backport PR and no commit evidence is UNVERIFIED. Both fail the gate. A maintainer can confirm a squashed backport with a comment on the merged backport PR that is exactly `backport-audit: complete`. Two more sections start from the backport PRs on the branch instead of from labels. DUPLICATE lists an original with more than one live backport PR and fails the gate. UNLABELLED lists backports of originals that are not labelled for this line. It is informational and does not change the exit code. Exit code 2 means the audit could not give an answer. That happens when a `gh` listing reaches its cap, since `gh` truncates silently, or when a git read that a verdict depends on fails. The label listings are checked against the lower of `--limit` and 1000, because GitHub search never returns more than 1000 results. ### Verification `cmd/backport-audit/main_test.go` covers candidate selection, line resolution and the evidence rules. `make unit-tests` runs it through a new `test-backport-audit` target, since `./cmd/...` is not part of `go-unit-tests`. `hack/release-freeze-contract.bats` pins the audit's label set equal to the one `backport.yaml` acts on, in both directions. ### Limits A MISSING verdict is a prompt to check, not proof of absence. A hand backport that was squashed, reworded and never linked to its original is invisible to every evidence layer. A label added after merge is read against the line that was current at merge time, while the bot targets the line current when the label is added. Such a backport shows as MISSING on the old line and under UNLABELLED on the new one. The audit reports that something is missing, never why. `docs/release.md` now notes that before #3155 the bot left only a "Backport failed" comment on a conflicting cherry-pick, which is the first thing to check behind a MISSING. `make generate` does not apply. This is a standalone `main` package with no API types, no CRDs and no package `values.yaml`. ### Screenshots Not a UI change. Sample CLI output is above. ### Downstream repositories I walked the trigger map in `docs/agents/contributing.md` against the diff file by file. The diff adds `cmd/backport-audit/{main.go,main_test.go,README.md}` and changes `Makefile`, `hack/release-freeze-contract.bats`, `docs/release.md`, `docs/agents/releasing.md` and one `.gitignore` line for the root binary. Nothing in it reaches a downstream repository. No package under `packages/apps/` or `packages/extra/` is added, renamed or removed, and no `values.schema.json`, `values.yaml` default or version enum changes, so the website generator lists and the terraform provider schemas are untouched. No `ApplicationDefinition` semantics, `release.prefix`, output Secret or Service name, namespace, platform variant or bundle changes. The paths that reach ccp and external-apps-example (`hack/package.mk`, `hack/common-envs.mk`, `hack/update-crd.sh`) are not touched. The Makefile change adds a `test-backport-audit` target to `unit-tests` and changes no existing target. No node prerequisite changes, so talm and the ansible playbooks need nothing. The website "developer tooling" trigger covers tools contributors use on packages. This is maintainer release tooling, and its process doc lives in this repository only. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note ```release-note feat(release): add `backport-audit`, a maintainer tool that reports which `kind/backport` / `kind/backport-previous` PRs have not landed on a release branch (missing outright, partially backported, waiting on an open backport PR, or deliberately dropped) and exits non-zero while anything is outstanding, so a patch cut can gate on it ```
What this PR does
The kubeconfig generated by the
infopackage for non-root tenants pointed--oidc-issuer-urlatkeycloak.<TENANT>.<ROOT-DOMAIN>, but Keycloak's ingress and cert live atkeycloak.<ROOT-DOMAIN>(built from_cluster.root-host).kubectl oidc-loginthen failed TLS verification against the nginx-ingress default fake cert, so the auto-generated kubeconfig was unusable for any non-root tenant.$hostwas being sourced from_namespace.host(tenant-specific subdomain) with a lookup-based override that tried to readtenant-root'sspec.values.host. That override has been dead since #1787 migratedtenant-roottovaluesFrom(Secret-backed) —spec.valuesis now always empty, so the override never fires.Source
$hostfrom_cluster.root-hostdirectly, matching whatpackages/system/dashboard/templates/gatekeeper.yamlalready does for the same Keycloak realm.Verified with
helm template tenant1 packages/extra/info -n tenant-tenant1 --set-string _namespace.host=tenant1.example.org --set-string _cluster.root-host=example.org ...— issuer URL is nowhttps://keycloak.example.org/realms/cozyregardless of the per-namespace host.Release note
Summary by CodeRabbit