Skip to content

Commit b5cc2ae

Browse files
myasnikovdaniilclaude
myasnikovdaniil
and
claude
committed
fix(kubernetes): address review findings on Talos worker bootstrap
- talos-reconcile-job: emit a `---` separator inside the nodeGroups range so a tenant with multiple node groups renders valid multi-document YAML instead of two Jobs concatenated into one doc - talos-reconcile + bootstrap-token jobs: add a restricted securityContext (drop ALL caps, no privilege escalation, read-only root fs) and CPU/memory requests+limits; drop the unused management SA token mount on the bootstrap-token job, which only talks to the tenant apiserver via the mounted admin kubeconfig - migration 44: list Helm-managed resources by the app.kubernetes.io/managed-by=Helm label, not meta.helm.sh/release-name (an annotation, so it never matched as a --selector — the keep annotation was silently never applied) - cluster.yaml: fail fast on a malformed nodeHealthCheck.maxUnhealthy rather than silently coercing it to 0 via `| int` - config: make helm-install-disable-wait parsing genuinely case-insensitive (EqualFold + trim) to match its documented contract - capi-providers bootstrap/cpprovider Makefiles: guard the patches/*.diff loop against an empty glob Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]>
1 parent e566992 commit b5cc2ae

8 files changed

Lines changed: 62 additions & 11 deletions

File tree

‎packages/apps/kubernetes/templates/cluster.yaml‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -610,7 +610,17 @@ spec:
610610
rejects a quoted plain integer ("0"). Quote only when the operator
611611
supplied a percentage; otherwise render the value as a bare int so
612612
the admission webhook is happy with the chart-default "0". */}}
613-
maxUnhealthy: {{ if hasSuffix "%" (toString $.Values.nodeHealthCheck.maxUnhealthy) }}{{ $.Values.nodeHealthCheck.maxUnhealthy | quote }}{{ else }}{{ $.Values.nodeHealthCheck.maxUnhealthy | int }}{{ end }}
613+
{{- $maxUnhealthy := toString $.Values.nodeHealthCheck.maxUnhealthy }}
614+
{{- if hasSuffix "%" $maxUnhealthy }}
615+
{{- if not (regexMatch "^[0-9]+%$" $maxUnhealthy) }}
616+
{{- fail (printf "nodeHealthCheck.maxUnhealthy must be a bare integer (e.g. 0, 1) or an integer percentage (e.g. 50%%), got %q" $maxUnhealthy) }}
617+
{{- end }}
618+
maxUnhealthy: {{ $maxUnhealthy | quote }}
619+
{{- else if regexMatch "^[0-9]+$" $maxUnhealthy }}
620+
maxUnhealthy: {{ $maxUnhealthy | int }}
621+
{{- else }}
622+
{{- fail (printf "nodeHealthCheck.maxUnhealthy must be a bare integer (e.g. 0, 1) or an integer percentage (e.g. 50%%), got %q" $maxUnhealthy) }}
623+
{{- end }}
614624
nodeStartupTimeout: {{ $.Values.nodeHealthCheck.nodeStartupTimeout }}
615625
selector:
616626
matchLabels:

‎packages/apps/kubernetes/templates/talos/bootstrap-token-tenant-job.yaml‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,10 @@ spec:
5050
template:
5151
spec:
5252
restartPolicy: OnFailure
53+
# This Job only talks to the tenant apiserver via the mounted admin
54+
# kubeconfig (KUBECONFIG below); it never calls the management apiserver,
55+
# so the management ServiceAccount token is not needed. Drop it.
56+
automountServiceAccountToken: false
5357
serviceAccountName: {{ .Release.Name }}-bootstrap-token-tenant
5458
containers:
5559
- name: kubectl
@@ -71,6 +75,19 @@ spec:
7175
- name: tenant-admin
7276
mountPath: /tenant-admin
7377
readOnly: true
78+
resources:
79+
requests:
80+
cpu: 10m
81+
memory: 64Mi
82+
limits:
83+
cpu: 100m
84+
memory: 256Mi
85+
securityContext:
86+
allowPrivilegeEscalation: false
87+
capabilities:
88+
drop:
89+
- ALL
90+
readOnlyRootFilesystem: true
7491
command:
7592
- sh
7693
- -ec

‎packages/apps/kubernetes/templates/talos/talos-reconcile-job.yaml‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -257,6 +257,7 @@ subjects:
257257
{{- $kubeReservedCpu := $kubeletOverride.kubeReservedCpu | default (printf "%dm" $autoReservedMillicores) }}
258258
{{- $evictionHardMemory := $kubeletOverride.evictionHardMemory | default "7%" }}
259259
{{- $evictionSoftMemory := $kubeletOverride.evictionSoftMemory | default "10%" }}
260+
---
260261
apiVersion: batch/v1
261262
kind: Job
262263
metadata:
@@ -492,4 +493,10 @@ spec:
492493
limits:
493494
cpu: 100m
494495
memory: 256Mi
496+
securityContext:
497+
allowPrivilegeEscalation: false
498+
capabilities:
499+
drop:
500+
- ALL
501+
readOnlyRootFilesystem: true
495502
{{- end }}

