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

CHANGELOG.md

@@ -1,6 +1,9 @@
 ## next
 
+## 3.9.2
 - Parse JSON with `JSON.parse` instead of `MultiJson.load`, and drop the `multi_json` dependency. On Ruby 3.0 and 3.1, `multi_json` 1.19.1 (the newest those versions can install) crashes with `json` 3.x
+- Pod log retrieval now uses the task's `kubeconfig` instead of falling back to `ENV['KUBECONFIG']`
+- Reject namespaces, contexts and kinds that `kubectl` would parse as flags rather than as positional arguments
 
 ## 3.9.1
 - Test against k8s 1.34

lib/krane/container_logs.rb

@@ -16,8 +16,8 @@ def initialize(parent_id:, container_name:, namespace:, context:, logger:)
       @printed_latest = false
     end
 
-    def sync
-      new_logs = fetch_latest
+    def sync(kubectl)
+      new_logs = fetch_latest(kubectl)
       return unless new_logs.present?
       @lines += sort_and_deduplicate(new_logs)
     end
@@ -47,7 +47,7 @@ def printing_started?
 
     private
 
-    def fetch_latest
+    def fetch_latest(kubectl)
       cmd = ["logs", @parent_id, "--container=#{container_name}", "--timestamps"]
       cmd << if @last_timestamp.present?
         "--since-time=#{rfc3339_timestamp(@last_timestamp)}"
@@ -58,11 +58,6 @@ def fetch_latest
       out.encode('UTF-8', invalid: :replace, replace: '').split("\n")
     end
 
-    def kubectl
-      task_config = TaskConfig.new(@context, @namespace, @logger)
-      @kubectl ||= Kubectl.new(task_config: task_config, log_failure_by_default: false)
-    end
-
     def rfc3339_timestamp(time)
       time.strftime("%FT%T.%N%:z")
     end

lib/krane/kubernetes_resource.rb

@@ -35,6 +35,11 @@ class KubernetesResource
       If you have reason to believe it will succeed, retry the deploy to continue to monitor the rollout.
       MSG
 
+    # Kubernetes kinds begin with a letter, and may contain dashes after that (the API server
+    # lowercases the kind and validates it as a DNS-1035 label). Only the first character matters
+    # here: a leading dash would make kubectl parse the kind as a flag rather than a resource kind.
+    KIND_FORMAT = /\A[A-Za-z]/
+
     ALLOWED_DEPLOY_METHOD_OVERRIDES = %w(create replace replace-force)
     DEPLOY_METHOD_OVERRIDE_ANNOTATION = "deploy-method-override"
     TIMEOUT_OVERRIDE_ANNOTATION = "timeout-override"
@@ -91,6 +96,10 @@ def validate_definition_essentials(definition)
           raise InvalidTemplateError.new("Template is missing required field 'kind'", content: debug_content)
         end
 
+        unless definition["kind"].match?(KIND_FORMAT)
+          raise InvalidTemplateError.new("Template's 'kind' must begin with a letter", content: debug_content)
+        end
+
         if definition.dig('metadata', 'name').blank? && definition.dig('metadata', 'generateName').blank?
           raise InvalidTemplateError.new("Template must specify one of 'metadata.name' or 'metadata.generateName'",
             content: debug_content)
@@ -250,7 +259,7 @@ def deploy_method_override
 
     def sync_debug_info(kubectl)
       @debug_events = fetch_events(kubectl) unless ENV[DISABLE_FETCHING_EVENT_INFO]
-      @debug_logs = fetch_debug_logs if print_debug_logs? && !ENV[DISABLE_FETCHING_LOG_INFO]
+      @debug_logs = fetch_debug_logs(kubectl) if print_debug_logs? && !ENV[DISABLE_FETCHING_LOG_INFO]
     end
 
     def debug_message(cause = nil, info_hash = {})

lib/krane/kubernetes_resource/daemon_set.rb

@@ -30,9 +30,9 @@ def deploy_failed?
       observed_generation == current_generation
     end
 
-    def fetch_debug_logs
+    def fetch_debug_logs(kubectl)
       most_useful_pod = pods.find(&:deploy_failed?) || pods.find(&:deploy_timed_out?) || pods.first
