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

TLS init #410

Merged
merged 8 commits into from Jan 7, 2021
Merged

TLS init #410

merged 8 commits into from Jan 7, 2021

Conversation

thisisnotashwin
Copy link
Contributor

@thisisnotashwin thisisnotashwin commented Dec 16, 2020

Changes proposed in this PR:

  • Create sub command tls-init that manages creating server tls certificates.

The responsibility of the subcommand is as follows:

  • If CA cert/private key as provided as file paths, create a Server TLS cert/key pair and save that as a TLS secret in kubernetes.
  • If a CA isn't provided, create a CA and save the CA certificate and private key as separate Kubernetes secrets. This CA/key will be used in all successive Server certificate/key generations.

How I've tested this PR:

  • Updated helm to use the image ashwinvenkatesh/consul-k8s:tls-init and also updated the TLS-init job to use the consul-k8s binary with the tls-init subcommand. I tested initial TLS cert generation, updating the DNS sans and performing a helm upgrade. This issued new certificate that the consul servers would start using after they were restarted. ran openssl s_client -connect localhost:8501 from within the server pods to validate that they did infact use the new certificated that had updated DNS sans
  • Units tests

How I expect reviewers to test this PR:

  • Unit tests
  • Code review

I will add instructions for how to test the end to end flow on a corresponding helm-PR

Checklist:

  • Tests added
  • CHANGELOG entry added (HashiCorp engineers only, community PRs should not add a changelog entry)

Closes hashicorp/consul-helm#512, Makes hashicorp/consul-helm#719 unnecessary

@thisisnotashwin thisisnotashwin requested review from ishustava, a team and ndhanushkodi and removed request for a team December 18, 2020 19:09
@thisisnotashwin thisisnotashwin added type/enhancement New feature or request theme/tls About running Consul with TLS labels Dec 18, 2020
@thisisnotashwin thisisnotashwin marked this pull request as ready for review December 18, 2020 19:10
CHANGELOG.md Show resolved Hide resolved
Copy link
Member

@ishustava ishustava left a comment

Choose a reason for hiding this comment

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

Wow! Awesome work, Ashwin! I love that it's now a command and how thorough you were with tests. I had some comments about the code and a bunch of edits. I'm still reviewing and would also like to try it out, but I thought I'll leave the comments I have so far.

Thanks for doing this refactor!

CHANGELOG.md Outdated Show resolved Hide resolved
helper/cert/tls_util.go Outdated Show resolved Hide resolved
subcommand/tls-init/command.go Outdated Show resolved Hide resolved
subcommand/tls-init/command.go Outdated Show resolved Hide resolved
subcommand/tls-init/command.go Outdated Show resolved Hide resolved
subcommand/tls-init/command.go Outdated Show resolved Hide resolved
subcommand/tls-init/command.go Show resolved Hide resolved
subcommand/tls-init/command.go Outdated Show resolved Hide resolved
subcommand/tls-init/commant_test.go Outdated Show resolved Hide resolved
subcommand/tls-init/commant_test.go Outdated Show resolved Hide resolved
thisisnotashwin and others added 2 commits December 21, 2020 10:48
Co-authored-by: Iryna Shustava <ishustava@users.noreply.github.com>
- Simplify logic around using kubernetes secret as CA
Copy link
Contributor

@ndhanushkodi ndhanushkodi left a comment

Choose a reason for hiding this comment

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

This is awesome work!! Do you think it would be worth adding a test to make sure the CA isn't recreated if it exists?

subcommand/tls-init/commant_test.go Outdated Show resolved Hide resolved
subcommand/tls-init/commant_test.go Outdated Show resolved Hide resolved
@thisisnotashwin
Copy link
Contributor Author

thisisnotashwin commented Jan 4, 2021

Do you think it would be worth adding a test to make sure the CA isn't recreated if it exists?

@ndhanushkodi I considered this, but I was hoping the tests that use the CA from a secret or from a filepath have assertions that validate that the created server certificates were created using the existing CA, which would fail in case a new CA was created. I could add a test that asserts that explicitly though, but I felt like it was covered by the existing tests.

I was wrong. It does not verify that it used the existing CA cert. Will add that test. Good catch.

Follow up: Added

- Remove redundant assertions.
Copy link
Member

@ishustava ishustava left a comment

Choose a reason for hiding this comment

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

Looks great, Ashwin! I've left some edits and comments, but they are mostly "cosmetical", non-blocking. I've tried it out too, and it works like magic 🧙‍♂️ !

subcommand/tls-init/command.go Outdated Show resolved Hide resolved
subcommand/tls-init/command.go Outdated Show resolved Hide resolved
subcommand/tls-init/command.go Outdated Show resolved Hide resolved
subcommand/tls-init/command.go Outdated Show resolved Hide resolved
subcommand/tls-init/command.go Outdated Show resolved Hide resolved
subcommand/tls-init/command_test.go Outdated Show resolved Hide resolved
subcommand/tls-init/command_test.go Show resolved Hide resolved
subcommand/tls-init/command_test.go Outdated Show resolved Hide resolved
subcommand/tls-init/command.go Show resolved Hide resolved
subcommand/tls-init/command.go Outdated Show resolved Hide resolved
Co-authored-by: Iryna Shustava <ishustava@users.noreply.github.com>
@thisisnotashwin thisisnotashwin merged commit 509717f into master Jan 7, 2021
@thisisnotashwin thisisnotashwin deleted the tls-init branch January 7, 2021 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/tls About running Consul with TLS type/enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Print tls-init errors
5 participants