‎packages/core/platform/images/migrations/migrations/45‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,11 @@ PATCHED=0
4242
SKIPPED=0
4343

4444
KEEP_ANNOTATION="helm.sh/resource-policy=keep"
45-
HELM_RELEASE_LABEL="meta.helm.sh/release-name"
45+
# Helm injects the app.kubernetes.io/managed-by=Helm LABEL into every resource it
46+
# renders (e.g. the chart's KubeadmConfigTemplate). meta.helm.sh/release-name is an
47+
# ANNOTATION, not a label, so it cannot be used as a --selector — doing so silently
48+
# matches nothing and the keep annotation is never applied.
49+
HELM_MANAGED_SELECTOR="app.kubernetes.io/managed-by=Helm"
4650

4751
annotate_object() {
4852
local kind="$1" ns="$2" name="$3"
@@ -76,7 +80,7 @@ annotate_object() {
7680
list_helm_managed() {
7781
local kind="$1" out
7882
if ! out=$(kubectl get "$kind" --all-namespaces \
79-
--selector "$HELM_RELEASE_LABEL" \
83+
--selector "$HELM_MANAGED_SELECTOR" \
8084
--output 'jsonpath={range .items[*]}{.metadata.namespace}{"\t"}{.metadata.name}{"\n"}{end}' \
8185
2>&1); then
8286
case "$out" in

‎packages/system/capi-providers-bootstrap/Makefile‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ update:
2929
curl -sSL "https://github.com/siderolabs/cluster-api-bootstrap-provider-talos/releases/download/$(CABPT_UPSTREAM_VERSION)/bootstrap-components.yaml" \
3030
-o .update-tmp/bootstrap-components.yaml
3131
for p in patches/*.diff; do \
32+
[ -e "$$p" ] || continue; \
3233
(cd .update-tmp && patch bootstrap-components.yaml < ../$$p) || exit 1; \
3334
done
3435
mv .update-tmp/bootstrap-components.yaml files/bootstrap-components-talos.yaml

‎packages/system/capi-providers-cpprovider/Makefile‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ update:
4444
| tar -xzf - --strip=1 -C .update-tmp
4545
cd .update-tmp && git init -q && git add . && git commit -qm init && \
4646
for p in ../images/cluster-api-control-plane-provider-kamaji/patches/*.diff; do \
47+
[ -e "$$p" ] || continue; \
4748
git apply --recount "$$p"; \
4849
done
4950
cd .update-tmp/config/manager && \

‎pkg/config/config.go‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ package config
1919
import (
2020
"fmt"
2121
"regexp"
22+
"strings"
2223
"time"
2324
)
2425

@@ -103,12 +104,13 @@ func ParsePositiveDuration(flagName, raw string) (time.Duration, error) {
103104
// callers can leave HelmInstallDisableWait zero and let flux defaults
104105
// apply.
105106
func ParseHelmInstallDisableWaitAnnotation(raw string) (bool, error) {
106-
switch raw {
107-
case "":
107+
raw = strings.TrimSpace(raw)
108+
switch {
109+
case raw == "":
108110
return false, nil
109-
case "true", "True", "TRUE":
111+
case strings.EqualFold(raw, "true"):
110112
return true, nil
111-
case "false", "False", "FALSE":
113+
case strings.EqualFold(raw, "false"):
112114
return false, nil
113115
default:
114116
return false, fmt.Errorf("must be \"true\" or \"false\", got %q", raw)

‎pkg/config/config_test.go‎

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -217,10 +217,19 @@ func TestParseHelmInstallDisableWaitAnnotation(t *testing.T) {
217217
want: false,
218218
},
219219
{
220-
name: "mixed-case rejected (Helm-style scrubbing not applied here)",
221-
input: "tRue",
222-
wantErr: true,
223-
errMatch: `must be "true" or "false"`,
220+
name: "mixed-case true accepted (case-insensitive)",
221+
input: "tRue",
222+
want: true,
223+
},
224+
{
225+
name: "mixed-case false accepted (case-insensitive)",
226+
input: "fAlSe",
227+
want: false,
228+
},
229+
{
230+
name: "surrounding whitespace trimmed",
231+
input: " true ",
232+
want: true,
224233
},
225234
{
226235
name: "integer rejected",

0 commit comments

Comments
 (0)