Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Krane global deploy #574

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 43 additions & 0 deletions lib/krane/cli/global_deploy_command.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
# frozen_string_literal: true

module Krane
module CLI
class GlobalDeployCommand
DEFAULT_DEPLOY_TIMEOUT = '300s'
OPTIONS = {
"filenames" => { type: :string, banner: '/tmp/my-resource.yml', aliases: :f, required: true,
desc: "Path to file or directory that contains the configuration to apply" },
"global-timeout" => { type: :string, banner: "duration", default: DEFAULT_DEPLOY_TIMEOUT,
desc: "Max duration to monitor workloads correctly deployed" },
"verify-result" => { type: :boolean, default: true,
desc: "Verify workloads correctly deployed" },
"selector" => { type: :string, banner: "'label=value'", required: true,
desc: "Select workloads by selector(s)" },
}

def self.from_options(context, options)
require 'krane/global_deploy_task'
require 'kubernetes-deploy/options_helper'
require 'kubernetes-deploy/label_selector'

selector = KubernetesDeploy::LabelSelector.parse(options[:selector])

KubernetesDeploy::OptionsHelper.with_processed_template_paths([options[:filenames]],
require_explicit_path: true) do |paths|
deploy = ::Krane::GlobalDeployTask.new(
context: context,
current_sha: ENV["REVISION"],
template_paths: paths,
max_watch_seconds: KubernetesDeploy::DurationParser.new(options["global-timeout"]).parse!.to_i,
selector: selector,
)

deploy.run!(
verify_result: options["verify-result"],
prune: false,
Copy link
Contributor Author

@dturn dturn Oct 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will get flipped to true in a follow-up PR #589

)
end
end
end
end
end
9 changes: 9 additions & 0 deletions lib/krane/cli/krane.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
require 'krane/cli/run_command'
require 'krane/cli/render_command'
require 'krane/cli/deploy_command'
require 'krane/cli/global_deploy_command'

module Krane
module CLI
Expand Down Expand Up @@ -58,6 +59,14 @@ def deploy(namespace, context)
end
end

desc("global-deploy CONTEXT", "Ship global resources to a cluster")
expand_options(GlobalDeployCommand::OPTIONS)
def global_deploy(context)
rescue_and_exit do
GlobalDeployCommand.from_options(context, options)
end
end

def self.exit_on_failure?
true
end
Expand Down
15 changes: 15 additions & 0 deletions lib/krane/global_deploy_task.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
# frozen_string_literal: true
require 'kubernetes-deploy/deploy_task'
require 'kubernetes-deploy/global_deploy_task_config_validator'

module Krane
class GlobalDeployTask < KubernetesDeploy::DeployTask
def initialize(**args)
super(args.merge(allow_globals: true))
end

def run!(**args)
super(args.merge(task_config_validator: KubernetesDeploy::GlobalDeployTaskConfigValidator))
end
end
end
6 changes: 4 additions & 2 deletions lib/kubernetes-deploy/cluster_resource_discovery.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ def global_resource_kinds
private

def fetch_globals
raw, _, st = kubectl.run("api-resources", "--namespaced=false", output: "wide", attempts: 5)
raw, _, st = kubectl.run("api-resources", "--namespaced=false", output: "wide",
attempts: 5, use_namespace: false)
if st.success?
rows = raw.split("\n")
header = rows[0]
Expand All @@ -42,7 +43,8 @@ def fetch_globals
end

