From 92328603de6fe8255d5a8507294928a0d765ff5c Mon Sep 17 00:00:00 2001 From: Joseph Fourny Date: Fri, 21 Aug 2020 16:30:34 -0400 Subject: [PATCH] CLI patches Psych for K8s compatible YAML serialization Fixes issue: https://github.com/Shopify/krane/issues/740 --- lib/krane/cli/krane.rb | 3 + lib/krane/common.rb | 1 + lib/krane/psych_k8s_patch.rb | 62 ++++++++++++ .../pod_with_e_notation_env.yml | 34 +++++++ test/unit/krane/psych_k8s_patch_test.rb | 68 +++++++++++++ test/unit/krane/renderer_test.rb | 97 +++++++++++++++++++ 6 files changed, 265 insertions(+) create mode 100644 lib/krane/psych_k8s_patch.rb create mode 100644 test/fixtures/for_unit_tests/pod_with_e_notation_env.yml create mode 100644 test/unit/krane/psych_k8s_patch_test.rb diff --git a/lib/krane/cli/krane.rb b/lib/krane/cli/krane.rb index 5d0763448..065dcc41e 100644 --- a/lib/krane/cli/krane.rb +++ b/lib/krane/cli/krane.rb @@ -17,6 +17,9 @@ class Krane < Thor package_name "Krane" + # Hack: CLI activates patch of Psych to ensure serialized YAML is K8s compatible. + PsychK8sPatch.activate + def self.expand_options(task_options) task_options.each { |option_name, config| method_option(option_name, config) } end diff --git a/lib/krane/common.rb b/lib/krane/common.rb index 028f304b6..a02499752 100644 --- a/lib/krane/common.rb +++ b/lib/krane/common.rb @@ -17,6 +17,7 @@ require 'krane/statsd' require 'krane/task_config' require 'krane/task_config_validator' +require 'krane/psych_k8s_patch' module Krane MIN_KUBE_VERSION = '1.11.0' diff --git a/lib/krane/psych_k8s_patch.rb b/lib/krane/psych_k8s_patch.rb new file mode 100644 index 000000000..7c5ad71c6 --- /dev/null +++ b/lib/krane/psych_k8s_patch.rb @@ -0,0 +1,62 @@ +# frozen_string_literal: true + +require 'psych' + +# PsychK8sPatch applies patching to Psych.dump/dump_stream with an alternative string serializer that is more compatible +# with Kubernetes (and other Go-based tooling). Related issue: https://github.com/Shopify/krane/issues/740 +module PsychK8sPatch + # Activate will apply the patch after creating backup aliases for the original methods. + def self.activate + class << Psych + raise "Patch was already activated!" if @activated + @activated = true + alias_method :__orig__dump, :dump + alias_method :__orig__dump_stream, :dump_stream + + def dump(o, io = nil, options = {}) + if io.is_a?(Hash) + options = io + io = nil + end + + visitor = Psych::Visitors::YAMLTree.create(options) + visitor << o + visitor.tree.each { |n| PsychK8sPatch.massage_node(n) } + visitor.tree.yaml(io, options) + end + + def dump_stream(*objects) + visitor = Psych::Visitors::YAMLTree.create({}) + objects.each do |o| + visitor << o + end + visitor.tree.each { |n| PsychK8sPatch.massage_node(n) } + visitor.tree.yaml + end + end + end + + # Deactivate will restore the original methods from backup aliases. + def self.deactivate + class << Psych + raise "Patch was not activated!" if !@activated + @activated = false + alias_method :dump, :__orig__dump + alias_method :dump_stream, :__orig__dump_stream + end + end + + private + + # fix_node applies DOUBLE_QUOTED style to string scalars that look like scientific/e-notation numbers. + # This is required by YAML 1.2. Failure to do so results in Go-based tools (ie: K8s) to interpret as number! + def self.massage_node(n) + if n.is_a?(Psych::Nodes::Scalar) && + (n.style == Psych::Nodes::Scalar::PLAIN) && + n.value.is_a?(String) && + n.value =~ /\A[+-]?\d+(?:\.\d+)?[eE][+-]?\d+\z/ + + n.style = Psych::Nodes::Scalar::DOUBLE_QUOTED + end + end +end diff --git a/test/fixtures/for_unit_tests/pod_with_e_notation_env.yml b/test/fixtures/for_unit_tests/pod_with_e_notation_env.yml new file mode 100644 index 000000000..c2ef88467 --- /dev/null +++ b/test/fixtures/for_unit_tests/pod_with_e_notation_env.yml @@ -0,0 +1,34 @@ +--- +apiVersion: apps/v1 +kind: Deployment +metadata: + name: web + annotations: + shipit.shopify.io/restart: "true" + labels: + name: web + app: hello-cloud +spec: + replicas: 1 + selector: + matchLabels: + name: web + app: hello-cloud + progressDeadlineSeconds: 60 + template: + metadata: + labels: + name: web + app: hello-cloud + spec: + containers: + - name: app + image: busybox + imagePullPolicy: IfNotPresent + command: ["tail", "-f", "/dev/null"] + ports: + - containerPort: 80 + name: http + env: + - name: FOO + value: "123e4" diff --git a/test/unit/krane/psych_k8s_patch_test.rb b/test/unit/krane/psych_k8s_patch_test.rb new file mode 100644 index 000000000..31a1be919 --- /dev/null +++ b/test/unit/krane/psych_k8s_patch_test.rb @@ -0,0 +1,68 @@ +# frozen_string_literal: true +require 'test_helper' + +class PsychK8sPatchTest < Krane::TestCase + + INPUTS = [ + 'a: "123e4"', + 'a: "123E4"', + 'a: "+123e4"', + 'a: "-123e4"', + 'a: "123e+4"', + 'a: "123e-4"', + 'a: "123.0e-4"' + ] + + EXPECTED_DEACTIVATED = [ + %(---\n- a: 123e4\n), # Psych sees this as non-numeric; deviates from YAML 1.2 spec :( + %(---\n- a: 123E4\n), # Psych sees this as non-numeric; deviates from YAML 1.2 spec :( + %(---\n- a: "+123e4"\n), # Psych sees this as non-numeric; deviates from YAML 1.2 spec; quoted due to '+' :| + %(---\n- a: "-123e4"\n), # Psych sees this as non-numeric; deviates from YAML 1.2 spec; quoted due to '-' :| + %(---\n- a: 123e+4\n), # Psych sees this as non-numeric; deviates from YAML 1.2 spec :( + %(---\n- a: 123e-4\n), # Psych sees this as non-numeric; deviates from YAML 1.2 spec :( + %(---\n- a: '123.0e-4'\n), # Psych sees this as numeric; encapsulated with single quotes :) + ] + + EXPECTED_ACTIVATED = [ + %(---\n- a: "123e4"\n), + %(---\n- a: "123E4"\n), + %(---\n- a: "+123e4"\n), + %(---\n- a: "-123e4"\n), + %(---\n- a: "123e+4"\n), + %(---\n- a: "123e-4"\n), + %(---\n- a: '123.0e-4'\n), + ] + + def test_dump + run_all_test_cases(->(n) { Psych.dump(n) }) + end + + def test_dump_stream + run_all_test_cases(->(n) { Psych.dump_stream(n) }) + end + + def test_to_yaml + run_all_test_cases(->(n) { n.to_yaml }) + end + + def run_all_test_cases(serializer) + run_test_cases(INPUTS, EXPECTED_DEACTIVATED, serializer) + run_test_cases_activated(INPUTS, EXPECTED_ACTIVATED, serializer) + end + + def run_test_cases(inputs, expectations, serializer) + (0..inputs.length - 1).each { |i| + loaded = YAML.load_stream(inputs[i]) + assert_equal(expectations[i].strip, serializer.call(loaded).strip) + } + end + + def run_test_cases_activated(inputs, expectations, serializer) + PsychK8sPatch.activate + begin + run_test_cases(inputs, expectations, serializer) + ensure + PsychK8sPatch.deactivate + end + end +end diff --git a/test/unit/krane/renderer_test.rb b/test/unit/krane/renderer_test.rb index 67b7831b6..9730ae501 100644 --- a/test/unit/krane/renderer_test.rb +++ b/test/unit/krane/renderer_test.rb @@ -43,6 +43,103 @@ def test_can_render_template_with_correct_indentation assert_equal(expected, actual) end + def test_renders_env_with_e_notation_incorrectly_when_psych_patch_deactivated + expected = <<~EOY + --- + apiVersion: apps/v1 + kind: Deployment + metadata: + name: web + annotations: + shipit.shopify.io/restart: 'true' + labels: + name: web + app: hello-cloud + spec: + replicas: 1 + selector: + matchLabels: + name: web + app: hello-cloud + progressDeadlineSeconds: 60 + template: + metadata: + labels: + name: web + app: hello-cloud + spec: + containers: + - name: app + image: busybox + imagePullPolicy: IfNotPresent + command: + - tail + - "-f" + - "/dev/null" + ports: + - containerPort: 80 + name: http + env: + - name: FOO + value: 123e4 + EOY + actual = YAML.load_stream(render("pod_with_e_notation_env.yml")).map do |t| + YAML.dump(t) + end.join + assert_equal(expected, actual) + end + + def test_renders_env_with_e_notation_correctly_when_psych_patch_activated + expected = <<~EOY + --- + apiVersion: apps/v1 + kind: Deployment + metadata: + name: web + annotations: + shipit.shopify.io/restart: 'true' + labels: + name: web + app: hello-cloud + spec: + replicas: 1 + selector: + matchLabels: + name: web + app: hello-cloud + progressDeadlineSeconds: 60 + template: + metadata: + labels: + name: web + app: hello-cloud + spec: + containers: + - name: app + image: busybox + imagePullPolicy: IfNotPresent + command: + - tail + - "-f" + - "/dev/null" + ports: + - containerPort: 80 + name: http + env: + - name: FOO + value: "123e4" + EOY + PsychK8sPatch.activate + begin + actual = YAML.load_stream(render("pod_with_e_notation_env.yml")).map do |t| + YAML.dump(t) + end.join + ensure + PsychK8sPatch.deactivate + end + assert_equal(expected, actual) + end + def test_invalid_partial_raises err = assert_raises(Krane::InvalidTemplateError) do render('broken-partial-inclusion.yaml.erb')