Deploy secrets in DeployTask like other resources #424

DazWorrall · 2019-02-25T16:18:16Z

What are you trying to accomplish with this PR?

Have ejson secrets provisioned like any other resource, rather than separately/specially in EjsonSecretProvisioner. This PR also adds support for deploying secrets from yaml templates.

How is this accomplished?

The individual commits tell the story, it might be easier to review this PR by stepping through them individually.

What could go wrong?

A scary-ish change in this PR is adding secrets to the prune whitelist on deploy task. We were pruning secrets before of course, but in a more targeted way. Some questions I have:

Do we still need/want the 'managed secret' annotation?
What additional testing would add confidence this isn't going to break anything in the wild?
Would anyone prefer I separate out the 'sensitive output' refactor?
I flagged secrets for predeployment, on the basis that pods deployed later may need the new versions. Is that sound?

Still tbd after this: support for adding labels from ejson, and resolving how to 'template' ejson secrets. This PR is big enough already.

DazWorrall · 2019-02-25T16:19:42Z

Test failures on CI are due to small output differences across k8s versions, I'll fix those.

benlangfeld · 2019-02-25T16:58:34Z

I had been working on exactly the same change this morning, was nearly done with it too 😅

I flagged secrets for predeployment, on the basis that pods deployed later may need the new versions. Is that sound?

This is a requirement for my use case.

benlangfeld

This is awesome

dturn

Some minor thoughts

lib/kubernetes-deploy/deploy_task.rb

lib/kubernetes-deploy/ejson_secret_provisioner.rb

lib/kubernetes-deploy/deploy_task.rb

benlangfeld · 2019-02-25T20:26:31Z

I need this and #421 quite urgently. Is there anything I can do to help move either forward? I do not have access to buildkite to see what the failures are :/

DazWorrall · 2019-02-26T08:56:16Z

@benlangfeld some context on timelines: I'm actively working on this, but not exclusively, and as I'm separated from my reviewers by a few timezones this will probably take in the order of ~days to complete - not a few hours, but (hopefully) not ~weeks either.

Also, sorry for not communicating more loudly I was doing this before you started hacking yourself.

DazWorrall · 2019-02-26T10:28:04Z

An annoyance to fix later: I couldn't replicate the policial failures locally, so I guess we have some config drift.

lib/kubernetes-deploy/kubernetes_resource.rb

DazWorrall · 2019-02-26T11:09:42Z

This is ready for another pass.

rendhalver · 2019-02-26T13:39:49Z

* Do we still need/want the 'managed secret' annotation?

I can't find a reference to this annotation?
By the name I would assume it would stop a secret deployed by something else from being removed by kubernetes-deploy?
Am I correct?

I would think keeping that annotation is a good idea and would be useful for our setup.

DazWorrall · 2019-02-26T14:32:24Z

I can't find a reference to this annotation?

Secrets created from ejson are currently annotated with kubernetes-deploy.shopify.io/ejson-secret=true.

By the name I would assume it would stop a secret deployed by something else from being removed by kubernetes-deploy? Am I correct?

No, it's used by the current implementation to ensure only 'managed' secrets are pruned, but after this the lifecycle of secrets will be managed like any other resource.

So I guess the answer to my own question is "no it's not needed", but want to make sure.

benlangfeld

This works great for me in a dummy app. I can even deploy Secrets both via EJSON and normal templates in the same deployment. The ejson-keys Secret is not pruned, but others are pruned correctly. Perfect from my perspective.

KnVerey

Do we still need/want the 'managed secret' annotation?

I think it's helpful, and its current text actually still makes sense, but our references to it as a "management" thing should change to avoid confusion for future contributors.

What additional testing would add confidence this isn't going to break anything in the wild?

I don't think there's any additional unit/integration tests we can do unless we decide to make a transitional version (see my first inline comment). Each org is going to need to audit its namespaces for secrets that have been applied. I don't really see a viable way around that requirement other than disabling secret pruning (which I'd rather not do).

Would anyone prefer I separate out the 'sensitive output' refactor?

No, this is fine.

I flagged secrets for predeployment, on the basis that pods deployed later may need the new versions. Is that sound?

Yep!

Still tbd after this: support for adding labels from ejson, and resolving how to 'template' ejson secrets. This PR is big enough already.

Agreed.

lib/kubernetes-deploy/ejson_secret_provisioner.rb

lib/kubernetes-deploy/kubernetes_resource/secret.rb

test/helpers/fixture_set.rb

test/integration/kubernetes_deploy_test.rb

KnVerey · 2019-02-26T23:25:37Z

test/integration/kubernetes_deploy_test.rb

  end

  def test_can_deploy_template_dir_with_only_secrets_ejson
    ejson_cloud = FixtureSetAssertions::EjsonCloud.new(@namespace)
    ejson_cloud.create_ejson_keys_secret
    assert_deploy_success(deploy_fixtures("ejson-cloud", subset: ["secrets.ejson"]))
    assert_logs_match_all([
-      "Deploying kubernetes secrets from secrets.ejson",


Can you add an assertion on the line you replaced this with either here or in one of the main ejson provisioning tests?

test/unit/kubernetes-deploy/kubectl_test.rb

KnVerey · 2019-02-26T23:48:13Z

CHANGELOG.md

@@ -1,5 +1,8 @@
 ## next

+*Features*
+- Support for deploying Secrets from templates ([#424](https://github.com/Shopify/kubernetes-deploy/pull/424)).


I think we also need a flashy "Breaking change" entry here because of the pruning implications and the critical nature of secrets. These are the cases I've thought of so far:

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.

We could actually avoid this one by making a transitional release, but I'm not sure it is justified, since the impact is not deleting something, we're not at 1.0, and we have been maintaining this changelog strictly for folks for a long time.

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

We could potentially make a transitional version log warnings about these, but I kinda doubt people would notice them. Users (including us at Shopify) are going to have to audit their cluster(s) for applied secrets before rolling out this version regardless. ⚠️

If you previously passed secrets manifests to kubernetes-deploy (we would have ¯\_(ツ)_/¯ applied them) and they are no longer in the set you pass to the first deploy using this version, it will delete them

Can you think of any others? One case I think actually doesn't cause trouble is deploying with the new version and then rolling back to deploying with the old (as long as we keep our ejson secret annotation).

Can you think of any others?

I cannot 👍

One case I think actually doesn't cause trouble is deploying with the new version and then rolling back to deploying with the old (as long as we keep our ejson secret annotation).

Which is a solid reason to keep the annotation in itself!

KnVerey · 2019-02-27T00:10:18Z

Did a 🎩 (PRINT_LOGS=1) of some of the tests and have a couple additional thoughts based on the output below.

I think we should:

Remove the first two lines in the box. Seems on the too-verbose side now that secret creation isn't a separate phase.
Either suppress the "Discovering templates" when there are none, or even better, make the ejson secrets just show up in that list (maybe like - Secret/monitoring-token (from ejson))
Search the provisioner for references to "Creation" that need to be changed to "Generation"

benlangfeld · 2019-02-27T07:45:56Z

@DazWorrall I'm addressing some of those code review comments in #425

@benlangfeld

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

* Rebase on master * Fix changelog after rebase * Remove unnecessary logging * Fix unknown Secret status * Update test name to not include 'update' * Write new unrecognized resource test

resources

DazWorrall · 2019-03-01T16:53:34Z

@KnVerey rebased and addressed your feedback. I had to use some unpleasant stubbing to get the tests done but I'm happy we've covered all the known failure states now.

I had to add a couple of serial tests - they proved flaky during CI when ran in parallel, but running in isolation they're fine so there's a race somewhere I don't understand.

@DazWorrall test failures fixed at #435

Sorry I missed this @benlangfeld, I was iterating between meetings and not paying attention to my inbox :( I appreciate the thought!

benlangfeld · 2019-03-01T17:00:03Z

Sorry I missed this @benlangfeld, I was iterating between meetings and not paying attention to my inbox :( I appreciate the thought!

No problem. Just doing whatever I can to get this thing merged.

KnVerey

A couple more small comments, but LGTM.

KnVerey · 2019-03-01T18:12:32Z

test/integration-serial/serial_deploy_test.rb

+    refute_logs_match("kind: Deployment") # content of the sensitive template
+  end
+
+  def test_apply_failure_with_sensitive_resources_hides_raw_output


It is the stubbing that makes the test above concurrency-unfriendly (mocha isn't threadsafe), but this one looks really normal and should be able to run in parallel. What flakes did you see?

These pass for me. aab294c

If that commit fails in CI, might I propose that this be merged with the serial tests in place and we come back to this issue at lower priority? I don't think this should be a condition of merging this feature.

It's ok to let the one with stubbing run serially permanently, but I think this one belongs in the regular file. There's not any follow-up work to do--just trying to get them committed in the right place. 😄

KnVerey · 2019-03-01T18:15:04Z

test/integration-serial/serial_deploy_test.rb

+      secret["type"] = "something/invalid"
+    end
+    assert_deploy_failure(result)
+    refute_logs_match(/Kubectl err:/)


This assertion is too general. There are a handful of other kubectl commands that get run during the test, some of which might fail and be retried. Maybe that was a source of flakiness?

Addressed in 9e13b30

KnVerey · 2019-03-01T18:18:02Z

test/integration-serial/serial_deploy_test.rb

+    logger.level = 0
+    # An invalid PATCH produces the kind of error we want to catch, so first create a valid secret:
+    assert_deploy_success(deploy_fixtures("hello-cloud", subset: %w(secret.yml)))
+    # Then try to PATCH an immutable field


Smart! 👏

KnVerey · 2019-03-01T18:20:50Z

lib/kubernetes-deploy/deploy_task.rb

@@ -302,10 +302,10 @@ def split_templates(filename)
      raise FatalDeploymentError, "Failed to render and parse template"
    end

-    def record_invalid_template(err:, filename:, content:)
+    def record_invalid_template(err:, filename:, content: nil)
      debug_msg = ColorizedString.new("Invalid template: #{filename}\n").red
      debug_msg += "> Error message:\n#{FormattedLogger.indent_four(err)}"


shouldn't we also be suppressing (or replacing) the error itself? Just because it had a filename in it doesn't really tell us what else it contains

Like this? 97a4fd3

KnVerey · 2019-03-01T18:23:17Z

test/integration-serial/serial_deploy_test.rb

+      deployment["spec"]["template"]["spec"]["containers"].first["ports"].first["name"] = bad_port_name
+    end
+    assert_deploy_failure(result)
+    refute_logs_match(/Kubectl err:/)


Same comment as below--need to be more specific with this assertion or it will be flakey

Addressed in 9e13b30

benlangfeld · 2019-03-01T18:30:00Z

@KnVerey If I prepare a PR for those last review comments, would this get merged today?

KnVerey · 2019-03-01T19:05:37Z

@KnVerey If I prepare a PR for those last review comments, would this get merged today?

I'm in a bunch of meetings right now, but I'll do my best

benlangfeld · 2019-03-01T19:58:21Z

@KnVerey Could #438 possibly get run through CI?

dturn

minor stuff, but I'd be ok with this as is

dturn · 2019-03-01T20:26:16Z

README.md

-4. Add the a basic example of the type to the hello-cloud [fixture set](https://github.com/Shopify/kubernetes-deploy/tree/master/test/fixtures/hello-cloud) and appropriate assertions to `#assert_all_up` in [`hello_cloud.rb`](https://github.com/Shopify/kubernetes-deploy/blob/master/test/helpers/fixture_sets/hello_cloud.rb). This will get you coverage in several existing tests, such as `test_full_hello_cloud_set_deploy_succeeds`.
-5. Add tests for any edge cases you foresee.
+4. Add the new class to list of resources in
+   [`deploy_task.rb`](https://github.com/Shopify/kubernetes-deploy/blob/6a0dd662735bbcc0c0cf110d049a08a044a07dd1/lib/kubernetes-deploy/deploy_task.rb#L8)


any reason these don't point to master?

Addressed in #438

any reason these don't point to master?

I used a specific commit so the line numbers don't rot, not a huge deal.

dturn · 2019-03-01T20:38:58Z

lib/kubernetes-deploy/deploy_task.rb

      warn_msg = "WARNING: Any resources not mentioned in the error(s) below were likely created/updated. " \
        "You may wish to roll back this deploy."
      @logger.summary.add_paragraph(ColorizedString.new(warn_msg).yellow)

      unidentified_errors = []
+      sensitive_filenames = resources.select(&:kubectl_output_is_sensitive?).map { |r| File.basename(r.file_path) }


nit: sensitive_filenames -> filenames_with_sensitive_content

Addressed in #438

dturn · 2019-03-01T20:51:56Z

test/helpers/fixture_sets/hello_cloud.rb

@@ -21,6 +21,7 @@ def assert_all_up
      assert_stateful_set_up
      assert_job_up
      assert_network_policy_up
+      assert_secret_created


why _created and not _present

Just to avoid confusing this with the more generic method of the same name in the superclass.

…ew-feedback Secrets as resources: more review feedback

benlangfeld · 2019-03-01T21:57:27Z

Looks like this is ready 💃

benlangfeld · 2019-03-01T22:03:52Z

Thank you to everyone involved. This change is very important for my use case and I'm very grateful for it getting to master ❤️

KnVerey · 2019-03-01T22:05:09Z

@benlangfeld if you need this immediately, can you use a git ref to reference it from master for a few days? I'd really like to include #415 in the next release too; it's very nearly ready but Tim was off today.

benlangfeld · 2019-03-01T22:06:24Z

Absolutely. I also need #421 , but at least I can now rebase that on less of a moving target.

DazWorrall requested review from KnVerey, dturn and timothysmith0609 February 25, 2019 16:18

benlangfeld approved these changes Feb 25, 2019

View reviewed changes

dturn reviewed Feb 25, 2019

View reviewed changes

lib/kubernetes-deploy/deploy_task.rb Outdated Show resolved Hide resolved

lib/kubernetes-deploy/deploy_task.rb Show resolved Hide resolved

lib/kubernetes-deploy/ejson_secret_provisioner.rb Outdated Show resolved Hide resolved

timothysmith0609 reviewed Feb 25, 2019

View reviewed changes

lib/kubernetes-deploy/deploy_task.rb Outdated Show resolved Hide resolved

DazWorrall mentioned this pull request Feb 25, 2019

Secrets should be predeployed before tasks #209

Closed

DazWorrall force-pushed the secrets-as-resources branch 3 times, most recently from 63bd3aa to 9cfa46f Compare February 26, 2019 10:27

DazWorrall commented Feb 26, 2019

View reviewed changes

lib/kubernetes-deploy/kubernetes_resource.rb Show resolved Hide resolved

DazWorrall force-pushed the secrets-as-resources branch 2 times, most recently from df9a98b to 57d3e9a Compare February 26, 2019 10:53

DazWorrall marked this pull request as ready for review February 26, 2019 11:09

benlangfeld approved these changes Feb 26, 2019

View reviewed changes

KnVerey reviewed Feb 26, 2019

View reviewed changes

benlangfeld mentioned this pull request Feb 27, 2019

Secrets as resources review fixes #425

Merged

This was referenced Feb 27, 2019

Support label based namespace partitioning #426

Closed

Apply sorting to overview outputs #261

Closed

DazWorrall force-pushed the secrets-as-resources branch from f73800a to 39e9524 Compare February 27, 2019 11:50

benlangfeld mentioned this pull request Mar 1, 2019

Secrets as resources test fixes #435

Closed

DazWorrall and others added 4 commits March 1, 2019 15:20

Deploy resources from EjsonSecretProvisioner

aa4cc7b

Misc review feedback

5c6bea5

* Rebase on master * Fix changelog after rebase * Remove unnecessary logging * Fix unknown Secret status * Update test name to not include 'update' * Write new unrecognized resource test

Hide some validation and deploy output when deploying sensitive

ade8c51

resources

DazWorrall force-pushed the secrets-as-resources branch from d0e5bd5 to 0581074 Compare March 1, 2019 15:37

Better logging when handling failures with sensitive resources

67bb1b9

DazWorrall force-pushed the secrets-as-resources branch from 0581074 to 67bb1b9 Compare March 1, 2019 16:43

KnVerey approved these changes Mar 1, 2019

View reviewed changes

benlangfeld mentioned this pull request Mar 1, 2019

Pick a name? #30

Closed

benlangfeld added 2 commits March 1, 2019 16:36

Typo

c3acf91

Catch more specific Kubectl error output

9e13b30

dturn approved these changes Mar 1, 2019

View reviewed changes

benlangfeld and others added 4 commits March 1, 2019 18:27

Suppresses error messages on invalid sensitive templates

b7a7026

This is not flaky any more (finer-grained assertion on kubectl error)

eb94bbe

Minor review fixes

16f6411

Merge pull request #438 from powerhome/secrets-as-resources-more-revi…

022a78f

…ew-feedback Secrets as resources: more review feedback

KnVerey merged commit 00e6b55 into master Mar 1, 2019

KnVerey deleted the secrets-as-resources branch March 1, 2019 22:02

timothysmith0609 mentioned this pull request Mar 8, 2019

Never prune ejson-keys #447

Merged

adrianna-chang-shopify mentioned this pull request Aug 28, 2019

Add support for Krane annotations #539

Merged

6 tasks

This pull request was closed.

Deploy secrets in DeployTask like other resources #424

Deploy secrets in DeployTask like other resources #424

Conversation

DazWorrall commented Feb 25, 2019 • edited Loading

DazWorrall commented Feb 25, 2019

benlangfeld commented Feb 25, 2019

benlangfeld left a comment

Choose a reason for hiding this comment

dturn left a comment

Choose a reason for hiding this comment

benlangfeld commented Feb 25, 2019

DazWorrall commented Feb 26, 2019

DazWorrall commented Feb 26, 2019

DazWorrall commented Feb 26, 2019

rendhalver commented Feb 26, 2019

DazWorrall commented Feb 26, 2019 • edited Loading

benlangfeld left a comment

Choose a reason for hiding this comment

KnVerey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KnVerey commented Feb 27, 2019

benlangfeld commented Feb 27, 2019

DazWorrall commented Mar 1, 2019

benlangfeld commented Mar 1, 2019

KnVerey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benlangfeld Mar 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benlangfeld commented Mar 1, 2019

KnVerey commented Mar 1, 2019

benlangfeld commented Mar 1, 2019

dturn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benlangfeld Mar 1, 2019 • edited Loading

Choose a reason for hiding this comment

benlangfeld commented Mar 1, 2019

benlangfeld commented Mar 1, 2019

KnVerey commented Mar 1, 2019

benlangfeld commented Mar 1, 2019

DazWorrall commented Feb 25, 2019 •

edited

Loading

DazWorrall commented Feb 26, 2019 •

edited

Loading

benlangfeld Mar 1, 2019 •

edited

Loading

benlangfeld Mar 1, 2019 •

edited

Loading