def fetch_crds
raw_json, _, st = kubectl.run("get", "CustomResourceDefinition", output: "json", attempts: 5)
raw_json, _, st = kubectl.run("get", "CustomResourceDefinition", output: "json",
attempts: 5, use_namespace: false)
if st.success?
JSON.parse(raw_json)["items"]
else
Expand Down
52 changes: 17 additions & 35 deletions lib/kubernetes-deploy/deploy_task.rb
Original file line number Diff line number Diff line change
Expand Up @@ -122,15 +122,15 @@ def server_version
# @param protected_namespaces [Array<String>] Array of protected Kubernetes namespaces (defaults
# to KubernetesDeploy::DeployTask::PROTECTED_NAMESPACES)
# @param render_erb [Boolean] Enable ERB rendering
def initialize(namespace:, context:, current_sha:, logger: nil, kubectl_instance: nil, bindings: {},
def initialize(namespace: nil, context:, current_sha:, logger: nil, kubectl_instance: nil, bindings: {},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a big smell to me that a bunch of these arguments (bindings, namespace, protected_namespaces) aren't/shouldn't be supported for GlobalDeployTask, and the allow_globals is always true for one subclass and always false for the other (once we do the rename). It's true that this PR achieves adding the new task with a minimal amount of code. But by doing this way, you're exchanging diff minimization for cognitive overhead for future users (who need to know these oddities about the arguments) and maintainers (who need to simultaneously consider the two types of possible work being executed when changing this task). In your earlier comments you mentioned you planned to create an abstract superclass for the deploy tasks. Did that not work out?

max_watch_seconds: nil, selector: nil, template_paths: [], template_dir: nil, protected_namespaces: nil,
render_erb: true, allow_globals: false)
template_dir = File.expand_path(template_dir) if template_dir
template_paths = (template_paths.map { |path| File.expand_path(path) } << template_dir).compact

@logger = logger || KubernetesDeploy::FormattedLogger.build(namespace, context)
@template_sets = TemplateSets.from_dirs_and_files(paths: template_paths, logger: @logger)
@task_config = KubernetesDeploy::TaskConfig.new(context, namespace, @logger)
@task_config = KubernetesDeploy::TaskConfig.new(context, namespace, @logger, allow_globals && namespace.blank?)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

allow_globals && namespace.blank? is the result of having to support the legacy behavior of kubernetes-deploy.

Copy link
Contributor

@jessie-JNing jessie-JNing Oct 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we also make https://github.com/Shopify/kubernetes-deploy/pull/574/files#diff-4c33bbb177fdf882c41d892f3bde9570R144 be allow_globals && namespace.blank?

I'm not very sure about the @allow_globals and @global_mode in TaskConfig.

But, this PR looks good to me 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@allow_globals at this point is only used to determine if we need to display a warning when you use exe/kubernetes-deploy with a global resource. We should be able to kill it when we delete exe/kubernetes-deploy.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Thanks for the explanation.

@bindings = bindings
@namespace = namespace
@namespace_tags = []
Expand Down Expand Up @@ -161,14 +161,17 @@ def run(*args)
# @param prune [Boolean] Enable deletion of resources that do not appear in the template dir
#
# @return [nil]
def run!(verify_result: true, allow_protected_ns: false, prune: true)
def run!(verify_result: true, allow_protected_ns: false, prune: true,
task_config_validator: DeployTaskConfigValidator)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The validator selection is an internal detail of the class and should not be configurable by users. This is another symptom of not implementing the abstract superclass (or two separate tasks that leverage common components, or some other option that gives us separation in the code).

start = Time.now.utc
@logger.reset

@logger.phase_heading("Initializing deploy")
validate_configuration(allow_protected_ns: allow_protected_ns, prune: prune)
validator = validate_configuration(allow_protected_ns: allow_protected_ns, prune: prune,
task_config_validator: task_config_validator)

resources = discover_resources
validate_resources(resources)
validate_resources(resources, validator)

@logger.phase_heading("Checking initial resource statuses")
check_initial_status(resources)
Expand Down Expand Up @@ -280,7 +283,9 @@ def predeploy_priority_resources(resource_list)
end
measure_method(:predeploy_priority_resources, 'priority_resources.duration')

def validate_resources(resources)
def validate_resources(resources, validator)
validator.validate_resources(resources, @allow_globals)

KubernetesDeploy::Concurrency.split_across_threads(resources) do |r|
r.validate_definition(kubectl, selector: @selector)
end
Expand All @@ -298,28 +303,9 @@ def validate_resources(resources)
end
raise FatalDeploymentError, "Template validation failed"
end
validate_globals(resources)
end
measure_method(:validate_resources)

def validate_globals(resources)
return unless (global = resources.select(&:global?).presence)
global_names = global.map do |resource|
"#{resource.name} (#{resource.type}) in #{File.basename(resource.file_path)}"
end
global_names = FormattedLogger.indent_four(global_names.join("\n"))

if @allow_globals
msg = "The ability for this task to deploy global resources will be removed in the next version,"\
" which will affect the following resources:"
msg += "\n#{global_names}"
@logger.summary.add_paragraph(ColorizedString.new(msg).yellow)
else
@logger.summary.add_paragraph(ColorizedString.new("Global resources:\n#{global_names}").yellow)
raise FatalDeploymentError, "This command is namespaced and cannot be used to deploy global resources."
end
end

def check_initial_status(resources)
cache = ResourceCache.new(@task_config)
KubernetesDeploy::Concurrency.split_across_threads(resources) { |r| r.sync(cache) }
Expand Down Expand Up @@ -375,8 +361,8 @@ def record_warnings(warning:, filename:)
@logger.summary.add_paragraph(ColorizedString.new(warn_msg).yellow)
end

def validate_configuration(allow_protected_ns:, prune:)
task_config_validator = DeployTaskConfigValidator.new(@protected_namespaces, allow_protected_ns, prune,
def validate_configuration(allow_protected_ns:, prune:, task_config_validator:)
task_config_validator = task_config_validator.new(@protected_namespaces, allow_protected_ns, prune,
@task_config, kubectl, kubeclient_builder)
errors = []
errors += task_config_validator.errors
Expand All @@ -391,6 +377,7 @@ def validate_configuration(allow_protected_ns:, prune:)
@logger.info("Using resource selector #{@selector}") if @selector
@namespace_tags |= tags_from_namespace_labels
@logger.info("All required parameters and files are present")
task_config_validator
end
measure_method(:validate_configuration)

Expand Down Expand Up @@ -474,7 +461,8 @@ def apply_all(resources, prune)
end

output_is_sensitive = resources.any?(&:sensitive_template_content?)
out, err, st = kubectl.run(*command, log_failure: false, output_is_sensitive: output_is_sensitive)
out, err, st = kubectl.run(*command, log_failure: false, output_is_sensitive: output_is_sensitive,
use_namespace: !@task_config.global_mode)

if st.success?
log_pruning(out) if prune
Expand Down Expand Up @@ -555,13 +543,7 @@ def find_bad_files_from_kubectl_output(line)
end

def namespace_definition
@namespace_definition ||= begin
definition, _err, st = kubectl.run("get", "namespace", @namespace, use_namespace: false,
log_failure: true, raise_if_not_found: true, attempts: 3, output: 'json')
st.success? ? JSON.parse(definition, symbolize_names: true) : nil
end
rescue Kubectl::ResourceNotFoundError
nil
@task_config.namespace_definition
end

# make sure to never prune the ejson-keys secret
Expand Down
18 changes: 18 additions & 0 deletions lib/kubernetes-deploy/deploy_task_config_validator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,24 @@ def initialize(protected_namespaces, allow_protected_ns, prune, *arguments)
@validations += %i(validate_protected_namespaces)
end

def validate_resources(resources, allow_globals)
return unless (global = resources.select(&:global?).presence)
global_names = global.map do |resource|
"#{resource.name} (#{resource.type}) in #{File.basename(resource.file_path)}"
end
global_names = FormattedLogger.indent_four(global_names.join("\n"))

if allow_globals
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Especially since the big rename PR looks so imminent, I'd suggest waiting to merge the globals command into a version that doesn't need to support the legacy behaviour. If you're using a version where we're giving you a deploy command, you should absolutely not be using the regular deploy command to ship globals.

msg = "The ability for this task to deploy global resources will be removed in the next version,"\
" which will affect the following resources:"
msg += "\n#{global_names}"
logger.summary.add_paragraph(ColorizedString.new(msg).yellow)
else
logger.summary.add_paragraph(ColorizedString.new("Global resources:\n#{global_names}").yellow)
raise FatalDeploymentError, "This command is namespaced and cannot be used to deploy global resources."
end
end

private

def validate_protected_namespaces
Expand Down
22 changes: 22 additions & 0 deletions lib/kubernetes-deploy/global_deploy_task_config_validator.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
# frozen_string_literal: true
module KubernetesDeploy
class GlobalDeployTaskConfigValidator < TaskConfigValidator
def initialize(protected_namespaces, allow_protected_ns, prune, *arguments)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also validate that the selector is present, or do we get that for free? Specifically, what happens if you pass in an empty string selector, either via CLI or via ruby? It wasn't required on the regular command, so we might not have cared to check.

super(*arguments, skip: [:validate_namespace_exists])
@protected_namespaces = protected_namespaces
@allow_protected_ns = allow_protected_ns
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two vars don't make any sense for the global command

@prune = prune
end

def validate_resources(resources, _)
return unless (namespaced = resources.reject(&:global?).presence)
namespaced_names = namespaced.map do |resource|
"#{resource.name} (#{resource.type}) in #{File.basename(resource.file_path)}"
end
namespaced_names = KubernetesDeploy::FormattedLogger.indent_four(namespaced_names.join("\n"))

logger.summary.add_paragraph(ColorizedString.new("Namespaced resources:\n#{namespaced_names}").yellow)
raise KubernetesDeploy::FatalDeploymentError, "Deploying namespaced resource is not allowed from this command."
end
end
end
4 changes: 2 additions & 2 deletions lib/kubernetes-deploy/kubernetes_resource.rb
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,7 @@ def debug_message(cause = nil, info_hash = {})
def fetch_events(kubectl)
return {} unless exists?
out, _err, st = kubectl.run("get", "events", "--output=go-template=#{Event.go_template_for(type, name)}",
log_failure: false)
log_failure: false, use_namespace: !global?)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 It surprises me, but is true, that events can be both namespaced and not. Are we actually formulating a query that works here? Just removing the namespace arg doesn't seem to make kubectl fetch the non-namespaced version:

kubectl get events -v=9
GET https://kubernetes.docker.internal:6443/api/v1/namespaces/default/events?limit=500 200 OK in 19 milliseconds

return {} unless st.success?

event_collector = Hash.new { |hash, key| hash[key] = [] }
Expand Down Expand Up @@ -500,7 +500,7 @@ def validate_spec_with_kubectl(kubectl)
def validate_with_dry_run_option(kubectl, dry_run_option)
command = ["apply", "-f", file_path, dry_run_option, "--output=name"]
kubectl.run(*command, log_failure: false, output_is_sensitive: sensitive_template_content?,
retry_whitelist: [:client_timeout], attempts: 3)
retry_whitelist: [:client_timeout], attempts: 3, use_namespace: !global?)
end

