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.

Sign up for GitHub

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

Jump to bottom

auto_encrypt: use server-port instead #6287

Merged
merged 4 commits into from
Aug 23, 2019
Merged

auto_encrypt: use server-port instead #6287

merged 4 commits into from
Aug 23, 2019

Conversation

hanshasselberg
Copy link
Member

AutoEncrypt needs the server port in order to receive the certificates.

@hanshasselberg hanshasselberg requested review from freddygv and a team August 7, 2019 08:42
freddygv
freddygv reviewed Aug 7, 2019
View reviewed changes
Copy link
Contributor

@freddygv freddygv 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 @i0rek, thanks for catching that issue.

One other thing to update is this test file:
https://github.com/hashicorp/consul/blob/beb91cf5d9fc7c3d50f7e70612d630c4c5428d42/agent/consul/auto_encrypt_test.go

The whole test file could probably be removed, since the aim was to check the port-related behavior.

agent/consul/auto_encrypt.go Outdated
@@ -116,27 +115,14 @@ func (c *Client) RequestAutoEncryptCerts(servers []string, defaultPort int, toke

// resolveAddr is used to resolve the host into IPs, port, and error.
// If no port is given, use the default
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment should be updated to remove references to ports, since resolveAddr just returns IPs now.

@freddygv
Copy link
Contributor

freddygv commented Aug 7, 2019 •
edited
Loading

One other question: since we are relying on the error string from net.SplitHostPort, how can we protect ourselves from that error message changing in the future? We could add a test for it, just to make sure it doesn't change without us knowing.

@hanshasselberg
Copy link
Member Author

Thanks for the review @freddygv! I kept the your tests since they are useful even without the port and I added a test for the error message as suggested.

freddygv
freddygv approved these changes Aug 22, 2019
View reviewed changes
Copy link
Contributor

@freddygv freddygv left a comment

Choose a reason for hiding this comment

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

:shipit:

@hanshasselberg hanshasselberg merged commit 3e46352 into master Aug 23, 2019
@hanshasselberg hanshasselberg added the theme/tls Using TLS (Transport Layer Security) or mTLS (mutual TLS) to secure communication label Aug 23, 2019
@hanshasselberg hanshasselberg deleted the auto_encrypt_fix branch August 23, 2019 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@freddygv freddygv freddygv approved these changes

Assignees
No one assigned
Labels
theme/tls Using TLS (Transport Layer Security) or mTLS (mutual TLS) to secure communication
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hanshasselberg @freddygv