-      most_useful_pod.fetch_debug_logs
+      most_useful_pod.fetch_debug_logs(kubectl)
     end
 
     def print_debug_logs?

lib/krane/kubernetes_resource/deployment.rb

@@ -29,8 +29,8 @@ def print_debug_logs?
       @latest_rs.present?
     end
 
-    def fetch_debug_logs
-      @latest_rs.fetch_debug_logs
+    def fetch_debug_logs(kubectl)
+      @latest_rs.fetch_debug_logs(kubectl)
     end
 
     def deploy_succeeded?

lib/krane/kubernetes_resource/pod.rb

@@ -28,12 +28,12 @@ def initialize(namespace:, context:, definition:, logger:,
             logger: logger, statsd_tags: statsd_tags)
     end
 
-    def sync(_cache)
+    def sync(cache)
       super
       raise_predates_deploy_error if exists? && unmanaged? && !deploy_started?
 
       if exists?
-        logs.sync if unmanaged?
+        logs.sync(cache.kubectl) if unmanaged?
         update_container_statuses(@instance_data["status"])
       else # reset
         @containers.each(&:reset_status)
@@ -93,8 +93,8 @@ def failure_message
       "#{phase_failure_message} #{container_problems}".strip.presence
     end
 
-    def fetch_debug_logs
-      logs.sync
+    def fetch_debug_logs(kubectl)
+      logs.sync(kubectl)
       logs
     end
 

lib/krane/kubernetes_resource/pod_set_base.rb

@@ -18,15 +18,15 @@ def fetch_events(kubectl)
       own_events.merge(most_useful_pod.fetch_events(kubectl))
     end
 
-    def fetch_debug_logs
+    def fetch_debug_logs(kubectl)
       logs = Krane::RemoteLogs.new(
         logger: @logger,
         parent_id: id,
         container_names: container_names,
         namespace: @namespace,
         context: @context
       )
-      logs.sync
+      logs.sync(kubectl)
       logs
     end
 

lib/krane/remote_logs.rb

@@ -23,8 +23,8 @@ def empty?
       @container_logs.all?(&:empty?)
     end
 
-    def sync
-      @container_logs.each(&:sync)
+    def sync(kubectl)
+      @container_logs.each { |cl| cl.sync(kubectl) }
     end
 
     def print_latest

lib/krane/resource_cache.rb

@@ -4,6 +4,8 @@
 
 module Krane
   class ResourceCache
+    attr_reader :kubectl
+
     delegate :namespace, :context, :logger, to: :@task_config
 
     def initialize(task_config)

lib/krane/task_config_validator.rb

@@ -9,6 +9,10 @@ class TaskConfigValidator
       validate_namespace_exists
     ).freeze
 
+    # Kubernetes namespaces are DNS-1123 labels. Anything else, in particular a value starting with
+    # a dash, would be parsed by kubectl as a flag rather than as a resource name.
+    NAMESPACE_FORMAT = /\A[a-z0-9]([-a-z0-9]{0,61}[a-z0-9])?\z/
+
     delegate :context, :namespace, :logger, to: :@task_config
 
     def initialize(task_config, kubectl, kubeclient_builder, only: nil)
@@ -44,6 +48,12 @@ def validate_context_exists_in_kubeconfig
         return @errors << "Context can not be blank"
       end
 
+      # A context is passed to kubectl as a bare positional argument; a leading dash would make
+      # kubectl parse it as a flag. Context names are otherwise too varied to constrain further.
+      if context.start_with?("-")
+        return @errors << "Context can not start with a dash"
+      end
+
       _, err, st = @kubectl.run("config", "get-contexts", context, "-o", "name",
         use_namespace: false, use_context: false, log_failure: false, attempts: 2)
 
@@ -70,6 +80,10 @@ def validate_namespace_exists
         return @errors << "Namespace can not be blank"
       end
 
+      unless namespace.match?(NAMESPACE_FORMAT)
+        return @errors << "Namespace is not a valid Kubernetes namespace name"
+      end
+
       _, err, st = @kubectl.run("get", "namespace", "-o", "name", namespace,
         use_namespace: false, log_failure: false, attempts: 3)
 

lib/krane/version.rb

@@ -1,4 +1,4 @@
 # frozen_string_literal: true
 module Krane
-  VERSION = "3.9.1"
+  VERSION = "3.9.2"
 end

test/integration/krane_deploy_test.rb

@@ -1324,7 +1324,6 @@ def test_cronjobs_can_be_deployed
   def test_jobs_can_fail
     fixtures = deploy_fixtures("hello-cloud", subset: ["job.yml"]) do |f|
       spec = f["job.yml"]["Job"].first["spec"]
-      spec["backoffLimit"] = 1
       spec["activeDeadlineSeconds"] = 1
       spec["template"]["spec"]["containers"].first["command"] = %w(/not/a/command)
     end
@@ -1334,8 +1333,8 @@ def test_jobs_can_fail
       "Deploying Job/hello-job (timeout: 600s)",
       "Result: FAILURE",
       "Job/hello-job: FAILED",
+      "DeadlineExceeded (Job was active longer than specified deadline)",
       "Final status: Failed",
-      *(%r{\[Job/hello-job\]\tDeadlineExceeded: Job was active longer than specified deadline \(\d+ events\)} if ENV['CI'] == 'true'),
     ])
   end
 

test/unit/container_logs_test.rb

@@ -13,14 +13,27 @@ def setup
     )
   end
 
+  def test_sync_uses_the_kubectl_it_is_given_and_builds_no_client_of_its_own
+    # Building its own Kubectl is what made log reads fall back to ENV['KUBECONFIG']
+    # instead of the credentials the task was configured with.
+    Krane::Kubectl.expects(:new).never
+    given_kubectl = mock('kubectl')
+    given_kubectl.expects(:run).with('logs', 'pod/pod-123-456', '--container=A', '--timestamps',
+      '--tail=25', log_failure: false).returns([logs_response_1, "", ""])
+
+    @logs.sync(given_kubectl)
+
+    assert_equal(generate_log_messages(1..3), @logs.lines)
+  end
+
   def test_sync_deduplicates_logs_emitted_fractional_seconds_apart
     Krane::Kubectl.any_instance.stubs(:run)
       .returns([logs_response_1, "", ""])
       .then.returns([logs_response_2, "", ""])
       .then.returns([logs_response_3, "", ""])
-    @logs.sync
-    @logs.sync
-    @logs.sync
+    @logs.sync(kubectl)
+    @logs.sync(kubectl)
+    @logs.sync(kubectl)
 
     assert_equal(generate_log_messages(1..15), @logs.lines)
   end
@@ -30,17 +43,17 @@ def test_sync_handles_cycles_where_no_new_logs_available
       .returns([logs_response_1, "", ""])
       .then.returns(["", "", ""])
       .then.returns([logs_response_2, "", ""])
-    @logs.sync
-    @logs.sync
-    @logs.sync
+    @logs.sync(kubectl)
+    @logs.sync(kubectl)
+    @logs.sync(kubectl)
 
     assert_equal(generate_log_messages(1..10), @logs.lines)
   end
 
   def test_empty_delegated_to_lines
     Krane::Kubectl.any_instance.stubs(:run).returns([logs_response_1, "", ""])
     assert_predicate(@logs, :empty?)
-    @logs.sync
+    @logs.sync(kubectl)
     refute_predicate(@logs, :empty?)
   end
 
@@ -49,7 +62,7 @@ def test_print_latest_and_print_all_output_the_correct_chunks
       .returns([logs_response_1, "", ""])
       .then.returns([logs_response_2, "", ""])
 
-    @logs.sync
+    @logs.sync(kubectl)
     @logs.print_latest
     assert_logs_match_all(generate_log_messages(1..3), in_order: true)
 
@@ -58,7 +71,7 @@ def test_print_latest_and_print_all_output_the_correct_chunks
     assert_logs_match_all(generate_log_messages(1..3), in_order: true)
 
     reset_logger
-    @logs.sync
+    @logs.sync(kubectl)
     @logs.print_latest
     assert_logs_match_all(generate_log_messages(4..10), in_order: true)
     refute_logs_match("Line 3")