def labels
Expand Down
4 changes: 2 additions & 2 deletions lib/kubernetes-deploy/resource_cache.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

module KubernetesDeploy
class ResourceCache
delegate :namespace, :context, :logger, to: :@task_config
delegate :namespace, :context, :logger, :global_mode, to: :@task_config

def initialize(task_config)
@task_config = task_config
Expand Down Expand Up @@ -53,7 +53,7 @@ def fetch_by_kind(kind)
resource_class = KubernetesResource.class_for_kind(kind)
output_is_sensitive = resource_class.nil? ? false : resource_class::SENSITIVE_TEMPLATE_CONTENT
raw_json, _, st = @kubectl.run("get", kind, "--chunk-size=0", attempts: 5, output: "json",
output_is_sensitive: output_is_sensitive)
output_is_sensitive: output_is_sensitive, use_namespace: !global_mode)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't necessarily an on/off mode for the cache. It needs to be on a per-resource basis. To give a concrete example, Dale's PR needs to fetch nodes for the purposes of validating daemonSet deploys.

raise KubectlError unless st.success?

instances = {}
Expand Down
6 changes: 4 additions & 2 deletions lib/kubernetes-deploy/task_config.rb
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
# frozen_string_literal: true
module KubernetesDeploy
class TaskConfig
attr_reader :context, :namespace
attr_reader :context, :namespace, :global_mode
attr_accessor :namespace_definition

