Repository navigation
Fetch pod logs with the caller's kubectl, and reject flag-like arguments - #1002
Merged
Merged
Conversation
erictt
marked this pull request as draft
September 23, 2026 17:41
erictt
force-pushed
the
fix-kubeconfig-propagation-and-arg-injection
branch
from
September 24, 2026 15:26
ed750c1 to
8f155fa
Compare
erictt
force-pushed
the
fix-kubeconfig-propagation-and-arg-injection
branch
2 times, most recently
from
September 28, 2026 19:21
007029e to
b4229ef
Compare
erictt
force-pushed
the
fix-ci-dependency-drift
branch
from
September 28, 2026 19:50
699b553 to
8c434a8
Compare
- `ContainerLogs` built its own `Kubectl` from a bare `TaskConfig`, so `kubectl logs` fell back to `ENV['KUBECONFIG']` while the rest of the task used the kubeconfig it was configured with. Both log-fetching paths already hold a task-scoped kubectl (`sync_debug_info` is handed one, and `Pod#sync`'s `ResourceCache` owns one), so pass that down instead. This is the shape `fetch_events(kubectl)` already uses, and the one the test double in `kubernetes_resource_test.rb` already declared. - `namespace`, `context` and `kind` reached kubectl as bare positional argv elements, so a value such as `--server=http://example.com` was parsed as a global flag. Validate all three before they are used.
erictt
force-pushed
the
fix-kubeconfig-propagation-and-arg-injection
branch
from
September 28, 2026 19:51
b4229ef to
821341c
Compare
erictt
marked this pull request as ready for review
September 28, 2026 21:32
jpfourny
approved these changes
Sep 28, 2026
erictt
force-pushed
the
fix-kubeconfig-propagation-and-arg-injection
branch
from
September 29, 2026 15:47
14ee475 to
66b8eaf
Compare
erictt
force-pushed
the
fix-kubeconfig-propagation-and-arg-injection
branch
from
September 29, 2026 16:02
66b8eaf to
56b1188
Compare
This branch was successfully deployed
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes shop/issues#84857 and shop/issues#84856, and bumps the version to 3.9.2.
Pod logs ignored the task's kubeconfig (shop/issues#84857)
ContainerLogsbuilt its ownKubectlfrom aTaskConfigwithout a kubeconfig, sokubectl logsfell back toENV['KUBECONFIG']. Every other call in the task used the explicitkubeconfig:. In a process that deploys with a per-task kubeconfig but has a different ambient one, logs could be read from the wrong cluster.Both log paths already hold a task-scoped kubectl:
sync_debug_info(kubectl)andPod#sync(cache)throughResourceCache. SoContainerLogs/RemoteLogs#syncandfetch_debug_logsnow take the caller's kubectl, the same wayfetch_events(kubectl)does, and the extra client is gone. No constructor signatures change.Arguments parsed by kubectl as flags (shop/issues#84856)
namespace,contextandkindwere passed to kubectl as bare positional arguments, so a value like--server=http://attacker.examplewas parsed as a flag. Each is now validated before it's used:-. Context names vary too much (gke_…,arn:aws:eks:…) for a stricter format.Cron-Tabis a valid CRD kind.I audited every other dynamic argument to
Kubectl#run. The rest are either krane-generated (tempfile paths, discovery API paths) or passed as a single--flag=valueargument.Flaky
test_jobs_can_failIt failed in each of the last 4 CI runs. With
backoffLimit: 1, the Job could fail on its first pod crash before Kubernetes recordedDeadlineExceeded. The test now uses the defaultbackoffLimit, and it asserts on krane'sDeadlineExceeded (…)failure message instead of the asynchronous event.Testing
8 new unit tests, each confirmed to fail without the fix. CI passes 97/97, including
test_jobs_can_failin all 24 integration jobs.