Repository navigation
fix(kubernetes): render the OIDC UID header only from v1.33 - #4407
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: cozystack/cozystack/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (10)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe Kubernetes chart now renders the OIDC UID-header flag from effective version 1.33 and no longer adds its own feature-gate flag. It updates emulated-version parsing and render guards. OIDC tests and documentation reflect the revised behavior. ChangesKubernetes OIDC flag handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The chart’s v1.33 UID-header behavior and documentation are aligned. No actionable issue remains to block merge after normal checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Out of Scope Changes checkExplanation The whole-PR change summary includes changes unrelated to [
✨ 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 |
github-actions
Bot
added
size/XXL
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
LGTM with non-blocking notes
Reviewed 3b1427523 against merge-base 15cdc47fd. I found nothing blocking. The v1.32 render now matches every released tag, and the new patch-level refusal only rejects values the pinned kube-apiserver already refuses.
Caveats
- I diffed full
helm templateoutput of base against head withtests/values/{common,oidc-system}.yamlat v1.31–v1.35. With the random Kamaji tokens masked, the only change is the two UID lines at v1.32 with OIDC System, and CustomConfig gives the same result. None of v1.5.4, v1.6.3 or v1.7.0-alpha.1 renders--requestheader-uid-headers, so upgrading from a release changes nothing on v1.31 or v1.32 because of this PR. - The upstream rules check out at the pinned tags.
toVersionMaprefusesPatch() != 0at v1.31.14 (apiserver/pkg/util/version/registry.go:327) and at v1.33.13 (component-base/compatibility/registry.go:303). At v1.35.6RemoteRequestHeaderUIDis still 1.32 alpha and 1.33 beta, and its spec has noMinCompatibilityVersion, so--min-compatibility-versioncan't turn the gate off under the header flag. Kamaji 26.9.4-edge doesn't touch--emulated-version, so the value does reach the apiserver and the refusal never fires on an entry the control plane would drop. These edge inputs render the way upstreamversion.Parsereads them:1.32-rc,v1.33.0,kube=1.32=x,kube=1.34.0-rc.1,1.32.0.5,1.32.04, and a value padded with spaces. - Mixed kube gate spelling is now passed through. On the base branch,
kube:Foo=truenext toBar=trueon v1.33+ with OIDC on failed the render. On head it renders and the apiserver refuses it at startup (registry.go:363at v1.33.13), so the operator gets a crash-looping control plane instead of a render error. No release had that check, so this costs observability, not correctness. - Clusters built from
mainrunning v1.32 with OIDC lose the header flag and the gate term on the next reconcile, silently, and aggregated API servers stop getting the caller's UID. Expected, and the PR body says so. - Optional follow-up, not blocking: two
--emulated-versionshapes still render cleanly even though the apiserver refuses both at startup. One is a trailing comma (kube=1.33,), which handstoVersionMapan empty element forversion.Parse. The other is a zero-prefixed major (kube=01.33). The parent branch renders both the same way, so this PR doesn't change them. If the goal is a render error instead of a crash-loop, they fit next to the patch-level refusal. - This is stacked on #4398 and I didn't re-review that delta. It includes the header flag on v1.33+ OIDC tenants, which is new compared with v1.6.3 and changes
extraArgs, so an upgrade rolls those tenants' control planes. - Nothing ran against a live cluster. I didn't watch Kamaji's hash-based argument rebuild during the v1.32
extraArgschange. helm unittest: 239/239 pass. Reverting any of seven pieces of the new logic turns the OIDC suite red: the floor, the patch refusal, the first-=split, the quoted-value guard, the version parse, the explicit-true override ofAllBeta=false, and the empty component.
15cdc47 to
a23e843
Compare
3b14275 to
90b5c63
Compare
|
IvanHunters I rebased this onto the new head of #4398 ( |
57125eb to
e4c2dc8
Compare
90b5c63 to
90b05a1
Compare
|
IvanHunters #4398 moved again after its own review, so this is rebased onto |
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
LGTM with non-blocking notes
I rendered base and head across every version and --emulated-version / --feature-gates shape I could think of. Each difference is one of the three changes the PR intends, and the upstream claims in the description hold when checked against real kube-apiserver binaries.
Findings
- [MINOR]
packages/apps/kubernetes/templates/cluster.yaml:606, below the floor the chart now passes an operator's apiserver-fatal header entry through without a word
- [MINOR]
packages/apps/kubernetes/tests/oidc_test.yaml:2258, the parsing cases cannot tell "read as 1.32" from "misread"
Caveats
- v1.32 OIDC tenants built from main lose both UID lines on the next render, which rolls the control plane. No release carries them:
git tag --contains d870b392f, the commit that added them, prints nothing. - The mixed bare/
kube:gate spelling is no longer refused. The apiserver still refuses it (set kube feature gates ... do not mix use, confirmed on v1.35.6), so all that's lost is the earlier, clearer error. - The patch-level
--emulated-versionrefusal sits inside the OIDC block, so withspec.oidc.mode: Nonean entry such as--emulated-version=1.32.4still renders and crash-loops as before. The release note reads as if the refusal were unconditional; saying "with OIDC on" there would match the code. - The patch refusal never fires where the apiserver would have started: across the matrix head refuses only
1.32.4,1.32.4-rcand1.33.5, and v1.31.14 and v1.35.6 both refuse a non-zero patch intoVersionMap.
| {{- end }} | ||
| {{- $uidHeaderUnsupported = le $effectiveMinor 31 }} | ||
| {{- $uidGateRequired = eq $effectiveMinor 32 }} | ||
| {{- $uidHeaderUnsupported = lt $effectiveMinor 33 }} |
There was a problem hiding this comment.
[MINOR] below the floor the chart now passes an operator's apiserver-fatal header entry through without a word
With OIDC on and an effective version of 1.32, an operator's own --requestheader-uid-headers=X-Remote-Uid now passes through with no gate term, and kube-apiserver v1.32.13 refuses it (--requestheader-uid-headers requires the "RemoteRequestHeaderUID" feature to be enabled). At the merge-base the chart added --feature-gates=RemoteRequestHeaderUID=true for exactly this input. This only affects clusters built from main and matches released charts, so it is not a regression. From v1.33 the chart already refuses the equivalent header-plus-opt-out shape, and refusing a header entry below the floor when no term enables the gate would give v1.32 the same signal. The same skip also drops two render-time refusals the merge-base had at 1.32, a header entry beside --feature-gates=RemoteRequestHeaderUID=false and a header list ending in a comma; both now pass through to an apiserver that refuses them.
# in packages/apps/kubernetes
$ helm template k . --namespace tenant-root -f tests/values/common.yaml -f tests/values/oidc-system.yaml --set version=v1.32 --set-json 'controlPlane.apiServer.extraArgs=["--requestheader-uid-headers=X-Remote-Uid"]' | grep -A3 '^ extraArgs:'
extraArgs:
- --requestheader-uid-headers=X-Remote-Uid
- --authentication-config=/etc/kubernetes/authentication-config/config.yaml
extraVolumeMounts:
There was a problem hiding this comment.
I'm leaving this one out. Below the floor the chart reads none of the UID flags, the same as on v1.31 and in every released chart. Refusing a header entry on v1.32 would bring back the version-specific gate reasoning this PR removes, and it would apply to only one minor. The PR body lists it as a named item, together with the two refusals you point out that the branch history had at 1.32.
| # strconv.ParseBool rejects tRuE, so the apiserver never enables the gate on | ||
| # it. Folding case before the match would read it as ownership and withhold | ||
| # the chart's own term. | ||
| - it: reads a suffixed --emulated-version as the minor it names |
There was a problem hiding this comment.
[MINOR] the parsing cases cannot tell "read as 1.32" from "misread"
Every new parsing case emulates 1.32, below the floor, so a correct minor and a garbage minor (atoi gives 0) render identically. If the minor is taken without the $versionPattern extraction, only "ignores an --emulated-version whose minor is not a number" goes red:
# in packages/apps/kubernetes
$ sed -i '' 's/splitList "." (regexReplaceAll $versionPattern $emulatedKubeVersion "${1}")/splitList "." $emulatedKubeVersion/' templates/cluster.yaml
$ helm unittest . -f tests/oidc_test.yaml -f tests/oidc_withhold_test.yaml
Tests: 1 failed, 124 passed, 125 total
$ git checkout -- templates/cluster.yaml
Add above-floor positive cases: on v1.35, emulate 1.33-rc, v1.33, 1.33.0 and kube=1.33=x, and assert --requestheader-uid-headers=X-Remote-Uid is rendered.
There was a problem hiding this comment.
Every parsing case sat below the floor, where a minor of 32 and a minor of 0 render the same. I added four cases on v1.35 that emulate 1.33-rc, v1.33, 1.33.0 and kube=1.33=x and assert that --requestheader-uid-headers=X-Remote-Uid renders. Your mutation now turns 1.33-rc and kube=1.33=x red, next to the 1.garbage case. v1.33 and 1.33.0 stay green under it, because splitList "." puts 33 at index 1 for both with or without the pattern. Taking the minor from the major turns all four red, and taking it from the last component turns 1.33.0 red.
e4c2dc8 to
fb2e57c
Compare
90b05a1 to
823762b
Compare
|
IvanHunters Rebased onto the current #4398 head ( |
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
LGTM with non-blocking notes
I found nothing blocking at 823762bf against merge-base fb2e57c3. Since my last review your own change moved only in the tests and the guide (plus the regenerated cozyrds schema); templates/cluster.yaml contributes the same diff it did at 90b05a10, and the rest of the movement comes from the rebase onto the new #4398 head. Default renders still change only for OIDC System at v1.32, and the patch-level refusal only rejects values the pinned kube-apiserver already refuses.
From my earlier round
- The parsing cases now tell a correct minor from a misread one. The four above-floor cases at
tests/oidc_test.yaml:2281-2346go red under each of the three ways of misreading the minor: dropping the$versionPatternextraction reddens1.33-rcandkube=1.33=x, reading the major reddens all four, and reading the last component reddens1.33.0. Closed. - The v1.32 pass-through of an operator's own header entry stays, as you decided. I rendered that input on
v1.6.3and the head gives the sameextraArgs, so no released chart loses anything. I'm not holding it.
Caveats
- All of this assumes no release is cut from
mainfirst.mainadds--feature-gates=RemoteRequestHeaderUID=truebeside a bare header entry at v1.32, and neitherv1.6.3norv1.7.0-alpha.1does. If a release goes out frommainbefore this merges, v1.32 tenants on it lose that term on upgrade, and an operator who wrote only--requestheader-uid-headers=X-Remote-Uidreaches an apiserver that refuses it, with no render error. Merging before the next cut frommainavoids that. - Dropping the mixed gate-spelling refusal gives up a render-time error. That configuration is fatal to the apiserver, so it now shows up as a crash-looping control plane rather than as a failed render. The PR says so and the README note covers it.
- I didn't run the live rollout. On
main-built v1.32 OIDC tenantsextraArgsloses two entries, so the control plane rolls. A render cannot show that the rollout converges. - What I ran rather than read: kube-apiserver
version.Parseat v1.35.6 over 24--emulated-versionvalues against the chart's reading of each, and the gate's stage in--helpat every pinned version (Alpha at v1.32, Beta from v1.33). Reverting the floor, the patch refusal, the first-=split or the minor extraction each turnstests/oidc_test.yamlred.
Recommended follow-ups
- Upgrade a v1.32 OIDC tenant built from
mainto this chart on a test cluster and confirm the control plane rolls.
fb2e57c to
7d7f363
Compare
RemoteRequestHeaderUID is alpha and off on v1.32, so the chart had to enable an alpha gate on v1.32 tenants with OIDC on to make --requestheader-uid-headers start. From v1.33 the gate is beta and on by default. No release has shipped the v1.32 path, so moving the floor to an effective v1.33 now removes a behaviour no released chart had: the chart renders no --feature-gates term of its own, and below the floor it renders no UID flag and runs none of the UID checks, as it already did on v1.31. The refusal of a list that ends in a bare flag still applies there, because it guards the --authentication-config the chart appends on every version. The refusal of a kube gate spelling mixed between bare and kube:-prefixed terms goes too. Once the chart writes no gate term, the spelling of the operator's terms no longer concerns anything it renders, and the chart already passes other gate values the apiserver refuses, unknown names and unparseable values, through to it. --emulated-version is still read, because emulation moves the effective version below the floor. Its kube version is now read from the numbers at the start of the value, with the element split at its first equals sign as toVersionMap splits it, so v1.32, 1.32.0 and 1.32-rc all read as 1.32. With OIDC on, an --emulated-version entry without quotes whose kube version has a non-zero patch component fails the render and names the entry: the apiserver refuses such a value and would otherwise only crash-loop. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>
The tenant OIDC guide, the version field description and the e2e helper comment still described a --feature-gates term rendered on v1.32. Left as they were, they would tell an operator that the chart switches that alpha gate on, which it no longer does. The guide now says what a v1.32 tenant has to write itself to get the UID, and which --emulated-version values fail the render, so an operator who meets that refusal finds it explained. The breaking-changes note in the README said the chart refuses the mixed gate spelling while OIDC is on; with that refusal gone it would promise a render error that no longer comes. The version description returns to its earlier wording in values.yaml and in the update-versions.sh template, because that template rewrites the line on every version bump and would otherwise bring the old text back. make generate carries it into the schema, README, API types and ApplicationDefinition. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>
823762b to
a92e89e
Compare
The base branch was changed.
2 participants
What this PR does
Stacked on #4398; review the diff against
fix/kamaji-pin-hash-reset.The tenant Kubernetes chart now renders
--requestheader-uid-headers=X-Remote-Uidonly from an effective Kubernetes version of v1.33, whereRemoteRequestHeaderUIDis beta and on by default. It no longer renders a--feature-gatesterm of its own on any version. On v1.32 it used to add a term enabling that gate, which is alpha there, whenever it could read the operator's entries and they left the gate unset; no release tag contains that path, so dropping it now changes nothing a user has run.The chart still reads
--emulated-version, because emulation moves the effective version:--emulated-version=1.32on a v1.35 tenant puts the gate back to alpha-and-off, and the header flag is not rendered there. It still reads the operator's--feature-gatesentries for an opt-out (RemoteRequestHeaderUID=false, orAllBeta=falsewith no explicit=true), so the header flag never lands next to a disabled gate.AllAlphais no longer read, since it says nothing about a beta gate.An unquoted
--emulated-version=valueentry whose kube version has a non-zero patch component now fails the render and names the entry. The rule mirrors upstream rather than the regex in #4396:toVersionMapparses each value withversion.Parseand refuses onlyver.Patch() != 0(staging/src/k8s.io/component-base/compatibility/registry.golines 320-326 at v1.35.0, the same code instaging/src/k8s.io/apiserver/pkg/util/version/registry.goat v1.31.0).version.Parsetakes^\s*v?([0-9]+(?:\.[0-9]+)*)(.*)*$(versionMatchREink8s.io/apimachinery/pkg/util/version/version.go, identical at v0.31.1 and v0.35.0). So1.32,v1.32,1.32-rc,1.32.0and1.32.0-rcall start an apiserver emulating 1.32, and1.32.4and1.32.4-rcdo not. A^[0-9]+\.[0-9]+$check would have refused the first four. The chart extracts the numeric run with the same pattern, reads the minor from it, and fails only on a non-zero third component. A quoted value is still left unread.Two deliberate narrowings:
kube:-prefixed--feature-gatesterms. Once the chart writes no gate term, the spelling of the operator's terms no longer concerns anything it renders. The apiserver still refuses the mix (component-base/compatibility/registry.go:405at v1.35.0), and the chart now passes it through like the other fatal gate values it already leaves to the apiserver: unknown names, unparseable values, quoted terms.--requestheader-uid-headersentry without the gate reaches an apiserver that refuses it, which is how every released chart version behaved. A cluster built frommainthat already runs a v1.32 tenant with OIDC on loses the chart's--requestheader-uid-headersand gate term on the next render; no release carried them. That holds only while no release is cut frommainbefore this merges; a release that carries the v1.32 gate term first would turn this into a breaking change for v1.32 tenants, whose migration is to write both flags intoextraArgsthemselves.An element such as
kube=1.32=xis now split at its first=only, the waytoVersionMapsplits it withSplitN(compVer, "=", 2). The chart used to skip that element, which the apiserver reads as 1.32, and so rendered the header flag against an emulated 1.32. That gap predates this branch.Rebased onto #4398 at
fb2e57c3b, which reads a two-element--feature-gatesand--emulated-versionseparately. That behaviour carries over unchanged under the v1.33 floor: behind a hidden--emulated-versiononly an explicitRemoteRequestHeaderUID=falseis refused beside a header entry, while a blanketAllBeta=falsewaits for a reading, because a lower emulated version can make the gate alpha again; nothing is refused as an opt-out behind a hidden--feature-gatesvalue; the empty-element refusal applies behind both; and behind either oneX-Remote-Uidis added only beside a uid-headers list of the operator's. The README breaking-changes note from #4398 now says the chart refuses neither the mixed gate spelling nor two kube emulated versions. Indocs/oidc-tenant.mdthe paragraphs that list refusals now name the managed-flag move and refusal from #4398 and no longer carry a count, and the two-element paragraph now says what the chart renders behind a hidden value.Tests:
helm unittestgoes from 308 on #4398 to 298. 26 cases are deleted. 25 of them pinned the v1.32 gate term,AllAlphahandling or the mixed-spelling refusal. One emulated a version below 1.32, which the below-floor emulation case now covers. v1.32 cases that tested parsing rather than the gate are retargeted to v1.34. Sixteen cases are added: the patch-level refusal, the version parsing below the floor and above it (on v1.35 emulating1.33-rc,v1.33,1.33.0andkube=1.33=x, where only a correct minor renders the header), the first-=split, a pin that a mixed gate spelling now passes through, emulation landing exactly on 1.33, and a v1.32 opt-out beside a header entry passing through untouched.Coverage notes, not applied: no case pins that the patch-level refusal stays off with
spec.oidc.mode: None, where the block that holds it does not run. The case "splits a component element at its first equals sign only" pins that the kube element ofkube=1.32=xis read. It does not pin the split itself, because the version pattern ignores what follows the numbers either way.The
extraArgsfield description still calls the field the escape hatch for apiserver flags unrelated to OIDC. It is unchanged here, and it stays accurate for the v1.32 advice in the guide, because the UID flags belong to the aggregation layer rather than to OIDC.Optional, not applied: on v1.32 the chart could refuse an operator's
--requestheader-uid-headersentry that comes without the gate, since the gate is off by default there and the apiserver will not start. The same skip also drops two refusals the branch history had at 1.32, a header entry besideRemoteRequestHeaderUID=falseand a header list ending in a comma; both now reach an apiserver that refuses them. It stays out so that v1.32 matches v1.31 and the released charts, where the chart judges none of these flags.Not applied: for a value with a zero-prefixed major such as
01.32.4, the patch refusal suggests01.32. The apiserver refuses both values over the leading zero, so the configuration fails either way and only the suggested fix is off.Why the patch refusal stays while the mixed-spelling refusal goes: the patch refusal is about
--emulated-version, which the chart reads itself to decide what to render. The mixed spelling concerns--feature-gates, which the chart no longer writes to.Not split out: the
--emulated-versionparsing and the patch-level refusal share the fix commit with the floor move, because both change how the same value is read. The change also removes a comment fromhack/e2e-chainsaw/_lib/run-kubernetes.shthat became false, so select-e2e runs the full suite for this PR.Default path: renders of the parent and this branch through
tests/values/common.yamlandtests/values/oidc-system.yamlat v1.31 to v1.35 are byte-identical, once the randomly generated Kamaji token fields are masked, except for OIDC System at v1.32, which loses exactly the two UID lines.Screenshots
Not a UI change.
Downstream repositories
The only field that changes is the description of
version, which returns to its earlier wording. The website's kubernetes reference page is regenerated from the README on the next tag, and the Terraform provider keeps its own copy of that description, which already reads "Kubernetes major.minor version to deploy.".Closes #4373
Closes #4396
Release note
Summary by CodeRabbit