def initialize(context, namespace, logger = nil)
def initialize(context, namespace, logger = nil, global_mode = false)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is "global mode" anything beyond the absence of the namespace?

@context = context
@namespace = namespace
@logger = logger
@global_mode = global_mode
end

def logger
Expand Down
12 changes: 7 additions & 5 deletions lib/kubernetes-deploy/task_config_validator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,12 @@ class TaskConfigValidator

delegate :context, :namespace, :logger, to: :@task_config

def initialize(task_config, kubectl, kubeclient_builder, only: nil)
def initialize(task_config, kubectl, kubeclient_builder, only: nil, skip: [])
@task_config = task_config
@kubectl = kubectl
@kubeclient_builder = kubeclient_builder
@errors = nil
@validations = only || DEFAULT_VALIDATIONS
@validations = (only || DEFAULT_VALIDATIONS) - skip
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm kinda surprised to see that in addition to subclassing, we also need the ability to modify the defaults with both only and skip options. Am I reading correctly that the only option is just used by tests? Could this work:

class TaskConfigValidator
  def validations
    DEFAULT_VALIDATIONS
  end
end

class SomeValidator < TaskConfigValidator
  def validations
    DEFAULT_VALIDATIONS - [:validation_to_skip] # or + something, or just a specific list
  end
end

Then the superclass isn't messing around with constructing the list, when it should in practice be a static property of each subclass.

end

def valid?
Expand Down Expand Up @@ -70,10 +70,12 @@ def validate_namespace_exists
return @errors << "Namespace can not be blank"
end

_, err, st = @kubectl.run("get", "namespace", "-o", "name", namespace,
use_namespace: false, log_failure: false)
definition, err, st = @kubectl.run("get", "namespace", namespace,
use_namespace: false, log_failure: false, attempts: 3, output: 'json')

unless st.success?
if st.success?
@task_config.namespace_definition = JSON.parse(definition, symbolize_names: true)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It isn't intuitive that validate_namespace_exists would have the side-effect of setting a property on an entirely different object. Could we make task_config capable of caching/returning the namespace object, and raising errors we can catch here?

else
@errors << if err.match("Error from server [(]NotFound[)]: namespace")
"Could not find Namespace: #{namespace} in Context: #{context}"
else
Expand Down
Loading