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

Add get-consul-client-ca command #211

Merged
merged 7 commits into from
Mar 30, 2020
Merged

Conversation

ishustava
Copy link
Contributor

@ishustava ishustava commented Feb 20, 2020

When auto-encrypt is enabled, we need to retrieve Consul client CA from the Consul servers.

This command calls the /agent/connect/ca/roots endpoint, finds the currently active root CA, and writes it to the provided output file location.

This command allows you to provide a cloud-join string instead of the server address and this command will discover the servers. This allows us to re-use the client.join value in the Helm chart without requiring operators to provide the address of the server in addition to the join value.

@ishustava ishustava added the theme/tls About running Consul with TLS label Feb 20, 2020
@ishustava ishustava force-pushed the auto_encrypt/get-consul-client-ca branch 3 times, most recently from 49c4e4a to 354e9d6 Compare March 5, 2020 17:15
@ishustava ishustava marked this pull request as ready for review March 5, 2020 17:15
@ishustava ishustava requested a review from a team March 5, 2020 17:16
Copy link
Contributor

@adilyse adilyse left a comment

Choose a reason for hiding this comment

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

This looks good!

A couple of themes to my comments:

  • Not importing anything from Consul besides the api or sdk. I understand that not all of these might be possible right now, but it might be good to figure out a plan for how to remove them in the future.

  • Adding additional comments for extra context so that the subtleties are clearer.

  • Removing temp files in the tests. I only commented on one specific one, but there seem to be a bunch.

Also, it would be great to have some unit tests for theconsulClient function that addresses all of the different scenarios and checks that the config that is generated matches expectations.

subcommand/get-consul-client-ca/command.go Show resolved Hide resolved
subcommand/get-consul-client-ca/command.go Show resolved Hide resolved
subcommand/get-consul-client-ca/command.go Outdated Show resolved Hide resolved
subcommand/get-consul-client-ca/command.go Outdated Show resolved Hide resolved
subcommand/get-consul-client-ca/command.go Show resolved Hide resolved
subcommand/get-consul-client-ca/command_test.go Outdated Show resolved Hide resolved
subcommand/get-consul-client-ca/command_test.go Outdated Show resolved Hide resolved
subcommand/get-consul-client-ca/command_test.go Outdated Show resolved Hide resolved
subcommand/get-consul-client-ca/command_test.go Outdated Show resolved Hide resolved
subcommand/get-consul-client-ca/command_test.go Outdated Show resolved Hide resolved
Copy link
Member

@lkysow lkysow left a comment

Choose a reason for hiding this comment

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

I was mostly through my review so going to leave it anyway in case it's helpful.

subcommand/lifecycle-sidecar/command_test.go Show resolved Hide resolved
subcommand/get-consul-client-ca/command.go Show resolved Hide resolved
subcommand/get-consul-client-ca/command.go Outdated Show resolved Hide resolved
subcommand/get-consul-client-ca/command.go Outdated Show resolved Hide resolved
subcommand/get-consul-client-ca/command.go Show resolved Hide resolved
subcommand/get-consul-client-ca/command_test.go Outdated Show resolved Hide resolved
subcommand/get-consul-client-ca/command_test.go Outdated Show resolved Hide resolved
subcommand/get-consul-client-ca/command_test.go Outdated Show resolved Hide resolved
@ishustava ishustava force-pushed the auto_encrypt/get-consul-client-ca branch from 2037244 to 76e021f Compare March 12, 2020 17:39
When auto-encrypt is enabled, we need to retrieve
Consul client CA from the Consul servers.

This command calls the '/agent/connect/ca/roots' endpoint,
finds the currently active root CA, and writes it
to the provided output file location.
@ishustava ishustava force-pushed the auto_encrypt/get-consul-client-ca branch from 76e021f to c005a2a Compare March 12, 2020 17:47
@ishustava ishustava force-pushed the auto_encrypt/get-consul-client-ca branch from c005a2a to 9eed6f8 Compare March 12, 2020 18:25
@ishustava
Copy link
Contributor Author

@adilyse @lkysow thanks so much for your reviews, they were super helpful 😄

I've made some changes and refactors that came out of your suggestions that are ready for re-review. Summary here:

  • To remove the dependency on the tlsutil package from Consul, I've refactored the cert package. I've pulled some generic cert generation functions out into its own file called tls-util.go and made functions in source_gen.go call them. This allowed us to re-use them for tests in this command and the server-acl-init command. I've made the changes to the server-acl-init tests in this PR, but happy to pull it out if you think it's better.
  • After adding some tests for what was previously consulClient function's logic, I've found some mistakes in my implementation (thanks Rebecca for the suggestion!). I've changed the server address parsing to essentially better handle different possibilities for the flag value (different combinations of scheme and host or host and port). Generally, these are the cases:
    • If the scheme and port are provided, we don't do anything
    • If scheme without a port is provided, we look at the scheme and use the default port for that scheme
    • If the scheme is not provided, but only the host and port are provided, we use Consul's default scheme (HTTP).
    • If neither scheme or port are provided, we assume HTTPS port and scheme

Copy link
Member

@lkysow lkysow left a comment

Choose a reason for hiding this comment

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

Moving on to actually testing now.

subcommand/get-consul-client-ca/command.go Outdated Show resolved Hide resolved
subcommand/get-consul-client-ca/command.go Outdated Show resolved Hide resolved
subcommand/get-consul-client-ca/command_test.go Outdated Show resolved Hide resolved
subcommand/server-acl-init/command_test.go Show resolved Hide resolved
Co-Authored-By: Luke Kysow <1034429+lkysow@users.noreply.github.com>
Copy link
Member

@lkysow lkysow left a comment

Choose a reason for hiding this comment

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

🎉

@ishustava ishustava merged commit 82e34ba into master Mar 30, 2020
@ishustava ishustava deleted the auto_encrypt/get-consul-client-ca branch March 30, 2020 17:11
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants