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 tls support and tests to connect-init command #459

Merged
merged 7 commits into from
Mar 22, 2021

Conversation

kschoche
Copy link
Contributor

Changes proposed in this PR:

  • Add TLS tests to connect-init/command_test.go
  • Modify the common.GenerateServerCerts function to do it's own cleanup and propogate this to existing tests.
    The reason for this is that it helped to clean up my test so that I didn't have to generate server certs when TLS is disabled in the table test due to scoping of the cleanup func, as well as it generally cleans up existing code.
  • Remove old copy of generateServerCerts from server-acl-init test and convert it to the one in common.
  • In case it isn't obvious connect-init already works with TLS so no actual code is changed. Inside of connect-inject/container_init.go we are passing in the required ENV variables in the command template to enable it to be picked up by the consul client:
const initContainerCommandTpl = `
{{- if .ConsulCACert}}
export CONSUL_HTTP_ADDR="https://${HOST_IP}:8501"
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 }}
EOF
{{- else}}
export CONSUL_HTTP_ADDR="${HOST_IP}:8500"
export CONSUL_GRPC_ADDR="${HOST_IP}:8502"
{{- end}}
consul-k8s connect-init -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \
  {{- if .AuthMethod }}

How I've tested this PR:
Unit tests have been updated to enable TLS and pass.
Manually tested TLS enabled with a deployment.

How I expect reviewers to test this PR:
Unit tests have been updated to enable TLS and pass.
Manually tested TLS enabled with a deployment.

Checklist:

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

@kschoche kschoche added the theme/tproxy Items related to transparent proxy label Mar 19, 2021
@kschoche kschoche self-assigned this Mar 19, 2021
@kschoche kschoche requested review from a team, lkysow and ishustava and removed request for a team March 19, 2021 20:15
@kschoche kschoche marked this pull request as ready for review March 19, 2021 20:15
@kschoche kschoche added the type/enhancement New feature or request label Mar 19, 2021
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.

Nice work! Good catch on the generate certs duplication.

subcommand/connect-init/command_test.go Outdated Show resolved Hide resolved
subcommand/connect-init/command_test.go Outdated Show resolved Hide resolved
subcommand/connect-init/command_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@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 🎉 !! Thanks for adding these tests 🙏

Comment on lines +47 to 50
t.Cleanup(func() {
os.Remove(caFile.Name())
os.Remove(certFile.Name())
os.Remove(certKeyFile.Name())
Copy link
Contributor

Choose a reason for hiding this comment

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

Great! I really like this pattern!

// CA certificate, server certificate, and server key.
// Note that it's the responsibility of the caller to
// remove the temporary files created by this function.
func generateServerCerts(t *testing.T) (string, string, string, func()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nice catch!

@kschoche kschoche merged commit 6b1a0b0 into feature-tproxy Mar 22, 2021
@kschoche kschoche deleted the tproxy_add_tls_support_to_connect_init branch March 22, 2021 22:36
thisisnotashwin pushed a commit that referenced this pull request Mar 26, 2021
* add tls support and tests to connect-init command
thisisnotashwin pushed a commit that referenced this pull request Mar 26, 2021
* add tls support and tests to connect-init command
ishustava pushed a commit that referenced this pull request Apr 14, 2021
* add tls support and tests to connect-init command
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/tproxy Items related to transparent proxy type/enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants