-
Notifications
You must be signed in to change notification settings - Fork 316
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
Enable TLS for connect-inject #181
Conversation
* Add new flag -consul-ca-cert * Make Consul addresses use HTTPS if CA is provided * Provide CA certificate to the init and both sidecar containers, so that service registration and envoy bootstrapping can use TLS.
export CONSUL_GRPC_ADDR="https://${HOST_IP}:8502" | ||
export CONSUL_CACERT=/consul/connect-inject/consul-ca.pem | ||
cat <<EOF >/consul/connect-inject/consul-ca.pem | ||
{{ .ConsulCACert }} |
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.
Why do this instead of mounting in the same volume in the initContainer?
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.
We talked offline and this makes sense to me now based on the tradeoffs. It's nicer than mounting in a secret and fits with the pattern we're following where everything lives in that directory (like the acl token file)
Co-Authored-By: Luke Kysow <1034429+lkysow@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.
Looks good. I think it's okay to merge after my comments are addressed but lmk if you want another review
connect-inject/lifecycle_sidecar.go
Outdated
@@ -14,6 +14,12 @@ func (h *Handler) lifecycleSidecar(pod *corev1.Pod) corev1.Container { | |||
if h.AuthMethod != "" { | |||
command = append(command, "-token-file=/consul/connect-inject/acl-token") | |||
} | |||
if h.ConsulCACert != "" { | |||
command = append(command, "-http-addr", "https://${HOST_IP}:8501") | |||
command = append(command, "-ca-file", "/consul/connect-inject/consul-ca.pem") |
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.
Lets make this a constant
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.
Hmm ok, I think we should make acl token an env variable too to keep it consistent.
@@ -28,12 +34,6 @@ func (h *Handler) lifecycleSidecar(pod *corev1.Pod) corev1.Container { | |||
FieldRef: &corev1.ObjectFieldSelector{FieldPath: "status.hostIP"}, | |||
}, | |||
}, | |||
// Kubernetes will interpolate HOST_IP when creating this environment |
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.
I'd like to keep that comment, maybe on lines 18 and 21?
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.
I've added this back!
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
) | ||
|
||
func TestHandlerContainerSidecar(t *testing.T) { |
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.
this is now "envoySidecar" I thnk
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.
and this file should be renamed IIRC
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.
yep yep! thank you!
}, nil | ||
} | ||
if h.ConsulCACert != "" { | ||
caCertEnvVar := corev1.EnvVar{ |
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.
I don't personally see the benefit of these extra vars, but personal preference
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.
This is needed so that preStop
command will succeed.
Co-Authored-By: Luke Kysow <1034429+lkysow@users.noreply.github.com>
Co-Authored-By: Luke Kysow <1034429+lkysow@users.noreply.github.com>
Co-Authored-By: Luke Kysow <1034429+lkysow@users.noreply.github.com>
so that service registration and envoy bootstrapping
can use TLS.
This PR has a slightly different approach to what was proposed in #30 and assumes the implementation in the hashicorp/consul-helm#313.
Fixes #79