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 option for TLS enabled etcd server connections. #5496

Merged
merged 22 commits into from
Dec 13, 2019

Conversation

PrismaPhonic
Copy link
Contributor

This PR adds the option for TLS enabled etcd server connections, and includes a test to confirm that when an etcd instance has been started with key, and cert, that a client setup with the correct TLS info can connect, put a value on a key, and retrieve that value off the key.

dkhenry
dkhenry previously requested changes Dec 3, 2019
go/vt/topo/etcd2topo/server.go Outdated Show resolved Hide resolved
go/vt/topo/etcd2topo/server_test.go Outdated Show resolved Hide resolved
go/vt/topo/etcd2topo/server.go Outdated Show resolved Hide resolved
go/vt/topo/etcd2topo/server.go Outdated Show resolved Hide resolved
go/vt/topo/etcd2topo/server.go Outdated Show resolved Hide resolved
go/vt/topo/etcd2topo/server.go Outdated Show resolved Hide resolved
go/vt/topo/etcd2topo/server_test.go Outdated Show resolved Hide resolved
cmd.Wait()
os.RemoveAll(dataDir)
server.Close()
client.Close()
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't be closing an unexported field from the server. Cleaning up internal resources is the job of the owning object (e.g. this should happen inside server.Close() if necessary).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted, will fix. Is there some equivalent of Drop for golang? (A way to provide clean-up instructions that are called automatically when the GC decides to clean an object up?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

There is a way to do that in Go, but we do not speak of it because it must never be used. You didn't hear this from me. I was never here.

go/vt/topo/etcd2topo/server_test.go Outdated Show resolved Hide resolved
go/vt/topo/etcd2topo/server_test.go Outdated Show resolved Hide resolved
go/vt/topo/etcd2topo/server_test.go Outdated Show resolved Hide resolved
go/vt/topo/etcd2topo/server_test.go Outdated Show resolved Hide resolved
@PrismaPhonic PrismaPhonic force-pushed the feature/tls-enabled-etcd branch 2 times, most recently from d25f623 to 24d7950 Compare December 11, 2019 01:02
Copy link
Member

@enisoc enisoc left a comment

Choose a reason for hiding this comment

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

LGTM

@enisoc enisoc dismissed dkhenry’s stale review December 11, 2019 21:27

comments have been addressed

@enisoc
Copy link
Member

enisoc commented Dec 11, 2019

The unit test is failing:

--- FAIL: TestEtcd2TLS (6.08s)
##[error]    server_test.go:150: newCellClient(https://localhost:6700) failed: dial tcp 127.0.0.1:6700: connect: connection refused
FAIL
FAIL	vitess.io/vitess/go/vt/topo/etcd2topo	9.431s

I think you need to push the clientv3.New() call into the retry loop since the server might not be ready yet. Also it looks like you don't yet Close() this test client.

Signed-off-by: Peter Farr <Peter@PrismaPhonic.com>
Signed-off-by: Peter Farr <Peter@PrismaPhonic.com>
Signed-off-by: Peter Farr <Peter@PrismaPhonic.com>
Signed-off-by: Peter Farr <Peter@PrismaPhonic.com>
Signed-off-by: Peter Farr <Peter@PrismaPhonic.com>
Signed-off-by: Peter Farr <Peter@PrismaPhonic.com>
…sts. Updated server tests to use helper, and verified we are now working with https

Signed-off-by: Peter Farr <Peter@PrismaPhonic.com>
PrismaPhonic and others added 15 commits December 12, 2019 15:22
…iewer.

Signed-off-by: Peter Farr <Peter@PrismaPhonic.com>
Co-Authored-By: Anthony Yeh <enisoc@planetscale.com>
Signed-off-by: Peter Farr <Peter@PrismaPhonic.com>
Signed-off-by: Peter Farr <Peter@PrismaPhonic.com>
…ally by server object when server.close() is called

Signed-off-by: Peter Farr <Peter@PrismaPhonic.com>
Signed-off-by: Peter Farr <Peter@PrismaPhonic.com>
…t of it in tests.

Signed-off-by: Peter Farr <Peter@PrismaPhonic.com>
…se that client at the end of the function that establishes an etcd server for testing.

Signed-off-by: Peter Farr <Peter@PrismaPhonic.com>
Signed-off-by: Peter Farr <Peter@PrismaPhonic.com>
Signed-off-by: Peter Farr <Peter@PrismaPhonic.com>
Signed-off-by: Peter Farr <Peter@PrismaPhonic.com>
Signed-off-by: Peter Farr <Peter@PrismaPhonic.com>
Signed-off-by: Peter Farr <Peter@PrismaPhonic.com>
Signed-off-by: Peter Farr <Peter@PrismaPhonic.com>
Signed-off-by: Peter Farr <Peter@PrismaPhonic.com>
Signed-off-by: Anthony Yeh <enisoc@planetscale.com>
@enisoc enisoc merged commit e140145 into vitessio:master Dec 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants