From f73800ae03d7dd94cf62d8eefd661c33f0092136 Mon Sep 17 00:00:00 2001 From: Ben Langfeld Date: Wed, 27 Feb 2019 08:35:39 -0300 Subject: [PATCH] Secrets as resources review fixes (#425) Thanks so much @benlangfeld for your help with these: * Remove excess logging * Stop referring to EJSON secrets as generically "managed" * Consistent timeout for Secret resources Unifying the constant used for simple resources of this type is left as an exercise for another change, mostly because the name of such a thing may be controversial and I don't want to block merging this on detailed review. * Give secrets the same statsd tags as other resources * Removes duplicate spec Doesn't test anything more than test_create_and_update_secrets_from_ejson * Replaces log assertion * Point out breaking changes in CHANGELOG * Include ejson generated secrets in discovery log --- CHANGELOG.md | 5 ++- lib/kubernetes-deploy/deploy_task.rb | 6 +++- .../ejson_secret_provisioner.rb | 24 +++++++------- .../kubernetes_resource/secret.rb | 2 +- test/helpers/fixture_set.rb | 4 +-- test/helpers/fixture_sets/ejson_cloud.rb | 6 ++-- test/integration/kubernetes_deploy_test.rb | 33 ++++++++----------- .../ejson_secret_provisioner_test.rb | 13 ++++---- 8 files changed, 46 insertions(+), 47 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e1c614068..5f1bf1b7b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,7 +1,10 @@ ## next *Features* -- Support for deploying Secrets from templates ([#424](https://github.com/Shopify/kubernetes-deploy/pull/424)). +- **[Breaking change]** Support for deploying Secrets from templates ([#424](https://github.com/Shopify/kubernetes-deploy/pull/424)). + * If you previously used this gem to deploy secrets from EJSON and the first time commit you deploy using this version removes one or more of them, they will not be pruned. + * If you previously manually `kubectl apply`'d secrets that are not passed to kubernetes-deploy, your first deploy using this version is going to delete them. + * If you previously passed secrets manifests to kubernetes-deploy and they are no longer in the set you pass to the first deploy using this version, it will delete them. *Bug fixes* - Attempting to deploy from a directory that only contains `secrets.ejson` will no longer fail deploy ([#416](https://github.com/Shopify/kubernetes-deploy/pull/416)) diff --git a/lib/kubernetes-deploy/deploy_task.rb b/lib/kubernetes-deploy/deploy_task.rb index 071631afd..0119bba1e 100644 --- a/lib/kubernetes-deploy/deploy_task.rb +++ b/lib/kubernetes-deploy/deploy_task.rb @@ -248,6 +248,7 @@ def secrets_from_ejson context: @context, template_dir: @template_dir, logger: @logger, + statsd_tags: @namespace_tags ) unless ejson.resources.empty? @@ -272,11 +273,14 @@ def discover_resources @logger.info(" - #{r.id}") end end + secrets_from_ejson.each do |secret| + resources << secret + @logger.info(" - #{secret.id} (from ejson)") + end if (global = resources.select(&:global?).presence) @logger.warn("Detected non-namespaced #{'resource'.pluralize(global.count)} which will never be pruned:") global.each { |r| @logger.warn(" - #{r.id}") } end - resources += secrets_from_ejson end measure_method(:discover_resources) diff --git a/lib/kubernetes-deploy/ejson_secret_provisioner.rb b/lib/kubernetes-deploy/ejson_secret_provisioner.rb index b41039d87..6347ec041 100644 --- a/lib/kubernetes-deploy/ejson_secret_provisioner.rb +++ b/lib/kubernetes-deploy/ejson_secret_provisioner.rb @@ -7,21 +7,22 @@ module KubernetesDeploy class EjsonSecretError < FatalDeploymentError def initialize(msg) - super("Creation of Kubernetes secrets from ejson failed: #{msg}") + super("Generation of Kubernetes secrets from ejson failed: #{msg}") end end class EjsonSecretProvisioner - MANAGEMENT_ANNOTATION = "kubernetes-deploy.shopify.io/ejson-secret" - MANAGED_SECRET_EJSON_KEY = "kubernetes_secrets" + EJSON_SECRET_ANNOTATION = "kubernetes-deploy.shopify.io/ejson-secret" + EJSON_SECRET_KEY = "kubernetes_secrets" EJSON_SECRETS_FILE = "secrets.ejson" EJSON_KEYS_SECRET = "ejson-keys" - def initialize(namespace:, context:, template_dir:, logger:) + def initialize(namespace:, context:, template_dir:, logger:, statsd_tags:) @namespace = namespace @context = context @ejson_file = "#{template_dir}/#{EJSON_SECRETS_FILE}" @logger = logger + @statsd_tags = statsd_tags @kubectl = Kubectl.new( namespace: @namespace, context: @context, @@ -32,17 +33,17 @@ def initialize(namespace:, context:, template_dir:, logger:) end def resources - @resources ||= create_secrets + @resources ||= build_secrets end private - def create_secrets + def build_secrets return [] unless File.exist?(@ejson_file) with_decrypted_ejson do |decrypted| - secrets = decrypted[MANAGED_SECRET_EJSON_KEY] + secrets = decrypted[EJSON_SECRET_KEY] unless secrets.present? - @logger.warn("#{EJSON_SECRETS_FILE} does not have key #{MANAGED_SECRET_EJSON_KEY}."\ + @logger.warn("#{EJSON_SECRETS_FILE} does not have key #{EJSON_SECRET_KEY}."\ "No secrets will be created.") return [] end @@ -95,13 +96,13 @@ def generate_secret_resource(secret_name, secret_type, data) "name" => secret_name, "labels" => { "name" => secret_name }, "namespace" => @namespace, - "annotations" => { MANAGEMENT_ANNOTATION => "true" }, + "annotations" => { EJSON_SECRET_ANNOTATION => "true" }, }, "data" => encoded_data, } KubernetesDeploy::Secret.build( - namespace: @namespace, context: @context, logger: @logger, definition: secret, statsd_tags: [], + namespace: @namespace, context: @context, logger: @logger, definition: secret, statsd_tags: @statsd_tags, ) end @@ -123,7 +124,6 @@ def with_decrypted_ejson end def decrypt_ejson(key_dir) - @logger.info("Decrypting #{EJSON_SECRETS_FILE}") # ejson seems to dump both errors and output to STDOUT out_err, st = Open3.capture2e("EJSON_KEYDIR=#{key_dir} ejson decrypt #{@ejson_file}") raise EjsonSecretError, out_err unless st.success? @@ -133,8 +133,6 @@ def decrypt_ejson(key_dir) end def fetch_private_key_from_secret - @logger.info("Fetching ejson private key from secret #{EJSON_KEYS_SECRET}") - secret = run_kubectl_json("get", "secret", EJSON_KEYS_SECRET) encoded_private_key = secret["data"][public_key] unless encoded_private_key diff --git a/lib/kubernetes-deploy/kubernetes_resource/secret.rb b/lib/kubernetes-deploy/kubernetes_resource/secret.rb index 5ea8a1a1a..6c38786cf 100644 --- a/lib/kubernetes-deploy/kubernetes_resource/secret.rb +++ b/lib/kubernetes-deploy/kubernetes_resource/secret.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module KubernetesDeploy class Secret < KubernetesResource - TIMEOUT = 10.seconds + TIMEOUT = 30.seconds KUBECTL_OUTPUT_IS_SENSITIVE = true def status diff --git a/test/helpers/fixture_set.rb b/test/helpers/fixture_set.rb index 6270b9794..45726f405 100644 --- a/test/helpers/fixture_set.rb +++ b/test/helpers/fixture_set.rb @@ -123,10 +123,10 @@ def assert_pod_templates_present(cm_name) assert_equal(1, pod_templates.size, "Expected 1 podtemplate, got #{pod_templates.size}") end - def assert_secret_present(secret_name, expected_data = nil, type: 'Opaque', managed: false) + def assert_secret_present(secret_name, expected_data = nil, type: 'Opaque', ejson: false) secret = kubeclient.get_secret(secret_name, namespace) refute_nil(secret, "Secret `#{secret_name}` not found") - assert_annotated(secret, KubernetesDeploy::EjsonSecretProvisioner::MANAGEMENT_ANNOTATION) if managed + assert_annotated(secret, KubernetesDeploy::EjsonSecretProvisioner::EJSON_SECRET_ANNOTATION) if ejson assert_equal(type, secret["type"]) return unless expected_data diff --git a/test/helpers/fixture_sets/ejson_cloud.rb b/test/helpers/fixture_sets/ejson_cloud.rb index ffe330b2a..0c4c504ac 100644 --- a/test/helpers/fixture_sets/ejson_cloud.rb +++ b/test/helpers/fixture_sets/ejson_cloud.rb @@ -37,12 +37,12 @@ def catphotoscom_key_value def assert_all_secrets_present assert_secret_present("ejson-keys") cert_data = { "tls.crt" => "this-is-the-certificate", "tls.key" => catphotoscom_key_value } - assert_secret_present("catphotoscom", cert_data, type: "kubernetes.io/tls", managed: true) + assert_secret_present("catphotoscom", cert_data, type: "kubernetes.io/tls", ejson: true) token_data = { "api-token" => "this-is-the-api-token", "service" => "Datadog" } # Impt: it isn't _service: Datadog - assert_secret_present("monitoring-token", token_data, managed: true) + assert_secret_present("monitoring-token", token_data, ejson: true) - assert_secret_present("unused-secret", { "this-is-for-testing-deletion" => "true" }, managed: true) + assert_secret_present("unused-secret", { "this-is-for-testing-deletion" => "true" }, ejson: true) end def assert_web_resources_up diff --git a/test/integration/kubernetes_deploy_test.rb b/test/integration/kubernetes_deploy_test.rb index e9769494d..72b08fd4e 100644 --- a/test/integration/kubernetes_deploy_test.rb +++ b/test/integration/kubernetes_deploy_test.rb @@ -341,16 +341,6 @@ def test_output_of_failed_unmanaged_pod ], in_order: true) end - def test_deployment_includes_ejson_secrets - ejson_cloud = FixtureSetAssertions::EjsonCloud.new(@namespace) - ejson_cloud.create_ejson_keys_secret - assert_deploy_success(deploy_fixtures("ejson-cloud")) - ejson_cloud.assert_secret_present('unused-secret', managed: true) - assert_logs_match_all([ - %r{Secret\/catphotoscom\s+Available}, - ], in_order: true) - end - def test_deployment_container_mounting_secret_that_does_not_exist_as_env_var_fails_quickly result = deploy_fixtures("ejson-cloud", subset: ["web.yaml"]) do |fixtures| # exclude secret ejson # Remove the volumes. Right now Kubernetes does not expose a useful status when mounting fails. :( @@ -532,14 +522,16 @@ def test_create_ejson_secrets_with_malformed_secret_data fixtures["secrets.ejson"]["kubernetes_secrets"]["monitoring-token"]["data"] = malformed end assert_deploy_failure(result) - assert_logs_match("Creation of kubernetes secrets from ejson failed: data for secret monitoring-token was invalid") + assert_logs_match( + "Generation of kubernetes secrets from ejson failed: data for secret monitoring-token was invalid" + ) end def test_pruning_of_secrets_created_from_ejson ejson_cloud = FixtureSetAssertions::EjsonCloud.new(@namespace) ejson_cloud.create_ejson_keys_secret assert_deploy_success(deploy_fixtures("ejson-cloud")) - ejson_cloud.assert_secret_present('unused-secret', managed: true) + ejson_cloud.assert_secret_present('unused-secret', ejson: true) result = deploy_fixtures("ejson-cloud") do |fixtures| fixtures["secrets.ejson"]["kubernetes_secrets"].delete("unused-secret") @@ -550,10 +542,10 @@ def test_pruning_of_secrets_created_from_ejson # The removed secret was pruned ejson_cloud.refute_resource_exists('secret', 'unused-secret') # The remaining secrets exist - ejson_cloud.assert_secret_present('monitoring-token', managed: true) - ejson_cloud.assert_secret_present('catphotoscom', type: 'kubernetes.io/tls', managed: true) + ejson_cloud.assert_secret_present('monitoring-token', ejson: true) + ejson_cloud.assert_secret_present('catphotoscom', type: 'kubernetes.io/tls', ejson: true) # The unmanaged secret was not pruned - ejson_cloud.assert_secret_present('ejson-keys', managed: false) + ejson_cloud.assert_secret_present('ejson-keys', ejson: false) end def test_pruning_of_existing_managed_secrets_when_ejson_file_has_been_deleted @@ -586,6 +578,7 @@ def test_can_deploy_template_dir_with_only_secrets_ejson ejson_cloud.create_ejson_keys_secret assert_deploy_success(deploy_fixtures("ejson-cloud", subset: ["secrets.ejson"])) assert_logs_match_all([ + "Generated 3 kubernetes secrets from secrets.ejson", "Result: SUCCESS", %r{Secret\/catphotoscom\s+Available}, %r{Secret\/unused-secret\s+Available}, @@ -905,7 +898,7 @@ def test_ejson_secrets_respects_no_prune_flag ejson_cloud = FixtureSetAssertions::EjsonCloud.new(@namespace) ejson_cloud.create_ejson_keys_secret assert_deploy_success(deploy_fixtures("ejson-cloud")) - ejson_cloud.assert_secret_present('unused-secret', managed: true) + ejson_cloud.assert_secret_present('unused-secret', ejson: true) result = deploy_fixtures("ejson-cloud", prune: false) do |fixtures| fixtures["secrets.ejson"]["kubernetes_secrets"].delete("unused-secret") @@ -913,11 +906,11 @@ def test_ejson_secrets_respects_no_prune_flag assert_deploy_success(result) # The removed secret was not pruned - ejson_cloud.assert_secret_present('unused-secret', managed: true) + ejson_cloud.assert_secret_present('unused-secret', ejson: true) # The remaining secrets also exist - ejson_cloud.assert_secret_present('monitoring-token', managed: true) - ejson_cloud.assert_secret_present('catphotoscom', type: 'kubernetes.io/tls', managed: true) - ejson_cloud.assert_secret_present('ejson-keys', managed: false) + ejson_cloud.assert_secret_present('monitoring-token', ejson: true) + ejson_cloud.assert_secret_present('catphotoscom', type: 'kubernetes.io/tls', ejson: true) + ejson_cloud.assert_secret_present('ejson-keys', ejson: false) end def test_partials diff --git a/test/unit/kubernetes-deploy/ejson_secret_provisioner_test.rb b/test/unit/kubernetes-deploy/ejson_secret_provisioner_test.rb index f14bf666d..7d23e167a 100644 --- a/test/unit/kubernetes-deploy/ejson_secret_provisioner_test.rb +++ b/test/unit/kubernetes-deploy/ejson_secret_provisioner_test.rb @@ -59,7 +59,7 @@ def test_run_with_cloud_keys_secret_missing end end - def test_run_with_file_missing_section_for_managed_secrets_logs_warning + def test_run_with_file_missing_section_for_ejson_secrets_logs_warning stub_kubectl_response("get", "secret", "ejson-keys", resp: dummy_ejson_secret) new_content = { "_public_key" => fixture_public_key, "not_the_right_key" => [] } @@ -104,10 +104,10 @@ def with_ejson_file(content) end def dummy_ejson_secret(data = correct_ejson_key_secret_data) - dummy_secret_hash(data: data, name: 'ejson-keys', managed: false) + dummy_secret_hash(data: data, name: 'ejson-keys', ejson: false) end - def dummy_secret_hash(name: SecureRandom.hex(4), data: {}, managed: true) + def dummy_secret_hash(name: SecureRandom.hex(4), data: {}, ejson: true) encoded_data = data.each_with_object({}) do |(key, value), encoded| encoded[key] = Base64.strict_encode64(value) end @@ -123,8 +123,8 @@ def dummy_secret_hash(name: SecureRandom.hex(4), data: {}, managed: true) }, "data" => encoded_data, } - if managed - secret['metadata']['annotations'] = { KubernetesDeploy::EjsonSecretProvisioner::MANAGEMENT_ANNOTATION => true } + if ejson + secret['metadata']['annotations'] = { KubernetesDeploy::EjsonSecretProvisioner::EJSON_SECRET_ANNOTATION => true } end secret end @@ -135,7 +135,8 @@ def build_provisioner(dir = nil) namespace: 'test', context: KubeclientHelper::TEST_CONTEXT, template_dir: dir, - logger: logger + logger: logger, + statsd_tags: [] ) end end