Uh oh!
There was an error while loading. Please reload this page.
release-train: develop -> staging - #856
Merged
Merged
Conversation
…ace admin (backend#2542) (#853) INSTALL.md's "Upgrade and rollback" showed the helm commands and said nothing about the access they require. Namespace admin is not enough: the chart templates cluster-scoped objects (Namespace, PersistentVolume, StorageClass, PriorityClass, ClusterRole, ClusterRoleBinding, OpenShift SCC) and helm must READ each to diff a release, while Kubernetes' built-in `admin` ClusterRole has no cluster-scoped rules. So the operator fails on one object, then the next. Measured on a real fleet during the backend#947/#1528 root rotation: the metrics-server preflight opt-out shipped for backend#2469 cleared the first denial and the very next upgrade failed on priorityclasses. Three things the section now states, each of which cost time to learn: - Do NOT use the `create: false` gates to work around a permissions error. ClusterRole/ClusterRoleBinding have no gate, re-applying them needs escalate/bind regardless of read access, so flag-chasing cannot finish -- and every flag added persists into the stored release values, quietly becoming a standing config change. - On EKS, `--access-scope type=cluster` means "applies in all namespaces", NOT "grants cluster-scoped resources". AmazonEKSAdminPolicy at type=cluster reads as fully privileged and confers none of these objects; the denial text ("forbidden ... at the cluster scope") sounds like it contradicts the policy attached to you. Different axes. - Verify a temporary elevation was actually dropped: the revert names a policy, so a mismatch leaves it standing while looking reverted. Plus a note that the auto-upgrade CronJob succeeds where a human admin cannot (it holds a ClusterRole for exactly those kinds), that its --reset-then-reuse-values replays hand-passed `--set` values so a one-off does not stay one, and that it skips entirely at the latest version -- an hourly schedule is not an hourly helm upgrade. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…der (backend#2455) (#845) * fix(installer): escape inner quotes in Invoke-BoundedProcess arg builder (backend#2455) $psi.Arguments is one flat command line, so each arg has to survive CommandLineToArgvW re-splitting it back into argv. The old joiner wrapped whitespace-bearing args in quotes but never escaped an inner `"`, so any arg carrying BOTH a space and a quote (and even a quote with no space, which took the raw pass-through branch) reached the child with its quotes silently consumed and merged into adjacent tokens. #817 dodged this for one call site by never passing a quoted arg; this fixes the general helper. - Add ConvertTo-Win32Arg, which follows the exact CommandLineToArgvW/MSVCRT rules: escape `"` as \", double a run of backslashes before a quote (2N+1) and a trailing run before the close quote (2N), and leave a safe arg untouched. Invoke-BoundedProcess now delegates every arg to it. - Drop the fragile `^".*"$` "already-quoted, leave alone" escape hatch and its one dependent call site: Set-NodeGpuCapacity now passes $patchFile raw and lets the helper quote it (a spaced temp path was the only reason it self-quoted). Swept all ~27 call sites; the env-derived docker-login username is the other arg that can now carry a quote safely. - Replace the source-guard tests that pinned the buggy behavior with golden encodings plus a round-trip test (whitespace, embedded quote, whitespace + quote, empty string, backslashes-before-quote, trailing backslash) that re-splits via a from-spec CommandLineToArgvW parser and, on Windows, the real shell32 API — asserting each arg comes back as one original token. - Regenerate scripts/manifest.sha256 for the edited installer. Verified locally with pwsh 7.6.5 + Pester 6.1.0: install-k8s suite 765 passed / 0 failed / 14 skipped; installer-parity/install/telemetry 158/0; gen-manifest + check-drift + installer-parity bats green; manifest drift gate clean. Found by @saadqbal during client#817 review. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(installer): fix the inert shell32 cross-check -- flat string[] cases (backend#2455) The Windows-only "encoder agrees with real shell32!CommandLineToArgvW" test built its cases as @((,@(...)), ...) (comma-separated), which nests each case one level: $argv iterated as an Object[]-of-Object[], so ConvertTo-Win32Arg was handed an array and threw ParameterBindingArgumentTransformationException before any comparison ran. macOS skips the block, so a local "765 passed" hid it while windows-latest went red (939 passed / 1 failed) and this cross-check -- the one that breaks the circularity of the from-spec reimplementation -- never actually verified the encoder (LukasWodka on #845). Switch to the newline-separated ,@(...) shape the round-trip test already uses so each $argv is a flat [string[]]. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(installer): make the shell32 cross-check preserve empty args (backend#2455) With the case array fixed, the Windows-only real-shell32 oracle finally ran and caught a harness bug: realArgv returned ,@($r) and the caller piped it through | Select-Object -Skip 1, which dropped a trailing empty argument -- so the empty """" case saw 0 recovered tokens instead of 1 (windows-latest: 939 passed / 1 failed). Collect into a List[string] and return via the ,$arr idiom, then assign (not pipe) and slice off argv[0] by index, which preserves empty and 0/1-element results. Verified the array logic on the empty and mid-empty cases. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(installer): dump real shell32 output in the cross-check (backend#2455) The Windows-only real-shell32 oracle disagrees on the space+quote case in a way not reproducible on macOS (no shell32), and "got a" alone is not debuggable. Surface arg / encoded line / real shell32 tokens in the failure -Because and a Write-Host so the next windows-latest run shows exactly what CommandLineToArgvW returned. Diagnostic only -- no change to the encoder or the assertions. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(installer): drop argv[0] with an index loop, not a range slice (backend#2455) The real-shell32 diagnostic confirmed the encoder is correct: CommandLineToArgvW returns ["prog.exe","a b\"c"] exactly as intended (shell32=[prog.exe|a b\"c] n=2). The remaining failure was the harness -- $full[1..($full.Count-1)] collapses to a SCALAR string when it selects a single element under Windows PowerShell, so $got[$k] then indexed into that string chars ("got a" for "a b\"c"). Replace the range slice with an explicit index loop that keeps $got a real array on every host; verified it returns whole tokens (incl. the empty arg) for 1- and multi-arg cases. Keeps a concise -Because dumping the real shell32 output for future debug. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(installer): append arg chars as [string] for PS 5.1 (backend#2455) ConvertTo-Win32Arg appended $Arg[$i] (a [char] from string indexing) to the StringBuilder. Under Windows PowerShell 5.1 -- the host the installer relaunches into -- the Append overload binder can bind a [char] to a numeric overload and write the code point instead of the character, corrupting a quoted arg (e.g. a spaced --patch-file); pwsh-7 CI and the golden tests do not see it. Cast to [string] so Append(string) is selected unambiguously on every host. Output is byte-identical on pwsh 7 (golden vectors unchanged). (Bugbot High on #845.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(installer): source guard matches the call, not a comment (backend#2455) The Invoke-BoundedProcess source guard matched the bare name ConvertTo-Win32Arg, which a comment in the same function body also contains -- so deleting the actual call would still pass. Match the call expression (ForEach-Object { ConvertTo-Win32Arg $_) so the guard can detect its own removal. (Bugbot Medium on #845.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
LukasWodka
commented
Aug 26, 2026
ContributorAuthor
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit fff6871. Configure here.
saadqbal
removed their request for review
August 26, 2026 13:28
Uh oh!
There was an error while loading. Please reload this page.
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 freeto 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.
Automated promotion by the release train (RFC-0008 D14). Head is the train-managed
release-train/to-stagingbranch (a mirror ofdevelop), so it never collides with a human PR. Merged only when the fr-gate is green.Note
Medium Risk
Installer changes affect every bounded external process on Windows (kubectl, docker); incorrect quoting could break installs, though extensive round-trip tests reduce that risk. Doc-only RBAC guidance has no runtime impact.
Overview
Documentation: Adds an Upgrade and rollback subsection explaining that manual Helm upgrades need cluster-scoped RBAC, not namespace
adminalone — Helm must read cluster objects (PriorityClass, ClusterRole, etc.) to diff releases. It warns against usingcreate: falseflags to dodge permission errors, clarifies EKSAmazonEKSAdminPolicyvsAmazonEKSClusterAdminPolicy, and notes howautoUpgradepersists--setvalues via--reset-then-reuse-values.Windows installer: Introduces
ConvertTo-Win32ArgsoInvoke-BoundedProcessbuilds$psi.Argumentsusing CommandLineToArgvW rules (escaped quotes, trailing backslashes, empty args). This fixes args that mix spaces and quotes (e.g. docker--formatwith Go templates) that previously broke parsing (#817 / backend#2455).Set-NodeGpuCapacitynow passes the patch file path raw instead of pre-quoting.Tests: Replaces lightweight quoting guards with golden encoding, round-trip parsing, and a Windows-only
shell32!CommandLineToArgvWcross-check; related Pester jobs loadConvertTo-Win32Argin isolated runspaces.Reviewed by Cursor Bugbot for commit fff6871. Bugbot is set up for automated code reviews on this repo. Configure here.