-
Notifications
You must be signed in to change notification settings - Fork 484
Enable Consul Connect on terraform-aws-consul #173
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The examples, and tests seem reasonable, but I think the approach and docs feel a bit incomplete. I think the key thing missing for me is how the service setup works. Does the proxy and service configuration happen when you run run-consul
? Or are you expected to manually run consul services register
and consul connect proxy
after run-consul
?
Hashing these out into an overview docs on how to configure consul connect for both server and client side would help clarify things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code updates looks good to me! Had one comment regarding the example, but can be punted for now.
…ed on, works as expected
This is ready for review for the Trial project day. There are some concrete next steps here, namely:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for sharing the next steps and the contribution! I had one more nit for the docs, but I think feature wise this is a good, self-contained implementation to kick start our support for Consul Connect.
Note that we can't merge this in until you sign the CLA for Hashicorp, and we will need at least one more review from one of the code owners (@brikis98 or @robmorgan ), but from my side I don't have much to add for this initial version!
Co-authored-by: Yoriyasu Yano <430092+yorinasub17@users.noreply.github.com>
Co-authored-by: Yoriyasu Yano <430092+yorinasub17@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @7hacker!
@@ -530,6 +546,9 @@ function run { | |||
--enable-gossip-encryption) | |||
enable_gossip_encryption="true" | |||
;; | |||
--enable-connect) | |||
enable_connect="true" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here. Declare above as a local
variable and pass it explicitly to other functions that need it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tackeled in #202.
nohup consul connect proxy -sidecar-for foo &>/dev/null & | ||
|
||
# Start a proxy sidecar for service bar | ||
nohup consul connect proxy -sidecar-for bar &>/dev/null & |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have the services actually try to communicate with each other here? E.g., Send a request from one to the other?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no that doesnt happen yet, but should happen along with tests to verify the intentions
@@ -309,6 +313,17 @@ EOF | |||
) | |||
fi | |||
|
|||
local connect_configuration="" | |||
if [[ "$enable_connect" == "true" && "$server" == "true" ]]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
enable_connect
needs to be declared above and passed explicitly into the function. We use local
rather than global variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tackeled in #202.
Co-authored-by: Yevgeniy Brikman <brikis98@users.noreply.github.com>
Co-authored-by: Yevgeniy Brikman <brikis98@users.noreply.github.com>
variable "ssh_key_name" { | ||
description = "The name of an EC2 Key Pair that can be used to SSH to the EC2 Instances in this cluster. Set to an empty string to not associate a Key Pair." | ||
type = string | ||
default = nt-trial |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
set this back to default = null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tackeled in #202.
@7hacker - I would like to jump in and help getting the PR merged. Any support needed? |
Closing this PR due to inactivity. |
This PR addresses issue #165
Design
We add a flag, '--enable-connect' to the run-consul module that turns on Consul Connect while bootstrapping a new cluster. This flag creates the basic key-value pair in the servers configuration (default.json).
To use in production, we suggest overriding the default configuration as specified in the Readme. This enables the production deployments to:
Adopt the best practices in ensuring secure communications, ACL as specified here: https://learn.hashicorp.com/consul/developer-mesh/connect-production
Use an alternative CA provider like Vault or apply specific private_key and root_cert values to the default CA provider by Consul, as specified here: https://learn.hashicorp.com/consul/developer-mesh/connect-production
Use Envoy as a proxy as specified here: https://www.consul.io/docs/connect/proxies/envoy
Examples
We also provide examples showing:
Test
Test include running through the example and verifying: