From faa5e66f4f8579d13a8440746ddcc0325fce61f2 Mon Sep 17 00:00:00 2001 From: Dirceu Tiegs Date: Thu, 12 Sep 2019 14:02:17 -0400 Subject: [PATCH 1/4] Use TaskConfigValidator for RunnerTask --- lib/kubernetes-deploy/runner_task.rb | 29 ++++------------ .../serial_task_run_test.rb | 1 - test/integration/runner_task_test.rb | 6 ++-- .../kubernetes-deploy/runner_task_test.rb | 34 ------------------- 4 files changed, 10 insertions(+), 60 deletions(-) delete mode 100644 test/unit/kubernetes-deploy/runner_task_test.rb diff --git a/lib/kubernetes-deploy/runner_task.rb b/lib/kubernetes-deploy/runner_task.rb index ed9a35354..c16d1325f 100644 --- a/lib/kubernetes-deploy/runner_task.rb +++ b/lib/kubernetes-deploy/runner_task.rb @@ -35,7 +35,7 @@ def run!(task_template:, entrypoint:, args:, env_vars: [], verify_result: true) @logger.reset @logger.phase_heading("Initializing task") - validate_configuration(task_template, args) + verify_config!(task_template, args) pod = build_pod(task_template, entrypoint, args, env_vars, verify_result) validate_pod(pod) @@ -107,7 +107,7 @@ def record_status_once(pod) @logger.summary.add_paragraph(warning) end - def validate_configuration(task_template, args) + def verify_config!(task_template, args) @logger.info("Validating configuration") errors = [] @@ -115,33 +115,18 @@ def validate_configuration(task_template, args) errors << "Task template name can't be nil" end - if @namespace.blank? - errors << "Namespace can't be empty" - end - if args.blank? errors << "Args can't be nil" end - begin - kubeclient.get_namespace(@namespace) if @namespace.present? - @logger.info("Using namespace '#{@namespace}' in context '#{@context}'") - - rescue Kubeclient::ResourceNotFoundError - errors << "Namespace was not found" - rescue Kubeclient::HttpError - errors << "Could not connect to kubernetes cluster" - rescue KubernetesDeploy::KubeclientBuilder::ContextMissingError - errors << "Could not connect to kubernetes cluster - context invalid" - end - - unless errors.empty? + task_config_validator = TaskConfigValidator.new(@task_config, kubectl, kubeclient_builder) + unless task_config_validator.valid? @logger.summary.add_action("Configuration invalid") - @logger.summary.add_paragraph(errors.map { |err| "- #{err}" }.join("\n")) - raise TaskConfigurationError, "Configuration invalid: #{errors.join(', ')}" + @logger.summary.add_paragraph([task_config_validator.errors + errors].map { |err| "- #{err}" }.join("\n")) + raise KubernetesDeploy::TaskConfigurationError end - TaskConfigValidator.new(@task_config, kubectl, kubeclient_builder, only: [:validate_server_version]).valid? + @logger.info("Using namespace '#{@namespace}' in context '#{@context}'") end def get_template(template_name) diff --git a/test/integration-serial/serial_task_run_test.rb b/test/integration-serial/serial_task_run_test.rb index 03715b9d4..0415a9fa3 100644 --- a/test/integration-serial/serial_task_run_test.rb +++ b/test/integration-serial/serial_task_run_test.rb @@ -13,7 +13,6 @@ def test_run_without_verify_result_fails_if_pod_was_not_created # Sketchy, but stubbing the kubeclient doesn't work (and wouldn't be concurrency-friendly) # Finding a way to reliably trigger a create failure would be much better, if possible mock = mock() - mock.expects(:get_namespace) template = kubeclient.get_pod_template('hello-cloud-template-runner', @namespace) mock.expects(:get_pod_template).returns(template) mock.expects(:create_pod).raises(Kubeclient::HttpError.new("409", "Pod with same name exists", {})) diff --git a/test/integration/runner_task_test.rb b/test/integration/runner_task_test.rb index ced52c5a7..2ab882f81 100644 --- a/test/integration/runner_task_test.rb +++ b/test/integration/runner_task_test.rb @@ -179,7 +179,7 @@ def test_run_bang_raises_exceptions_as_well_as_printing_failure end def test_run_fails_if_context_is_invalid - task_runner = build_task_runner(context: "missing") + task_runner = build_task_runner(context: "unknown") assert_task_run_failure(task_runner.run(run_params)) assert_logs_match_all([ @@ -187,7 +187,7 @@ def test_run_fails_if_context_is_invalid "Validating configuration", "Result: FAILURE", "Configuration invalid", - "- Could not connect to kubernetes cluster - context invalid", + "Context unknown missing from your kubeconfig file(s)", ], in_order: true) end @@ -200,7 +200,7 @@ def test_run_fails_if_namespace_is_missing "Validating configuration", "Result: FAILURE", "Configuration invalid", - "- Namespace was not found", + "Could not find Namespace:", ], in_order: true) end diff --git a/test/unit/kubernetes-deploy/runner_task_test.rb b/test/unit/kubernetes-deploy/runner_task_test.rb deleted file mode 100644 index e7aa2c08f..000000000 --- a/test/unit/kubernetes-deploy/runner_task_test.rb +++ /dev/null @@ -1,34 +0,0 @@ -# frozen_string_literal: true -require 'test_helper' -require 'kubernetes-deploy/runner_task' - -class RunnerTaskUnitTest < KubernetesDeploy::TestCase - def test_run_with_invalid_configuration - task_runner = KubernetesDeploy::RunnerTask.new( - context: KubeclientHelper::TEST_CONTEXT, - namespace: nil, - logger: logger, - ) - - refute(task_runner.run(task_template: nil, entrypoint: nil, args: nil)) - assert_logs_match(/Task template name can't be nil/) - assert_logs_match(/Namespace can't be empty/) - assert_logs_match(/Args can't be nil/) - end - - def test_run_bang_with_invalid_configuration - task_runner = KubernetesDeploy::RunnerTask.new( - context: KubeclientHelper::TEST_CONTEXT, - namespace: nil, - logger: logger, - ) - - err = assert_raises(KubernetesDeploy::FatalDeploymentError) do - task_runner.run!(task_template: nil, entrypoint: nil, args: nil) - end - - assert_match(/Task template name can't be nil/, err.to_s) - assert_match(/Namespace can't be empty/, err.to_s) - assert_match(/Args can't be nil/, err.to_s) - end -end From 3c0d941efb3c72fbe830b43370673ba35e0f195d Mon Sep 17 00:00:00 2001 From: Dirceu Tiegs Date: Fri, 13 Sep 2019 10:18:15 -0400 Subject: [PATCH 2/4] Extract extra conditions to RunnerTaskConfigValidator --- lib/kubernetes-deploy/runner_task.rb | 24 ++++------ .../runner_task_config_validator.rb | 25 +++++++++++ test/integration/runner_task_test.rb | 44 +++++++++++++++++++ 3 files changed, 78 insertions(+), 15 deletions(-) create mode 100644 lib/kubernetes-deploy/runner_task_config_validator.rb diff --git a/lib/kubernetes-deploy/runner_task.rb b/lib/kubernetes-deploy/runner_task.rb index c16d1325f..b5675a993 100644 --- a/lib/kubernetes-deploy/runner_task.rb +++ b/lib/kubernetes-deploy/runner_task.rb @@ -8,6 +8,7 @@ require 'kubernetes-deploy/resource_watcher' require 'kubernetes-deploy/kubernetes_resource' require 'kubernetes-deploy/kubernetes_resource/pod' +require 'kubernetes-deploy/runner_task_config_validator' module KubernetesDeploy class RunnerTask @@ -35,7 +36,11 @@ def run!(task_template:, entrypoint:, args:, env_vars: [], verify_result: true) @logger.reset @logger.phase_heading("Initializing task") + + @logger.info("Validating configuration") verify_config!(task_template, args) + @logger.info("Using namespace '#{@namespace}' in context '#{@context}'") + pod = build_pod(task_template, entrypoint, args, env_vars, verify_result) validate_pod(pod) @@ -108,25 +113,14 @@ def record_status_once(pod) end def verify_config!(task_template, args) - @logger.info("Validating configuration") - errors = [] - - if task_template.blank? - errors << "Task template name can't be nil" - end - - if args.blank? - errors << "Args can't be nil" - end - - task_config_validator = TaskConfigValidator.new(@task_config, kubectl, kubeclient_builder) + task_config_validator = RunnerTaskConfigValidator.new(@task_config, kubectl, kubeclient_builder) + task_config_validator.template = task_template + task_config_validator.args = args unless task_config_validator.valid? @logger.summary.add_action("Configuration invalid") - @logger.summary.add_paragraph([task_config_validator.errors + errors].map { |err| "- #{err}" }.join("\n")) + @logger.summary.add_paragraph([task_config_validator.errors].map { |err| "- #{err}" }.join("\n")) raise KubernetesDeploy::TaskConfigurationError end - - @logger.info("Using namespace '#{@namespace}' in context '#{@context}'") end def get_template(template_name) diff --git a/lib/kubernetes-deploy/runner_task_config_validator.rb b/lib/kubernetes-deploy/runner_task_config_validator.rb new file mode 100644 index 000000000..e26cb5dbb --- /dev/null +++ b/lib/kubernetes-deploy/runner_task_config_validator.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true +module KubernetesDeploy + class RunnerTaskConfigValidator < TaskConfigValidator + attr_accessor :template, :args + + def initialize(*args) + super + @validations += %i(validate_template validate_args) + end + + private + + def validate_args + if args.blank? + @errors << "Args can't be nil" + end + end + + def validate_template + if template.blank? + @errors << "Task template name can't be nil" + end + end + end +end diff --git a/test/integration/runner_task_test.rb b/test/integration/runner_task_test.rb index 2ab882f81..8ac7e952f 100644 --- a/test/integration/runner_task_test.rb +++ b/test/integration/runner_task_test.rb @@ -204,6 +204,50 @@ def test_run_fails_if_namespace_is_missing ], in_order: true) end + def test_run_fails_if_args_are_missing + task_runner = build_task_runner + result = task_runner.run(task_template: 'hello-cloud-template-runner', + entrypoint: ['/bin/sh', '-c'], + args: nil, + env_vars: ["MY_CUSTOM_VARIABLE=MITTENS"]) + assert_task_run_failure(result) + + assert_logs_match_all([ + "Initializing task", + "Validating configuration", + "Result: FAILURE", + "Configuration invalid", + "Args can't be nil", + ], in_order: true) + end + + def test_run_fails_if_task_template_is_blank + task_runner = build_task_runner + result = task_runner.run(task_template: '', + entrypoint: ['/bin/sh', '-c'], + args: nil, + env_vars: ["MY_CUSTOM_VARIABLE=MITTENS"]) + assert_task_run_failure(result) + + assert_logs_match_all([ + "Initializing task", + "Validating configuration", + "Result: FAILURE", + "Configuration invalid", + "Task template name can't be nil", + ], in_order: true) + end + + def test_run_bang_fails_if_task_template_or_args_are_invalid + task_runner = build_task_runner + assert_raises(KubernetesDeploy::TaskConfigurationError) do + task_runner.run!(task_template: '', + entrypoint: ['/bin/sh', '-c'], + args: nil, + env_vars: ["MY_CUSTOM_VARIABLE=MITTENS"]) + end + end + def test_run_with_template_missing task_runner = build_task_runner assert_task_run_failure(task_runner.run(run_params)) From a4a0dae33a9ac3f9ed7dd412d17176c3e52de16f Mon Sep 17 00:00:00 2001 From: Dirceu Tiegs Date: Mon, 16 Sep 2019 14:08:54 -0400 Subject: [PATCH 3/4] Address PR feedback --- lib/kubernetes-deploy/runner_task.rb | 5 ++--- .../runner_task_config_validator.rb | 12 ++++++------ 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/lib/kubernetes-deploy/runner_task.rb b/lib/kubernetes-deploy/runner_task.rb index b5675a993..916491aa2 100644 --- a/lib/kubernetes-deploy/runner_task.rb +++ b/lib/kubernetes-deploy/runner_task.rb @@ -113,9 +113,8 @@ def record_status_once(pod) end def verify_config!(task_template, args) - task_config_validator = RunnerTaskConfigValidator.new(@task_config, kubectl, kubeclient_builder) - task_config_validator.template = task_template - task_config_validator.args = args + task_config_validator = RunnerTaskConfigValidator.new(task_template, args, @task_config, kubectl, + kubeclient_builder) unless task_config_validator.valid? @logger.summary.add_action("Configuration invalid") @logger.summary.add_paragraph([task_config_validator.errors].map { |err| "- #{err}" }.join("\n")) diff --git a/lib/kubernetes-deploy/runner_task_config_validator.rb b/lib/kubernetes-deploy/runner_task_config_validator.rb index e26cb5dbb..650eb76ad 100644 --- a/lib/kubernetes-deploy/runner_task_config_validator.rb +++ b/lib/kubernetes-deploy/runner_task_config_validator.rb @@ -1,23 +1,23 @@ # frozen_string_literal: true module KubernetesDeploy class RunnerTaskConfigValidator < TaskConfigValidator - attr_accessor :template, :args - - def initialize(*args) - super + def initialize(template, args, *arguments) + super(*arguments) + @template = template + @args = args @validations += %i(validate_template validate_args) end private def validate_args - if args.blank? + if @args.blank? @errors << "Args can't be nil" end end def validate_template - if template.blank? + if @template.blank? @errors << "Task template name can't be nil" end end From 34b38710d9f4547cfc4c6c1918e9d4acd88b2f5b Mon Sep 17 00:00:00 2001 From: Dirceu Tiegs Date: Wed, 18 Sep 2019 08:05:21 -0400 Subject: [PATCH 4/4] Add changelog entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8b30cdb7f..b780ec300 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ *Enhancements* - Officially support Kubernetes 1.15 ([#546](https://github.com/Shopify/kubernetes-deploy/pull/546)) - Make sure that we only declare a Service of type LoadBalancer as deployed after its IP address is published. [#547](https://github.com/Shopify/kubernetes-deploy/pull/547) +- Add more validations to `RunnerTask`. [#554](https://github.com/Shopify/kubernetes-deploy/pull/554) *Bug Fixes*