Skip to content

Fetch pod logs with the caller's kubectl, and reject flag-like arguments - #1002

Merged
erictt merged 3 commits into
mainfrom
fix-kubeconfig-propagation-and-arg-injection
Sep 29, 2026
Merged

erictt merged 3 commits into
mainfrom
fix-kubeconfig-propagation-and-arg-injection

Conversation

@erictt

@erictt erictt commented Sep 22, 2026 •

Copy link
Copy Markdown
Contributor

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)

ContainerLogs built its own Kubectl from a TaskConfig without a kubeconfig, so kubectl logs fell back to ENV['KUBECONFIG']. Every other call in the task used the explicit kubeconfig:. 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) and Pod#sync(cache) through ResourceCache. So ContainerLogs/RemoteLogs#sync and fetch_debug_logs now take the caller's kubectl, the same way fetch_events(kubectl) does, and the extra client is gone. No constructor signatures change.

Arguments parsed by kubectl as flags (shop/issues#84856)

namespace, context and kind were passed to kubectl as bare positional arguments, so a value like --server=http://attacker.example was parsed as a flag. Each is now validated before it's used:

  • namespace: must be a DNS-1123 label, which Kubernetes requires anyway.
  • context: must not start with -. Context names vary too much (gke_…, arn:aws:eks:…) for a stricter format.
  • kind: must start with a letter. Hyphens stay allowed because Cron-Tab is 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=value argument.

Flaky test_jobs_can_fail

It failed in each of the last 4 CI runs. With backoffLimit: 1, the Job could fail on its first pod crash before Kubernetes recorded DeadlineExceeded. The test now uses the default backoffLimit, and it asserts on krane's DeadlineExceeded (…) 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_fail in all 24 integration jobs.

@erictt
erictt requested a review from a team as a code owner September 22, 2026 14:38
@erictt
erictt marked this pull request as draft September 23, 2026 17:41
@erictt
erictt force-pushed the fix-kubeconfig-propagation-and-arg-injection branch from ed750c1 to 8f155fa Compare September 24, 2026 15:26
@erictt erictt changed the title Scope kubectl log reads to the task's kubeconfig and reject flag-like arguments Fetch pod logs with the caller's kubectl, and reject flag-like arguments Sep 24, 2026
@erictt
erictt force-pushed the fix-kubeconfig-propagation-and-arg-injection branch 2 times, most recently from 007029e to b4229ef Compare September 28, 2026 19:21
@erictt
erictt changed the base branch from main to fix-ci-dependency-drift September 28, 2026 19:21
@erictt
erictt force-pushed the fix-ci-dependency-drift branch from 699b553 to 8c434a8 Compare September 28, 2026 19:50
- `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
erictt force-pushed the fix-kubeconfig-propagation-and-arg-injection branch from b4229ef to 821341c Compare September 28, 2026 19:51
@erictt
erictt marked this pull request as ready for review September 28, 2026 21:32
@erictt
erictt changed the base branch from fix-ci-dependency-drift to main September 29, 2026 15:39
@erictt
erictt force-pushed the fix-kubeconfig-propagation-and-arg-injection branch from 14ee475 to 66b8eaf Compare September 29, 2026 15:47
@erictt
erictt force-pushed the fix-kubeconfig-propagation-and-arg-injection branch from 66b8eaf to 56b1188 Compare September 29, 2026 16:02
@erictt
erictt merged commit a6e6a87 into main Sep 29, 2026
97 checks passed

This branch was successfully deployed

1 active deployment
rubygems — 56b1188e Deployed Sep 29, 2026 by shopify-shipit[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants