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

@@ -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))

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)
 

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

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

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
 

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

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,19 +898,19 @@ 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")
     end
     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

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