From d986a143268fb85b225e821c044bd3f52504af0e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Almeida?= <57680069+zeval@users.noreply.github.com> Date: Tue, 15 Sep 2026 16:16:08 +0200 Subject: [PATCH] fix(kubernetes): make retained PVC preparation robust (#3697) --- .../runtime/lifecycle/default_scripts.go | 56 ++++++- .../default_scripts_kubernetes_test.go | 7 +- .../executor_kubernetes_bootstrap.go | 2 + .../lifecycle/full_worker_prepare_test.go | 1 + .../lifecycle/kubernetes_prepare_copy_test.go | 147 ++++++++++++++++++ .../task-03-docker-lifecycle.md | 14 ++ k8s/worker-images/full/prepare.sh | 13 +- 7 files changed, 233 insertions(+), 7 deletions(-) create mode 100644 apps/backend/internal/agent/runtime/lifecycle/kubernetes_prepare_copy_test.go diff --git a/apps/backend/internal/agent/runtime/lifecycle/default_scripts.go b/apps/backend/internal/agent/runtime/lifecycle/default_scripts.go index 5d3beeffd1..60f2299488 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/default_scripts.go +++ b/apps/backend/internal/agent/runtime/lifecycle/default_scripts.go @@ -3,6 +3,7 @@ package lifecycle import ( "crypto/sha256" "encoding/hex" + "strings" ) const executorTypeSSH = "ssh" @@ -12,6 +13,13 @@ const executorTypeSSH = "ssh" // user-authored prepare scripts must remain unchanged. const legacySpritesPrepareScriptSHA256 = "e656f9e51496e1bb1e5cee205058bbeaa7fe1ab37073f600e6c94310ead7be4d" +// These hashes identify scripts supplied by Kandev. Only exact matches receive +// managed fixes; customized scripts remain byte-for-byte unchanged. +const ( + legacyKubernetesDefaultPrepareScriptSHA256 = "5fe05aedccc24565f338fc6d03f6814024aaf15655643b9ab6317986dfac0ce2" + legacyKubernetesFullPrepareScriptSHA256 = "540eece886af9577c6e9e652a845e373692c222453d62144cd6f03a4ab8bd734" +) + // DefaultPrepareScript returns the default prepare script for a given executor type string. func DefaultPrepareScript(executorType string) string { switch executorType { @@ -37,6 +45,41 @@ func isLegacySpritesPrepareScript(script string) bool { return hex.EncodeToString(digest[:]) == legacySpritesPrepareScriptSHA256 } +func upgradeLegacyKubernetesPrepareScript(script string) string { + digest := sha256.Sum256([]byte(script)) + switch hex.EncodeToString(digest[:]) { + case legacyKubernetesDefaultPrepareScriptSHA256: + return defaultKubernetesPrepareScript + case legacyKubernetesFullPrepareScriptSHA256: + upgraded := strings.Replace(script, + "clone_tmp=/opt/kandev/.workspace-clone\n", + "clone_tmp=/opt/kandev/.workspace-clone\n\n"+kubernetesRepositoryOriginNormalizer, + 1, + ) + upgraded = strings.Replace(upgraded, legacyKubernetesRepositoryOriginComparison, + currentKubernetesRepositoryOriginComparison, 1) + return strings.Replace(upgraded, + `cp -a "$clone_tmp"/. "$workspace"/`, + `cp -R "$clone_tmp"/. "$workspace"/`, 1) + default: + return script + } +} + +const kubernetesRepositoryOriginNormalizer = `normalize_repository_origin() { + printf '%s\n' "$1" | sed \ + -e 's|^https://[^/@]*@github.com/|https://github.com/|' \ + -e 's|^git@github.com:|https://github.com/|' \ + -e 's|^ssh://git@github.com/|https://github.com/|' +} +` + +const legacyKubernetesRepositoryOriginComparison = ` expected_origin=$(printf '%s\n' "$repository_url" | sed 's|^https://[^/@]*@github.com/|https://github.com/|') + retained_origin=$(printf '%s\n' "$workspace_origin" | sed 's|^https://[^/@]*@github.com/|https://github.com/|')` + +const currentKubernetesRepositoryOriginComparison = ` expected_origin=$(normalize_repository_origin "$repository_url") + retained_origin=$(normalize_repository_origin "$workspace_origin")` + // KandevBranchCheckoutPostlude returns a kandev-managed shell snippet that // guarantees the session's feature branch is checked out inside the // workspace, no matter what the user's stored prepare_script does. @@ -166,6 +209,13 @@ repository_url={{repository.clone_url}} repository_branch={{repository.branch}} clone_tmp=/opt/kandev/.workspace-clone +normalize_repository_origin() { + printf '%s\n' "$1" | sed \ + -e 's|^https://[^/@]*@github.com/|https://github.com/|' \ + -e 's|^git@github.com:|https://github.com/|' \ + -e 's|^ssh://git@github.com/|https://github.com/|' +} + # ---- Git identity and HTTPS authentication ---- {{git.identity_setup}} git config --global --add safe.directory '*' @@ -182,8 +232,8 @@ if [ -n "$repository_url" ]; then exit 1 fi workspace_origin=$(git -C "$workspace" remote get-url origin 2>/dev/null || true) - expected_origin=$(printf '%s\n' "$repository_url" | sed 's|^https://[^/@]*@github.com/|https://github.com/|') - retained_origin=$(printf '%s\n' "$workspace_origin" | sed 's|^https://[^/@]*@github.com/|https://github.com/|') + expected_origin=$(normalize_repository_origin "$repository_url") + retained_origin=$(normalize_repository_origin "$workspace_origin") if [ -z "$workspace_origin" ] || [ "$retained_origin" != "$expected_origin" ]; then echo 'kandev: retained workspace repository origin does not match the configured repository' >&2 exit 1 @@ -198,7 +248,7 @@ if [ -n "$repository_url" ]; then rm -rf "$clone_tmp" trap 'rm -rf "$clone_tmp"' 0 1 2 15 git clone --depth=1 --branch "$repository_branch" "$repository_url" "$clone_tmp" - cp -a "$clone_tmp"/. "$workspace"/ + cp -R "$clone_tmp"/. "$workspace"/ rm -rf "$clone_tmp" trap - 0 1 2 15 fi diff --git a/apps/backend/internal/agent/runtime/lifecycle/default_scripts_kubernetes_test.go b/apps/backend/internal/agent/runtime/lifecycle/default_scripts_kubernetes_test.go index 52b829c73f..06c3dae46e 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/default_scripts_kubernetes_test.go +++ b/apps/backend/internal/agent/runtime/lifecycle/default_scripts_kubernetes_test.go @@ -40,6 +40,11 @@ func TestDefaultPrepareScriptKubernetesReusesRetainedPVCWorkspace(t *testing.T) if err := os.MkdirAll(home, 0o700); err != nil { t.Fatal(err) } + bin := filepath.Join(root, "bin") + if err := os.MkdirAll(bin, 0o755); err != nil { + t.Fatal(err) + } + installMetadataRestrictedCopy(t, bin) cloneTmp := filepath.Join(root, "runtime", "workspace-clone") if err := os.MkdirAll(filepath.Dir(cloneTmp), 0o700); err != nil { t.Fatal(err) @@ -58,7 +63,7 @@ func TestDefaultPrepareScriptKubernetesReusesRetainedPVCWorkspace(t *testing.T) run := func() { t.Helper() cmd := exec.Command("sh", "-eu", "-c", script) - cmd.Env = append(os.Environ(), "HOME="+home) + cmd.Env = append(os.Environ(), "HOME="+home, "PATH="+bin+":"+os.Getenv("PATH")) if output, err := cmd.CombinedOutput(); err != nil { t.Fatalf("Kubernetes prepare failed: %v\n%s", err, output) } diff --git a/apps/backend/internal/agent/runtime/lifecycle/executor_kubernetes_bootstrap.go b/apps/backend/internal/agent/runtime/lifecycle/executor_kubernetes_bootstrap.go index fa308f6465..8f5526e1cb 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/executor_kubernetes_bootstrap.go +++ b/apps/backend/internal/agent/runtime/lifecycle/executor_kubernetes_bootstrap.go @@ -304,6 +304,8 @@ func kubernetesPrepareScript(req *ExecutorCreateRequest) (string, error) { script := getMetadataString(req.Metadata, MetadataKeySetupScript) if script == "" { script = DefaultPrepareScript("k8s") + } else { + script = upgradeLegacyKubernetesPrepareScript(script) } if script == "" { return ":\n", nil diff --git a/apps/backend/internal/agent/runtime/lifecycle/full_worker_prepare_test.go b/apps/backend/internal/agent/runtime/lifecycle/full_worker_prepare_test.go index 9ca26364bd..a2411ba3dd 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/full_worker_prepare_test.go +++ b/apps/backend/internal/agent/runtime/lifecycle/full_worker_prepare_test.go @@ -58,6 +58,7 @@ func TestFullWorkerPreparationClonesBeforeCachesAndRetainsWorkspace(t *testing.T if err := os.MkdirAll(bin, 0755); err != nil { t.Fatal(err) } + installMetadataRestrictedCopy(t, bin) if err := os.WriteFile(filepath.Join(bin, "docker"), []byte("#!/bin/sh\nexit 0\n"), 0755); err != nil { t.Fatal(err) } diff --git a/apps/backend/internal/agent/runtime/lifecycle/kubernetes_prepare_copy_test.go b/apps/backend/internal/agent/runtime/lifecycle/kubernetes_prepare_copy_test.go new file mode 100644 index 0000000000..72781db7d7 --- /dev/null +++ b/apps/backend/internal/agent/runtime/lifecycle/kubernetes_prepare_copy_test.go @@ -0,0 +1,147 @@ +package lifecycle + +import ( + "os" + "os/exec" + "path/filepath" + "strings" + "testing" +) + +func installMetadataRestrictedCopy(t *testing.T, bin string) { + t.Helper() + realCopy, err := exec.LookPath("cp") + if err != nil { + t.Fatal(err) + } + wrapper := `#!/bin/sh +for arg in "$@"; do + case "$arg" in + -a|-p|--archive|--preserve|--preserve=*) + echo 'cp: preserving metadata: Operation not permitted' >&2 + exit 1 + ;; + esac +done +exec ` + shellQuote(realCopy) + ` "$@" +` + if err := os.WriteFile(filepath.Join(bin, "cp"), []byte(wrapper), 0o755); err != nil { + t.Fatal(err) + } +} + +func TestKubernetesPreparationAcceptsEquivalentGitHubOrigins(t *testing.T) { + full, err := os.ReadFile("../../../../../../k8s/worker-images/full/prepare.sh") + if err != nil { + t.Fatal(err) + } + for name, script := range map[string]string{ + "default": DefaultPrepareScript("k8s"), + "full": string(full), + } { + t.Run(name, func(t *testing.T) { + runKubernetesPreparationWithEquivalentGitHubOrigins(t, script, name == "full") + }) + } +} + +func TestKubernetesPrepareScriptUpgradesPersistedManagedScripts(t *testing.T) { + full, err := os.ReadFile("../../../../../../k8s/worker-images/full/prepare.sh") + if err != nil { + t.Fatal(err) + } + legacyDefault := persistedKubernetesPrepareScriptBeforePVCFix(t, DefaultPrepareScript("k8s")) + for name, current := range map[string]string{ + "default": DefaultPrepareScript("k8s"), + "full": string(full), + } { + t.Run(name, func(t *testing.T) { + legacy := persistedKubernetesPrepareScriptBeforePVCFix(t, current) + if upgraded := upgradeLegacyKubernetesPrepareScript(legacy); upgraded != current { + t.Fatalf("managed script upgrade did not produce the current script") + } + req := validKubernetesCreateRequest() + req.Metadata[MetadataKeySetupScript] = legacy + + resolved, err := kubernetesPrepareScript(req) + + requireNoError(t, err) + if !strings.Contains(resolved, "normalize_repository_origin()") || + !strings.Contains(resolved, `cp -R "$clone_tmp"/. "$workspace"/`) { + t.Fatalf("persisted managed script was not upgraded:\n%s", resolved) + } + if strings.Contains(resolved, `cp -a "$clone_tmp"/. "$workspace"/`) { + t.Fatalf("persisted managed script retained archive copy:\n%s", resolved) + } + }) + } + + const customization = "# user-managed customization" + req := validKubernetesCreateRequest() + req.Metadata[MetadataKeySetupScript] = legacyDefault + "\n" + customization + resolved, err := kubernetesPrepareScript(req) + requireNoError(t, err) + if !strings.Contains(resolved, customization) || + !strings.Contains(resolved, `cp -a "$clone_tmp"/. "$workspace"/`) || + strings.Contains(resolved, "normalize_repository_origin()") { + t.Fatalf("custom prepare script was modified:\n%s", resolved) + } +} + +func persistedKubernetesPrepareScriptBeforePVCFix(t *testing.T, current string) string { + t.Helper() + legacy := strings.Replace(current, ` +normalize_repository_origin() { + printf '%s\n' "$1" | sed \ + -e 's|^https://[^/@]*@github.com/|https://github.com/|' \ + -e 's|^git@github.com:|https://github.com/|' \ + -e 's|^ssh://git@github.com/|https://github.com/|' +} +`, "", 1) + legacy = strings.Replace(legacy, ` expected_origin=$(normalize_repository_origin "$repository_url") + retained_origin=$(normalize_repository_origin "$workspace_origin")`, ` expected_origin=$(printf '%s\n' "$repository_url" | sed 's|^https://[^/@]*@github.com/|https://github.com/|') + retained_origin=$(printf '%s\n' "$workspace_origin" | sed 's|^https://[^/@]*@github.com/|https://github.com/|')`, 1) + legacy = strings.Replace(legacy, + `cp -R "$clone_tmp"/. "$workspace"/`, + `cp -a "$clone_tmp"/. "$workspace"/`, 1) + if legacy == current { + t.Fatal("legacy fixture did not reverse the managed PVC preparation changes") + } + return legacy +} + +func runKubernetesPreparationWithEquivalentGitHubOrigins(t *testing.T, script string, needsDocker bool) { + t.Helper() + workspace, _ := setupPostludeRepo(t, "main") + runIn(t, workspace, "git", "remote", "set-url", "origin", "https://github.com/acme/repo.git") + marker := filepath.Join(workspace, "retained.txt") + requireNoError(t, os.WriteFile(marker, []byte("preserve me"), 0o600)) + root := t.TempDir() + home := filepath.Join(root, "home") + bin := filepath.Join(root, "bin") + requireNoError(t, os.MkdirAll(home, 0o700)) + requireNoError(t, os.MkdirAll(bin, 0o755)) + if needsDocker { + requireNoError(t, os.WriteFile(filepath.Join(bin, "docker"), []byte("#!/bin/sh\nexit 0\n"), 0o755)) + } + + rendered := strings.NewReplacer( + "{{git.identity_setup}}", ":", + "{{github.auth_setup}}", ":", + "{{repository.branch}}", shellQuote("main"), + "{{repository.clone_url}}", shellQuote("git@github.com:acme/repo.git"), + "{{workspace.path}}", shellQuote(workspace), + "{{repository.setup_script}}", ":", + "{{kandev.agents.install}}", ":", + "/opt/kandev/.workspace-clone", filepath.Join(root, "runtime", "workspace-clone"), + ).Replace(script) + cmd := exec.Command("sh", "-eu", "-c", rendered) + cmd.Env = append(os.Environ(), "HOME="+home, "PATH="+bin+":"+os.Getenv("PATH")) + if output, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("Kubernetes prepare rejected equivalent GitHub origins: %v\n%s", err, output) + } + data, err := os.ReadFile(marker) + if err != nil || string(data) != "preserve me" { + t.Fatalf("retained workspace data = %q, %v", data, err) + } +} diff --git a/docs/plans/kubernetes-docker-workloads/task-03-docker-lifecycle.md b/docs/plans/kubernetes-docker-workloads/task-03-docker-lifecycle.md index d117f83fd4..5bda86ed2a 100644 --- a/docs/plans/kubernetes-docker-workloads/task-03-docker-lifecycle.md +++ b/docs/plans/kubernetes-docker-workloads/task-03-docker-lifecycle.md @@ -134,3 +134,17 @@ The exact implementation revision was exercised with image config digest Required Kubernetes and lifecycle Go packages, changed-file ESLint and test discovery also passed. This evidence applies to the recorded Kind/runtime combination; other container runtimes still require their own cgroup validation. + +A later Kubernetes acceptance run exposed two retained-workspace portability +gaps. A non-root workload could copy a checkout onto a group-writable, +root-owned PVC and then fail when `cp -a` restored metadata on the mount root. +The preparation scripts now copy checkout contents recursively without +restoring source ownership or timestamps. The same run also showed that a +credential-free GitHub HTTPS origin and the equivalent GitHub SSH origin were +compared literally during lost-Pod replacement. Preparation now normalizes +those GitHub URL forms before comparison while continuing to reject a different +repository. Runtime resolution upgrades exact persisted copies of the two +Kandev-supplied scripts; any customized script remains byte-for-byte unchanged. +Focused lifecycle regressions exercise both the built-in Kubernetes script and +the full worker script, preserve retained files, and reproduce the restricted +copy boundary with the real `cp` implementation. diff --git a/k8s/worker-images/full/prepare.sh b/k8s/worker-images/full/prepare.sh index 7ccb1d9725..eabeb83b5b 100644 --- a/k8s/worker-images/full/prepare.sh +++ b/k8s/worker-images/full/prepare.sh @@ -28,6 +28,13 @@ repository_url={{repository.clone_url}} repository_branch={{repository.branch}} clone_tmp=/opt/kandev/.workspace-clone +normalize_repository_origin() { + printf '%s\n' "$1" | sed \ + -e 's|^https://[^/@]*@github.com/|https://github.com/|' \ + -e 's|^git@github.com:|https://github.com/|' \ + -e 's|^ssh://git@github.com/|https://github.com/|' +} + # ---- Git identity and HTTPS authentication ---- {{git.identity_setup}} git config --global --add safe.directory '*' @@ -44,8 +51,8 @@ if [ -n "$repository_url" ]; then exit 1 fi workspace_origin=$(git -C "$workspace" remote get-url origin 2>/dev/null || true) - expected_origin=$(printf '%s\n' "$repository_url" | sed 's|^https://[^/@]*@github.com/|https://github.com/|') - retained_origin=$(printf '%s\n' "$workspace_origin" | sed 's|^https://[^/@]*@github.com/|https://github.com/|') + expected_origin=$(normalize_repository_origin "$repository_url") + retained_origin=$(normalize_repository_origin "$workspace_origin") if [ -z "$workspace_origin" ] || [ "$retained_origin" != "$expected_origin" ]; then echo 'kandev: retained workspace repository origin does not match the configured repository' >&2 exit 1 @@ -60,7 +67,7 @@ if [ -n "$repository_url" ]; then rm -rf "$clone_tmp" trap 'rm -rf "$clone_tmp"' 0 1 2 15 git clone --depth=1 --branch "$repository_branch" "$repository_url" "$clone_tmp" - cp -a "$clone_tmp"/. "$workspace"/ + cp -R "$clone_tmp"/. "$workspace"/ rm -rf "$clone_tmp" trap - 0 1 2 15 fi