From 2194206d7fed2f98f243b0c6501e80227880539f Mon Sep 17 00:00:00 2001 From: Daniel Turner Date: Fri, 16 Aug 2019 13:58:47 -0700 Subject: [PATCH 1/7] First pass at shared validation --- lib/kubernetes-deploy/common.rb | 2 + lib/kubernetes-deploy/restart_task.rb | 23 ++++--- lib/kubernetes-deploy/task_config.rb | 29 +++++++++ lib/kubernetes-deploy/validator.rb | 86 +++++++++++++++++++++++++++ test/integration/restart_task_test.rb | 4 +- test/integration/validator_test.rb | 44 ++++++++++++++ test/unit/task_config_test.rb | 27 +++++++++ 7 files changed, 200 insertions(+), 15 deletions(-) create mode 100644 lib/kubernetes-deploy/task_config.rb create mode 100644 lib/kubernetes-deploy/validator.rb create mode 100644 test/integration/validator_test.rb create mode 100644 test/unit/task_config_test.rb diff --git a/lib/kubernetes-deploy/common.rb b/lib/kubernetes-deploy/common.rb index e68113311..95b4afe19 100644 --- a/lib/kubernetes-deploy/common.rb +++ b/lib/kubernetes-deploy/common.rb @@ -15,6 +15,8 @@ require 'kubernetes-deploy/errors' require 'kubernetes-deploy/formatted_logger' require 'kubernetes-deploy/statsd' +require 'kubernetes-deploy/task_config' +require 'kubernetes-deploy/validator' module KubernetesDeploy MIN_KUBE_VERSION = '1.10.0' diff --git a/lib/kubernetes-deploy/restart_task.rb b/lib/kubernetes-deploy/restart_task.rb index 833aa6323..1dfa40074 100644 --- a/lib/kubernetes-deploy/restart_task.rb +++ b/lib/kubernetes-deploy/restart_task.rb @@ -22,6 +22,7 @@ def initialize(deployment_name, response) ANNOTATION = "shipit.shopify.io/restart" def initialize(context:, namespace:, logger: nil, max_watch_seconds: nil) + @task_config = KubernetesDeploy::TaskConfig.new(context, namespace, logger) @context = context @namespace = namespace @logger = logger || KubernetesDeploy::FormattedLogger.build(namespace, context) @@ -40,11 +41,14 @@ def perform!(deployments_names = nil, selector: nil) @logger.reset @logger.phase_heading("Initializing restart") - verify_namespace - deployments = identify_target_deployments(deployments_names, selector: selector) - if kubectl.server_version < Gem::Version.new(MIN_KUBE_VERSION) - @logger.warn(KubernetesDeploy::Errors.server_version_warning(kubectl.server_version)) + errors = Validator.new(@task_config).errors + unless errors.empty? + @logger.summary.add_action("Configuration invalid") + @logger.summary.add_paragraph(errors.map { |err| "- #{err}" }.join("\n")) + raise KubernetesDeploy::TaskConfigurationError end + + deployments = identify_target_deployments(deployments_names, selector: selector) @logger.phase_heading("Triggering restart by touching ENV[RESTARTED_AT]") patch_kubeclient_deployments(deployments) @@ -120,13 +124,6 @@ def build_watchables(kubeclient_resources, started) end end - def verify_namespace - kubeclient.get_namespace(@namespace) - @logger.info("Namespace #{@namespace} found in context #{@context}") - rescue Kubeclient::ResourceNotFoundError - raise NamespaceNotFoundError.new(@namespace, @context) - end - def patch_deployment_with_restart(record) v1beta1_kubeclient.patch_deployment( record.metadata.name, @@ -181,7 +178,7 @@ def kubeclient end def kubectl - @kubectl ||= Kubectl.new(namespace: @namespace, context: @context, logger: @logger, log_failure_by_default: true) + @task_config.kubectl end def v1beta1_kubeclient @@ -189,7 +186,7 @@ def v1beta1_kubeclient end def kubeclient_builder - @kubeclient_builder ||= KubeclientBuilder.new + @task_config.kubeclient_builder end end end diff --git a/lib/kubernetes-deploy/task_config.rb b/lib/kubernetes-deploy/task_config.rb new file mode 100644 index 000000000..7f506f64c --- /dev/null +++ b/lib/kubernetes-deploy/task_config.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true +module KubernetesDeploy + class TaskConfig + attr_reader :context, :namespace + + def initialize(context, namespace, logger = nil) + @context = context + @namespace = namespace + @logger = logger + end + + def logger + @logger ||= KubernetesDeploy::FormattedLogger.build(@namespace, @context) + end + + def kubectl(log_failure_by_default: true) + @kubectl ||= Kubectl.new( + namespace: @namespace, + context: @context, + logger: logger, + log_failure_by_default: log_failure_by_default + ) + end + + def kubeclient_builder + @kubeclient ||= KubeclientBuilder.new + end + end +end diff --git a/lib/kubernetes-deploy/validator.rb b/lib/kubernetes-deploy/validator.rb new file mode 100644 index 000000000..dd4e7a02c --- /dev/null +++ b/lib/kubernetes-deploy/validator.rb @@ -0,0 +1,86 @@ +# frozen_string_literal: true +module KubernetesDeploy + class Validator + DEFAULT_VALIDATIONS = %i( + validate_kubeconfig + validate_context_exists + validate_namespace_exists + validate_server_version + ).freeze + + delegate :context, :namespace, :logger, :kubectl, :kubeclient_builder, to: :@task_config + + def initialize(task_config) + @task_config = task_config + @errors = nil + end + + def valid? + @errors = [] + DEFAULT_VALIDATIONS.each do |validator_name| + send(validator_name) + break if @errors.present? + end + @errors.empty? + end + + def errors + valid? + @errors + end + + private + + def validate_kubeconfig + @errors += kubeclient_builder.validate_config_files + end + + def validate_context_exists + unless context.present? + return @errors << "Context can not be blank" + end + + _, err, st = kubectl.run("config", "get-contexts", context, "-o", "name", + use_namespace: false, use_context: false, log_failure: false) + + unless st.success? + @errors << if err.match("error: context #{context} not found") + "Context #{context} missing from your kubeconfig file(s)" + else + "Something went wrong. #{err} " + end + return + end + + _, err, st = kubectl.run("get", "namespaces", "-o", "name", + use_namespace: false, log_failure: false) + + unless st.success? + @errors << "Something went wrong connectting to #{context}. #{err} " + end + end + + def validate_namespace_exists + unless namespace.present? + return @errors << "Namespace can not be blank" + end + + _, err, st = kubectl.run("get", "namespace", "-o", "name", namespace, + use_namespace: false, log_failure: false) + + unless st.success? + @errors << if err.match("Error from server [(]NotFound[)]: namespace") + "Cloud not find Namespace: #{namespace} in Context: #{context}" + else + "Could not connect to kubernetes cluster. #{err}" + end + end + end + + def validate_server_version + if kubectl.server_version < Gem::Version.new(MIN_KUBE_VERSION) + logger.warn(KubernetesDeploy::Errors.server_version_warning(kubectl.server_version)) + end + end + end +end diff --git a/test/integration/restart_task_test.rb b/test/integration/restart_task_test.rb index fdf1f71c6..6127e8d6a 100644 --- a/test/integration/restart_task_test.rb +++ b/test/integration/restart_task_test.rb @@ -174,7 +174,7 @@ def test_restart_not_existing_context assert_restart_failure(restart.perform(%w(web))) assert_logs_match_all([ "Result: FAILURE", - "`walrus` context must be configured in your kubeconfig file(s)", + /- Context walrus missing from your kubeconfig file\(s\)/, ], in_order: true) end @@ -188,7 +188,7 @@ def test_restart_not_existing_namespace assert_restart_failure(restart.perform(%w(web))) assert_logs_match_all([ "Result: FAILURE", - "Namespace `walrus` not found in context `#{TEST_CONTEXT}`", + "- Cloud not find Namespace: walrus in Context: #{KubeclientHelper::TEST_CONTEXT}", ], in_order: true) end diff --git a/test/integration/validator_test.rb b/test/integration/validator_test.rb new file mode 100644 index 000000000..63cb54eb7 --- /dev/null +++ b/test/integration/validator_test.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true +require 'integration_test_helper' + +class ValidatorTest < KubernetesDeploy::IntegrationTest + def test_valid_configuration + assert_predicate(validator(context: KubeclientHelper::TEST_CONTEXT, namespace: 'default'), :valid?) + end + + def test_invalid_kubeconfig + assert_match(/Context test-context missing from/, validator.errors.join("\n")) + end + + def test_context_does_not_exists + assert_match("Context test-context missing from your kubeconfig file(s)", + validator.errors.join("\n")) + end + + def test_namespace_does_not_exists + assert_match(/Cloud not find Namespace: test-namespace in Context: #{KubeclientHelper::TEST_CONTEXT}/, + validator(context: KubeclientHelper::TEST_CONTEXT).errors.join("\n")) + end + + def test_invalid_server_version + old_min_version = KubernetesDeploy::MIN_KUBE_VERSION + new_min_version = "99999" + KubernetesDeploy.const_set(:MIN_KUBE_VERSION, new_min_version) + validator(context: KubeclientHelper::TEST_CONTEXT, namespace: 'default', logger: @logger).valid? + assert_logs_match_all([ + "Minimum cluster version requirement of #{new_min_version} not met.", + ]) + ensure + KubernetesDeploy.const_set(:MIN_KUBE_VERSION, old_min_version) + end + + private + + def validator(context: nil, namespace: nil, logger: nil) + KubernetesDeploy::Validator.new(task_config(context: context, namespace: namespace, logger: logger)) + end + + def task_config(context: nil, namespace: nil, logger: nil) + KubernetesDeploy::TaskConfig.new(context || "test-context", namespace || "test-namespace", logger) + end +end diff --git a/test/unit/task_config_test.rb b/test/unit/task_config_test.rb new file mode 100644 index 000000000..281db7ae6 --- /dev/null +++ b/test/unit/task_config_test.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true +require 'test_helper' + +class TaskConfigTest < KubernetesDeploy::TestCase + def test_responds_to_namespace + assert_equal(task_config.namespace, "test-namespace") + end + + def test_responds_to_context + assert_equal(task_config.context, "test-context") + end + + def test_builds_a_logger_if_none_provided + assert_equal(task_config.logger.class, KubernetesDeploy::FormattedLogger) + end + + def test_uses_provided_logger + logger = KubernetesDeploy::FormattedLogger.build(nil, nil) + assert_equal(task_config(logger: logger).logger, logger) + end + + private + + def task_config(context: nil, namespace: nil, logger: nil) + KubernetesDeploy::TaskConfig.new(context || "test-context", namespace || "test-namespace", logger) + end +end From 2987834ecf5ea25498a1ae16549fb3b710a256b7 Mon Sep 17 00:00:00 2001 From: Daniel Turner Date: Mon, 26 Aug 2019 11:23:22 -0700 Subject: [PATCH 2/7] PR feedback --- lib/kubernetes-deploy/common.rb | 2 +- lib/kubernetes-deploy/deploy_task.rb | 5 ++--- lib/kubernetes-deploy/errors.rb | 8 -------- lib/kubernetes-deploy/restart_task.rb | 10 +++++----- lib/kubernetes-deploy/runner_task.rb | 5 ++--- lib/kubernetes-deploy/task_config.rb | 13 ------------- .../{validator.rb => task_config_validator.rb} | 18 ++++++++++++------ 7 files changed, 22 insertions(+), 39 deletions(-) rename lib/kubernetes-deploy/{validator.rb => task_config_validator.rb} (79%) diff --git a/lib/kubernetes-deploy/common.rb b/lib/kubernetes-deploy/common.rb index 95b4afe19..d4d6873b5 100644 --- a/lib/kubernetes-deploy/common.rb +++ b/lib/kubernetes-deploy/common.rb @@ -16,7 +16,7 @@ require 'kubernetes-deploy/formatted_logger' require 'kubernetes-deploy/statsd' require 'kubernetes-deploy/task_config' -require 'kubernetes-deploy/validator' +require 'kubernetes-deploy/task_config_validator' module KubernetesDeploy MIN_KUBE_VERSION = '1.10.0' diff --git a/lib/kubernetes-deploy/deploy_task.rb b/lib/kubernetes-deploy/deploy_task.rb index 042e63b12..a1406ba3f 100644 --- a/lib/kubernetes-deploy/deploy_task.rb +++ b/lib/kubernetes-deploy/deploy_task.rb @@ -109,6 +109,7 @@ def server_version def initialize(namespace:, context:, current_sha:, template_dir:, logger: nil, kubectl_instance: nil, bindings: {}, max_watch_seconds: nil, selector: nil) + @task_config = KubernetesDeploy::TaskConfig.new(context, namespace, logger) @namespace = namespace @namespace_tags = [] @context = context @@ -551,9 +552,7 @@ def confirm_cluster_reachable end end raise FatalDeploymentError, "Failed to reach server for #{@context}" unless success - if kubectl.server_version < Gem::Version.new(MIN_KUBE_VERSION) - @logger.warn(KubernetesDeploy::Errors.server_version_warning(server_version)) - end + TaskConfigValidator.new(@task_config, only: [:validate_server_version]).valid? end def confirm_namespace_exists diff --git a/lib/kubernetes-deploy/errors.rb b/lib/kubernetes-deploy/errors.rb index d70d84e9d..cae3ba3f1 100644 --- a/lib/kubernetes-deploy/errors.rb +++ b/lib/kubernetes-deploy/errors.rb @@ -30,12 +30,4 @@ def initialize "kubernetes-deploy will not continue since it is extremely unlikely that this secret should be pruned.") end end - - module Errors - extend self - def server_version_warning(server_version) - "Minimum cluster version requirement of #{MIN_KUBE_VERSION} not met. "\ - "Using #{server_version} could result in unexpected behavior as it is no longer tested against" - end - end end diff --git a/lib/kubernetes-deploy/restart_task.rb b/lib/kubernetes-deploy/restart_task.rb index 1dfa40074..ab79c3268 100644 --- a/lib/kubernetes-deploy/restart_task.rb +++ b/lib/kubernetes-deploy/restart_task.rb @@ -41,10 +41,10 @@ def perform!(deployments_names = nil, selector: nil) @logger.reset @logger.phase_heading("Initializing restart") - errors = Validator.new(@task_config).errors - unless errors.empty? + task_config_validator = TaskConfigValidator.new(@task_config) + unless task_config_validator.valid? @logger.summary.add_action("Configuration invalid") - @logger.summary.add_paragraph(errors.map { |err| "- #{err}" }.join("\n")) + @logger.summary.add_paragraph(task_config_validator.errors.map { |err| "- #{err}" }.join("\n")) raise KubernetesDeploy::TaskConfigurationError end @@ -178,7 +178,7 @@ def kubeclient end def kubectl - @task_config.kubectl + @kubectl ||= Kubectl.new(namespace: @namespace, context: @context, logger: @logger, log_failure_by_default: true) end def v1beta1_kubeclient @@ -186,7 +186,7 @@ def v1beta1_kubeclient end def kubeclient_builder - @task_config.kubeclient_builder + @kubeclient_builder ||= KubeclientBuilder.new end end end diff --git a/lib/kubernetes-deploy/runner_task.rb b/lib/kubernetes-deploy/runner_task.rb index 925583162..44af45c79 100644 --- a/lib/kubernetes-deploy/runner_task.rb +++ b/lib/kubernetes-deploy/runner_task.rb @@ -17,6 +17,7 @@ class TaskTemplateMissingError < TaskConfigurationError; end def initialize(namespace:, context:, logger: nil, max_watch_seconds: nil) @logger = logger || KubernetesDeploy::FormattedLogger.build(namespace, context) + @task_config = KubernetesDeploy::TaskConfig.new(context, namespace, @logger) @namespace = namespace @context = context @max_watch_seconds = max_watch_seconds @@ -140,9 +141,7 @@ def validate_configuration(task_template, args) raise TaskConfigurationError, "Configuration invalid: #{errors.join(', ')}" end - if kubectl.server_version < Gem::Version.new(MIN_KUBE_VERSION) - @logger.warn(KubernetesDeploy::Errors.server_version_warning(kubectl.server_version)) - end + TaskConfigValidator.new(@task_config, only: [:validate_server_version]).valid? end def get_template(template_name) diff --git a/lib/kubernetes-deploy/task_config.rb b/lib/kubernetes-deploy/task_config.rb index 7f506f64c..69ba12860 100644 --- a/lib/kubernetes-deploy/task_config.rb +++ b/lib/kubernetes-deploy/task_config.rb @@ -12,18 +12,5 @@ def initialize(context, namespace, logger = nil) def logger @logger ||= KubernetesDeploy::FormattedLogger.build(@namespace, @context) end - - def kubectl(log_failure_by_default: true) - @kubectl ||= Kubectl.new( - namespace: @namespace, - context: @context, - logger: logger, - log_failure_by_default: log_failure_by_default - ) - end - - def kubeclient_builder - @kubeclient ||= KubeclientBuilder.new - end end end diff --git a/lib/kubernetes-deploy/validator.rb b/lib/kubernetes-deploy/task_config_validator.rb similarity index 79% rename from lib/kubernetes-deploy/validator.rb rename to lib/kubernetes-deploy/task_config_validator.rb index dd4e7a02c..da6326c91 100644 --- a/lib/kubernetes-deploy/validator.rb +++ b/lib/kubernetes-deploy/task_config_validator.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true module KubernetesDeploy - class Validator + class TaskConfigValidator DEFAULT_VALIDATIONS = %i( validate_kubeconfig validate_context_exists @@ -8,18 +8,19 @@ class Validator validate_server_version ).freeze - delegate :context, :namespace, :logger, :kubectl, :kubeclient_builder, to: :@task_config + delegate :context, :namespace, :logger, to: :@task_config - def initialize(task_config) + def initialize(task_config, only: nil) @task_config = task_config @errors = nil + @validations = only || DEFAULT_VALIDATIONS end def valid? @errors = [] - DEFAULT_VALIDATIONS.each do |validator_name| - send(validator_name) + @validations.each do |validator_name| break if @errors.present? + send(validator_name) end @errors.empty? end @@ -79,8 +80,13 @@ def validate_namespace_exists def validate_server_version if kubectl.server_version < Gem::Version.new(MIN_KUBE_VERSION) - logger.warn(KubernetesDeploy::Errors.server_version_warning(kubectl.server_version)) + logger.warn(server_version_warning(kubectl.server_version)) end end + + def server_version_warning(server_version) + "Minimum cluster version requirement of #{MIN_KUBE_VERSION} not met. "\ + "Using #{server_version} could result in unexpected behavior as it is no longer tested against" + end end end From 5736e1573cec78ae76449d81b8625a5341b4babe Mon Sep 17 00:00:00 2001 From: Daniel Turner Date: Mon, 26 Aug 2019 16:15:06 -0700 Subject: [PATCH 3/7] Fix up --- lib/kubernetes-deploy/deploy_task.rb | 2 +- lib/kubernetes-deploy/restart_task.rb | 2 +- lib/kubernetes-deploy/runner_task.rb | 2 +- lib/kubernetes-deploy/task_config_validator.rb | 16 +++++++++------- ...tor_test.rb => task_config_validator_test.rb} | 13 +++++++++++-- 5 files changed, 23 insertions(+), 12 deletions(-) rename test/integration/{validator_test.rb => task_config_validator_test.rb} (69%) diff --git a/lib/kubernetes-deploy/deploy_task.rb b/lib/kubernetes-deploy/deploy_task.rb index a1406ba3f..71e57d773 100644 --- a/lib/kubernetes-deploy/deploy_task.rb +++ b/lib/kubernetes-deploy/deploy_task.rb @@ -552,7 +552,7 @@ def confirm_cluster_reachable end end raise FatalDeploymentError, "Failed to reach server for #{@context}" unless success - TaskConfigValidator.new(@task_config, only: [:validate_server_version]).valid? + TaskConfigValidator.new(@task_config, kubectl, kubeclient_builder, only: [:validate_server_version]).valid? end def confirm_namespace_exists diff --git a/lib/kubernetes-deploy/restart_task.rb b/lib/kubernetes-deploy/restart_task.rb index ab79c3268..526c46d49 100644 --- a/lib/kubernetes-deploy/restart_task.rb +++ b/lib/kubernetes-deploy/restart_task.rb @@ -41,7 +41,7 @@ def perform!(deployments_names = nil, selector: nil) @logger.reset @logger.phase_heading("Initializing restart") - task_config_validator = TaskConfigValidator.new(@task_config) + 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(task_config_validator.errors.map { |err| "- #{err}" }.join("\n")) diff --git a/lib/kubernetes-deploy/runner_task.rb b/lib/kubernetes-deploy/runner_task.rb index 44af45c79..ed9a35354 100644 --- a/lib/kubernetes-deploy/runner_task.rb +++ b/lib/kubernetes-deploy/runner_task.rb @@ -141,7 +141,7 @@ def validate_configuration(task_template, args) raise TaskConfigurationError, "Configuration invalid: #{errors.join(', ')}" end - TaskConfigValidator.new(@task_config, only: [:validate_server_version]).valid? + TaskConfigValidator.new(@task_config, kubectl, kubeclient_builder, only: [:validate_server_version]).valid? end def get_template(template_name) diff --git a/lib/kubernetes-deploy/task_config_validator.rb b/lib/kubernetes-deploy/task_config_validator.rb index da6326c91..5de68a93c 100644 --- a/lib/kubernetes-deploy/task_config_validator.rb +++ b/lib/kubernetes-deploy/task_config_validator.rb @@ -10,8 +10,10 @@ class TaskConfigValidator delegate :context, :namespace, :logger, to: :@task_config - def initialize(task_config, only: nil) + def initialize(task_config, kubectl, kubeclient_builder, only: nil) @task_config = task_config + @kubectl = kubectl + @kubeclient_builder = kubeclient_builder @errors = nil @validations = only || DEFAULT_VALIDATIONS end @@ -33,7 +35,7 @@ def errors private def validate_kubeconfig - @errors += kubeclient_builder.validate_config_files + @errors += @kubeclient_builder.validate_config_files end def validate_context_exists @@ -41,7 +43,7 @@ def validate_context_exists return @errors << "Context can not be blank" end - _, err, st = kubectl.run("config", "get-contexts", context, "-o", "name", + _, err, st = @kubectl.run("config", "get-contexts", context, "-o", "name", use_namespace: false, use_context: false, log_failure: false) unless st.success? @@ -53,7 +55,7 @@ def validate_context_exists return end - _, err, st = kubectl.run("get", "namespaces", "-o", "name", + _, err, st = @kubectl.run("get", "namespaces", "-o", "name", use_namespace: false, log_failure: false) unless st.success? @@ -66,7 +68,7 @@ def validate_namespace_exists return @errors << "Namespace can not be blank" end - _, err, st = kubectl.run("get", "namespace", "-o", "name", namespace, + _, err, st = @kubectl.run("get", "namespace", "-o", "name", namespace, use_namespace: false, log_failure: false) unless st.success? @@ -79,8 +81,8 @@ def validate_namespace_exists end def validate_server_version - if kubectl.server_version < Gem::Version.new(MIN_KUBE_VERSION) - logger.warn(server_version_warning(kubectl.server_version)) + if @kubectl.server_version < Gem::Version.new(MIN_KUBE_VERSION) + logger.warn(server_version_warning(@kubectl.server_version)) end end diff --git a/test/integration/validator_test.rb b/test/integration/task_config_validator_test.rb similarity index 69% rename from test/integration/validator_test.rb rename to test/integration/task_config_validator_test.rb index 63cb54eb7..65d9ec61a 100644 --- a/test/integration/validator_test.rb +++ b/test/integration/task_config_validator_test.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true require 'integration_test_helper' -class ValidatorTest < KubernetesDeploy::IntegrationTest +class TaskConfigValidatorTest < KubernetesDeploy::IntegrationTest def test_valid_configuration assert_predicate(validator(context: KubeclientHelper::TEST_CONTEXT, namespace: 'default'), :valid?) end @@ -10,6 +10,11 @@ def test_invalid_kubeconfig assert_match(/Context test-context missing from/, validator.errors.join("\n")) end + def test_only_is_respected + validator = KubernetesDeploy::TaskConfigValidator.new(task_config, nil, nil, only: []) + assert_predicate(validator, :valid?) + end + def test_context_does_not_exists assert_match("Context test-context missing from your kubeconfig file(s)", validator.errors.join("\n")) @@ -35,7 +40,11 @@ def test_invalid_server_version private def validator(context: nil, namespace: nil, logger: nil) - KubernetesDeploy::Validator.new(task_config(context: context, namespace: namespace, logger: logger)) + config = task_config(context: context, namespace: namespace, logger: logger) + kubectl = KubernetesDeploy::Kubectl.new(namespace: config.namespace, + context: config.context, logger: config.logger, log_failure_by_default: true) + kubeclient_builder = KubernetesDeploy::KubeclientBuilder.new + KubernetesDeploy::TaskConfigValidator.new(config, kubectl, kubeclient_builder) end def task_config(context: nil, namespace: nil, logger: nil) From e79a6ac2ff02266c3815d7af40e42b6ca2f41eb6 Mon Sep 17 00:00:00 2001 From: Daniel Turner Date: Tue, 27 Aug 2019 09:50:41 -0700 Subject: [PATCH 4/7] Improve tests --- lib/kubernetes-deploy/deploy_task.rb | 4 +-- lib/kubernetes-deploy/restart_task.rb | 4 +-- .../task_config_validator.rb | 8 +++--- .../integration/task_config_validator_test.rb | 26 ++++++++++++------- 4 files changed, 26 insertions(+), 16 deletions(-) diff --git a/lib/kubernetes-deploy/deploy_task.rb b/lib/kubernetes-deploy/deploy_task.rb index 71e57d773..3d07da78f 100644 --- a/lib/kubernetes-deploy/deploy_task.rb +++ b/lib/kubernetes-deploy/deploy_task.rb @@ -109,13 +109,13 @@ def server_version def initialize(namespace:, context:, current_sha:, template_dir:, logger: nil, kubectl_instance: nil, bindings: {}, max_watch_seconds: nil, selector: nil) - @task_config = KubernetesDeploy::TaskConfig.new(context, namespace, logger) + @logger = logger || KubernetesDeploy::FormattedLogger.build(namespace, context) + @task_config = KubernetesDeploy::TaskConfig.new(context, namespace, @logger) @namespace = namespace @namespace_tags = [] @context = context @current_sha = current_sha @template_dir = File.expand_path(template_dir) - @logger = logger || KubernetesDeploy::FormattedLogger.build(namespace, context) @kubectl = kubectl_instance @max_watch_seconds = max_watch_seconds @renderer = KubernetesDeploy::Renderer.new( diff --git a/lib/kubernetes-deploy/restart_task.rb b/lib/kubernetes-deploy/restart_task.rb index 526c46d49..e1425cc1e 100644 --- a/lib/kubernetes-deploy/restart_task.rb +++ b/lib/kubernetes-deploy/restart_task.rb @@ -22,10 +22,10 @@ def initialize(deployment_name, response) ANNOTATION = "shipit.shopify.io/restart" def initialize(context:, namespace:, logger: nil, max_watch_seconds: nil) - @task_config = KubernetesDeploy::TaskConfig.new(context, namespace, logger) + @logger = logger || KubernetesDeploy::FormattedLogger.build(namespace, context) + @task_config = KubernetesDeploy::TaskConfig.new(context, namespace, @logger) @context = context @namespace = namespace - @logger = logger || KubernetesDeploy::FormattedLogger.build(namespace, context) @max_watch_seconds = max_watch_seconds end diff --git a/lib/kubernetes-deploy/task_config_validator.rb b/lib/kubernetes-deploy/task_config_validator.rb index 5de68a93c..4cbf978f8 100644 --- a/lib/kubernetes-deploy/task_config_validator.rb +++ b/lib/kubernetes-deploy/task_config_validator.rb @@ -3,7 +3,8 @@ module KubernetesDeploy class TaskConfigValidator DEFAULT_VALIDATIONS = %i( validate_kubeconfig - validate_context_exists + validate_context_exists_in_kubeconfig + validate_context_reachable validate_namespace_exists validate_server_version ).freeze @@ -38,7 +39,7 @@ def validate_kubeconfig @errors += @kubeclient_builder.validate_config_files end - def validate_context_exists + def validate_context_exists_in_kubeconfig unless context.present? return @errors << "Context can not be blank" end @@ -52,9 +53,10 @@ def validate_context_exists else "Something went wrong. #{err} " end - return end + end + def validate_context_reachable _, err, st = @kubectl.run("get", "namespaces", "-o", "name", use_namespace: false, log_failure: false) diff --git a/test/integration/task_config_validator_test.rb b/test/integration/task_config_validator_test.rb index 65d9ec61a..ff7cdc38d 100644 --- a/test/integration/task_config_validator_test.rb +++ b/test/integration/task_config_validator_test.rb @@ -6,20 +6,28 @@ def test_valid_configuration assert_predicate(validator(context: KubeclientHelper::TEST_CONTEXT, namespace: 'default'), :valid?) end - def test_invalid_kubeconfig - assert_match(/Context test-context missing from/, validator.errors.join("\n")) - end - def test_only_is_respected validator = KubernetesDeploy::TaskConfigValidator.new(task_config, nil, nil, only: []) assert_predicate(validator, :valid?) end - def test_context_does_not_exists - assert_match("Context test-context missing from your kubeconfig file(s)", + def test_invalid_kubeconfig + bad_file = "/IM_NOT_A_REAL_FILE.yml" + builder = KubernetesDeploy::KubeclientBuilder.new(kubeconfig: bad_file) + assert_match("Kube config not found at #{bad_file}", + validator(kubeclient_builder: builder, only: [:validate_kubeconfig]).errors.join("\n")) + end + + def test_context_does_not_exists_in_kubeconfig + assert_match(/Context #{task_config.context} missing from your kubeconfig file/, validator.errors.join("\n")) end + def test_context_not_reachable + assert_match(/Something went wrong connectting to #{task_config.context}/, + validator(only: [:validate_context_reachable]).errors.join("\n")) + end + def test_namespace_does_not_exists assert_match(/Cloud not find Namespace: test-namespace in Context: #{KubeclientHelper::TEST_CONTEXT}/, validator(context: KubeclientHelper::TEST_CONTEXT).errors.join("\n")) @@ -39,12 +47,12 @@ def test_invalid_server_version private - def validator(context: nil, namespace: nil, logger: nil) + def validator(context: nil, namespace: nil, logger: nil, kubeclient_builder: nil, only: nil) config = task_config(context: context, namespace: namespace, logger: logger) kubectl = KubernetesDeploy::Kubectl.new(namespace: config.namespace, context: config.context, logger: config.logger, log_failure_by_default: true) - kubeclient_builder = KubernetesDeploy::KubeclientBuilder.new - KubernetesDeploy::TaskConfigValidator.new(config, kubectl, kubeclient_builder) + kubeclient_builder ||= KubernetesDeploy::KubeclientBuilder.new + KubernetesDeploy::TaskConfigValidator.new(config, kubectl, kubeclient_builder, only: only) end def task_config(context: nil, namespace: nil, logger: nil) From 74a8d62ae58016b1ca30be2df77028f9f98bab9e Mon Sep 17 00:00:00 2001 From: Daniel Turner Date: Thu, 29 Aug 2019 09:25:05 -0700 Subject: [PATCH 5/7] PR feedback --- lib/kubernetes-deploy/task_config_validator.rb | 2 +- test/integration/task_config_validator_test.rb | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/kubernetes-deploy/task_config_validator.rb b/lib/kubernetes-deploy/task_config_validator.rb index 4cbf978f8..f463a0acb 100644 --- a/lib/kubernetes-deploy/task_config_validator.rb +++ b/lib/kubernetes-deploy/task_config_validator.rb @@ -5,8 +5,8 @@ class TaskConfigValidator validate_kubeconfig validate_context_exists_in_kubeconfig validate_context_reachable - validate_namespace_exists validate_server_version + validate_namespace_exists ).freeze delegate :context, :namespace, :logger, to: :@task_config diff --git a/test/integration/task_config_validator_test.rb b/test/integration/task_config_validator_test.rb index ff7cdc38d..2072bda00 100644 --- a/test/integration/task_config_validator_test.rb +++ b/test/integration/task_config_validator_test.rb @@ -7,8 +7,7 @@ def test_valid_configuration end def test_only_is_respected - validator = KubernetesDeploy::TaskConfigValidator.new(task_config, nil, nil, only: []) - assert_predicate(validator, :valid?) + assert_predicate(validator(only: []), :valid?) end def test_invalid_kubeconfig From 6579cb496cc4e076d99ff46204acb3fcab6365c7 Mon Sep 17 00:00:00 2001 From: Daniel Turner Date: Thu, 29 Aug 2019 09:34:34 -0700 Subject: [PATCH 6/7] Move restart verification to a method --- lib/kubernetes-deploy/restart_task.rb | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/lib/kubernetes-deploy/restart_task.rb b/lib/kubernetes-deploy/restart_task.rb index e1425cc1e..3c483797f 100644 --- a/lib/kubernetes-deploy/restart_task.rb +++ b/lib/kubernetes-deploy/restart_task.rb @@ -41,14 +41,9 @@ def perform!(deployments_names = nil, selector: nil) @logger.reset @logger.phase_heading("Initializing restart") - 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(task_config_validator.errors.map { |err| "- #{err}" }.join("\n")) - raise KubernetesDeploy::TaskConfigurationError - end - + verify_config! deployments = identify_target_deployments(deployments_names, selector: selector) + @logger.phase_heading("Triggering restart by touching ENV[RESTARTED_AT]") patch_kubeclient_deployments(deployments) @@ -173,6 +168,15 @@ def build_patch_payload(deployment) } end + def verify_config! + 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(task_config_validator.errors.map { |err| "- #{err}" }.join("\n")) + raise KubernetesDeploy::TaskConfigurationError + end + end + def kubeclient @kubeclient ||= kubeclient_builder.build_v1_kubeclient(@context) end From 641d7a64776530dc1f5c151cd2a8f31079ea72fc Mon Sep 17 00:00:00 2001 From: Daniel Turner Date: Thu, 29 Aug 2019 12:49:37 -0700 Subject: [PATCH 7/7] SP fixes --- lib/kubernetes-deploy/kubeclient_builder.rb | 4 ++-- .../task_config_validator.rb | 4 ++-- test/integration-serial/serial_deploy_test.rb | 4 ++-- test/integration/restart_task_test.rb | 2 +- .../integration/task_config_validator_test.rb | 20 +++++++++---------- test/test_helper.rb | 4 ++++ .../kubeclient_builder_test.rb | 8 ++++---- test/unit/task_config_test.rb | 14 +++++-------- 8 files changed, 30 insertions(+), 30 deletions(-) diff --git a/lib/kubernetes-deploy/kubeclient_builder.rb b/lib/kubernetes-deploy/kubeclient_builder.rb index f4529ec1f..1d8d6a7e3 100644 --- a/lib/kubernetes-deploy/kubeclient_builder.rb +++ b/lib/kubernetes-deploy/kubeclient_builder.rb @@ -106,11 +106,11 @@ def build_storage_v1_kubeclient(context) def validate_config_files errors = [] if @kubeconfig_files.empty? - errors << "Kube config file name(s) not set in $KUBECONFIG" + errors << "Kubeconfig file name(s) not set in $KUBECONFIG" else @kubeconfig_files.each do |f| # If any files in the list are not valid, we can't be sure the merged context list is what the user intended - errors << "Kube config not found at #{f}" unless File.file?(f) + errors << "Kubeconfig not found at #{f}" unless File.file?(f) end end errors diff --git a/lib/kubernetes-deploy/task_config_validator.rb b/lib/kubernetes-deploy/task_config_validator.rb index f463a0acb..5fc334772 100644 --- a/lib/kubernetes-deploy/task_config_validator.rb +++ b/lib/kubernetes-deploy/task_config_validator.rb @@ -61,7 +61,7 @@ def validate_context_reachable use_namespace: false, log_failure: false) unless st.success? - @errors << "Something went wrong connectting to #{context}. #{err} " + @errors << "Something went wrong connecting to #{context}. #{err} " end end @@ -75,7 +75,7 @@ def validate_namespace_exists unless st.success? @errors << if err.match("Error from server [(]NotFound[)]: namespace") - "Cloud not find Namespace: #{namespace} in Context: #{context}" + "Could not find Namespace: #{namespace} in Context: #{context}" else "Could not connect to kubernetes cluster. #{err}" end diff --git a/test/integration-serial/serial_deploy_test.rb b/test/integration-serial/serial_deploy_test.rb index 29cb78a0e..b8ca2f1bd 100644 --- a/test/integration-serial/serial_deploy_test.rb +++ b/test/integration-serial/serial_deploy_test.rb @@ -75,7 +75,7 @@ def test_multiple_configuration_files assert_logs_match_all([ 'Result: FAILURE', 'Configuration invalid', - "Kube config not found at #{config_file}", + "Kubeconfig not found at #{config_file}", ], in_order: true) reset_logger @@ -85,7 +85,7 @@ def test_multiple_configuration_files assert_logs_match_all([ 'Result: FAILURE', 'Configuration invalid', - "Kube config file name(s) not set in $KUBECONFIG", + "Kubeconfig file name(s) not set in $KUBECONFIG", ], in_order: true) reset_logger diff --git a/test/integration/restart_task_test.rb b/test/integration/restart_task_test.rb index 6127e8d6a..3ce1214c9 100644 --- a/test/integration/restart_task_test.rb +++ b/test/integration/restart_task_test.rb @@ -188,7 +188,7 @@ def test_restart_not_existing_namespace assert_restart_failure(restart.perform(%w(web))) assert_logs_match_all([ "Result: FAILURE", - "- Cloud not find Namespace: walrus in Context: #{KubeclientHelper::TEST_CONTEXT}", + "- Could not find Namespace: walrus in Context: #{KubeclientHelper::TEST_CONTEXT}", ], in_order: true) end diff --git a/test/integration/task_config_validator_test.rb b/test/integration/task_config_validator_test.rb index 2072bda00..47d106dac 100644 --- a/test/integration/task_config_validator_test.rb +++ b/test/integration/task_config_validator_test.rb @@ -13,22 +13,24 @@ def test_only_is_respected def test_invalid_kubeconfig bad_file = "/IM_NOT_A_REAL_FILE.yml" builder = KubernetesDeploy::KubeclientBuilder.new(kubeconfig: bad_file) - assert_match("Kube config not found at #{bad_file}", + assert_match("Kubeconfig not found at #{bad_file}", validator(kubeclient_builder: builder, only: [:validate_kubeconfig]).errors.join("\n")) end def test_context_does_not_exists_in_kubeconfig - assert_match(/Context #{task_config.context} missing from your kubeconfig file/, - validator.errors.join("\n")) + fake_context = "fake-context" + assert_match(/Context #{fake_context} missing from your kubeconfig file/, + validator(context: fake_context).errors.join("\n")) end def test_context_not_reachable - assert_match(/Something went wrong connectting to #{task_config.context}/, - validator(only: [:validate_context_reachable]).errors.join("\n")) + fake_context = "fake-context" + assert_match(/Something went wrong connecting to #{fake_context}/, + validator(context: fake_context, only: [:validate_context_reachable]).errors.join("\n")) end def test_namespace_does_not_exists - assert_match(/Cloud not find Namespace: test-namespace in Context: #{KubeclientHelper::TEST_CONTEXT}/, + assert_match(/Could not find Namespace: test-namespace in Context: #{KubeclientHelper::TEST_CONTEXT}/, validator(context: KubeclientHelper::TEST_CONTEXT).errors.join("\n")) end @@ -47,14 +49,12 @@ def test_invalid_server_version private def validator(context: nil, namespace: nil, logger: nil, kubeclient_builder: nil, only: nil) + context ||= "test-context" + namespace ||= "test-namespace" config = task_config(context: context, namespace: namespace, logger: logger) kubectl = KubernetesDeploy::Kubectl.new(namespace: config.namespace, context: config.context, logger: config.logger, log_failure_by_default: true) kubeclient_builder ||= KubernetesDeploy::KubeclientBuilder.new KubernetesDeploy::TaskConfigValidator.new(config, kubectl, kubeclient_builder, only: only) end - - def task_config(context: nil, namespace: nil, logger: nil) - KubernetesDeploy::TaskConfig.new(context || "test-context", namespace || "test-namespace", logger) - end end diff --git a/test/test_helper.rb b/test/test_helper.rb index 48dd2eba8..959bd88da 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -218,6 +218,10 @@ def mock_output_stream end end + def task_config(context: KubeclientHelper::TEST_CONTEXT, namespace: @namespace, logger: @logger) + KubernetesDeploy::TaskConfig.new(context, namespace, logger) + end + private def log_to_real_fds? diff --git a/test/unit/kubernetes-deploy/kubeclient_builder_test.rb b/test/unit/kubernetes-deploy/kubeclient_builder_test.rb index 8a9f19724..4f93dac4d 100644 --- a/test/unit/kubernetes-deploy/kubeclient_builder_test.rb +++ b/test/unit/kubernetes-deploy/kubeclient_builder_test.rb @@ -13,14 +13,14 @@ def test_config_validation_missing_file builder = KubernetesDeploy::KubeclientBuilder.new(kubeconfig: config_file) errors = builder.validate_config_files assert_equal(1, errors.length) - assert_equal(errors.first, "Kube config not found at #{config_file}") + assert_equal(errors.first, "Kubeconfig not found at #{config_file}") end def test_build_runs_config_validation config_file = File.join(__dir__, '../../fixtures/kube-config/unknown_config.yml') kubeclient_builder = KubernetesDeploy::KubeclientBuilder.new(kubeconfig: config_file) - expected_err = /Kube config not found at .*unknown_config.yml/ + expected_err = /Kubeconfig not found at .*unknown_config.yml/ assert_raises_message(KubernetesDeploy::TaskConfigurationError, expected_err) do kubeclient_builder.build_v1_kubeclient('test-context') end @@ -30,12 +30,12 @@ def test_no_config_files_specified builder = KubernetesDeploy::KubeclientBuilder.new(kubeconfig: " : ") errors = builder.validate_config_files assert_equal(1, errors.length) - assert_equal(errors.first, "Kube config file name(s) not set in $KUBECONFIG") + assert_equal(errors.first, "Kubeconfig file name(s) not set in $KUBECONFIG") builder = KubernetesDeploy::KubeclientBuilder.new(kubeconfig: "") errors = builder.validate_config_files assert_equal(1, errors.length) - assert_equal(errors.first, "Kube config file name(s) not set in $KUBECONFIG") + assert_equal(errors.first, "Kubeconfig file name(s) not set in $KUBECONFIG") end def test_multiple_valid_configuration_files diff --git a/test/unit/task_config_test.rb b/test/unit/task_config_test.rb index 281db7ae6..af5cad964 100644 --- a/test/unit/task_config_test.rb +++ b/test/unit/task_config_test.rb @@ -3,25 +3,21 @@ class TaskConfigTest < KubernetesDeploy::TestCase def test_responds_to_namespace - assert_equal(task_config.namespace, "test-namespace") + namespace = "test-namespace" + assert_equal(task_config(namespace: namespace).namespace, namespace) end def test_responds_to_context - assert_equal(task_config.context, "test-context") + context = "test-context" + assert_equal(task_config(context: "test-context").context, context) end def test_builds_a_logger_if_none_provided - assert_equal(task_config.logger.class, KubernetesDeploy::FormattedLogger) + assert_equal(task_config(logger: nil).logger.class, KubernetesDeploy::FormattedLogger) end def test_uses_provided_logger logger = KubernetesDeploy::FormattedLogger.build(nil, nil) assert_equal(task_config(logger: logger).logger, logger) end - - private - - def task_config(context: nil, namespace: nil, logger: nil) - KubernetesDeploy::TaskConfig.new(context || "test-context", namespace || "test-namespace", logger) - end end