@@ -70,7 +83,7 @@ def test_print_latest_and_print_all_output_the_correct_chunks
 
   def test_print_latest_supports_prefixing
     Krane::Kubectl.any_instance.stubs(:run).returns([logs_response_1, "", ""])
-    @logs.sync
+    @logs.sync(kubectl)
     expected = [
       "[A]  Line 1",
       "[A]  Line 2",
@@ -87,8 +100,8 @@ def test_logs_without_timestamps_are_not_deduped
       .returns([logs_response_1_with_anomaly, "", ""])
       .then.returns([logs_response_2_with_anomaly, "", ""])
 
-    @logs.sync
-    @logs.sync
+    @logs.sync(kubectl)
+    @logs.sync(kubectl)
     @logs.print_all
     assert_logs_match_all([
       "No timestamp", # moved to start of batch 1
@@ -106,9 +119,9 @@ def test_deduplication_works_when_exact_same_batch_is_returned_more_than_once
       .then.returns([logs_response_1, "", ""])
       .then.returns([logs_response_2, "", ""])
 
-    @logs.sync
-    @logs.sync
-    @logs.sync
+    @logs.sync(kubectl)
+    @logs.sync(kubectl)
+    @logs.sync(kubectl)
 
     @logs.print_all
     assert_logs_match_all(generate_log_messages(1..10), in_order: true)
@@ -140,7 +153,7 @@ def test_deduplication_works_when_last_line_is_out_of_order
       .returns([regression_data, "", ""]).times(12)
 
     12.times do
-      @logs.sync
+      @logs.sync(kubectl)
       @logs.print_latest
     end
 
@@ -154,6 +167,10 @@ def test_deduplication_works_when_last_line_is_out_of_order
 
   private
 
+  def kubectl
+    @kubectl ||= Krane::Kubectl.new(task_config: task_config, log_failure_by_default: false)
+  end
+
   def generate_log_messages(range)
     range.map { |i| "Line #{i}" }
   end

test/unit/krane/kubernetes_resource/pod_test.rb

@@ -324,6 +324,18 @@ def test_deploy_failed_is_false_for_disappeared_managed_pods
     assert_nil(pod.failure_message)
   end
 
+  def test_debug_logs_are_fetched_with_the_kubectl_the_caller_supplies
+    # The caller's kubectl carries the task's kubeconfig; building a fresh one here
+    # would silently fall back to ENV['KUBECONFIG'].
+    pod = Krane::Pod.new(namespace: 'test', context: 'nope', definition: build_pod_template,
+      logger: @logger, deploy_started_at: Time.now.utc)
+    Krane::Kubectl.expects(:new).never
+    given_kubectl = mock('kubectl')
+    given_kubectl.expects(:run).with { |*args, **| args.first == 'logs' }.returns(["", "", ""])
+
+    pod.fetch_debug_logs(given_kubectl)
+  end
+
   private
 
   def pod_spec

test/unit/krane/kubernetes_resource_test.rb

@@ -339,6 +339,23 @@ def test_lowercase_custom_resource_kind_does_not_raise
     )
   end
 
+  def test_build_rejects_kinds_that_kubectl_would_parse_as_flags
+    ["--server=http://attacker.example", "-oyaml"].each do |kind|
+      assert_raises_message(Krane::InvalidTemplateError, "Template's 'kind' must begin with a letter") do
+        Krane::KubernetesResource.build(namespace: "test", context: "test", logger: @logger,
+          statsd_tags: [], definition: { "kind" => kind, "metadata" => { "name" => "test" } })
+      end
+    end
+  end
+
+  def test_build_accepts_hyphenated_kinds
+    # The API server lowercases the kind and validates it as a DNS-1035 label, so dashes are legal
+    resource = Krane::KubernetesResource.build(namespace: "test", context: "test", logger: @logger,
+      statsd_tags: [], definition: { "kind" => "Cron-Tab", "metadata" => { "name" => "test" } })
+
+    assert_equal("Cron-Tab", resource.type)
+  end
+
   def test_build_handles_hardcoded_and_core_and_dynamic_objects
     # Dynamic with no rollout config
     no_config_crd = Krane::KubernetesResource.build(namespace: "test", context: "test",